| Message ID | 20221014194930.2630416-2-dave@stgolabs.net |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Add basic MSI/MSI-X support | expand |
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
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
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
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;