Re: [PATCH 14/22] cxl: Define a driver interface for DPA allocation

[Alejandro Lucero Palau]

On 2/13/26 16:14, Gregory Price wrote:
> This review was generated by kreview-0811365ff2.
>
> This is not an automated email, the reviewer though this report
> looked valid enough to consider discussion.
>
> ----
>
> Cursory browse, this does look like a legitimate reference leak.
>
> ~Gregory
>
> ----
>
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index a172ce4e9b19..d60a697f12cc 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> [ ... ]
>
> > +static struct cxl_endpoint_decoder *
> > +cxl_find_free_decoder(struct cxl_memdev *cxlmd)
> > +{
> > +	struct cxl_port *endpoint = cxlmd->endpoint;
> > +	struct device *dev;
> > +
> > +	guard(rwsem_read)(&cxl_rwsem.dpa);
> > +	dev = device_find_child(&endpoint->dev, NULL,
> > +				find_free_decoder);
> > +	if (!dev)
> > +		return NULL;
> > +
> > +	return to_cxl_endpoint_decoder(dev);
> > +}
> [ ... ]
>
> > +struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_memdev *cxlmd,
> > +					     enum cxl_partition_mode mode,
> > +					     resource_size_t alloc)
> > +{
> > +	int rc;
> > +
> > +	if (!IS_ALIGNED(alloc, SZ_256M))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	struct cxl_endpoint_decoder *cxled __free(put_cxled) =
> > +		cxl_find_free_decoder(cxlmd);
> > +
> > +	if (!cxled)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	rc = cxl_dpa_set_part(cxled, mode);
> > +	if (rc)
> > +		return ERR_PTR(rc);
> > +
> > +	rc = cxl_dpa_alloc(cxled, alloc);
> > +	if (rc)
> > +		return ERR_PTR(rc);
> > +
> > +	return no_free_ptr(cxled);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_request_dpa, "CXL");
> Does this leak the device reference obtained by device_find_child()
> in cxl_find_free_decoder()?
>
> device_find_child() increments the device refcount (reference A).
> Then cxl_dpa_alloc() -> __cxl_dpa_reserve() calls get_device() on
> the same device (reference B).
>
> On the success path, no_free_ptr() inhibits the put_cxled cleanup,
> so reference A is transferred to the caller.  The matching cleanup
> function cxl_dpa_free() calls __cxl_dpa_release(), which drops
> reference B via put_device(), but reference A is never released:
>
>      cxl_dpa_free()
>        -> devm_cxl_dpa_release()
>          -> __cxl_dpa_release()
>            -> put_device()     /* drops reference B only */
>
>      /* reference A from device_find_child() is still held */
>
> The only caller in this series (sfc efx_cxl_init/efx_cxl_exit)
> never calls put_device() on the returned cxled either, so the
> endpoint decoder device refcount remains permanently elevated.


This is right, and it took a good bunch of time to debug it. Was it 
detected by an automatic tool?


Anyways, I had one patch for solving this which I forgot to apply to v23 
since the focus there was to mainly support the auto-discover region 
which does not go through this path:

+       /* removing the reference from cxl_find_free_decoder ...
+        * when alloc succeds another get happened
+        */
+
+       put_device(&cxled->cxld.dev);


I added that comment because it is not trivial to know if it is right to 
do the put while you get a new reference to the device. I will apply it.

Thanks!