diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 677230f..7d606e5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,8 +43,6 @@ jobs: sudo apt-get update sudo apt-get install -y build-essential clang llvm linux-headers-$(uname -r) - # A stock kernel has no pci_mmcfg_read_config() yet, so this exercises - # the compat/ shim path. The in-tree job below covers the real thing. - name: Build module run: make W=1 KCFLAGS=-W ${{ matrix.make_args }} @@ -69,7 +67,7 @@ jobs: if-no-files-found: error in-tree: - name: Current i2c-host integration (2-patch series) + name: Current i2c-host integration runs-on: ubuntu-latest timeout-minutes: 45 @@ -91,12 +89,6 @@ jobs: - name: Prepare current maintainer tree run: | git clone --depth=1 --branch i2c/i2c-host "$I2C_TREE" linux-i2c - # Patch 1/2: the PCI/x86 accessor the driver depends on. If this - # stops applying, the tracked copy is stale and must be refreshed - # from the kernel tree - that is a real failure, not CI noise. - git -C linux-i2c apply --verbose \ - ../docs/submission/0001-pci-mmcfg-accessor.patch - # Patch 2/2: the driver itself. cp i2c-imc-skylake.c linux-i2c/drivers/i2c/busses/ cat docs/submission/Kconfig.kernel >> linux-i2c/drivers/i2c/busses/Kconfig printf '%s\n' 'obj-$(CONFIG_I2C_IMC_SKYLAKE) += i2c-imc-skylake.o' \ @@ -110,15 +102,10 @@ jobs: scripts/config --file ../linux-out/.config --enable I2C --module I2C_IMC_SKYLAKE make O=../linux-out olddefconfig grep '^CONFIG_I2C_IMC_SKYLAKE=m' ../linux-out/.config - grep '^CONFIG_PCI_MMCONFIG=y' ../linux-out/.config - - name: Build the series in-tree + - name: Build driver in-tree working-directory: linux-i2c - run: | - make O=../linux-out W=1 \ - drivers/pci/access.o \ - arch/x86/pci/mmconfig-shared.o \ - drivers/i2c/busses/i2c-imc-skylake.o + run: make O=../linux-out W=1 drivers/i2c/busses/i2c-imc-skylake.o - name: Build arm64 COMPILE_TEST with Clang working-directory: linux-i2c @@ -130,15 +117,12 @@ jobs: make O=../linux-out-arm64 ARCH=arm64 LLVM=1 olddefconfig grep '^CONFIG_I2C_IMC_SKYLAKE=m' ../linux-out-arm64/.config make O=../linux-out-arm64 ARCH=arm64 LLVM=1 W=1 \ - drivers/pci/access.o \ drivers/i2c/busses/i2c-imc-skylake.o - name: Run Sparse in-tree working-directory: linux-i2c run: | make O=../linux-out C=2 CF=-D__CHECK_ENDIAN__ \ - drivers/pci/access.o \ - arch/x86/pci/mmconfig-shared.o \ drivers/i2c/busses/i2c-imc-skylake.o - name: Run current-tree checkpatch @@ -146,8 +130,6 @@ jobs: run: | scripts/checkpatch.pl --strict --no-tree -f \ drivers/i2c/busses/i2c-imc-skylake.c - scripts/checkpatch.pl --strict \ - ../docs/submission/0001-pci-mmcfg-accessor.patch - name: Build the documentation working-directory: linux-i2c diff --git a/Makefile b/Makefile index 999298c..2429bcf 100644 --- a/Makefile +++ b/Makefile @@ -6,17 +6,6 @@ INSTALL_MOD_PATH ?= $(DESTDIR) INSTALL_MOD_DIR ?= updates PWD := $(shell pwd) -# The driver calls pci_mmcfg_{read,write}_config(), added by patch 1/2 of the -# upstream series. Against a kernel that does not have them yet, force-include -# the out-of-tree shim so the driver source stays identical to the submitted -# patch. See compat/pci-mmcfg-compat.h. -IMC_NEED_COMPAT := $(shell grep -qs pci_mmcfg_read_config \ - $(srctree)/include/linux/pci.h || echo y) -ifeq ($(IMC_NEED_COMPAT),y) -ccflags-y += -include $(src)/compat/pci-mmcfg-compat.h -$(warning i2c-imc-skylake: target kernel lacks pci_mmcfg_read_config(); building with the out-of-tree compat shim) -endif - all: $(MAKE) -C $(KDIR) M=$(PWD) modules diff --git a/README.md b/README.md index 8dbb1e8..da3e1d4 100644 --- a/README.md +++ b/README.md @@ -31,14 +31,8 @@ Patch series v2 is archived on [lore.kernel.org](https://lore.kernel.org/linux-i2c/20260620144131.415559-1-simone.chifari@gmail.com/T/#t) and received no replies. -The next posting is an RFC in **two patches**: - -1. a PCI/x86 change adding `pci_mmcfg_read_config()` / `pci_mmcfg_write_config()`, - which force config access through the memory-mapped (ECAM) path; -2. this driver, which uses them. - -The split exists because those are two decisions belonging to two different -sets of maintainers. See [docs/submission/cover-letter.txt](docs/submission/cover-letter.txt). +The next posting is `[PATCH v4]`, a single patch. See +[docs/submission/cover-letter.txt](docs/submission/cover-letter.txt). **No iMC I2C driver has ever been merged into mainline.** Andy Lutomirski's `i2c_imc` (2013-2016) and Stefan Schaeckeler's 2020 Broadwell rewrite both @@ -47,21 +41,29 @@ either; it detects it. See the cover letter for what that means in practice. --- -## Why the ECAM path +## Retracted: the CF8/CFC claim -The engine's registers are at config offsets `0x9C`-`0xB8`, inside the first -256 bytes. On x86 that range is dispatched to `raw_pci_ops`, which is CF8/CFC -whenever configuration type 1 is usable; the memory-mapped path is only used -from `0x100` upwards. On the tested X299 board a dword written through CF8/CFC -to the command register does not take effect: the register reads back -unchanged, the GO bit is never consumed, no transaction is issued. The same -write through ECAM works. +Up to v3 this project claimed that firmware filters CF8/CFC config writes to +the PCU function, and that the registers were reachable only through the +memory-mapped window. **That was wrong.** It was never tested directly; the +only evidence was a boot log line naming the access method. -What drops the write is **not known**. Earlier revisions of this project -attributed it to System Management Mode; the only evidence was a boot log line -naming the access method, which says nothing about who filters the write, so -that claim has been removed. [tools/probe-cf8-write.sh](tools/probe-cf8-write.sh) -is there to settle it with an `MSR_SMI_COUNT` measurement. +Measured on this board, driver unloaded: + +```console +# setpci -s 16:1e.5 9c.L=20085002 # read SPD 0x50, register 0x02 +# setpci -s 16:1e.5 9c.L +20005002 # GO consumed +# setpci -s 16:1e.5 a8.L +0000500c # READ_DONE, no error +# setpci -s 16:1e.5 b4.L +0000000c # SPD byte 2: DDR4 type code +``` + +The command executes and returns correct data through the ordinary accessors, +and `MSR_SMI_COUNT` does not move across it. The driver now uses +`pci_read_config_dword()` and `pci_write_config_dword()` like any other driver. +Reproduce with [tools/probe-cf8-transaction.sh](tools/probe-cf8-transaction.sh). --- @@ -73,9 +75,9 @@ is there to settle it with an `MSR_SMI_COUNT` measurement. of the transfer, restores both afterwards; * **compares the command registers after every transfer** against what it left in them, and fails with `-EAGAIN` if anything else changed; -* requires a positive sign that the engine accepted the command, because the - done bits are latched and would otherwise let a never-executed command look - like a completed one. +* reads the command word back and requires it to be the one just written, + because the done bits are latched and a stale DONE would otherwise let a + command that never reached the register look like a completed transfer. If your log shows @@ -125,14 +127,6 @@ make reload ALLOW_UNSAFE=1 # load for testing, after checking arbitration sudo make install # install into the kernel tree ``` -The driver calls `pci_mmcfg_read_config()`, which is added by patch 1 of the -upstream series and is not in any released kernel yet. When the target kernel -lacks it, the build force-includes -[compat/pci-mmcfg-compat.h](compat/pci-mmcfg-compat.h), which reimplements the -two accessors out-of-tree so the driver source stays byte-identical to the -submitted patch. The build prints a warning when the shim is in use. The shim -is **not** part of the upstream submission. - To permit a later `modprobe` or PCI modalias load: ```bash diff --git a/compat/pci-mmcfg-compat.h b/compat/pci-mmcfg-compat.h deleted file mode 100644 index 77a5072..0000000 --- a/compat/pci-mmcfg-compat.h +++ /dev/null @@ -1,147 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* - * Out-of-tree compatibility shim - NOT part of the upstream submission. - * - * i2c-imc-skylake.c reaches the PCU function's config space with - * pci_mmcfg_read_config() / pci_mmcfg_write_config(), which are added by - * patch 1/2 of the series ("PCI: provide config access forced through the - * ECAM path"). A stock kernel does not have them yet. - * - * The Makefile force-includes this header when the target kernel lacks those - * symbols, so the driver source stays byte-identical to the submitted patch - * while remaining testable on an unpatched kernel. It reimplements the two - * accessors the way earlier revisions of the driver did it: locate the ECAM - * base in the ACPI MCFG table and map the function's config page. - * - * Limitations, acceptable for a test shim and for nothing else: - * - a single device at a time (there is one PCU function per system); - * - dword accesses only, which is all the driver issues; - * - no participation in the PCI core's config-space locking. That is - * precisely the problem patch 1/2 exists to solve. - */ - -#ifndef IMC_PCI_MMCFG_COMPAT_H -#define IMC_PCI_MMCFG_COMPAT_H - -#include -#include -#include -#include - -#define IMC_COMPAT_CFG_SIZE 0x1000UL - -static void __iomem *imc_compat_cfg; -static resource_size_t imc_compat_phys; - -static u64 imc_compat_mmcfg_base(struct pci_dev *pdev) -{ - unsigned int seg = pci_domain_nr(pdev->bus); - unsigned int bus = pdev->bus->number; - struct acpi_table_header *hdr; - struct acpi_table_mcfg *mcfg; - struct acpi_mcfg_allocation *e; - unsigned long n, i; - u64 base = 0; - - if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MCFG, 0, &hdr))) - return 0; - - mcfg = (struct acpi_table_mcfg *)hdr; - if (hdr->length < sizeof(*mcfg)) { - acpi_put_table(hdr); - return 0; - } - e = (struct acpi_mcfg_allocation *)(mcfg + 1); - n = (hdr->length - sizeof(*mcfg)) / sizeof(*e); - for (i = 0; i < n; i++) { - if (e[i].pci_segment == seg && - bus >= e[i].start_bus_number && - bus <= e[i].end_bus_number) { - base = e[i].address; - break; - } - } - acpi_put_table(hdr); - - return base; -} - -static void imc_compat_unmap(void *data) -{ - iounmap(imc_compat_cfg); - imc_compat_cfg = NULL; - imc_compat_phys = 0; -} - -static void __iomem *imc_compat_map(struct pci_dev *pdev) -{ - resource_size_t phys; - u64 base; - - base = imc_compat_mmcfg_base(pdev); - if (!base) - return NULL; - - phys = base + - ((resource_size_t)pdev->bus->number << 20) + - ((resource_size_t)PCI_SLOT(pdev->devfn) << 15) + - ((resource_size_t)PCI_FUNC(pdev->devfn) << 12); - - if (phys < base || phys + IMC_COMPAT_CFG_SIZE < phys) - return NULL; - - if (imc_compat_cfg && imc_compat_phys == phys) - return imc_compat_cfg; - if (imc_compat_cfg) - return NULL; /* a second device would need a real cache */ - - imc_compat_cfg = ioremap_uc(phys, IMC_COMPAT_CFG_SIZE); - if (!imc_compat_cfg) - return NULL; - imc_compat_phys = phys; - - if (devm_add_action_or_reset(&pdev->dev, imc_compat_unmap, NULL)) - return NULL; - - return imc_compat_cfg; -} - -static inline int pci_mmcfg_read_config(struct pci_dev *dev, int where, - int size, u32 *val) -{ - void __iomem *cfg; - - if (size != 4 || (where & 3) || where + size > 0x1000) { - *val = ~0U; - return -EINVAL; - } - - cfg = imc_compat_map(dev); - if (!cfg) { - *val = ~0U; - return -ENXIO; - } - - *val = readl(cfg + where); - - return 0; -} - -static inline int pci_mmcfg_write_config(struct pci_dev *dev, int where, - int size, u32 val) -{ - void __iomem *cfg; - - if (size != 4 || (where & 3) || where + size > 0x1000) - return -EINVAL; - - cfg = imc_compat_map(dev); - if (!cfg) - return -ENXIO; - - writel(val, cfg + where); - - return 0; -} - -#endif /* IMC_PCI_MMCFG_COMPAT_H */ diff --git a/docs/submission/0001-pci-mmcfg-accessor.patch b/docs/submission/0001-pci-mmcfg-accessor.patch deleted file mode 100644 index 78830c0..0000000 --- a/docs/submission/0001-pci-mmcfg-accessor.patch +++ /dev/null @@ -1,204 +0,0 @@ -From 2bde3ce8de136654c563b0bc0b05847293bf15e5 Mon Sep 17 00:00:00 2001 -From: Simone Chifari -Date: Sun, 26 Jul 2026 19:24:49 +0200 -Subject: [RFC PATCH v4 1/2] PCI: allow config access forced through the - memory-mapped path - -On x86, raw_pci_read() and raw_pci_write() dispatch the first 256 bytes -of config space to raw_pci_ops, which is CF8/CFC whenever configuration -type 1 is usable. raw_pci_ext_ops, the memory-mapped (ECAM) path, is -only consulted from offset 0x100 upwards. - -A platform that filters or drops CF8/CFC accesses to one function -therefore makes that function's low config space unreachable through the -normal accessors, even though the ECAM window reaches it. This is -observable on Intel Skylake-X boards: a dword written to the iMC SMBus -command registers at offsets 0x9C-0xB8 of the PCU function 8086:2085 -does not take effect, the GO bit is never consumed and no transaction is -issued, while the same write through ECAM works. Everything else that -reads or writes that function's low config space, including setpci and -sysfs, is equally affected. - -Add pci_mmcfg_read_config() and pci_mmcfg_write_config(). They take -pci_lock and honour pci_cfg_access_lock() like the other config -accessors, on top of a weak arch_pci_mmcfg_access() hook. x86 -implements the hook over the existing pci_mmcfg_list, so no MCFG table -is re-parsed and no window that the architecture has already mapped is -mapped a second time. Architectures with no memory-mapped config space -keep the default, which reports -ENXIO. - -These do not replace pci_read_config_dword() and friends. They are -meant for a device documented to be unreachable otherwise, and callers -are expected to justify the use in a comment. - -Signed-off-by: Simone Chifari ---- - arch/x86/pci/mmconfig-shared.c | 28 +++++++++++ - drivers/pci/access.c | 89 ++++++++++++++++++++++++++++++++++ - include/linux/pci.h | 11 +++++ - 3 files changed, 128 insertions(+) - -diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c -index acdb8dcae..715fdcdae 100644 ---- a/arch/x86/pci/mmconfig-shared.c -+++ b/arch/x86/pci/mmconfig-shared.c -@@ -779,6 +779,34 @@ static int __init pci_mmcfg_late_insert_resources(void) - */ - late_initcall(pci_mmcfg_late_insert_resources); - -+/* -+ * raw_pci_read()/raw_pci_write() dispatch the first 256 config bytes to -+ * raw_pci_ops, which is CF8/CFC whenever configuration type 1 is usable. -+ * A platform that filters CF8/CFC for one function therefore makes that -+ * function's low config space unreachable through the normal accessors, -+ * even though the ECAM window reaches it. Give the PCI core a way to -+ * select the ECAM path explicitly for such a device. -+ * -+ * See the pci_mmcfg_read_config() kerneldoc for the usage restrictions. -+ */ -+int arch_pci_mmcfg_access(struct pci_dev *dev, int where, int size, u32 *val, -+ bool write) -+{ -+ unsigned int seg = pci_domain_nr(dev->bus); -+ unsigned int bus = dev->bus->number; -+ int ret; -+ -+ if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed) -+ return -ENXIO; -+ -+ if (write) -+ ret = pci_mmcfg.write(seg, bus, dev->devfn, where, size, *val); -+ else -+ ret = pci_mmcfg.read(seg, bus, dev->devfn, where, size, val); -+ -+ return ret < 0 ? ret : 0; -+} -+ - /* Add ECAM information for host bridges */ - int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end, - phys_addr_t addr) -diff --git a/drivers/pci/access.c b/drivers/pci/access.c -index b123da16b..e058b9f2b 100644 ---- a/drivers/pci/access.c -+++ b/drivers/pci/access.c -@@ -277,6 +277,95 @@ PCI_USER_WRITE_CONFIG(byte, u8) - PCI_USER_WRITE_CONFIG(word, u16) - PCI_USER_WRITE_CONFIG(dword, u32) - -+/* -+ * Architecture hook for config access forced through the memory-mapped -+ * (ECAM) path. Called with @pci_lock held and interrupts disabled. -+ * Architectures without a memory-mapped config space leave the default, -+ * which reports that the path does not exist. -+ */ -+int __weak arch_pci_mmcfg_access(struct pci_dev *dev, int where, int size, -+ u32 *val, bool write) -+{ -+ return -ENXIO; -+} -+ -+static int pci_mmcfg_access(struct pci_dev *dev, int where, int size, u32 *val, -+ bool write) -+{ -+ int ret; -+ -+ if (size != 1 && size != 2 && size != 4) -+ return -EINVAL; -+ if (where & (size - 1)) -+ return -EINVAL; -+ if (where + size > dev->cfg_size) -+ return -EINVAL; -+ if (pci_dev_is_disconnected(dev)) -+ return -ENODEV; -+ -+ raw_spin_lock_irq(&pci_lock); -+ if (unlikely(dev->block_cfg_access)) -+ pci_wait_cfg(dev); -+ ret = arch_pci_mmcfg_access(dev, where, size, val, write); -+ raw_spin_unlock_irq(&pci_lock); -+ -+ return ret; -+} -+ -+/** -+ * pci_mmcfg_read_config - read config space through the ECAM path -+ * @dev: PCI device -+ * @where: byte offset in config space, naturally aligned for @size -+ * @size: 1, 2 or 4 bytes -+ * @val: where to store the value read -+ * -+ * Read config space of @dev through the memory-mapped (ECAM) path -+ * regardless of the access method the platform selected for the first 256 -+ * config bytes. On x86 that method is CF8/CFC, and some platforms filter -+ * or trap those accesses for individual functions, which makes the -+ * registers of such a function unreachable through the normal accessors. -+ * -+ * Drivers should use pci_read_config_dword() and friends. This is only -+ * for a device documented to be unreachable otherwise, and callers must -+ * be able to justify why in a comment. -+ * -+ * Must be called from a context that may sleep. -+ * -+ * Return: 0 on success, -ENXIO when the platform has no ECAM path, -+ * otherwise a negative errno. @val is set to all ones on error. -+ */ -+int pci_mmcfg_read_config(struct pci_dev *dev, int where, int size, u32 *val) -+{ -+ int ret; -+ -+ *val = 0; -+ ret = pci_mmcfg_access(dev, where, size, val, false); -+ if (ret) -+ *val = ~0; -+ -+ return ret; -+} -+EXPORT_SYMBOL_GPL(pci_mmcfg_read_config); -+ -+/** -+ * pci_mmcfg_write_config - write config space through the ECAM path -+ * @dev: PCI device -+ * @where: byte offset in config space, naturally aligned for @size -+ * @size: 1, 2 or 4 bytes -+ * @val: value to write -+ * -+ * Write counterpart of pci_mmcfg_read_config(). The same restrictions -+ * apply; see that function. -+ * -+ * Return: 0 on success, -ENXIO when the platform has no ECAM path, -+ * otherwise a negative errno. -+ */ -+int pci_mmcfg_write_config(struct pci_dev *dev, int where, int size, u32 val) -+{ -+ return pci_mmcfg_access(dev, where, size, &val, true); -+} -+EXPORT_SYMBOL_GPL(pci_mmcfg_write_config); -+ - /** - * pci_cfg_access_lock - Lock PCI config reads/writes - * @dev: pci device struct -diff --git a/include/linux/pci.h b/include/linux/pci.h -index 2c4454583..942b20dcc 100644 ---- a/include/linux/pci.h -+++ b/include/linux/pci.h -@@ -1412,6 +1412,17 @@ int pci_user_write_config_byte(struct pci_dev *dev, int where, u8 val); - int pci_user_write_config_word(struct pci_dev *dev, int where, u16 val); - int pci_user_write_config_dword(struct pci_dev *dev, int where, u32 val); - -+/* -+ * Config access forced through the memory-mapped (ECAM) path. Only for -+ * devices whose registers are unreachable with the access method the -+ * platform selected for the first 256 config bytes. Both return -ENXIO on -+ * an architecture or platform that has no such path. -+ */ -+int arch_pci_mmcfg_access(struct pci_dev *dev, int where, int size, u32 *val, -+ bool write); -+int pci_mmcfg_read_config(struct pci_dev *dev, int where, int size, u32 *val); -+int pci_mmcfg_write_config(struct pci_dev *dev, int where, int size, u32 val); -+ - int __must_check pci_enable_device(struct pci_dev *dev); - int __must_check pci_enable_device_mem(struct pci_dev *dev); - int __must_check pci_reenable_device(struct pci_dev *); --- -2.43.0 - diff --git a/docs/submission/Kconfig.kernel b/docs/submission/Kconfig.kernel index beb641c..1d310ba 100644 --- a/docs/submission/Kconfig.kernel +++ b/docs/submission/Kconfig.kernel @@ -1,17 +1,13 @@ config I2C_IMC_SKYLAKE tristate "Intel Skylake-X iMC SMBus adapter" - depends on PCI && (PCI_MMCONFIG || COMPILE_TEST) + depends on PCI && (X86_64 || COMPILE_TEST) help Say Y here if you want kernel support for the integrated memory controller (iMC) SMBus engine found in Intel Skylake-X / Cascade Lake-X processors (socket LGA 2066, platform X299, PCU function 8086:2085). - The engine is exposed through the PCI configuration space of the - PCU function. Those registers are unreachable through the config - accessors the platform selects for the first 256 bytes, so the - driver reaches them through the memory-mapped (ECAM) path. Two - I2C adapters are registered, one per hardware SMBus channel, + Two I2C adapters are registered, one per hardware SMBus channel, allowing access to DDR4 DIMM SPD EEPROMs (0x50-0x57) and thermal sensors from userspace via standard i2c-tools and lm-sensors. diff --git a/docs/submission/cover-letter.txt b/docs/submission/cover-letter.txt index 49b8b37..15407a6 100644 --- a/docs/submission/cover-letter.txt +++ b/docs/submission/cover-letter.txt @@ -1,72 +1,50 @@ -Subject: [RFC PATCH v4 0/2] i2c: imc-skylake: Intel Skylake-X iMC SMBus adapter +Subject: [PATCH v4] i2c: imc-skylake: add Intel Skylake-X iMC SMBus adapter -This series exposes the two SMBus channels of the integrated memory -controller of Intel Skylake-X and Cascade Lake-X processors as standard -Linux I2C adapters, so that DDR4 SPD EEPROMs and DIMM thermal sensors can -be read with i2c-tools, decode-dimms and lm-sensors. +The integrated memory controller of Intel Skylake-X and Cascade Lake-X +processors contains an SMBus engine wired to the DDR4 DIMMs. This driver +exposes its two channels as standard Linux I2C adapters, so that DDR4 SPD +EEPROMs and DIMM thermal sensors can be read with i2c-tools, decode-dimms +and lm-sensors. -v3 was never posted. v2 was posted as i2c-imc-x299 and received no replies: +v2 was posted as i2c-imc-x299 and received no replies: https://lore.kernel.org/linux-i2c/20260620144131.415559-1-simone.chifari@gmail.com/ -Patch 1 is a PCI/x86 change and is the reason this is still an RFC. -Patch 2 is the driver and depends on patch 1. +v3 was never posted. - 1/2 PCI: allow config access forced through the memory-mapped path - 2/2 i2c: imc-skylake: add driver for the Intel Skylake-X iMC SMBus engine +Correction to v2 +~~~~~~~~~~~~~~~~ +v2 claimed that firmware filters CF8/CFC config writes to this function and +that the registers can only be reached through the ECAM window. That claim +was wrong and I am retracting it. -Why patch 1 exists -~~~~~~~~~~~~~~~~~~ -The engine's registers are at config space offsets 0x9C-0xB8, inside the -first 256 bytes. On x86, raw_pci_read()/raw_pci_write() dispatch that range -to raw_pci_ops, which is CF8/CFC whenever configuration type 1 is usable -(arch/x86/pci/common.c). The extended path, raw_pci_ext_ops, is only -consulted from offset 0x100 upwards. +It was never tested directly. The only evidence behind it was the boot log +line "PCI: Using configuration type 1", which says which accessor the kernel +uses and nothing about a write failing. Measured on the same board: -On the tested board a dword written to the command register through -CF8/CFC does not take effect: the register reads back unchanged, the GO bit -is never consumed and no SMBus transaction is issued. The same write through -the ECAM window works, and that is how the register layout was confirmed. + # setpci -s 16:1e.5 9c.L=20085002 # read SPD 0x50, register 0x02 + # setpci -s 16:1e.5 9c.L + 20005002 # GO consumed + # setpci -s 16:1e.5 a8.L + 0000500c # READ_DONE, no error + # setpci -s 16:1e.5 b4.L + 0000000c # SPD byte 2: DDR4 type code -This is not only a problem for this driver. If CF8/CFC writes to that -function are dropped, then setpci, sysfs and anything else the kernel does -to its low config space is equally unreliable. So rather than have a leaf -driver map the ECAM page itself, patch 1 adds - - int pci_mmcfg_read_config(struct pci_dev *, int where, int size, u32 *val); - int pci_mmcfg_write_config(struct pci_dev *, int where, int size, u32 val); - -in drivers/pci/access.c, taking pci_lock and honouring -pci_cfg_access_lock(), on top of a weak arch_pci_mmcfg_access() hook that -x86 implements over the already-validated pci_mmcfg_list. No MCFG re-parsing -and no second mapping of a window the architecture has already mapped. - -Questions for the PCI and x86 maintainers: - - - is an explicit per-access opt-in the right shape, or should a platform - that filters CF8/CFC for a function be handled by a quirk that redirects - all of its config accesses? - - if a helper is right, is drivers/pci/access.c plus a weak arch hook the - right place, or should this live entirely in arch/x86? - -I do not consider patch 1 settled and will rework it in whatever direction -you point. - -What causes the dropped write is not known. Earlier revisions of this -driver attributed it to System Management Mode; the only evidence for that -was the boot log line naming the access method, which says nothing about -who filters the write, so the attribution has been removed everywhere. The -observation stands on its own: the write does not reach the register. +The command written through CF8/CFC is executed and returns correct data, +and MSR_SMI_COUNT does not move across it. This driver therefore uses +pci_read_config_dword() and pci_write_config_dword() like any other driver. +There is no ECAM mapping, no MCFG parsing and no PCI-core change in this +series any more. Firmware arbitration ~~~~~~~~~~~~~~~~~~~~ -This is the reason no iMC SMBus driver has ever been merged, and this -series does not solve it either. +This is the reason no iMC SMBus driver has ever been merged, and this patch +does not solve it either. It does make the condition detectable. Andy Lutomirski's original i2c_imc put it plainly: a BMC, the memory controller's power management, and possibly SMM all like to touch this bus, -and Intel defined no way to arbitrate between them. That driver already had -a module parameter called allow_unsafe_access. Stefan Schaeckeler's 2020 +and Intel defined no way to arbitrate between them. That driver already had +a module parameter called allow_unsafe_access. Stefan Schaeckeler's 2020 Broadwell rewrite went further and stopped the PCU's TSOD polling for the duration of a transfer (write 0 to TSODCNTL, wait 10 ms for in-flight transactions to drain, restore afterwards), because on Broadwell the @@ -85,41 +63,44 @@ So this driver: duration of the transfer, and restores both afterwards; - after every transfer compares the command registers against what it left in them and fails the transfer with -EAGAIN if anything else - changed. This is the check the original driver had, and it is what makes - an unarbitrated system detectable rather than silently wrong; - - requires a positive sign that the engine acted on the command, because - the done bits are latched: without that, a command that is never - accepted is indistinguishable from a completed one and the caller would - receive the previous transaction's data. + changed. This is the check the original driver had, and it is what + makes an unarbitrated system detectable rather than silently wrong; + - reads the command word back and requires it to be the one just + written, because the done bits are latched. With the driver unloaded + and nothing running, STATUS on the test system reads 0x0000300c: + READ_DONE and WRITE_DONE both still set from an earlier transaction. + Without the check, a command word that never reaches the register is + indistinguishable from a completed transfer and the caller receives the + previous transaction's data. The engine only ever clears GO, so that + is the single permitted difference between the two words. What is missing: no equivalent of Broadwell's TSODCNTL has been identified -on this part, so a system with CLTT enabled is expected to trip the -interference check rather than be arbitrated against. Finding that register -is the main open item. I would rather ship a driver that detects the -condition and refuses than one that quietly races. +on this part. Offset 0xe0, where Broadwell has it, reads as zero here. A +system with CLTT enabled is therefore expected to trip the interference +check rather than be arbitrated against. I would rather ship a driver that +detects the condition and refuses than one that quietly races. Relation to prior work ~~~~~~~~~~~~~~~~~~~~~~ -There is no iMC I2C driver in mainline. Lutomirski's i2c-imc was an RFC; -the 2020 resend and github.com/Linux4/i2c-imc are out of tree. Neither can -be extended to cover this part: the device ID (0x2085 vs 0x3ca8/0x6fa8), -the register layout (0x9C-0xB8 vs 0x180-0x188) and the working access -method all differ. A separate file is cleaner than a hw_data table with -mutually exclusive access paths. +There is no iMC I2C driver in mainline. Lutomirski's i2c-imc was an RFC; +the 2020 resend and github.com/Linux4/i2c-imc are out of tree. Neither can +be extended to cover this part: the device ID (0x2085 vs 0x3ca8/0x6fa8) and +the register layout (0x9C-0xB8 vs 0x180-0x188) differ. A separate file is +cleaner than a hw_data table. The register encoding was cross-checked against an independent Windows implementation for the same engine, which predates this work: https://github.com/namazso/PawnIO.Modules/blob/c975ca1d1b8a9c7473a175fa1ca24d83b6cbc8f3/SmbusIntelSkylakeIMC.p -It also implements SMBus BYTE through a pointer-select bit. That is not +It also implements SMBus BYTE through a pointer-select bit. That is not advertised here because it has not been validated on the Linux system, so i2cdetect cannot scan these buses; devices are addressed directly. Changes since v2 ~~~~~~~~~~~~~~~~ - - Split into a PCI/x86 patch and the driver; the driver no longer walks - the MCFG table or maps the ECAM page itself. + - Retract the CF8/CFC claim; use the ordinary config accessors. The + driver no longer maps anything. - Rename i2c-imc-x299 to i2c-imc-skylake; the engine is in the CPU, not the chipset. - Add the default-off allow_unsafe_access gate. @@ -127,35 +108,36 @@ Changes since v2 - Preserve and restore command and TSOD state for both channels. - Validate READ_DONE, WRITE_DONE and the hardware error state. - Detect interference from another master and report it as -EAGAIN. - - Require a positive sign that a command was accepted. + - Read the command word back and require it to be the one written. - Add bounded command-toggle recovery for a stuck BUSY state. - Add WORD_DATA support with the hardware byte ordering. - Reject I2C_M_TEN explicitly and validate transfer direction. - Use the i2c core's suspended-adapter mechanism instead of a private flag and a non-standard errno. - Use devm_mutex_init() and devm-managed adapters; drop the unnecessary - pci_enable_device(). + pci_enable_device(); return -ENODEV rather than -EPERM from the + default-off gate, which was logging a driver-core error on every boot. - Add Documentation/i2c/busses/i2c-imc-skylake.rst. - - Drop the ACPI dependency; the Kconfig entry now depends on PCI_MMCONFIG. - - Remove the unsupported attribution of the dropped write to SMM. + - Drop the ACPI dependency. Validation ~~~~~~~~~~ - - In-tree build of both patches, W=1, x86_64, GCC: no diagnostics. - - arm64 COMPILE_TEST with Clang: no diagnostics. - - checkpatch.pl --strict: clean on both patches. + - In-tree build, W=1, x86_64 GCC and arm64 Clang COMPILE_TEST: no + diagnostics. + - checkpatch.pl --strict: clean. - Sparse with __CHECK_ENDIAN__: clean. - - Read-only hardware test on one Intel X299 system with four DDR4 DIMMs: - both adapters, four SPD EEPROMs, BYTE_DATA, WORD_DATA byte order, ENXIO - for an absent device, 100 concurrent reads per channel, the default-off - gate, and 20 unload/reload cycles, with no kernel warning or oops, no - interference reported and every command accepted. + - Read-only hardware test on one Intel Core i9-10900X (Cascade Lake-X) + system with four DDR4 DIMMs: both adapters, four SPD EEPROMs, BYTE_DATA, + WORD_DATA byte order, ENXIO for an absent device, 100 concurrent reads + per channel, the default-off gate, and 20 unload/reload cycles: 15 + checks, 0 failures, no kernel warning or oops, no interference + reported, every command word confirmed in the register. + - Config space of the PCU function sampled with the driver unloaded: no + register changed on its own, and TSOD_ACTIVE was clear on both channels, + so no firmware activity was observed on this system. -Still required before this can lose the RFC prefix: an independent -Tested-by, ideally on Cascade Lake-X; suspend/resume with active clients; -a measurement of MSR_SMI_COUNT across a failing CF8/CFC write, to say -something about the mechanism rather than only about the effect; and a -direction from the PCI and x86 maintainers on patch 1. +Still missing: an independent Tested-by, ideally on Skylake-X rather than +Cascade Lake-X, and suspend/resume testing with active I2C clients. Tool assistance ~~~~~~~~~~~~~~~ @@ -163,24 +145,18 @@ Per Documentation/process/generated-content.rst: this work was developed with an AI coding assistant, run interactively rather than autonomously. - What: Claude Code, model claude-opus-5. - - Where: it contributed to the transaction state machine of patch 2, the - kerneldoc and .rst documentation, the commit messages and this cover - letter. The register decoding, the ECAM finding and every hardware - measurement are mine. + - Where: it contributed to the transaction state machine, the .rst + documentation, the commit message and this cover letter. The register + decoding is mine. - How it was used: iteratively, one reviewed change at a time, in - response to specific requests, not from a single generating prompt. A + response to specific requests, not from a single generating prompt. A representative request is "the done bits are latched; show me how a command that is never accepted can be mistaken for a completed one, and - make that case detectable", which produced the check now in - imc_engine_responded(). + make that case detectable", which produced imc_engine_responded(). It + also designed the measurement that retracted the CF8/CFC claim above, + after asking why that claim had never been tested directly. - Testing: every revision was built with W=1, checked with - checkpatch --strict and sparse, and the hardware behaviour was tested - on the X299 system by me. + checkpatch --strict and sparse, and tested on the hardware by me. I have reviewed all of it, I understand it, and I take responsibility for -it. The Assisted-by trailer is on patch 2, which is where the assistance -went; patch 1 was written by hand from the existing arch/x86 code. - -Simone Chifari (2): - PCI: allow config access forced through the memory-mapped path - i2c: imc-skylake: add driver for the Intel Skylake-X iMC SMBus engine +it. diff --git a/docs/submission/i2c-imc-skylake.rst b/docs/submission/i2c-imc-skylake.rst index c5598ae..0ee63ad 100644 --- a/docs/submission/i2c-imc-skylake.rst +++ b/docs/submission/i2c-imc-skylake.rst @@ -73,21 +73,8 @@ Config space access ------------------- The engine's registers live at config space offsets 0x9C-0xB8 of the PCU -function. On x86 the first 256 bytes of config space are reached with the -access method the platform selected at boot, which is normally CF8/CFC. On the -tested board a write issued that way to the command register has no effect: the -GO bit is never consumed and no SMBus transaction is issued. The same write -through the memory-mapped (ECAM) path works. - -The driver therefore uses ``pci_mmcfg_read_config()`` and -``pci_mmcfg_write_config()``, which force the ECAM path for this device. It -requires ``CONFIG_PCI_MMCONFIG``; on a system booted with ``pci=nommconf`` the -driver will fail to probe. - -The mechanism that drops the CF8/CFC write has not been identified. -``MSR_SMI_COUNT`` does not increase across a failing write, so it is not -attributed to System Management Mode here. - +function and are reached with the ordinary ``pci_read_config_dword()`` and +``pci_write_config_dword()``. Nothing special is required. Supported transactions ---------------------- diff --git a/i2c-imc-skylake.c b/i2c-imc-skylake.c index 202450f..6d8fc39 100644 --- a/i2c-imc-skylake.c +++ b/i2c-imc-skylake.c @@ -3,23 +3,12 @@ * Intel Skylake-X iMC SMBus I2C adapter. * * The integrated memory controller (iMC) on Intel Skylake-X / Cascade Lake-X - * processors exposes an SMBus engine used to reach the SPD EEPROMs and thermal - * sensors on DDR4 DIMMs. The engine is driven through the PCI configuration - * space of the Sky Lake-E PCU function (8086:2085). This driver presents that - * engine as two standard Linux I2C adapters - one per hardware SMBus channel - - * so that i2c-tools and lm-sensors can use it without bespoke sysfs hacks. - * - * Why the config accesses go through pci_mmcfg_{read,write}_config(): - * The SMBus registers live at offsets 0x9C-0xB8, inside the first 256 config - * bytes. raw_pci_read()/raw_pci_write() dispatch that range to raw_pci_ops, - * which on this platform is CF8/CFC (the boot log reports "PCI: Using - * configuration type 1"). On the tested board a dword written that way to - * the command register does not take effect: the register reads back without - * the GO bit ever having been consumed and no SMBus transaction is issued. - * The same write through the ECAM window does work, and that is also how the - * register layout was confirmed on hardware. The mechanism behind the - * dropped write has not been identified; measurements of MSR_SMI_COUNT - * across a failing write show no SMI, so it is not attributed to SMM here. + * processors contains an SMBus engine wired to the DDR4 DIMMs. It reaches the + * SPD EEPROMs (0x50-0x57) and the thermal sensors (0x18-0x1f) of the installed + * modules. The engine is driven through the PCI configuration space of the + * Sky Lake-E PCU function (8086:2085). This driver presents it as two standard + * Linux I2C adapters - one per hardware SMBus channel - so that i2c-tools, + * decode-dimms and lm-sensors can use it without bespoke sysfs hacks. * * Per-channel register triple within the config space of the function: * ch0 ch1 @@ -30,12 +19,11 @@ * SMBus command byte (DATA bits[15:8]) = (rw << 7) | addr7: the 7-bit slave * address with bit7 = direction (1 write / 0 read). The register/offset goes * in DATA[7:0]. So 0x50 reads SPD EEPROM 0x50. This was decoded and - * confirmed on hardware. + * confirmed on hardware, and cross-checked against an independent Windows + * implementation of the same engine. * * The two channels are independent SMBus buses (DIMMs 1,2 on ch0; DIMMs 3,4 on * ch1), hence two i2c_adapter instances exposed as separate /dev/i2c-* nodes. - * Each channel carries the DIMM SPD EEPROMs (0x50-0x57) and thermal sensors - - * all reachable by 7-bit address. * * Bus arbitration / concurrency with firmware: * The driver cannot coordinate with SMM, a BMC, or the iMC's own closed-loop @@ -66,15 +54,10 @@ #include #include -/* expected PCI id of the Sky Lake-E PCU SMBus function */ +/* PCI id of the Sky Lake-E PCU function carrying the SMBus engine */ #define PCU_DEVICE 0x2085 -#define PCU_ID ((PCU_DEVICE << 16) | PCI_VENDOR_ID_INTEL) -/* function-global config registers */ -#define CFG_VENDOR_DEV 0x00 /* cfg[0]: vendor/device id, for sanity check */ -#define CFG_IMC_BUS 0xCC /* cfg[0xCC] bits[15:8]: iMC SMBus bus number */ - -/* per-channel register offsets */ +/* per-channel register offsets in the function's config space */ #define CH0_CTRL 0xB4 #define CH0_DATA 0x9C #define CH0_STAT 0xA8 @@ -103,8 +86,10 @@ #define WRITE_OPERATION BIT(15) #define COMMAND_PREFIX (COMMAND_TOGGLE | GO_BIT) #define COMMAND_KEEP_MASK (~TSOD_ACTIVE_BIT) -/* command bits this driver produces itself; everything else belongs to nobody - * we know about, so a change in them during a transfer means interference. +/* + * Command bits this driver produces itself. Everything outside this mask + * belongs to whoever else is driving the engine, so a change there during a + * transfer means interference. */ #define COMMAND_OUR_BITS (COMMAND_TOGGLE | GO_BIT | WORD_BIT | \ GENMASK(15, 0)) @@ -157,20 +142,19 @@ struct imc_xfer_state { }; /* - * Config accessors. Every access goes through the ECAM path; see the comment - * at the top of the file. A failure is recorded in s->io_err and reported - * once, at the end of the transfer, so the transaction logic does not have to - * check a return value on every register touch. A failed read yields all-ones, - * which no polling loop mistakes for a completed transaction. + * Register accessors. A config-space failure is recorded in s->io_err and + * reported once, at the end of the transfer, so the transaction logic does not + * have to check a return value on every register touch. A failed read yields + * all-ones, which no polling loop mistakes for a completed transaction. */ static u32 imc_reg(struct imc_smbus *s, unsigned int off) { u32 val; int ret; - ret = pci_mmcfg_read_config(s->pdev, off, 4, &val); + ret = pci_read_config_dword(s->pdev, off, &val); if (ret) { - s->io_err = ret; + s->io_err = pcibios_err_to_errno(ret); return ~0U; } @@ -181,9 +165,9 @@ static void imc_set(struct imc_smbus *s, unsigned int off, u32 val) { int ret; - ret = pci_mmcfg_write_config(s->pdev, off, 4, val); + ret = pci_write_config_dword(s->pdev, off, val); if (ret) - s->io_err = ret; + s->io_err = pcibios_err_to_errno(ret); } static u32 imc_busy_mask(struct imc_smbus *s) @@ -255,7 +239,6 @@ static int imc_wait_transfer(struct imc_smbus *s, const struct imc_chan *c, command = imc_reg(s, c->data); imc_set(s, c->data, command ^ COMMAND_TOGGLE); - imc_reg(s, c->data); ret = imc_wait_done(s, c, status); if (ret) @@ -287,19 +270,21 @@ static int imc_check_status(struct imc_smbus *s, const struct imc_chan *c, } /* - * The done bits are latched and the driver has found no way to clear them, so - * a transaction that is never started at all would otherwise be indistinguish- - * able from one that completed: STATUS still shows the previous DONE and the - * caller would receive the previous transaction's data as valid. + * The done bits are latched and no way to clear them has been found, so a + * command that never reaches the register is indistinguishable from one that + * completed: STATUS still shows the previous DONE, the GO bit of the stale + * command word is already clear, and the caller would receive the previous + * transaction's data as valid. * - * Require one of two positive signs that the engine acted on the command: the - * GO bit was observed set in the read-back that follows the command write, or - * STATUS changed during the transfer. Neither of them appearing means nothing - * happened. + * Read the command word back and require it to be the one just written. The + * engine only ever clears GO, so that bit is the single permitted difference; + * anything else means the write did not land. Checking the word rather than + * a status transition matters because two identical transfers in a row leave + * STATUS bit for bit the same. */ -static bool imc_engine_responded(bool go_seen, u32 status_pre, u32 status) +static bool imc_command_landed(u32 command, u32 readback) { - return go_seen || status != status_pre; + return !((command ^ readback) & ~GO_BIT); } /* @@ -345,7 +330,6 @@ static void imc_restore_xfer(struct imc_smbus *s, * transaction that was already in flight when we found it. */ imc_set(s, imc_chans[i].data, state->command[i] & ~GO_BIT); - imc_reg(s, imc_chans[i].data); } } @@ -367,7 +351,6 @@ static int imc_prepare_xfer(struct imc_smbus *s, const struct imc_chan *c, imc_set(s, imc_chans[i].data, state->command[i] & COMMAND_KEEP_MASK); - imc_reg(s, imc_chans[i].data); ret = imc_wait_go_clear(s, &imc_chans[i]); if (ret) { imc_restore_xfer(s, state); @@ -382,8 +365,7 @@ static int imc_access(struct imc_smbus *s, const struct imc_chan *c, u8 addr, u8 reg, char read_write, int size, u16 *value) { struct imc_xfer_state state = {}; - u32 command, expected, raw, status_pre, status = 0; - bool go_seen; + u32 command, expected, raw, readback, status = 0; int ret; ret = imc_prepare_xfer(s, c, &state); @@ -397,27 +379,23 @@ static int imc_access(struct imc_smbus *s, const struct imc_chan *c, u8 addr, if (read_write == I2C_SMBUS_WRITE) { raw = size == I2C_SMBUS_WORD_DATA ? swab16(*value) : *value; imc_set(s, c->ctrl, raw << 16); - imc_reg(s, c->ctrl); command |= WRITE_OPERATION; } - status_pre = imc_reg(s, c->stat); imc_set(s, c->data, command); - /* read back to flush the write and to see whether GO was accepted */ - go_seen = imc_reg(s, c->data) & GO_BIT; + readback = imc_reg(s, c->data); + if (!imc_command_landed(command, readback)) { + dev_warn_ratelimited(s->dev, + "ch%d command 0x%08x did not reach the register (read back 0x%08x)\n", + c->idx, command, readback); + ret = -EIO; + goto restore_command; + } ret = imc_wait_transfer(s, c, &status); if (ret) goto restore_command; - if (!imc_engine_responded(go_seen, status_pre, status)) { - dev_warn_ratelimited(s->dev, - "ch%d command not accepted for addr 0x%02x reg 0x%02x (stat 0x%08x unchanged, GO never seen)\n", - c->idx, addr, reg, status); - ret = -ETIMEDOUT; - goto restore_command; - } - expected = read_write == I2C_SMBUS_WRITE ? STAT_WRITE_DONE : STAT_READ_DONE; ret = imc_check_status(s, c, status, expected, addr, reg); @@ -549,8 +527,6 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imc_pm_ops, imc_suspend, imc_resume); static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct imc_smbus *s; - u8 imc_bus_hw; - u32 cfg0, cc; int ret, i; if (!allow_unsafe_access) { @@ -575,40 +551,6 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (ret) return ret; - ret = pci_mmcfg_read_config(pdev, CFG_VENDOR_DEV, 4, &cfg0); - if (ret) { - dev_err(&pdev->dev, - "no memory-mapped path to config space: %d\n", ret); - return ret; - } - if (cfg0 != PCU_ID) { - dev_err(&pdev->dev, "wrong device through ECAM (cfg[0]=0x%08x)\n", - cfg0); - return -ENODEV; - } - - /* - * Cross-check the iMC bus number against the configuration register - * value: cfg[0xCC] bits[15:8] = the iMC SMBus bus number as seen by - * the PCU. On all known Skylake-X / Cascade Lake-X boards this matches - * the probed bus number. - * A mismatch means the accessor landed on the wrong function - warn but - * continue; the binding is already locked to 8086:2085. - */ - ret = pci_mmcfg_read_config(pdev, CFG_IMC_BUS, 4, &cc); - if (ret) - return ret; - imc_bus_hw = (cc >> 8) & 0xFF; - - if (imc_bus_hw && imc_bus_hw != (u8)pdev->bus->number) - dev_warn(&pdev->dev, - "cfg[0xCC] reports iMC bus 0x%02x but probed bus=0x%02x\n", - imc_bus_hw, (u8)pdev->bus->number); - else - dev_dbg(&pdev->dev, - "cfg[0xCC]=0x%08x iMC bus 0x%02x confirmed\n", - cc, imc_bus_hw); - /* * Lifetime safety: the I2C core guarantees that smbus_xfer callbacks * are not invoked after i2c_del_adapter() returns. Since we use diff --git a/test-smoke.sh b/test-smoke.sh index c575ae4..228c3b8 100755 --- a/test-smoke.sh +++ b/test-smoke.sh @@ -240,12 +240,13 @@ else _ok "no foreign master detected during the test" fi -# Likewise for a command the engine never accepted: that path exists because -# the done bits are latched and must not be mistaken for a fresh completion. -if grep -q 'command not accepted' <<< "$NEW_DMESG"; then - _fail "engine did not accept a command; config access path is unreliable" +# Likewise for a command that never reached the register: that path exists +# because the done bits are latched and a stale DONE must not be mistaken for +# a fresh completion. +if grep -q 'did not reach the register' <<< "$NEW_DMESG"; then + _fail "a command never reached the register; config access is unreliable" else - _ok "every command was accepted by the engine" + _ok "every command reached the register" fi echo "=== result: $PASS passed, $FAIL failed ===" diff --git a/tools/dump-pcu-config.sh b/tools/dump-pcu-config.sh index 3c62e5c..f8e9be7 100755 --- a/tools/dump-pcu-config.sh +++ b/tools/dump-pcu-config.sh @@ -58,8 +58,10 @@ echo "=== engine registers ===" snapshot_regs "$BDF" echo +# lspci -xxxx prints two-digit offsets for the first 256 bytes and three or +# more digits above that, so anchor on the short form here. echo "=== neighbourhood of the Broadwell TSODCNTL offset (0xe0) ===" -awk '/^0[cdef]0:/ { print }' "$second" +awk '/^[cdef]0:/ { print }' "$second" echo echo "=== the driver's register window (0x90-0xbf) ===" -awk '/^0[9ab]0:/ { print }' "$second" +awk '/^[9ab]0:/ { print }' "$second" diff --git a/tools/probe-cf8-transaction.sh b/tools/probe-cf8-transaction.sh new file mode 100755 index 0000000..e00390c --- /dev/null +++ b/tools/probe-cf8-transaction.sh @@ -0,0 +1,151 @@ +#!/usr/bin/env bash +# +# WRITES TO HARDWARE. Settles the question patch 1/2 of the series exists for: +# does a command written through CF8/CFC actually start an SMBus transaction, +# or only land in the register? +# +# tools/probe-cf8-write.sh already showed that the register itself is writable +# through CF8/CFC. This issues a full command with the GO bit set and watches +# whether the engine acts on it. +# +# The target defaults to a read of address 0x77, which is not populated, so the +# only thing that can happen on the bus is an address NACK - the same operation +# test-smoke.sh already performs through the driver. The original DATA value is +# restored with GO masked on every exit path. +# +set -euo pipefail + +# shellcheck source=tools/common.sh +source "$(dirname "$(readlink -f "$0")")/common.sh" + +COMMAND_TOGGLE=$((1 << 29)) +GO_BIT=$((1 << 19)) +TSOD_ACTIVE_BIT=$((1 << 20)) +STAT_BUSY=1 +STAT_ERROR=2 +STAT_READ_DONE=4 +STAT_WRITE_DONE=8 + +ADDR=0x77 +REG=0x00 +CHANNEL=0 +CONFIRMED=false + +while (($#)); do + case $1 in + --i-understand) CONFIRMED=true; shift ;; + --addr) ADDR=$2; shift 2 ;; + --reg) REG=$2; shift 2 ;; + --channel) CHANNEL=$2; shift 2 ;; + -h|--help) + sed -n '3,15p' "$0" + echo "usage: $0 --i-understand [--addr 0xNN] [--reg 0xNN] [--channel 0|1]" + exit 0 + ;; + *) die "unknown argument: $1" ;; + esac +done + +$CONFIRMED || { + echo "This issues an SMBus read transaction from userspace. Re-run with --i-understand." >&2 + exit 2 +} + +require_root +require_cmd lspci setpci awk + +case $CHANNEL in +0) REG_DATA=$REG_CH0_DATA; REG_STAT=$REG_CH0_STAT; REG_CTRL=$REG_CH0_CTRL ;; +1) REG_DATA=$REG_CH1_DATA; REG_STAT=$REG_CH1_STAT; REG_CTRL=$REG_CH1_CTRL ;; +*) die "channel must be 0 or 1" ;; +esac + +driver_is_loaded && die "unload i2c-imc-skylake first" + +BDF=$(find_pcu) +original=$(read_reg "$BDF" "$REG_DATA") +status0=$(read_reg "$BDF" "$REG_STAT") +ctrl0=$(read_reg "$BDF" "$REG_CTRL") + +echo "device: $BDF, channel $CHANNEL" +echo "target: read address $ADDR register $REG" +echo "DATA before: $original" +echo "STATUS before: $status0" +echo "CTRL before: $ctrl0" +echo + +((original & GO_BIT)) && die "GO already set, refusing" +((original & TSOD_ACTIVE_BIT)) && die "TSOD_ACTIVE set, refusing" +((status0 & STAT_BUSY)) && die "engine BUSY, refusing" + +restore() +{ + setpci -s "$BDF" "$REG_DATA.L=$(printf '%08x' "$((original & ~GO_BIT))")" || true + echo + echo "DATA restored to $(read_reg "$BDF" "$REG_DATA")" +} +trap restore EXIT + +issue() +{ + local command=$1 label=$2 + local i data status ctrl go_seen=no changed=no + + printf '=== %s: writing 0x%08x ===\n' "$label" "$command" + setpci -s "$BDF" "$REG_DATA.L=$(printf '%08x' "$command")" + + for i in $(seq 1 40); do + data=$(read_reg "$BDF" "$REG_DATA") + status=$(read_reg "$BDF" "$REG_STAT") + ((data & GO_BIT)) && go_seen=yes + [[ "$status" != "$status0" ]] && changed=yes + if ((i <= 3)) || [[ "$changed" == yes ]]; then + printf ' poll %-2d DATA=%s STATUS=%s\n' "$i" "$data" "$status" + fi + ((data & GO_BIT)) || break + sleep 0.005 + done + + data=$(read_reg "$BDF" "$REG_DATA") + status=$(read_reg "$BDF" "$REG_STAT") + ctrl=$(read_reg "$BDF" "$REG_CTRL") + + echo " final DATA=$data STATUS=$status CTRL=$ctrl" + echo " GO observed set: $go_seen" + echo " STATUS changed: $changed (baseline $status0)" + if ((status & STAT_ERROR)); then + echo " -> ERROR/NACK bit set: the engine ran the transaction and the" + echo " device did not answer, which is the expected result here." + fi + echo +} + +# 1: exactly what the driver writes - toggle bit forced to 1. +issue $((COMMAND_TOGGLE | GO_BIT | (ADDR << 8) | REG)) "same toggle as the driver" + +# 2: the toggle bit inverted relative to what is in the register now, in case +# the engine latches a command on a change of that bit rather than on GO. +issue $(( ((original ^ COMMAND_TOGGLE) & COMMAND_TOGGLE) | GO_BIT | (ADDR << 8) | REG )) \ + "toggle bit inverted" + +echo "=== verdict ===" +final_status=$(read_reg "$BDF" "$REG_STAT") +if [[ "$final_status" != "$status0" ]]; then + cat <<-EOF + CF8/CFC STARTED A TRANSACTION. + STATUS moved from $status0 to $final_status without any ECAM access. + The premise of the ECAM patch does not hold: the driver can use + pci_read_config_dword() and pci_write_config_dword() like any other + driver, and patch 1/2 of the series is unnecessary. + EOF +else + cat <<-EOF + NO TRANSACTION. + The register accepted the write (probe-cf8-write.sh proved that) but the + engine did not act on it: STATUS never moved from $status0. This is the + evidence patch 1/2 needs - the failure is not a filtered write, it is a + command the engine ignores when it arrives this way. + Re-run the same command through the driver's ECAM path to complete the + comparison. + EOF +fi diff --git a/tools/watch-engine.sh b/tools/watch-engine.sh index ac586cc..c8ae77a 100755 --- a/tools/watch-engine.sh +++ b/tools/watch-engine.sh @@ -46,13 +46,13 @@ deadline=$((SECONDS + SECONDS_TOTAL)) while ((SECONDS < deadline)); do sleep "$INTERVAL" mapfile -t current < <(snapshot_regs "$BDF") - ((samples++)) + samples=$((samples + 1)) for i in "${!current[@]}"; do if [[ "${current[i]}" != "${previous[i]}" ]]; then printf '%s CHANGE %s -> %s\n' \ "$(date -Is)" "${previous[i]}" "${current[i]}" - ((changes++)) + changes=$((changes + 1)) fi done previous=("${current[@]}")