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 <anders...@kernel.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
> +}
