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

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

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

> On 8/4/24 18:15, Jonathan Cameron wrote:
> > On Mon, 15 Jul 2024 18:28:22 +0100
> > alejandro.lucero-palau@amd.com wrote:
> >  
> >> From: Alejandro Lucero <alucerop@amd.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@amd.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