Re: [NDCTL PATCH v8 4/4] cxl/test: Add test for cxl features device

[Marc Herbert <Marc.Herbert@linux.intel.com>]

[Sorry for commenting for the first time on v8]

Could you move all the cxl-features.sh code to a
main() function and then call main "$@" at the end?

It's a pretty standard practice and has many benefits listed here:
https://github.com/thesofproject/sof-test/issues/740

More inline

On 2025-05-22 08:56, Dave Jiang wrote:

> diff --git a/test/cxl-features.sh b/test/cxl-features.sh
> new file mode 100755
> index 000000000000..a730310ec7b0
> --- /dev/null
> +++ b/test/cxl-features.sh
> @@ -0,0 +1,31 @@
> +#!/bin/bash -Ex
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2025 Intel Corporation. All rights reserved.
> +
> +rc=77

Once this is a function:

 local rc=77


> +# 237 is -ENODEV
> +ERR_NODEV=237
> +
> +. $(dirname $0)/common
> +FEATURES="$TEST_PATH"/fwctl
> +
> +trap 'err $LINENO' ERR
> +
> +modprobe cxl_test
> +
> +test -x "$FEATURES" || do_skip "no CXL Features Control"
> +# disable trap
> +trap - $(compgen -A signal)

ALL of them? Is that really necessary? Or just ERR maybe?
A comment maybe?
traps are usually the reserved domain of test/common.

If this just for ERR, then you shouldn't need to change
traps at all:

rc=0
"$FEATURES" || rc=$?

|| and friends turn off "set -e" and "trap ERR" temporarily
(otherwise they would be incompatible with each other)
    

> +"$FEATURES"
> +rc=$?
> +
> +echo "error: $rc"
> +if [ "$rc" -eq "$ERR_NODEV" ]; then
> +	do_skip "no CXL FWCTL char dev"
> +elif [ "$rc" -ne 0 ]; then
> +	echo "fail: $LINENO" && exit 1
> +fi
> +
> +trap 'err $LINENO' ERR

In a distant future this should live in test/common...

> +
> +_cxl_cleanup