[ndctl PATCH v2] test/cxl-poison.sh: test inject and clear poison by region offset

[ndctl PATCH v2] test/cxl-poison.sh: test inject and clear poison by region offset

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

The CXL kernel driver recently added support to inject and clear
poison in a region by specifying an offset. Add a test case to the
existing cxl-poison unit test that demonstrates how to use the new
debugfs attributes. Use the kernel trace log to validate the round
trip address translations.

SKIP, do not fail, if the new debugfs attributes are not present.

See the kernel ABI documentation for usage:
Documentation/ABI/testing/debugfs-cxl

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---

Changes in v2:
Add test_poison_by_region_offset_negative set of test cases


 test/cxl-poison.sh | 129 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 117 insertions(+), 12 deletions(-)

diff --git a/test/cxl-poison.sh b/test/cxl-poison.sh
index 6ed890bc666c..517e3db23223 100644
--- a/test/cxl-poison.sh
+++ b/test/cxl-poison.sh
@@ -65,18 +65,61 @@ create_x2_region()
 
 inject_poison_sysfs()
 {
-	memdev="$1"
+	dev="$1"
 	addr="$2"
+	expect_fail="$3"
 
-	echo "$addr" > /sys/kernel/debug/cxl/"$memdev"/inject_poison
+	if [[ "$expect_fail" == "true" ]]; then
+		if echo "$addr" > /sys/kernel/debug/cxl/"$dev"/inject_poison 2>/dev/null; then
+			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()
 {
-	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)
+	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)
+	trace_hpa=$(echo "$trace_line" | grep -o 'hpa=0x[0-9a-fA-F]\+' | cut -d= -f2)
+
+	if [[ "$trace_region" != "$expected_region" ]]; then
+		echo "Expected region $expected_region not found in trace"
+		echo "$trace_line"
+		err "$LINENO"
+	fi
+
+	if [[ "$trace_hpa" != "$expected_hpa" ]]; then
+		echo "Expected HPA $expected_hpa not found in trace"
+		echo "$trace_line"
+		err "$LINENO"
+	fi
 }
 
 validate_poison_found()
@@ -97,7 +140,7 @@ validate_poison_found()
 	fi
 }
 
-test_poison_by_memdev()
+test_poison_by_memdev_by_dpa()
 {
 	find_memdev
 	inject_poison_sysfs "$memdev" "0x40000000"
@@ -113,9 +156,8 @@ test_poison_by_memdev()
 	validate_poison_found "-m $memdev" 0
 }
 
-test_poison_by_region()
+test_poison_by_region_by_dpa()
 {
-	create_x2_region
 	inject_poison_sysfs "$mem0" "0x40000000"
 	inject_poison_sysfs "$mem1" "0x40000000"
 	validate_poison_found "-r $region" 2
@@ -125,13 +167,76 @@ test_poison_by_region()
 	validate_poison_found "-r $region" 0
 }
 
-# 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)
+
+	# Test two HPA addresses: base and base + granularity
+	# This hits the two memdevs in the region interleave.
+	hpa1=$(printf "0x%x" $((base)))
+	hpa2=$(printf "0x%x" $((base + gran)))
+
+	# Inject at the offset and check result using the hpa's 
+	# ABI takes an offset, but recall the hpa to check trace event
+
+	inject_poison_sysfs "$region" 0
+	check_trace_entry "$region" "$hpa1"
+	inject_poison_sysfs "$region" "$gran"
+	check_trace_entry "$region" "$hpa2"
+	validate_poison_found "-r $region" 2
+
+	clear_poison_sysfs "$region" 0
+	check_trace_entry "$region" "$hpa1"
+	clear_poison_sysfs "$region" "$gran"
+	check_trace_entry "$region" "$hpa2"
+	validate_poison_found "-r $region" 0
+}
+
+test_poison_by_region_offset_negative()
+{
+	region_size=$(cat /sys/bus/cxl/devices/"$region"/size)
+	cache_size=0
+
+	# This case is a no-op until cxl-test ELC mocking arrives
+	# Try to get cache_size if the attribute exists
+	if [ -f "/sys/bus/cxl/devices/$region/cache_size" ]; then
+		cache_size=$(cat /sys/bus/cxl/devices/"$region"/cache_size)
+	fi
+
+	# Offset within extended linear cache (if cache_size > 0)
+	if [[ $cache_size -gt 0 ]]; then
+		cache_offset=$((cache_size - 1))
+		echo "Testing offset within cache: $cache_offset (cache_size: $cache_size)"
+		inject_poison_sysfs "$region" "$cache_offset" "true"
+		clear_poison_sysfs "$region" "$cache_offset" "true"
+	else
+		echo "Skipping cache test - cache_size is 0"
+	fi
+
+	# Offset exceeds region size
+	exceed_offset=$((region_size))
+	inject_poison_sysfs "$region" "$exceed_offset" "true"
+	clear_poison_sysfs "$region" "$exceed_offset" "true"
+
+	# Offset exceeds region size by a lot
+	large_offset=$((region_size * 2))
+	inject_poison_sysfs "$region" "$large_offset" "true"
+	clear_poison_sysfs "$region" "$large_offset" "true"
+}
+
+# Clear old trace events, enable cxl_poison, enable global tracing
+echo "" > /sys/kernel/tracing/trace
 echo 1 > /sys/kernel/tracing/events/cxl/cxl_poison/enable
+echo 1 > /sys/kernel/tracing/tracing_on
 
-test_poison_by_memdev
-test_poison_by_region
+test_poison_by_memdev_by_dpa
+create_x2_region
+test_poison_by_region_by_dpa
+[ -f "/sys/kernel/debug/cxl/$region/inject_poison" ] ||
+       do_skip "test cases requires inject by region kernel support"
+test_poison_by_region_offset
+test_poison_by_region_offset_negative
 
 check_dmesg "$LINENO"
 
-- 
2.37.3

Re: [ndctl PATCH v2] test/cxl-poison.sh: test inject and clear poison by region offset

[Marc Herbert]

Reviewing only the shell language part of this, not the CXL logic.

On 2025-08-04 01:14, alison.schofield@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.

Re: [ndctl PATCH v2] test/cxl-poison.sh: test inject and clear poison by region offset

[Alison Schofield]

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.schofield@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.


>