Re: [PATCH 1/3] perf tests test_arm_coresight: Fix the shellcheck warning in latest test_arm_coresight.sh

[Suzuki K Poulose]

On 05/10/2023 06:02, Namhyung Kim wrote:
On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
<atraj...@linux.vnet.ibm.com> wrote:

Running shellcheck on tests/shell/test_arm_coresight.sh
throws below warnings:

         In tests/shell/test_arm_coresight.sh line 15:
         cs_etm_path=$(find  /sys/bus/event_source/devices/cs_etm/ -name cpu* 
-print -quit)
                   ^--^ SC2061: Quote the parameter to -name so the shell won't 
interpret it.

         In tests/shell/test_arm_coresight.sh line 20:
                 if [ $archhver -eq 5 -a "$(printf "0x%X\n" $archpart)" = 
"0xA13" ] ; then
                                      ^-- SC2166: Prefer [ p ] && [ q ] as [ p 
-a q ] is not well defined

This warning is observed after commit:
"commit bb350847965d ("perf test: Update cs_etm testcase for Arm ETE")"

Fixed this issue by using quoting 'cpu*' for SC2061 and
using "&&" in line number 20 for SC2166 warning

Fixes: bb350847965d ("perf test: Update cs_etm testcase for Arm ETE")
Signed-off-by: Athira Rajeev <atraj...@linux.vnet.ibm.com>

Thanks for the fix.

Nothing to do with this patch, but I am wondering if the original patch
is over engineered and may not be future proof.

e.g.,

cs_etm_dev_name() {
+	cs_etm_path=$(find  /sys/bus/event_source/devices/cs_etm/ -name cpu* 
-print -quit)

Right there you got the device name and we can easily deduce the name of
the "ETM" node.

e.g,:
        etm=$(basename $(readlink cs_etm_path) | sed "s/[0-9]\+$//")

And practically, nobody prevents an ETE mixed with an ETM on a "hybrid"
system (hopefully, no one builds it ;-))

Also, instead of hardcoding "ete" and "etm" prefixes from the arch part,
we should simply use the cpu nodes from :

/sys/bus/event_source/devices/cs_etm/

e.g.,

arm_cs_etm_traverse_path_test() {
        # Iterate for every ETM device
        for c in /sys/bus/event_source/devices/cs_etm/cpu*; do
                # Read the link to be on the safer side
                dev=`readlink $c`
                
                # Find the ETM device belonging to which CPU
                cpu=`cat $dev/cpu`

                # Use depth-first search (DFS) to iterate outputs
                arm_cs_iterate_devices $dev $cpu
        done;
}




You'd better add Coresight folks on this.
Maybe this file was missing in the MAINTAINERS file.

And the original author of the commit, that introduced the issue too.

Suzuki


Thanks,
Namhyung


---
  tools/perf/tests/shell/test_arm_coresight.sh | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/test_arm_coresight.sh 
b/tools/perf/tests/shell/test_arm_coresight.sh
index fe78c4626e45..f2115dfa24a5 100755
--- a/tools/perf/tests/shell/test_arm_coresight.sh
+++ b/tools/perf/tests/shell/test_arm_coresight.sh
@@ -12,12 +12,12 @@
  glb_err=0

  cs_etm_dev_name() {
-       cs_etm_path=$(find  /sys/bus/event_source/devices/cs_etm/ -name cpu* 
-print -quit)
+       cs_etm_path=$(find  /sys/bus/event_source/devices/cs_etm/ -name 'cpu*' 
-print -quit)
         trcdevarch=$(cat ${cs_etm_path}/mgmt/trcdevarch)
         archhver=$((($trcdevarch >> 12) & 0xf))
         archpart=$(($trcdevarch & 0xfff))

-       if [ $archhver -eq 5 -a "$(printf "0x%X\n" $archpart)" = "0xA13" ] ; 
then
+       if [ $archhver -eq 5 ] && [ "$(printf "0x%X\n" $archpart)" = "0xA13" ] 
; then
                 echo "ete"
         else
                 echo "etm"
--
2.31.1