Re: [NDCTL PATCH v5 3/3] cxl/test: Add test for cxl features device

[Dan Williams <dan.j.williams@intel.com>]

Dave Jiang wrote:
> Add a unit test to verify the features ioctl commands. Test support added
> for locating a features device, retrieve and verify the supported features
> commands, retrieve specific feature command data, retrieve test feature
> data, and write and verify test feature data.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v5:
> - Make command prep common. (Alison)
> - Rename fwctl.c to cxl-features-control.c.  (Alison)
> - Update test code to retrieve cxl_fwctl object.
> - Create helper for aligned allocation and zeroing. (Alison)
> - Correct check of open() return value. (Alison)
> ---
>  test/cxl-features-control.c | 439 ++++++++++++++++++++++++++++++++++++
>  test/cxl-features.sh        |  31 +++
>  test/cxl-topology.sh        |   4 +
>  test/meson.build            |  45 ++++
>  4 files changed, 519 insertions(+)
>  create mode 100644 test/cxl-features-control.c
>  create mode 100755 test/cxl-features.sh
> 
> diff --git a/test/cxl-features-control.c b/test/cxl-features-control.c

Not that this filename matters, but if someone was looking for FWCTL
examples with CXL filename does not shout "look here for
fwctl examples".

[..]
> diff --git a/test/cxl-features.sh b/test/cxl-features.sh
> new file mode 100755
> index 000000000000..e648242a4a01
> --- /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
> +# 237 is -ENODEV
> +ERR_NODEV=237
> +
> +. $(dirname $0)/common
> +FEATURES="$TEST_PATH"/cxl-features-control
> +
> +trap 'err $LINENO' ERR
> +
> +modprobe cxl_test
> +
> +test -x "$FEATURES" || do_skip "no CXL Features Contrl"
> +# disable trap
> +trap - $(compgen -A signal)
> +"$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
> +
> +_cxl_cleanup
> diff --git a/test/cxl-topology.sh b/test/cxl-topology.sh
> index e8b9f56543b5..90b9c98273db 100644
> --- a/test/cxl-topology.sh
> +++ b/test/cxl-topology.sh
> @@ -172,11 +172,15 @@ done
>  # validate host bridge tear down for the first 2 bridges
>  for b in ${bridge[0]} ${bridge[1]}
>  do
> +	echo "XXX SHELL disable port $b to validate teardown" > /dev/kmsg
> +
>  	$CXL disable-port $b -f
>  	json=$($CXL list -M -i -p $b)
>  	count=$(jq "map(select(.state == \"disabled\")) | length" <<< $json)
>  	((count == 4)) || err "$LINENO"
>  
> +	echo "XXX SHELL enable port $b to validate teardown" > /dev/kmsg
> +

Are these unrelated changes? I do not see what new cxl-topology.sh print
statements has to do with this new fwctl test?

>  	$CXL enable-port $b -m
>  	json=$($CXL list -M -p $b)
>  	count=$(jq "length" <<< $json)
> diff --git a/test/meson.build b/test/meson.build
> index d871e28e17ce..89e73c2575f6 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -17,6 +17,13 @@ ndctl_deps = libndctl_deps + [
>    versiondep,
>  ]
>  
> +libcxl_deps = [
> +  cxl_dep,
> +  ndctl_dep,
> +  uuid,
> +  kmod,
> +]
> +
>  libndctl = executable('libndctl', testcore + [ 'libndctl.c'],
>    dependencies : libndctl_deps,
>    include_directories : root_inc,
> @@ -130,6 +137,33 @@ revoke_devmem = executable('revoke_devmem', testcore + [
>    include_directories : root_inc,
>  )
>  
> +fs = import('fs')
> +
> +feature_hdrs = [
> +  '/usr/include/cxl/features.h',
> +  '/usr/include/fwctl/cxl.h',
> +  '/usr/include/fwctl/fwctl.h',
> +]
> +
> +feat_hdrs_exist = true
> +foreach file : feature_hdrs
> +  if not fs.exists(file)
> +    feat_hdrs_exist = false
> +    break
> +  endif
> +endforeach
> +
> +if feat_hdrs_exist
> +    features = executable('cxl-features-control', [
> +        'cxl-features-control.c',
> +    ],
> +    include_directories : root_inc,
> +    dependencies : libcxl_deps,
> +    )
> +else
> +    features = []
> +endif

This does not look like typical meson conditional functionality. I would
expect is to match what happens for conditional "keyutils"
functionality.  A new "if get_option('fwctl').enabled()" option that
gates other dependencies.

You can add Acked-by for the series after addressing above comments.