Verma, Vishal L wrote: > On Thu, 2023-07-27 at 14:21 -0700, Ira Weiny wrote: > > Previously CXL event testing was run by hand. This reduces testing > > coverage including a lack of regression testing. > > > > Add a CXL test as part of the meson test infrastructure. Passing is > > predicated on receiving the appropriate number of errors in each log. > > Individual event values are not checked. > > > > Signed-off-by: Ira Weiny <ira.we...@intel.com> > > --- > > test/cxl-events.sh | 68 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > test/meson.build | 2 ++ > > 2 files changed, 70 insertions(+) > > Hi Ira, > > Thanks for adding this test. Just a few minor comments below, otherwise > looks good.
Thanks! > > > > > diff --git a/test/cxl-events.sh b/test/cxl-events.sh > > new file mode 100644 > > index 000000000000..f51046ec39ad > > --- /dev/null > > +++ b/test/cxl-events.sh > > @@ -0,0 +1,68 @@ > > +#!/bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (C) 2023 Intel Corporation. All rights reserved. > > + > > +. $(dirname $0)/common > > + > > +set -ex > > + > > +trap 'err $LINENO' ERR > > + > > +check_prereq "jq" > > + > > +modprobe -r cxl_test > > +modprobe cxl_test > > + > > +dev_path="/sys/bus/platform/devices" > > + > > +test_cxl_events() > > +{ > > + memdev="$1" > > + > > + echo "TEST: triggering $memdev" > > + echo 1 > $dev_path/$memdev/event_trigger > > Quote the "$variables" here. We don't expect spaces in the path in this > case, so it will still work, but it is good practice to always quote > everything. Done. > > We might also need a test to see if this file exists first. For kernels > that don't have this support, we probably want to print a message and > skip the test (return '77'). Good idea. > > > +} > > + > > +readarray -t memdevs < <("$CXL" list -b cxl_test -Mi | jq -r '.[].host') > > + > > +echo "TEST: Prep event trace" > > +echo "" > /sys/kernel/tracing/trace > > +echo 1 > /sys/kernel/tracing/events/cxl/enable > > +echo 1 > /sys/kernel/tracing/tracing_on > > + > > +# Only need to test 1 device > > +#for memdev in ${memdevs[@]}; do > > +#done > > Probably just remove the commented out loop, if we need to test more > than one memdev in the future, it is easy enough to add back then. Done. > > > + > > +test_cxl_events "$memdevs" > > Shouldn't use "$memdevs" here since it is an array. If you want to pass > in just the first memdev, use "${memdevs[0]}" Ah yea caught my hack ;-) done. > > > + > > +echo 0 > /sys/kernel/tracing/tracing_on > > + > > +echo "TEST: Events seen" > > +cat /sys/kernel/tracing/trace > > +num_overflow=$(grep "cxl_overflow" /sys/kernel/tracing/trace | wc -l) > > +num_fatal=$(grep "log=Fatal" /sys/kernel/tracing/trace | wc -l) > > +num_failure=$(grep "log=Failure" /sys/kernel/tracing/trace | wc -l) > > +num_info=$(grep "log=Informational" /sys/kernel/tracing/trace | wc -l) > > Minor nit, but you can 'grep -c' instead of 'grep | wc -l' Ok Done. > > Also could put /sys/kernel/tracing/trace into a variable just for > readability since it is used many times. Done. > > > +echo " LOG (Expected) : (Found)" > > +echo " overflow ( 1) : $num_overflow" > > +echo " Fatal ( 2) : $num_fatal" > > +echo " Failure (16) : $num_failure" > > +echo " Informational ( 3) : $num_info" > > + > > +if [ "$num_overflow" -ne 1 ]; then > > + err "$LINENO" > > +fi > > +if [ "$num_fatal" -ne 2 ]; then > > + err "$LINENO" > > +fi > > +if [ "$num_failure" -ne 16 ]; then > > + err "$LINENO" > > +fi > > +if [ "$num_info" -ne 3 ]; then > > + err "$LINENO" > > +fi > > Perhaps define variables for each of the expected nums, that way there > is only one spot to change in case the numbers change in the future. Good idea, done. V2 on it's way soon, thanks for looking, Ira > > > + > > +check_dmesg "$LINENO" > > + > > +modprobe -r cxl_test > > diff --git a/test/meson.build b/test/meson.build > > index a956885f6df6..a33255bde1a8 100644 > > --- a/test/meson.build > > +++ b/test/meson.build > > @@ -155,6 +155,7 @@ cxl_sysfs = find_program('cxl-region-sysfs.sh') > > cxl_labels = find_program('cxl-labels.sh') > > cxl_create_region = find_program('cxl-create-region.sh') > > cxl_xor_region = find_program('cxl-xor-region.sh') > > +cxl_events = find_program('cxl-events.sh') > > > > tests = [ > > [ 'libndctl', libndctl, 'ndctl' ], > > @@ -183,6 +184,7 @@ tests = [ > > [ 'cxl-labels.sh', cxl_labels, 'cxl' ], > > [ 'cxl-create-region.sh', cxl_create_region, 'cxl' ], > > [ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ], > > + [ 'cxl-events.sh', cxl_events, 'cxl' ], > > ] > > > > if get_option('destructive').enabled() > > > > --- > > base-commit: 2fd570a0ed788b1bd0971dfdb1466a5dbcb79775 > > change-id: 20230726-cxl-event-dc00a2f94b60 > > > > Best regards, >