Re: [PATCH v17 05/22] sfc: setup cxl component regs and set media ready

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 24 Jun 2025 15:13:38 +0100
alejandro.lucero-palau@amd.com wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> Use cxl code for registers discovery and mapping regarding cxl component
> regs and validate registers found are as expected.
> 
> Set media ready explicitly as there is no means for doing so without
> a mailbox, and without the related cxl register, not mandatory for type2.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>

Perhaps add a brief note to the description on why you decided on the
mix of warn vs err messages in the different conditions.

Superficially there is a call in here that can defer.  If it can't
add a comment on why as if it can you should be failing the main
driver probe until it doesn't defer (or adding a bunch of descriptive
comments on why that doesn't make sense!)

> ---
>  drivers/net/ethernet/sfc/efx_cxl.c | 34 ++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index f1db7284dee8..ea02eb82b73c 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -9,6 +9,7 @@
>   * by the Free Software Foundation, incorporated herein by reference.
>   */
>  
> +#include <cxl/cxl.h>
>  #include <cxl/pci.h>
>  #include <linux/pci.h>
>  
> @@ -23,6 +24,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  	struct pci_dev *pci_dev = efx->pci_dev;
>  	struct efx_cxl *cxl;
>  	u16 dvsec;
> +	int rc;
>  
>  	probe_data->cxl_pio_initialised = false;
>  
> @@ -43,6 +45,38 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  	if (!cxl)
>  		return -ENOMEM;
>  
> +	rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_COMPONENT,
> +				&cxl->cxlds.reg_map);
> +	if (rc) {
> +		dev_warn(&pci_dev->dev, "No component registers (err=%d)\n", rc);
> +		return rc;

I haven't checked the code paths to see if we might hit them but this might
defer.  In which case
		return dev_err_probe() is appropriate as it stashes away the
cause of deferral for debugging purposes and doesn't print if that's what
happened as we'll be back later.

If we can hit the deferral then you should catch that at the caller of efx_cxl_init()
and fail the probe (we'll be back a bit later and should then succeed).


> +	}
> +
> +	if (!cxl->cxlds.reg_map.component_map.hdm_decoder.valid) {
> +		dev_err(&pci_dev->dev, "Expected HDM component register not found\n");
> +		return -ENODEV;

Trivial but given this is new code maybe differing from style of existing sfc
and using
		return dev_err_probe(&pci->dev, "Expected HDM component register not found\n");
would be a nice to have.  Given deferral isn't a thing for this call, it just saves on about
2 lines of code for each use.

or use pci_err() and pci_warn()?

 
> +	}
> +
> +	if (!cxl->cxlds.reg_map.component_map.ras.valid) {
> +		dev_err(&pci_dev->dev, "Expected RAS component register not found\n");
> +		return -ENODEV;
> +	}
> +
> +	rc = cxl_map_component_regs(&cxl->cxlds.reg_map,
> +				    &cxl->cxlds.regs.component,
> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));
> +	if (rc) {
> +		dev_err(&pci_dev->dev, "Failed to map RAS capability.\n");
> +		return rc;

> +	}
> +
> +	/*
> +	 * Set media ready explicitly as there are neither mailbox for checking
> +	 * this state nor the CXL register involved, both not mandatory for
> +	 * type2.
> +	 */
> +	cxl->cxlds.media_ready = true;
> +
>  	probe_data->cxl = cxl;
>  
>  	return 0;