linux-kernel - Re: [PATCH] selftest: remoteproc: Add basic test for start/stop sequence

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ad6zsxcmqxavf7uo76ipriqjg7ipijafkaehgs5wthyyf364lv@kg4kzrxgucgl>
Date: Tue, 24 Sep 2024 17:53:26 -0700
From: Bjorn Andersson <andersson@...nel.org>
To: Wasim Nazir <quic_wasimn@...cinc.com>
Cc: Shuah Khan <shuah@...nel.org>, 
	Mathieu Poirier <mathieu.poirier@...aro.org>, linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH] selftest: remoteproc: Add basic test for start/stop
 sequence

On Fri, Sep 20, 2024 at 11:28:42PM GMT, Wasim Nazir wrote:
> Add new basic remoteproc test that check start/stop
> sequence of all subsystems available.
> 

Please describe your test scenario more than just "check start/stop
sequence".

Signed-off-by...

> diff --git a/MAINTAINERS b/MAINTAINERS
> index e062b5328341..aff76edc4242 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18225,6 +18225,7 @@ F:	Documentation/staging/remoteproc.rst
>  F:	drivers/remoteproc/
>  F:	include/linux/remoteproc.h
>  F:	include/linux/remoteproc/
> +F:	tools/testing/selftests/remoteproc/
> 
>  REMOTE PROCESSOR MESSAGING (RPMSG) SUBSYSTEM
>  M:	Bjorn Andersson <andersson@...nel.org>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 697f13bbbc32..31db0311efdc 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -68,6 +68,7 @@ TARGETS += proc
>  TARGETS += pstore
>  TARGETS += ptrace
>  TARGETS += openat2
> +TARGETS += remoteproc
>  TARGETS += resctrl
>  TARGETS += riscv
>  TARGETS += rlimits
> diff --git a/tools/testing/selftests/remoteproc/Makefile b/tools/testing/selftests/remoteproc/Makefile
> new file mode 100644
> index 000000000000..a84b3934fd36
> --- /dev/null
> +++ b/tools/testing/selftests/remoteproc/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +TEST_PROGS := remoteproc_test.sh
> +
> +include ../lib.mk
> diff --git a/tools/testing/selftests/remoteproc/config b/tools/testing/selftests/remoteproc/config
> new file mode 100644
> index 000000000000..a5c237d2f3b4
> --- /dev/null
> +++ b/tools/testing/selftests/remoteproc/config
> @@ -0,0 +1 @@
> +CONFIG_REMOTEPROC=y
> diff --git a/tools/testing/selftests/remoteproc/remoteproc_test.sh b/tools/testing/selftests/remoteproc/remoteproc_test.sh
> new file mode 100644
> index 000000000000..88c8f15d8406
> --- /dev/null
> +++ b/tools/testing/selftests/remoteproc/remoteproc_test.sh
> @@ -0,0 +1,165 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> +#
> +
> +DIR="$(dirname $(readlink -f "$0"))"
> +
> +KTAP_HELPERS="${DIR}/../kselftest/ktap_helpers.sh"
> +if [ -e "$KTAP_HELPERS" ]; then
> +    source "$KTAP_HELPERS"
> +else
> +    echo -n "1..0 # SKIP $KTAP_HELPERS file not found"
> +	exit 4
> +fi
> +
> +RPROC_SYS=/sys/class/remoteproc
> +RPROC_SEQ_SLEEP=5
> +rproc_ss_files=
> +num_tests=0
> +test_err=0
> +
> +check_error() {
> +	if [ $? -ne 0 ]; then
> +		test_err=$((test_err+1))
> +		ktap_print_msg "$@"
> +	fi
> +}
> +
> +rproc_seq_test_ss_one() {
> +	ss=$1

"ss" or "subsystem" are Qualcomm terms, please use "remoteproc instance"
instead.

> +	rproc=${RPROC_SYS}/$ss
> +	rproc_name=$(cat $rproc/name)
> +	rproc_state=$(cat $rproc/state)
> +	rproc_ssr=$(cat $rproc/recovery)
> +	ktap_print_msg "Testing rproc sequence for $rproc_name"
> +
> +	# Reset test_err value
> +	test_err=0
> +	if [ "$rproc_ssr" != "enabled" ]; then
> +		echo enabled > $rproc/recovery
> +		check_error "$rproc_name SSR-enabled failed"

Same with "SSR", you can express this with standard terms.

Why do we need "recovery" enabled in order to perform start/stop or
stop/start testing? Doesn't recovery only affect the crash code path?

> +	fi
> +
> +	if [ "$rproc_state" != "running" ]; then

I'd like to see your arguments in the commit message, or a comment here,
of why you do either start/stop or stop/start - instead of e.g. make
sure they are all stopped and then start/stop them in the test.


PS. Please use check go/upstream and adopt b4.

Regards,
Bjorn

> +		echo start > "$rproc/state"
> +		check_error "$rproc_name state-start failed"
> +
> +		sleep ${RPROC_SEQ_SLEEP}
> +
> +		echo stop > "$rproc/state"
> +		check_error "$rproc_name state-stop failed"
> +	else
> +		echo stop > "$rproc/state"
> +		check_error "$rproc_name state-stop failed"
> +
> +		sleep ${RPROC_SEQ_SLEEP}
> +
> +		echo start > "$rproc/state"
> +		check_error "$rproc_name state-start failed"
> +	fi
> +
> +	if [ $test_err -ne 0 ]; then
> +		ktap_test_fail "$rproc_name"
> +	else
> +		ktap_test_pass "$rproc_name"
> +	fi
> +}