Re: [PATCH v9 14/27] sfc: create type2 cxl memdev

Dan Williams
2025-01-18T02:41:28Z

alejandro.lucero-palau@ wrote:
> From: Alejandro Lucero <alucerop@amd.com>
>
> Use cxl API for creating a cxl memory device using the type2
> cxl_dev_state struct.
>
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/net/ethernet/sfc/efx_cxl.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 911f29b91bd3..f4bf137fd878 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -96,10 +96,19 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
> */
> cxl_set_media_ready(cxl->cxlds);

It is unfortunate the media_ready is just being used as an
offline/online flag for memdevs. I would be open to just switching to
typical device online/offline semantics and drop the "media_ready" flag.

>
> + cxl->cxlmd = devm_cxl_add_memdev(&pci_dev->dev, cxl->cxlds);
> + if (IS_ERR(cxl->cxlmd)) {
> + pci_err(pci_dev, "CXL accel memdev creation failed");
> + rc = PTR_ERR(cxl->cxlmd);
> + goto err_memdev;
> + }
> +
> probe_data->cxl = cxl;
>
> return 0;
>
> +err_memdev:
> + cxl_release_resource(cxl->cxlds, CXL_RES_RAM);
> err_resource_set:
> kfree(cxl->cxlds);

In general a function should not mix devm and goto as that is a recipe
for bugs.

The bug I see here is that devm_cxl_add_memdev() runs the teardown flow
*after* efx_pci_probe() returns an error code. That happens in
device_unbind_cleanup(), but when it goes to cleanup endpoint decoders
and anything else that might reference @cxlds it crashes because @cxlds
is long gone.

So if you use devm_cxl_add_memdev() then cxlds must be devm allocated as
well to make sure it gets freed in the proper reverse order.