From c9b8799f69444855ae8726b36c0c9fd883e59a20 Mon Sep 17 00:00:00 2001 From: Simone Date: Mon, 27 Jul 2026 00:06:11 +0200 Subject: [PATCH] i2c-imc-skylake: suspend/resume, and correct the ee1004 claim Two things came out of preparing the suspend test, and one of them is a correction. ee1004 does not bind to this adapter. Both functionality combinations it accepts include I2C_FUNC_SMBUS_BYTE (drivers/misc/eeprom/ee1004.c:301), which this driver does not implement, so the probe fails with -EPFNOSUPPORT. The cover letter claimed BYTE_DATA and WORD_DATA "cover ee1004 and jc42" and the .rst told the reader to instantiate it; both were wrong and are corrected. jc42 is fine, it only needs BYTE_DATA and WORD_DATA, but no TSOD is populated here to bind to. The practical cost is that decode-dimms does not work through this adapter. SPD bytes stay readable byte by byte with i2cget. Receive Byte is what PawnIO implements with its pointer-select bit and what this project chose not to advertise for want of validation; that choice was not free, and adding it validated is the most useful thing left. Suspend and resume tested with continuous traffic on both adapters, s2idle and S3. Transfers recover, nothing complains. Across S3 the engine comes back reprogrammed on both channels: DATA 0x2000b600 -> 0x20005000 STATUS 0x0000500c -> 0x00005004 The command word addresses SPD 0x50 for a read and the status drops to READ_DONE alone. Linux issued none of it: the adapter was marked suspended and userspace frozen. This is consistent with firmware reading SPD during memory training on resume, and it is the first direct observation of a second master on this engine. It is also the concrete justification for restoring command state per transfer and comparing it afterwards. Add tools/test-suspend.sh, which drives that test and reports the register delta across the sleep. Traffic goes through i2c-dev rather than a bound client: with ee1004 unable to attach and no TSOD present, there is no in-kernel client to use, and the PM path exercised is the same either way -- i2c_smbus_xfer() consults __i2c_check_suspended() before reaching the adapter. --- docs/submission/cover-letter.txt | 32 +++++- docs/submission/i2c-imc-skylake.rst | 23 ++-- tools/test-suspend.sh | 164 ++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+), 11 deletions(-) create mode 100755 tools/test-suspend.sh diff --git a/docs/submission/cover-letter.txt b/docs/submission/cover-letter.txt index c3e46a7..3391b70 100644 --- a/docs/submission/cover-letter.txt +++ b/docs/submission/cover-letter.txt @@ -106,7 +106,14 @@ implementation for the same engine, which predates this work: 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. +i2cdetect cannot scan these buses and devices are addressed directly. + +That omission costs more than the scan. Both functionality combinations +ee1004 accepts include I2C_FUNC_SMBUS_BYTE, so ee1004 does not bind to this +adapter and decode-dimms does not work through it; SPD bytes are readable +with i2cget, and jc42 binds fine where a sensor is populated. Implementing +Receive Byte is the most useful thing still missing, and I would rather add +it in a follow-up, validated, than advertise it now. Changes since v2 ~~~~~~~~~~~~~~~~ @@ -131,6 +138,8 @@ Changes since v2 - Add Documentation/i2c/busses/i2c-imc-skylake.rst. - Drop the ACPI dependency. - Refuse to bind when the iMC reports TSOD polling enabled. + - Correct the claim that BYTE_DATA and WORD_DATA cover ee1004: they do + not, ee1004 also needs Receive Byte. Validation ~~~~~~~~~~ @@ -145,8 +154,23 @@ Validation checks, 0 failures, no kernel warning or oops, no interference reported, every command word confirmed in the register. - Engine registers sampled every 2 s for one hour with the driver - unloaded: 1791 samples, 0 changes. No firmware activity was observed - on this system. + unloaded: 1791 samples, 0 changes. No firmware activity while the + system is up. + - Suspend and resume, s2idle and S3, with continuous traffic on both + adapters: transfers recover and nothing complains. Across S3 the + firmware demonstrably drives the engine. Both channels, sampled + immediately before suspend and immediately after resume: + + DATA 0x2000b600 -> 0x20005000 + STATUS 0x0000500c -> 0x00005004 + + The command word came back addressing SPD 0x50 for a read, and the + status went from both done bits latched to READ_DONE alone. Nothing in + Linux issued that: the adapter was marked suspended and userspace was + frozen. This is consistent with firmware reading SPD during memory + training on resume. It is the only direct observation of a second + master on this engine I have, and it is the concrete reason the driver + restores command state per transfer and compares it afterwards. - SMBCNTL on all four iMC channels reads 0xac000000: TSOD polling disabled, TSOD_PRESENT empty. - Writes validated end to end against a populated device: a 12-write @@ -158,7 +182,7 @@ Validation after. Still missing: an independent Tested-by, ideally on Skylake-X rather than -Cascade Lake-X, and suspend/resume testing with active I2C clients. +Cascade Lake-X, and SMBus Receive Byte so that ee1004 binds. One note on the register interface, since a reader with the datasheet will ask. That document describes an SMBus interface on the iMC channel diff --git a/docs/submission/i2c-imc-skylake.rst b/docs/submission/i2c-imc-skylake.rst index e70fcef..ccc7027 100644 --- a/docs/submission/i2c-imc-skylake.rst +++ b/docs/submission/i2c-imc-skylake.rst @@ -91,7 +91,13 @@ Supported transactions ---------------------- The driver implements SMBus Read/Write Byte Data and Read/Write Word Data. -That covers ee1004 (DDR4 SPD) and jc42 (TSOD). + +That is enough for ``jc42`` (TSOD), which asks for exactly those two. It is +**not** enough for ``ee1004``: both of its accepted combinations include +``I2C_FUNC_SMBUS_BYTE``, which this adapter does not implement, so the driver +probe fails with ``-EPFNOSUPPORT``. SPD bytes are still readable directly with +``i2cget``. Implementing SMBus Receive Byte would make ``ee1004`` and +``decode-dimms`` work; it is the most useful thing still missing. 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 @@ -110,15 +116,18 @@ Word transfers use the standard SMBus byte order, so ``jc42`` reading through 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:: +The adapter does not create clients by itself. There is no firmware +description of these buses and the driver does not scan. - # echo ee1004 0x50 > /sys/bus/i2c/devices/i2c-6/new_device - # echo jc42 0x18 > /sys/bus/i2c/devices/i2c-6/new_device +``jc42`` can be instantiated where a thermal sensor is populated:: -Check first that the address is not already bound to a driver. + # echo jc42 0x18 > /sys/bus/i2c/devices/i2c-6/new_device +Whether one is populated is visible in ``SMBCNTL.TSOD_PRESENT``; on the +development system that mask is empty and no ``jc42`` binds. + +``ee1004`` will not bind for the reason given above. Read SPD bytes directly +instead. Module parameters ----------------- diff --git a/tools/test-suspend.sh b/tools/test-suspend.sh new file mode 100755 index 0000000..3337c48 --- /dev/null +++ b/tools/test-suspend.sh @@ -0,0 +1,164 @@ +#!/usr/bin/env bash +# +# Suspend/resume validation with active I2C clients. +# +# Hammers both adapters from userspace and suspends the machine underneath the +# traffic. The driver hands suspend and resume to the i2c core +# (i2c_mark_adapter_suspended / _resumed), so the core must reject transfers +# with -ESHUTDOWN for the duration and accept them again afterwards. What this +# checks: +# +# Traffic goes through i2c-dev rather than a bound client driver: ee1004 needs +# I2C_FUNC_SMBUS_BYTE, which this adapter does not implement, and no TSOD is +# populated for jc42 to bind to. The PM path being exercised is the same one +# either way - i2c_smbus_xfer() calls __i2c_check_suspended() before reaching +# the adapter. +# +# - transfers fail while the adapter is suspended rather than touching the +# engine or hanging; +# - transfers succeed again after resume, with correct data; +# - nothing in the kernel log complains. +# +# The machine really does suspend. Wake-up is scheduled through the RTC alarm, +# so no key press is needed, but if the platform fails to wake you will have to +# press the power button. +# +set -euo pipefail + +# shellcheck source=tools/common.sh +source "$(dirname "$(readlink -f "$0")")/common.sh" + +MODE=freeze +ASLEEP=20 +SPD_ADDR=0x50 +CONFIRMED=false +READER="" + +while (($#)); do + case $1 in + --i-understand) CONFIRMED=true; shift ;; + --mode) MODE=$2; shift 2 ;; + --seconds) ASLEEP=$2; shift 2 ;; + -h|--help) + sed -n '3,19p' "$0" + echo "usage: $0 --i-understand [--mode freeze|mem] [--seconds N]" + exit 0 + ;; + *) die "unknown argument: $1" ;; + esac +done + +$CONFIRMED || { + echo "This suspends the machine. Re-run with --i-understand." >&2 + exit 2 +} + +require_root +require_cmd i2cdetect i2cget rtcwake awk + +bus_of() +{ + i2cdetect -l | awk -v ch="$1" ' + tolower($0) ~ /imc.*channel/ && $0 ~ "channel " ch { + sub(/^i2c-/, "", $1); print $1; exit + }' +} + +cleanup() +{ + [[ -n "$READER" ]] && kill "$READER" 2>/dev/null || true +} +trap cleanup EXIT + +BUSES=() +for ch in 0 1; do + b=$(bus_of "$ch") + [[ -n "$b" ]] || die "adapter for channel $ch not found" + BUSES+=("$b") +done +echo "adapters: ${BUSES[*]}" + +for b in "${BUSES[@]}"; do + i2cget -y "$b" "$SPD_ADDR" 0x02 >/dev/null 2>&1 || + die "bus $b: cannot read the SPD at $SPD_ADDR, nothing to hammer" + echo " bus $b: SPD $SPD_ADDR responds" +done + +BDF=$(find_pcu) +mapfile -t REGS_BEFORE < <(snapshot_regs "$BDF") + +LOG=$(mktemp) +DMESG_START=$(dmesg | wc -l) + +# Hammer the clients and record the outcome of every read with a timestamp. +( + while :; do + for b in "${BUSES[@]}"; do + if i2cget -y "$b" "$SPD_ADDR" 0x02 >/dev/null 2>&1; then + printf '%s OK\n' "$(date +%s.%N)" + else + printf '%s FAIL\n' "$(date +%s.%N)" + fi + done + sleep 0.1 + done +) >> "$LOG" 2>&1 & +READER=$! + +sleep 2 +BEFORE=$(date +%s) +echo "suspending: mode=$MODE for ${ASLEEP}s" +rtcwake -m "$MODE" -s "$ASLEEP" >/dev/null 2>&1 || die "rtcwake failed" +AFTER=$(date +%s) +sleep 2 + +kill "$READER" 2>/dev/null || true +READER="" +sleep 0.5 + +echo +echo "=== result ===" +printf 'wall clock across suspend: %ss (asked for %ss)\n' "$((AFTER - BEFORE))" "$ASLEEP" + +pre=$(awk -v t="$BEFORE" '$1 < t && $2 == "OK"' "$LOG" | wc -l) +post=$(awk -v t="$AFTER" '$1 > t && $2 == "OK"' "$LOG" | wc -l) +fails=$(awk '$2 == "FAIL"' "$LOG" | wc -l) +fails_outside=$(awk -v a="$BEFORE" -v b="$AFTER" '$2 == "FAIL" && ($1 < a || $1 > b)' "$LOG" | wc -l) + +printf 'reads OK before suspend: %s\n' "$pre" +printf 'reads OK after resume: %s\n' "$post" +printf 'failed reads total: %s\n' "$fails" +printf 'failed reads outside the suspend window: %s\n' "$fails_outside" + +echo +echo "engine registers across the suspend:" +mapfile -t REGS_AFTER < <(snapshot_regs "$BDF") +moved=0 +for i in "${!REGS_AFTER[@]}"; do + if [[ "${REGS_AFTER[i]}" == "${REGS_BEFORE[i]}" ]]; then + printf ' %-28s unchanged\n' "${REGS_BEFORE[i]}" + else + printf ' %s -> %s CHANGED\n' "${REGS_BEFORE[i]}" "${REGS_AFTER[i]}" + moved=$((moved + 1)) + fi +done +if ((moved)); then + echo " note: something reprogrammed the engine across the suspend." + echo " On resume the driver restores command state per transfer, but a" + echo " change here means firmware touched it while the OS was down." +fi + +echo +NEW=$(dmesg | tail -n "+$((DMESG_START + 1))") +grep -Ei 'i2c-imc-skylake|BUG:|WARNING:|Oops:|KASAN:|lockdep' <<< "$NEW" || echo "(no driver or kernel complaints)" + +echo +rc=0 +((pre > 0)) || { echo "FAIL: no successful read before suspend"; rc=1; } +((post > 0)) || { echo "FAIL: reads did not recover after resume"; rc=1; } +((fails_outside == 0)) || { echo "FAIL: reads failed outside the suspend window"; rc=1; } +grep -Eiq 'BUG:|WARNING:|Oops:|KASAN:|lockdep' <<< "$NEW" && { echo "FAIL: kernel complaint"; rc=1; } +((rc == 0)) && echo "PASS: transfers resumed correctly and nothing complained" + +rm -f "$LOG" +exit $rc