[V2,01/11] cxl/pci: Add generic MSI-X/MSI irq support

  From: Davidlohr Bueso <dave@stgolabs.net>

Currently the only CXL features targeted for irq support require their
message numbers to be within the first 16 entries.  The device may
however support less than 16 entries depending on the support it
provides.

Attempt to allocate these 16 irq vectors.  If the device supports less
then the PCI infrastructure will allocate that number.  Store the number
of vectors actually allocated in the device state for later use
by individual functions.

Upon successful allocation, users can plug in their respective isr at
any point thereafter, for example, if the irq setup is not done in the
PCI driver, such as the case of the CXL-PMU.

Cc: Bjorn Helgaas <helgaas@kernel.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Co-developed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

---
Changes from V1:
	Jonathan
		pci_alloc_irq_vectors() cleans up the vectors automatically
		use msi_enabled rather than nr_irq_vecs

Changes from Ira
	Remove reviews
	Allocate up to a static 16 vectors.
	Change cover letter
---
 drivers/cxl/cxlmem.h |  3 +++
 drivers/cxl/cxlpci.h |  6 ++++++
 drivers/cxl/pci.c    | 23 +++++++++++++++++++++++
 3 files changed, 32 insertions(+)
  

On Wed, 30 Nov 2022 16:27:09 -0800
ira.weiny@intel.com wrote:

> From: Davidlohr Bueso <dave@stgolabs.net>
> 
> Currently the only CXL features targeted for irq support require their
> message numbers to be within the first 16 entries.  The device may
> however support less than 16 entries depending on the support it
> provides.
> 
> Attempt to allocate these 16 irq vectors.  If the device supports less
> then the PCI infrastructure will allocate that number.  Store the number
> of vectors actually allocated in the device state for later use
> by individual functions.
> 
> Upon successful allocation, users can plug in their respective isr at
> any point thereafter, for example, if the irq setup is not done in the
> PCI driver, such as the case of the CXL-PMU.
> 
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes from V1:
> 	Jonathan
> 		pci_alloc_irq_vectors() cleans up the vectors automatically
> 		use msi_enabled rather than nr_irq_vecs
> 
> Changes from Ira
> 	Remove reviews
> 	Allocate up to a static 16 vectors.
> 	Change cover letter
> ---
>  drivers/cxl/cxlmem.h |  3 +++
>  drivers/cxl/cxlpci.h |  6 ++++++
>  drivers/cxl/pci.c    | 23 +++++++++++++++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 88e3a8e54b6a..cd35f43fedd4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info {
>   * @info: Cached DVSEC information about the device.
>   * @serial: PCIe Device Serial Number
>   * @doe_mbs: PCI DOE mailbox array
> + * @msi_enabled: MSI-X/MSI has been enabled
>   * @mbox_send: @dev specific transport for transmitting mailbox commands
>   *
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> @@ -247,6 +248,8 @@ struct cxl_dev_state {
>  
>  	struct xarray doe_mbs;
>  
> +	bool msi_enabled;
> +
>  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
>  };
>  
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index eec597dbe763..b7f4e2f417d3 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -53,6 +53,12 @@
>  #define	    CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK			GENMASK(15, 8)
>  #define     CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK		GENMASK(31, 16)
>  
> +/*
> + * NOTE: Currently all the functions which are enabled for CXL require their
> + * vectors to be in the first 16.  Use this as the max.
> + */
> +#define CXL_PCI_REQUIRED_VECTORS 16
> +
>  /* Register Block Identifier (RBI) */
>  enum cxl_regloc_type {
>  	CXL_REGLOC_RBI_EMPTY = 0,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index faeb5d9d7a7a..8f86f85d89c7 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -428,6 +428,27 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>  	}
>  }
>  
> +static void cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> +{
> +	struct device *dev = cxlds->dev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int nvecs;
> +
> +	/*
> +	 * NOTE: pci_alloc_irq_vectors() handles calling pci_free_irq_vectors()
> +	 * automatically despite not being called pcim_*.  See
> +	 * pci_setup_msi_context().
> +	 */
> +	nvecs = pci_alloc_irq_vectors(pdev, 1, CXL_PCI_REQUIRED_VECTORS,
> +				   PCI_IRQ_MSIX | PCI_IRQ_MSI);
> +	if (nvecs < 0) {
> +		dev_dbg(dev, "Failed to alloc irq vectors; use polling instead.\n");
> +		return;
> +	}
> +
> +	cxlds->msi_enabled = true;
> +}
> +
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct cxl_register_map map;
> @@ -494,6 +515,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	cxl_pci_alloc_irq_vectors(cxlds);
> +
>  	cxlmd = devm_cxl_add_memdev(cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
  

On 11/30/2022 5:27 PM, ira.weiny@intel.com wrote:
> From: Davidlohr Bueso <dave@stgolabs.net>
> 
> Currently the only CXL features targeted for irq support require their
> message numbers to be within the first 16 entries.  The device may
> however support less than 16 entries depending on the support it
> provides.
> 
> Attempt to allocate these 16 irq vectors.  If the device supports less
> then the PCI infrastructure will allocate that number.  Store the number
> of vectors actually allocated in the device state for later use
> by individual functions.
> 
> Upon successful allocation, users can plug in their respective isr at
> any point thereafter, for example, if the irq setup is not done in the
> PCI driver, such as the case of the CXL-PMU.
> 
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> 
> ---
> Changes from V1:
> 	Jonathan
> 		pci_alloc_irq_vectors() cleans up the vectors automatically
> 		use msi_enabled rather than nr_irq_vecs
> 
> Changes from Ira
> 	Remove reviews
> 	Allocate up to a static 16 vectors.
> 	Change cover letter
> ---
>   drivers/cxl/cxlmem.h |  3 +++
>   drivers/cxl/cxlpci.h |  6 ++++++
>   drivers/cxl/pci.c    | 23 +++++++++++++++++++++++
>   3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 88e3a8e54b6a..cd35f43fedd4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info {
>    * @info: Cached DVSEC information about the device.
>    * @serial: PCIe Device Serial Number
>    * @doe_mbs: PCI DOE mailbox array
> + * @msi_enabled: MSI-X/MSI has been enabled
>    * @mbox_send: @dev specific transport for transmitting mailbox commands
>    *
>    * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> @@ -247,6 +248,8 @@ struct cxl_dev_state {
>   
>   	struct xarray doe_mbs;
>   
> +	bool msi_enabled;
> +
>   	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
>   };
>   
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index eec597dbe763..b7f4e2f417d3 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -53,6 +53,12 @@
>   #define	    CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK			GENMASK(15, 8)
>   #define     CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK		GENMASK(31, 16)
>   
> +/*
> + * NOTE: Currently all the functions which are enabled for CXL require their
> + * vectors to be in the first 16.  Use this as the max.
> + */
> +#define CXL_PCI_REQUIRED_VECTORS 16
> +
>   /* Register Block Identifier (RBI) */
>   enum cxl_regloc_type {
>   	CXL_REGLOC_RBI_EMPTY = 0,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index faeb5d9d7a7a..8f86f85d89c7 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -428,6 +428,27 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>   	}
>   }
>   
> +static void cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> +{
> +	struct device *dev = cxlds->dev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int nvecs;
> +
> +	/*
> +	 * NOTE: pci_alloc_irq_vectors() handles calling pci_free_irq_vectors()
> +	 * automatically despite not being called pcim_*.  See
> +	 * pci_setup_msi_context().
> +	 */
> +	nvecs = pci_alloc_irq_vectors(pdev, 1, CXL_PCI_REQUIRED_VECTORS,
> +				   PCI_IRQ_MSIX | PCI_IRQ_MSI);
> +	if (nvecs < 0) {
> +		dev_dbg(dev, "Failed to alloc irq vectors; use polling instead.\n");
> +		return;
> +	}
> +
> +	cxlds->msi_enabled = true;
> +}
> +
>   static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   {
>   	struct cxl_register_map map;
> @@ -494,6 +515,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	if (rc)
>   		return rc;
>   
> +	cxl_pci_alloc_irq_vectors(cxlds);
> +
>   	cxlmd = devm_cxl_add_memdev(cxlds);
>   	if (IS_ERR(cxlmd))
>   		return PTR_ERR(cxlmd);
  

ira.weiny@ wrote:
> From: Davidlohr Bueso <dave@stgolabs.net>
> 
> Currently the only CXL features targeted for irq support require their
> message numbers to be within the first 16 entries.  The device may
> however support less than 16 entries depending on the support it
> provides.
> 
> Attempt to allocate these 16 irq vectors.  If the device supports less
> then the PCI infrastructure will allocate that number.

What happens if the device supports 16, but irq-core allocates less? I
believe the answer is with the first user, but this patch does not
include a user.

> Store the number of vectors actually allocated in the device state for
> later use by individual functions.

The patch does not do that.

I know this patch has gone through a lot of discussion, but this
mismatch shows it should really be squashed with the first user because
it does not stand on its own anymore.

> Upon successful allocation, users can plug in their respective isr at
> any point thereafter, for example, if the irq setup is not done in the
> PCI driver, such as the case of the CXL-PMU.
> 
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> 
> ---
> Changes from V1:
> 	Jonathan
> 		pci_alloc_irq_vectors() cleans up the vectors automatically
> 		use msi_enabled rather than nr_irq_vecs
> 
> Changes from Ira
> 	Remove reviews
> 	Allocate up to a static 16 vectors.
> 	Change cover letter
> ---
>  drivers/cxl/cxlmem.h |  3 +++
>  drivers/cxl/cxlpci.h |  6 ++++++
>  drivers/cxl/pci.c    | 23 +++++++++++++++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 88e3a8e54b6a..cd35f43fedd4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info {
>   * @info: Cached DVSEC information about the device.
>   * @serial: PCIe Device Serial Number
>   * @doe_mbs: PCI DOE mailbox array
> + * @msi_enabled: MSI-X/MSI has been enabled
>   * @mbox_send: @dev specific transport for transmitting mailbox commands
>   *
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> @@ -247,6 +248,8 @@ struct cxl_dev_state {
>  
>  	struct xarray doe_mbs;
>  
> +	bool msi_enabled;
> +

This goes unused in this patch and it also duplicates what the core
offers with pdev->{msi,msix}_enabled.

>  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
>  };
>  
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index eec597dbe763..b7f4e2f417d3 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -53,6 +53,12 @@
>  #define	    CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK			GENMASK(15, 8)
>  #define     CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK		GENMASK(31, 16)
>  
> +/*
> + * NOTE: Currently all the functions which are enabled for CXL require their
> + * vectors to be in the first 16.  Use this as the max.
> + */
> +#define CXL_PCI_REQUIRED_VECTORS 16
> +
>  /* Register Block Identifier (RBI) */
>  enum cxl_regloc_type {
>  	CXL_REGLOC_RBI_EMPTY = 0,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index faeb5d9d7a7a..8f86f85d89c7 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -428,6 +428,27 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>  	}
>  }
>  
> +static void cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> +{
> +	struct device *dev = cxlds->dev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int nvecs;
> +
> +	/*
> +	 * NOTE: pci_alloc_irq_vectors() handles calling pci_free_irq_vectors()
> +	 * automatically despite not being called pcim_*.  See
> +	 * pci_setup_msi_context().
> +	 */
> +	nvecs = pci_alloc_irq_vectors(pdev, 1, CXL_PCI_REQUIRED_VECTORS,
> +				   PCI_IRQ_MSIX | PCI_IRQ_MSI);

clang-format would scooch that second line in for you.

Might also be worth a comment for the next person that goes looking for
why this isn't PCI_IRQ_ALL_TYPES.

From CXL 3.0 3.1.1 CXL.io Endpoint:
A Function on a CXL device must not generate INTx messages if that
Function participates in CXL.cache protocol or CXL.mem protocols.


> +	if (nvecs < 0) {
> +		dev_dbg(dev, "Failed to alloc irq vectors; use polling instead.\n");
> +		return;
> +	}
> +
> +	cxlds->msi_enabled = true;
> +}
> +
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct cxl_register_map map;
> @@ -494,6 +515,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	cxl_pci_alloc_irq_vectors(cxlds);
> +
>  	cxlmd = devm_cxl_add_memdev(cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
> -- 
> 2.37.2
>
  

On Thu, Dec 01, 2022 at 04:23:21PM -0800, Dan Williams wrote:
> ira.weiny@ wrote:
> > From: Davidlohr Bueso <dave@stgolabs.net>
> > 
> > Currently the only CXL features targeted for irq support require their
> > message numbers to be within the first 16 entries.  The device may
> > however support less than 16 entries depending on the support it
> > provides.
> > 
> > Attempt to allocate these 16 irq vectors.  If the device supports less
> > then the PCI infrastructure will allocate that number.
> 
> What happens if the device supports 16, but irq-core allocates less? I
> believe the answer is with the first user, but this patch does not
> include a user.
> 
> > Store the number of vectors actually allocated in the device state for
> > later use by individual functions.
> 
> The patch does not do that.

Sorry missed updating this message.

> 
> I know this patch has gone through a lot of discussion, but this
> mismatch shows it should really be squashed with the first user because
> it does not stand on its own anymore.

It is separate because it was Davidlohr's to begin with.

I'll squash it back.

> 
> > Upon successful allocation, users can plug in their respective isr at
> > any point thereafter, for example, if the irq setup is not done in the
> > PCI driver, such as the case of the CXL-PMU.
> > 
> > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > 
> > ---
> > Changes from V1:
> > 	Jonathan
> > 		pci_alloc_irq_vectors() cleans up the vectors automatically
> > 		use msi_enabled rather than nr_irq_vecs
> > 
> > Changes from Ira
> > 	Remove reviews
> > 	Allocate up to a static 16 vectors.
> > 	Change cover letter
> > ---
> >  drivers/cxl/cxlmem.h |  3 +++
> >  drivers/cxl/cxlpci.h |  6 ++++++
> >  drivers/cxl/pci.c    | 23 +++++++++++++++++++++++
> >  3 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 88e3a8e54b6a..cd35f43fedd4 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info {
> >   * @info: Cached DVSEC information about the device.
> >   * @serial: PCIe Device Serial Number
> >   * @doe_mbs: PCI DOE mailbox array
> > + * @msi_enabled: MSI-X/MSI has been enabled
> >   * @mbox_send: @dev specific transport for transmitting mailbox commands
> >   *
> >   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> > @@ -247,6 +248,8 @@ struct cxl_dev_state {
> >  
> >  	struct xarray doe_mbs;
> >  
> > +	bool msi_enabled;
> > +
> 
> This goes unused in this patch and it also duplicates what the core
> offers with pdev->{msi,msix}_enabled.

I tried to argue that with Jonathan and lost.  What I had in V1 was to store
the number actually allocated.  Then if a function reports something higher
later it can't be used.

I admit that at this point I really don't understand PCI interrupts at all.
Every time this patch is discussed I get (what is to me) confusing information.
And I've been unable to discern from the spec how exactly this is all supposed
to work.

> 
> >  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> >  };
> >  
> > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> > index eec597dbe763..b7f4e2f417d3 100644
> > --- a/drivers/cxl/cxlpci.h
> > +++ b/drivers/cxl/cxlpci.h
> > @@ -53,6 +53,12 @@
> >  #define	    CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK			GENMASK(15, 8)
> >  #define     CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK		GENMASK(31, 16)
> >  
> > +/*
> > + * NOTE: Currently all the functions which are enabled for CXL require their
> > + * vectors to be in the first 16.  Use this as the max.
> > + */
> > +#define CXL_PCI_REQUIRED_VECTORS 16
> > +
> >  /* Register Block Identifier (RBI) */
> >  enum cxl_regloc_type {
> >  	CXL_REGLOC_RBI_EMPTY = 0,
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index faeb5d9d7a7a..8f86f85d89c7 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -428,6 +428,27 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
> >  	}
> >  }
> >  
> > +static void cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> > +{
> > +	struct device *dev = cxlds->dev;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int nvecs;
> > +
> > +	/*
> > +	 * NOTE: pci_alloc_irq_vectors() handles calling pci_free_irq_vectors()
> > +	 * automatically despite not being called pcim_*.  See
> > +	 * pci_setup_msi_context().
> > +	 */
> > +	nvecs = pci_alloc_irq_vectors(pdev, 1, CXL_PCI_REQUIRED_VECTORS,
> > +				   PCI_IRQ_MSIX | PCI_IRQ_MSI);
> 
> clang-format would scooch that second line in for you.
> 
> Might also be worth a comment for the next person that goes looking for
> why this isn't PCI_IRQ_ALL_TYPES.
> 
> From CXL 3.0 3.1.1 CXL.io Endpoint:
> A Function on a CXL device must not generate INTx messages if that
> Function participates in CXL.cache protocol or CXL.mem protocols.

Seems reasonable.

Ira

> 
> 
> > +	if (nvecs < 0) {
> > +		dev_dbg(dev, "Failed to alloc irq vectors; use polling instead.\n");
> > +		return;
> > +	}
> > +
> > +	cxlds->msi_enabled = true;
> > +}
> > +
> >  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  {
> >  	struct cxl_register_map map;
> > @@ -494,6 +515,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	if (rc)
> >  		return rc;
> >  
> > +	cxl_pci_alloc_irq_vectors(cxlds);
> > +
> >  	cxlmd = devm_cxl_add_memdev(cxlds);
> >  	if (IS_ERR(cxlmd))
> >  		return PTR_ERR(cxlmd);
> > -- 
> > 2.37.2
> > 
> 
>
  

Ira Weiny wrote:
> On Thu, Dec 01, 2022 at 04:23:21PM -0800, Dan Williams wrote:
> > ira.weiny@ wrote:
> > > From: Davidlohr Bueso <dave@stgolabs.net>
> > > 
> > > Currently the only CXL features targeted for irq support require their
> > > message numbers to be within the first 16 entries.  The device may
> > > however support less than 16 entries depending on the support it
> > > provides.
> > > 
> > > Attempt to allocate these 16 irq vectors.  If the device supports less
> > > then the PCI infrastructure will allocate that number.
> > 
> > What happens if the device supports 16, but irq-core allocates less? I
> > believe the answer is with the first user, but this patch does not
> > include a user.
> > 
> > > Store the number of vectors actually allocated in the device state for
> > > later use by individual functions.
> > 
> > The patch does not do that.
> 
> Sorry missed updating this message.
> 
> > 
> > I know this patch has gone through a lot of discussion, but this
> > mismatch shows it should really be squashed with the first user because
> > it does not stand on its own anymore.
> 
> It is separate because it was Davidlohr's to begin with.
> 
> I'll squash it back.
> 
> > 
> > > Upon successful allocation, users can plug in their respective isr at
> > > any point thereafter, for example, if the irq setup is not done in the
> > > PCI driver, such as the case of the CXL-PMU.
> > > 
> > > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > > 
> > > ---
> > > Changes from V1:
> > > 	Jonathan
> > > 		pci_alloc_irq_vectors() cleans up the vectors automatically
> > > 		use msi_enabled rather than nr_irq_vecs
> > > 
> > > Changes from Ira
> > > 	Remove reviews
> > > 	Allocate up to a static 16 vectors.
> > > 	Change cover letter
> > > ---
> > >  drivers/cxl/cxlmem.h |  3 +++
> > >  drivers/cxl/cxlpci.h |  6 ++++++
> > >  drivers/cxl/pci.c    | 23 +++++++++++++++++++++++
> > >  3 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > index 88e3a8e54b6a..cd35f43fedd4 100644
> > > --- a/drivers/cxl/cxlmem.h
> > > +++ b/drivers/cxl/cxlmem.h
> > > @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info {
> > >   * @info: Cached DVSEC information about the device.
> > >   * @serial: PCIe Device Serial Number
> > >   * @doe_mbs: PCI DOE mailbox array
> > > + * @msi_enabled: MSI-X/MSI has been enabled
> > >   * @mbox_send: @dev specific transport for transmitting mailbox commands
> > >   *
> > >   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> > > @@ -247,6 +248,8 @@ struct cxl_dev_state {
> > >  
> > >  	struct xarray doe_mbs;
> > >  
> > > +	bool msi_enabled;
> > > +
> > 
> > This goes unused in this patch and it also duplicates what the core
> > offers with pdev->{msi,msix}_enabled.
> 
> I tried to argue that with Jonathan and lost.  What I had in V1 was to store
> the number actually allocated.  Then if a function reports something higher
> later it can't be used.

A successful pci_alloc_irq_vectors() call assigns a vector number to all
interrupt sources on the device regardless of how many interrupt sources
there are. If the device has 32 interrupt sources and 16 irqs are returned
from pci_alloc_irq_vectors() then each interrupt source will be sharing
a vector with one or more other vectors. All PCI IRQ vectors are shared.

So I do not see the point of this msi_enabled flag cxl_dev_state. If
pci_alloc_irq_vectors() returns at least 1 then you are good to go.
  

On Thu, 1 Dec 2022 18:00:59 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Ira Weiny wrote:
> > On Thu, Dec 01, 2022 at 04:23:21PM -0800, Dan Williams wrote:  
> > > ira.weiny@ wrote:  
> > > > From: Davidlohr Bueso <dave@stgolabs.net>
> > > > 
> > > > Currently the only CXL features targeted for irq support require their
> > > > message numbers to be within the first 16 entries.  The device may
> > > > however support less than 16 entries depending on the support it
> > > > provides.
> > > > 
> > > > Attempt to allocate these 16 irq vectors.  If the device supports less
> > > > then the PCI infrastructure will allocate that number.  
> > > 
> > > What happens if the device supports 16, but irq-core allocates less? I
> > > believe the answer is with the first user, but this patch does not
> > > include a user.
> > >   
> > > > Store the number of vectors actually allocated in the device state for
> > > > later use by individual functions.  
> > > 
> > > The patch does not do that.  
> > 
> > Sorry missed updating this message.
> >   
> > > 
> > > I know this patch has gone through a lot of discussion, but this
> > > mismatch shows it should really be squashed with the first user because
> > > it does not stand on its own anymore.  
> > 
> > It is separate because it was Davidlohr's to begin with.
> > 
> > I'll squash it back.
> >   
> > >   
> > > > Upon successful allocation, users can plug in their respective isr at
> > > > any point thereafter, for example, if the irq setup is not done in the
> > > > PCI driver, such as the case of the CXL-PMU.
> > > > 
> > > > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > > > 
> > > > ---
> > > > Changes from V1:
> > > > 	Jonathan
> > > > 		pci_alloc_irq_vectors() cleans up the vectors automatically
> > > > 		use msi_enabled rather than nr_irq_vecs
> > > > 
> > > > Changes from Ira
> > > > 	Remove reviews
> > > > 	Allocate up to a static 16 vectors.
> > > > 	Change cover letter
> > > > ---
> > > >  drivers/cxl/cxlmem.h |  3 +++
> > > >  drivers/cxl/cxlpci.h |  6 ++++++
> > > >  drivers/cxl/pci.c    | 23 +++++++++++++++++++++++
> > > >  3 files changed, 32 insertions(+)
> > > > 
> > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > > index 88e3a8e54b6a..cd35f43fedd4 100644
> > > > --- a/drivers/cxl/cxlmem.h
> > > > +++ b/drivers/cxl/cxlmem.h
> > > > @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info {
> > > >   * @info: Cached DVSEC information about the device.
> > > >   * @serial: PCIe Device Serial Number
> > > >   * @doe_mbs: PCI DOE mailbox array
> > > > + * @msi_enabled: MSI-X/MSI has been enabled
> > > >   * @mbox_send: @dev specific transport for transmitting mailbox commands
> > > >   *
> > > >   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> > > > @@ -247,6 +248,8 @@ struct cxl_dev_state {
> > > >  
> > > >  	struct xarray doe_mbs;
> > > >  
> > > > +	bool msi_enabled;
> > > > +  
> > > 
> > > This goes unused in this patch and it also duplicates what the core
> > > offers with pdev->{msi,msix}_enabled.  
> > 
> > I tried to argue that with Jonathan and lost.  What I had in V1 was to store
> > the number actually allocated.  Then if a function reports something higher
> > later it can't be used.  
> 
> A successful pci_alloc_irq_vectors() call assigns a vector number to all
> interrupt sources on the device regardless of how many interrupt sources
> there are. If the device has 32 interrupt sources and 16 irqs are returned
> from pci_alloc_irq_vectors() then each interrupt source will be sharing
> a vector with one or more other vectors. All PCI IRQ vectors are shared.

Assuming my understanding is correct...
Subtle tweak to that description (not that it matters in practice).
Some of the vectors will be shared. For MSI at least it is up to the
device to assign msgnums in whatever way it likes such that they are
fit in the number that were enabled.  So it 'could' put them all on the
first msgnum if it wants to, or put any that would otherwise have been
greater than 16 on msgnum 15.  Impdef how it decides that spread.

MSIX is has a layer of indirection in control of software, so it gets
more complex...



> 
> So I do not see the point of this msi_enabled flag cxl_dev_state. If
> pci_alloc_irq_vectors() returns at least 1 then you are good to go.