Re: [PATCH v6 02/28] sfc: add cxl support using new CXL API

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

On 12/3/24 20:33, Edward Cree wrote:
> On 02/12/2024 17:11, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Add CXL initialization based on new CXL API for accel drivers and make
>> it dependable on kernel CXL configuration.
> You probably mean 'dependent' (or maybe just 'depend').


Right. I'll fix it.


>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Some nits to consider for v7, but this already gets my
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
>
> ...
>> @@ -1214,6 +1222,17 @@ static int efx_pci_probe(struct pci_dev *pci_dev,
>>   	if (rc)
>>   		goto fail2;
>>   
>> +#ifdef CONFIG_SFC_CXL
>> +	/* A successful cxl initialization implies a CXL region created to be
>> +	 * 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.
>> +	 */
>> +	rc = efx_cxl_init(probe_data);
>> +	if (rc)
>> +		pci_err(pci_dev, "CXL initialization failed with error %d\n", rc);
>> +
>> +#endif
>>   	rc = efx_pci_probe_post_io(efx);
>>   	if (rc) {
>>   		/* On failure, retry once immediately.
> nit: weird to have the blank line before the #endif rather than after.


I'll fix it.


>
> ...
>> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
>> index 620ba6ef3514..7f11ff200c25 100644
>> --- a/drivers/net/ethernet/sfc/net_driver.h
>> +++ b/drivers/net/ethernet/sfc/net_driver.h
>> @@ -1199,14 +1199,24 @@ struct efx_nic {
>>   	atomic_t n_rx_noskb_drops;
>>   };
>>   
>> +#ifdef CONFIG_SFC_CXL
>> +struct efx_cxl;
>> +#endif
>> +
>>   /**
>>    * struct efx_probe_data - State after hardware probe
>>    * @pci_dev: The PCI device
>>    * @efx: Efx NIC details
>> + * @cxl: details of related cxl objects
>> + * @cxl_pio_initialised: cxl initialization outcome.
>>    */
>>   struct efx_probe_data {
>>   	struct pci_dev *pci_dev;
>>   	struct efx_nic efx;
>> +#ifdef CONFIG_SFC_CXL
>> +	struct efx_cxl *cxl;
>> +	bool cxl_pio_initialised;
> Not clear why this needs to be a separate bool rather than just
>   seeing if probe_data->cxl is nonnull; afaict from a quick skim of
>   the series it's only used in patch 28, which sets it to true in
>   the same place probe_data->cxl is populated.


In a previous version, I think it was v2 or v3, I did use the check of 
the sfc cxl struct, or maybe the pointer obtained with ioremap if 
everything was correct, and it was suggested then to use another way 
instead of that, so I added that variable.

Nothing harmful, I guess.

Thanks!