Re: [PATCH v9 06/27] cxl: add function for type2 cxl regs setup

Alejandro Lucero Palau
2025-01-22T09:05:33Z

On 1/21/25 22:51, Dan Williams wrote:
> Alejandro Lucero Palau wrote:
>> On 1/18/25 01:51, Dan Williams wrote:
>>> alejandro.lucero-palau@ wrote:
>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>
>>>> Create a new function for a type2 device initialising
>>>> cxl_dev_state struct regarding cxl regs setup and mapping.
>>>>
>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>>>> Reviewed-by: Fan Ni <fan.ni@samsung.com>
>>>> ---
>>>> drivers/cxl/core/pci.c | 51 ++++++++++++++++++++++++++++++++++++++++++
>>>> include/cxl/cxl.h | 2 ++
>>>> 2 files changed, 53 insertions(+)
>>>>
>>>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>>>> index 5821d582c520..493ab33fe771 100644
>>>> --- a/drivers/cxl/core/pci.c
>>>> +++ b/drivers/cxl/core/pci.c
>>>> @@ -1107,6 +1107,57 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>>> }
>>>> EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
>>>>
>>>> +static int cxl_pci_setup_memdev_regs(struct pci_dev *pdev,
>>>> + struct cxl_dev_state *cxlds)
>>>> +{
>>>> + struct cxl_register_map map;
>>>> + int rc;
>>>> +
>>>> + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map,
>>>> + cxlds->capabilities);
>>>> + /*
>>>> + * This call can return -ENODEV if regs not found. This is not an error
>>>> + * for Type2 since these regs are not mandatory. If they do exist then
>>>> + * mapping them should not fail. If they should exist, it is with driver
>>>> + * calling cxl_pci_check_caps where the problem should be found.
>>>> + */
>>> There is no common definition of type-2 so the core should not try to
>>> assume it knows, or be told what is mandatory. Just export the raw
>>> helpers and leave it to the caller to make these decisions.
>>
>> The code does not know, but it knows it does not know, therefore handles
>> this new situation not needed before Type2 support in the generic code
>> for the pci driver and Type3.
>>
>> This is added to the API for accel drivers following the design
>> restrictions I have commented earlier in another patch. Your suggestion
>> seems to go against that decision what was implicitly taken after the
>> first versions and which had no complains until now.
> Apologies for that, I had not looked at the implications of that general
> decision until now, but the result is going in the wrong direction from
> what it is doing to the core.

After yesterday's meeting listening to Jonathan and you discussing last
reviews, what I thought was mainly related to this patchset, I was not
sure I had to address this concern, but it is clear now.

I'm a bit disappointed this requiring new design after so many cycles
and about something I thought it was set and consensus existed.

Anyway, I'll work on that, not sure yet what I should change and what
should stay, because the main reason for the current design of an accel
driver API does not exist anymore.

I need time for figuring out the work to do, so DCD should take priority
now for trying to merge it with 6.14.

>
>>>> + return 0;
>>>> +
>>>> + if (rc)
>>>> + return rc;
>>>> +
>>>> + return cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>>>> +}
>>>> +
>>>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
>>>> +{
>>>> + int rc;
>>>> +
>>>> + rc = cxl_pci_setup_memdev_regs(pdev, cxlds);
>>>> + if (rc)
>>>> + return rc;
>>>> +
>>>> + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
>>>> + &cxlds->reg_map, cxlds->capabilities);
>>>> + if (rc) {
>>>> + dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>>>> + return rc;
>>>> + }
>>>> +
>>>> + if (!test_bit(CXL_CM_CAP_CAP_ID_RAS, cxlds->capabilities))
>>>> + return rc;
> This is injecting logic in a bitmap and a new CXL core exported ABI just
> to avoid the driver optionally skipping RAS register enumeration.
>
> The core should not care how and whether endpoint drivers (accel or
> cxl_pci) consume register blocks, just arrange for their enumeration and
> let the leaf driver logic take it from there.