[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Y0mJU6IV+Xm8Pu/m@iweiny-desk3>
Date: Fri, 14 Oct 2022 09:07:47 -0700
From: Ira Weiny <ira.weiny@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC: Davidlohr Bueso <dave@...olabs.net>, <dan.j.williams@...el.com>,
<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
<bwidawsk@...nel.org>, <a.manzanares@...sung.com>,
<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>
Subject: Re: [PATCH] cxl: Add generic MSI/MSI-X interrupt support
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@...olabs.net> 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@...olabs.net>
>
> 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);
> >
>
Powered by blists - more mailing lists