Messages in this thread | | | Date | Tue, 23 Aug 2022 09:12:08 -0700 | From | Davidlohr Bueso <> | Subject | Re: [RFC PATCH 0/9] CXL: Read and clear event logs |
| |
On Mon, 22 Aug 2022, Ira Weiny wrote:
>Generally it seems ok but I have questions below. > >> One thing I have not >> considered in this is the DOE stuff. > >I think this is the harder thing to support because of needing to allow both >the PCI layer and the CXL layer to create irqs. Potentially at different >times.
I agree.
>> -/* Register Block Identifier (RBI) */ >> -enum cxl_regloc_type { >> - CXL_REGLOC_RBI_EMPTY = 0, >> - CXL_REGLOC_RBI_COMPONENT, >> - CXL_REGLOC_RBI_VIRT, >> - CXL_REGLOC_RBI_MEMDEV, >> - CXL_REGLOC_RBI_TYPES >> -}; > >Why move this?
That was sloppy of me, sorry. I wanted to reuse struct cxlds forward declaration, no idea why that diff formed.
>> - >> static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev, >> struct cxl_register_map *map) >> { >> @@ -75,4 +66,44 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port); >> struct cxl_dev_state; >> int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm); >> void read_cdat_data(struct cxl_port *port); >> + >> +#define CXL_IRQ_CAPABILITY_TABLE \ >> + C(ISOLATION, "isolation", NULL), \ >> + C(PMU, "pmu_overflow", NULL), /* per pmu instance */ \ >> + C(MBOX, "mailbox", NULL), /* primary-only */ \ >> + C(EVENT, "event", NULL), > >This is defining get_max_msgnum to NULL right?
Yes. So untl there are any users everything's a nop.
>> + >> +#undef C >> +#define C(a, b, c) CXL_IRQ_CAPABILITY_##a >> +enum { CXL_IRQ_CAPABILITY_TABLE }; >> +#undef C >> +#define C(a, b, c) { b, c } >> +/** >> + * 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. In cases >> + * where there is only one instance it also indicates which >> + * MSI/MSI-X vector is used for the interrupt message generated >> + * in association with the feature. 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[] = { CXL_IRQ_CAPABILITY_TABLE }; >> +#undef C > >Why all this macro magic?
A nifty trick Dan likes, it avoids duplicating the fields (enums + the table).
>> + >> +/* Register Block Identifier (RBI) */ >> +enum cxl_regloc_type { >> + CXL_REGLOC_RBI_EMPTY = 0, >> + CXL_REGLOC_RBI_COMPONENT, >> + CXL_REGLOC_RBI_VIRT, >> + CXL_REGLOC_RBI_MEMDEV, >> + CXL_REGLOC_RBI_TYPES >> +}; >> + >> #endif /* __CXL_PCI_H__ */ >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c >> index faeb5d9d7a7a..c0fe78e0559b 100644 >> --- a/drivers/cxl/pci.c >> +++ b/drivers/cxl/pci.c >> @@ -387,6 +387,52 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, >> return rc; >> } >> >> +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 */ >> + >> + vectors++; > >This is pretty much what earlier versions of the DOE code did with the >exception of only have 1 get_max_msgnum() calls defined (for DOE). But there >was a lot of debate about how to share vectors with the PCI layer. And >eventually we got rid of it. I'm still trying to figure it out. Sorry for >being slow.
That makes sense, thanks for the explanation. And no not slow, it is _I_ that needs to go re-read the DOE stuff with more attention. But while I knew this was the hardest part, all I really wanted was a basic irq support to add to the bg cmd handling series.
>Perhaps we do this for this series. However, won't we have an issue if we want >to support switch events?
If possible, could you elaborate more on this?
Thanks, Davidlohr
| |