Re: [PATCH V1 1/4] selftests: amd-pstate: Modify amd-pstate-ut.sh to basic.sh.

[Nathan Fontenot <nafonten@amd.com>]

On 7/6/22 02:36, Meng Li wrote:
> Modify amd-pstate-ut.sh to basic.sh.
> The purpose of this modification is to facilitate the subsequent
> addition of gitsource, tbench and other tests.
> 
> Signed-off-by: Meng Li <li.meng@amd.com>
> ---
>  tools/testing/selftests/amd-pstate/Makefile   |   3 +-
>  .../selftests/amd-pstate/amd-pstate-ut.sh     |  55 --------
>  tools/testing/selftests/amd-pstate/basic.sh   |  38 +++++
>  tools/testing/selftests/amd-pstate/main.sh    | 131 ++++++++++++++++++
>  tools/testing/selftests/amd-pstate/run.sh     |  15 ++
>  5 files changed, 186 insertions(+), 56 deletions(-)
>  delete mode 100755 tools/testing/selftests/amd-pstate/amd-pstate-ut.sh
>  create mode 100755 tools/testing/selftests/amd-pstate/basic.sh
>  create mode 100755 tools/testing/selftests/amd-pstate/main.sh
>  create mode 100755 tools/testing/selftests/amd-pstate/run.sh
> 
> diff --git a/tools/testing/selftests/amd-pstate/Makefile b/tools/testing/selftests/amd-pstate/Makefile
> index 199867f44b32..167ab51ec290 100644
> --- a/tools/testing/selftests/amd-pstate/Makefile
> +++ b/tools/testing/selftests/amd-pstate/Makefile
> @@ -4,6 +4,7 @@
>  # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
>  all:
>  
> -TEST_PROGS := amd-pstate-ut.sh
> +TEST_PROGS := run.sh
> +TEST_FILES := main.sh basic.sh
>  
>  include ../lib.mk
> diff --git a/tools/testing/selftests/amd-pstate/amd-pstate-ut.sh b/tools/testing/selftests/amd-pstate/amd-pstate-ut.sh
> deleted file mode 100755
> index 273364650285..000000000000
> --- a/tools/testing/selftests/amd-pstate/amd-pstate-ut.sh
> +++ /dev/null
> @@ -1,55 +0,0 @@
> -#!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0
> -
> -# amd-pstate-ut is a test module for testing the amd-pstate driver.
> -# It can only run on x86 architectures and current cpufreq driver
> -# must be amd-pstate.
> -# (1) It can help all users to verify their processor support
> -# (SBIOS/Firmware or Hardware).
> -# (2) Kernel can have a basic function test to avoid the kernel
> -# regression during the update.
> -# (3) We can introduce more functional or performance tests to align
> -# the result together, it will benefit power and performance scale optimization.
> -
> -# Kselftest framework requirement - SKIP code is 4.
> -ksft_skip=4
> -
> -# amd-pstate-ut only run on x86/x86_64 AMD systems.
> -ARCH=$(uname -m 2>/dev/null | sed -e 's/i.86/x86/' -e 's/x86_64/x86/')
> -VENDOR=$(cat /proc/cpuinfo | grep -m 1 'vendor_id' | awk '{print $NF}')
> -
> -if ! echo "$ARCH" | grep -q x86; then
> -	echo "$0 # Skipped: Test can only run on x86 architectures."
> -	exit $ksft_skip
> -fi
> -
> -if ! echo "$VENDOR" | grep -iq amd; then
> -	echo "$0 # Skipped: Test can only run on AMD CPU."
> -	echo "$0 # Current cpu vendor is $VENDOR."
> -	exit $ksft_skip
> -fi
> -
> -scaling_driver=$(cat /sys/devices/system/cpu/cpufreq/policy0/scaling_driver)
> -if [ "$scaling_driver" != "amd-pstate" ]; then
> -	echo "$0 # Skipped: Test can only run on amd-pstate driver."
> -	echo "$0 # Current cpufreq scaling drvier is $scaling_driver."
> -	exit $ksft_skip
> -fi
> -
> -msg="Skip all tests:"
> -if [ ! -w /dev ]; then
> -    echo $msg please run this as root >&2
> -    exit $ksft_skip
> -fi
> -
> -if ! /sbin/modprobe -q -n amd-pstate-ut; then
> -	echo "amd-pstate-ut: module amd-pstate-ut is not found [SKIP]"
> -	exit $ksft_skip
> -fi
> -if /sbin/modprobe -q amd-pstate-ut; then
> -	/sbin/modprobe -q -r amd-pstate-ut
> -	echo "amd-pstate-ut: ok"
> -else
> -	echo "amd-pstate-ut: [FAIL]"
> -	exit 1
> -fi
> diff --git a/tools/testing/selftests/amd-pstate/basic.sh b/tools/testing/selftests/amd-pstate/basic.sh
> new file mode 100755
> index 000000000000..e4c43193e4a3
> --- /dev/null
> +++ b/tools/testing/selftests/amd-pstate/basic.sh
> @@ -0,0 +1,38 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# amd-pstate-ut is a test module for testing the amd-pstate driver.
> +# It can only run on x86 architectures and current cpufreq driver
> +# must be amd-pstate.
> +# (1) It can help all users to verify their processor support
> +# (SBIOS/Firmware or Hardware).
> +# (2) Kernel can have a basic function test to avoid the kernel
> +# regression during the update.
> +# (3) We can introduce more functional or performance tests to align
> +# the result together, it will benefit power and performance scale optimization.
> +
> +# protect against multiple inclusion
> +if [ $FILE_BASIC ]; then
> +	return 0
> +else
> +	FILE_BASIC=DONE
> +fi
> +
> +amd_pstate_basic()
> +{
> +	printf "\n---------------------------------------------\n"
> +	printf "*** Running AMD P-state ut                ***"
> +	printf "\n---------------------------------------------\n"
> +
> +	if ! /sbin/modprobe -q -n amd-pstate-ut; then
> +		echo "amd-pstate-ut: module amd-pstate-ut is not found [SKIP]"
> +		exit $ksft_skip
> +	fi
> +	if /sbin/modprobe -q amd-pstate-ut; then
> +		/sbin/modprobe -q -r amd-pstate-ut
> +		echo "amd-pstate-basic: ok"
> +	else
> +		echo "amd-pstate-basic: [FAIL]"
> +		exit 1
> +	fi
> +}
> diff --git a/tools/testing/selftests/amd-pstate/main.sh b/tools/testing/selftests/amd-pstate/main.sh
> new file mode 100755
> index 000000000000..1c28b5d7b4c5
> --- /dev/null
> +++ b/tools/testing/selftests/amd-pstate/main.sh
> @@ -0,0 +1,131 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# protect against multiple inclusion
> +if [ $FILE_MAIN ]; then
> +	return 0
> +else
> +	FILE_MAIN=DONE
> +fi
> +
> +source basic.sh
> +
> +# amd-pstate-ut only run on x86/x86_64 AMD systems.
> +ARCH=$(uname -m 2>/dev/null | sed -e 's/i.86/x86/' -e 's/x86_64/x86/')
> +VENDOR=$(cat /proc/cpuinfo | grep -m 1 'vendor_id' | awk '{print $NF}')
> +
> +FUNC=all
> +OUTFILE=selftest
> +
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +
> +# All amd-pstate tests
> +amd_pstate_all()
> +{
> +	printf "\n=============================================\n"
> +	printf "***** Running AMD P-state Sanity Tests  *****\n"
> +	printf "=============================================\n\n"
> +
> +	# unit test for amd-pstate kernel driver
> +	amd_pstate_basic
> +}
> +
> +helpme()
> +{
> +	printf "Usage: $0 [OPTION...]
> +	[-h <help>]
> +	[-o <output-file-for-dump>]
> +	[-c <all: All testing,
> +	     basic: Basic testing.>]
> +	\n"
> +	exit 2
> +}
> +
> +parse_arguments()
> +{
> +	while getopts ho:c: arg
> +	do
> +		case $arg in
> +			h) # --help
> +				helpme
> +				;;
> +
> +			c) # --func_type (Function to perform: basic (default: all))
> +				FUNC=$OPTARG
> +				;;
> +
> +			o) # --output-file (Output file to store dumps)
> +				OUTFILE=$OPTARG
> +				;;
> +
> +			\?)
> +				helpme
> +				;;
> +		esac
> +	done
> +}
> +
> +prerequisite()
> +{
> +	if ! echo "$ARCH" | grep -q x86; then
> +		echo "$0 # Skipped: Test can only run on x86 architectures."
> +		exit $ksft_skip
> +	fi
> +
> +	if ! echo "$VENDOR" | grep -iq amd; then
> +		echo "$0 # Skipped: Test can only run on AMD CPU."
> +		echo "$0 # Current cpu vendor is $VENDOR."
> +		exit $ksft_skip
> +	fi
> +
> +	scaling_driver=$(cat /sys/devices/system/cpu/cpufreq/policy0/scaling_driver)
> +	if [ "$scaling_driver" != "amd-pstate" ]; then
> +		echo "$0 # Skipped: Test can only run on amd-pstate driver."
> +		echo "$0 # Current cpufreq scaling drvier is $scaling_driver."
> +		exit $ksft_skip
> +	fi
> +
> +	msg="Skip all tests:"
> +	if [ ! -w /dev ]; then
> +		echo $msg please run this as root >&2
> +		exit $ksft_skip
> +	fi
> +}
> +
> +do_test()
> +{
> +	case "$FUNC" in
> +		"all")
> +		amd_pstate_all
> +		;;
> +
> +		"basic")
> +		amd_pstate_basic
> +		;;
> +
> +		*)
> +		echo "Invalid [-f] function type"
> +		helpme
> +		;;
> +	esac
> +}
> +
> +# clear dumps
> +pre_clear_dumps()
> +{
> +	case "$FUNC" in
> +		"all")
> +		rm -rf $OUTFILE*
> +		;;
> +
> +		"basic")
> +		;;
> +
> +	esac

Small nit. You have two different indentation styles for your case statements, do_test
and pre_clear_dumps vs parse_arguments. Would be nice to stick to a single style.

> +}
> +
> +post_clear_dumps()
> +{
> +	rm -rf $OUTFILE.log
> +}
> diff --git a/tools/testing/selftests/amd-pstate/run.sh b/tools/testing/selftests/amd-pstate/run.sh
> new file mode 100755
> index 000000000000..247e80bfee44
> --- /dev/null
> +++ b/tools/testing/selftests/amd-pstate/run.sh
> @@ -0,0 +1,15 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +source main.sh
> +
> +# Parse arguments
> +parse_arguments $@
> +
> +# Make sure all requirements are met
> +prerequisite
> +
> +# Run requested functions
> +pre_clear_dumps
> +do_test | tee -a $OUTFILE.log
> +post_clear_dumps

run.sh seems like it's just a wrapper for main.sh, can these two files be merged into a single file?

-Nathan