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

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

[Gregory Price]

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.

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!

Re: [PATCH 21/22] sfc: create cxl region

[Gregory Price]

This review was generated by kreview-0811365ff2. 

This is not an automated email, the reviewer though this report
looked valid enough to consider discussion.

----
sfc code so won't speak to the internals, but ordering question seems
legit.

~Gregory
----

On Sat, Feb 01, 2026, Alejandro Lucero wrote:

> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 2cfd0a46225f..4d5f3974e51d 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c

[ ... ]

> @@ -147,11 +155,11 @@ void efx_cxl_exit(struct efx_probe_data *probe_data)
>
>  	if (probe_data->cxl->hdm_was_committed) {
>  		iounmap(probe_data->cxl->ctpio_cxl);
> -		cxl_unregister_region(probe_data->cxl->efx_region);
>  	} else {
>  		cxl_dpa_free(probe_data->cxl->cxled);
>  		cxl_put_root_decoder(probe_data->cxl->cxlrd);
>  	}
> +	cxl_unregister_region(probe_data->cxl->efx_region);

In the non-committed path, cxl_dpa_free() is called while the
endpoint decoder is still attached to the region.  Looking at
cxl_dpa_free() in drivers/cxl/core/hdm.c:

    if (cxled->cxld.region) {
        dev_dbg(dev, "decoder assigned to: %s\n",
            dev_name(&cxled->cxld.region->dev));
        return -EBUSY;
    }

Since cxl_unregister_region() has not run yet, cxled->cxld.region
is still set, and cxl_dpa_free() returns -EBUSY without freeing
the DPA.  The return value is not checked.

Should cxl_unregister_region() be called before cxl_dpa_free()
and cxl_put_root_decoder() in the else branch, matching the
reverse order of allocation in efx_cxl_init()?

The cover letter notes that v23 expects committed decoders as the
primary flow, and uncommitted decoder support is deferred to
follow-up work, so this else branch may not be reachable in
practice today.  Still worth fixing the ordering now so it
doesn't bite when the uncommitted path is enabled later.

This issue is not fixed by the remaining commits in the series
(through 10fe989f9e85).

Re: [PATCH 21/22] sfc: create cxl region

[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.
>
> ----
> sfc code so won't speak to the internals, but ordering question seems
> legit.


Hi Gregory,


Yes, it makes sense and pointing out to those changes introduced in v22 
and mainly in v23.

I'll fix it.


Regarding the below comment, which if I am not wrong comes from kreview, 
I think the patchset needs to support both cases and therefore the code 
needs to deal with both module exit paths.


Thank you


>
> ~Gregory
> ----
>
> On Sat, Feb 01, 2026, Alejandro Lucero wrote:
>
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 2cfd0a46225f..4d5f3974e51d 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> [ ... ]
>
>> @@ -147,11 +155,11 @@ void efx_cxl_exit(struct efx_probe_data *probe_data)
>>
>>   	if (probe_data->cxl->hdm_was_committed) {
>>   		iounmap(probe_data->cxl->ctpio_cxl);
>> -		cxl_unregister_region(probe_data->cxl->efx_region);
>>   	} else {
>>   		cxl_dpa_free(probe_data->cxl->cxled);
>>   		cxl_put_root_decoder(probe_data->cxl->cxlrd);
>>   	}
>> +	cxl_unregister_region(probe_data->cxl->efx_region);
> In the non-committed path, cxl_dpa_free() is called while the
> endpoint decoder is still attached to the region.  Looking at
> cxl_dpa_free() in drivers/cxl/core/hdm.c:
>
>      if (cxled->cxld.region) {
>          dev_dbg(dev, "decoder assigned to: %s\n",
>              dev_name(&cxled->cxld.region->dev));
>          return -EBUSY;
>      }
>
> Since cxl_unregister_region() has not run yet, cxled->cxld.region
> is still set, and cxl_dpa_free() returns -EBUSY without freeing
> the DPA.  The return value is not checked.
>
> Should cxl_unregister_region() be called before cxl_dpa_free()
> and cxl_put_root_decoder() in the else branch, matching the
> reverse order of allocation in efx_cxl_init()?
>
> The cover letter notes that v23 expects committed decoders as the
> primary flow, and uncommitted decoder support is deferred to
> follow-up work, so this else branch may not be reachable in
> practice today.  Still worth fixing the ordering now so it
> doesn't bite when the uncommitted path is enabled later.
>
> This issue is not fixed by the remaining commits in the series
> (through 10fe989f9e85).

Re: [PATCH 22/22] sfc: support pio mapping based on cxl

[Gregory Price]

This review was generated by kreview-0811365ff2. 

This is not an automated email, the reviewer though this report
looked valid enough to consider discussion.

----
I am completely unfamiliar with this code, but the question it poses
at least seems reasonable.

~Gregory
----

On Sat, Feb 01, 2026, Alejandro Lucero wrote:

> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index fcec81f862ec..2bb6d3136c7c 100644
> --- a/drivers/net/ethernet/sfc/ef10.c
> +++ b/drivers/net/ethernet/sfc/ef10.c

[ ... ]

> @@ -1263,8 +1281,25 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>  	iounmap(efx->membase);
>  	efx->membase = membase;
>
> -	/* Set up the WC mapping if needed */
> -	if (wc_mem_map_size) {
> +	if (!wc_mem_map_size)
> +		goto skip_pio;
> +
> +	/* Set up the WC mapping */
> +
> +#ifdef CONFIG_SFC_CXL
> +	probe_data = container_of(efx, struct efx_probe_data, efx);
> +	if ((nic_data->datapath_caps3 &
> +	    (1 << MC_CMD_GET_CAPABILITIES_V7_OUT_CXL_CONFIG_ENABLE_LBN)) &&
> +	    probe_data->cxl_pio_initialised) {
> +		/* Using PIO through CXL mapping? */
> +		nic_data->pio_write_base = probe_data->cxl->ctpio_cxl +
> +					   (pio_write_vi_base * efx->vi_stride +
> +					    ER_DZ_TX_PIOBUF - uc_mem_map_size);
> +		probe_data->cxl_pio_in_use = true;
> +	} else
> +#endif
> +	{
> +		/* Using legacy PIO BAR mapping */
>  		nic_data->wc_membase = ioremap_wc(efx->membase_phys +
>  						  uc_mem_map_size,
>  						  wc_mem_map_size);
> @@ -1279,12 +1314,13 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>  			nic_data->wc_membase +
>  			(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>  			 uc_mem_map_size);
> -
> -		rc = efx_ef10_link_piobufs(efx);
> -		if (rc)
> -			efx_ef10_free_piobufs(efx);
>  	}

The CXL path sets nic_data->pio_write_base but does not set
nic_data->pio_write_vi_base, while the legacy path does:

    nic_data->pio_write_vi_base = pio_write_vi_base;

Since nic_data is kzalloc'd, pio_write_vi_base stays at 0 in the CXL
path.  efx_ef10_link_piobufs() then uses nic_data->pio_write_vi_base
to issue MC_CMD_LINK_PIOBUF commands:

    MCDI_SET_DWORD(inbuf, LINK_PIOBUF_IN_TXQ_INSTANCE,
                   nic_data->pio_write_vi_base + index);

and also for the special-case check:

    if (tx_queue->queue == nic_data->pio_write_vi_base) {

Wouldn't this link PIO buffers to incorrect VI instances when using
CXL, since the local variable pio_write_vi_base has the correct
non-zero value but the struct field was never updated?

Re: [PATCH 22/22] sfc: support pio mapping based on cxl

[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.
>
> ----
> I am completely unfamiliar with this code, but the question it poses
> at least seems reasonable.


Yes, and again, it makes sense. We have only tried with one VI, so that 
explains why we have not suffered the issue. But it needs to be fixed.


Thanks!


> ~Gregory
> ----
>
> On Sat, Feb 01, 2026, Alejandro Lucero wrote:
>
>> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
>> index fcec81f862ec..2bb6d3136c7c 100644
>> --- a/drivers/net/ethernet/sfc/ef10.c
>> +++ b/drivers/net/ethernet/sfc/ef10.c
> [ ... ]
>
>> @@ -1263,8 +1281,25 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   	iounmap(efx->membase);
>>   	efx->membase = membase;
>>
>> -	/* Set up the WC mapping if needed */
>> -	if (wc_mem_map_size) {
>> +	if (!wc_mem_map_size)
>> +		goto skip_pio;
>> +
>> +	/* Set up the WC mapping */
>> +
>> +#ifdef CONFIG_SFC_CXL
>> +	probe_data = container_of(efx, struct efx_probe_data, efx);
>> +	if ((nic_data->datapath_caps3 &
>> +	    (1 << MC_CMD_GET_CAPABILITIES_V7_OUT_CXL_CONFIG_ENABLE_LBN)) &&
>> +	    probe_data->cxl_pio_initialised) {
>> +		/* Using PIO through CXL mapping? */
>> +		nic_data->pio_write_base = probe_data->cxl->ctpio_cxl +
>> +					   (pio_write_vi_base * efx->vi_stride +
>> +					    ER_DZ_TX_PIOBUF - uc_mem_map_size);
>> +		probe_data->cxl_pio_in_use = true;
>> +	} else
>> +#endif
>> +	{
>> +		/* Using legacy PIO BAR mapping */
>>   		nic_data->wc_membase = ioremap_wc(efx->membase_phys +
>>   						  uc_mem_map_size,
>>   						  wc_mem_map_size);
>> @@ -1279,12 +1314,13 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   			nic_data->wc_membase +
>>   			(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>>   			 uc_mem_map_size);
>> -
>> -		rc = efx_ef10_link_piobufs(efx);
>> -		if (rc)
>> -			efx_ef10_free_piobufs(efx);
>>   	}
> The CXL path sets nic_data->pio_write_base but does not set
> nic_data->pio_write_vi_base, while the legacy path does:
>
>      nic_data->pio_write_vi_base = pio_write_vi_base;
>
> Since nic_data is kzalloc'd, pio_write_vi_base stays at 0 in the CXL
> path.  efx_ef10_link_piobufs() then uses nic_data->pio_write_vi_base
> to issue MC_CMD_LINK_PIOBUF commands:
>
>      MCDI_SET_DWORD(inbuf, LINK_PIOBUF_IN_TXQ_INSTANCE,
>                     nic_data->pio_write_vi_base + index);
>
> and also for the special-case check:
>
>      if (tx_queue->queue == nic_data->pio_write_vi_base) {
>
> Wouldn't this link PIO buffers to incorrect VI instances when using
> CXL, since the local variable pio_write_vi_base has the correct
> non-zero value but the struct field was never updated?