Re: [PATCH v5 9/9] selftests/dax: add dax/kmem hotplug sysfs regression test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Cascading test failures due to unhandled baseline restoration failure.
- [Medium] Device state restoration is clobbered before the destructive test, leaving the device altered if the test is skipped.
--

commit 86db030de2c10bd6941657c29afc9ffc162f74b5
Author: Gregory Price <gourry@gourry.net>

selftests/dax: add dax/kmem hotplug sysfs regression test

This commit adds a kselftest for the dax/kmem whole-device "state" sysfs
attribute to validate transitions between unplugged, online, and
online_movable.

> diff --git a/tools/testing/selftests/dax/dax-kmem-hotplug.sh b/tools/testing/selftests/dax/dax-kmem-hotplug.sh
> new file mode 100755
> index 0000000000000..803bbd5a64098
> --- /dev/null
> +++ b/tools/testing/selftests/dax/dax-kmem-hotplug.sh

[ ... ]

> +set_state unplugged; rc=$?
> +mt=$(memtotal_kb)
> +if [ "$rc" != 0 ]; then
> +	ktap_test_skip "unplug from movable not accepted (memory in use?) rc=$rc"
> +elif [ "$(get_state)" = unplugged ] && [ "$mt" -lt "$mt_movable" ]; then
> +	ktap_test_pass "unplug from online_movable removed memory: $mt_movable -> $mt kB"
> +else
> +	ktap_test_fail "unplug from movable reported success but memory remained: state=$(get_state) MemTotal $mt_movable -> $mt"
> +fi
> +
> +set_state online_kernel; rc=$?

[Severity: Medium]
If the unplug command fails and we emit the ktap_test_skip message, the script
continues execution to the online_kernel test.

Since that test expects an unplugged baseline, does this lead to cascading
spurious test failures if the memory couldn't be offlined?

[ ... ]

> +[ -n "$ORIG" ] && set_state "$ORIG"
> +
> +# DESTRUCTIVE: unbinding the driver while memory is online causes the resources
> +# to leak - but the unbind should not deadlock.  Instead the driver leaks it
> +# with a single "suck online" warning. This leaves the memory online and the
> +# device unbound until reboot, so it runs last.
> +set_state unplugged; set_state online
> +if [ "$(get_state)" = online ] && [ -w "$DRV/unbind" ]; then

[Severity: Medium]
We restore the original state of the device here, but then unconditionally
overwrite it by running "set_state unplugged; set_state online" before checking
if the destructive test can actually run.

If $DRV/unbind is not writable, the test is skipped and the script exits.

Does this leave the device permanently altered (left "online") instead of in
its original state?

> +	mt_on=$(memtotal_kb)
> +	dmesg -C 2>/dev/null
> +	echo "$DAX" > "$DRV/unbind" 2>/dev/null
> +	mt_after=$(memtotal_kb)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624145744.3532049-1-gourry@gourry.net?part=9