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

Alejandro Lucero Palau
2024-09-16T10:30:33Z

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);