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

[Athira Rajeev]

> On 05-Oct-2023, at 3:06 PM, Suzuki K Poulose <suzuki.poulose at arm.com> wrote:
> 
> On 05/10/2023 06:02, Namhyung Kim wrote:
>> On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
>> <atrajeev at 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 <atrajeev at 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

Hi All,
Thanks for the discussion and feedbacks.

This patch fixes the shellcheck warning introduced in function "cs_etm_dev_name". But with the changes that Suzuki suggested, we won't need the function "cs_etm_dev_name" since the code will use "/sys/bus/event_source/devices/cs_etm/" .  In that case, can I drop this patch for now from this series ?

Thanks
Athira

> 
>> 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