Reviewing only the shell language part of this, not the CXL logic.
On 2025-08-04 01:14, alison.schofi...@intel.com wrote:
>
> inject_poison_sysfs()
> {
> - memdev="$1"
> + dev="$1"
> addr="$2"
> + expect_fail="$3"
You can make expect_fail and maybe others "local" instead of global
(the default).
> - echo "$addr" > /sys/kernel/debug/cxl/"$memdev"/inject_poison
> + if [[ "$expect_fail" == "true" ]]; then
It looks like this script has full control over $expect_fail, never
affected by any outside input. So you can trust it and simplify this to:
local expect_fail=${3-:false}
...
if "$expect_fail"; then
> + if echo "$addr" > /sys/kernel/debug/cxl/"$dev"/inject_poison
> 2>/dev/null; then
Is it expected that this particular /sys may not exist in some test
conditions? If not, then there's no reason to discard stderr.
stderr is generally just for "totally unexpected" issues and should
almost never discarded. Especially not in test code where you really
want to get all the information possible when something totally
unexpected happens. Even more so when this happens in some distant CI
system few people have direct access to for reproduction.
In the extremely rare cases where stderr should be discarded, there
needs to be comment with a convincing rationale for it.
> + echo "Expected inject_poison to fail for $addr"
> + err "$LINENO"
> + fi
> + else
> + echo "$addr" > /sys/kernel/debug/cxl/"$dev"/inject_poison
> + fi
> }
>
> clear_poison_sysfs()
> {
Same as above. In fact there seems to be only word difference between
these two functions, which begs for something like this:
inject_poison_sysfs()
{
_do_poison_sysfs 'inject' "$@"
}
clear_poison_sysfs()
{
_do_poison_sysfs 'clear' "$@"
}
> - memdev="$1"
> + dev="$1"
> addr="$2"
> + expect_fail="$3"
>
> - echo "$addr" > /sys/kernel/debug/cxl/"$memdev"/clear_poison
> + if [[ "$expect_fail" == "true" ]]; then
> + if echo "$addr" > /sys/kernel/debug/cxl/"$dev"/clear_poison
> 2>/dev/null; then
> + echo "Expected clear_poison to fail for $addr"
> + err "$LINENO"
> + fi
> + else
> + echo "$addr" > /sys/kernel/debug/cxl/"$dev"/clear_poison
> + fi
> +}
> +
> +check_trace_entry()
> +{
> + expected_region="$1"
> + expected_hpa="$2"
> + trace_line=$(grep "cxl_poison" /sys/kernel/tracing/trace | tail -n 1)
Probably "local" (but don't forget SC2155)
Nit: you can save one process and one pipe with awk:
local trace_line; trace_line=$( awk '/cxl_poison' { L=$0 } END { print L }'
/sys/kernel/tracing/trace )
> + if [[ -z "$trace_line" ]]; then
> + echo "No cxl_poison trace event found"
> + err "$LINENO"
> + fi
> +
> + trace_region=$(echo "$trace_line" | grep -o 'region=[^ ]*' | cut -d=
> -f2)
I think sed is more typical for this sort of stuff but whatever works.
> -# Turn tracing on. Note that 'cxl list --media-errors' toggles the tracing.
> -# Turning it on here allows the test user to also view inject and clear
> -# trace events.
> +test_poison_by_region_offset()
> +{
> + base=$(cat /sys/bus/cxl/devices/"$region"/resource)
> + gran=$(cat /sys/bus/cxl/devices/"$region"/interleave_granularity)
local if that makes sense.
