[PATCH 0/3] Fix for shellcheck issues with latest scripts in tests/shell

[PATCH 0/3] Fix for shellcheck issues with latest scripts in tests/shell

[Athira Rajeev]

shellcheck was run on perf tool shell scripts as a pre-requisite
to include a build option for shellcheck discussed here:
https://www.spinics.net/lists/linux-perf-users/msg25553.html

And fixes were added for the coding/formatting issues in
two patchsets:
https://lore.kernel.org/linux-perf-users/20230613164145.50488-1-atrajeev@linux.vnet.ibm.com/
https://lore.kernel.org/linux-perf-users/20230709182800.53002-1-atrajeev@linux.vnet.ibm.com/

Three additional issues were observed and fixes are part of:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next

With recent commits in perf, other three issues are observed.
shellcheck version: 0.6.0

With this patchset:

for F in $(find tests/shell/ -perm -o=x -name '*.sh'); do shellcheck -S warning $F; done
echo $?
0

The changes are with recent commits ( which is mentioned in each patch)
for ock_contention, record_sideband and test_arm_coresight testcases.
The changes are made on top of:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next

Athira Rajeev (3):
  perf tests test_arm_coresight: Fix the shellcheck warning in latest
    test_arm_coresight.sh
  tools/perf/tests Ignore the shellcheck SC2046 warning in
    lock_contentio
  tools/perf/tests: Fix shellcheck warning in record_sideband.sh test

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

-- 
2.31.1

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

[Athira Rajeev]

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@linux.vnet.ibm.com>
---
 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

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

[Namhyung Kim]

On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
<atrajeev@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@linux.vnet.ibm.com>

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

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
>

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

[James Clark]

On 05/10/2023 06:02, Namhyung Kim wrote:
> On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
> <atrajeev@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@linux.vnet.ibm.com>
> 
> You'd better add Coresight folks on this.
> Maybe this file was missing in the MAINTAINERS file.
> 
> Thanks,
> Namhyung
> 

get_maintainer already does a pretty good job with the defaults, but
maybe I can try to add all the Coresight Perf stuff in the maintainers file.

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

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

Re: [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@arm.com> wrote:
> 
> On 05/10/2023 06:02, Namhyung Kim wrote:
>> On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
>> <atrajeev@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@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

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

[Suzuki K Poulose]

Hi,

On 12/10/2023 16:56, Athira Rajeev wrote:
> 
> 
>> On 05-Oct-2023, at 3:06 PM, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> On 05/10/2023 06:02, Namhyung Kim wrote:
>>> On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
>>> <atrajeev@linux.vnet.ibm.com> wrote:
>>>>

...

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

Yes, please. James will send out the proposed patch

Suzuki

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

[Athira Rajeev]

> On 12-Oct-2023, at 9:37 PM, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
> Hi,
> 
> On 12/10/2023 16:56, Athira Rajeev wrote:
>>> On 05-Oct-2023, at 3:06 PM, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>> 
>>> On 05/10/2023 06:02, Namhyung Kim wrote:
>>>> On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
>>>> <atrajeev@linux.vnet.ibm.com> wrote:
>>>>> 
> 
> ...
> 
>>> 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 ?
> 
> Yes, please. James will send out the proposed patch

Hi Suzuki,

Sure. Thanks! 

Athira
> 
> Suzuki
> 
> 

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

[James Clark]

On 29/09/2023 05:11, Athira Rajeev 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@linux.vnet.ibm.com>
> ---
>  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"


Reviewed-by: James Clark <james.clark@arm.com>

Re: [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 1:50 PM, James Clark <james.clark@arm.com> wrote:
> 
> 
> 
> On 29/09/2023 05:11, Athira Rajeev 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@linux.vnet.ibm.com>
>> ---
>> 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"
> 
> 
> Reviewed-by: James Clark <james.clark@arm.com>

Thanks James for checking

Athira

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

[Arnaldo Carvalho de Melo]

Em Thu, Oct 05, 2023 at 02:24:15PM +0530, Athira Rajeev escreveu:
> > On 05-Oct-2023, at 1:50 PM, James Clark <james.clark@arm.com> wrote:
> > On 29/09/2023 05:11, Athira Rajeev 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@linux.vnet.ibm.com>
> >> ---
> >> 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"
> > 
> > 
> > Reviewed-by: James Clark <james.clark@arm.com>

Some are not applying, even after b4 picking up v2

Total patches: 3
---
Cover: ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.cover
 Link: https://lore.kernel.org/r/20231013073021.99794-1-atrajeev@linux.vnet.ibm.com
 Base: not specified
       git am ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.mbx
⬢[acme@toolbox perf-tools-next]$        git am ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.mbx
Applying: tools/perf/tests Ignore the shellcheck SC2046 warning in lock_contention
error: patch failed: tools/perf/tests/shell/lock_contention.sh:33
error: tools/perf/tests/shell/lock_contention.sh: patch does not apply
Patch failed at 0001 tools/perf/tests Ignore the shellcheck SC2046 warning in lock_contention
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
⬢[acme@toolbox perf-tools-next]$ git am --abort
⬢[acme@toolbox perf-tools-next]$

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

[Athira Rajeev]

> On 07-Nov-2023, at 3:14 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> Em Thu, Oct 05, 2023 at 02:24:15PM +0530, Athira Rajeev escreveu:
>>> On 05-Oct-2023, at 1:50 PM, James Clark <james.clark@arm.com> wrote:
>>> On 29/09/2023 05:11, Athira Rajeev 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@linux.vnet.ibm.com>
>>>> ---
>>>> 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"
>>> 
>>> 
>>> Reviewed-by: James Clark <james.clark@arm.com>
> 
> Some are not applying, even after b4 picking up v2
> 
> Total patches: 3
> ---
> Cover: ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.cover
> Link: https://lore.kernel.org/r/20231013073021.99794-1-atrajeev@linux.vnet.ibm.com
> Base: not specified
>       git am ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.mbx
> ⬢[acme@toolbox perf-tools-next]$        git am ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.mbx
> Applying: tools/perf/tests Ignore the shellcheck SC2046 warning in lock_contention
> error: patch failed: tools/perf/tests/shell/lock_contention.sh:33
> error: tools/perf/tests/shell/lock_contention.sh: patch does not apply
> Patch failed at 0001 tools/perf/tests Ignore the shellcheck SC2046 warning in lock_contention
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> ⬢[acme@toolbox perf-tools-next]$ git am --abort
> ⬢[acme@toolbox perf-tools-next]$

Hi Arnaldo

The patch is picked up : https://lore.kernel.org/all/169757198796.167943.10552920255799914362.b4-ty@kernel.org/ .
Thanks for looking into.

Athira

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

[Arnaldo Carvalho de Melo]

Em Tue, Nov 07, 2023 at 12:08:25PM +0530, Athira Rajeev escreveu:
> > On 07-Nov-2023, at 3:14 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >>> Reviewed-by: James Clark <james.clark@arm.com>

> > Some are not applying, even after b4 picking up v2

> > Total patches: 3

> > Cover: ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.cover
> > Link: https://lore.kernel.org/r/20231013073021.99794-1-atrajeev@linux.vnet.ibm.com
> > Base: not specified
> >       git am ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.mbx
> > ⬢[acme@toolbox perf-tools-next]$        git am ./v2_20231013_atrajeev_fix_for_shellcheck_issues_with_latest_scripts_in_tests_shell.mbx
> > Applying: tools/perf/tests Ignore the shellcheck SC2046 warning in lock_contention
> > error: patch failed: tools/perf/tests/shell/lock_contention.sh:33
> > error: tools/perf/tests/shell/lock_contention.sh: patch does not apply
> > Patch failed at 0001 tools/perf/tests Ignore the shellcheck SC2046 warning in lock_contention
> > hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> > ⬢[acme@toolbox perf-tools-next]$ git am --abort
> > ⬢[acme@toolbox perf-tools-next]$
 
> The patch is picked up : https://lore.kernel.org/all/169757198796.167943.10552920255799914362.b4-ty@kernel.org/ .
> Thanks for looking into.

Thanks for checking, my mistake then,

- Arnaldo

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

[David Laight]

From: Athira Rajeev
> Sent: 29 September 2023 05:12
> 
> 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@linux.vnet.ibm.com>
> ---
>  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)

Isn't the intention to get the shell to expand "cpu* ?
So quoting it completely breaks the script.

>  	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

The only issue I see there is that $archhver could be in "".
IIRC test is required to parse the 5 part expression "a op b -a c op d"
in the 'expected' manner even if any of a/b/c/d look like operators.
In any case '(' and ')' can be used to force the ordering.
Or, more usually, prepend an x as in:

if [ "x$archhver" = x5 -a "$(printf "0x%X\n" $archpart)" = "0xA13" ] ; then

	David

>  		echo "ete"
>  	else
>  		echo "etm"
> --
> 2.31.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[David Laight]

From: David Laight
> Sent: 05 October 2023 11:16
...
> > -	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)
> 
> Isn't the intention to get the shell to expand "cpu* ?
> So quoting it completely breaks the script.

Complete brain-fade :-(

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [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 9:15 PM, David Laight <David.Laight@ACULAB.COM> wrote:
> 
> From: David Laight
>> Sent: 05 October 2023 11:16
> ...
>>> - 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)
>> 
>> Isn't the intention to get the shell to expand "cpu* ?
>> So quoting it completely breaks the script.
> 
> Complete brain-fade :-(

Hi David,

Yeah, quoting it also will expand

Thanks
Athira
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

[PATCH 2/3] tools/perf/tests Ignore the shellcheck SC2046 warning in lock_contentio

[Athira Rajeev]

Running shellcheck on lock_contention.sh generates below
warning

	In tests/shell/lock_contention.sh line 36:
	   if [ `nproc` -lt 4 ]; then
		  ^-----^ SC2046: Quote this to prevent word splitting.

Here since nproc will generate a single word output
and there is no possibility of word splitting, this
warning can be ignored. Use exception for this with
"disable" option in shellcheck. This warning is observed
after commit:
"commit 29441ab3a30a ("perf test lock_contention.sh: Skip test
if not enough CPUs")"

Fixes: 29441ab3a30a ("perf test lock_contention.sh: Skip test if not enough CPUs")
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/lock_contention.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/tests/shell/lock_contention.sh b/tools/perf/tests/shell/lock_contention.sh
index d5a191d3d090..c1ec5762215b 100755
--- a/tools/perf/tests/shell/lock_contention.sh
+++ b/tools/perf/tests/shell/lock_contention.sh
@@ -33,6 +33,7 @@ check() {
 		exit
 	fi
 
+	# shellcheck disable=SC2046
 	if [ `nproc` -lt 4 ]; then
 		echo "[Skip] Low number of CPUs (`nproc`), lock event cannot be triggered certainly"
 		err=2
-- 
2.31.1

[PATCH 3/3] tools/perf/tests: Fix shellcheck warning in record_sideband.sh test

[Athira Rajeev]

Running shellcheck on record_sideband.sh throws below
warning:

	In tests/shell/record_sideband.sh line 25:
	  if ! perf record -o ${perfdata} -BN --no-bpf-event -C $1 true 2>&1 >/dev/null
	    ^--^ SC2069: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).

This shows shellcheck warning SC2069 where the redirection
order needs to be fixed. Use { cmd > file; } 2>&1 to fix the
redirection of perf record output

Fixes: 23b97c7ee963 ("perf test: Add test case for record sideband events")
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/record_sideband.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/record_sideband.sh b/tools/perf/tests/shell/record_sideband.sh
index 5024a7ce0c51..7e036763a43c 100755
--- a/tools/perf/tests/shell/record_sideband.sh
+++ b/tools/perf/tests/shell/record_sideband.sh
@@ -22,7 +22,7 @@ trap trap_cleanup EXIT TERM INT
 
 can_cpu_wide()
 {
-    if ! perf record -o ${perfdata} -BN --no-bpf-event -C $1 true 2>&1 >/dev/null
+    if ! { perf record -o ${perfdata} -BN --no-bpf-event -C $1 true > /dev/null; } 2>&1
     then
         echo "record sideband test [Skipped cannot record cpu$1]"
         err=2
-- 
2.31.1

Re: [PATCH 3/3] tools/perf/tests: Fix shellcheck warning in record_sideband.sh test

[Namhyung Kim]

On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
> Running shellcheck on record_sideband.sh throws below
> warning:
>
>         In tests/shell/record_sideband.sh line 25:
>           if ! perf record -o ${perfdata} -BN --no-bpf-event -C $1 true 2>&1 >/dev/null
>             ^--^ SC2069: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
>
> This shows shellcheck warning SC2069 where the redirection
> order needs to be fixed. Use { cmd > file; } 2>&1 to fix the
> redirection of perf record output
>
> Fixes: 23b97c7ee963 ("perf test: Add test case for record sideband events")
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>  tools/perf/tests/shell/record_sideband.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/tests/shell/record_sideband.sh b/tools/perf/tests/shell/record_sideband.sh
> index 5024a7ce0c51..7e036763a43c 100755
> --- a/tools/perf/tests/shell/record_sideband.sh
> +++ b/tools/perf/tests/shell/record_sideband.sh
> @@ -22,7 +22,7 @@ trap trap_cleanup EXIT TERM INT
>
>  can_cpu_wide()
>  {
> -    if ! perf record -o ${perfdata} -BN --no-bpf-event -C $1 true 2>&1 >/dev/null
> +    if ! { perf record -o ${perfdata} -BN --no-bpf-event -C $1 true > /dev/null; } 2>&1

I think we usually go without braces.

Thanks,
Namhyung


>      then
>          echo "record sideband test [Skipped cannot record cpu$1]"
>          err=2
> --
> 2.31.1
>

Re: [PATCH 3/3] tools/perf/tests: Fix shellcheck warning in record_sideband.sh test

[Athira Rajeev]

> On 05-Oct-2023, at 10:34 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> On Thu, Sep 28, 2023 at 9:11 PM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> Running shellcheck on record_sideband.sh throws below
>> warning:
>> 
>>        In tests/shell/record_sideband.sh line 25:
>>          if ! perf record -o ${perfdata} -BN --no-bpf-event -C $1 true 2>&1 >/dev/null
>>            ^--^ SC2069: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
>> 
>> This shows shellcheck warning SC2069 where the redirection
>> order needs to be fixed. Use { cmd > file; } 2>&1 to fix the
>> redirection of perf record output
>> 
>> Fixes: 23b97c7ee963 ("perf test: Add test case for record sideband events")
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> tools/perf/tests/shell/record_sideband.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tools/perf/tests/shell/record_sideband.sh b/tools/perf/tests/shell/record_sideband.sh
>> index 5024a7ce0c51..7e036763a43c 100755
>> --- a/tools/perf/tests/shell/record_sideband.sh
>> +++ b/tools/perf/tests/shell/record_sideband.sh
>> @@ -22,7 +22,7 @@ trap trap_cleanup EXIT TERM INT
>> 
>> can_cpu_wide()
>> {
>> -    if ! perf record -o ${perfdata} -BN --no-bpf-event -C $1 true 2>&1 >/dev/null
>> +    if ! { perf record -o ${perfdata} -BN --no-bpf-event -C $1 true > /dev/null; } 2>&1
> 
> I think we usually go without braces.

Hi Namhyung

Thanks for reviving.I will fix this in V2

Thanks
Athira
> 
> Thanks,
> Namhyung
> 
> 
>>     then
>>         echo "record sideband test [Skipped cannot record cpu$1]"
>>         err=2
>> --
>> 2.31.1

Re: [PATCH 0/3] Fix for shellcheck issues with latest scripts in tests/shell

[kajoljain]

Patchset looks fine to me.

Reviewed-by: Kajol Jain <kjain@linux.ibm.com>

thanks,
Kajol Jain

On 9/29/23 09:41, Athira Rajeev wrote:
> shellcheck was run on perf tool shell scripts as a pre-requisite
> to include a build option for shellcheck discussed here:
> https://www.spinics.net/lists/linux-perf-users/msg25553.html
> 
> And fixes were added for the coding/formatting issues in
> two patchsets:
> https://lore.kernel.org/linux-perf-users/20230613164145.50488-1-atrajeev@linux.vnet.ibm.com/
> https://lore.kernel.org/linux-perf-users/20230709182800.53002-1-atrajeev@linux.vnet.ibm.com/
> 
> Three additional issues were observed and fixes are part of:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next
> 
> With recent commits in perf, other three issues are observed.
> shellcheck version: 0.6.0
> 
> With this patchset:
> 
> for F in $(find tests/shell/ -perm -o=x -name '*.sh'); do shellcheck -S warning $F; done
> echo $?
> 0
> 
> The changes are with recent commits ( which is mentioned in each patch)
> for ock_contention, record_sideband and test_arm_coresight testcases.
> The changes are made on top of:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next
> 
> Athira Rajeev (3):
>   perf tests test_arm_coresight: Fix the shellcheck warning in latest
>     test_arm_coresight.sh
>   tools/perf/tests Ignore the shellcheck SC2046 warning in
>     lock_contentio
>   tools/perf/tests: Fix shellcheck warning in record_sideband.sh test
> 
>  tools/perf/tests/shell/lock_contention.sh    | 1 +
>  tools/perf/tests/shell/record_sideband.sh    | 2 +-
>  tools/perf/tests/shell/test_arm_coresight.sh | 4 ++--
>  3 files changed, 4 insertions(+), 3 deletions(-)
>