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

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

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