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

[Alejandro Lucero Palau <alucerop@amd.com>]

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.