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

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

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.


Thanks