| Message ID | 169476525052.1013896.6235102957693675187.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | a76b62518eb30ef59158fa777ab2e2a23e1334f9 |
| Headers | show |
| Series | cxl/port: Fix cxl_test register enumeration regression | expand |
On 9/15/23 01:07, Dan Williams wrote: > The cxl_test unit test environment models a CXL topology for > sysfs/user-ABI regression testing. It uses interface mocking via the > "--wrap=" linker option to redirect cxl_core routines that parse > hardware registers with versions that just publish objects, like > devm_cxl_enumerate_decoders(). > > Starting with: > > Commit 19ab69a60e3b ("cxl/port: Store the port's Component Register mappings in struct cxl_port") > > ...port register enumeration is moved into devm_cxl_add_port(). This > conflicts with the "cxl_test avoids emulating registers stance" so > either the port code needs to be refactored (too violent), or modified > so that register enumeration is skipped on "fake" cxl_test ports > (annoying, but straightforward). > > This conflict has happened previously and the "check for platform > device" workaround to avoid instrusive refactoring was deployed in those > scenarios. In general, refactoring should only benefit production code, > test code needs to remain minimally instrusive to the greatest extent > possible. > > This was missed previously because it may sometimes just cause warning > messages to be emitted, but it can also cause test failures. The > backport to -stable is only nice to have for clean cxl_test runs. > > Fixes: 19ab69a60e3b ("cxl/port: Store the port's Component Register mappings in struct cxl_port") > Cc: <stable@vger.kernel.org> > Reported-by: Alison Schofield <alison.schofield@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> Tested-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/port.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 724be8448eb4..7ca01a834e18 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1,5 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* Copyright(c) 2020 Intel Corporation. All rights reserved. */ > +#include <linux/platform_device.h> > #include <linux/memregion.h> > #include <linux/workqueue.h> > #include <linux/debugfs.h> > @@ -706,16 +707,20 @@ static int cxl_setup_comp_regs(struct device *dev, struct cxl_register_map *map, > return cxl_setup_regs(map); > } > > -static inline int cxl_port_setup_regs(struct cxl_port *port, > - resource_size_t component_reg_phys) > +static int cxl_port_setup_regs(struct cxl_port *port, > + resource_size_t component_reg_phys) > { > + if (dev_is_platform(port->uport_dev)) > + return 0; > return cxl_setup_comp_regs(&port->dev, &port->comp_map, > component_reg_phys); > } > > -static inline int cxl_dport_setup_regs(struct cxl_dport *dport, > - resource_size_t component_reg_phys) > +static int cxl_dport_setup_regs(struct cxl_dport *dport, > + resource_size_t component_reg_phys) > { > + if (dev_is_platform(dport->dport_dev)) > + return 0; > return cxl_setup_comp_regs(dport->dport_dev, &dport->comp_map, > component_reg_phys); > } >
On 9/15/2023 1:07 AM, Dan Williams wrote: > The cxl_test unit test environment models a CXL topology for > sysfs/user-ABI regression testing. It uses interface mocking via the > "--wrap=" linker option to redirect cxl_core routines that parse > hardware registers with versions that just publish objects, like > devm_cxl_enumerate_decoders(). > > Starting with: > > Commit 19ab69a60e3b ("cxl/port: Store the port's Component Register mappings in struct cxl_port") > > ...port register enumeration is moved into devm_cxl_add_port(). This > conflicts with the "cxl_test avoids emulating registers stance" so > either the port code needs to be refactored (too violent), or modified > so that register enumeration is skipped on "fake" cxl_test ports > (annoying, but straightforward). > > This conflict has happened previously and the "check for platform > device" workaround to avoid instrusive refactoring was deployed in those > scenarios. In general, refactoring should only benefit production code, > test code needs to remain minimally instrusive to the greatest extent > possible. > > This was missed previously because it may sometimes just cause warning > messages to be emitted, but it can also cause test failures. The > backport to -stable is only nice to have for clean cxl_test runs. > > Fixes: 19ab69a60e3b ("cxl/port: Store the port's Component Register mappings in struct cxl_port") > Cc: <stable@vger.kernel.org> > Reported-by: Alison Schofield <alison.schofield@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > This patch has already hit upstream, but I'd like to report this also fixed a 100% reproducible issue just loading cxl_test modules in a Parallels ARM VM on a MacBook Pro.
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 724be8448eb4..7ca01a834e18 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* Copyright(c) 2020 Intel Corporation. All rights reserved. */ +#include <linux/platform_device.h> #include <linux/memregion.h> #include <linux/workqueue.h> #include <linux/debugfs.h> @@ -706,16 +707,20 @@ static int cxl_setup_comp_regs(struct device *dev, struct cxl_register_map *map, return cxl_setup_regs(map); } -static inline int cxl_port_setup_regs(struct cxl_port *port, - resource_size_t component_reg_phys) +static int cxl_port_setup_regs(struct cxl_port *port, + resource_size_t component_reg_phys) { + if (dev_is_platform(port->uport_dev)) + return 0; return cxl_setup_comp_regs(&port->dev, &port->comp_map, component_reg_phys); } -static inline int cxl_dport_setup_regs(struct cxl_dport *dport, - resource_size_t component_reg_phys) +static int cxl_dport_setup_regs(struct cxl_dport *dport, + resource_size_t component_reg_phys) { + if (dev_is_platform(dport->dport_dev)) + return 0; return cxl_setup_comp_regs(dport->dport_dev, &dport->comp_map, component_reg_phys); }
The cxl_test unit test environment models a CXL topology for sysfs/user-ABI regression testing. It uses interface mocking via the "--wrap=" linker option to redirect cxl_core routines that parse hardware registers with versions that just publish objects, like devm_cxl_enumerate_decoders(). Starting with: Commit 19ab69a60e3b ("cxl/port: Store the port's Component Register mappings in struct cxl_port") ...port register enumeration is moved into devm_cxl_add_port(). This conflicts with the "cxl_test avoids emulating registers stance" so either the port code needs to be refactored (too violent), or modified so that register enumeration is skipped on "fake" cxl_test ports (annoying, but straightforward). This conflict has happened previously and the "check for platform device" workaround to avoid instrusive refactoring was deployed in those scenarios. In general, refactoring should only benefit production code, test code needs to remain minimally instrusive to the greatest extent possible. This was missed previously because it may sometimes just cause warning messages to be emitted, but it can also cause test failures. The backport to -stable is only nice to have for clean cxl_test runs. Fixes: 19ab69a60e3b ("cxl/port: Store the port's Component Register mappings in struct cxl_port") Cc: <stable@vger.kernel.org> Reported-by: Alison Schofield <alison.schofield@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/port.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)