netdev - Re: [PATCH v2 02/15] cxl: add function for type2 cxl regs setup

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240815174035.00005bb0@Huawei.com>
Date: Thu, 15 Aug 2024 17:40:35 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Alejandro Lucero Palau <alucerop@....com>
CC: <alejandro.lucero-palau@....com>, <linux-cxl@...r.kernel.org>,
	<netdev@...r.kernel.org>, <dan.j.williams@...el.com>,
	<martin.habets@...inx.com>, <edward.cree@....com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<richard.hughes@....com>
Subject: Re: [PATCH v2 02/15] cxl: add function for type2 cxl regs setup

On Wed, 14 Aug 2024 08:56:35 +0100
Alejandro Lucero Palau <alucerop@....com> wrote:

> On 8/4/24 18:15, Jonathan Cameron wrote:
> > On Mon, 15 Jul 2024 18:28:22 +0100
> > alejandro.lucero-palau@....com wrote:
> >  
> >> From: Alejandro Lucero <alucerop@....com>
> >>
> >> Create a new function for a type2 device initialising the opaque
> >> cxl_dev_state struct regarding cxl regs setup and mapping.
> >>
> >> Signed-off-by: Alejandro Lucero <alucerop@....com>
> >> ---
> >>   drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
> >>   drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
> >>   include/linux/cxl_accel_mem.h      |  1 +
> >>   3 files changed, 32 insertions(+)
> >>
> >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> >> index e53646e9f2fb..b34d6259faf4 100644
> >> --- a/drivers/cxl/pci.c
> >> +++ b/drivers/cxl/pci.c
> >> @@ -11,6 +11,7 @@
> >>   #include <linux/pci.h>
> >>   #include <linux/aer.h>
> >>   #include <linux/io.h>
> >> +#include <linux/cxl_accel_mem.h>
> >>   #include "cxlmem.h"
> >>   #include "cxlpci.h"
> >>   #include "cxl.h"
> >> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> >>   	return cxl_setup_regs(map);
> >>   }
> >>   
> >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
> >> +{
> >> +	struct cxl_register_map map;
> >> +	int rc;
> >> +
> >> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
> >> +				&cxlds->reg_map);
> >> +	if (rc)
> >> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);  
> > Not fatal?  If we think it will happen on real devices, then dev_warn
> > is too strong.  
> 
> 
> This is more complex than what it seems, and it is not properly handled 
> with the current code.
> 
> I will cover it in another patch in more detail, but the fact is those 
> calls to cxl_pci_setup_regs need to be handled better, because Type2 has 
> some of these registers as optional.

I'd argue you don't have to support all type 2 devices with your
first code.  Things like optionality of registers can come in when
a device shows up where they aren't present.

Jonathan