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