On Sun, Sep 29, 2024 at 10:03:23PM -0500, Bjorn Andersson wrote:
> On Fri, Sep 27, 2024 at 04:51:32PM GMT, Wasim Nazir wrote:
> > This test includes:
> > 1) Start/stop test for each rproc instance sequencially
> > 2) Start/stop test for all rproc instances concurrently
> >
>
> This fails to describe the purpose of the patch. Provide a proper commit
> mesasge.
>
> In particular, I expect an argumentation for your test scheme. Will this
> work across all remoteproc instances? Does it have any dependencies,
> etc...
As we are tesing only the core ops based on the availabe sysfs entries,
it should work accross targets where remoteproc config is enabled & instances
are available. Otherwise I am skipping the tests.
Please correct me if I am missing anything here.
I will try to elaborate purpose of the tests in next patch.
>
> > Changes in v2:
> > - Update commit message
> > - Addressed start/stop flow
>
> The changelog goes below the '---' line, adjacent to your diffstat -
> which is missing from your patch. I don't know how you're sending these
> patches, but your system is either configured weirdly or you're not
> following my instructions on go/upstream.
>
Will do the correction in next patch.
> >
> > Signed-off-by: Wasim Nazir <quic_wasimn@xxxxxxxxxxx>
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a77770cd96b8..02ebad5ae790 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -19596,6 +19596,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@xxxxxxxxxx>
> > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> > index b38199965f99..0c8a0f427d01 100644
> > --- a/tools/testing/selftests/Makefile
> > +++ b/tools/testing/selftests/Makefile
> > @@ -82,6 +82,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..589368285307
> > --- /dev/null
> > +++ b/tools/testing/selftests/remoteproc/remoteproc_test.sh
> > @@ -0,0 +1,134 @@
> > +#!/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
> > + . "$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_instances=
> > +# Declare an array to save initial states of each instance
> > +org_instance_to_state=""
> > +num_tests=0
> > +test_err=0
> > +
> > +check_error() {
> > + if [ $? -ne 0 ]; then
> > + test_err=$((test_err+1))
> > + ktap_print_msg "$@"
> > + fi
> > +}
> > +
> > +rproc_stop_instances() {
> > + for instance in ${rproc_instances}; do
> > + rproc=${RPROC_SYS}/$instance
> > + rproc_name=$(cat $rproc/name)
> > + rproc_state=$(cat $rproc/state)
> > +
> > + echo stop > "$rproc/state"
> > + check_error "$rproc_name state-stop failed at state $rproc_state"
> > + done
> > + sleep ${RPROC_SEQ_SLEEP}
> > +}
> > +
> > +rproc_start_instances() {
> > + for instance in ${rproc_instances}; do
> > + rproc=${RPROC_SYS}/$instance
> > + rproc_name=$(cat $rproc/name)
> > + rproc_state=$(cat $rproc/state)
> > +
> > + echo start > "$rproc/state"
> > + check_error "$rproc_name state-start failed at state $rproc_state"
> > + done
> > + sleep ${RPROC_SEQ_SLEEP}
> > +}
> > +
> > +rproc_seq_test_instance_one() {
> > + instance=$1
> > + rproc=${RPROC_SYS}/$instance
> > + rproc_name=$(cat $rproc/name)
> > + rproc_state=$(cat $rproc/state)
> > + ktap_print_msg "Testing rproc sequence for $rproc_name"
> > +
> > + # Reset test_err value
> > + test_err=0
> > +
> > + # Begin start/stop sequence
> > + echo start > "$rproc/state"
> > + check_error "$rproc_name state-start failed at state $rproc_state"
> > +
> > + sleep ${RPROC_SEQ_SLEEP}
> > +
> > + echo stop > "$rproc/state"
> > + check_error "$rproc_name state-stop failed at state $rproc_state"
> > +
> > + if [ $test_err -ne 0 ]; then
> > + ktap_test_fail "$rproc_name"
> > + else
> > + ktap_test_pass "$rproc_name"
> > + fi
> > +}
> > +
> > +rproc_seq_test_instances_concurrently() {
> > + # Reset test_err value
> > + test_err=0
> > +
> > + rproc_start_instances
> > +
> > + rproc_stop_instances
> > +
> > + if [ $test_err -ne 0 ]; then
> > + ktap_test_fail "for any of $rproc_instances"
> > + else
> > + ktap_test_pass "for all $rproc_instances"
> > + fi
> > +}
> > +
> > +ktap_print_header
> > +
> > +if [ ! -d "${RPROC_SYS}" ]; then
> > + ktap_skip_all "${RPROC_SYS} doesn't exist."
> > + exit "${KSFT_SKIP}"
> > +fi
> > +
> > +rproc_instances=$(find ${RPROC_SYS}/remoteproc* -maxdepth 1 -exec basename {} \;)
> > +num_tests=$(echo ${rproc_instances} | wc -w)
> > +if [ "${num_tests}" -eq 0 ]; then
> > + ktap_skip_all "${RPROC_SYS}/remoteproc* doesn't exist."
> > + exit "${KSFT_SKIP}"
> > +fi
> > +
> > +# Total tests will be:
> > +# 1) Seq tests for each instance sequencially
> > +# 2) Seq tests for all instances concurrently
> > +num_tests=$((num_tests+1))
> > +
> > +ktap_set_plan "${num_tests}"
> > +
> > +# Stop all instances
> > +rproc_stop_instances
>
> Will this not fail for remoteproc instances that aren't started
> automatically?
This is don't care case where I only intend to stop previous instances
wherever applicable.
After this I am expecting that all instances will be in stop state so
that I can test start/stop sequence. This is inline to the previous review comments.
I will add comments in code for further clarification.
Thanks & Regards,
Wasim
>
> Regards,
> Bjorn
>
> > +
> > +# Test 1
> > +ktap_print_msg "Testing rproc start/stop sequence for each instance sequencially"
> > +for instance in ${rproc_instances}; do
> > + rproc_seq_test_instance_one $instance
> > +done
> > +
> > +# Test 2
> > +ktap_print_msg "Testing rproc start/stop sequence for all instances concurrently"
> > +rproc_seq_test_instances_concurrently
> > +
> > +# Restore all instances
> > +rproc_start_instances
> > +
> > +ktap_finished
> > --
> > 2.46.1
> >
>
