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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <682f8eb15905c_3e70100b5@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 22 May 2025 13:53:05 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Dan Williams <dan.j.williams@...el.com>, Alejandro Lucero Palau
	<alucerop@....com>, <alejandro.lucero-palau@....com>,
	<linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>, <edward.cree@....com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <dave.jiang@...el.com>
CC: Martin Habets <habetsm.xilinx@...il.com>, Zhi Wang <zhi@...dia.com>,
	Edward Cree <ecree.xilinx@...il.com>, Jonathan Cameron
	<Jonathan.Cameron@...wei.com>, Ben Cheatham <benjamin.cheatham@....com>
Subject: Re: [PATCH v16 06/22] sfc: make regs setup with checking and set
 media ready

Dan Williams wrote:
> Alejandro Lucero Palau wrote:
> > 
> > On 5/21/25 19:34, Dan Williams wrote:
> > > alejandro.lucero-palau@ wrote:
> > >> From: Alejandro Lucero <alucerop@....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@....com>
> > >> Reviewed-by: Martin Habets <habetsm.xilinx@...il.com>
> > >> Reviewed-by: Zhi Wang <zhi@...dia.com>
> > >> Acked-by: Edward Cree <ecree.xilinx@...il.com>
> > >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > >> Reviewed-by: Ben Cheatham <benjamin.cheatham@....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.