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

[Ira Weiny]

On Thu, Oct 13, 2022 at 01:19:13PM +0100, Jonathan Cameron wrote:
> On Wed, 12 Oct 2022 11:04:32 -0700
> Davidlohr Bueso <dave@xxxxxxxxxxxx> wrote:
> 
> > Introduce a generic irq table for CXL components/features that can have
> > standard irq support - DOE requires dynamic vector sizing and is not
> > considered here.
> > 
> > Create an infrastructure to query the max vectors required for the CXL
> > device.
> > 
> > Signed-off-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> 
> Hi Davidlohr,
> 
> Basically good, but a few comments inline.
> 
> I'll role this onto front of the v2 of the CPMU set as well.

And I don't mind this landing ahead of the event stuff.  I'll take this in my
series too but expect it to drop out when applied.

Ira

> 
> > ---
> >  drivers/cxl/pci.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> > 
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index faeb5d9d7a7a..467f2d568e3e 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -428,6 +428,66 @@ 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);
> 
> For the CPMU case I need to walk the register locator dvsec block so need
> the callback to take the pci_dev not the cxl_dev_state.
> 
> Also need it later to map the resulting register blocks to go find the irq before
> then dropping them mappings so that the resulting CPMU device can grab them
> later.
> 
> > +};
> > +
> > +static const struct cxl_irq_cap cxl_irq_cap_table[] = {
> > +	{ "isolation", NULL },
> > +	{ "pmu_overflow", NULL },
> > +	{ "mailbox", NULL },
> > +	{ "event", NULL },
> 
> Fill these in as we provide them, not upfront. I'd rather see this
> attached to one (or possibly several) of the series that are coming along
> than stand alone.  so start off with an empty table.
> 
> 
> 
> > +};
> > +
> > +static void cxl_pci_free_irq_vectors(void *data)
> > +{
> > +	pci_free_irq_vectors(data);
> > +}
> > +
> > +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 -EINVAL; /* no irq support whatsoever */
> 
> return 0 in this case.  No irqs present is a 'good' result if there
> aren't any.  Will be up to the consumers of the interrupts to get
> their own interrupt vector numbers and they should get the same
> answers!
> 
> > +
> > +	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 where supported\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;
> > @@ -498,6 +558,9 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	if (IS_ERR(cxlmd))
> >  		return PTR_ERR(cxlmd);
> >  
> > +	/* TODO: When there are users, this return value must be checked */
> > +	cxl_pci_alloc_irq_vectors(cxlds);
> > +
> 
> Gut feeling is this will end up moving ahead of any of the sub device creation
> because many of them end up needing interrupts.
> 
> Also check response from the start - can't see a reason to not do so as we
> won't be registering any at all if no callbacks provided.
> 
> So I'd move it above the devm_cxl_add_memdev() call.
> 
> 
> 
> >  	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM))
> >  		rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
> >  
>