On Wed, Aug 13, 2025 at 06:02:39PM -0700, Marc Herbert wrote:
> Reviewing only the shell language part of this, not the CXL logic.
Thanks for the review. I applied most all of your suggestions in v3.
See below...
>
> 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).
Done.
>
>
> > - 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
>
I've switched to a bool comparison in v3.
>
> > + 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.
No reason. I removed the direct.
>
> 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' "$@"
> }
Dedup'd that in v3.
>
>
>
> > - 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)
>
Added local's to above params. Shellcheck has been happy. No new
complaints.
> 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 )
I'm going to stick with the readability (and maintainability) of this:
trace_line=$(grep "cxl_poison" /sys/kernel/tracing/trace | tail -n 1)
over this:
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.
>
Point taken. The grep|cut version is working well and is quite readable,
so I'll leave it as-is.
>
> > -# 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.
Done.
>
