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

["Dan Williams (nvidia)" <djbw@kernel.org>]

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;