Re: [PATCH v3 12/20] efx: use acquire_endpoint when looking for free HPA

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

On 9/13/24 00:09, Dave Jiang wrote:
>
> On 9/7/24 1:18 AM, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Asking for availbale HPA space is the previous step to try to obtain
>> an HPA range suitable to accel driver purposes.
>>
>> Add this call to efx cxl initialization and use acquire_endpoint for
>> avoiding potential races with cxl port creation.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/net/ethernet/sfc/efx.c     |  8 +++++++-
>>   drivers/net/ethernet/sfc/efx_cxl.c | 32 ++++++++++++++++++++++++++++++
>>   2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
>> index 3a7406aa950c..08a2f527df16 100644
>> --- a/drivers/net/ethernet/sfc/efx.c
>> +++ b/drivers/net/ethernet/sfc/efx.c
>> @@ -1117,10 +1117,16 @@ static int efx_pci_probe(struct pci_dev *pci_dev,
>>   	 * used for PIO buffers. If there is no CXL support, or initialization
>>   	 * fails, efx_cxl_pio_initialised wll be false and legacy PIO buffers
>>   	 * defined at specific PCI BAR regions will be used.
>> +	 *
>> +	 * The only error to handle is -EPROBE_DEFER happening if the root port
>> +	 * is not there yet.
>>   	 */
>>   	rc = efx_cxl_init(efx);
>> -	if (rc)
>> +	if (rc) {
>> +		if (rc == -EPROBE_DEFER)
>> +			goto fail2;
>>   		pci_err(pci_dev, "CXL initialization failed with error %d\n", rc);
>> +	}
>>   
>>   	rc = efx_pci_probe_post_io(efx);
>>   	if (rc) {
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 899bc823a212..826759caa552 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -23,6 +23,7 @@ int efx_cxl_init(struct efx_nic *efx)
>>   	struct pci_dev *pci_dev = efx->pci_dev;
>>   	struct efx_cxl *cxl;
>>   	struct resource res;
>> +	resource_size_t max;
>>   	u16 dvsec;
>>   	int rc;
>>   
>> @@ -90,7 +91,38 @@ int efx_cxl_init(struct efx_nic *efx)
>>   		goto err;
>>   	}
>>   
>> +	cxl->endpoint = cxl_acquire_endpoint(cxl->cxlmd);
>> +	if (IS_ERR(cxl->endpoint)) {
>> +		rc = PTR_ERR(cxl->endpoint);
>> +		if (rc != -EPROBE_DEFER) {
>> +			pci_err(pci_dev, "CXL accel acquire endpoint failed");
>> +			goto err;
>> +		}
> What happens if (rc == -EPROBE_DEFER)? Here it drops down but you don't have a valid cxl->endpoint when cxl_get_hpa_freespace() is called.
>
> DJ
>   


I missed it!

I thought about implementing a test case for this situation what would 
have shown the bug ...

FWIW, the caller is checking that specific return.

Thanks!


>> +	}
>> +
>> +	cxl->cxlrd = cxl_get_hpa_freespace(cxl->endpoint,
>> +					   CXL_DECODER_F_RAM | CXL_DECODER_F_TYPE2,
>> +					   &max);
>> +
>> +	if (IS_ERR(cxl->cxlrd)) {
>> +		pci_err(pci_dev, "cxl_get_hpa_freespace failed\n");
>> +		rc = PTR_ERR(cxl->cxlrd);
>> +		goto err_release;
>> +	}
>> +
>> +	if (max < EFX_CTPIO_BUFFER_SIZE) {
>> +		pci_err(pci_dev, "%s: no enough free HPA space %llu < %u\n",
>> +			__func__, max, EFX_CTPIO_BUFFER_SIZE);
>> +		rc = -ENOSPC;
>> +		goto err;
>> +	}
>> +
>> +	cxl_release_endpoint(cxl->cxlmd, cxl->endpoint);
>> +
>>   	return 0;
>> +
>> +err_release:
>> +	cxl_release_endpoint(cxl->cxlmd, cxl->endpoint);
>>   err:
>>   	kfree(cxl->cxlds);
>>   	kfree(cxl);