diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ad7f87a..677230f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,14 +9,31 @@ on: - main workflow_dispatch: +permissions: + contents: read + concurrency: group: ci-${{ github.ref }} cancel-in-progress: true +env: + I2C_TREE: https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git + SPARSE_REPOSITORY: https://git.kernel.org/pub/scm/devel/sparse/sparse.git + SPARSE_COMMIT: 00c4b8e75cbf92003bc4a695a223045092b173f3 + jobs: - build: + out-of-tree: + name: Out-of-tree (${{ matrix.compiler }}) runs-on: ubuntu-latest timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + include: + - compiler: gcc + make_args: "" + - compiler: clang + make_args: LLVM=1 steps: - uses: actions/checkout@v4 @@ -24,20 +41,121 @@ jobs: - name: Install build dependencies run: | sudo apt-get update - sudo apt-get install -y build-essential linux-headers-$(uname -r) + 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 + run: make W=1 KCFLAGS=-W ${{ matrix.make_args }} - name: Run checkpatch + if: matrix.compiler == 'gcc' + run: make checkpatch + + - name: Check shell scripts + if: matrix.compiler == 'gcc' run: | - curl -sSLo checkpatch.pl https://raw.githubusercontent.com/torvalds/linux/master/scripts/checkpatch.pl - curl -sSLo spelling.txt https://raw.githubusercontent.com/torvalds/linux/master/scripts/spelling.txt - perl checkpatch.pl --no-tree --strict --no-summary --show-types -f i2c-imc-skylake.c + sudo apt-get install -y shellcheck + bash -n test-smoke.sh + for f in tools/*.sh; do bash -n "$f"; done + shellcheck -S warning test-smoke.sh tools/*.sh - name: Upload artifact + if: matrix.compiler == 'gcc' uses: actions/upload-artifact@v4 with: name: i2c-imc-skylake-${{ github.ref_name }} path: i2c-imc-skylake.ko if-no-files-found: error + + in-tree: + name: Current i2c-host integration (2-patch series) + runs-on: ubuntu-latest + timeout-minutes: 45 + + steps: + - uses: actions/checkout@v4 + + - name: Install kernel build dependencies + run: | + sudo apt-get update + sudo apt-get install -y bc bison build-essential clang flex libelf-dev libssl-dev llvm + + - name: Build pinned Sparse + run: | + git clone --filter=blob:none "$SPARSE_REPOSITORY" sparse + git -C sparse checkout "$SPARSE_COMMIT" + make -C sparse -j"$(nproc)" + echo "$PWD/sparse" >> "$GITHUB_PATH" + + - 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' \ + >> linux-i2c/drivers/i2c/busses/Makefile + git -C linux-i2c rev-parse HEAD + + - name: Configure kernel + working-directory: linux-i2c + run: | + make O=../linux-out x86_64_defconfig + 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 + 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 + + - name: Build arm64 COMPILE_TEST with Clang + working-directory: linux-i2c + run: | + make O=../linux-out-arm64 ARCH=arm64 LLVM=1 defconfig + scripts/config --file ../linux-out-arm64/.config \ + --enable COMPILE_TEST --enable PCI --enable I2C \ + --module I2C_IMC_SKYLAKE + 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 + working-directory: linux-i2c + 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 + run: | + sudo apt-get install -y python3-sphinx python3-sphinx-rtd-theme + cp ../docs/submission/i2c-imc-skylake.rst Documentation/i2c/busses/ + sed -i 's|^ i2c-i801$| i2c-i801\n i2c-imc-skylake|' \ + Documentation/i2c/busses/index.rst + grep -q i2c-imc-skylake Documentation/i2c/busses/index.rst + make O=../linux-out htmldocs SPHINXDIRS=i2c 2>&1 | tee doc.log | tail -20 + ! grep -Ei 'WARNING|ERROR' doc.log diff --git a/.gitignore b/.gitignore index 6d0a15f..09110c5 100644 --- a/.gitignore +++ b/.gitignore @@ -51,6 +51,8 @@ docs/archive-i2c-imc-driver-plan.md docs/archive-v3-review-fixes.md docs/submission/submission-instructions.md docs/submission/review-answers.md +docs/submission/v3-readiness-audit.md +docs/submission/v4-review-360.md # Private Gitea CI (replaced by .github/workflows/ for public repo) .gitea/ diff --git a/Makefile b/Makefile index 66f850b..999298c 100644 --- a/Makefile +++ b/Makefile @@ -2,45 +2,53 @@ obj-m := i2c-imc-skylake.o KVER ?= $(shell uname -r) KDIR ?= /lib/modules/$(KVER)/build -MODDIR = /lib/modules/$(KVER)/kernel/drivers/i2c/busses +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 clean: $(MAKE) -C $(KDIR) M=$(PWD) clean -# Install into kernel tree + modprobe. -# Ubuntu/Debian kernel 6.x+ ships .ko.zst; depmod skips uncompressed .ko files, -# so compress with zstd when available, else fall back to plain .ko. +# Install through Kbuild so ownership, compression and destination follow the +# target kernel/distribution policy. Invoke this target with suitable privileges. install: all - if command -v zstd >/dev/null 2>&1; then \ - zstd -q -f i2c-imc-skylake.ko -o i2c-imc-skylake.ko.zst; \ - sudo install -D -m 644 i2c-imc-skylake.ko.zst $(MODDIR)/i2c-imc-skylake.ko.zst; \ - sudo rm -f $(MODDIR)/i2c-imc-skylake.ko; \ - else \ - sudo install -D -m 644 i2c-imc-skylake.ko $(MODDIR)/i2c-imc-skylake.ko; \ - sudo rm -f $(MODDIR)/i2c-imc-skylake.ko.zst; \ - fi - sudo depmod -a $(KVER) - sudo modprobe i2c-imc-skylake + $(MAKE) -C $(KDIR) M=$(PWD) modules_install \ + INSTALL_MOD_PATH=$(INSTALL_MOD_PATH) \ + INSTALL_MOD_DIR=$(INSTALL_MOD_DIR) -# Remove module and .ko/.ko.zst from kernel tree +# Remove a module installed in INSTALL_MOD_DIR. Invoke with suitable privileges. uninstall: - sudo rmmod i2c-imc-skylake 2>/dev/null || true - sudo rm -f $(MODDIR)/i2c-imc-skylake.ko $(MODDIR)/i2c-imc-skylake.ko.zst - sudo depmod -a $(KVER) + rm -f $(INSTALL_MOD_PATH)/lib/modules/$(KVER)/$(INSTALL_MOD_DIR)/i2c-imc-skylake.ko* + depmod -a -b $(INSTALL_MOD_PATH) $(KVER) -# Quick rmmod + insmod cycle for leak/oops testing +# Runtime helpers require explicit acknowledgement of the firmware-arbitration +# risk: make reload ALLOW_UNSAFE=1 (or make load ALLOW_UNSAFE=1). reload: all + @test "$(ALLOW_UNSAFE)" = "1" || \ + { echo "Refusing: rerun with ALLOW_UNSAFE=1" >&2; exit 2; } sudo rmmod i2c-imc-skylake 2>/dev/null || true - sudo insmod i2c-imc-skylake.ko + sudo insmod i2c-imc-skylake.ko allow_unsafe_access=1 # Load without build (use after install) load: + @test "$(ALLOW_UNSAFE)" = "1" || \ + { echo "Refusing: rerun with ALLOW_UNSAFE=1" >&2; exit 2; } sudo modprobe i2c-dev - sudo modprobe i2c-imc-skylake + sudo modprobe i2c-imc-skylake allow_unsafe_access=1 unload: sudo rmmod i2c-imc-skylake 2>/dev/null || true @@ -53,11 +61,17 @@ checkpatch: sparse: $(MAKE) -C $(KDIR) M=$(PWD) C=1 CF="-D__CHECK_ENDIAN__" -# Smoke test — requires root + module loaded (run after make install or make reload) -test: - sudo bash test-smoke.sh +# Smoke test requires root or non-interactive sudo. +test: all + MODULE_PATH=$(PWD)/i2c-imc-skylake.ko bash test-smoke.sh + +# Hardware measurements that the upstream submission still needs. Read-only +# unless explicitly asked otherwise; see tools/README.md. +measure: + @echo "Run the scripts under tools/ individually; see tools/README.md" >&2 + @false log: sudo dmesg | grep i2c-imc-skylake | tail -80 -.PHONY: all clean install uninstall reload load unload checkpatch sparse test log +.PHONY: all clean install uninstall reload load unload checkpatch sparse test measure log diff --git a/README.md b/README.md index 3d8d5b3..8dbb1e8 100644 --- a/README.md +++ b/README.md @@ -1,45 +1,106 @@ # Intel Skylake-X iMC SMBus I2C Driver [![License: GPL v2](https://img.shields.io/badge/License-GPL%20v2-blue.svg)](https://www.gnu.org/licenses/old-licenses/gpl-2.0.html) -[![Kernel Compatibility](https://img.shields.io/badge/Kernel-6.x%20%7C%207.x-green.svg)](#) -[![Code Style: checkpatch](https://img.shields.io/badge/checkpatch-clean-brightgreen.svg)](#) -[![Upstream Status](https://img.shields.io/badge/Upstream-Submitted%20(v2)-orange.svg)](https://lore.kernel.org/linux-i2c/) +[![CI](https://github.com/systempal/i2c-imc-skylake/actions/workflows/ci.yml/badge.svg)](https://github.com/systempal/i2c-imc-skylake/actions/workflows/ci.yml) +[![Upstream Status](https://img.shields.io/badge/Upstream-v2%20archived-orange.svg)](https://lore.kernel.org/linux-i2c/20260620144131.415559-1-simone.chifari@gmail.com/T/#t) -A modern Linux PCI bus driver for the integrated Memory Controller (iMC) SMBus controller found in Intel Skylake-X and Cascade Lake-X (HEDT X299, function ID `8086:2085`) processors. +An experimental Linux PCI driver for the SMBus engine in the integrated memory +controller (iMC) of Intel Skylake-X and Cascade Lake-X processors +(PCU function `8086:2085`). -This driver maps the iMC SMBus engine and registers it as two standard Linux I2C adapters (one per physical channel), allowing native userspace tools like `i2c-tools` and `lm-sensors` to interact with target devices (such as DDR4 DIMM SPD EEPROMs and thermal sensors) without custom raw PCI port-IO writes. +The engine reaches the DDR4 DIMMs: SPD EEPROMs at `0x50`-`0x57`, thermal +sensors (TSOD) at `0x18`-`0x1f`, and whatever else a given module carries on +those lines. The driver registers it as two standard Linux I2C adapters, one +per hardware channel, so `i2c-tools`, `decode-dimms` and `lm-sensors` work +without custom raw PCI writes. + +> [!WARNING] +> The engine has no arbitration mechanism. SMM, a BMC and the memory +> controller's own closed-loop thermal throttling (CLTT) can drive the same +> registers, and the PCI ID does not tell the driver whether any of them is +> active. The driver is disabled by default and binds only with +> `allow_unsafe_access=1`. Use it only where concurrent firmware access has +> been excluded — and check the log for the interference warning described +> below. --- -## Origin & Use Case +## Upstream status -This driver was born out of the need to control the RGB lighting effects on **Kingston FURY DDR4 RGB** memory modules under Linux on the Intel X299 (HEDT) platform. +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. -These memory modules carry an onboard **ENE KB9012** LED controller accessible via the SMBus channels of the CPU's integrated memory controller (iMC). However, standard Linux tools and OpenRGB could not communicate with them because: -1. The X299 System Management Mode (SMM) firmware traps standard port-based PCI configuration writes, breaking standard driver access. -2. The iMC SMBus channels were not exposed as standard Linux I2C buses. +The next posting is an RFC in **two patches**: -By resolving the MMCONFIG base and using ECAM MMIO, this driver bypasses SMM and registers the physical memory channels as standard `/dev/i2c-*` adapters. +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. -While the kernel driver itself is kept 100% generic and brand-agnostic (to comply with Linux upstream requirements), its primary practical use cases are: -* Driving userspace Linux daemons for hardware/software-rendered RGB lighting animations without screen or GUI lag. -* Enabling compatibility with OpenRGB for memory modules on Skylake-X / Cascade Lake-X systems. -* Reading standard DDR4 SPD data and DIMM thermal sensor metrics using standard utilities like `decode-dimms`. +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). + +**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 +stalled on the same firmware-arbitration problem. This driver does not solve it +either; it detects it. See the cover letter for what that means in practice. --- -## Upstream Status +## Why the ECAM path -This driver is currently submitted for review to the Linux kernel `linux-i2c` subsystem mailing list for mainlining (patch series v2). See the [lore.kernel.org linux-i2c archive](https://lore.kernel.org/linux-i2c/) for the discussion thread. +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. + +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. --- -## Technical Features & SMM Bypass +## What the driver does about concurrency -* **ECAM MMIO Access**: On the X299 platform, System Management Mode (SMM) firmware traps and drops legacy port-based (`CF8/CFC`) configuration space writes to the PCU function. This driver bypasses the trap by resolving the MMCONFIG base from the ACPI `MCFG` table and accessing the registers directly via ECAM MMIO memory-mapped pages. -* **Dual Channel Support**: Registers two independent I2C buses (`iMC SMBus Skylake-X channel 0` and `channel 1`), mapping physically to DIMM slots 1-2 and 3-4 respectively. -* **Clean Lifecycle**: Fully compliant with the `devm` kernel framework. All mapped registers, adapters, and mutexes are cleaned up automatically upon driver unbinding. -* **Zero Out-of-Tree Brandings**: Code is completely generic and compliant with standard Linux kernel styling guidelines. +* serializes both channels under one mutex and waits for the whole engine to be + idle before touching any register; +* saves the command state of both channels, clears TSOD_ACTIVE for the duration + 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. + +If your log shows + +```text +i2c-imc-skylake 0000:16:1e.5: ch0 command changed under us (...): another master is using the engine +``` + +then firmware is using the engine on your system. Unload the driver. + +Not implemented: the Broadwell-E iMC driver additionally stops the PCU's TSOD +polling for the duration of a transfer. The equivalent register has not been +identified on this part — see [tools/README.md](tools/README.md). + +--- + +## Origin & use case + +This driver started from the need to reach the **ENE KB9012** LED controller on +Kingston FURY DDR4 RGB modules under Linux on X299, which is on the same iMC +SMBus lines. It is device-oriented rather than tied to a DIMM brand; the +practical uses are: + +* reading DDR4 SPD data and DIMM thermal sensors with standard tools; +* OpenRGB and other userspace daemons for modules whose controllers sit on + these buses. --- @@ -47,77 +108,91 @@ This driver is currently submitted for review to the Linux kernel `linux-i2c` su ### Prerequisites -Ensure you have kernel headers and compiler utilities installed: - ```bash # Debian / Ubuntu -sudo apt-get install build-essential linux-headers-$(uname -r) +sudo apt-get install build-essential i2c-tools strace linux-headers-$(uname -r) # Fedora / RHEL -sudo dnf install gcc make kernel-devel +sudo dnf install gcc i2c-tools make strace kernel-devel ``` -### Manual Build & Load +### Build & load -1. **Compile the driver**: - ```bash - make - ``` -2. **Test style compliance**: - ```bash - make checkpatch - ``` -3. **Load immediately (testing)**: - ```bash - make reload - ``` -4. **Install permanently into the kernel tree**: - ```bash - sudo make install - ``` +```bash +make # build +make checkpatch # style +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 +echo 'options i2c-imc-skylake allow_unsafe_access=1' | \ + sudo tee /etc/modprobe.d/i2c-imc-skylake.conf +``` + +This opt-in is intentionally not installed by the project. --- -## DKMS Support (Dynamic Kernel Module Support) +## DKMS -To build and load the driver automatically on every kernel upgrade: - -1. **Install DKMS**: - ```bash - sudo apt-get install dkms - ``` -2. **Register the module**: - ```bash - sudo dkms add . - ``` -3. **Build and install**: - ```bash - sudo dkms install -m i2c-imc-skylake -v 1.0 - ``` +```bash +sudo apt-get install dkms +sudo dkms add . +sudo dkms install -m i2c-imc-skylake -v 1.0.0 +``` --- -## Verification & Testing - -Once loaded, verify that the two channels are registered successfully: +## Verification ```bash i2cdetect -l | grep iMC ``` -Expected output: ```text i2c-6 smbus iMC SMBus Skylake-X channel 0 SMBus adapter i2c-7 smbus iMC SMBus Skylake-X channel 1 SMBus adapter ``` -The engine has no SMBus QUICK/BYTE primitive, so `i2cdetect` cannot scan the bus. Read a device directly with a byte-data access (a register offset is always required), e.g. SPD byte 2 (DDR4 type code `0x0c`) from the EEPROM at `0x50` on channel 0: +The driver advertises SMBus BYTE_DATA and WORD_DATA only, so `i2cdetect` cannot +scan the bus. Read a device directly — SPD byte 2 (DDR4 type code `0x0c`) from +EEPROM `0x50` on channel 0: + ```bash sudo i2cget -y 6 0x50 0x02 ``` +The adapter does not instantiate `ee1004` or `jc42` clients. Check first that +the address is not already bound, then: + +```bash +echo ee1004 0x50 | sudo tee /sys/bus/i2c/devices/i2c-6/new_device +echo jc42 0x18 | sudo tee /sys/bus/i2c/devices/i2c-6/new_device +``` + +Read-only integration test against the module just built: + +```bash +make +sudo make test +``` + +Hardware measurements still owed to the upstream submission are in +[tools/README.md](tools/README.md). + --- ## License -This driver is licensed under the **GPL v2 only** (`GPL-2.0-only`). +GPL v2 only (`GPL-2.0-only`). diff --git a/VERSION b/VERSION new file mode 100644 index 0000000..afaf360 --- /dev/null +++ b/VERSION @@ -0,0 +1 @@ +1.0.0 \ No newline at end of file diff --git a/compat/pci-mmcfg-compat.h b/compat/pci-mmcfg-compat.h new file mode 100644 index 0000000..77a5072 --- /dev/null +++ b/compat/pci-mmcfg-compat.h @@ -0,0 +1,147 @@ +/* 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/dkms.conf b/dkms.conf index b92ec38..737315c 100644 --- a/dkms.conf +++ b/dkms.conf @@ -1,5 +1,5 @@ PACKAGE_NAME="i2c-imc-skylake" -PACKAGE_VERSION="1.0" +PACKAGE_VERSION="$(cat "$(dirname "${BASH_SOURCE[0]}")/VERSION")" BUILT_MODULE_NAME[0]="i2c-imc-skylake" DEST_MODULE_LOCATION[0]="/updates" diff --git a/docs/submission/0001-pci-mmcfg-accessor.patch b/docs/submission/0001-pci-mmcfg-accessor.patch new file mode 100644 index 0000000..78830c0 --- /dev/null +++ b/docs/submission/0001-pci-mmcfg-accessor.patch @@ -0,0 +1,204 @@ +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 b106389..beb641c 100644 --- a/docs/submission/Kconfig.kernel +++ b/docs/submission/Kconfig.kernel @@ -1,6 +1,6 @@ config I2C_IMC_SKYLAKE tristate "Intel Skylake-X iMC SMBus adapter" - depends on PCI && ACPI && (X86_64 || COMPILE_TEST) + depends on PCI && (PCI_MMCONFIG || 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 @@ -8,11 +8,20 @@ config I2C_IMC_SKYLAKE 8086:2085). The engine is exposed through the PCI configuration space of the - PCU function and is driven via ECAM MMIO (not CF8/CFC port I/O, - which is trapped by System Management Mode on this platform). - Two I2C adapters are registered, one per hardware SMBus channel, + 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, allowing access to DDR4 DIMM SPD EEPROMs (0x50-0x57) and thermal sensors from userspace via standard i2c-tools and lm-sensors. + Firmware, SMM, a BMC or CLTT may drive the same engine and no + generic arbitration mechanism is known. The driver therefore + refuses to bind unless the allow_unsafe_access module parameter + is enabled. See Documentation/i2c/busses/i2c-imc-skylake.rst. + If unsure, say N. This driver is only useful on Intel X299 desktop / HEDT systems with the Skylake-X or Cascade Lake-X CPU. + + This driver can also be built as a module. If so, the module + will be called i2c-imc-skylake. diff --git a/docs/submission/cover-letter.txt b/docs/submission/cover-letter.txt index c4d1c92..49b8b37 100644 --- a/docs/submission/cover-letter.txt +++ b/docs/submission/cover-letter.txt @@ -1,120 +1,186 @@ -Subject: [PATCH v3 0/1] i2c: imc-skylake: add Intel Skylake-X iMC SMBus adapter +Subject: [RFC PATCH v4 0/2] i2c: imc-skylake: Intel Skylake-X iMC SMBus adapter -This is v3 of the i2c-imc-skylake driver. v2 was submitted under the name -i2c-imc-x299; the driver has been renamed to reflect that the iMC SMBus -engine is in the Skylake-X / Cascade Lake-X CPU, not in the X299 chipset. +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. -This series adds a driver for the integrated memory controller (iMC) SMBus -engine on Intel Skylake-X / Cascade Lake-X processors (socket LGA 2066, -platform X299, PCU function 8086:2085). +v3 was never posted. v2 was posted as i2c-imc-x299 and received no replies: -The engine provides two SMBus channels — one per pair of DIMM slots — over -which SPD EEPROMs, DDR4 thermal sensors and third-party LED controllers -(e.g. ENE KB9012 at 0x27) are accessible. Exposing it as a pair of standard -Linux I2C adapters lets existing tools (i2c-tools, lm-sensors) use -it without bespoke sysfs hacks. + https://lore.kernel.org/linux-i2c/20260620144131.415559-1-simone.chifari@gmail.com/ -Why ECAM MMIO instead of pci_{read,write}_config_dword -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -On X299 the kernel selects "configuration type 1" (CF8/CFC port I/O) for PCI -config space access, as reported by boot-time dmesg: +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. - PCI: Using configuration type 1 for base access + 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 -System Management Mode traps writes to those ports for this device, so a -standard pci_write_config_dword() targeting the SMBus DATA register never -reaches the hardware — the GO bit never clears and the transaction never -completes. -The Windows NTIOLib (used by Kingston FURY) reaches the same registers via the -ECAM (MMIO) window, which is not trapped. This driver follows the same path: -ioremap() of the ECAM page for the target function and driving the registers -by MMIO read/write. +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. -Note that pci_mmcfg_* helpers are arch-internal and not exported to modules, -so a manual walk of the ACPI MCFG table is used to locate mmcfg_base at probe -time (no module parameter, no hardcoding). +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. -Relation to prior iMC SMBus work -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Two previous attempts to upstream an iMC SMBus driver exist: +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 - - Lutomirski 2013–2016 (Sandy Bridge-EP 8086:3ca8): used CF8/CFC - pci_read/write_config_dword — correct on that platform since SMM does not - trap those ports on Sandy Bridge-EP. Not merged due to missing bus - arbitration (BMC / CLTT sharing) and a required allow_unsafe_access flag. + 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); - - schaecsn 2020 (Broadwell-E 8086:6fa8): added TSOD arbitration using the - Broadwell-documented tsod_polling_interval quiesce procedure. Not merged - (no reviewer response); register layout differs from X299. +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. -This driver is for a different device ID (0x2085), different register layout -(DATA/STATUS/CTRL at 0x9C-0xB8 vs 0x180-0x188), and a fundamentally different -access method (ECAM ioremap). Combining it with the Lutomirski/schaecsn code -would require a large per-generation hw_data table with mutually incompatible -access methods; a separate file is cleaner. +Questions for the PCI and x86 maintainers: -The X299 HEDT (High-End Desktop) platform has no BMC and no CLTT firmware -polling, so the arbitration safety concern of the Lutomirski patch does not -apply. No allow_unsafe_access flag is needed. + - 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? -Independent validation +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. + +Firmware arbitration +~~~~~~~~~~~~~~~~~~~~ +This is the reason no iMC SMBus driver has ever been merged, and this +series does not solve it either. + +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 +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 +original driver's consistency check tripped about every 30 minutes. + + https://lwn.net/Articles/635898/ + https://lore.kernel.org/linux-i2c/1582498270-50674-1-git-send-email-schaecsn@gmx.net/ + +So this driver: + + - keeps the default-off allow_unsafe_access gate, deliberately the same + name as the original; + - serializes both channels under one mutex and waits for the whole engine + to be idle before touching any register; + - saves the command state of both channels, clears TSOD_ACTIVE for the + 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. + +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. + +Relation to prior work ~~~~~~~~~~~~~~~~~~~~~~ -The register layout (DATA/STATUS/CTRL at 0x9C-0xB8, stride 4, GO bit 19, -WORD bit 17) was independently reverse-engineered and confirmed by the -PawnIO.Modules project (SmbusIntelSkylakeIMC.p, Windows userspace, Feb 2026) -and by commercial monitoring tools (HWiNFO, SIV) that access the same PCI -config space via the Windows kernel API (ECAM by default on this platform). +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. + +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 +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 -~~~~~~~~~~~~~~~~~ - - Rename driver from i2c-imc-x299 to i2c-imc-skylake - - Fix iMC SMBus attribution: engine is in the CPU (Skylake-X), not - the X299 chipset - - Add I2C_SMBUS_WORD_DATA support (WORD_BIT bit 17) for TSOD reads - - Update adapter name: "iMC SMBus Skylake-X channel N" - - Use devm_ioremap_uc() for the ECAM mapping (uncached, strongly - ordered) instead of devm_ioremap() - - Use ARRAY_SIZE() for the adapter loop; initialise wval on the - word-read path - - Build FRAME from named bits (ENGINE_ENABLE | GO_BIT) instead of a - magic constant; document the CTRL/DATA/STATUS encoding in full - - Cross-check the iMC bus number against cfg[0xCC] at probe - - Kconfig: depends on PCI && ACPI && (X86_64 || COMPILE_TEST); drop - the CONFIG_ACPI fallback stub accordingly - - Quiet probe: demote informational probe messages to dev_dbg() +~~~~~~~~~~~~~~~~ + - Split into a PCI/x86 patch and the driver; the driver no longer walks + the MCFG table or maps the ECAM page itself. + - 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. + - Wait for both channels to be idle before touching the data latch. + - 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. + - 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(). + - 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. -Testing -~~~~~~~ -Tested on: Intel X299 platform (Skylake-X CPU), 4× DDR4 DIMMs, -kernel 7.0.0-14-generic (Linux Mint 22.3). +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. + - 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. - $ i2cdetect -l | grep iMC - i2c-7 smbus iMC SMBus Skylake-X channel 0 SMBus adapter - i2c-8 smbus iMC SMBus Skylake-X channel 1 SMBus adapter +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. -The driver supports SMBus BYTE_DATA and WORD_DATA transfers. WORD_DATA -uses the engine's WORD_BIT (bit 17) and is needed for DDR4 thermal sensors -(TSOD) which expose 16-bit temperature registers. The engine stores word -data in little-endian order in the CTRL register, so byte-swapping is applied -on both read and write paths. +Tool assistance +~~~~~~~~~~~~~~~ +Per Documentation/process/generated-content.rst: this work was developed +with an AI coding assistant, run interactively rather than autonomously. -Since the hardware requires a register offset for every transaction, -I2C_SMBUS_QUICK and I2C_SMBUS_BYTE (which have no offset) are not supported -(returning -EOPNOTSUPP). For the same reason i2cdetect cannot scan the bus; -access a device directly with a byte-data read, e.g. `i2cget -y 0x50 0x02` -reads SPD byte 2. Standard kernel drivers such as ee1004 (for DDR4 SPD) load -and bind successfully via BYTE_DATA reads. + - 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. + - How it was used: iteratively, one reviewed change at a time, in + 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(). + - 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. -udev autoloads the module on PCI add event (MODULE_DEVICE_TABLE present). -20 rmmod/modprobe cycles: no oops, no warnings, no resource leaks in dmesg. +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 (1): - i2c: imc-skylake: add driver for Intel Skylake-X iMC SMBus engine - - MAINTAINERS | 6 + - drivers/i2c/busses/Kconfig | 19 + - drivers/i2c/busses/Makefile | 1 + - drivers/i2c/busses/i2c-imc-skylake.c | 551 +++++++++++++++++++++++++++++++++ - 4 files changed, 577 insertions(+) +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 diff --git a/docs/submission/i2c-imc-skylake.rst b/docs/submission/i2c-imc-skylake.rst new file mode 100644 index 0000000..c5598ae --- /dev/null +++ b/docs/submission/i2c-imc-skylake.rst @@ -0,0 +1,130 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================= +Kernel driver i2c-imc-skylake +============================= + +Supported adapters: + * Intel Skylake-X / Cascade Lake-X integrated memory controller (iMC) + SMBus engine, PCU function 8086:2085 (socket LGA 2066, platform X299) + +Author: Simone Chifari + + +Description +----------- + +The integrated memory controller of Intel Skylake-X and Cascade Lake-X +processors contains an SMBus engine wired to the DDR4 DIMMs. It reaches the +SPD EEPROMs (0x50-0x57) and the thermal sensors (TSOD, 0x18-0x1f) of the +installed modules, plus whatever else a particular module carries on those +lines. + +The engine has two independent channels. DIMM slots 1 and 2 are on channel 0, +slots 3 and 4 on channel 1, so the driver registers two i2c adapters:: + + $ i2cdetect -l | grep iMC + i2c-6 smbus iMC SMBus Skylake-X channel 0 SMBus adapter + i2c-7 smbus iMC SMBus Skylake-X channel 1 SMBus adapter + +Both channels share one hardware engine, so the driver serializes every +transaction across both adapters. + + +This driver is not enabled by default +------------------------------------- + +The SMBus engine has no arbitration mechanism. System Management Mode, a BMC, +and the memory controller's own closed-loop thermal throttling (CLTT) can all +drive the same registers, and nothing in the PCI ID tells the driver whether +any of them is doing so on a given system. Two masters using the engine at the +same time can send a transaction to the wrong address or return the wrong data. + +The driver therefore refuses to bind unless the operator opts in:: + + modprobe i2c-imc-skylake allow_unsafe_access=1 + +Do this only on a system where concurrent firmware access has been excluded. +Desktop and HEDT boards have no BMC, but CLTT may still be enabled by the +firmware. + +The driver reduces, but cannot remove, the risk: + + * every transaction waits for both channels to report idle before it + touches any register; + * the previous command state of both channels is saved, the TSOD-active + state is cleared for the duration of the transfer, and both are restored + afterwards; + * after each transfer the command registers are compared against what the + driver left in them. A difference means another master used the engine, + and the transfer is failed with ``-EAGAIN`` rather than reported as + successful. + +The last check is what makes an unarbitrated system detectable: if the log +shows + + ``chN command changed under us (...): another master is using the engine`` + +then firmware is using the engine and the driver should not be loaded on that +system. + + +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. + + +Supported transactions +---------------------- + +The driver implements SMBus Read/Write Byte Data and Read/Write Word Data. +That covers ee1004 (DDR4 SPD) and jc42 (TSOD). + +SMBus Quick and Receive Byte are not implemented, so ``i2cdetect`` cannot scan +these buses. Address a device directly instead, for example SPD byte 2 of the +EEPROM at 0x50:: + + $ i2cget -y 6 0x50 0x02 + 0x0c + +Block transfers are not implemented. The engine has block primitives, but no +device on this bus needs them. + +Word transfers use the standard SMBus byte order, so ``jc42`` reading through +``i2c_smbus_read_word_swapped()`` gets the value it expects. + + +Instantiating clients +--------------------- + +The adapter does not create ``ee1004`` or ``jc42`` clients by itself. There is +no firmware description of these buses and the driver does not scan. Create +them explicitly if you want them:: + + # echo ee1004 0x50 > /sys/bus/i2c/devices/i2c-6/new_device + # echo jc42 0x18 > /sys/bus/i2c/devices/i2c-6/new_device + +Check first that the address is not already bound to a driver. + + +Module parameters +----------------- + +allow_unsafe_access + Bool, read-only after load, defaults to false. The driver does not bind + unless this is set. See the section above. diff --git a/i2c-imc-skylake.c b/i2c-imc-skylake.c index e1df8ab..202450f 100644 --- a/i2c-imc-skylake.c +++ b/i2c-imc-skylake.c @@ -5,30 +5,28 @@ * 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 (0000:16:1e.5, 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. + * 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 ECAM MMIO instead of the usual CF8/CFC config accessors: - * On this platform System Management Mode (SMM) traps and mangles port-based - * (CF8/CFC) writes to the first 256 config bytes of this function, so the SMBus - * command never completes. The boot log reports "PCI: Using configuration - * type 1 (probe)" confirming the default path is port-based; the MMCONFIG - * (ECAM) window is not trapped. Windows' NTIOLib reaches the registers via - * ECAM, which is how we confirmed the register layout on hardware. We map the - * ECAM page of the target function and drive the registers by MMIO, exactly as - * the firmware does. + * 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. * - * ECAM phys(off) = mmcfg_base + (bus<<20) + (dev<<15) + (fn<<12) + off - * mmcfg_base is read from the ACPI MCFG table at probe time (not hardcoded). - * - * Per-channel register triple within the config page: + * Per-channel register triple within the config space of the function: * ch0 ch1 * CTRL (data) 0xB4 0xB8 write: data byte in bits[23:16]; read: low byte - * DATA (cmd) 0x9C 0xA0 FRAME | (cmd << 8) | reg ; bit19 = GO - * STATUS 0xA8 0xAC busy while bit0 set; done when clear, - * bit1 (0x02) set on completion = device NACKed + * DATA (cmd) 0x9C 0xA0 command toggle | GO | address | register + * STATUS 0xA8 0xAC bit0 BUSY, bit1 ERROR/NACK, + * bit2 READ_DONE, bit3 WRITE_DONE * 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 @@ -40,21 +38,27 @@ * all reachable by 7-bit address. * * Bus arbitration / concurrency with firmware: - * The engine could in principle also be driven by SMM or by the iMC's own - * closed-loop thermal throttling (CLTT) TSOD polling. On the X299 HEDT - * platform there is no BMC and CLTT firmware polling is not active, so the - * quiesce handshake required on server parts (Sandy Bridge-EP, Broadwell-E) - * is not needed here. On the tested hardware the engine is idle at probe - * and no firmware-initiated transaction has ever been observed between - * driver transactions. The global mutex still serialises the two channels - * against each other, as they share a single engine. + * The driver cannot coordinate with SMM, a BMC, or the iMC's own closed-loop + * thermal throttling (CLTT), and the PCI ID alone does not prove that none of + * them is active. Binding is therefore disabled by default and requires + * allow_unsafe_access=1, following the same choice made by the earlier iMC + * drivers for Sandy Bridge-EP and Broadwell-E. On top of that, transactions + * are serialized across both channels, wait for the complete engine to become + * idle, temporarily clear TSOD_ACTIVE, restore the previous command state, + * and verify afterwards that no other master modified the command registers + * while the transfer was in flight. + * + * Not implemented: the Broadwell-E driver additionally stops the PCU's TSOD + * polling for the duration of a transfer (write 0 to the polling interval, + * wait 10 ms for in-flight transactions to drain, restore afterwards). The + * equivalent register has not been identified on this part, so a system with + * CLTT enabled is expected to trip the interference check below rather than + * to be arbitrated against. */ -#include #include #include #include -#include #include #include #include @@ -66,13 +70,11 @@ #define PCU_DEVICE 0x2085 #define PCU_ID ((PCU_DEVICE << 16) | PCI_VENDOR_ID_INTEL) -#define CFG_SIZE 0x1000UL - -/* function-global config registers (offsets within the config page) */ -#define CFG_VENDOR_DEV 0x00 /* cfg[0]: vendor/device id, for ECAM sanity check */ +/* 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 within the config page */ +/* per-channel register offsets */ #define CH0_CTRL 0xB4 #define CH0_DATA 0x9C #define CH0_STAT 0xA8 @@ -82,7 +84,10 @@ /* * Command word written to the DATA register: - * bits[31:16] = FRAME (engine config + GO bit19), constant + * bit29 = command toggle + * bit20 = TSOD active state, preserved across transfers + * bit19 = GO + * bit17 = word transfer * bits[15:8] = SMBus command byte = (rw << 7) | addr7 * rw bit (0x80): 1 = write, 0 = read * addr7 : 7-bit SMBus slave address @@ -91,16 +96,43 @@ * encoding was confirmed on hardware: command 0x50 reads SPD EEPROM 0x50 * (DDR4 signature). */ -#define ENGINE_ENABLE BIT(29) /* engine enable bit */ +#define COMMAND_TOGGLE BIT(29) #define GO_BIT BIT(19) /* start transaction */ -#define FRAME (ENGINE_ENABLE | GO_BIT) -#define RW_WRITE 0x80 /* OR into the command byte for a write */ +#define TSOD_ACTIVE_BIT BIT(20) #define WORD_BIT BIT(17) /* 16-bit word transfer (vs 8-bit byte) */ +#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. + */ +#define COMMAND_OUR_BITS (COMMAND_TOGGLE | GO_BIT | WORD_BIT | \ + GENMASK(15, 0)) #define STAT_BUSY BIT(0) /* low bit set while transaction in flight */ -#define STAT_NACK BIT(1) /* set on completion if the device NACKed */ +#define STAT_ERROR BIT(1) +#define STAT_READ_DONE BIT(2) +#define STAT_WRITE_DONE BIT(3) +#define STAT_ANY_DONE (STAT_ERROR | STAT_READ_DONE | STAT_WRITE_DONE) + +/* + * Polling bounds. Transactions on the tested system complete in a few + * milliseconds; these are upper bounds that avoid busy-spinning and cover + * device clock stretching, not a measured multi-platform worst case. The + * mutex is held across a whole transfer, so the worst case a caller on the + * other channel can wait for is the sum of all of them, roughly 750 ms. + */ +#define IMC_POLL_US 10 +#define IMC_IDLE_TIMEOUT_US 50000 +#define IMC_GO_TIMEOUT_US 200000 +#define IMC_DONE_TIMEOUT_US 50000 + +static bool allow_unsafe_access; +module_param(allow_unsafe_access, bool, 0444); +MODULE_PARM_DESC(allow_unsafe_access, + "allow access without firmware/SMM arbitration (unsafe)"); struct imc_chan { - u32 ctrl, data, stat; + unsigned int ctrl, data, stat; int idx; /* channel index 0 or 1 */ }; @@ -111,229 +143,301 @@ static const struct imc_chan imc_chans[2] = { /* one driver state object, shared by both per-channel adapters */ struct imc_smbus { + struct pci_dev *pdev; /* config space carrying the engine */ struct device *dev; /* &pdev->dev, for dev_*() logging */ - void __iomem *cfg; /* ioremapped ECAM page of the function */ struct mutex lock; /* serialises all SMBus transactions */ - /* needed: both channels share same ECAM mapping */ + /* needed: both channels share one engine */ struct i2c_adapter adap[2]; /* one per hardware channel */ + int io_err; /* sticky config-access error, under lock */ +}; + +struct imc_xfer_state { + u32 command[ARRAY_SIZE(imc_chans)]; + bool restore[ARRAY_SIZE(imc_chans)]; }; /* - * ECAM base discovery from ACPI MCFG. acpi_table_parse() is not exported to - * modules, so map the MCFG table with acpi_get_table() (exported) and walk the - * allocation entries by hand. Uses pdev's PCI segment and bus number so no - * module parameter override is needed. CONFIG_ACPI is guaranteed by Kconfig. + * 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. */ -static u64 imc_detect_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; -} - -/* - * Wait until GO clears (transaction issued), then until the busy bit drops. - * Returns 0 on completion, -ETIMEDOUT if the engine never went idle. On - * success *status (if non-NULL) gets the final status word. Process context - * only - it sleeps between polls. - * - * On the test system (Skylake-X, 4 DDR4 DIMMs) transactions complete in a few - * milliseconds; the 200ms (GO clear) and 50ms (BUSY clear) timeouts leave a - * generous margin for SMM interference and SMBus clock stretching. - */ -static int imc_wait(struct imc_smbus *s, const struct imc_chan *c, u32 *status) +static u32 imc_reg(struct imc_smbus *s, unsigned int off) { u32 val; int ret; - ret = readl_poll_timeout(s->cfg + c->data, val, !(val & GO_BIT), 10, 200000); - if (ret) - return ret; - - ret = readl_poll_timeout(s->cfg + c->stat, val, !(val & STAT_BUSY), 10, 50000); - if (ret) - return ret; - - if (status) - *status = val; - - return 0; -} - -/* - * Poll the busy bit clear only (no GO check). The firmware polls STATUS after - * the CTRL (data-latch) write too, before issuing the DATA/GO word. - * Timeout: 50ms, same rationale as in imc_wait(). - * - * Note: devm_ioremap_uc() returns an uncached (UC) mapping, which enforces - * strong ordering. writel() includes a full mb() barrier, ensuring the write - * is visible to hardware before polling begins. - */ -static int imc_wait_status(struct imc_smbus *s, const struct imc_chan *c) -{ - u32 val; - int ret; - - ret = readl_poll_timeout(s->cfg + c->stat, val, !(val & STAT_BUSY), 10, 50000); + ret = pci_mmcfg_read_config(s->pdev, off, 4, &val); if (ret) { - dev_dbg(s->dev, "pre-command poll timed out, engine still busy\n"); - return ret; + s->io_err = ret; + return ~0U; } - return 0; + return val; } -/* translate a completed transaction's status into an errno (0 = ACK) */ -static int imc_check_status(struct imc_smbus *s, u32 status, u8 addr, u8 reg) +static void imc_set(struct imc_smbus *s, unsigned int off, u32 val) { - if (status & STAT_NACK) { - /* no device at this address, or it refused the access */ + int ret; + + ret = pci_mmcfg_write_config(s->pdev, off, 4, val); + if (ret) + s->io_err = ret; +} + +static u32 imc_busy_mask(struct imc_smbus *s) +{ + u32 busy = 0; + int i; + + for (i = 0; i < ARRAY_SIZE(imc_chans); i++) + if (imc_reg(s, imc_chans[i].stat) & STAT_BUSY) + busy |= BIT(i); + + return busy; +} + +static int imc_wait_engine_idle(struct imc_smbus *s) +{ + u32 busy; + int ret; + + ret = read_poll_timeout(imc_busy_mask, busy, !busy || s->io_err, + IMC_POLL_US, IMC_IDLE_TIMEOUT_US, false, s); + if (ret) + dev_warn_ratelimited(s->dev, + "engine busy on channel mask 0x%x\n", busy); + + return ret; +} + +static int imc_wait_go_clear(struct imc_smbus *s, const struct imc_chan *c) +{ + u32 val; + + return read_poll_timeout(imc_reg, val, !(val & GO_BIT) || s->io_err, + IMC_POLL_US, IMC_GO_TIMEOUT_US, false, + s, c->data); +} + +static int imc_wait_done(struct imc_smbus *s, const struct imc_chan *c, + u32 *status) +{ + u32 val; + int ret; + + ret = read_poll_timeout(imc_reg, val, + (!(val & STAT_BUSY) && (val & STAT_ANY_DONE)) || + s->io_err, + IMC_POLL_US, IMC_DONE_TIMEOUT_US, false, + s, c->stat); + *status = val; + + return ret; +} + +static int imc_wait_transfer(struct imc_smbus *s, const struct imc_chan *c, + u32 *status) +{ + u32 command; + int ret; + + ret = imc_wait_go_clear(s, c); + if (ret) + *status = imc_reg(s, c->stat); + else + ret = imc_wait_done(s, c, status); + if (!ret) + return 0; + if (!(*status & STAT_BUSY) || (*status & STAT_ANY_DONE)) + return ret; + + 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) + dev_warn_ratelimited(s->dev, + "ch%d recovery timed out (stat 0x%08x)\n", + c->idx, *status); + + return ret; +} + +static int imc_check_status(struct imc_smbus *s, const struct imc_chan *c, + u32 status, u32 expected, u8 addr, u8 reg) +{ + if (status & STAT_ERROR) { dev_dbg(s->dev, "addr 0x%02x reg 0x%02x NACK (stat 0x%08x)\n", addr, reg, status); return -ENXIO; } - return 0; -} -/* SMBus write-byte to addr: latch the data byte, then issue the command. */ -static int imc_write_byte(struct imc_smbus *s, const struct imc_chan *c, - u8 addr, u8 reg, u8 val) -{ - u32 status = 0; - int ret; - - writel((u32)val << 16, s->cfg + c->ctrl); - ret = imc_wait_status(s, c); - if (ret) - return ret; - writel(FRAME | ((u32)(addr | RW_WRITE) << 8) | reg, s->cfg + c->data); - ret = imc_wait(s, c, &status); - if (ret) { + /* Done bits are latched; only the bit for this operation is required. */ + if (!(status & expected)) { dev_warn_ratelimited(s->dev, - "write addr 0x%02x reg 0x%02x timed out\n", - addr, reg); - return ret; + "ch%d unexpected completion for addr 0x%02x reg 0x%02x (stat 0x%08x)\n", + c->idx, addr, reg, status); + return -EIO; } - ret = imc_check_status(s, status, addr, reg); - if (ret) - return ret; return 0; } -/* SMBus read-byte from addr: issue the command, return the low data byte. */ -static int imc_read_byte(struct imc_smbus *s, const struct imc_chan *c, - u8 addr, u8 reg, u8 *val) -{ - u32 status = 0; - int ret; - - ret = imc_wait_status(s, c); - if (ret) - return ret; - writel(FRAME | ((u32)addr << 8) | reg, s->cfg + c->data); - ret = imc_wait(s, c, &status); - if (ret) { - dev_warn_ratelimited(s->dev, - "read addr 0x%02x reg 0x%02x timed out\n", - addr, reg); - return ret; - } - ret = imc_check_status(s, status, addr, reg); - if (ret) - return ret; - *val = readl(s->cfg + c->ctrl) & 0xFF; - return 0; -} - /* - * SMBus write-word to addr: latch the 16-bit value into CTRL[31:16], then - * issue the command with WORD_BIT set. The value is byte-swapped between host - * order and the engine's CTRL byte layout (swab16) so that data->word follows - * the standard SMBus convention (low data byte first). The read path applies - * the inverse swap. + * 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. * - * Validated on hardware: a WORD read returns the same byte order as two - * consecutive BYTE_DATA reads (word low byte == register R, high byte == R+1), - * confirmed against DDR4 SPD bytes. This is exactly the convention jc42 relies - * on via i2c_smbus_read_word_swapped(), so no double swap occurs. + * 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. */ -static int imc_write_word(struct imc_smbus *s, const struct imc_chan *c, - u8 addr, u8 reg, u16 val) +static bool imc_engine_responded(bool go_seen, u32 status_pre, u32 status) { - u32 status = 0; - int ret; - - /* host order -> engine CTRL byte order (see function comment) */ - writel((u32)swab16(val) << 16, s->cfg + c->ctrl); - ret = imc_wait_status(s, c); - if (ret) - return ret; - writel(FRAME | WORD_BIT | ((u32)(addr | RW_WRITE) << 8) | reg, - s->cfg + c->data); - ret = imc_wait(s, c, &status); - if (ret) { - dev_warn_ratelimited(s->dev, - "write word addr 0x%02x reg 0x%02x timed out\n", - addr, reg); - return ret; - } - return imc_check_status(s, status, addr, reg); + return go_seen || status != status_pre; } /* - * SMBus read-word from addr: issue the command with WORD_BIT set, return the - * 16-bit value from CTRL[15:0] byte-swapped back to host order (inverse of the - * write path). See imc_write_word() for the byte-order convention. + * Detect a second master. Compare the command registers against what this + * driver left in them: for the channel it drove, only the bits it does not own; + * for the other channel, all of them. A difference means firmware, a BMC or + * CLTT touched the engine while the transfer was in flight, so the result + * cannot be trusted. */ -static int imc_read_word(struct imc_smbus *s, const struct imc_chan *c, - u8 addr, u8 reg, u16 *val) +static int imc_check_interference(struct imc_smbus *s, const struct imc_chan *c, + const struct imc_xfer_state *state) { - u32 status = 0, raw; + int i; + + for (i = 0; i < ARRAY_SIZE(imc_chans); i++) { + u32 mask = i == c->idx ? ~COMMAND_OUR_BITS : ~0U; + u32 expect = state->command[i] & COMMAND_KEEP_MASK; + u32 now = imc_reg(s, imc_chans[i].data); + + if (!((now ^ expect) & mask)) + continue; + + dev_warn_ratelimited(s->dev, + "ch%d command changed under us (0x%08x -> 0x%08x): another master is using the engine\n", + i, expect, now); + return -EAGAIN; + } + + return 0; +} + +static void imc_restore_xfer(struct imc_smbus *s, + const struct imc_xfer_state *state) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(imc_chans); i++) { + if (!state->restore[i]) + continue; + /* + * Mask GO: the saved word is only meant to put the previous + * address, register and TSOD state back, never to re-arm a + * 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); + } +} + +static int imc_prepare_xfer(struct imc_smbus *s, const struct imc_chan *c, + struct imc_xfer_state *state) +{ + int i, ret; + + ret = imc_wait_engine_idle(s); + if (ret) + return ret; + + for (i = 0; i < ARRAY_SIZE(imc_chans); i++) { + state->command[i] = imc_reg(s, imc_chans[i].data); + state->restore[i] = i == c->idx || + (state->command[i] & TSOD_ACTIVE_BIT); + if (!(state->command[i] & TSOD_ACTIVE_BIT)) + continue; + + 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); + return ret; + } + } + + return 0; +} + +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; int ret; - ret = imc_wait_status(s, c); + ret = imc_prepare_xfer(s, c, &state); if (ret) return ret; - writel(FRAME | WORD_BIT | ((u32)addr << 8) | reg, s->cfg + c->data); - ret = imc_wait(s, c, &status); - if (ret) { - dev_warn_ratelimited(s->dev, - "read word addr 0x%02x reg 0x%02x timed out\n", - addr, reg); - return ret; + + command = COMMAND_PREFIX | ((u32)addr << 8) | reg; + if (size == I2C_SMBUS_WORD_DATA) + command |= WORD_BIT; + + 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; } - ret = imc_check_status(s, status, addr, reg); + + 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; + + ret = imc_wait_transfer(s, c, &status); if (ret) - return ret; - raw = readl(s->cfg + c->ctrl) & 0xFFFF; - *val = swab16(raw); - return 0; + 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); + if (ret) + goto restore_command; + + ret = imc_check_interference(s, c, &state); + if (ret) + goto restore_command; + + if (read_write == I2C_SMBUS_READ) { + raw = imc_reg(s, c->ctrl); + *value = size == I2C_SMBUS_WORD_DATA ? + swab16(raw & 0xffff) : raw & 0xff; + } + +restore_command: + imc_restore_xfer(s, &state); + + return ret; } /* @@ -349,46 +453,60 @@ static s32 imc_smbus_xfer(struct i2c_adapter *adap, u16 addr, { struct imc_smbus *s = i2c_get_adapdata(adap); const struct imc_chan *c = adap->algo_data; - u8 reg, val = 0; + u16 val = 0; + u8 reg; int ret; + if (flags & I2C_M_TEN) + return -EAFNOSUPPORT; if (addr > 0x7f) return -EINVAL; if (size != I2C_SMBUS_BYTE_DATA && size != I2C_SMBUS_WORD_DATA) return -EOPNOTSUPP; + if (read_write != I2C_SMBUS_READ && read_write != I2C_SMBUS_WRITE) + return -EINVAL; reg = command; mutex_lock(&s->lock); + s->io_err = 0; + if (read_write == I2C_SMBUS_WRITE) { if (size == I2C_SMBUS_WORD_DATA) { dev_dbg(s->dev, "ch%d W addr=%02x reg=%02x val=%04x\n", c->idx, addr, reg, data->word); - ret = imc_write_word(s, c, addr, reg, data->word); + val = data->word; } else { val = data->byte; dev_dbg(s->dev, "ch%d W addr=%02x reg=%02x val=%02x\n", c->idx, addr, reg, val); - ret = imc_write_byte(s, c, addr, reg, val); } + ret = imc_access(s, c, addr, reg, read_write, size, &val); } else { + ret = imc_access(s, c, addr, reg, read_write, size, &val); if (size == I2C_SMBUS_WORD_DATA) { - u16 wval = 0; - - ret = imc_read_word(s, c, addr, reg, &wval); if (!ret) - data->word = wval; + data->word = val; dev_dbg(s->dev, "ch%d R addr=%02x reg=%02x -> %04x (ret %d)\n", - c->idx, addr, reg, wval, ret); + c->idx, addr, reg, val, ret); } else { - ret = imc_read_byte(s, c, addr, reg, &val); if (!ret) data->byte = val; dev_dbg(s->dev, "ch%d R addr=%02x reg=%02x -> %02x (ret %d)\n", c->idx, addr, reg, val, ret); } } + + /* + * A config-space failure invalidates whatever the transaction logic + * concluded, so it wins over any other result. + */ + if (s->io_err) { + dev_warn_ratelimited(s->dev, "config access failed: %d\n", + s->io_err); + ret = s->io_err; + } mutex_unlock(&s->lock); return ret; @@ -404,70 +522,68 @@ static const struct i2c_algorithm imc_algo = { .functionality = imc_func, }; -static void imc_mutex_destroy(void *data) +static int imc_suspend(struct device *dev) { - struct mutex *lock = data; + struct imc_smbus *s = dev_get_drvdata(dev); + int i; - mutex_destroy(lock); + for (i = 0; i < ARRAY_SIZE(s->adap); i++) + i2c_mark_adapter_suspended(&s->adap[i]); + + return 0; } +static int imc_resume(struct device *dev) +{ + struct imc_smbus *s = dev_get_drvdata(dev); + int i; + + for (i = 0; i < ARRAY_SIZE(s->adap); i++) + i2c_mark_adapter_resumed(&s->adap[i]); + + return 0; +} + +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) { - resource_size_t phys; struct imc_smbus *s; u8 imc_bus_hw; - u64 base; u32 cfg0, cc; int ret, i; + if (!allow_unsafe_access) { + dev_info(&pdev->dev, + "firmware/SMM arbitration is unknown; set allow_unsafe_access=1 to bind\n"); + return -ENODEV; + } + s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL); if (!s) return -ENOMEM; - ret = pcim_enable_device(pdev); - if (ret) { - dev_err(&pdev->dev, "cannot enable PCI device: %d\n", ret); - return ret; - } - + /* + * No pci_enable_device(): the driver uses no BAR, no interrupt and no + * DMA. The engine is reached entirely through config space, which does + * not require the function's memory or I/O decoding to be enabled. + */ + s->pdev = pdev; s->dev = &pdev->dev; - mutex_init(&s->lock); - ret = devm_add_action_or_reset(&pdev->dev, imc_mutex_destroy, &s->lock); + pci_set_drvdata(pdev, s); + ret = devm_mutex_init(&pdev->dev, &s->lock); if (ret) return ret; - base = imc_detect_mmcfg_base(pdev); - if (!base) { - dev_err(&pdev->dev, "cannot resolve MMCONFIG base\n"); - return -ENODEV; + 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; } - - 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 + CFG_SIZE < phys) { - dev_err(&pdev->dev, "ECAM address overflow\n"); - return -EINVAL; - } - - /* - * Deliberately no request_mem_region(): the MMCONFIG window is already - * claimed as a firmware/PCI resource, so a reservation would fail with - * -EBUSY. The pci_driver binding keeps the function alive; the registers - * we drive are side-band controls the kernel does not otherwise touch. - */ - s->cfg = devm_ioremap_uc(&pdev->dev, phys, CFG_SIZE); - if (!s->cfg) { - dev_err(&pdev->dev, "ioremap(%pa) failed\n", &phys); - return -ENOMEM; - } - - cfg0 = readl(s->cfg + CFG_VENDOR_DEV); if (cfg0 != PCU_ID) { - dev_err(&pdev->dev, "wrong device at ECAM %pa (cfg[0]=0x%08x)\n", - &phys, cfg0); + dev_err(&pdev->dev, "wrong device through ECAM (cfg[0]=0x%08x)\n", + cfg0); return -ENODEV; } @@ -476,10 +592,12 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) * 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 ECAM walk landed on the wrong slot — warn but + * A mismatch means the accessor landed on the wrong function - warn but * continue; the binding is already locked to 8086:2085. */ - cc = readl(s->cfg + CFG_IMC_BUS); + 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) @@ -491,8 +609,6 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) "cfg[0xCC]=0x%08x iMC bus 0x%02x confirmed\n", cc, imc_bus_hw); - dev_dbg(&pdev->dev, "ECAM mapped at %pa\n", &phys); - /* * Lifetime safety: the I2C core guarantees that smbus_xfer callbacks * are not invoked after i2c_del_adapter() returns. Since we use @@ -501,17 +617,19 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) */ for (i = 0; i < ARRAY_SIZE(s->adap); i++) { struct i2c_adapter *a = &s->adap[i]; - int n; a->owner = THIS_MODULE; a->algo = &imc_algo; a->algo_data = (void *)&imc_chans[i]; a->dev.parent = &pdev->dev; + /* + * One retry: interference from another master is reported as + * -EAGAIN and is usually over by the next attempt. + */ + a->retries = 1; i2c_set_adapdata(a, s); - n = snprintf(a->name, sizeof(a->name), - "iMC SMBus Skylake-X channel %d", i); - if (n >= sizeof(a->name)) - dev_warn(&pdev->dev, "adapter name truncated\n"); + snprintf(a->name, sizeof(a->name), + "iMC SMBus Skylake-X channel %d", i); ret = devm_i2c_add_adapter(&pdev->dev, a); if (ret) { @@ -532,20 +650,21 @@ static const struct pci_device_id imc_pci_ids[] = { MODULE_DEVICE_TABLE(pci, imc_pci_ids); /* - * All resources (ioremap, i2c adapters, mutex) are devm-managed and are - * released automatically after probe returns or during device unbinding, so - * no .remove callback is needed. The register state is deliberately left - * as-is on unbind: the engine carries no driver-private latched state that - * needs teardown, and the SMBus controls we drive are side-band registers the - * rest of the kernel does not touch. + * All resources (mutex, i2c adapters) are devm-managed and are released + * automatically after probe returns or during device unbinding, so no .remove + * callback is needed. The register state is deliberately left as-is on + * unbind: the engine carries no driver-private latched state that needs + * teardown, and the SMBus controls we drive are side-band registers the rest + * of the kernel does not touch. */ static struct pci_driver imc_driver = { .name = "i2c-imc-skylake", .id_table = imc_pci_ids, .probe = imc_pci_probe, + .driver.pm = pm_sleep_ptr(&imc_pm_ops), }; module_pci_driver(imc_driver); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Simone Chifari"); -MODULE_DESCRIPTION("Intel Skylake-X iMC SMBus I2C adapter (ECAM MMIO)"); +MODULE_DESCRIPTION("Intel Skylake-X iMC SMBus I2C adapter"); diff --git a/test-smoke.sh b/test-smoke.sh index 8a7b0ae..c575ae4 100755 --- a/test-smoke.sh +++ b/test-smoke.sh @@ -1,91 +1,252 @@ #!/usr/bin/env bash -# Smoke test for i2c-imc-skylake (pci_driver, no bus/dev/fn params). -# Run after: make install OR make reload -# Requires: root, i2c-tools (i2cdetect), module already loaded. +# Read-only smoke test for i2c-imc-skylake. set -euo pipefail PASS=0 FAIL=0 +CH0="" +CH1="" +CH0_ADDR="" +CH1_ADDR="" -_ok() { echo "PASS: $*"; ((PASS++)) || true; } -_fail() { echo "FAIL: $*"; ((FAIL++)) || true; } - -echo "=== adapter list ===" -i2cdetect -l | grep -i imc || _fail "no imc adapters visible" - -_bus() { - i2cdetect -l 2>/dev/null | grep -i "imc.*channel $1" | awk '{print $1}' | tr -d 'i2c-' -} - -CH0=$(_bus 0) -CH1=$(_bus 1) - -if [ -n "$CH0" ]; then - _ok "ch0 adapter at i2c-$CH0" +if ((EUID == 0)); then + SUDO=() else - _fail "ch0 adapter not found" -fi - -if [ -n "$CH1" ]; then - _ok "ch1 adapter at i2c-$CH1" -else - _fail "ch1 adapter not found" -fi - -# The engine has no QUICK/BYTE primitive, so `i2cdetect` cannot scan the bus -# ("Bus doesn't support detection commands"). Probe SPD addresses directly with -# a byte-data read of register 0x00 instead. -_scan_spd() { - local bus=$1 found="" - echo "--- SPD scan bus $bus (byte-data read of reg 0x00) ---" - for a in 0x50 0x51 0x52 0x53 0x54 0x55 0x56 0x57; do - if v=$(i2cget -y "$bus" "$a" 0x00 2>/dev/null); then - echo " $a present (reg0=$v)" - found="$a" - fi - done - [ -n "$found" ] -} - -if [ -n "$CH0" ]; then - echo "" - if _scan_spd "$CH0"; then - _ok "SPD device present on ch0" - else - _fail "no SPD device on ch0" + SUDO=(sudo -n) + if ! "${SUDO[@]}" true 2>/dev/null; then + echo "Run as root or configure non-interactive sudo." >&2 + exit 2 fi fi -if [ -n "$CH1" ]; then - echo "" - _scan_spd "$CH1" && _ok "SPD device present on ch1" || true +MODULE_PATH=${MODULE_PATH:-$(modinfo -n i2c-imc-skylake)} +if [[ ! -r "$MODULE_PATH" ]]; then + echo "Module not found: $MODULE_PATH" >&2 + exit 2 fi -echo "" -echo "=== udev autoload ===" -sudo rmmod i2c-imc-skylake -sleep 1 -sudo udevadm trigger --subsystem-match=pci --action=add -sleep 2 -if lsmod | grep i2c_imc_skylake >/dev/null; then - _ok "udev autoload" -else - _fail "udev did not reload module — check /etc/modules-load.d/ or udev rules" +_ok() +{ + echo "PASS: $*" + ((PASS++)) || true +} + +_fail() +{ + echo "FAIL: $*" + ((FAIL++)) || true +} + +_module_loaded() +{ + lsmod | awk '$1 == "i2c_imc_skylake" { found = 1 } END { exit !found }' +} + +_bus() +{ + local channel=$1 + + i2cdetect -l 2>/dev/null | awk -v channel="$channel" ' + tolower($0) ~ /imc.*channel/ && $0 ~ "channel " channel { + sub(/^i2c-/, "", $1) + print $1 + exit + }' +} + +_find_pci_device() +{ + local device path vendor + + for path in /sys/bus/pci/devices/*; do + [[ -r "$path/vendor" && -r "$path/device" ]] || continue + read -r vendor < "$path/vendor" + read -r device < "$path/device" + if [[ "$vendor" == "0x8086" && "$device" == "0x2085" ]]; then + printf '%s\n' "$path" + return 0 + fi + done + + return 1 +} + +_refresh_buses() +{ + CH0=$(_bus 0) + CH1=$(_bus 1) +} + +_scan_spd() +{ + local bus=$1 result_var=$2 + local address address_hex device readable="" value + local found=false + + echo "--- SPD scan bus $bus (read-only BYTE_DATA) ---" + for address in {80..87}; do + printf -v address_hex '0x%02x' "$address" + printf -v device '%d-%04x' "$bus" "$address" + if [[ -L "/sys/bus/i2c/devices/$device/driver" ]]; then + echo " $address_hex present (bound to $(basename "$(readlink "/sys/bus/i2c/devices/$device/driver")"))" + found=true + continue + fi + if value=$("${SUDO[@]}" i2cget -y "$bus" "$address_hex" 0x00 2>/dev/null); then + echo " $address_hex present (reg0=$value)" + found=true + readable=${readable:-$address_hex} + fi + done + + printf -v "$result_var" '%s' "$readable" + $found +} + +_word_test() +{ + local bus=$1 address=$2 + local byte0 byte1 expected word + + if ! byte0=$("${SUDO[@]}" i2cget -y "$bus" "$address" 0x00 2>/dev/null) || + ! byte1=$("${SUDO[@]}" i2cget -y "$bus" "$address" 0x01 2>/dev/null) || + ! word=$("${SUDO[@]}" i2cget -y "$bus" "$address" 0x00 w 2>/dev/null); then + return 1 + fi + + expected=$((byte0 | (byte1 << 8))) + ((word == expected)) +} + +_nack_test() +{ + local bus=$1 + local trace + + command -v strace >/dev/null || return 1 + trace=$("${SUDO[@]}" strace -qq -f -e trace=ioctl \ + i2cget -y "$bus" 0x77 0x00 2>&1 || true) + grep -q '= -1 ENXIO' <<< "$trace" +} + +_stress_reader() +{ + local bus=$1 address=$2 + local iteration + + for iteration in {1..100}; do + "${SUDO[@]}" i2cget -y "$bus" "$address" 0x02 >/dev/null 2>&1 || return 1 + done +} + +_load_unsafe() +{ + "${SUDO[@]}" insmod "$MODULE_PATH" allow_unsafe_access=1 +} + +DMESG_START=$("${SUDO[@]}" dmesg | wc -l) +"${SUDO[@]}" modprobe i2c-dev +_module_loaded && "${SUDO[@]}" rmmod i2c-imc-skylake +_load_unsafe + +echo "=== adapter discovery ===" +_refresh_buses +[[ -n "$CH0" ]] && _ok "ch0 adapter at i2c-$CH0" || _fail "ch0 adapter not found" +[[ -n "$CH1" ]] && _ok "ch1 adapter at i2c-$CH1" || _fail "ch1 adapter not found" + +if [[ -n "$CH0" ]]; then + _scan_spd "$CH0" CH0_ADDR && _ok "SPD device present on ch0" || _fail "no SPD device on ch0" +fi +if [[ -n "$CH1" ]]; then + _scan_spd "$CH1" CH1_ADDR && _ok "SPD device present on ch1" || _fail "no SPD device on ch1" fi -echo "" -echo "=== load/unload x5 (leak/oops) ===" -for i in $(seq 1 5); do - sudo rmmod i2c-imc-skylake - sudo modprobe i2c-imc-skylake - echo " cycle $i OK" +echo "=== transfer semantics ===" +if [[ -n "$CH0_ADDR" ]]; then + _word_test "$CH0" "$CH0_ADDR" && _ok "WORD_DATA byte order on ch0" || _fail "WORD_DATA test on ch0" +fi +if [[ -n "$CH1_ADDR" ]]; then + _word_test "$CH1" "$CH1_ADDR" && _ok "WORD_DATA byte order on ch1" || _fail "WORD_DATA test on ch1" + +fi +if [[ -n "$CH0" ]]; then + _nack_test "$CH0" && _ok "absent-device ENXIO on ch0" || _fail "expected ENXIO at address 0x77 on ch0" +fi +if [[ -n "$CH1" ]]; then + _nack_test "$CH1" && _ok "absent-device ENXIO on ch1" || _fail "expected ENXIO at address 0x77 on ch1" +fi + +if [[ -n "$CH0_ADDR" && -n "$CH1_ADDR" ]]; then + _stress_reader "$CH0" "$CH0_ADDR" & + pid0=$! + _stress_reader "$CH1" "$CH1_ADDR" & + pid1=$! + stress_ok=true + wait "$pid0" || stress_ok=false + wait "$pid1" || stress_ok=false + $stress_ok && _ok "concurrent 100-read stress on both channels" || _fail "concurrent read stress" +fi + +echo "=== safe default and targeted udev event ===" +PCI_DEVICE=$(_find_pci_device) || { + _fail "PCI device 8086:2085 not found" + PCI_DEVICE="" +} +if [[ -n "$PCI_DEVICE" ]]; then + "${SUDO[@]}" rmmod i2c-imc-skylake + "${SUDO[@]}" insmod "$MODULE_PATH" + _refresh_buses + if [[ -z "$CH0" && -z "$CH1" ]]; then + _ok "default-off gate rejected bind without opt-in" + else + _fail "driver bound without allow_unsafe_access=1" + fi + _module_loaded && "${SUDO[@]}" rmmod i2c-imc-skylake + + if "${SUDO[@]}" udevadm trigger --action=add "$PCI_DEVICE" && + "${SUDO[@]}" udevadm settle --timeout=5; then + _ok "targeted PCI udev event completed" + else + _fail "targeted PCI udev event failed" + fi + _module_loaded && "${SUDO[@]}" rmmod i2c-imc-skylake + _load_unsafe +fi + +echo "=== load/unload x20 ===" +for iteration in {1..20}; do + "${SUDO[@]}" rmmod i2c-imc-skylake + _load_unsafe + echo " cycle $iteration OK" done -_ok "5 load/unload cycles completed" +_ok "20 load/unload cycles completed" -echo "" -echo "=== dmesg tail ===" -dmesg | tail -20 +echo "=== kernel messages produced by this test ===" +NEW_DMESG=$("${SUDO[@]}" dmesg | tail -n "+$((DMESG_START + 1))") +RELEVANT_DMESG=$(grep -Ei 'i2c-imc-skylake [0-9a-f]{4}:|BUG:|WARNING:|Oops:|KASAN:|lockdep' <<< "$NEW_DMESG" || true) +printf '%s\n' "$RELEVANT_DMESG" +if grep -Eiq 'BUG:|WARNING:|Oops:|KASAN:|lockdep' <<< "$NEW_DMESG"; then + _fail "kernel warning/oops detected" +else + _ok "no kernel warning/oops detected" +fi + +# The driver compares the command registers before and after every transfer. +# A hit here means firmware, a BMC or CLTT is driving the same engine, which +# invalidates every other result in this run. +if grep -q 'another master is using the engine' <<< "$NEW_DMESG"; then + _fail "foreign master detected on the engine; do not use this system" +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" +else + _ok "every command was accepted by the engine" +fi -echo "" echo "=== result: $PASS passed, $FAIL failed ===" -[ "$FAIL" -eq 0 ] +((FAIL == 0)) diff --git a/tools/README.md b/tools/README.md new file mode 100644 index 0000000..d75e28a --- /dev/null +++ b/tools/README.md @@ -0,0 +1,66 @@ +# Hardware measurements still owed to the submission + +Three scripts, meant to be run by hand on an X299 (or Cascade Lake-X) system +with the PCU function `8086:2085`. None of them is run by CI and none is run by +`make test`. + +Output of all three belongs in the cover letter or in a reply on the list, as +raw numbers. Do not paraphrase them. + +| Script | Answers | Writes to hardware | +| --- | --- | --- | +| `watch-engine.sh` | Is firmware using the engine on this system? | no | +| `dump-pcu-config.sh` | Which PCU registers move on their own? Is there a TSOD polling control? | no | +| `probe-cf8-write.sh` | Is the CF8/CFC write really dropped, and does it raise an SMI? | **yes**, requires `--i-understand` | + +## 1. `watch-engine.sh` — foreign activity (read-only) + +Samples the six per-channel registers and reports every change that this +system's Linux side did not cause. Run it with the driver **unloaded**, so that +anything observed is firmware. + +```bash +sudo ./tools/watch-engine.sh --seconds 3600 +``` + +What the cover letter needs: a run of at least one hour, ideally across a +suspend/resume cycle, with the observed change count. A non-zero count on a +system means the driver must not be loaded there, and is itself a result worth +reporting. + +## 2. `dump-pcu-config.sh` — register survey (read-only) + +Dumps the function's 4 KB config space twice and diffs them, then prints the +neighbourhood of the offsets that matter. + +```bash +sudo ./tools/dump-pcu-config.sh --settle 10 +``` + +This is the search for the open item F2: the Broadwell-E iMC driver stops the +PCU's TSOD polling for the duration of a transfer by writing 0 to `TSODCNTL` +at offset `0xe0` of its PCU function, waiting 10 ms for in-flight transactions +to drain, and restoring the value afterwards. No equivalent register has been +identified on `8086:2085`. A field that changes on its own with a fixed cadence +is the first candidate. + +## 3. `probe-cf8-write.sh` — the dropped write (WRITES TO HARDWARE) + +This is the only script that writes. It changes the register/offset field of a +command register while the GO bit is clear, which does not start an SMBus +transaction, then reads it back and reports whether the write stuck. It also +reads `MSR_SMI_COUNT` (0x34) on every CPU before and after. + +```bash +sudo ./tools/probe-cf8-write.sh --i-understand +``` + +Two results matter: + +* the value does **not** read back → the claim "CF8/CFC writes to this function + do not take effect" is confirmed on the tested board; +* the SMI delta is **0** → the behaviour is not System Management Mode, and the + driver's comments and the cover letter must not say it is. + +Requirements: `msr-tools` (`rdmsr`), `pciutils` (`setpci`, `lspci`), and the +`msr` module (`modprobe msr`). diff --git a/tools/common.sh b/tools/common.sh new file mode 100755 index 0000000..fd3df26 --- /dev/null +++ b/tools/common.sh @@ -0,0 +1,90 @@ +# Shared helpers for the tools/ scripts. Sourced, not executed. +# shellcheck shell=bash + +PCU_VENDOR_DEVICE=8086:2085 + +# Per-channel register offsets in the PCU function's config space. +REG_CH0_DATA=0x9c +REG_CH1_DATA=0xa0 +REG_CH0_STAT=0xa8 +REG_CH1_STAT=0xac +REG_CH0_CTRL=0xb4 +REG_CH1_CTRL=0xb8 + +ALL_REGS=( + "ch0.DATA:$REG_CH0_DATA" + "ch1.DATA:$REG_CH1_DATA" + "ch0.STAT:$REG_CH0_STAT" + "ch1.STAT:$REG_CH1_STAT" + "ch0.CTRL:$REG_CH0_CTRL" + "ch1.CTRL:$REG_CH1_CTRL" +) + +die() +{ + echo "error: $*" >&2 + exit 1 +} + +require_root() +{ + ((EUID == 0)) || die "run as root" +} + +require_cmd() +{ + local cmd + for cmd in "$@"; do + command -v "$cmd" >/dev/null || die "missing command: $cmd" + done +} + +# Print the domain:bus:dev.fn of the PCU function, or fail. +find_pcu() +{ + local bdf + + bdf=$(lspci -Dn -d "$PCU_VENDOR_DEVICE" | awk 'NR == 1 { print $1 }') + [[ -n "$bdf" ]] || die "PCI function $PCU_VENDOR_DEVICE not found" + printf '%s\n' "$bdf" +} + +# read_reg -> 32-bit value as 0x%08x +read_reg() +{ + local bdf=$1 off=$2 raw + + raw=$(setpci -s "$bdf" "$off.L") || die "setpci read $off failed" + printf '0x%08x\n' "$((16#$raw))" +} + +# Snapshot every engine register into a "name=value" list. +snapshot_regs() +{ + local bdf=$1 entry name off + + for entry in "${ALL_REGS[@]}"; do + name=${entry%%:*} + off=${entry##*:} + printf '%s=%s\n' "$name" "$(read_reg "$bdf" "$off")" + done +} + +# Sum of MSR_SMI_COUNT (0x34) across all CPUs. Prints -1 if unavailable. +read_smi_count() +{ + local cpu total=0 value + + command -v rdmsr >/dev/null || { echo -1; return; } + for cpu in /dev/cpu/[0-9]*; do + [[ -e "$cpu/msr" ]] || continue + value=$(rdmsr -p "${cpu##*/}" -d 0x34 2>/dev/null) || continue + total=$((total + value)) + done + printf '%s\n' "$total" +} + +driver_is_loaded() +{ + lsmod | awk '$1 == "i2c_imc_skylake" { found = 1 } END { exit !found }' +} diff --git a/tools/dump-pcu-config.sh b/tools/dump-pcu-config.sh new file mode 100755 index 0000000..3c62e5c --- /dev/null +++ b/tools/dump-pcu-config.sh @@ -0,0 +1,65 @@ +#!/usr/bin/env bash +# Dump the PCU function's config space twice and report what moved on its own. +# Read-only. Used to look for a TSOD polling control register (see tools/README.md). +set -euo pipefail + +# shellcheck source=tools/common.sh +source "$(dirname "$(readlink -f "$0")")/common.sh" + +SETTLE=10 +OUTDIR=${OUTDIR:-.} + +while (($#)); do + case $1 in + --settle) SETTLE=$2; shift 2 ;; + --outdir) OUTDIR=$2; shift 2 ;; + -h|--help) + echo "usage: $0 [--settle SECONDS] [--outdir DIR]" + exit 0 + ;; + *) die "unknown argument: $1" ;; + esac +done + +require_root +require_cmd lspci awk + +BDF=$(find_pcu) +first=$(mktemp) +second=$(mktemp) +trap 'rm -f "$first" "$second"' EXIT + +echo "device: $BDF" +if driver_is_loaded; then + echo "warning: i2c-imc-skylake is loaded; unload it first for a clean survey." >&2 +fi + +lspci -s "$BDF" -xxxx > "$first" +echo "first dump taken, waiting ${SETTLE}s..." +sleep "$SETTLE" +lspci -s "$BDF" -xxxx > "$second" + +stamp=$(date +%Y%m%d-%H%M%S) +cp "$second" "$OUTDIR/pcu-config-$stamp.txt" +echo "full dump saved to $OUTDIR/pcu-config-$stamp.txt" +echo + +echo "=== bytes that changed on their own over ${SETTLE}s ===" +if diff -u "$first" "$second" | grep -E '^[+-][0-9a-f]{3}:' ; then + echo + echo "A field with a steady cadence is the first candidate for a TSOD" + echo "polling control. Compare with the Broadwell-E driver's TSODCNTL (0xe0)." +else + echo "(none: the whole config space was stable)" +fi +echo + +echo "=== engine registers ===" +snapshot_regs "$BDF" +echo + +echo "=== neighbourhood of the Broadwell TSODCNTL offset (0xe0) ===" +awk '/^0[cdef]0:/ { print }' "$second" +echo +echo "=== the driver's register window (0x90-0xbf) ===" +awk '/^0[9ab]0:/ { print }' "$second" diff --git a/tools/probe-cf8-write.sh b/tools/probe-cf8-write.sh new file mode 100755 index 0000000..f5b598d --- /dev/null +++ b/tools/probe-cf8-write.sh @@ -0,0 +1,128 @@ +#!/usr/bin/env bash +# +# WRITES TO HARDWARE. Answers two questions the submission still owes: +# +# 1. does a CF8/CFC dword write to the iMC SMBus command register take +# effect on this board? +# 2. does that write raise an SMI? +# +# It modifies only the register/offset field, DATA[7:0], while the GO bit is +# clear, so no SMBus transaction is issued, and it restores the original value +# on every exit path. It refuses to run if the engine is busy, if a transaction +# is armed, if TSOD is active, or if the driver is loaded. +# +set -euo pipefail + +# shellcheck source=tools/common.sh +source "$(dirname "$(readlink -f "$0")")/common.sh" + +GO_BIT=$((1 << 19)) +TSOD_ACTIVE_BIT=$((1 << 20)) +STAT_BUSY=1 + +CHANNEL=0 +CONFIRMED=false + +while (($#)); do + case $1 in + --i-understand) CONFIRMED=true; shift ;; + --channel) CHANNEL=$2; shift 2 ;; + -h|--help) + sed -n '3,14p' "$0" + echo "usage: $0 --i-understand [--channel 0|1]" + exit 0 + ;; + *) die "unknown argument: $1" ;; + esac +done + +if ! $CONFIRMED; then + cat >&2 <<-'EOF' + This script writes to a register of the memory controller's SMBus engine. + + It is written to be a no-op for the bus: the GO bit stays clear, so no + SMBus transaction is issued, and the original register value is restored + before the script exits. It is still a write to firmware-owned hardware. + + Re-run with --i-understand to proceed. + EOF + exit 2 +fi + +require_root +require_cmd lspci setpci awk + +case $CHANNEL in +0) REG_DATA=$REG_CH0_DATA; REG_STAT=$REG_CH0_STAT ;; +1) REG_DATA=$REG_CH1_DATA; REG_STAT=$REG_CH1_STAT ;; +*) die "channel must be 0 or 1" ;; +esac + +if ! command -v rdmsr >/dev/null; then + echo "warning: rdmsr not found, the SMI question will stay unanswered." >&2 + echo " install msr-tools and 'modprobe msr' for the full result." >&2 +fi + +driver_is_loaded && die "unload i2c-imc-skylake first" + +BDF=$(find_pcu) +echo "device: $BDF" +echo "channel: $CHANNEL (DATA at $REG_DATA, STATUS at $REG_STAT)" +echo + +original=$(read_reg "$BDF" "$REG_DATA") +status=$(read_reg "$BDF" "$REG_STAT") +echo "DATA before: $original" +echo "STATUS before: $status" + +((original & GO_BIT)) && die "GO is set: a transaction is armed, refusing" +((original & TSOD_ACTIVE_BIT)) && + die "TSOD_ACTIVE is set: firmware is using the engine, refusing" +((status & STAT_BUSY)) && die "engine is BUSY, refusing" + +restore() +{ + setpci -s "$BDF" "$REG_DATA.L=$(printf '%08x' "$original")" || true + echo "DATA restored to $(read_reg "$BDF" "$REG_DATA")" +} +trap restore EXIT + +# Flip the low byte to something that is certainly different, GO still clear. +probe=$(( (original & ~0xff) | ( ((original & 0xff) ^ 0x5a) & 0xff ) )) +probe_hex=$(printf '0x%08x' "$probe") + +smi_before=$(read_smi_count) +echo +echo "writing $probe_hex through CF8/CFC (setpci)..." +setpci -s "$BDF" "$REG_DATA.L=$(printf '%08x' "$probe")" +readback=$(read_reg "$BDF" "$REG_DATA") +smi_after=$(read_smi_count) + +echo "DATA after write: $readback" +echo + +echo "=== result 1: did the write take effect? ===" +if [[ "$readback" == "$probe_hex" ]]; then + echo "STUCK - the CF8/CFC write reached the register." + echo " The premise of the ECAM patch does not hold on this board;" + echo " do not claim CF8/CFC writes are dropped." +else + echo "DROPPED - wrote $probe_hex, read back $readback." + echo " Confirms that CF8/CFC writes to this function do not" + echo " take effect on this board." +fi +echo + +echo "=== result 2: was an SMI raised? ===" +if ((smi_before < 0 || smi_after < 0)); then + echo "UNKNOWN - MSR_SMI_COUNT not readable (need msr-tools + modprobe msr)." +else + delta=$((smi_after - smi_before)) + echo "MSR_SMI_COUNT: $smi_before -> $smi_after (delta $delta)" + if ((delta == 0)); then + echo "NO SMI - the behaviour must not be attributed to SMM." + else + echo "SMI RAISED - System Management Mode is involved; say so, with" + echo " this number, in the changelog." + fi +fi diff --git a/tools/watch-engine.sh b/tools/watch-engine.sh new file mode 100755 index 0000000..ac586cc --- /dev/null +++ b/tools/watch-engine.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +# Report changes to the iMC SMBus engine registers that Linux did not cause. +# Read-only. Run with the driver unloaded so anything observed is firmware. +set -euo pipefail + +# shellcheck source=tools/common.sh +source "$(dirname "$(readlink -f "$0")")/common.sh" + +SECONDS_TOTAL=3600 +INTERVAL=1 + +while (($#)); do + case $1 in + --seconds) SECONDS_TOTAL=$2; shift 2 ;; + --interval) INTERVAL=$2; shift 2 ;; + -h|--help) + echo "usage: $0 [--seconds N] [--interval N]" + exit 0 + ;; + *) die "unknown argument: $1" ;; + esac +done + +require_root +require_cmd lspci setpci awk + +BDF=$(find_pcu) + +if driver_is_loaded; then + echo "warning: i2c-imc-skylake is loaded; changes may be caused by Linux." >&2 + echo " unload it for a clean firmware-activity measurement." >&2 +fi + +echo "device: $BDF" +echo "duration: ${SECONDS_TOTAL}s, sampling every ${INTERVAL}s" +echo "started: $(date -Is)" +echo + +mapfile -t previous < <(snapshot_regs "$BDF") +printf 'baseline: %s\n' "${previous[*]}" + +changes=0 +samples=0 +deadline=$((SECONDS + SECONDS_TOTAL)) + +while ((SECONDS < deadline)); do + sleep "$INTERVAL" + mapfile -t current < <(snapshot_regs "$BDF") + ((samples++)) + + for i in "${!current[@]}"; do + if [[ "${current[i]}" != "${previous[i]}" ]]; then + printf '%s CHANGE %s -> %s\n' \ + "$(date -Is)" "${previous[i]}" "${current[i]}" + ((changes++)) + fi + done + previous=("${current[@]}") +done + +echo +echo "finished: $(date -Is)" +echo "samples: $samples" +echo "changes: $changes" +echo +if ((changes == 0)); then + echo "RESULT: no foreign activity observed on this system over ${SECONDS_TOTAL}s." +else + echo "RESULT: the engine is in use by something other than Linux." + echo " Do not load i2c-imc-skylake on this system." +fi