Re: [PATCH v4 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] Skipped unplug test causes a cascading spurious failure in the subsequent `online_kernel` test.
- [Medium] The test script modifies system-wide DAX device hotplug state but lacks a `trap` to ensure restoration on unexpected termination.
--

commit eaf4726251fa25a05d2dfac099a7956bef58a48a
Author: Gregory Price <gourry@gourry.net>

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

This commit adds a kselftest to validate the whole-device hotplug sysfs
attribute for dax/kmem devices. It verifies transitions between unplugged,
online, and online_movable states, ensuring memory is properly added and
removed.

> 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..e43cda8ba1274
> --- /dev/null
> +++ b/tools/testing/selftests/dax/dax-kmem-hotplug.sh

[ ... ]

> +DAX=$(find_kmem_dax)
> +if [ -z "$DAX" ]; then
> +	ktap_skip_all "no kmem-bound dax device with a hotplug attribute"
> +	exit "$KSFT_SKIP"
> +fi
> +HP=$DAX_BASE/$DAX/hotplug
> +ORIG=$(get_state)

[Severity: Medium]
Does this script ensure the original state is restored if execution is
aborted early?

Since the original state is recorded here but no shell trap is established,
an unexpected exit or user interrupt could bypass the manual restoration
at the end of the script. Would adding an exit trap handler right after
recording the state ensure reliable cleanup?

[ ... ]

> +# The online -> unplug -> online_movable -> unplug cycle once regressed: a
> +# re-online failed to re-reserve the per-range resources, so this final unplug
> +# reported success while leaving the memory online.  Assert it is really freed.
> +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_mov" ]; then
> +	ktap_test_pass "unplug from online_movable removed memory: $mt_mov -> $mt kB"
> +else
> +	ktap_test_fail "unplug success but memory remained: $(get_state) $mt_mov -> $mt kB"
> +fi
> +
> +set_state online_kernel; rc=$?

[Severity: Medium]
If the preceding unplug transition fails and execution enters the skip
branch, will this subsequent state transition fail spuriously?

Because the driver rejects direct transitions between different online states
without an intervening unplug, falling through to the online_kernel test
while the device is still in the online_movable state seems like it will
unconditionally fail instead of safely skipping the rest of the checks.

> +mt=$(memtotal_kb)
> +if [ "$rc" = 0 ] && [ "$(get_state)" = online_kernel ] && [ "$mt" -gt "$mt_unplugged" ]; then
> +	ktap_test_pass "online_kernel: MemTotal $mt_unplugged -> $mt kB"
> +else
> +	ktap_test_fail "online_kernel: rc=$rc state=$(get_state) MemTotal=$mt"
> +fi
> +set_state unplugged

[ ... ]

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