[PATCH v2 0/2] cxl: Add basic MSI/MSI-X support

[PATCH v2 0/2] cxl: Add basic MSI/MSI-X support

[Davidlohr Bueso]

Hi,

Changes from v1 (https://lore.kernel.org/all/20221012180432.473373-1-dave@stgolabs.net/):

- Do not populate the cxl_irq_cap_table. (Jonathan)
- Moved vector allocation further above. (Jonathan)
- Added an irq_type flag, makes sense to have for checking during setup.
- Left no irq support as an error, but mentioned that in the the call is
  in the context of allocation. Otherwise this makes the user handling
  unnecessarily difficult.
- Picked up Dave's review.
- Added a second patch which adds the basic mailbox support (Dave)


Patch 1 contains updates for the generic table handling.

Patch 2 populates the table and adds mailbox support.

Please consider for v6.3.

Thanks!

Davidlohr Bueso (2):
  cxl/pci: Add generic MSI/MSI-X interrupt support
  cxl/mbox: Wire up basic irq support

 drivers/cxl/cxl.h    |  6 +++
 drivers/cxl/cxlmem.h |  2 +
 drivers/cxl/pci.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)

--
2.37.3

[PATCH 1/2] cxl/pci: Add generic MSI/MSI-X interrupt support

[Davidlohr Bueso]

Introduce a generic irq table for CXL components that can have
standard irq support - DOE requires dynamic vector sizing and is
as such is not considered here.

Create an infrastructure to query the max vectors required for the CXL
device. Users can check the irq_type in the device state to figure
if they want to attempt to register a handler for it's specific irq
and deal with it accordingly.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/cxl.h    |  5 ++++
 drivers/cxl/cxlmem.h |  2 ++
 drivers/cxl/pci.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index f680450f0b16..879661702054 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -119,6 +119,11 @@ static inline int ways_to_cxl(unsigned int ways, u8 *iw)
 	return 0;
 }
 
+enum {
+	CXL_IRQ_NONE,
+	CXL_IRQ_MSI,
+};
+
 /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
 #define CXLDEV_CAP_ARRAY_OFFSET 0x0
 #define   CXLDEV_CAP_ARRAY_CAP_ID 0
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 88e3a8e54b6a..ca020767f7fc 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -247,6 +247,8 @@ struct cxl_dev_state {
 
 	struct xarray doe_mbs;
 
+	int irq_type;
+
 	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
 };
 
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index faeb5d9d7a7a..942c4449d30f 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -428,6 +428,67 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
 	}
 }
 
+/**
+ * struct cxl_irq_cap - CXL feature that is capable of receiving MSI/MSI-X irqs.
+ *
+ * @name: Name of the device generating this interrupt.
+ * @get_max_msgnum: Get the feature's largest interrupt message number.  If the
+ *		    feature does not have the Interrupt Supported bit set, then
+ *		    return -1.
+ */
+struct cxl_irq_cap {
+	const char *name;
+	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
+};
+
+static const struct cxl_irq_cap cxl_irq_cap_table[] = { NULL };
+
+static void cxl_pci_free_irq_vectors(void *data)
+{
+	pci_free_irq_vectors(data);
+}
+
+/*
+ * Attempt to allocate the largest amount of necessary vectors.
+ *
+ * Returns 0 upon a successful allocation of *all* vectors, or a
+ * negative value otherwise.
+ */
+static int cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
+{
+	struct device *dev = cxlds->dev;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int rc, i, vectors = -1;
+
+	for (i = 0; i < ARRAY_SIZE(cxl_irq_cap_table); i++) {
+		int irq;
+
+		if (!cxl_irq_cap_table[i].get_max_msgnum)
+			continue;
+
+		irq = cxl_irq_cap_table[i].get_max_msgnum(cxlds);
+		vectors = max_t(int, irq, vectors);
+	}
+
+	if (vectors == -1)
+		return -1;
+
+	vectors++;
+	rc = pci_alloc_irq_vectors(pdev, vectors, vectors,
+				   PCI_IRQ_MSIX | PCI_IRQ_MSI);
+	if (rc < 0)
+		return rc;
+
+	if (rc != vectors) {
+		dev_err(dev, "Not enough interrupts; use polling instead.\n");
+		/* some got allocated, clean them up */
+		cxl_pci_free_irq_vectors(pdev);
+		return -ENOSPC;
+	}
+
+	return devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
+}
+
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct cxl_register_map map;
@@ -478,6 +539,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	devm_cxl_pci_create_doe(cxlds);
 
+	if (!cxl_pci_alloc_irq_vectors(cxlds))
+		cxlds->irq_type = CXL_IRQ_MSI;
+	else
+		cxlds->irq_type = CXL_IRQ_NONE;
+
 	rc = cxl_pci_setup_mailbox(cxlds);
 	if (rc)
 		return rc;
-- 
2.37.3

[PATCH 2/2] cxl/mbox: Wire up basic irq support

[Davidlohr Bueso]

This adds support for mailbox interrupts, which are needed, for
example, for background completion handling.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
Note: We could also handle doorbell irq, but not sure this is
actually needed.

 drivers/cxl/cxl.h |  1 +
 drivers/cxl/pci.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 879661702054..d15a743bfc9e 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -140,6 +140,7 @@ enum {
 /* CXL 2.0 8.2.8.4 Mailbox Registers */
 #define CXLDEV_MBOX_CAPS_OFFSET 0x00
 #define   CXLDEV_MBOX_CAP_PAYLOAD_SIZE_MASK GENMASK(4, 0)
+#define   CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK GENMASK(10, 7)
 #define CXLDEV_MBOX_CTRL_OFFSET 0x04
 #define   CXLDEV_MBOX_CTRL_DOORBELL BIT(0)
 #define CXLDEV_MBOX_CMD_OFFSET 0x08
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 942c4449d30f..6e18ca3e551f 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -51,6 +51,20 @@ static unsigned short mbox_ready_timeout = 60;
 module_param(mbox_ready_timeout, ushort, 0644);
 MODULE_PARM_DESC(mbox_ready_timeout, "seconds to wait for mailbox ready");
 
+static int cxl_pci_mbox_get_max_msgnum(struct cxl_dev_state *cxlds)
+{
+	int cap;
+
+	cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
+	return FIELD_GET(CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK, cap);
+}
+
+static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
+{
+	/* TODO: handle completion of background commands */
+	return IRQ_HANDLED;
+}
+
 static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
 {
 	const unsigned long start = jiffies;
@@ -271,6 +285,15 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 	dev_dbg(cxlds->dev, "Mailbox payload sized %zu",
 		cxlds->payload_size);
 
+	if (cxlds->irq_type == CXL_IRQ_MSI) {
+		struct device *dev = cxlds->dev;
+		int irq = cxl_pci_mbox_get_max_msgnum(cxlds);
+
+		if (devm_request_irq(dev, irq, cxl_pci_mbox_irq,
+				     IRQF_SHARED, "mailbox", cxlds))
+			dev_dbg(dev, "Mailbox irq (%d) supported", irq);
+	}
+
 	return 0;
 }
 
@@ -441,7 +464,9 @@ struct cxl_irq_cap {
 	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
 };
 
-static const struct cxl_irq_cap cxl_irq_cap_table[] = { NULL };
+static const struct cxl_irq_cap cxl_irq_cap_table[] = {
+	{ "mailbox", cxl_pci_mbox_get_max_msgnum }
+};
 
 static void cxl_pci_free_irq_vectors(void *data)
 {
-- 
2.37.3

Re: [PATCH 2/2] cxl/mbox: Wire up basic irq support

[Ira Weiny]

On Fri, Oct 14, 2022 at 12:49:30PM -0700, Davidlohr Bueso wrote:
> This adds support for mailbox interrupts, which are needed, for
> example, for background completion handling.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
> Note: We could also handle doorbell irq, but not sure this is
> actually needed.
> 
>  drivers/cxl/cxl.h |  1 +
>  drivers/cxl/pci.c | 27 ++++++++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 879661702054..d15a743bfc9e 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -140,6 +140,7 @@ enum {
>  /* CXL 2.0 8.2.8.4 Mailbox Registers */
>  #define CXLDEV_MBOX_CAPS_OFFSET 0x00
>  #define   CXLDEV_MBOX_CAP_PAYLOAD_SIZE_MASK GENMASK(4, 0)
> +#define   CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK GENMASK(10, 7)
>  #define CXLDEV_MBOX_CTRL_OFFSET 0x04
>  #define   CXLDEV_MBOX_CTRL_DOORBELL BIT(0)
>  #define CXLDEV_MBOX_CMD_OFFSET 0x08
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 942c4449d30f..6e18ca3e551f 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -51,6 +51,20 @@ static unsigned short mbox_ready_timeout = 60;
>  module_param(mbox_ready_timeout, ushort, 0644);
>  MODULE_PARM_DESC(mbox_ready_timeout, "seconds to wait for mailbox ready");
>  
> +static int cxl_pci_mbox_get_max_msgnum(struct cxl_dev_state *cxlds)
> +{
> +	int cap;
> +
> +	cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> +	return FIELD_GET(CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK, cap);

I'm not a fan of the irq_type in cxlds.

Why doesn't this store the msgnum in cxlds and...

> +}
> +
> +static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
> +{
> +	/* TODO: handle completion of background commands */
> +	return IRQ_HANDLED;
> +}
> +
>  static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
>  {
>  	const unsigned long start = jiffies;
> @@ -271,6 +285,15 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>  	dev_dbg(cxlds->dev, "Mailbox payload sized %zu",
>  		cxlds->payload_size);
>  
> +	if (cxlds->irq_type == CXL_IRQ_MSI) {
> +		struct device *dev = cxlds->dev;
> +		int irq = cxl_pci_mbox_get_max_msgnum(cxlds);

... use the stored msgnum in cxlds here?  ... and use that as a flag if this
should be set up?

> +
> +		if (devm_request_irq(dev, irq, cxl_pci_mbox_irq,

I was using pci_request_irq().

Is devm_request_irq() correct when having allocated the vectors with
pci_alloc_irq_vectors()?

Looking at pci_request_irq() is uses pci_irq_vector() to convert the msgnum to
the irq parameter of request_threaded_irq()?

Ira

> +				     IRQF_SHARED, "mailbox", cxlds))
> +			dev_dbg(dev, "Mailbox irq (%d) supported", irq);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -441,7 +464,9 @@ struct cxl_irq_cap {
>  	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
>  };
>  
> -static const struct cxl_irq_cap cxl_irq_cap_table[] = { NULL };
> +static const struct cxl_irq_cap cxl_irq_cap_table[] = {
> +	{ "mailbox", cxl_pci_mbox_get_max_msgnum }
> +};
>  
>  static void cxl_pci_free_irq_vectors(void *data)
>  {
> -- 
> 2.37.3
> 

Re: [PATCH 1/2] cxl/pci: Add generic MSI/MSI-X interrupt support

[Ira Weiny]

On Fri, Oct 14, 2022 at 12:49:29PM -0700, Davidlohr Bueso wrote:
> Introduce a generic irq table for CXL components that can have
> standard irq support - DOE requires dynamic vector sizing and is
> as such is not considered here.
> 
> Create an infrastructure to query the max vectors required for the CXL
> device. Users can check the irq_type in the device state to figure
> if they want to attempt to register a handler for it's specific irq
> and deal with it accordingly.
> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/cxl.h    |  5 ++++
>  drivers/cxl/cxlmem.h |  2 ++
>  drivers/cxl/pci.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index f680450f0b16..879661702054 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -119,6 +119,11 @@ static inline int ways_to_cxl(unsigned int ways, u8 *iw)
>  	return 0;
>  }
>  
> +enum {
> +	CXL_IRQ_NONE,
> +	CXL_IRQ_MSI,
> +};

I don't recall this being in v1?

Right now do we have any users who will register irq's without having MSI
support?

> +
>  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
>  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
>  #define   CXLDEV_CAP_ARRAY_CAP_ID 0
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 88e3a8e54b6a..ca020767f7fc 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -247,6 +247,8 @@ struct cxl_dev_state {
>  
>  	struct xarray doe_mbs;
>  
> +	int irq_type;
> +
>  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
>  };
>  
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index faeb5d9d7a7a..942c4449d30f 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -428,6 +428,67 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>  	}
>  }
>  
> +/**
> + * struct cxl_irq_cap - CXL feature that is capable of receiving MSI/MSI-X irqs.
> + *
> + * @name: Name of the device generating this interrupt.
> + * @get_max_msgnum: Get the feature's largest interrupt message number.  If the
> + *		    feature does not have the Interrupt Supported bit set, then
> + *		    return -1.
> + */
> +struct cxl_irq_cap {
> +	const char *name;
> +	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
> +};
> +
> +static const struct cxl_irq_cap cxl_irq_cap_table[] = { NULL };
> +
> +static void cxl_pci_free_irq_vectors(void *data)
> +{
> +	pci_free_irq_vectors(data);
> +}
> +
> +/*
> + * Attempt to allocate the largest amount of necessary vectors.
> + *
> + * Returns 0 upon a successful allocation of *all* vectors, or a
> + * negative value otherwise.
> + */
> +static int cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> +{
> +	struct device *dev = cxlds->dev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int rc, i, vectors = -1;
> +
> +	for (i = 0; i < ARRAY_SIZE(cxl_irq_cap_table); i++) {
> +		int irq;
> +
> +		if (!cxl_irq_cap_table[i].get_max_msgnum)
> +			continue;
> +
> +		irq = cxl_irq_cap_table[i].get_max_msgnum(cxlds);
> +		vectors = max_t(int, irq, vectors);
> +	}
> +
> +	if (vectors == -1)
> +		return -1;
> +
> +	vectors++;
> +	rc = pci_alloc_irq_vectors(pdev, vectors, vectors,
> +				   PCI_IRQ_MSIX | PCI_IRQ_MSI);

Yea without PCI_IRQ_LEGACY I don't think we need any communication about which
type of vectors were allocated.

Basically if cxl_pci_alloc_irq_vectors() is successful all users can assume
that at least MSI is available...

For the mailboxes they could key off of the message number being set in cxlds.

> +	if (rc < 0)
> +		return rc;
> +
> +	if (rc != vectors) {
> +		dev_err(dev, "Not enough interrupts; use polling instead.\n");
> +		/* some got allocated, clean them up */
> +		cxl_pci_free_irq_vectors(pdev);
> +		return -ENOSPC;
> +	}
> +
> +	return devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
> +}
> +
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct cxl_register_map map;
> @@ -478,6 +539,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	devm_cxl_pci_create_doe(cxlds);
>  
> +	if (!cxl_pci_alloc_irq_vectors(cxlds))

This can't be here for the event stuff because I need the mailboxes set up to
find out the message numbers for those events.  I had a hell of a time by
accident putting it here.  :-(

> +		cxlds->irq_type = CXL_IRQ_MSI;
> +	else
> +		cxlds->irq_type = CXL_IRQ_NONE;
> +
>  	rc = cxl_pci_setup_mailbox(cxlds);
>  	if (rc)
>  		return rc;

Can't the mailbox irq's be set up after this call?  Mailbox access during set
up is probably fine using polling, right?

Ira

Re: [PATCH 1/2] cxl/pci: Add generic MSI/MSI-X interrupt support

[Davidlohr Bueso]

On Sun, 16 Oct 2022, Ira Weiny wrote:

>> +enum {
>> +	CXL_IRQ_NONE,
>> +	CXL_IRQ_MSI,
>> +};
>
>I don't recall this being in v1?

No, it wasn't. I added it because it was a clean way of doing the irq setup
for each interested party in it's own setup call (such as I do in patch 2).
Jonathan preferred it this way... but per all the below, it seems actually
better to stick with the original plan and do the request_irq for all
interested parties at once, after a succesful call to cxl_pci_alloc_irq_vectors().

>
>Right now do we have any users who will register irq's without having MSI
>support?

We don't, and as you know, the fw interrupts thing is only for events; so
actually if we were to have any kind of flags, I guess a cxlds->has_msi
boolean would do, instead of the enum. But the below voids this I guess.

>
>> +
>>  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
>>  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
>>  #define   CXLDEV_CAP_ARRAY_CAP_ID 0
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index 88e3a8e54b6a..ca020767f7fc 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -247,6 +247,8 @@ struct cxl_dev_state {
>>
>>	struct xarray doe_mbs;
>>
>> +	int irq_type;
>> +
>>	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
>>  };
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index faeb5d9d7a7a..942c4449d30f 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -428,6 +428,67 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>>	}
>>  }
>>
>> +/**
>> + * struct cxl_irq_cap - CXL feature that is capable of receiving MSI/MSI-X irqs.
>> + *
>> + * @name: Name of the device generating this interrupt.
>> + * @get_max_msgnum: Get the feature's largest interrupt message number.  If the
>> + *		    feature does not have the Interrupt Supported bit set, then
>> + *		    return -1.
>> + */
>> +struct cxl_irq_cap {
>> +	const char *name;
>> +	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
>> +};
>> +
>> +static const struct cxl_irq_cap cxl_irq_cap_table[] = { NULL };
>> +
>> +static void cxl_pci_free_irq_vectors(void *data)
>> +{
>> +	pci_free_irq_vectors(data);
>> +}
>> +
>> +/*
>> + * Attempt to allocate the largest amount of necessary vectors.
>> + *
>> + * Returns 0 upon a successful allocation of *all* vectors, or a
>> + * negative value otherwise.
>> + */
>> +static int cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
>> +{
>> +	struct device *dev = cxlds->dev;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	int rc, i, vectors = -1;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cxl_irq_cap_table); i++) {
>> +		int irq;
>> +
>> +		if (!cxl_irq_cap_table[i].get_max_msgnum)
>> +			continue;
>> +
>> +		irq = cxl_irq_cap_table[i].get_max_msgnum(cxlds);
>> +		vectors = max_t(int, irq, vectors);
>> +	}
>> +
>> +	if (vectors == -1)
>> +		return -1;
>> +
>> +	vectors++;
>> +	rc = pci_alloc_irq_vectors(pdev, vectors, vectors,
>> +				   PCI_IRQ_MSIX | PCI_IRQ_MSI);
>
>Yea without PCI_IRQ_LEGACY I don't think we need any communication about which
>type of vectors were allocated.
>
>Basically if cxl_pci_alloc_irq_vectors() is successful all users can assume
>that at least MSI is available...

Agreed, and that's why I added the flag to indicate to the users if the previous
cxl_pci_alloc_irq_vectors() call had been successful or not, basically to avoid
having them unnecessarily attempt to install their isr. But again all this was
because the request_irq() calls were now going to be in each component setup.

>
>For the mailboxes they could key off of the message number being set in cxlds.
>
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	if (rc != vectors) {
>> +		dev_err(dev, "Not enough interrupts; use polling instead.\n");
>> +		/* some got allocated, clean them up */
>> +		cxl_pci_free_irq_vectors(pdev);
>> +		return -ENOSPC;
>> +	}
>> +
>> +	return devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
>> +}
>> +
>>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>  {
>>	struct cxl_register_map map;
>> @@ -478,6 +539,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>
>>	devm_cxl_pci_create_doe(cxlds);
>>
>> +	if (!cxl_pci_alloc_irq_vectors(cxlds))
>
>This can't be here for the event stuff because I need the mailboxes set up to
>find out the message numbers for those events.  I had a hell of a time by
>accident putting it here.  :-(

I'm fine with putting this back down, right before the devm_cxl_add_memdev().

>
>> +		cxlds->irq_type = CXL_IRQ_MSI;
>> +	else
>> +		cxlds->irq_type = CXL_IRQ_NONE;
>> +
>>	rc = cxl_pci_setup_mailbox(cxlds);
>>	if (rc)
>>		return rc;
>
>Can't the mailbox irq's be set up after this call?  Mailbox access during set
>up is probably fine using polling, right?

Again, fine by me. So we'd end up in the original:

if (!cxl_pci_alloc_irq_vectors(cxlds)) {
    cxl_mbox_setup_irq();
    cxl_events_setup_irq();
    cxl_pmu_setup_irq();
}

Thanks,
Davidlohr

Re: [PATCH 1/2] cxl/pci: Add generic MSI/MSI-X interrupt support

[Jonathan Cameron]

On Sun, 16 Oct 2022 17:37:07 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Sun, 16 Oct 2022, Ira Weiny wrote:
> 
> >> +enum {
> >> +	CXL_IRQ_NONE,
> >> +	CXL_IRQ_MSI,
> >> +};  
> >
> >I don't recall this being in v1?  
> 
> No, it wasn't. I added it because it was a clean way of doing the irq setup
> for each interested party in it's own setup call (such as I do in patch 2).
> Jonathan preferred it this way... but per all the below, it seems actually
> better to stick with the original plan and do the request_irq for all
> interested parties at once, after a succesful call to cxl_pci_alloc_irq_vectors().
> 
> >
> >Right now do we have any users who will register irq's without having MSI
> >support?  
> 
> We don't, and as you know, the fw interrupts thing is only for events; so
> actually if we were to have any kind of flags, I guess a cxlds->has_msi
> boolean would do, instead of the enum. But the below voids this I guess.

I still want that bool for the PMUs.  Might not apply everywhere but IRQ
setup for the PMU at least is a job for the cpmu driver, not the pci driver.

Just call it has_int though to avoid the msi/msix naming confusion.
Also, might just be local to function initializing the various other
devices, so passed in as a parameter to those calls so they can do
what they like with it.


> 
> >  
> >> +
> >>  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
> >>  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
> >>  #define   CXLDEV_CAP_ARRAY_CAP_ID 0
> >> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> >> index 88e3a8e54b6a..ca020767f7fc 100644
> >> --- a/drivers/cxl/cxlmem.h
> >> +++ b/drivers/cxl/cxlmem.h
> >> @@ -247,6 +247,8 @@ struct cxl_dev_state {
> >>
> >>	struct xarray doe_mbs;
> >>
> >> +	int irq_type;
> >> +
> >>	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> >>  };
> >>
> >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> >> index faeb5d9d7a7a..942c4449d30f 100644
> >> --- a/drivers/cxl/pci.c
> >> +++ b/drivers/cxl/pci.c
> >> @@ -428,6 +428,67 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
> >>	}
> >>  }
> >>
> >> +/**
> >> + * struct cxl_irq_cap - CXL feature that is capable of receiving MSI/MSI-X irqs.
> >> + *
> >> + * @name: Name of the device generating this interrupt.
> >> + * @get_max_msgnum: Get the feature's largest interrupt message number.  If the
> >> + *		    feature does not have the Interrupt Supported bit set, then
> >> + *		    return -1.
> >> + */
> >> +struct cxl_irq_cap {
> >> +	const char *name;
> >> +	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
> >> +};
> >> +
> >> +static const struct cxl_irq_cap cxl_irq_cap_table[] = { NULL };
> >> +
> >> +static void cxl_pci_free_irq_vectors(void *data)
> >> +{
> >> +	pci_free_irq_vectors(data);
> >> +}
> >> +
> >> +/*
> >> + * Attempt to allocate the largest amount of necessary vectors.
> >> + *
> >> + * Returns 0 upon a successful allocation of *all* vectors, or a
> >> + * negative value otherwise.
> >> + */
> >> +static int cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> >> +{
> >> +	struct device *dev = cxlds->dev;
> >> +	struct pci_dev *pdev = to_pci_dev(dev);
> >> +	int rc, i, vectors = -1;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(cxl_irq_cap_table); i++) {
> >> +		int irq;
> >> +
> >> +		if (!cxl_irq_cap_table[i].get_max_msgnum)
> >> +			continue;
> >> +
> >> +		irq = cxl_irq_cap_table[i].get_max_msgnum(cxlds);
> >> +		vectors = max_t(int, irq, vectors);
> >> +	}
> >> +
> >> +	if (vectors == -1)
> >> +		return -1;
> >> +
> >> +	vectors++;
> >> +	rc = pci_alloc_irq_vectors(pdev, vectors, vectors,
> >> +				   PCI_IRQ_MSIX | PCI_IRQ_MSI);  
> >
> >Yea without PCI_IRQ_LEGACY I don't think we need any communication about which
> >type of vectors were allocated.
> >
> >Basically if cxl_pci_alloc_irq_vectors() is successful all users can assume
> >that at least MSI is available...  
> 
> Agreed, and that's why I added the flag to indicate to the users if the previous
> cxl_pci_alloc_irq_vectors() call had been successful or not, basically to avoid
> having them unnecessarily attempt to install their isr. But again all this was
> because the request_irq() calls were now going to be in each component setup.
> 
> >
> >For the mailboxes they could key off of the message number being set in cxlds.
> >  
> >> +	if (rc < 0)
> >> +		return rc;
> >> +
> >> +	if (rc != vectors) {
> >> +		dev_err(dev, "Not enough interrupts; use polling instead.\n");
> >> +		/* some got allocated, clean them up */
> >> +		cxl_pci_free_irq_vectors(pdev);
> >> +		return -ENOSPC;
> >> +	}
> >> +
> >> +	return devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
> >> +}
> >> +
> >>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>  {
> >>	struct cxl_register_map map;
> >> @@ -478,6 +539,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>
> >>	devm_cxl_pci_create_doe(cxlds);
> >>
> >> +	if (!cxl_pci_alloc_irq_vectors(cxlds))  
> >
> >This can't be here for the event stuff because I need the mailboxes set up to
> >find out the message numbers for those events.  I had a hell of a time by
> >accident putting it here.  :-(  
> 
> I'm fine with putting this back down, right before the devm_cxl_add_memdev().

Ah. I'd missed that subtlety.

> 
> >  
> >> +		cxlds->irq_type = CXL_IRQ_MSI;
> >> +	else
> >> +		cxlds->irq_type = CXL_IRQ_NONE;
> >> +
> >>	rc = cxl_pci_setup_mailbox(cxlds);
> >>	if (rc)
> >>		return rc;  
> >
> >Can't the mailbox irq's be set up after this call?  Mailbox access during set
> >up is probably fine using polling, right?  
> 
> Again, fine by me. So we'd end up in the original:
> 
> if (!cxl_pci_alloc_irq_vectors(cxlds)) {
>     cxl_mbox_setup_irq();
>     cxl_events_setup_irq();
>     cxl_pmu_setup_irq();
> }

For the pmu just pass a flag into the existing setup call, I don't want to
see separate setup of irqs from the rest of the setup.  Not sure how this
works out for the other cases.

Jonathan



> 
> Thanks,
> Davidlohr

Re: [PATCH 2/2] cxl/mbox: Wire up basic irq support

[Jonathan Cameron]

On Sun, 16 Oct 2022 15:06:23 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> On Fri, Oct 14, 2022 at 12:49:30PM -0700, Davidlohr Bueso wrote:
> > This adds support for mailbox interrupts, which are needed, for
> > example, for background completion handling.
> > 
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > ---
...

> > +				     IRQF_SHARED, "mailbox", cxlds))
> > +			dev_dbg(dev, "Mailbox irq (%d) supported", irq);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -441,7 +464,9 @@ struct cxl_irq_cap {
> >  	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
> >  };
> >  
> > -static const struct cxl_irq_cap cxl_irq_cap_table[] = { NULL };
> > +static const struct cxl_irq_cap cxl_irq_cap_table[] = {

Trivial patch mangling point but better to have the NULL on separate line
in previous patch to reduce the diff here.

> > +	{ "mailbox", cxl_pci_mbox_get_max_msgnum }

trailing comma needed as we definitely expect to add more entries!

> > +};
> >  
> >  static void cxl_pci_free_irq_vectors(void *data)
> >  {
> > -- 
> > 2.37.3
> >