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

Jonathan Cameron
2024-08-15T16:40:44Z

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