lkml.org 
[lkml]   [2022]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-08-23 20:16    [W:2.050 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site