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@xxxxxxxxxxxx>
> ---
> 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
>