Re: [PATCH v16 06/22] sfc: make regs setup with checking and set media ready

[Dan Williams <dan.j.williams@intel.com>]

Dan Williams wrote:
> Alejandro Lucero Palau wrote:
> > 
> > On 5/21/25 19:34, Dan Williams wrote:
> > > alejandro.lucero-palau@ wrote:
> > >> From: Alejandro Lucero <alucerop@amd.com>
> > >>
> > >> Use cxl code for registers discovery and mapping.
> > >>
> > >> Validate capabilities found based on those registers against expected
> > >> capabilities.
> > >>
> > >> 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>
> > >> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
> > >> Reviewed-by: Zhi Wang <zhi@nvidia.com>
> > >> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> > >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
> > >> ---
> > >>   drivers/net/ethernet/sfc/efx_cxl.c | 26 ++++++++++++++++++++++++++
> > >>   1 file changed, 26 insertions(+)
> > >>
> > >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> > >> index 753d5b7d49b6..e94af8bf3a79 100644
> > >> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> > >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> > >> @@ -19,10 +19,13 @@
> > >>   
> > >>   int efx_cxl_init(struct efx_probe_data *probe_data)
> > >>   {
> > >> +	DECLARE_BITMAP(expected, CXL_MAX_CAPS) = {};
> > >> +	DECLARE_BITMAP(found, CXL_MAX_CAPS) = {};
> > >>   	struct efx_nic *efx = &probe_data->efx;
> > >>   	struct pci_dev *pci_dev = efx->pci_dev;
> > >>   	struct efx_cxl *cxl;
> > >>   	u16 dvsec;
> > >> +	int rc;
> > >>   
> > >>   	probe_data->cxl_pio_initialised = false;
> > >>   
> > >> @@ -43,6 +46,29 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
> > >>   	if (!cxl)
> > >>   		return -ENOMEM;
> > >>   
> > >> +	set_bit(CXL_DEV_CAP_HDM, expected);
> > >> +	set_bit(CXL_DEV_CAP_RAS, expected);
> > >> +
> > >> +	rc = cxl_pci_accel_setup_regs(pci_dev, &cxl->cxlds, found);
> > >> +	if (rc) {
> > >> +		pci_err(pci_dev, "CXL accel setup regs failed");
> > >> +		return rc;
> > >> +	}
> > >> +
> > >> +	/*
> > >> +	 * Checking mandatory caps are there as, at least, a subset of those
> > >> +	 * found.
> > >> +	 */
> > >> +	if (cxl_check_caps(pci_dev, expected, found))
> > >> +		return -ENXIO;
> > > This all looks like an obfuscated way of writing:
> > >
> > >      cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> > >      if (!map.component_map.ras.valid || !map.component_map.hdm_decoder.valid)
> > >           /* sfc cxl expectations not met */

Now, I do notice that the proposal above got the registers blocks wrong.
I.e. that should be:

      cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
      if (!map.component_map.ras.valid || !map.component_map.hdm_decoder.valid)
           /* sfc cxl expectations not met */

I also apologize for the negative connotation of "obfuscated". I should
have said:

"This simply boils down to the following..." which is yes, a bit of a
mouthful to type out, but it has the benefit of no new changes to the
core which is my preference until it becomes exceedingly clear that new
APIs are needed.

So,

map.component_map.hdm_decoder.valid
map.component_map.ras.valid

...instead of:

set_bit(CXL_DEV_CAP_HDM, expected);
set_bit(CXL_DEV_CAP_RAS, expected);

...please.