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:   Mon, 11 May 2020 20:11:52 +0530
From:   Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
To:     Suzuki K Poulose <suzuki.poulose@....com>
Cc:     mike.leach@...aro.org, mathieu.poirier@...aro.org,
        swboyd@...omium.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux@...linux.org.uk
Subject: Re: [PATCH] coresight: dynamic-replicator: Fix handling of multiple
 connections

Hi Suzuki,

On 2020-05-11 20:00, Suzuki K Poulose wrote:
> On 05/11/2020 03:16 PM, Sai Prakash Ranjan wrote:
>> Hi Mike,
>> 
>> On 2020-05-11 16:44, Mike Leach wrote:
>> [...]
>> 
>>>> 
>>>> I checked with the debug team and there is a limitation with
>>>> the replicator(swao_replicator) in the AOSS group where it
>>>> loses the idfilter register context when the clock is disabled.
>>>> This is not just in SC7180 SoC but also reported on some latest
>>>> upcoming QCOM SoCs as well and will need to be taken care in
>>>> order to enable coresight on these chipsets.
>>>> 
>>>> Here's what's happening -  After the replicator is initialized,
>>>> the clock is disabled in amba_pm_runtime_suspend() as a part of
>>>> pm runtime workqueue with the assumption that there will be no
>>>> loss of context after the replicator is initialized. But it doesn't
>>>> hold good with the replicators with these unfortunate limitation
>>>> and the idfilter register context is lost.
>>>> 
>>>> [    5.889406] amba_pm_runtime_suspend devname=6b06000.replicator 
>>>> ret=0
>>>> [    5.914516] Workqueue: pm pm_runtime_work
>>>> [    5.918648] Call trace:
>>>> [    5.921185]  dump_backtrace+0x0/0x1d0
>>>> [    5.924958]  show_stack+0x2c/0x38
>>>> [    5.928382]  dump_stack+0xc0/0x104
>>>> [    5.931896]  amba_pm_runtime_suspend+0xd8/0xe0
>>>> [    5.936469]  __rpm_callback+0xe0/0x140
>>>> [    5.940332]  rpm_callback+0x38/0x98
>>>> [    5.943926]  rpm_suspend+0xec/0x618
>>>> [    5.947522]  rpm_idle+0x5c/0x3f8
>>>> [    5.950851]  pm_runtime_work+0xa8/0xc0
>>>> [    5.954718]  process_one_work+0x1f8/0x4c0
>>>> [    5.958848]  worker_thread+0x50/0x468
>>>> [    5.962623]  kthread+0x12c/0x158
>>>> [    5.965957]  ret_from_fork+0x10/0x1c
>>>> 
>>>> This is a platform/SoC specific replicator issue, so we can either
>>>> introduce some DT property for replicators to identify which 
>>>> replicator
>>>> has this limitation, check in replicator_enable() and reset the
>>>> registers
>>>> or have something like below diff to check the idfilter registers in
>>>> replicator_enable() and then reset with clear comment specifying 
>>>> it’s
>>>> the
>>>> hardware limitation on some QCOM SoCs. Please let me know your 
>>>> thoughts
>>>> on
>>>> this?
>>>> 
>> 
>> Sorry for hurrying up and sending the patch - 
>> https://lore.kernel.org/patchwork/patch/1239923/.
>> I will send v2 based on further feedbacks here or there.
>> 
>>> 
>>> 1) does this replicator part have a unique ID that differs from the
>>> standard ARM designed replicators?
>>> If so perhaps link the modification into this. (even if the part no 
>>> in
>>> PIDR0/1 is the same the UCI should be different for a different
>>> implementation)
>>> 
>> 
>> pid=0x2bb909 for both replicators. So part number is same.
>> UCI will be different for different implementation(QCOM maybe 
>> different from ARM),
>> but will it be different for different replicators under the same 
>> impl(i.e., on QCOM).
> 
> May be use PIDR4.DES_2 to match the Implementor and apply the work
> around for all QCOM replicators ?
> 
> To me that sounds the best option.
> 

Ok we can do this as well, but just for my understanding, why do we need 
to reset replicators
in replicator_probe() and not in replicator_enable()? Are we accessing 
anything before
we enable replicators?

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