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

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

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.


Thank you


>
>>>
>>> Otherwise this looks good and with those fixed up the Reviewed-by still
>>> stands.
>>
>>
>> No Reviewed-by yet ... this is a new patch after your series removing 
>> previous functionality.
>>
>>
>> Thanks
>>