Messages in this thread | | | Date | Fri, 14 Oct 2022 09:07:47 -0700 | From | Ira Weiny <> | 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@stgolabs.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@stgolabs.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); > > >
| |