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

["Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>]

Hey Dan,

Thanks for your review...

I want to confirm with you


On 18/10/2024 09:20, 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.

Yes, that's true.


> 
> 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.

Just to confirm, do you mean add device type check to drivers/cxl/core/cdat.c

--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -5,6 +5,7 @@
   #include <linux/fw_table.h>
   #include <linux/node.h>
   #include <linux/overflow.h>
+#include <linux/platform_device.h>
   #include "cxlpci.h"
   #include "cxlmem.h"
   #include "core.h"
@@ -641,9 +642,13 @@ static int cxl_endpoint_gather_bandwidth(struct cxl_region *cxlr,
          void *ptr;
          int rc;

+       if (dev_is_pci(cxlds->dev))
+               return -ENODEV;
+
          if (cxlds->rcd)
                  return -ENODEV;
   

or

tools/testing/cxl/mock_cdat.c

#include <cxl.h>
                                                                                 
void __wrap_cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
{
         for (int i = 0; i < cxlr->params.nr_targets; i++) {
                 struct cxl_endpoint_decoder *cxled = cxlr->params.targets[i];
                 struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
                 struct cxl_dev_state *cxlds = cxlmd->cxlds;
                                                                                 
                 if (!dev_is_pci(cxlds->dev))
                         return;
         }
                                                                                 
         cxl_region_shared_upstream_bandwidth_update(cxlr);
}


Thanks
Zhijian

> 
> 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.