linux-kernel - Re: [PATCH] coresight: dynamic-replicator: Fix handling of multiple connections

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 29 Apr 2020 17:17:41 +0530
From:   Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
To:     Suzuki K Poulose <suzuki.poulose@....com>, mike.leach@...aro.org
Cc:     mathieu.poirier@...aro.org, swboyd@...omium.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] coresight: dynamic-replicator: Fix handling of multiple
 connections

On 2020-04-28 17:53, Sai Prakash Ranjan wrote:
> On 2020-04-27 19:23, Suzuki K Poulose wrote:
>> On 04/27/2020 10:45 AM, Mike Leach wrote:
> [...]
>>>> 
>>>> This is not sufficient. You must prevent another session trying to
>>>> enable the other port of the replicator as this could silently fail
>>>> the "on-going" session. Not ideal. Fail the attempt to enable a port
>>>> if the other port is active. You could track this in software and
>>>> fail early.
>>>> 
>>>> Suzuki
>>> 
>>> While I have no issue in principle with not enabling a path to a sink
>>> that is not in use - indeed in some cases attaching to unused sinks
>>> can cause back-pressure that slows throughput (cf TPIU) - I am
>>> concerned that this modification is masking an underlying issue with
>>> the platform in question.
>>> 
>>> Should we decide to enable the diversion of different IDs to 
>>> different
>>> sinks or allow different sessions go to different sinks, then this 
>>> has
>>> potential to fail on the SC7180 SoC - and it will be difficult in
>>> future to associate a problem with this discussion.
>> 
>> Mike,
>> 
>> I think thats a good point.
>> Sai, please could we narrow down this to the real problem and may be
>> work around it for the "device" ? Do we know which sink is causing the
>> back pressure ? We could then push the "work around" to the replicator
>> it is connected to.
>> 
>> Suzuki
> 
> Hi Suzuki, Mike,
> 
> To add some more to the information provided earlier,
> swao_replicator(6b06000) and etf are
> in AOSS (Always-On-SubSystem) group. Also TPIU(connected to
> qdss_replicator) and EUD(connected
> to swao_replicator) sinks are unused.
> 
> Please ignore the id filter values provided earlier.
> Here are ID filter values after boot and before enabling replicator. As 
> per
> these idfilter values, we should not try to enable replicator if its 
> already
> enabled (in this case for swao_replicator) right?
> 
> localhost ~ # cat
> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter0
> 0x0
> localhost ~ # cat
> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter1
> 0x0
> 
> localhost ~ # cat
> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter0
> 0xff
> localhost ~ # cat
> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter1
> 0xff
> 

Looking more into replicator1(swao_replicator) values as 0x0 even after 
replicator_reset()
in replicator probe, I added dynamic_replicator_reset in 
dynamic_replicator_enable()
and am not seeing any hardlockup. Also I added some prints to check the 
idfilter
values before and after reset and found that its not set to 0xff even 
after replicator_reset()
in replicator probe, I don't see any other path setting it to 0x0.

After probe:

[    8.477669] func replicator_probe before reset replicator replicator1 
REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
[    8.489470] func replicator_probe after reset replicator replicator1 
REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
[    8.502738] func replicator_probe before reset replicator replicator0 
REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
[    8.515214] func replicator_probe after reset replicator replicator0 
REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
localhost ~ #
localhost ~ #
localhost ~ # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
localhost ~ #
localhost ~ # echo 1 > /sys/bus/coresight/devices/etm0/enable_source
[   58.490485] func dynamic_replicator_enable before reset replicator 
replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
[   58.503246] func dynamic_replicator_enable after reset replicator 
replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
[   58.520902] func dynamic_replicator_enable before reset replicator 
replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
[   58.533500] func dynamic_replicator_enable after reset replicator 
replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
localhost ~ #

Can we have a replicator_reset in dynamic_replicator_enable?

diff --git a/drivers/hwtracing/coresight/coresight-replicator.c 
b/drivers/hwtracing/coresight/coresight-replicator.c
index e7dc1c31d20d..794f8e4c049f 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -68,6 +68,8 @@ static int dynamic_replicator_enable(struct 
replicator_drvdata *drvdata,
         int rc = 0;
         u32 reg;

+       dynamic_replicator_reset(drvdata);
+
         switch (outport) {
         case 0:
                 reg = REPLICATOR_IDFILTER0;

Thanks,
Sai
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation