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 <[email protected]> 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/[email protected]?part=9
