| Message ID | 20221018030010.20913-3-dave@stgolabs.net |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Add general MSI-X/MSI irq support | expand |
On Mon, 17 Oct 2022 20:00:10 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: > With enough vectors properly allocated, this adds support for > (the primary) mailbox interrupt, which is needed for background > completion handling, beyond polling. > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> Whilst I get the need for an example, I'd rather this didn't go in until we have that background handler in place. Unused infrastructure tends to rot or not be quite what is needed. Follow up comment below. Jonathan > static void cxl_pci_free_irq_vectors(void *data) > @@ -562,6 +588,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > return rc; > > if (!cxl_pci_alloc_irq_vectors(cxlds)) { > + cxl_pci_mbox_irqsetup(cxlds); Ah. That rather wrecks my previous suggestion :) > cxlds->has_irq = true; > } else > cxlds->has_irq = false;
On Tue, 18 Oct 2022, Jonathan Cameron wrote: >Whilst I get the need for an example, I'd rather this didn't >go in until we have that background handler in place. One of the reasons why the bg stuff hasn't been re-posted is because the only user currently is the sanitation work (and hopefully maybe scan media soon), which in turn depends on the cache-flushing thing to be picked up (unless someone starts ranting again against wbinvd). And that is in Dave's pmem security series which I'm hoping will be picked up for v6.3 at some point. So I guess we're a while away from the irq thing. I was mostly suggesting sending an mbox-only just to layout the pci_alloc_irq_vectors if we're not using the common table, but per the above this might not be sooner than the pmu or events stuff... >Unused infrastructure tends to rot or not be quite what is >needed. No arguing there. Thanks, Davidlohr
Davidlohr Bueso wrote: > With enough vectors properly allocated, this adds support for > (the primary) mailbox interrupt, which is needed for background > completion handling, beyond polling. > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > drivers/cxl/cxl.h | 1 + > drivers/cxl/pci.c | 29 ++++++++++++++++++++++++++++- > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index f680450f0b16..13a9743b0012 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -135,6 +135,7 @@ static inline int ways_to_cxl(unsigned int ways, u8 *iw) > /* 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 9c3e95ebaa26..c3f3ee307d7a 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -274,6 +274,32 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds) > return 0; > } > > +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); > +} > + > +static irqreturn_t cxl_pci_mbox_irq(int irq, void *id) > +{ > + /* TODO: handle completion of background commands */ > + return IRQ_HANDLED; > +} > + > +static void cxl_pci_mbox_irqsetup(struct cxl_dev_state *cxlds) > +{ > + struct device *dev = cxlds->dev; > + struct pci_dev *pdev = to_pci_dev(dev); > + int irq; > + > + irq = cxl_pci_mbox_get_max_msgnum(cxlds); In this context cxl_pci_mbox_get_max_msgnum() looks misnamed. There is no max, it's only *the* message number for the mailbox. > + if (!pci_request_irq(pdev, irq, cxl_pci_mbox_irq, NULL, > + cxlds, "%s-mailbox", dev_name(dev))) > + dev_dbg(dev, "Mailbox irq (%d) supported", irq); > +} > + > static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map) > { > void __iomem *addr; > @@ -442,7 +468,7 @@ struct cxl_irq_cap { > }; > > static const struct cxl_irq_cap cxl_irq_cap_table[] = { > - NULL > + { "mailbox", cxl_pci_mbox_get_max_msgnum }, > }; > > static void cxl_pci_free_irq_vectors(void *data) > @@ -562,6 +588,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > return rc; > > if (!cxl_pci_alloc_irq_vectors(cxlds)) { > + cxl_pci_mbox_irqsetup(cxlds); > cxlds->has_irq = true; > } else > cxlds->has_irq = false; > -- > 2.38.0 >
Davidlohr Bueso wrote: > On Tue, 18 Oct 2022, Jonathan Cameron wrote: > > >Whilst I get the need for an example, I'd rather this didn't > >go in until we have that background handler in place. > > One of the reasons why the bg stuff hasn't been re-posted is > because the only user currently is the sanitation work > (and hopefully maybe scan media soon), which in turn depends > on the cache-flushing thing to be picked up (unless someone > starts ranting again against wbinvd). And that is in Dave's > pmem security series which I'm hoping will be picked up for > v6.3 at some point. > > So I guess we're a while away from the irq thing. I was mostly > suggesting sending an mbox-only just to layout the > pci_alloc_irq_vectors if we're not using the common table, > but per the above this might not be sooner than the pmu or > events stuff... > > >Unused infrastructure tends to rot or not be quite what is > >needed. > > No arguing there. I do feel that event notifications are likely the nearest consumer because the whole reason to do the RCD work is to support OS native error handling for these CXL 1.1+ devices that people can actually get their hands on today. Error handling and other RAS flows need to be able to hear the device's screams for help.
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index f680450f0b16..13a9743b0012 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -135,6 +135,7 @@ static inline int ways_to_cxl(unsigned int ways, u8 *iw) /* 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 9c3e95ebaa26..c3f3ee307d7a 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -274,6 +274,32 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds) return 0; } +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); +} + +static irqreturn_t cxl_pci_mbox_irq(int irq, void *id) +{ + /* TODO: handle completion of background commands */ + return IRQ_HANDLED; +} + +static void cxl_pci_mbox_irqsetup(struct cxl_dev_state *cxlds) +{ + struct device *dev = cxlds->dev; + struct pci_dev *pdev = to_pci_dev(dev); + int irq; + + irq = cxl_pci_mbox_get_max_msgnum(cxlds); + if (!pci_request_irq(pdev, irq, cxl_pci_mbox_irq, NULL, + cxlds, "%s-mailbox", dev_name(dev))) + dev_dbg(dev, "Mailbox irq (%d) supported", irq); +} + static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map) { void __iomem *addr; @@ -442,7 +468,7 @@ struct cxl_irq_cap { }; static const struct cxl_irq_cap cxl_irq_cap_table[] = { - NULL + { "mailbox", cxl_pci_mbox_get_max_msgnum }, }; static void cxl_pci_free_irq_vectors(void *data) @@ -562,6 +588,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) return rc; if (!cxl_pci_alloc_irq_vectors(cxlds)) { + cxl_pci_mbox_irqsetup(cxlds); cxlds->has_irq = true; } else cxlds->has_irq = false;
With enough vectors properly allocated, this adds support for (the primary) mailbox interrupt, which is needed for background completion handling, beyond polling. Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- drivers/cxl/cxl.h | 1 + drivers/cxl/pci.c | 29 ++++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-)