* Re: [PATCH 17/23] cxl: Cache and pass DVSEC ranges
@ 2021-11-27 18:35 kernel test robot
2021-11-29 9:39 ` kernel test robot
0 siblings, 1 reply; 9+ messages in thread
From: kernel test robot @ 2021-11-27 18:35 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 18441 bytes --]
CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211120000250.1663391-18-ben.widawsky@intel.com>
References: <20211120000250.1663391-18-ben.widawsky@intel.com>
TO: Ben Widawsky <ben.widawsky@intel.com>
TO: linux-cxl(a)vger.kernel.org
TO: linux-pci(a)vger.kernel.org
CC: Ben Widawsky <ben.widawsky@intel.com>
CC: Alison Schofield <alison.schofield@intel.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Ira Weiny <ira.weiny@intel.com>
CC: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
CC: Vishal Verma <vishal.l.verma@intel.com>
Hi Ben,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 53989fad1286e652ea3655ae3367ba698da8d2ff]
url: https://github.com/0day-ci/linux/commits/Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
base: 53989fad1286e652ea3655ae3367ba698da8d2ff
:::::: branch date: 8 days ago
:::::: commit date: 8 days ago
config: x86_64-randconfig-c007-20211118 (https://download.01.org/0day-ci/archive/20211128/202111280254.IoqCZcvv-lkp(a)intel.com/config)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
git checkout cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings generated.
drivers/power/supply/bq256xx_charger.c:275:8: warning: Excessive padding in 'struct bq256xx_chip_info' (11 padding bytes, where 3 is optimal).
Optimal fields order:
bq256xx_regmap_config,
bq256xx_get_ichg,
bq256xx_get_iindpm,
bq256xx_get_vbatreg,
bq256xx_get_iterm,
bq256xx_get_iprechg,
bq256xx_get_vindpm,
bq256xx_set_ichg,
bq256xx_set_iindpm,
bq256xx_set_vbatreg,
bq256xx_set_iterm,
bq256xx_set_iprechg,
bq256xx_set_vindpm,
model_id,
bq256xx_def_ichg,
bq256xx_def_iindpm,
bq256xx_def_vbatreg,
bq256xx_def_iterm,
bq256xx_def_iprechg,
bq256xx_def_vindpm,
bq256xx_max_ichg,
bq256xx_max_vbatreg,
has_usb_detect,
consider reordering the fields or adding explicit padding members [clang-analyzer-optin.performance.Padding]
struct bq256xx_chip_info {
~~~~~~~^~~~~~~~~~~~~~~~~~~
drivers/power/supply/bq256xx_charger.c:275:8: note: Excessive padding in 'struct bq256xx_chip_info' (11 padding bytes, where 3 is optimal). Optimal fields order: bq256xx_regmap_config, bq256xx_get_ichg, bq256xx_get_iindpm, bq256xx_get_vbatreg, bq256xx_get_iterm, bq256xx_get_iprechg, bq256xx_get_vindpm, bq256xx_set_ichg, bq256xx_set_iindpm, bq256xx_set_vbatreg, bq256xx_set_iterm, bq256xx_set_iprechg, bq256xx_set_vindpm, model_id, bq256xx_def_ichg, bq256xx_def_iindpm, bq256xx_def_vbatreg, bq256xx_def_iterm, bq256xx_def_iprechg, bq256xx_def_vindpm, bq256xx_max_ichg, bq256xx_max_vbatreg, has_usb_detect, consider reordering the fields or adding explicit padding members
struct bq256xx_chip_info {
~~~~~~~^~~~~~~~~~~~~~~~~~~
drivers/power/supply/bq256xx_charger.c:1521:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
ret = regmap_update_bits(bq->regmap, BQ256XX_CHARGER_CONTROL_1,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/power/supply/bq256xx_charger.c:1521:2: note: Value stored to 'ret' is never read
ret = regmap_update_bits(bq->regmap, BQ256XX_CHARGER_CONTROL_1,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
drivers/hwmon/acpi_power_meter.c:879:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(acpi_device_name(device), ACPI_POWER_METER_DEVICE_NAME);
^~~~~~
drivers/hwmon/acpi_power_meter.c:879:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(acpi_device_name(device), ACPI_POWER_METER_DEVICE_NAME);
^~~~~~
drivers/hwmon/acpi_power_meter.c:880:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(acpi_device_class(device), ACPI_POWER_METER_CLASS);
^~~~~~
drivers/hwmon/acpi_power_meter.c:880:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(acpi_device_class(device), ACPI_POWER_METER_CLASS);
^~~~~~
2 warnings generated.
drivers/usb/storage/freecom.c:449:2: warning: Value stored to 'result' is never read [clang-analyzer-deadcode.DeadStores]
result = usb_stor_control_msg(us, us->recv_ctrl_pipe,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/usb/storage/freecom.c:449:2: note: Value stored to 'result' is never read
result = usb_stor_control_msg(us, us->recv_ctrl_pipe,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/usb/storage/freecom.c:537:2: warning: Value stored to 'offset' is never read [clang-analyzer-deadcode.DeadStores]
offset = 0;
^ ~
drivers/usb/storage/freecom.c:537:2: note: Value stored to 'offset' is never read
offset = 0;
^ ~
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
3 warnings generated.
drivers/cxl/core/mbox.c:324:17: warning: Value stored to 'dev' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
struct device *dev = &cxlmd->dev;
^~~ ~~~~~~~~~~~
drivers/cxl/core/mbox.c:324:17: note: Value stored to 'dev' during its initialization is never read
struct device *dev = &cxlmd->dev;
^~~ ~~~~~~~~~~~
drivers/cxl/core/mbox.c:449:17: warning: Value stored to 'dev' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
struct device *dev = &cxlmd->dev;
^~~ ~~~~~~~~~~~
drivers/cxl/core/mbox.c:449:17: note: Value stored to 'dev' during its initialization is never read
struct device *dev = &cxlmd->dev;
^~~ ~~~~~~~~~~~
drivers/cxl/core/mbox.c:580:17: warning: Value stored to 'dev' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
struct device *dev = cxlds->dev;
^~~ ~~~~~~~~~~
drivers/cxl/core/mbox.c:580:17: note: Value stored to 'dev' during its initialization is never read
struct device *dev = cxlds->dev;
^~~ ~~~~~~~~~~
4 warnings generated.
drivers/cxl/pci.c:42:16: warning: Value stored to 'end' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
unsigned long end = start;
^~~ ~~~~~
drivers/cxl/pci.c:42:16: note: Value stored to 'end' during its initialization is never read
unsigned long end = start;
^~~ ~~~~~
drivers/cxl/pci.c:64:17: warning: Value stored to 'dev' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
struct device *dev = cxlds->dev;
^~~ ~~~~~~~~~~
drivers/cxl/pci.c:64:17: note: Value stored to 'dev' during its initialization is never read
struct device *dev = cxlds->dev;
^~~ ~~~~~~~~~~
>> drivers/cxl/pci.c:483:3: warning: Value stored to 'size' is never read [clang-analyzer-deadcode.DeadStores]
size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/cxl/pci.c:483:3: note: Value stored to 'size' is never read
size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Suppressed 1 warnings (1 with check filters).
3 warnings generated.
drivers/cxl/acpi.c:142:17: warning: Value stored to 'dev' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
struct device *dev = ctx->dev;
^~~ ~~~~~~~~
drivers/cxl/acpi.c:142:17: note: Value stored to 'dev' during its initialization is never read
struct device *dev = ctx->dev;
^~~ ~~~~~~~~
drivers/cxl/acpi.c:399:14: warning: 3rd function call argument is an uninitialized value [clang-analyzer-core.CallAndMessage]
root_port = devm_cxl_add_port(host, CXL_RESOURCE_NONE, root_port);
^ ~~~~~~~~~
drivers/cxl/acpi.c:386:2: note: 'root_port' declared without an initial value
struct cxl_port *root_port;
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/cxl/acpi.c:388:29: note: Assuming the condition is false
struct acpi_device *adev = ACPI_COMPANION(host);
^
include/linux/acpi.h:43:30: note: expanded from macro 'ACPI_COMPANION'
#define ACPI_COMPANION(dev) to_acpi_device_node((dev)->fwnode)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/acpi/acpi_bus.h:418:3: note: expanded from macro 'to_acpi_device_node'
is_acpi_device_node(__to_acpi_device_node_fwnode) ? \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/cxl/acpi.c:388:29: note: '?' condition is false
struct acpi_device *adev = ACPI_COMPANION(host);
^
include/linux/acpi.h:43:30: note: expanded from macro 'ACPI_COMPANION'
#define ACPI_COMPANION(dev) to_acpi_device_node((dev)->fwnode)
^
include/acpi/acpi_bus.h:418:3: note: expanded from macro 'to_acpi_device_node'
is_acpi_device_node(__to_acpi_device_node_fwnode) ? \
^
drivers/cxl/acpi.c:392:2: note: Assuming 'rc' is 0
if (rc)
^
include/linux/compiler.h:56:45: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
drivers/cxl/acpi.c:392:2: note: '?' condition is false
if (rc)
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:31: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
drivers/cxl/acpi.c:392:6: note: 'rc' is 0
if (rc)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
drivers/cxl/acpi.c:392:2: note: '?' condition is false
if (rc)
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:69: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
include/linux/compiler.h:69:2: note: expanded from macro '__trace_if_value'
(cond) ? \
^
drivers/cxl/acpi.c:392:2: note: Taking false branch
if (rc)
^
include/linux/compiler.h:56:23: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
drivers/cxl/acpi.c:396:2: note: '?' condition is false
if (rc)
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:31: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
drivers/cxl/acpi.c:396:6: note: 'rc' is 0
if (rc)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
vim +/size +483 drivers/cxl/pci.c
1d5a4159074bde Ben Widawsky 2021-04-07 454
cfdf51e15fc822 Ben Widawsky 2021-11-19 455 #define CDPD(cxlds, which) \
cfdf51e15fc822 Ben Widawsky 2021-11-19 456 cxlds->device_dvsec + CXL_DVSEC_PCIE_DEVICE_##which##_OFFSET
cfdf51e15fc822 Ben Widawsky 2021-11-19 457
cfdf51e15fc822 Ben Widawsky 2021-11-19 458 #define CDPDR(cxlds, which, sorb, lohi) \
cfdf51e15fc822 Ben Widawsky 2021-11-19 459 cxlds->device_dvsec + \
cfdf51e15fc822 Ben Widawsky 2021-11-19 460 CXL_DVSEC_PCIE_DEVICE_RANGE_##sorb##_##lohi##_OFFSET(which)
cfdf51e15fc822 Ben Widawsky 2021-11-19 461
cfdf51e15fc822 Ben Widawsky 2021-11-19 462 static int wait_for_valid(struct cxl_dev_state *cxlds)
cfdf51e15fc822 Ben Widawsky 2021-11-19 463 {
cfdf51e15fc822 Ben Widawsky 2021-11-19 464 struct pci_dev *pdev = to_pci_dev(cxlds->dev);
cfdf51e15fc822 Ben Widawsky 2021-11-19 465 const unsigned long timeout = jiffies + HZ;
cfdf51e15fc822 Ben Widawsky 2021-11-19 466 bool valid;
cfdf51e15fc822 Ben Widawsky 2021-11-19 467
cfdf51e15fc822 Ben Widawsky 2021-11-19 468 do {
cfdf51e15fc822 Ben Widawsky 2021-11-19 469 u64 size;
cfdf51e15fc822 Ben Widawsky 2021-11-19 470 u32 temp;
cfdf51e15fc822 Ben Widawsky 2021-11-19 471 int rc;
cfdf51e15fc822 Ben Widawsky 2021-11-19 472
cfdf51e15fc822 Ben Widawsky 2021-11-19 473 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, HIGH),
cfdf51e15fc822 Ben Widawsky 2021-11-19 474 &temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 475 if (rc)
cfdf51e15fc822 Ben Widawsky 2021-11-19 476 return -ENXIO;
cfdf51e15fc822 Ben Widawsky 2021-11-19 477 size = (u64)temp << 32;
cfdf51e15fc822 Ben Widawsky 2021-11-19 478
cfdf51e15fc822 Ben Widawsky 2021-11-19 479 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, LOW),
cfdf51e15fc822 Ben Widawsky 2021-11-19 480 &temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 481 if (rc)
cfdf51e15fc822 Ben Widawsky 2021-11-19 482 return -ENXIO;
cfdf51e15fc822 Ben Widawsky 2021-11-19 @483 size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
cfdf51e15fc822 Ben Widawsky 2021-11-19 484
cfdf51e15fc822 Ben Widawsky 2021-11-19 485 /*
cfdf51e15fc822 Ben Widawsky 2021-11-19 486 * Memory_Info_Valid: When set, indicates that the CXL Range 1
cfdf51e15fc822 Ben Widawsky 2021-11-19 487 * Size high and Size Low registers are valid. Must be set
cfdf51e15fc822 Ben Widawsky 2021-11-19 488 * within 1 second of deassertion of reset to CXL device.
cfdf51e15fc822 Ben Widawsky 2021-11-19 489 */
cfdf51e15fc822 Ben Widawsky 2021-11-19 490 valid = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID, temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 491 if (valid)
cfdf51e15fc822 Ben Widawsky 2021-11-19 492 break;
cfdf51e15fc822 Ben Widawsky 2021-11-19 493 cpu_relax();
cfdf51e15fc822 Ben Widawsky 2021-11-19 494 } while (!time_after(jiffies, timeout));
cfdf51e15fc822 Ben Widawsky 2021-11-19 495
cfdf51e15fc822 Ben Widawsky 2021-11-19 496 return valid ? 0 : -ETIMEDOUT;
cfdf51e15fc822 Ben Widawsky 2021-11-19 497 }
cfdf51e15fc822 Ben Widawsky 2021-11-19 498
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 17/23] cxl: Cache and pass DVSEC ranges
2021-11-27 18:35 [PATCH 17/23] cxl: Cache and pass DVSEC ranges kernel test robot
@ 2021-11-29 9:39 ` kernel test robot
0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-11-29 9:39 UTC (permalink / raw)
To: Ben Widawsky; +Cc: llvm, kbuild-all, linux-pci, linux-cxl
Hi Ben,
Thanks for your patch! Perhaps something to improve:
[auto build test WARNING on 53989fad1286e652ea3655ae3367ba698da8d2ff]
url: https://github.com/0day-ci/linux/commits/Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
base: 53989fad1286e652ea3655ae3367ba698da8d2ff
config: x86_64-randconfig-c007-20211118 (https://download.01.org/0day-ci/archive/20211128/202111280254.IoqCZcvv-lkp@intel.com/config)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
git checkout cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
>> drivers/cxl/pci.c:483:3: warning: Value stored to 'size' is never read [clang-analyzer-deadcode.DeadStores]
size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/size +483 drivers/cxl/pci.c
1d5a4159074bde Ben Widawsky 2021-04-07 454
cfdf51e15fc822 Ben Widawsky 2021-11-19 455 #define CDPD(cxlds, which) \
cfdf51e15fc822 Ben Widawsky 2021-11-19 456 cxlds->device_dvsec + CXL_DVSEC_PCIE_DEVICE_##which##_OFFSET
cfdf51e15fc822 Ben Widawsky 2021-11-19 457
cfdf51e15fc822 Ben Widawsky 2021-11-19 458 #define CDPDR(cxlds, which, sorb, lohi) \
cfdf51e15fc822 Ben Widawsky 2021-11-19 459 cxlds->device_dvsec + \
cfdf51e15fc822 Ben Widawsky 2021-11-19 460 CXL_DVSEC_PCIE_DEVICE_RANGE_##sorb##_##lohi##_OFFSET(which)
cfdf51e15fc822 Ben Widawsky 2021-11-19 461
cfdf51e15fc822 Ben Widawsky 2021-11-19 462 static int wait_for_valid(struct cxl_dev_state *cxlds)
cfdf51e15fc822 Ben Widawsky 2021-11-19 463 {
cfdf51e15fc822 Ben Widawsky 2021-11-19 464 struct pci_dev *pdev = to_pci_dev(cxlds->dev);
cfdf51e15fc822 Ben Widawsky 2021-11-19 465 const unsigned long timeout = jiffies + HZ;
cfdf51e15fc822 Ben Widawsky 2021-11-19 466 bool valid;
cfdf51e15fc822 Ben Widawsky 2021-11-19 467
cfdf51e15fc822 Ben Widawsky 2021-11-19 468 do {
cfdf51e15fc822 Ben Widawsky 2021-11-19 469 u64 size;
cfdf51e15fc822 Ben Widawsky 2021-11-19 470 u32 temp;
cfdf51e15fc822 Ben Widawsky 2021-11-19 471 int rc;
cfdf51e15fc822 Ben Widawsky 2021-11-19 472
cfdf51e15fc822 Ben Widawsky 2021-11-19 473 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, HIGH),
cfdf51e15fc822 Ben Widawsky 2021-11-19 474 &temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 475 if (rc)
cfdf51e15fc822 Ben Widawsky 2021-11-19 476 return -ENXIO;
cfdf51e15fc822 Ben Widawsky 2021-11-19 477 size = (u64)temp << 32;
cfdf51e15fc822 Ben Widawsky 2021-11-19 478
cfdf51e15fc822 Ben Widawsky 2021-11-19 479 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, LOW),
cfdf51e15fc822 Ben Widawsky 2021-11-19 480 &temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 481 if (rc)
cfdf51e15fc822 Ben Widawsky 2021-11-19 482 return -ENXIO;
cfdf51e15fc822 Ben Widawsky 2021-11-19 @483 size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
cfdf51e15fc822 Ben Widawsky 2021-11-19 484
cfdf51e15fc822 Ben Widawsky 2021-11-19 485 /*
cfdf51e15fc822 Ben Widawsky 2021-11-19 486 * Memory_Info_Valid: When set, indicates that the CXL Range 1
cfdf51e15fc822 Ben Widawsky 2021-11-19 487 * Size high and Size Low registers are valid. Must be set
cfdf51e15fc822 Ben Widawsky 2021-11-19 488 * within 1 second of deassertion of reset to CXL device.
cfdf51e15fc822 Ben Widawsky 2021-11-19 489 */
cfdf51e15fc822 Ben Widawsky 2021-11-19 490 valid = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID, temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 491 if (valid)
cfdf51e15fc822 Ben Widawsky 2021-11-19 492 break;
cfdf51e15fc822 Ben Widawsky 2021-11-19 493 cpu_relax();
cfdf51e15fc822 Ben Widawsky 2021-11-19 494 } while (!time_after(jiffies, timeout));
cfdf51e15fc822 Ben Widawsky 2021-11-19 495
cfdf51e15fc822 Ben Widawsky 2021-11-19 496 return valid ? 0 : -ETIMEDOUT;
cfdf51e15fc822 Ben Widawsky 2021-11-19 497 }
cfdf51e15fc822 Ben Widawsky 2021-11-19 498
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 17/23] cxl: Cache and pass DVSEC ranges
@ 2021-11-29 9:39 ` kernel test robot
0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-11-29 9:39 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5008 bytes --]
Hi Ben,
Thanks for your patch! Perhaps something to improve:
[auto build test WARNING on 53989fad1286e652ea3655ae3367ba698da8d2ff]
url: https://github.com/0day-ci/linux/commits/Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
base: 53989fad1286e652ea3655ae3367ba698da8d2ff
config: x86_64-randconfig-c007-20211118 (https://download.01.org/0day-ci/archive/20211128/202111280254.IoqCZcvv-lkp(a)intel.com/config)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
git checkout cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
>> drivers/cxl/pci.c:483:3: warning: Value stored to 'size' is never read [clang-analyzer-deadcode.DeadStores]
size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/size +483 drivers/cxl/pci.c
1d5a4159074bde Ben Widawsky 2021-04-07 454
cfdf51e15fc822 Ben Widawsky 2021-11-19 455 #define CDPD(cxlds, which) \
cfdf51e15fc822 Ben Widawsky 2021-11-19 456 cxlds->device_dvsec + CXL_DVSEC_PCIE_DEVICE_##which##_OFFSET
cfdf51e15fc822 Ben Widawsky 2021-11-19 457
cfdf51e15fc822 Ben Widawsky 2021-11-19 458 #define CDPDR(cxlds, which, sorb, lohi) \
cfdf51e15fc822 Ben Widawsky 2021-11-19 459 cxlds->device_dvsec + \
cfdf51e15fc822 Ben Widawsky 2021-11-19 460 CXL_DVSEC_PCIE_DEVICE_RANGE_##sorb##_##lohi##_OFFSET(which)
cfdf51e15fc822 Ben Widawsky 2021-11-19 461
cfdf51e15fc822 Ben Widawsky 2021-11-19 462 static int wait_for_valid(struct cxl_dev_state *cxlds)
cfdf51e15fc822 Ben Widawsky 2021-11-19 463 {
cfdf51e15fc822 Ben Widawsky 2021-11-19 464 struct pci_dev *pdev = to_pci_dev(cxlds->dev);
cfdf51e15fc822 Ben Widawsky 2021-11-19 465 const unsigned long timeout = jiffies + HZ;
cfdf51e15fc822 Ben Widawsky 2021-11-19 466 bool valid;
cfdf51e15fc822 Ben Widawsky 2021-11-19 467
cfdf51e15fc822 Ben Widawsky 2021-11-19 468 do {
cfdf51e15fc822 Ben Widawsky 2021-11-19 469 u64 size;
cfdf51e15fc822 Ben Widawsky 2021-11-19 470 u32 temp;
cfdf51e15fc822 Ben Widawsky 2021-11-19 471 int rc;
cfdf51e15fc822 Ben Widawsky 2021-11-19 472
cfdf51e15fc822 Ben Widawsky 2021-11-19 473 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, HIGH),
cfdf51e15fc822 Ben Widawsky 2021-11-19 474 &temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 475 if (rc)
cfdf51e15fc822 Ben Widawsky 2021-11-19 476 return -ENXIO;
cfdf51e15fc822 Ben Widawsky 2021-11-19 477 size = (u64)temp << 32;
cfdf51e15fc822 Ben Widawsky 2021-11-19 478
cfdf51e15fc822 Ben Widawsky 2021-11-19 479 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, LOW),
cfdf51e15fc822 Ben Widawsky 2021-11-19 480 &temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 481 if (rc)
cfdf51e15fc822 Ben Widawsky 2021-11-19 482 return -ENXIO;
cfdf51e15fc822 Ben Widawsky 2021-11-19 @483 size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
cfdf51e15fc822 Ben Widawsky 2021-11-19 484
cfdf51e15fc822 Ben Widawsky 2021-11-19 485 /*
cfdf51e15fc822 Ben Widawsky 2021-11-19 486 * Memory_Info_Valid: When set, indicates that the CXL Range 1
cfdf51e15fc822 Ben Widawsky 2021-11-19 487 * Size high and Size Low registers are valid. Must be set
cfdf51e15fc822 Ben Widawsky 2021-11-19 488 * within 1 second of deassertion of reset to CXL device.
cfdf51e15fc822 Ben Widawsky 2021-11-19 489 */
cfdf51e15fc822 Ben Widawsky 2021-11-19 490 valid = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID, temp);
cfdf51e15fc822 Ben Widawsky 2021-11-19 491 if (valid)
cfdf51e15fc822 Ben Widawsky 2021-11-19 492 break;
cfdf51e15fc822 Ben Widawsky 2021-11-19 493 cpu_relax();
cfdf51e15fc822 Ben Widawsky 2021-11-19 494 } while (!time_after(jiffies, timeout));
cfdf51e15fc822 Ben Widawsky 2021-11-19 495
cfdf51e15fc822 Ben Widawsky 2021-11-19 496 return valid ? 0 : -ETIMEDOUT;
cfdf51e15fc822 Ben Widawsky 2021-11-19 497 }
cfdf51e15fc822 Ben Widawsky 2021-11-19 498
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 00/23] Add drivers for CXL ports and mem devices
@ 2021-11-20 0:02 Ben Widawsky
2021-11-20 0:02 ` [PATCH 17/23] cxl: Cache and pass DVSEC ranges Ben Widawsky
0 siblings, 1 reply; 9+ messages in thread
From: Ben Widawsky @ 2021-11-20 0:02 UTC (permalink / raw)
To: linux-cxl, linux-pci
Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
Jonathan Cameron, Vishal Verma
This is the first set of patches from the RFC [1] for region creation. The
patches enable port enumeration for endpoint devices, and enumeration of decoder
resources for ports. In the RFC [1], I felt it necessary to post the consumer of
this work, the region driver, so that it was clear why these patches were
necessary. Because the region patches patches are less baked, and received no
review in the RFC, they are excluded here. If you find yourself unclear about
why these patches are interesting, go look at the RFC [1].
Each patch contains the list of changes from RFCv2. IMHO the following are the
high level most important changes:
1. Rework cxl_pci to fix mailbox handling and allow for wait media ready.
2. DVSEC range information is passed from cxl_pci and checked
linux-pci is on the Cc since CXL lives in a parallel universe to PCI and some
PCI mechanisms are reused here. Feedback from experts in that domain is very
welcome.
What was requested and not changed:
1. Dropping global list of root ports.
2. Improving find_parent_cxl_port()
---
Summary
=======
Two new drivers are introduced to support Compute Express Link 2.0 [2] HDM
decoder enumeration. While the existing cxl_acpi and cxl_pci drivers already
create some of the necessary devices, they did not do full enumeration of
decoders, and they did not do port enumeration for switches. Additionally, CXL
2.0 Root Port component registers are now handled as well.
cxl_port
========
The cxl_port driver is implemented within the cxl_port module. While loading of
this module is optional, the other new drivers depend, and cxl_acpi depend on it
for complete enumeration. The port driver is responsible for all activities
around HDM decoder enumeration and programming. Introduced earlier, the concept
of a port is an abstraction over CXL components with an upstream port, every
host bridge, switch, and endpoint.
cxl_mem
=======
The cxl_mem driver's main job is to walk up the hierarchy to make the
determination if it is CXL.mem routed, meaning, all components above it in the
hierarchy are participating in the CXL.mem protocol. It is implemented within
the cxl_mem module. As the host bridge ports are added by a platform specific
driver, such as cxl_acpi, the scope of the mem driver can be reduced to scan for
switches and ask cxl_core to work on enumerating them. With this done, the
determination as to whether a device is CXL.mem routed can be done simply by
checking if the struct device has a driver bound to it.
Results
=======
Running these patches should yield new devices and new drivers under
/sys/bus/cxl/devices and /sys/bus/cxl/drivers. For example, in a standard QEMU
run, using run_qemu [3]
/sys/bus/cxl/devices (new):
# The host bridge CHBS decoder
lrwxrwxrwx 1 root root 0 Nov 19 15:23 decoder1.0 -> ../../../devices/platform/ACPI0017:00/root0/port1/decoder1.0
# mem0's decoder
lrwxrwxrwx 1 root root 0 Nov 19 15:23 decoder2.0 -> ../../../devices/platform/ACPI0017:00/root0/port1/port2/decoder2.0
# mem1's decoder
lrwxrwxrwx 1 root root 0 Nov 19 15:23 decoder3.0 -> ../../../devices/platform/ACPI0017:00/root0/port1/port3/decoder3.0
# mem0's port
lrwxrwxrwx 1 root root 0 Nov 19 15:23 port2 -> ../../../devices/platform/ACPI0017:00/root0/port1/port2
# mem1's port
lrwxrwxrwx 1 root root 0 Nov 19 15:23 port3 -> ../../../devices/platform/ACPI0017:00/root0/port1/port3
/sys/bus/cxl/drivers:
drwxr-xr-x 2 root root 0 Nov 19 15:23 cxl_mem
drwxr-xr-x 2 root root 0 Nov 19 15:23 cxl_port
---
[1]: https://lore.kernel.org/linux-cxl/20211022183709.1199701-1-ben.widawsky@intel.com/T/#t
[2]: https://www.computeexpresslink.org/download-the-specification
[3]: https://github.com/pmem/run_qemu/
Ben Widawsky (23):
cxl: Rename CXL_MEM to CXL_PCI
cxl: Flesh out register names
cxl/pci: Extract device status check
cxl/pci: Implement Interface Ready Timeout
cxl/pci: Don't poll doorbell for mailbox access
cxl/pci: Don't check media status for mbox access
cxl/pci: Add new DVSEC definitions
cxl/acpi: Map component registers for Root Ports
cxl: Introduce module_cxl_driver
cxl/core: Convert decoder range to resource
cxl/core: Document and tighten up decoder APIs
cxl: Introduce endpoint decoders
cxl/core: Move target population locking to caller
cxl: Introduce topology host registration
cxl/core: Store global list of root ports
cxl/pci: Cache device DVSEC offset
cxl: Cache and pass DVSEC ranges
cxl/pci: Implement wait for media active
cxl/pci: Store component register base in cxlds
cxl/port: Introduce a port driver
cxl: Unify port enumeration for decoders
cxl/mem: Introduce cxl_mem driver
cxl/mem: Disable switch hierarchies for now
.../driver-api/cxl/memory-devices.rst | 14 +
drivers/cxl/Kconfig | 54 ++-
drivers/cxl/Makefile | 6 +-
drivers/cxl/acpi.c | 103 ++--
drivers/cxl/core/Makefile | 1 +
drivers/cxl/core/bus.c | 439 ++++++++++++++++--
drivers/cxl/core/core.h | 3 +
drivers/cxl/core/memdev.c | 2 +-
drivers/cxl/core/pci.c | 119 +++++
drivers/cxl/core/regs.c | 60 ++-
drivers/cxl/cxl.h | 73 ++-
drivers/cxl/cxlmem.h | 27 ++
drivers/cxl/mem.c | 197 ++++++++
drivers/cxl/pci.c | 341 ++++++++++----
drivers/cxl/pci.h | 53 ++-
drivers/cxl/port.c | 383 +++++++++++++++
tools/testing/cxl/Kbuild | 1 +
tools/testing/cxl/mock_acpi.c | 4 +-
18 files changed, 1666 insertions(+), 214 deletions(-)
create mode 100644 drivers/cxl/core/pci.c
create mode 100644 drivers/cxl/mem.c
create mode 100644 drivers/cxl/port.c
base-commit: 53989fad1286e652ea3655ae3367ba698da8d2ff
--
2.34.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 17/23] cxl: Cache and pass DVSEC ranges
2021-11-20 0:02 [PATCH 00/23] Add drivers for CXL ports and mem devices Ben Widawsky
@ 2021-11-20 0:02 ` Ben Widawsky
2021-11-20 4:29 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ben Widawsky @ 2021-11-20 0:02 UTC (permalink / raw)
To: linux-cxl, linux-pci
Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
Jonathan Cameron, Vishal Verma
CXL 1.1 specification provided a mechanism for mapping an address space
of a CXL device. That functionality is known as a "range" and can be
programmed through PCIe DVSEC. In addition to this, the specification
defines an active bit which a device will expose through the same DVSEC
to notify system software that memory is initialized and ready.
While CXL 2.0 introduces a more powerful mechanism called HDM decoders
that are controlled by MMIO behind a PCIe BAR, the spec does allow the
1.1 style mapping to still be present. In such a case, when the CXL
driver takes over, if it were to enable HDM decoding and there was an
actively used range, things would likely blow up, in particular if it
wasn't an identical mapping.
This patch caches the relevant information which the cxl_mem driver will
need to make the proper decision and passes it along.
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
drivers/cxl/cxlmem.h | 19 +++++++
drivers/cxl/pci.c | 126 +++++++++++++++++++++++++++++++++++++++++++
drivers/cxl/pci.h | 13 +++++
3 files changed, 158 insertions(+)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 3ef3c652599e..eac5528ccaae 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -89,6 +89,22 @@ struct cxl_mbox_cmd {
*/
#define CXL_CAPACITY_MULTIPLIER SZ_256M
+/**
+ * struct cxl_endpoint_dvsec_info - Cached DVSEC info
+ * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
+ * @ranges: Number of HDM ranges this device contains.
+ * @range.base: cached value of the range base in the DVSEC, PCIE_DEVICE
+ * @range.size: cached value of the range size in the DVSEC, PCIE_DEVICE
+ */
+struct cxl_endpoint_dvsec_info {
+ bool mem_enabled;
+ int ranges;
+ struct {
+ u64 base;
+ u64 size;
+ } range[2];
+};
+
/**
* struct cxl_dev_state - The driver device state
*
@@ -117,6 +133,7 @@ struct cxl_mbox_cmd {
* @active_persistent_bytes: sum of hard + soft persistent
* @next_volatile_bytes: volatile capacity change pending device reset
* @next_persistent_bytes: persistent capacity change pending device reset
+ * @info: Cached DVSEC information about the device.
* @mbox_send: @dev specific transport for transmitting mailbox commands
*
* See section 8.2.9.5.2 Capacity Configuration and Label Storage for
@@ -147,6 +164,8 @@ struct cxl_dev_state {
u64 next_volatile_bytes;
u64 next_persistent_bytes;
+ struct cxl_endpoint_dvsec_info *info;
+
int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
};
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index f3872cbee7f8..b3f46045bf3e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -452,8 +452,126 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
return rc;
}
+#define CDPD(cxlds, which) \
+ cxlds->device_dvsec + CXL_DVSEC_PCIE_DEVICE_##which##_OFFSET
+
+#define CDPDR(cxlds, which, sorb, lohi) \
+ cxlds->device_dvsec + \
+ CXL_DVSEC_PCIE_DEVICE_RANGE_##sorb##_##lohi##_OFFSET(which)
+
+static int wait_for_valid(struct cxl_dev_state *cxlds)
+{
+ struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+ const unsigned long timeout = jiffies + HZ;
+ bool valid;
+
+ do {
+ u64 size;
+ u32 temp;
+ int rc;
+
+ rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, HIGH),
+ &temp);
+ if (rc)
+ return -ENXIO;
+ size = (u64)temp << 32;
+
+ rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, LOW),
+ &temp);
+ if (rc)
+ return -ENXIO;
+ size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
+
+ /*
+ * Memory_Info_Valid: When set, indicates that the CXL Range 1
+ * Size high and Size Low registers are valid. Must be set
+ * within 1 second of deassertion of reset to CXL device.
+ */
+ valid = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID, temp);
+ if (valid)
+ break;
+ cpu_relax();
+ } while (!time_after(jiffies, timeout));
+
+ return valid ? 0 : -ETIMEDOUT;
+}
+
+static struct cxl_endpoint_dvsec_info *dvsec_ranges(struct cxl_dev_state *cxlds)
+{
+ struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+ struct cxl_endpoint_dvsec_info *info;
+ int hdm_count, rc, i;
+ u16 cap, ctrl;
+
+ rc = pci_read_config_word(pdev, CDPD(cxlds, CAP), &cap);
+ if (rc)
+ return ERR_PTR(-ENXIO);
+ rc = pci_read_config_word(pdev, CDPD(cxlds, CTRL), &ctrl);
+ if (rc)
+ return ERR_PTR(-ENXIO);
+
+ if (!(cap & CXL_DVSEC_PCIE_DEVICE_MEM_CAPABLE))
+ return ERR_PTR(-ENODEV);
+
+ /*
+ * It is not allowed by spec for MEM.capable to be set and have 0 HDM
+ * decoders. Therefore, the device is not considered CXL.mem capable.
+ */
+ hdm_count = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_HDM_COUNT_MASK, cap);
+ if (!hdm_count || hdm_count > 2)
+ return ERR_PTR(-EINVAL);
+
+ rc = wait_for_valid(cxlds);
+ if (rc)
+ return ERR_PTR(rc);
+
+ info = devm_kzalloc(cxlds->dev, sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return ERR_PTR(-ENOMEM);
+
+ info->mem_enabled = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_ENABLE, ctrl);
+
+ for (i = 0; i < hdm_count; i++) {
+ u64 base, size;
+ u32 temp;
+
+ rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, SIZE, HIGH),
+ &temp);
+ if (rc)
+ continue;
+ size = (u64)temp << 32;
+
+ rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, SIZE, LOW),
+ &temp);
+ if (rc)
+ continue;
+ size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
+
+ rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, BASE, HIGH),
+ &temp);
+ if (rc)
+ continue;
+ base = (u64)temp << 32;
+
+ rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, BASE, LOW),
+ &temp);
+ if (rc)
+ continue;
+ base |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_BASE_LOW_MASK;
+
+ info->range[i].base = base;
+ info->range[i].size = size;
+ info->ranges++;
+ }
+
+ return info;
+}
+
+#undef CDPDR
+
static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
+ struct cxl_endpoint_dvsec_info *info;
struct cxl_register_map map;
struct cxl_memdev *cxlmd;
struct cxl_dev_state *cxlds;
@@ -505,6 +623,14 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (rc)
return rc;
+ info = dvsec_ranges(cxlds);
+ if (IS_ERR(info))
+ dev_err(&pdev->dev,
+ "Failed to get DVSEC range information (%ld)\n",
+ PTR_ERR(info));
+ else
+ cxlds->info = info;
+
cxlmd = devm_cxl_add_memdev(cxlds);
if (IS_ERR(cxlmd))
return PTR_ERR(cxlmd);
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index a4b506bb37d1..2a48cd65bf59 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -15,6 +15,19 @@
/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */
#define CXL_DVSEC_PCIE_DEVICE 0
+#define CXL_DVSEC_PCIE_DEVICE_CAP_OFFSET 0xA
+#define CXL_DVSEC_PCIE_DEVICE_MEM_CAPABLE BIT(2)
+#define CXL_DVSEC_PCIE_DEVICE_HDM_COUNT_MASK GENMASK(5, 4)
+#define CXL_DVSEC_PCIE_DEVICE_CTRL_OFFSET 0xC
+#define CXL_DVSEC_PCIE_DEVICE_MEM_ENABLE BIT(2)
+#define CXL_DVSEC_PCIE_DEVICE_RANGE_SIZE_HIGH_OFFSET(i) 0x18 + (i * 0x10)
+#define CXL_DVSEC_PCIE_DEVICE_RANGE_SIZE_LOW_OFFSET(i) 0x1C + (i * 0x10)
+#define CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID BIT(0)
+#define CXL_DVSEC_PCIE_DEVICE_MEM_ACTIVE BIT(1)
+#define CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK GENMASK(31, 28)
+#define CXL_DVSEC_PCIE_DEVICE_RANGE_BASE_HIGH_OFFSET(i) 0x20 + (i * 0x10)
+#define CXL_DVSEC_PCIE_DEVICE_RANGE_BASE_LOW_OFFSET(i) 0x24 + (i * 0x10)
+#define CXL_DVSEC_PCIE_DEVICE_MEM_BASE_LOW_MASK GENMASK(31, 28)
/* CXL 2.0 8.1.4: Non-CXL Function Map DVSEC */
#define CXL_DVSEC_FUNCTION_MAP 2
--
2.34.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 17/23] cxl: Cache and pass DVSEC ranges
2021-11-20 0:02 ` [PATCH 17/23] cxl: Cache and pass DVSEC ranges Ben Widawsky
@ 2021-11-20 4:29 ` kernel test robot
2021-11-22 17:00 ` Jonathan Cameron
2021-11-26 11:37 ` Jonathan Cameron
2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-11-20 4:29 UTC (permalink / raw)
To: Ben Widawsky, linux-cxl, linux-pci
Cc: llvm, kbuild-all, Ben Widawsky, Alison Schofield, Dan Williams,
Ira Weiny, Jonathan Cameron, Vishal Verma
[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]
Hi Ben,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 53989fad1286e652ea3655ae3367ba698da8d2ff]
url: https://github.com/0day-ci/linux/commits/Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
base: 53989fad1286e652ea3655ae3367ba698da8d2ff
config: riscv-buildonly-randconfig-r001-20211119 (attached as .config)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
git checkout cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/cxl/pci.c:469:7: warning: variable 'size' set but not used [-Wunused-but-set-variable]
u64 size;
^
1 warning generated.
vim +/size +469 drivers/cxl/pci.c
454
455 #define CDPD(cxlds, which) \
456 cxlds->device_dvsec + CXL_DVSEC_PCIE_DEVICE_##which##_OFFSET
457
458 #define CDPDR(cxlds, which, sorb, lohi) \
459 cxlds->device_dvsec + \
460 CXL_DVSEC_PCIE_DEVICE_RANGE_##sorb##_##lohi##_OFFSET(which)
461
462 static int wait_for_valid(struct cxl_dev_state *cxlds)
463 {
464 struct pci_dev *pdev = to_pci_dev(cxlds->dev);
465 const unsigned long timeout = jiffies + HZ;
466 bool valid;
467
468 do {
> 469 u64 size;
470 u32 temp;
471 int rc;
472
473 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, HIGH),
474 &temp);
475 if (rc)
476 return -ENXIO;
477 size = (u64)temp << 32;
478
479 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, LOW),
480 &temp);
481 if (rc)
482 return -ENXIO;
483 size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
484
485 /*
486 * Memory_Info_Valid: When set, indicates that the CXL Range 1
487 * Size high and Size Low registers are valid. Must be set
488 * within 1 second of deassertion of reset to CXL device.
489 */
490 valid = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID, temp);
491 if (valid)
492 break;
493 cpu_relax();
494 } while (!time_after(jiffies, timeout));
495
496 return valid ? 0 : -ETIMEDOUT;
497 }
498
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37677 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 17/23] cxl: Cache and pass DVSEC ranges
@ 2021-11-20 4:29 ` kernel test robot
0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-11-20 4:29 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3254 bytes --]
Hi Ben,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 53989fad1286e652ea3655ae3367ba698da8d2ff]
url: https://github.com/0day-ci/linux/commits/Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
base: 53989fad1286e652ea3655ae3367ba698da8d2ff
config: riscv-buildonly-randconfig-r001-20211119 (attached as .config)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
git checkout cfdf51e15fc8229a494ee59d05bc7459ab5eecd8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/cxl/pci.c:469:7: warning: variable 'size' set but not used [-Wunused-but-set-variable]
u64 size;
^
1 warning generated.
vim +/size +469 drivers/cxl/pci.c
454
455 #define CDPD(cxlds, which) \
456 cxlds->device_dvsec + CXL_DVSEC_PCIE_DEVICE_##which##_OFFSET
457
458 #define CDPDR(cxlds, which, sorb, lohi) \
459 cxlds->device_dvsec + \
460 CXL_DVSEC_PCIE_DEVICE_RANGE_##sorb##_##lohi##_OFFSET(which)
461
462 static int wait_for_valid(struct cxl_dev_state *cxlds)
463 {
464 struct pci_dev *pdev = to_pci_dev(cxlds->dev);
465 const unsigned long timeout = jiffies + HZ;
466 bool valid;
467
468 do {
> 469 u64 size;
470 u32 temp;
471 int rc;
472
473 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, HIGH),
474 &temp);
475 if (rc)
476 return -ENXIO;
477 size = (u64)temp << 32;
478
479 rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, LOW),
480 &temp);
481 if (rc)
482 return -ENXIO;
483 size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
484
485 /*
486 * Memory_Info_Valid: When set, indicates that the CXL Range 1
487 * Size high and Size Low registers are valid. Must be set
488 * within 1 second of deassertion of reset to CXL device.
489 */
490 valid = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID, temp);
491 if (valid)
492 break;
493 cpu_relax();
494 } while (!time_after(jiffies, timeout));
495
496 return valid ? 0 : -ETIMEDOUT;
497 }
498
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37677 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 17/23] cxl: Cache and pass DVSEC ranges
2021-11-20 0:02 ` [PATCH 17/23] cxl: Cache and pass DVSEC ranges Ben Widawsky
2021-11-20 4:29 ` kernel test robot
@ 2021-11-22 17:00 ` Jonathan Cameron
2021-11-22 22:50 ` Ben Widawsky
2021-11-26 11:37 ` Jonathan Cameron
2 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2021-11-22 17:00 UTC (permalink / raw)
To: Ben Widawsky
Cc: linux-cxl, linux-pci, Alison Schofield, Dan Williams, Ira Weiny,
Vishal Verma
On Fri, 19 Nov 2021 16:02:44 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:
> CXL 1.1 specification provided a mechanism for mapping an address space
> of a CXL device. That functionality is known as a "range" and can be
> programmed through PCIe DVSEC. In addition to this, the specification
> defines an active bit which a device will expose through the same DVSEC
> to notify system software that memory is initialized and ready.
>
> While CXL 2.0 introduces a more powerful mechanism called HDM decoders
> that are controlled by MMIO behind a PCIe BAR, the spec does allow the
> 1.1 style mapping to still be present. In such a case, when the CXL
> driver takes over, if it were to enable HDM decoding and there was an
> actively used range, things would likely blow up, in particular if it
> wasn't an identical mapping.
>
> This patch caches the relevant information which the cxl_mem driver will
> need to make the proper decision and passes it along.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
0-day spotted issues in same code as me. See below.
This is another case where I'd treat failure as fatal. Anything that fails
is either dead, or non spec compliant so don't bother loading the driver
if that happens. Fewer paths to test etc...
> ---
> drivers/cxl/cxlmem.h | 19 +++++++
> drivers/cxl/pci.c | 126 +++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/pci.h | 13 +++++
> 3 files changed, 158 insertions(+)
>
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 3ef3c652599e..eac5528ccaae 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -89,6 +89,22 @@ struct cxl_mbox_cmd {
> */
> #define CXL_CAPACITY_MULTIPLIER SZ_256M
>
> +/**
> + * struct cxl_endpoint_dvsec_info - Cached DVSEC info
> + * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
> + * @ranges: Number of HDM ranges this device contains.
> + * @range.base: cached value of the range base in the DVSEC, PCIE_DEVICE
> + * @range.size: cached value of the range size in the DVSEC, PCIE_DEVICE
> + */
> +struct cxl_endpoint_dvsec_info {
> + bool mem_enabled;
> + int ranges;
> + struct {
> + u64 base;
> + u64 size;
> + } range[2];
> +};
> +
> /**
> * struct cxl_dev_state - The driver device state
> *
> @@ -117,6 +133,7 @@ struct cxl_mbox_cmd {
> * @active_persistent_bytes: sum of hard + soft persistent
> * @next_volatile_bytes: volatile capacity change pending device reset
> * @next_persistent_bytes: persistent capacity change pending device reset
> + * @info: Cached DVSEC information about the device.
> * @mbox_send: @dev specific transport for transmitting mailbox commands
> *
> * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> @@ -147,6 +164,8 @@ struct cxl_dev_state {
> u64 next_volatile_bytes;
> u64 next_persistent_bytes;
>
> + struct cxl_endpoint_dvsec_info *info;
> +
> int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> };
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index f3872cbee7f8..b3f46045bf3e 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -452,8 +452,126 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> return rc;
> }
>
> +#define CDPD(cxlds, which) \
> + cxlds->device_dvsec + CXL_DVSEC_PCIE_DEVICE_##which##_OFFSET
My usual grumble :) I personally find macros like this a bit of a pain when
reviewing. I'd really like to see things spelled out in the code so I
can immediately see what register we are dealing with even if it does
seem rather repetitive in the code.
> +
> +#define CDPDR(cxlds, which, sorb, lohi) \
> + cxlds->device_dvsec + \
> + CXL_DVSEC_PCIE_DEVICE_RANGE_##sorb##_##lohi##_OFFSET(which)
> +
> +static int wait_for_valid(struct cxl_dev_state *cxlds)
> +{
> + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> + const unsigned long timeout = jiffies + HZ;
> + bool valid;
> +
> + do {
> + u64 size;
> + u32 temp;
> + int rc;
> +
> + rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, HIGH),
> + &temp);
> + if (rc)
> + return -ENXIO;
> + size = (u64)temp << 32;
> +
> + rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, LOW),
> + &temp);
> + if (rc)
> + return -ENXIO;
> + size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
> +
> + /*
> + * Memory_Info_Valid: When set, indicates that the CXL Range 1
> + * Size high and Size Low registers are valid. Must be set
> + * within 1 second of deassertion of reset to CXL device.
> + */
> + valid = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID, temp);
> + if (valid)
> + break;
I think there is a race here. What if you read the high part, get garbage and then
read the low part which is now valid...
Swap this around so you read this one first and it will be fine.
However given as 0-day pointed out, size isn't used, this is fine anyway
(subject to removing the pointless code).
> + cpu_relax();
> + } while (!time_after(jiffies, timeout));
> +
> + return valid ? 0 : -ETIMEDOUT;
> +}
> +
> +static struct cxl_endpoint_dvsec_info *dvsec_ranges(struct cxl_dev_state *cxlds)
> +{
> + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> + struct cxl_endpoint_dvsec_info *info;
> + int hdm_count, rc, i;
> + u16 cap, ctrl;
> +
> + rc = pci_read_config_word(pdev, CDPD(cxlds, CAP), &cap);
> + if (rc)
> + return ERR_PTR(-ENXIO);
> + rc = pci_read_config_word(pdev, CDPD(cxlds, CTRL), &ctrl);
> + if (rc)
> + return ERR_PTR(-ENXIO);
> +
> + if (!(cap & CXL_DVSEC_PCIE_DEVICE_MEM_CAPABLE))
> + return ERR_PTR(-ENODEV);
> +
> + /*
> + * It is not allowed by spec for MEM.capable to be set and have 0 HDM
> + * decoders. Therefore, the device is not considered CXL.mem capable.
> + */
> + hdm_count = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_HDM_COUNT_MASK, cap);
> + if (!hdm_count || hdm_count > 2)
> + return ERR_PTR(-EINVAL);
> +
> + rc = wait_for_valid(cxlds);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + info = devm_kzalloc(cxlds->dev, sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return ERR_PTR(-ENOMEM);
> +
> + info->mem_enabled = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_ENABLE, ctrl);
> +
> + for (i = 0; i < hdm_count; i++) {
> + u64 base, size;
> + u32 temp;
> +
> + rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, SIZE, HIGH),
> + &temp);
> + if (rc)
> + continue;
> + size = (u64)temp << 32;
> +
> + rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, SIZE, LOW),
> + &temp);
> + if (rc)
> + continue;
> + size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
> +
> + rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, BASE, HIGH),
> + &temp);
> + if (rc)
> + continue;
> + base = (u64)temp << 32;
> +
> + rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, BASE, LOW),
> + &temp);
> + if (rc)
> + continue;
> + base |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_BASE_LOW_MASK;
> +
> + info->range[i].base = base;
> + info->range[i].size = size;
> + info->ranges++;
> + }
> +
> + return info;
> +}
> +
> +#undef CDPDR
> +
> static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> + struct cxl_endpoint_dvsec_info *info;
> struct cxl_register_map map;
> struct cxl_memdev *cxlmd;
> struct cxl_dev_state *cxlds;
> @@ -505,6 +623,14 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> if (rc)
> return rc;
>
> + info = dvsec_ranges(cxlds);
> + if (IS_ERR(info))
> + dev_err(&pdev->dev,
> + "Failed to get DVSEC range information (%ld)\n",
> + PTR_ERR(info));
> + else
> + cxlds->info = info;
> +
> cxlmd = devm_cxl_add_memdev(cxlds);
> if (IS_ERR(cxlmd))
> return PTR_ERR(cxlmd);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 17/23] cxl: Cache and pass DVSEC ranges
2021-11-22 17:00 ` Jonathan Cameron
@ 2021-11-22 22:50 ` Ben Widawsky
0 siblings, 0 replies; 9+ messages in thread
From: Ben Widawsky @ 2021-11-22 22:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-cxl, linux-pci, Alison Schofield, Dan Williams, Ira Weiny,
Vishal Verma
On 21-11-22 17:00:56, Jonathan Cameron wrote:
> On Fri, 19 Nov 2021 16:02:44 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> > CXL 1.1 specification provided a mechanism for mapping an address space
> > of a CXL device. That functionality is known as a "range" and can be
> > programmed through PCIe DVSEC. In addition to this, the specification
> > defines an active bit which a device will expose through the same DVSEC
> > to notify system software that memory is initialized and ready.
> >
> > While CXL 2.0 introduces a more powerful mechanism called HDM decoders
> > that are controlled by MMIO behind a PCIe BAR, the spec does allow the
> > 1.1 style mapping to still be present. In such a case, when the CXL
> > driver takes over, if it were to enable HDM decoding and there was an
> > actively used range, things would likely blow up, in particular if it
> > wasn't an identical mapping.
> >
> > This patch caches the relevant information which the cxl_mem driver will
> > need to make the proper decision and passes it along.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>
> 0-day spotted issues in same code as me. See below.
>
> This is another case where I'd treat failure as fatal. Anything that fails
> is either dead, or non spec compliant so don't bother loading the driver
> if that happens. Fewer paths to test etc...
I disagree about treating failure as fatal. The failure here only forbids using
the memory on the device. I can envision firmware bugs or the like where these
things might fail, but the mailbox interfaces still work perfectly fine, or at
least fine enough to upgrade the firmware and try again.
Dan and I had talked about a modparam (or perhaps sysfs on some higher level,
like CXL bus) to control the length of the timeout, so that one doesn't have to
wait forever to deal with this usage. Figured we'd cross that bridge when we
came to it.
>
> > ---
> > drivers/cxl/cxlmem.h | 19 +++++++
> > drivers/cxl/pci.c | 126 +++++++++++++++++++++++++++++++++++++++++++
> > drivers/cxl/pci.h | 13 +++++
> > 3 files changed, 158 insertions(+)
> >
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 3ef3c652599e..eac5528ccaae 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -89,6 +89,22 @@ struct cxl_mbox_cmd {
> > */
> > #define CXL_CAPACITY_MULTIPLIER SZ_256M
> >
> > +/**
> > + * struct cxl_endpoint_dvsec_info - Cached DVSEC info
> > + * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
> > + * @ranges: Number of HDM ranges this device contains.
> > + * @range.base: cached value of the range base in the DVSEC, PCIE_DEVICE
> > + * @range.size: cached value of the range size in the DVSEC, PCIE_DEVICE
> > + */
> > +struct cxl_endpoint_dvsec_info {
> > + bool mem_enabled;
> > + int ranges;
> > + struct {
> > + u64 base;
> > + u64 size;
> > + } range[2];
> > +};
> > +
> > /**
> > * struct cxl_dev_state - The driver device state
> > *
> > @@ -117,6 +133,7 @@ struct cxl_mbox_cmd {
> > * @active_persistent_bytes: sum of hard + soft persistent
> > * @next_volatile_bytes: volatile capacity change pending device reset
> > * @next_persistent_bytes: persistent capacity change pending device reset
> > + * @info: Cached DVSEC information about the device.
> > * @mbox_send: @dev specific transport for transmitting mailbox commands
> > *
> > * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> > @@ -147,6 +164,8 @@ struct cxl_dev_state {
> > u64 next_volatile_bytes;
> > u64 next_persistent_bytes;
> >
> > + struct cxl_endpoint_dvsec_info *info;
> > +
> > int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> > };
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index f3872cbee7f8..b3f46045bf3e 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -452,8 +452,126 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> > return rc;
> > }
> >
> > +#define CDPD(cxlds, which) \
> > + cxlds->device_dvsec + CXL_DVSEC_PCIE_DEVICE_##which##_OFFSET
>
> My usual grumble :) I personally find macros like this a bit of a pain when
> reviewing. I'd really like to see things spelled out in the code so I
> can immediately see what register we are dealing with even if it does
> seem rather repetitive in the code.
I understand. It's this or have every line look strange because of character
limits. I agree that whomever writes this stuff can reason it out better, and
that makes it harder on reviewers. I don't mind changing it back, I'd like hear
any other opinions before I do though.
>
> > +
> > +#define CDPDR(cxlds, which, sorb, lohi) \
> > + cxlds->device_dvsec + \
> > + CXL_DVSEC_PCIE_DEVICE_RANGE_##sorb##_##lohi##_OFFSET(which)
> > +
> > +static int wait_for_valid(struct cxl_dev_state *cxlds)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> > + const unsigned long timeout = jiffies + HZ;
> > + bool valid;
> > +
> > + do {
> > + u64 size;
> > + u32 temp;
> > + int rc;
> > +
> > + rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, HIGH),
> > + &temp);
> > + if (rc)
> > + return -ENXIO;
> > + size = (u64)temp << 32;
> > +
> > + rc = pci_read_config_dword(pdev, CDPDR(cxlds, 0, SIZE, LOW),
> > + &temp);
> > + if (rc)
> > + return -ENXIO;
> > + size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
> > +
> > + /*
> > + * Memory_Info_Valid: When set, indicates that the CXL Range 1
> > + * Size high and Size Low registers are valid. Must be set
> > + * within 1 second of deassertion of reset to CXL device.
> > + */
> > + valid = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_INFO_VALID, temp);
> > + if (valid)
> > + break;
>
> I think there is a race here. What if you read the high part, get garbage and then
> read the low part which is now valid...
>
> Swap this around so you read this one first and it will be fine.
>
> However given as 0-day pointed out, size isn't used, this is fine anyway
> (subject to removing the pointless code).
>
Yes. I've fixed that. And you're right, size is potentially invalid in this case
even if it found the valid bit.
> > + cpu_relax();
> > + } while (!time_after(jiffies, timeout));
> > +
> > + return valid ? 0 : -ETIMEDOUT;
> > +}
> > +
> > +static struct cxl_endpoint_dvsec_info *dvsec_ranges(struct cxl_dev_state *cxlds)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> > + struct cxl_endpoint_dvsec_info *info;
> > + int hdm_count, rc, i;
> > + u16 cap, ctrl;
> > +
> > + rc = pci_read_config_word(pdev, CDPD(cxlds, CAP), &cap);
> > + if (rc)
> > + return ERR_PTR(-ENXIO);
> > + rc = pci_read_config_word(pdev, CDPD(cxlds, CTRL), &ctrl);
> > + if (rc)
> > + return ERR_PTR(-ENXIO);
> > +
> > + if (!(cap & CXL_DVSEC_PCIE_DEVICE_MEM_CAPABLE))
> > + return ERR_PTR(-ENODEV);
> > +
> > + /*
> > + * It is not allowed by spec for MEM.capable to be set and have 0 HDM
> > + * decoders. Therefore, the device is not considered CXL.mem capable.
> > + */
> > + hdm_count = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_HDM_COUNT_MASK, cap);
> > + if (!hdm_count || hdm_count > 2)
> > + return ERR_PTR(-EINVAL);
> > +
> > + rc = wait_for_valid(cxlds);
> > + if (rc)
> > + return ERR_PTR(rc);
> > +
> > + info = devm_kzalloc(cxlds->dev, sizeof(*info), GFP_KERNEL);
> > + if (!info)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + info->mem_enabled = FIELD_GET(CXL_DVSEC_PCIE_DEVICE_MEM_ENABLE, ctrl);
> > +
> > + for (i = 0; i < hdm_count; i++) {
> > + u64 base, size;
> > + u32 temp;
> > +
> > + rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, SIZE, HIGH),
> > + &temp);
> > + if (rc)
> > + continue;
> > + size = (u64)temp << 32;
> > +
> > + rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, SIZE, LOW),
> > + &temp);
> > + if (rc)
> > + continue;
> > + size |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_SIZE_LOW_MASK;
> > +
> > + rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, BASE, HIGH),
> > + &temp);
> > + if (rc)
> > + continue;
> > + base = (u64)temp << 32;
> > +
> > + rc = pci_read_config_dword(pdev, CDPDR(cxlds, i, BASE, LOW),
> > + &temp);
> > + if (rc)
> > + continue;
> > + base |= temp & CXL_DVSEC_PCIE_DEVICE_MEM_BASE_LOW_MASK;
> > +
> > + info->range[i].base = base;
> > + info->range[i].size = size;
> > + info->ranges++;
> > + }
> > +
> > + return info;
> > +}
> > +
> > +#undef CDPDR
> > +
> > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > {
> > + struct cxl_endpoint_dvsec_info *info;
> > struct cxl_register_map map;
> > struct cxl_memdev *cxlmd;
> > struct cxl_dev_state *cxlds;
> > @@ -505,6 +623,14 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > if (rc)
> > return rc;
> >
> > + info = dvsec_ranges(cxlds);
> > + if (IS_ERR(info))
> > + dev_err(&pdev->dev,
> > + "Failed to get DVSEC range information (%ld)\n",
> > + PTR_ERR(info));
> > + else
> > + cxlds->info = info;
> > +
> > cxlmd = devm_cxl_add_memdev(cxlds);
> > if (IS_ERR(cxlmd))
> > return PTR_ERR(cxlmd);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 17/23] cxl: Cache and pass DVSEC ranges
2021-11-20 0:02 ` [PATCH 17/23] cxl: Cache and pass DVSEC ranges Ben Widawsky
2021-11-20 4:29 ` kernel test robot
2021-11-22 17:00 ` Jonathan Cameron
@ 2021-11-26 11:37 ` Jonathan Cameron
2 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2021-11-26 11:37 UTC (permalink / raw)
To: Ben Widawsky
Cc: linux-cxl, linux-pci, Alison Schofield, Dan Williams, Ira Weiny,
Vishal Verma
On Fri, 19 Nov 2021 16:02:44 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:
> CXL 1.1 specification provided a mechanism for mapping an address space
> of a CXL device. That functionality is known as a "range" and can be
> programmed through PCIe DVSEC. In addition to this, the specification
> defines an active bit which a device will expose through the same DVSEC
> to notify system software that memory is initialized and ready.
>
> While CXL 2.0 introduces a more powerful mechanism called HDM decoders
> that are controlled by MMIO behind a PCIe BAR, the spec does allow the
> 1.1 style mapping to still be present. In such a case, when the CXL
> driver takes over, if it were to enable HDM decoding and there was an
> actively used range, things would likely blow up, in particular if it
> wasn't an identical mapping.
>
> This patch caches the relevant information which the cxl_mem driver will
> need to make the proper decision and passes it along.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
> drivers/cxl/cxlmem.h | 19 +++++++
> drivers/cxl/pci.c | 126 +++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/pci.h | 13 +++++
> 3 files changed, 158 insertions(+)
>
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 3ef3c652599e..eac5528ccaae 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -89,6 +89,22 @@ struct cxl_mbox_cmd {
> */
> #define CXL_CAPACITY_MULTIPLIER SZ_256M
>
> +/**
> + * struct cxl_endpoint_dvsec_info - Cached DVSEC info
> + * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
> + * @ranges: Number of HDM ranges this device contains.
> + * @range.base: cached value of the range base in the DVSEC, PCIE_DEVICE
> + * @range.size: cached value of the range size in the DVSEC, PCIE_DEVICE
> + */
> +struct cxl_endpoint_dvsec_info {
> + bool mem_enabled;
> + int ranges;
> + struct {
> + u64 base;
> + u64 size;
> + } range[2];
kernel-doc wants documentation for range as well.
> +};
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-29 9:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-27 18:35 [PATCH 17/23] cxl: Cache and pass DVSEC ranges kernel test robot
2021-11-29 9:39 ` kernel test robot
2021-11-29 9:39 ` kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2021-11-20 0:02 [PATCH 00/23] Add drivers for CXL ports and mem devices Ben Widawsky
2021-11-20 0:02 ` [PATCH 17/23] cxl: Cache and pass DVSEC ranges Ben Widawsky
2021-11-20 4:29 ` kernel test robot
2021-11-20 4:29 ` kernel test robot
2021-11-22 17:00 ` Jonathan Cameron
2021-11-22 22:50 ` Ben Widawsky
2021-11-26 11:37 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.