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:
> 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 */

Actually that is subtly wrong again and points to a shared wart that, if
it could be cleaned up, would benefit cxl_pci Type-3 use case as well.
That @map unfortunately needs to be cxlds->reg_map.

I do not like the fact that 'struct cxl_dev_state' carries that 'struct
cxl_register_map' just to forward the enumeration to the endpoint
cxl_port. I also do not like that cxl_pci maps the RAS capability while
cxl_port maps the hdm_decoder capability. Ideally cxl_port would own
both those capabilities.

In that scenario simple use cases like sfc that only care about HDM and
RAS can forget about enumerating and mapping CXL component registers
altogether, just devm_cxl_add_memdev() and the core handles the rest.

Now that is the kind of helper and CXL core improvement I am interested
in seeing, and perhaps precludes the need for 'struct cxl_register_map'
to be moved to include/cxl/pci.h.

It may be something we can do after Terry's CXL protocol error series.