Re: [PATCH] testing/cxl: Fix abused pci_bus_read_config_word() on platform device

[Ira Weiny <ira.weiny@intel.com>]

Dan Williams wrote:
> Alison Schofield wrote:
> > On Wed, Oct 16, 2024 at 09:52:13AM +0800, Li Zhijian wrote:
> > > The cxl_region_shared_upstream_bandwidth_update() in clx_core works on
> > > PCI/PCIe CXL device only while cxl_test was implemeneted by platform
> > > device.
> > > 
> > > Mock a cxl_region_shared_upstream_bandwidth_update() which does nothing
> > > for cxl_core so that the cxl_test goes well.
> > > 
> > > Abuse cxl_region_shared_upstream_bandwidth_update() on platform device
> > > will cause a kernel panic with calltrace:
> > 
> > snip
> > 
> > > ---
> > >  tools/testing/cxl/Kbuild      | 2 ++
> > >  tools/testing/cxl/mock_cdat.c | 8 ++++++++
> > >  2 files changed, 10 insertions(+)
> > >  create mode 100644 tools/testing/cxl/mock_cdat.c
> > > 
> > > diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> > > index b1256fee3567..ed9f50dee3f5 100644
> > > --- a/tools/testing/cxl/Kbuild
> > > +++ b/tools/testing/cxl/Kbuild
> > > @@ -15,6 +15,7 @@ ldflags-y += --wrap=devm_cxl_add_rch_dport
> > >  ldflags-y += --wrap=cxl_rcd_component_reg_phys
> > >  ldflags-y += --wrap=cxl_endpoint_parse_cdat
> > >  ldflags-y += --wrap=cxl_dport_init_ras_reporting
> > > +ldflags-y += --wrap=cxl_region_shared_upstream_bandwidth_update
> > >  
> > >  DRIVERS := ../../../drivers
> > >  CXL_SRC := $(DRIVERS)/cxl
> > > @@ -61,6 +62,7 @@ cxl_core-y += $(CXL_CORE_SRC)/pci.o
> > >  cxl_core-y += $(CXL_CORE_SRC)/hdm.o
> > >  cxl_core-y += $(CXL_CORE_SRC)/pmu.o
> > >  cxl_core-y += $(CXL_CORE_SRC)/cdat.o
> > > +cxl_core-y += mock_cdat.o
> > >  cxl_core-$(CONFIG_TRACING) += $(CXL_CORE_SRC)/trace.o
> > >  cxl_core-$(CONFIG_CXL_REGION) += $(CXL_CORE_SRC)/region.o
> > >  cxl_core-y += config_check.o
> > > diff --git a/tools/testing/cxl/mock_cdat.c b/tools/testing/cxl/mock_cdat.c
> > > new file mode 100644
> > > index 000000000000..99974153b3f6
> > > --- /dev/null
> > > +++ b/tools/testing/cxl/mock_cdat.c
> > > @@ -0,0 +1,8 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/* Copyright (c) 2024 FUJITSU LIMITED. All rights reserved. */
> > > +
> > > +#include <cxl.h>
> > > +
> > > +void __wrap_cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
> > > +{
> > > +}
> > 
> > The addition of file mock_cdat.c made me wonder why this wrapper couldn't join
> > all the other __wrap's defined in test/mock.c. I tried, as you probably did,
> > and see the circular dependency. I mention it here in case anyone else has a
> > way to simplify this.
> 
> Yeah, unfortunately symbols can only be mocked across EXPORT_SYMBOL()
> boundaries, but since the caller of this is internal to the CXL core it
> is not amenable to the wrap approach.
> 
> So, unfortunately what this patch does is break the expectation that
> cxl_test can live alongside and not regress any production flows. I.e.
> what this patch does is replace 
> cxl_region_shared_upstream_bandwidth_update() for all use cases, not
> just platform devices.
> 
> Compare that to tools/testing/cxl/test/mock.c which arranges for all the
> mocked use cases to call back into the real routines in the real device
> case.
> 
> Given that I think this puts the device type check back in play.
> 
> However, instead of checking "dev_is_platform()" check "dev_is_pci()" to
> be consistent with all the other CXL core internal functions that exit
> early when passed cxl_test devices.

Zhijian,

Looks like we will need a spin of this fix.  Do you have time to do that?

Ira