Re: [PATCH v27 4/5] sfc: obtain and map cxl range using devm_cxl_probe_mem

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

On 6/17/26 10:42, Alejandro Lucero Palau wrote:
>
> On 6/16/26 20:51, Dan Williams (nvidia) wrote:
>> Alejandro Lucero Palau wrote:
>>> On 6/10/26 14:56, Alejandro Lucero Palau wrote:
>>>> On 6/10/26 07:10, Alejandro Lucero Palau wrote:
>>>>> On 6/10/26 00:30, Dan Williams (nvidia) wrote:
>>>>>> alejandro.lucero-palau@ wrote:
>>>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>>>
>>>>>>> Use core API for safely obtain the CXL range linked to an HDM
>>>>>>> committed
>>>>>>> by the BIOS. Map such a range for being used as the ctpio buffer.
>>>>>>>
>>>>>>> A potential user space action through sysfs unbinding or core cxl
>>>>>>> modules remove will trigger sfc driver device detachment, with that
>>>>>>> case
>>>>>>> not racing with this mapping as this is done during driver probe 
>>>>>>> and
>>>>>>> therefore protected with device lock against those user space 
>>>>>>> actions.
>>>>>>>
>>>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>>>>> ---
>>>>>>>    drivers/net/ethernet/sfc/efx.c     |  1 +
>>>>>>>    drivers/net/ethernet/sfc/efx_cxl.c | 24 ++++++++++++++++++++++++
>>>>>>>    drivers/net/ethernet/sfc/efx_cxl.h |  3 +++
>>>>>>>    3 files changed, 28 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/net/ethernet/sfc/efx.c
>>>>>>> b/drivers/net/ethernet/sfc/efx.c
>>>>>>> index 90ccbe310386..578054c21e79 100644
>>>>>>> --- a/drivers/net/ethernet/sfc/efx.c
>>>>>>> +++ b/drivers/net/ethernet/sfc/efx.c
>>>>>>> @@ -984,6 +984,7 @@ static void efx_pci_remove(struct pci_dev
>>>>>>> *pci_dev)
>>>>>>>        efx_fini_io(efx);
>>>>>>>          probe_data = container_of(efx, struct efx_probe_data, 
>>>>>>> efx);
>>>>>>> +    efx_cxl_exit(probe_data);
>>>>>>>          pci_dbg(efx->pci_dev, "shutdown successful\n");
>>>>>>>    diff --git a/drivers/net/ethernet/sfc/efx_cxl.c
>>>>>>> b/drivers/net/ethernet/sfc/efx_cxl.c
>>>>>>> index 4d55c08cf2a1..d5766a40e2cf 100644
>>>>>>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>>>>>>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>>>>>>> @@ -18,6 +18,7 @@ int efx_cxl_init(struct efx_probe_data 
>>>>>>> *probe_data)
>>>>>>>    {
>>>>>>>        struct efx_nic *efx = &probe_data->efx;
>>>>>>>        struct pci_dev *pci_dev = efx->pci_dev;
>>>>>>> +    struct range cxl_pio_range;
>>>>>>>        struct efx_cxl *cxl;
>>>>>>>        u16 dvsec;
>>>>>>>        int rc;
>>>>>>> @@ -75,9 +76,32 @@ int efx_cxl_init(struct efx_probe_data 
>>>>>>> *probe_data)
>>>>>>>            return -ENODEV;
>>>>>>>        }
>>>>>>>    +    cxl->cxlmd = devm_cxl_probe_mem(&cxl->cxlds, 
>>>>>>> &cxl_pio_range);
>>>>>>> +    if (IS_ERR(cxl->cxlmd)) {
>>>>>>> +        pci_err(pci_dev, "CXL accel memdev creation failed\n");
>>>>>>> +        return PTR_ERR(cxl->cxlmd);
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    cxl->ctpio_cxl = ioremap_wc(cxl_pio_range.start,
>>>>>>> +                    range_len(&cxl_pio_range));
>>>>>>> +    if (!cxl->ctpio_cxl) {
>>>>>>> +        pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
>>>>>>> +            &cxl_pio_range);
>>>>>>> +        return -ENOMEM;
>>>>>> Dave caught the iounmap leak, but another concern is since you 
>>>>>> want to
>>>>>> continue operation if efx_cxl_init() fails then you probably also 
>>>>>> want
>>>>>> to release the successful attachment to the CXL domain if this 
>>>>>> happens.
>>>>>
>>>>> I will do that.
>>>>>
>>>> Looking at this issue, I think an error when creating the memdev or
>>>> during the region attach triggers the memdev removal, but ...
>>>>
>>>>
>>>>>> Minor since something else is likely to fail if ioremap is not
>>>>>> reliable.
>>>>
>>>> .. if we want to specifically do that with an unlikely (but possible)
>>>> ioremap error something else needs to be exported like
>>>> cxl_memdev_unregister(). Are you happy with that approach?
>>>>
>>> I have just tested with this:
>>>
>>> +void cxl_memdev_remove(void *_cxlmd)
>>> +{
>>> +       struct cxl_memdev *cxlmd = _cxlmd;
>>> +       struct device *dev = &cxlmd->dev;
>>> +
>>> +       devm_remove_action_nowarn(cxlmd->cxlds->dev, 
>>> cxl_memdev_unregister,
>>> +                                 cxlmd);
>>> +
>>> +       cdev_device_del(&cxlmd->cdev, dev);
>>> +       cxl_memdev_shutdown(dev);
>>> +       put_device(dev);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_memdev_remove, "CXL");
>>>
>>>
>>> only called if the ioremap fails.
>>>
>>>
>>> Please, let me know if you like this approach before sending another
>>> version.
>> A devres group can automatically cleanup after devm_cxl_memdev_probe()
>> in the error path with no new exports needed from the CXL core.
>> Something like:
>>
>>          void *group = devres_open_group(cxl->cxlds.dev, NULL, 
>> GFP_KERNEL);
>>          int rc = 0;
>>
>>          if (!group)
>>                  return -ENOMEM;
>>                   cxl->cxlmd = devm_cxl_probe_mem(&cxl->cxlds, 
>> &cxl_pio_range);
>>          if (IS_ERR(cxl->cxlmd)) {
>>                  pci_err(pci_dev, "CXL accel memdev creation failed\n");
>>                  rc = PTR_ERR(cxl->cxlmd);
>>                  goto out;
>>          }
>>
>>          cxl->ctpio_cxl =
>>                  ioremap_wc(cxl_pio_range.start, 
>> range_len(&cxl_pio_range));
>>          if (!cxl->ctpio_cxl) {
>>                  pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
>>                          &cxl_pio_range);
>>                  rc = -ENOMEM;
>>          }
>>
>> out:
>>          if (rc)
>>                  devres_release_group(group);
>>          else
>>                  devres_remove_group(group);
>>          return rc;
>
>
> OK. I will use this in v28 instead of that export.
>

Adding this implies the full driver is detached from the device. The 
same can be obtained changing the check of efx_cxl_init() call and make 
the probe to fail if an error is returned.


I prefer to do this after discussing this internally. After all, if 
CXL.mem is there and the related initialization fails, this is not 
expected and it should be fixed.


Sending v28 shortly.



>
> Thanks
>