[RFC PATCH] coresight: dynamic-replicator: Fix handling of multiple connections

[RFC PATCH] coresight: dynamic-replicator: Fix handling of multiple connections

[Sai Prakash Ranjan]

Since commit 30af4fb619e5 ("coresight: dynamic-replicator:
Handle multiple connections"), we do not make sure that
the other port is disabled when the dynamic replicator is
enabled. This is seen to cause the CPU hardlockup atleast
on SC7180 SoC when enabling ETM with ETR as the sink via
sysfs.

  Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
  CPU: 7 PID: 0 Comm: swapper/7 Tainted: G S  B             5.4.25 #100
  Hardware name: Qualcomm Technologies, Inc. SC7180 IDP (DT)
  Call trace:
   dump_backtrace+0x0/0x188
   show_stack+0x20/0x2c
   dump_stack+0xdc/0x144
   panic+0x168/0x370
   arch_seccomp_spec_mitigate+0x0/0x14
   watchdog_timer_fn+0x68/0x290
   __hrtimer_run_queues+0x264/0x498
   hrtimer_interrupt+0xf0/0x22c
   arch_timer_handler_phys+0x40/0x50
   handle_percpu_devid_irq+0x8c/0x158
   __handle_domain_irq+0x84/0xc4
   gic_handle_irq+0x100/0x1c4
   el1_irq+0xbc/0x180
   arch_cpu_idle+0x3c/0x5c
   default_idle_call+0x1c/0x38
   do_idle+0x100/0x280
   cpu_startup_entry+0x24/0x28
   secondary_start_kernel+0x15c/0x170
  SMP: stopping secondary CPUs

Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 .../hwtracing/coresight/coresight-replicator.c    | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index e7dc1c31d20d..f4eaa38f8f43 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -66,14 +66,16 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
 				     int inport, int outport)
 {
 	int rc = 0;
-	u32 reg;
+	u32 reg0, reg1;
 
 	switch (outport) {
 	case 0:
-		reg = REPLICATOR_IDFILTER0;
+		reg0 = REPLICATOR_IDFILTER0;
+		reg1 = REPLICATOR_IDFILTER1;
 		break;
 	case 1:
-		reg = REPLICATOR_IDFILTER1;
+		reg0 = REPLICATOR_IDFILTER1;
+		reg1 = REPLICATOR_IDFILTER0;
 		break;
 	default:
 		WARN_ON(1);
@@ -87,8 +89,11 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
 		rc = coresight_claim_device_unlocked(drvdata->base);
 
 	/* Ensure that the outport is enabled. */
-	if (!rc)
-		writel_relaxed(0x00, drvdata->base + reg);
+	if (!rc) {
+		writel_relaxed(0x00, drvdata->base + reg0);
+		writel_relaxed(0xff, drvdata->base + reg1);
+	}
+
 	CS_LOCK(drvdata->base);
 
 	return rc;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Mike Leach]

Hi,

The programmable replicator hardware by design enables trace through
both ports on reset. (see 1, section 4.4, 9.11)  The replicator driver
overrides this functionality to disable output, until the Coresight
infrastructure chooses a path from source to sink.
Now given that the hardware design is such that we must be able to
allow trace to be sent to both ports, a generic patch to prevent this
does not seem appropriate here.

I think this needs further investigation - to determine why this
appears to be failing in this particular instance.

Regards

Mike

[1] CoreSight System-on-Chip SoC-600 TRM, r3p2, 100806_0302_00_en

On Sun, 5 Apr 2020 at 11:28, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Since commit 30af4fb619e5 ("coresight: dynamic-replicator:
> Handle multiple connections"), we do not make sure that
> the other port is disabled when the dynamic replicator is
> enabled. This is seen to cause the CPU hardlockup atleast
> on SC7180 SoC when enabling ETM with ETR as the sink via
> sysfs.
>
>   Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
>   CPU: 7 PID: 0 Comm: swapper/7 Tainted: G S  B             5.4.25 #100
>   Hardware name: Qualcomm Technologies, Inc. SC7180 IDP (DT)
>   Call trace:
>    dump_backtrace+0x0/0x188
>    show_stack+0x20/0x2c
>    dump_stack+0xdc/0x144
>    panic+0x168/0x370
>    arch_seccomp_spec_mitigate+0x0/0x14
>    watchdog_timer_fn+0x68/0x290
>    __hrtimer_run_queues+0x264/0x498
>    hrtimer_interrupt+0xf0/0x22c
>    arch_timer_handler_phys+0x40/0x50
>    handle_percpu_devid_irq+0x8c/0x158
>    __handle_domain_irq+0x84/0xc4
>    gic_handle_irq+0x100/0x1c4
>    el1_irq+0xbc/0x180
>    arch_cpu_idle+0x3c/0x5c
>    default_idle_call+0x1c/0x38
>    do_idle+0x100/0x280
>    cpu_startup_entry+0x24/0x28
>    secondary_start_kernel+0x15c/0x170
>   SMP: stopping secondary CPUs
>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>  .../hwtracing/coresight/coresight-replicator.c    | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index e7dc1c31d20d..f4eaa38f8f43 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -66,14 +66,16 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
>                                      int inport, int outport)
>  {
>         int rc = 0;
> -       u32 reg;
> +       u32 reg0, reg1;
>
>         switch (outport) {
>         case 0:
> -               reg = REPLICATOR_IDFILTER0;
> +               reg0 = REPLICATOR_IDFILTER0;
> +               reg1 = REPLICATOR_IDFILTER1;
>                 break;
>         case 1:
> -               reg = REPLICATOR_IDFILTER1;
> +               reg0 = REPLICATOR_IDFILTER1;
> +               reg1 = REPLICATOR_IDFILTER0;
>                 break;
>         default:
>                 WARN_ON(1);
> @@ -87,8 +89,11 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
>                 rc = coresight_claim_device_unlocked(drvdata->base);
>
>         /* Ensure that the outport is enabled. */
> -       if (!rc)
> -               writel_relaxed(0x00, drvdata->base + reg);
> +       if (!rc) {
> +               writel_relaxed(0x00, drvdata->base + reg0);
> +               writel_relaxed(0xff, drvdata->base + reg1);
> +       }
> +
>         CS_LOCK(drvdata->base);
>
>         return rc;
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

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

[Sai Prakash Ranjan]

Hi Mike,

Thanks for taking a look.

On 2020-04-06 16:25, Mike Leach wrote:
> Hi,
> 
> The programmable replicator hardware by design enables trace through
> both ports on reset. (see 1, section 4.4, 9.11)  The replicator driver
> overrides this functionality to disable output, until the Coresight
> infrastructure chooses a path from source to sink.
> Now given that the hardware design is such that we must be able to
> allow trace to be sent to both ports, a generic patch to prevent this
> does not seem appropriate here.
> 
> I think this needs further investigation - to determine why this
> appears to be failing in this particular instance.
> 

Yes, this probably needs further investigation, but CPU hardlock stack
trace doesnt help much. I could always trigger this hard lockup without
this patch on SC7180 SoC and this is only seen when ETR is used as the 
sink.

The only difference I could see between non working case (on SC7180 [1]) 
and
the working case (on SDM845 [2]) is the path from source to sink.

SC7180 source to sink path(Not working):
----------------------------------------

       etm0_out
	 |
   apss_funnel_in0
          |
  apss_merge_funnel_in
          |
      funnel1_in4
	 |
   merge_funnel_in1
	 |
    swao_funnel_in
          |
        etf_in
	 |
  swao_replicator_in
          |
   replicator_in
	 |
        etr_in


SDM845 source to sink path(Working):
------------------------------------

       etm0_out
          |
    apss_funnel_in0
          |
  apss_merge_funnel_in
          |
     funnel2_in5
	 |
   merge_funnel_in2
          |
       etf_in
	 |
   replicator_in
	 |
        etr_in


[1] - https://lore.kernel.org/patchwork/patch/1212946/
[2] - 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845.dtsi?h=v5.6#n1910


Thanks,
Sai

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

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

[Suzuki K Poulose]

On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
> Hi Mike,
> 
> Thanks for taking a look.
> 
> On 2020-04-06 16:25, Mike Leach wrote:
>> Hi,
>>
>> The programmable replicator hardware by design enables trace through
>> both ports on reset. (see 1, section 4.4, 9.11)  The replicator driver
>> overrides this functionality to disable output, until the Coresight
>> infrastructure chooses a path from source to sink.
>> Now given that the hardware design is such that we must be able to
>> allow trace to be sent to both ports, a generic patch to prevent this
>> does not seem appropriate here.
>>
>> I think this needs further investigation - to determine why this
>> appears to be failing in this particular instance.
>>
> 
> Yes, this probably needs further investigation, but CPU hardlock stack
> trace doesnt help much. I could always trigger this hard lockup without
> this patch on SC7180 SoC and this is only seen when ETR is used as the 
> sink.
> 
> The only difference I could see between non working case (on SC7180 [1]) 
> and
> the working case (on SDM845 [2]) is the path from source to sink.


> 
> SC7180 source to sink path(Not working):
> ----------------------------------------
> 
>        etm0_out
>       |
>    apss_funnel_in0
>           |
>   apss_merge_funnel_in
>           |
>       funnel1_in4
>       |
>    merge_funnel_in1
>       |
>     swao_funnel_in
>           |
>         etf_in
>       |
>   swao_replicator_in
>           |
>    replicator_in
>       |
>         etr_in


There seems to be two replicators back to back here. What is connected
to the other output of both of them ? Are there any TPIUs ? What happens
if you choose a sink on the other end of "swao_replicator" (ETB ?)

After boot, what do the idfilter registers read for both the replicators ?


I believe we need to properly assign the TRACE_IDs for tracing sessions,
(rather than static ids) in a way such that we could filter them and use
the multiple sinks in parallel for separate trace sessions and this is
not simple (involves kernel driver changes and the perf tool to be able
to decode the trace id changes too).


So for the moment, we need to :

1) Disallow turning the replicator ON, when it is already turned ON
2) Do what your patch does. i.e, disable the other end while one end
    is turned on.

Thoughts ?

Kind regards
Suzuki

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

[Sai Prakash Ranjan]

Hi Suzuki,

Thanks for looking into this issue.

On 2020-04-07 15:54, Suzuki K Poulose wrote:
> On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
> 
> There seems to be two replicators back to back here. What is connected
> to the other output of both of them ? Are there any TPIUs ? What 
> happens
> if you choose a sink on the other end of "swao_replicator" (ETB ?)
> 

The other outport of swao replicator is connected to EUD which is a
QCOM specific HW which can be used as a sink like USB.
And the other outport of other replicator(replicator_out) is connected 
to
TPIU.

> After boot, what do the idfilter registers read for both the 
> replicators ?
> 

Added some prints in replicator_probe.

  replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
idfilter1=0x0
  replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
idfilter1=0xff
  replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
idfilter1=0xff

> 
> I believe we need to properly assign the TRACE_IDs for tracing 
> sessions,
> (rather than static ids) in a way such that we could filter them and 
> use
> the multiple sinks in parallel for separate trace sessions and this is
> not simple (involves kernel driver changes and the perf tool to be able
> to decode the trace id changes too).
> 
> 
> So for the moment, we need to :
> 
> 1) Disallow turning the replicator ON, when it is already turned ON
> 2) Do what your patch does. i.e, disable the other end while one end
>    is turned on.
> 
> Thoughts ?
> 

Sounds good to me, Mike would have some comments.

Thanks,
Sai

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

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

[Suzuki K Poulose]

On 04/07/2020 12:29 PM, Sai Prakash Ranjan wrote:
> Hi Suzuki,
> 
> Thanks for looking into this issue.
> 
> On 2020-04-07 15:54, Suzuki K Poulose wrote:
>> On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
>>
>> There seems to be two replicators back to back here. What is connected
>> to the other output of both of them ? Are there any TPIUs ? What happens
>> if you choose a sink on the other end of "swao_replicator" (ETB ?)
>>
> 
> The other outport of swao replicator is connected to EUD which is a
> QCOM specific HW which can be used as a sink like USB.
> And the other outport of other replicator(replicator_out) is connected to
> TPIU.
> 
>> After boot, what do the idfilter registers read for both the 
>> replicators ?
>>
> 
> Added some prints in replicator_probe.
> 
>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
> idfilter1=0x0
>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
> idfilter1=0xff
>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
> idfilter1=0xff

Curious to see how the idfilterX is set to 0:
	 if that is never used.
		Or
	 if the user doesn't reset it back to 0xff.

Does your test ever touch EUD (enable the port for EUD at
swao-replicator) ? What are the values before you run your test ?


Suzuki

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

[Sai Prakash Ranjan]

Hi Suzuki,

On 2020-04-07 18:38, Suzuki K Poulose wrote:
> On 04/07/2020 12:29 PM, Sai Prakash Ranjan wrote:
>> Hi Suzuki,
>> 
>> Thanks for looking into this issue.
>> 
>> On 2020-04-07 15:54, Suzuki K Poulose wrote:
>>> On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
>>> 
>>> There seems to be two replicators back to back here. What is 
>>> connected
>>> to the other output of both of them ? Are there any TPIUs ? What 
>>> happens
>>> if you choose a sink on the other end of "swao_replicator" (ETB ?)
>>> 
>> 
>> The other outport of swao replicator is connected to EUD which is a
>> QCOM specific HW which can be used as a sink like USB.
>> And the other outport of other replicator(replicator_out) is connected 
>> to
>> TPIU.
>> 
>>> After boot, what do the idfilter registers read for both the 
>>> replicators ?
>>> 
>> 
>> Added some prints in replicator_probe.
>> 
>>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
>> idfilter1=0x0
>>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
>> idfilter1=0xff
>>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
>> idfilter1=0xff
> 
> Curious to see how the idfilterX is set to 0:
> 	 if that is never used.
> 		Or
> 	 if the user doesn't reset it back to 0xff.
> 

For both replicators, the default value seems to be 0x0.

  replicator probe in res ret=0 devname=6046000.replicator idfilter0=0x0 
idfilter1=0x0
  replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
idfilter1=0x0
  replicator probe in res ret=0 devname=6b06000.replicator idfilter0=0x0 
idfilter1=0x0
  replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
idfilter1=0xff
  replicator probe in res ret=0 devname=6046000.replicator idfilter0=0x0 
idfilter1=0x0
  replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
idfilter1=0xff

> Does your test ever touch EUD (enable the port for EUD at
> swao-replicator) ? What are the values before you run your test ?
> 
> 

No, we do not use EUD, downstream it is used as dummy sink.
And I just try to select the ETR as the sink and enable ETM0 as the 
trace source.

echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
echo 1 > /sys/bus/coresight/devices/etm0/enable_source

Also I see the KASAN warning but that seems like some other issue.

[  526.110401] 
==================================================================
[  526.117988] BUG: KASAN: slab-out-of-bounds in 
funnel_enable+0x54/0x1b0
[  526.124706] Read of size 4 at addr ffffff8135f9549c by task bash/1114
[  526.131324]
[  526.132886] CPU: 3 PID: 1114 Comm: bash Tainted: G S                
5.4.25 #232
[  526.140397] Hardware name: Qualcomm Technologies, Inc. SC7180 IDP 
(DT)
[  526.147113] Call trace:
[  526.149653]  dump_backtrace+0x0/0x188
[  526.153431]  show_stack+0x20/0x2c
[  526.156852]  dump_stack+0xdc/0x144
[  526.160370]  print_address_description+0x3c/0x494
[  526.165211]  __kasan_report+0x144/0x168
[  526.169170]  kasan_report+0x10/0x18
[  526.172769]  check_memory_region+0x1a4/0x1b4
[  526.177164]  __kasan_check_read+0x18/0x24
[  526.181292]  funnel_enable+0x54/0x1b0
[  526.185072]  coresight_enable_path+0x104/0x198
[  526.189649]  coresight_enable+0x118/0x26c
[  526.193778]  enable_source_store+0x64/0xa8
[  526.198007]  dev_attr_store+0x40/0x58
[  526.201788]  sysfs_kf_write+0x4c/0x64
[  526.205567]  kernfs_fop_write+0x16c/0x210
[  526.209700]  __vfs_write+0x54/0x1a8
[  526.213297]  vfs_write+0xe4/0x1a4
[  526.216714]  ksys_write+0x84/0xec
[  526.220131]  __arm64_sys_write+0x20/0x2c
[  526.224179]  el0_svc_common+0xa8/0x160
[  526.228040]  el0_svc_compat_handler+0x2c/0x38
[  526.232533]  el0_svc_compat+0x8/0x10
[  526.236225]
[  526.237782] Allocated by task 280:
[  526.241298]  __kasan_kmalloc+0xf0/0x1ac
[  526.245249]  kasan_kmalloc+0xc/0x14
[  526.248849]  __kmalloc+0x28c/0x3b4
[  526.252361]  coresight_register+0x88/0x250
[  526.256587]  funnel_probe+0x15c/0x228
[  526.260365]  dynamic_funnel_probe+0x20/0x2c
[  526.264679]  amba_probe+0xbc/0x158
[  526.268193]  really_probe+0x144/0x408
[  526.271970]  driver_probe_device+0x70/0x140
[  526.276282]  __device_attach_driver+0x9c/0x110
[  526.280861]  bus_for_each_drv+0x90/0xd8
[  526.284822]  __device_attach+0xb4/0x164
[  526.288772]  device_initial_probe+0x20/0x2c
[  526.293081]  bus_probe_device+0x34/0x94
[  526.297030]  deferred_probe_work_func+0xa4/0x100
[  526.301794]  process_one_work+0x33c/0x640
[  526.305922]  worker_thread+0x2a0/0x470
[  526.309786]  kthread+0x128/0x138
[  526.313119]  ret_from_fork+0x10/0x18
[  526.316810]
[  526.318364] Freed by task 0:
[  526.321344] (stack is not available)
[  526.325024]
[  526.326580] The buggy address belongs to the object at 
ffffff8135f95480
[  526.326580]  which belongs to the cache kmalloc-128 of size 128
[  526.339439] The buggy address is located 28 bytes inside of
[  526.339439]  128-byte region [ffffff8135f95480, ffffff8135f95500)
[  526.351399] The buggy address belongs to the page:
[  526.356342] page:ffffffff04b7e500 refcount:1 mapcount:0 
mapping:ffffff814b00c380 index:0x0 compound_mapcount: 0
[  526.366711] flags: 0x4000000000010200(slab|head)
[  526.371475] raw: 4000000000010200 ffffffff05034008 ffffffff0501eb08 
ffffff814b00c380
[  526.379435] raw: 0000000000000000 0000000000190019 00000001ffffffff 
0000000000000000
[  526.387393] page dumped because: kasan: bad access detected
[  526.393128]
[  526.394681] Memory state around the buggy address:
[  526.399619]  ffffff8135f95380: fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc
[  526.407046]  ffffff8135f95400: fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc
[  526.414473] >ffffff8135f95480: 04 fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc
[  526.421900]                             ^
[  526.426029]  ffffff8135f95500: fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc
[  526.433456]  ffffff8135f95580: fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc
[  526.440883] 
==================================================================

Thanks,
Sai

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

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

[Suzuki K Poulose]

On 04/07/2020 02:56 PM, Sai Prakash Ranjan wrote:
> Hi Suzuki,
> 
> On 2020-04-07 18:38, Suzuki K Poulose wrote:
>> On 04/07/2020 12:29 PM, Sai Prakash Ranjan wrote:
>>> Hi Suzuki,
>>>
>>> Thanks for looking into this issue.
>>>
>>> On 2020-04-07 15:54, Suzuki K Poulose wrote:
>>>> On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
>>>>
>>>> There seems to be two replicators back to back here. What is connected
>>>> to the other output of both of them ? Are there any TPIUs ? What 
>>>> happens
>>>> if you choose a sink on the other end of "swao_replicator" (ETB ?)
>>>>
>>>
>>> The other outport of swao replicator is connected to EUD which is a
>>> QCOM specific HW which can be used as a sink like USB.
>>> And the other outport of other replicator(replicator_out) is 
>>> connected to
>>> TPIU.
>>>
>>>> After boot, what do the idfilter registers read for both the 
>>>> replicators ?
>>>>
>>>
>>> Added some prints in replicator_probe.
>>>
>>>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
>>> idfilter1=0x0
>>>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
>>> idfilter1=0xff
>>>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
>>> idfilter1=0xff
>>
>> Curious to see how the idfilterX is set to 0:
>>      if that is never used.
>>         Or
>>      if the user doesn't reset it back to 0xff.
>>
> 
> For both replicators, the default value seems to be 0x0.
> 
>   replicator probe in res ret=0 devname=6046000.replicator idfilter0=0x0 
> idfilter1=0x0
>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
> idfilter1=0x0
>   replicator probe in res ret=0 devname=6b06000.replicator idfilter0=0x0 
> idfilter1=0x0
>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
> idfilter1=0xff
>   replicator probe in res ret=0 devname=6046000.replicator idfilter0=0x0 
> idfilter1=0x0
>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
> idfilter1=0xff

I am not sure how you have added the debugs, but it looks like the
drivers set 0xff for both the port filters on a successful probe.

> 
>> Does your test ever touch EUD (enable the port for EUD at
>> swao-replicator) ? What are the values before you run your test ?
>>
>>
> 
> No, we do not use EUD, downstream it is used as dummy sink.
> And I just try to select the ETR as the sink and enable ETM0 as the 
> trace source.
> 
> echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
> echo 1 > /sys/bus/coresight/devices/etm0/enable_source
> 
> Also I see the KASAN warning but that seems like some other issue.
> 

Does your funnel have sparse input described ? I think we have an
issue with the "refcnt" tracking for funnels (especially). When we
have a sparse input ports described (ie. if only input ports 0, 3,
5 are described to protect the secure side connections), we could
end up accessing beyond the memory allocated for csdev->refcnts.
i.e, csdev->pdata->nr_inport = 3, and we could access csdev->refcnts[5],
while sizeof(csdev->refcnts) = sizeof(atomic_t) * 3.

I will send a patch.


> [  526.110401] 
> ==================================================================
> [  526.117988] BUG: KASAN: slab-out-of-bounds in funnel_enable+0x54/0x1b0
> [  526.124706] Read of size 4 at addr ffffff8135f9549c by task bash/1114
> [  526.131324]
> [  526.132886] CPU: 3 PID: 1114 Comm: bash Tainted: G S 5.4.25 #232
> [  526.140397] Hardware name: Qualcomm Technologies, Inc. SC7180 IDP (DT)
> [  526.147113] Call trace:
> [  526.149653]  dump_backtrace+0x0/0x188
> [  526.153431]  show_stack+0x20/0x2c
> [  526.156852]  dump_stack+0xdc/0x144
> [  526.160370]  print_address_description+0x3c/0x494
> [  526.165211]  __kasan_report+0x144/0x168
> [  526.169170]  kasan_report+0x10/0x18
> [  526.172769]  check_memory_region+0x1a4/0x1b4
> [  526.177164]  __kasan_check_read+0x18/0x24
> [  526.181292]  funnel_enable+0x54/0x1b0
> [  526.185072]  coresight_enable_path+0x104/0x198
> [  526.189649]  coresight_enable+0x118/0x26c
> [  526.193778]  enable_source_store+0x64/0xa8
> [  526.198007]  dev_attr_store+0x40/0x58
> [  526.201788]  sysfs_kf_write+0x4c/0x64
> [  526.205567]  kernfs_fop_write+0x16c/0x210
> [  526.209700]  __vfs_write+0x54/0x1a8
> [  526.213297]  vfs_write+0xe4/0x1a4
> [  526.216714]  ksys_write+0x84/0xec
> [  526.220131]  __arm64_sys_write+0x20/0x2c
> [  526.224179]  el0_svc_common+0xa8/0x160
> [  526.228040]  el0_svc_compat_handler+0x2c/0x38
> [  526.232533]  el0_svc_compat+0x8/0x10
> [  526.236225]
> [  526.237782] Allocated by task 280:
> [  526.241298]  __kasan_kmalloc+0xf0/0x1ac
> [  526.245249]  kasan_kmalloc+0xc/0x14
> [  526.248849]  __kmalloc+0x28c/0x3b4
> [  526.252361]  coresight_register+0x88/0x250
> [  526.256587]  funnel_probe+0x15c/0x228
> [  526.260365]  dynamic_funnel_probe+0x20/0x2c
> [  526.264679]  amba_probe+0xbc/0x158
> [  526.268193]  really_probe+0x144/0x408
> [  526.271970]  driver_probe_device+0x70/0x140
> [  526.276282]  __device_attach_driver+0x9c/0x110
> [  526.280861]  bus_for_each_drv+0x90/0xd8
> [  526.284822]  __device_attach+0xb4/0x164
> [  526.288772]  device_initial_probe+0x20/0x2c
> [  526.293081]  bus_probe_device+0x34/0x94
> [  526.297030]  deferred_probe_work_func+0xa4/0x100
> [  526.301794]  process_one_work+0x33c/0x640
> [  526.305922]  worker_thread+0x2a0/0x470
> [  526.309786]  kthread+0x128/0x138
> [  526.313119]  ret_from_fork+0x10/0x18
> [  526.316810]
> [  526.318364] Freed by task 0:
> [  526.321344] (stack is not available)
> [  526.325024]
> [  526.326580] The buggy address belongs to the object at ffffff8135f95480
> [  526.326580]  which belongs to the cache kmalloc-128 of size 128
> [  526.339439] The buggy address is located 28 bytes inside of
> [  526.339439]  128-byte region [ffffff8135f95480, ffffff8135f95500)
> [  526.351399] The buggy address belongs to the page:
> [  526.356342] page:ffffffff04b7e500 refcount:1 mapcount:0 
> mapping:ffffff814b00c380 index:0x0 compound_mapcount: 0
> [  526.366711] flags: 0x4000000000010200(slab|head)
> [  526.371475] raw: 4000000000010200 ffffffff05034008 ffffffff0501eb08 
> ffffff814b00c380
> [  526.379435] raw: 0000000000000000 0000000000190019 00000001ffffffff 
> 0000000000000000
> [  526.387393] page dumped because: kasan: bad access detected
> [  526.393128]
> [  526.394681] Memory state around the buggy address:
> [  526.399619]  ffffff8135f95380: fc fc fc fc fc fc fc fc fc fc fc fc fc 
> fc fc fc
> [  526.407046]  ffffff8135f95400: fc fc fc fc fc fc fc fc fc fc fc fc fc 
> fc fc fc
> [  526.414473] >ffffff8135f95480: 04 fc fc fc fc fc fc fc fc fc fc fc fc 
> fc fc fc
> [  526.421900]                             ^
> [  526.426029]  ffffff8135f95500: fc fc fc fc fc fc fc fc fc fc fc fc fc 
> fc fc fc
> [  526.433456]  ffffff8135f95580: fc fc fc fc fc fc fc fc fc fc fc fc fc 
> fc fc fc
> [  526.440883] 
> ==================================================================
> 
> Thanks,
> Sai
> 

Suzuki

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

[Sai Prakash Ranjan]

Hi Suzuki,

On 2020-04-07 20:23, Suzuki K Poulose wrote:
> On 04/07/2020 02:56 PM, Sai Prakash Ranjan wrote:
>> Hi Suzuki,
>> 
>> On 2020-04-07 18:38, Suzuki K Poulose wrote:
>>> On 04/07/2020 12:29 PM, Sai Prakash Ranjan wrote:
>>>> Hi Suzuki,
>>>> 
>>>> Thanks for looking into this issue.
>>>> 
>>>> On 2020-04-07 15:54, Suzuki K Poulose wrote:
>>>>> On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
>>>>> 
>>>>> There seems to be two replicators back to back here. What is 
>>>>> connected
>>>>> to the other output of both of them ? Are there any TPIUs ? What 
>>>>> happens
>>>>> if you choose a sink on the other end of "swao_replicator" (ETB ?)
>>>>> 
>>>> 
>>>> The other outport of swao replicator is connected to EUD which is a
>>>> QCOM specific HW which can be used as a sink like USB.
>>>> And the other outport of other replicator(replicator_out) is 
>>>> connected to
>>>> TPIU.
>>>> 
>>>>> After boot, what do the idfilter registers read for both the 
>>>>> replicators ?
>>>>> 
>>>> 
>>>> Added some prints in replicator_probe.
>>>> 
>>>>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
>>>> idfilter1=0x0
>>>>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
>>>> idfilter1=0xff
>>>>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
>>>> idfilter1=0xff
>>> 
>>> Curious to see how the idfilterX is set to 0:
>>>      if that is never used.
>>>         Or
>>>      if the user doesn't reset it back to 0xff.
>>> 
>> 
>> For both replicators, the default value seems to be 0x0.
>> 
>>   replicator probe in res ret=0 devname=6046000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
>> idfilter1=0x0
>>   replicator probe in res ret=0 devname=6b06000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
>> idfilter1=0xff
>>   replicator probe in res ret=0 devname=6046000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
>> idfilter1=0xff
> 
> I am not sure how you have added the debugs, but it looks like the
> drivers set 0xff for both the port filters on a successful probe.
> 

Yes, thats done by replicator_reset in probe right? Below is the diff:

@@ -242,6 +244,9 @@ static int replicator_probe(struct device *dev, 
struct resource *res)
                 }
                 drvdata->base = base;
                 desc.groups = replicator_groups;
+               pr_info("replicator probe in res ret=%d devname=%s 
idfilter0=%#lx idfilter1=%#lx\n",
+                       ret, dev_name(dev), (readl_relaxed(base + 
REPLICATOR_IDFILTER0)),
+                       (readl_relaxed(base + REPLICATOR_IDFILTER1)));
         }

         dev_set_drvdata(dev, drvdata);
@@ -272,6 +277,12 @@ static int replicator_probe(struct device *dev, 
struct resource *res)
  out_disable_clk:
         if (ret && !IS_ERR_OR_NULL(drvdata->atclk))
                 clk_disable_unprepare(drvdata->atclk);
+
+       if (res)
+               pr_info("replicator probe ret=%d devname=%s 
idfilter0=%#lx idfilter1=%#lx\n",
+                       ret, dev_name(dev), (readl_relaxed(base + 
REPLICATOR_IDFILTER0)),
+                       (readl_relaxed(base + REPLICATOR_IDFILTER1)));
+
         return ret;
  }

>> 
>>> Does your test ever touch EUD (enable the port for EUD at
>>> swao-replicator) ? What are the values before you run your test ?
>>> 
>>> 
>> 
>> No, we do not use EUD, downstream it is used as dummy sink.
>> And I just try to select the ETR as the sink and enable ETM0 as the 
>> trace source.
>> 
>> echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
>> echo 1 > /sys/bus/coresight/devices/etm0/enable_source
>> 
>> Also I see the KASAN warning but that seems like some other issue.
>> 
> 
> Does your funnel have sparse input described ? I think we have an
> issue with the "refcnt" tracking for funnels (especially). When we
> have a sparse input ports described (ie. if only input ports 0, 3,
> 5 are described to protect the secure side connections), we could
> end up accessing beyond the memory allocated for csdev->refcnts.
> i.e, csdev->pdata->nr_inport = 3, and we could access 
> csdev->refcnts[5],
> while sizeof(csdev->refcnts) = sizeof(atomic_t) * 3.
> 
> I will send a patch.
> 

Thanks, I can test it out.

Thanks,
Sai

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

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

[Suzuki K Poulose]

On Tue, Apr 07, 2020 at 08:48:54PM +0530, Sai Prakash Ranjan wrote:
> Hi Suzuki,
> 
> On 2020-04-07 20:23, Suzuki K Poulose wrote:
> > On 04/07/2020 02:56 PM, Sai Prakash Ranjan wrote:
> > > Hi Suzuki,
> > > 
> > > On 2020-04-07 18:38, Suzuki K Poulose wrote:
> > > > On 04/07/2020 12:29 PM, Sai Prakash Ranjan wrote:
> > > > > Hi Suzuki,
> > > > > 
> > > > > Thanks for looking into this issue.
> > > > > 
> > > > > On 2020-04-07 15:54, Suzuki K Poulose wrote:
> > > > > > On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
> > > > > > 
> > > > > > There seems to be two replicators back to back here.
> > > > > > What is connected
> > > > > > to the other output of both of them ? Are there any
> > > > > > TPIUs ? What happens
> > > > > > if you choose a sink on the other end of "swao_replicator" (ETB ?)
> > > > > > 
> > > > > 
> > > > > The other outport of swao replicator is connected to EUD which is a
> > > > > QCOM specific HW which can be used as a sink like USB.
> > > > > And the other outport of other replicator(replicator_out) is
> > > > > connected to
> > > > > TPIU.
> > > > > 
> > > > > > After boot, what do the idfilter registers read for both
> > > > > > the replicators ?
> > > > > > 
> > > > > 
> > > > > Added some prints in replicator_probe.
> > > > > 
> > > > >   replicator probe ret=-517 devname=6046000.replicator
> > > > > idfilter0=0x0 idfilter1=0x0
> > > > >   replicator probe ret=0 devname=6b06000.replicator
> > > > > idfilter0=0xff idfilter1=0xff
> > > > >   replicator probe ret=0 devname=6046000.replicator
> > > > > idfilter0=0xff idfilter1=0xff
> > > > 
> > > > Curious to see how the idfilterX is set to 0:
> > > >      if that is never used.
> > > >         Or
> > > >      if the user doesn't reset it back to 0xff.
> > > > 
> > > 
> > > For both replicators, the default value seems to be 0x0.
> > > 
> > >   replicator probe in res ret=0 devname=6046000.replicator
> > > idfilter0=0x0 idfilter1=0x0
> > >   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0
> > > idfilter1=0x0
> > >   replicator probe in res ret=0 devname=6b06000.replicator
> > > idfilter0=0x0 idfilter1=0x0
> > >   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff
> > > idfilter1=0xff
> > >   replicator probe in res ret=0 devname=6046000.replicator
> > > idfilter0=0x0 idfilter1=0x0
> > >   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff
> > > idfilter1=0xff
> > 
> > I am not sure how you have added the debugs, but it looks like the
> > drivers set 0xff for both the port filters on a successful probe.
> > 
> 
> Yes, thats done by replicator_reset in probe right? Below is the diff:
> 
> @@ -242,6 +244,9 @@ static int replicator_probe(struct device *dev, struct
> resource *res)
>                 }
>                 drvdata->base = base;
>                 desc.groups = replicator_groups;
> +               pr_info("replicator probe in res ret=%d devname=%s
> idfilter0=%#lx idfilter1=%#lx\n",
> +                       ret, dev_name(dev), (readl_relaxed(base +
> REPLICATOR_IDFILTER0)),
> +                       (readl_relaxed(base + REPLICATOR_IDFILTER1)));
>         }
> 
>         dev_set_drvdata(dev, drvdata);
> @@ -272,6 +277,12 @@ static int replicator_probe(struct device *dev, struct
> resource *res)
>  out_disable_clk:
>         if (ret && !IS_ERR_OR_NULL(drvdata->atclk))
>                 clk_disable_unprepare(drvdata->atclk);
> +
> +       if (res)
> +               pr_info("replicator probe ret=%d devname=%s idfilter0=%#lx
> idfilter1=%#lx\n",
> +                       ret, dev_name(dev), (readl_relaxed(base +
> REPLICATOR_IDFILTER0)),
> +                       (readl_relaxed(base + REPLICATOR_IDFILTER1)));
> +
>         return ret;
>  }
> 
> > > 
> > > > Does your test ever touch EUD (enable the port for EUD at
> > > > swao-replicator) ? What are the values before you run your test ?
> > > > 
> > > > 
> > > 
> > > No, we do not use EUD, downstream it is used as dummy sink.
> > > And I just try to select the ETR as the sink and enable ETM0 as the
> > > trace source.
> > > 
> > > echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
> > > echo 1 > /sys/bus/coresight/devices/etm0/enable_source
> > > 
> > > Also I see the KASAN warning but that seems like some other issue.
> > > 
> > 
> > Does your funnel have sparse input described ? I think we have an
> > issue with the "refcnt" tracking for funnels (especially). When we
> > have a sparse input ports described (ie. if only input ports 0, 3,
> > 5 are described to protect the secure side connections), we could
> > end up accessing beyond the memory allocated for csdev->refcnts.
> > i.e, csdev->pdata->nr_inport = 3, and we could access csdev->refcnts[5],
> > while sizeof(csdev->refcnts) = sizeof(atomic_t) * 3.
> > 
> > I will send a patch.
> > 
> 
> Thanks, I can test it out.

Please find the untested patch below.

---8>---

[untested] coresight: Fix support for sparse port numbers

On some systems the firmware may not describe all the ports
connected to a component (e.g, for security reasons). This
could be especially problematic for "funnels" where we could
end up in modifying memory beyond the allocated space for
refcounts.

e.g, for a funnel with input ports listed 0, 3, 5, nr_inport = 3.
However the we could access refcnts[5] while checking for
references.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 .../hwtracing/coresight/coresight-platform.c  | 74 ++++++++++++-------
 drivers/hwtracing/coresight/coresight.c       |  8 +-
 2 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 3c5bee429105..1c610d6e944b 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -67,6 +67,7 @@ static void of_coresight_get_ports_legacy(const struct device_node *node,
 					  int *nr_inport, int *nr_outport)
 {
 	struct device_node *ep = NULL;
+	struct of_endpoint endpoint;
 	int in = 0, out = 0;
 
 	do {
@@ -74,10 +75,16 @@ static void of_coresight_get_ports_legacy(const struct device_node *node,
 		if (!ep)
 			break;
 
-		if (of_coresight_legacy_ep_is_input(ep))
-			in++;
-		else
-			out++;
+		if (of_graph_parse_endpoint(ep, &endpoint))
+			continue;
+
+		if (of_coresight_legacy_ep_is_input(ep)) {
+			in = (endpoint.port + 1 > in) ?
+				endpoint.port + 1 : in;
+		} else {
+			out = (endpoint.port + 1) > out ?
+				endpoint.port + 1 : out;
+		}
 
 	} while (ep);
 
@@ -117,9 +124,16 @@ of_coresight_count_ports(struct device_node *port_parent)
 {
 	int i = 0;
 	struct device_node *ep = NULL;
+	struct of_endpoint endpoint;
+
+	while ((ep = of_graph_get_next_endpoint(port_parent, ep))) {
+		/* Defer error handling to parsing */
+		if (of_graph_parse_endpoint(ep, &endpoint))
+			continue;
+		if (endpoint.port + 1 > i)
+			i = endpoint.port + 1;
+	}
 
-	while ((ep = of_graph_get_next_endpoint(port_parent, ep)))
-		i++;
 	return i;
 }
 
@@ -171,14 +185,12 @@ static int of_coresight_get_cpu(struct device *dev)
  * Parses the local port, remote device name and the remote port.
  *
  * Returns :
- *	 1	- If the parsing is successful and a connection record
- *		  was created for an output connection.
  *	 0	- If the parsing completed without any fatal errors.
  *	-Errno	- Fatal error, abort the scanning.
  */
 static int of_coresight_parse_endpoint(struct device *dev,
 				       struct device_node *ep,
-				       struct coresight_connection *conn)
+				       struct coresight_platform_data *pdata)
 {
 	int ret = 0;
 	struct of_endpoint endpoint, rendpoint;
@@ -186,6 +198,7 @@ static int of_coresight_parse_endpoint(struct device *dev,
 	struct device_node *rep = NULL;
 	struct device *rdev = NULL;
 	struct fwnode_handle *rdev_fwnode;
+	struct coresight_connection *conn;
 
 	do {
 		/* Parse the local port details */
@@ -212,6 +225,12 @@ static int of_coresight_parse_endpoint(struct device *dev,
 			break;
 		}
 
+		conn = &pdata->conns[endpoint.port];
+		if (conn->child_fwnode) {
+			dev_warn(dev, "Duplicate output port %d\n", endpoint.port);
+			ret = -EINVAL;
+			break;
+		}
 		conn->outport = endpoint.port;
 		/*
 		 * Hold the refcount to the target device. This could be
@@ -224,7 +243,6 @@ static int of_coresight_parse_endpoint(struct device *dev,
 		conn->child_fwnode = fwnode_handle_get(rdev_fwnode);
 		conn->child_port = rendpoint.port;
 		/* Connection record updated */
-		ret = 1;
 	} while (0);
 
 	of_node_put(rparent);
@@ -238,7 +256,6 @@ static int of_get_coresight_platform_data(struct device *dev,
 					  struct coresight_platform_data *pdata)
 {
 	int ret = 0;
-	struct coresight_connection *conn;
 	struct device_node *ep = NULL;
 	const struct device_node *parent = NULL;
 	bool legacy_binding = false;
@@ -267,8 +284,6 @@ static int of_get_coresight_platform_data(struct device *dev,
 		dev_warn_once(dev, "Uses obsolete Coresight DT bindings\n");
 	}
 
-	conn = pdata->conns;
-
 	/* Iterate through each output port to discover topology */
 	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
 		/*
@@ -280,15 +295,9 @@ static int of_get_coresight_platform_data(struct device *dev,
 		if (legacy_binding && of_coresight_legacy_ep_is_input(ep))
 			continue;
 
-		ret = of_coresight_parse_endpoint(dev, ep, conn);
-		switch (ret) {
-		case 1:
-			conn++;		/* Fall through */
-		case 0:
-			break;
-		default:
+		ret = of_coresight_parse_endpoint(dev, ep, pdata);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return 0;
@@ -627,6 +636,11 @@ static int acpi_coresight_parse_link(struct acpi_device *adev,
 		 *    coresight_remove_match().
 		 */
 		conn->child_fwnode = fwnode_handle_get(&r_adev->fwnode);
+	} else if (dir == ACPI_CORESIGHT_LINK_SLAVE) {
+		conn->child_port = fields[0].integer.value;
+	} else {
+		/* Invalid direction */
+		return -EINVAL;
 	}
 
 	return dir;
@@ -672,10 +686,14 @@ static int acpi_coresight_parse_graph(struct acpi_device *adev,
 			return dir;
 
 		if (dir == ACPI_CORESIGHT_LINK_MASTER) {
-			pdata->nr_outport++;
+			if (ptr->outport > pdata->nr_outport)
+				pdata->nr_outport = ptr->outport;
 			ptr++;
 		} else {
-			pdata->nr_inport++;
+			WARN_ON(pdata->nr_inport == ptr->child_port);
+			/* Do not move the ptr for input connections */
+			if (ptr->child_port > pdata->nr_inport)
+				pdata->nr_inport = ptr->child_port;
 		}
 	}
 
@@ -684,8 +702,13 @@ static int acpi_coresight_parse_graph(struct acpi_device *adev,
 		return rc;
 
 	/* Copy the connection information to the final location */
-	for (i = 0; i < pdata->nr_outport; i++)
-		pdata->conns[i] = conns[i];
+	for (i = 0; conns + i < ptr; i++) {
+		int port = conns[i].outport;
+
+		/* Duplicate output port */
+		WARN_ON(pdata->conns[port].child_fwnode);
+		pdata->conns[port] = conns[i];
+	}
 
 	devm_kfree(&adev->dev, conns);
 	return 0;
@@ -787,6 +810,7 @@ coresight_get_platform_data(struct device *dev)
 		goto error;
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	pdata->nr_outport = pdata->nr_inport = -1;
 	if (!pdata) {
 		ret = -ENOMEM;
 		goto error;
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index ef20f74c85fa..f07bc0a7ab88 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -990,6 +990,9 @@ static int coresight_orphan_match(struct device *dev, void *data)
 	for (i = 0; i < i_csdev->pdata->nr_outport; i++) {
 		conn = &i_csdev->pdata->conns[i];
 
+		/* Skip the port if FW doesn't describe it */
+		if (!conn->child_fwnode)
+			continue;
 		/* We have found at least one orphan connection */
 		if (conn->child_dev == NULL) {
 			/* Does it match this newly added device? */
@@ -1029,6 +1032,9 @@ static void coresight_fixup_device_conns(struct coresight_device *csdev)
 		struct coresight_connection *conn = &csdev->pdata->conns[i];
 		struct device *dev = NULL;
 
+		if (!conn->child_fwnode)
+			continue;
+
 		dev = bus_find_device_by_fwnode(&coresight_bustype, conn->child_fwnode);
 		if (dev) {
 			conn->child_dev = to_coresight_device(dev);
@@ -1061,7 +1067,7 @@ static int coresight_remove_match(struct device *dev, void *data)
 	for (i = 0; i < iterator->pdata->nr_outport; i++) {
 		conn = &iterator->pdata->conns[i];
 
-		if (conn->child_dev == NULL)
+		if (conn->child_dev == NULL || conn->child_fwnode == NULL)
 			continue;
 
 		if (csdev->dev.fwnode == conn->child_fwnode) {
-- 
2.24.1

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

[Stephen Boyd]

Quoting Suzuki K Poulose (2020-04-08 15:43:47)
> On Tue, Apr 07, 2020 at 08:48:54PM +0530, Sai Prakash Ranjan wrote:
> > 
> > Thanks, I can test it out.
> 
> Please find the untested patch below.
> 
> ---8>---
> 
> [untested] coresight: Fix support for sparse port numbers
> 
> On some systems the firmware may not describe all the ports
> connected to a component (e.g, for security reasons). This
> could be especially problematic for "funnels" where we could
> end up in modifying memory beyond the allocated space for
> refcounts.
> 
> e.g, for a funnel with input ports listed 0, 3, 5, nr_inport = 3.
> However the we could access refcnts[5] while checking for
> references.
> 
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---

Things don't blow up for me anymore with KASAN, so feel free to add

Tested-by: Stephen Boyd <swboyd@chromium.org>

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

[Sai Prakash Ranjan]

Hi Suzuki,

On 2020-04-09 04:13, Suzuki K Poulose wrote:
> On Tue, Apr 07, 2020 at 08:48:54PM +0530, Sai Prakash Ranjan wrote:
> 
> Please find the untested patch below.
> 
> ---8>---
> 
> [untested] coresight: Fix support for sparse port numbers
> 
> On some systems the firmware may not describe all the ports
> connected to a component (e.g, for security reasons). This
> could be especially problematic for "funnels" where we could
> end up in modifying memory beyond the allocated space for
> refcounts.
> 
> e.g, for a funnel with input ports listed 0, 3, 5, nr_inport = 3.
> However the we could access refcnts[5] while checking for
> references.
> 
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  .../hwtracing/coresight/coresight-platform.c  | 74 ++++++++++++-------
>  drivers/hwtracing/coresight/coresight.c       |  8 +-
>  2 files changed, 56 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c
> b/drivers/hwtracing/coresight/coresight-platform.c
> index 3c5bee429105..1c610d6e944b 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -67,6 +67,7 @@ static void of_coresight_get_ports_legacy(const
> struct device_node *node,
>  					  int *nr_inport, int *nr_outport)
>  {
>  	struct device_node *ep = NULL;
> +	struct of_endpoint endpoint;
>  	int in = 0, out = 0;
> 
>  	do {
> @@ -74,10 +75,16 @@ static void of_coresight_get_ports_legacy(const
> struct device_node *node,
>  		if (!ep)
>  			break;
> 
> -		if (of_coresight_legacy_ep_is_input(ep))
> -			in++;
> -		else
> -			out++;
> +		if (of_graph_parse_endpoint(ep, &endpoint))
> +			continue;
> +
> +		if (of_coresight_legacy_ep_is_input(ep)) {
> +			in = (endpoint.port + 1 > in) ?
> +				endpoint.port + 1 : in;
> +		} else {
> +			out = (endpoint.port + 1) > out ?
> +				endpoint.port + 1 : out;
> +		}
> 
>  	} while (ep);
> 
> @@ -117,9 +124,16 @@ of_coresight_count_ports(struct device_node 
> *port_parent)
>  {
>  	int i = 0;
>  	struct device_node *ep = NULL;
> +	struct of_endpoint endpoint;
> +
> +	while ((ep = of_graph_get_next_endpoint(port_parent, ep))) {
> +		/* Defer error handling to parsing */
> +		if (of_graph_parse_endpoint(ep, &endpoint))
> +			continue;
> +		if (endpoint.port + 1 > i)
> +			i = endpoint.port + 1;
> +	}
> 
> -	while ((ep = of_graph_get_next_endpoint(port_parent, ep)))
> -		i++;
>  	return i;
>  }
> 
> @@ -171,14 +185,12 @@ static int of_coresight_get_cpu(struct device 
> *dev)
>   * Parses the local port, remote device name and the remote port.
>   *
>   * Returns :
> - *	 1	- If the parsing is successful and a connection record
> - *		  was created for an output connection.
>   *	 0	- If the parsing completed without any fatal errors.
>   *	-Errno	- Fatal error, abort the scanning.
>   */
>  static int of_coresight_parse_endpoint(struct device *dev,
>  				       struct device_node *ep,
> -				       struct coresight_connection *conn)
> +				       struct coresight_platform_data *pdata)
>  {
>  	int ret = 0;
>  	struct of_endpoint endpoint, rendpoint;
> @@ -186,6 +198,7 @@ static int of_coresight_parse_endpoint(struct 
> device *dev,
>  	struct device_node *rep = NULL;
>  	struct device *rdev = NULL;
>  	struct fwnode_handle *rdev_fwnode;
> +	struct coresight_connection *conn;
> 
>  	do {
>  		/* Parse the local port details */
> @@ -212,6 +225,12 @@ static int of_coresight_parse_endpoint(struct 
> device *dev,
>  			break;
>  		}
> 
> +		conn = &pdata->conns[endpoint.port];
> +		if (conn->child_fwnode) {
> +			dev_warn(dev, "Duplicate output port %d\n", endpoint.port);
> +			ret = -EINVAL;
> +			break;
> +		}
>  		conn->outport = endpoint.port;
>  		/*
>  		 * Hold the refcount to the target device. This could be
> @@ -224,7 +243,6 @@ static int of_coresight_parse_endpoint(struct 
> device *dev,
>  		conn->child_fwnode = fwnode_handle_get(rdev_fwnode);
>  		conn->child_port = rendpoint.port;
>  		/* Connection record updated */
> -		ret = 1;
>  	} while (0);
> 
>  	of_node_put(rparent);
> @@ -238,7 +256,6 @@ static int of_get_coresight_platform_data(struct
> device *dev,
>  					  struct coresight_platform_data *pdata)
>  {
>  	int ret = 0;
> -	struct coresight_connection *conn;
>  	struct device_node *ep = NULL;
>  	const struct device_node *parent = NULL;
>  	bool legacy_binding = false;
> @@ -267,8 +284,6 @@ static int of_get_coresight_platform_data(struct
> device *dev,
>  		dev_warn_once(dev, "Uses obsolete Coresight DT bindings\n");
>  	}
> 
> -	conn = pdata->conns;
> -
>  	/* Iterate through each output port to discover topology */
>  	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
>  		/*
> @@ -280,15 +295,9 @@ static int of_get_coresight_platform_data(struct
> device *dev,
>  		if (legacy_binding && of_coresight_legacy_ep_is_input(ep))
>  			continue;
> 
> -		ret = of_coresight_parse_endpoint(dev, ep, conn);
> -		switch (ret) {
> -		case 1:
> -			conn++;		/* Fall through */
> -		case 0:
> -			break;
> -		default:
> +		ret = of_coresight_parse_endpoint(dev, ep, pdata);
> +		if (ret)
>  			return ret;
> -		}
>  	}
> 
>  	return 0;
> @@ -627,6 +636,11 @@ static int acpi_coresight_parse_link(struct
> acpi_device *adev,
>  		 *    coresight_remove_match().
>  		 */
>  		conn->child_fwnode = fwnode_handle_get(&r_adev->fwnode);
> +	} else if (dir == ACPI_CORESIGHT_LINK_SLAVE) {
> +		conn->child_port = fields[0].integer.value;
> +	} else {
> +		/* Invalid direction */
> +		return -EINVAL;
>  	}
> 
>  	return dir;
> @@ -672,10 +686,14 @@ static int acpi_coresight_parse_graph(struct
> acpi_device *adev,
>  			return dir;
> 
>  		if (dir == ACPI_CORESIGHT_LINK_MASTER) {
> -			pdata->nr_outport++;
> +			if (ptr->outport > pdata->nr_outport)
> +				pdata->nr_outport = ptr->outport;
>  			ptr++;
>  		} else {
> -			pdata->nr_inport++;
> +			WARN_ON(pdata->nr_inport == ptr->child_port);
> +			/* Do not move the ptr for input connections */
> +			if (ptr->child_port > pdata->nr_inport)
> +				pdata->nr_inport = ptr->child_port;
>  		}
>  	}
> 
> @@ -684,8 +702,13 @@ static int acpi_coresight_parse_graph(struct
> acpi_device *adev,
>  		return rc;
> 
>  	/* Copy the connection information to the final location */
> -	for (i = 0; i < pdata->nr_outport; i++)
> -		pdata->conns[i] = conns[i];
> +	for (i = 0; conns + i < ptr; i++) {
> +		int port = conns[i].outport;
> +
> +		/* Duplicate output port */
> +		WARN_ON(pdata->conns[port].child_fwnode);
> +		pdata->conns[port] = conns[i];
> +	}
> 
>  	devm_kfree(&adev->dev, conns);
>  	return 0;
> @@ -787,6 +810,7 @@ coresight_get_platform_data(struct device *dev)
>  		goto error;
> 
>  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	pdata->nr_outport = pdata->nr_inport = -1;
>  	if (!pdata) {
>  		ret = -ENOMEM;
>  		goto error;
> diff --git a/drivers/hwtracing/coresight/coresight.c
> b/drivers/hwtracing/coresight/coresight.c
> index ef20f74c85fa..f07bc0a7ab88 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -990,6 +990,9 @@ static int coresight_orphan_match(struct device
> *dev, void *data)
>  	for (i = 0; i < i_csdev->pdata->nr_outport; i++) {
>  		conn = &i_csdev->pdata->conns[i];
> 
> +		/* Skip the port if FW doesn't describe it */
> +		if (!conn->child_fwnode)
> +			continue;
>  		/* We have found at least one orphan connection */
>  		if (conn->child_dev == NULL) {
>  			/* Does it match this newly added device? */
> @@ -1029,6 +1032,9 @@ static void coresight_fixup_device_conns(struct
> coresight_device *csdev)
>  		struct coresight_connection *conn = &csdev->pdata->conns[i];
>  		struct device *dev = NULL;
> 
> +		if (!conn->child_fwnode)
> +			continue;
> +
>  		dev = bus_find_device_by_fwnode(&coresight_bustype, 
> conn->child_fwnode);
>  		if (dev) {
>  			conn->child_dev = to_coresight_device(dev);
> @@ -1061,7 +1067,7 @@ static int coresight_remove_match(struct device
> *dev, void *data)
>  	for (i = 0; i < iterator->pdata->nr_outport; i++) {
>  		conn = &iterator->pdata->conns[i];
> 
> -		if (conn->child_dev == NULL)
> +		if (conn->child_dev == NULL || conn->child_fwnode == NULL)
>  			continue;
> 
>  		if (csdev->dev.fwnode == conn->child_fwnode) {


Thanks Suzuki, I don't see the KASAN warning anymore with this patch.
But somehow tmc_etr probe fails with error -12(ENOMEM).

Thanks,
Sai

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

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

[Suzuki K Poulose]

Hi Sai,


Thanks for the quick testing ! Please see below for the
tmc_etr probe failure.

On 04/09/2020 08:51 AM, Sai Prakash Ranjan wrote:
> Hi Suzuki,
> 
> On 2020-04-09 04:13, Suzuki K Poulose wrote:
>> On Tue, Apr 07, 2020 at 08:48:54PM +0530, Sai Prakash Ranjan wrote:
>>
>> Please find the untested patch below.
>>
>> ---8>---
>>
>> [untested] coresight: Fix support for sparse port numbers
>>
>> On some systems the firmware may not describe all the ports
>> connected to a component (e.g, for security reasons). This
>> could be especially problematic for "funnels" where we could
>> end up in modifying memory beyond the allocated space for
>> refcounts.
>>
>> e.g, for a funnel with input ports listed 0, 3, 5, nr_inport = 3.
>> However the we could access refcnts[5] while checking for
>> references.
>>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>  .../hwtracing/coresight/coresight-platform.c  | 74 ++++++++++++-------
>>  drivers/hwtracing/coresight/coresight.c       |  8 +-
>>  2 files changed, 56 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c
>> b/drivers/hwtracing/coresight/coresight-platform.c
>> index 3c5bee429105..1c610d6e944b 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -67,6 +67,7 @@ static void of_coresight_get_ports_legacy(const

[...]

>> @@ -684,8 +702,13 @@ static int acpi_coresight_parse_graph(struct
>> acpi_device *adev,
>>          return rc;
>>
>>      /* Copy the connection information to the final location */
>> -    for (i = 0; i < pdata->nr_outport; i++)
>> -        pdata->conns[i] = conns[i];
>> +    for (i = 0; conns + i < ptr; i++) {
>> +        int port = conns[i].outport;
>> +
>> +        /* Duplicate output port */
>> +        WARN_ON(pdata->conns[port].child_fwnode);
>> +        pdata->conns[port] = conns[i];
>> +    }
>>
>>      devm_kfree(&adev->dev, conns);
>>      return 0;
>> @@ -787,6 +810,7 @@ coresight_get_platform_data(struct device *dev)
>>          goto error;
>>
>>      pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +    pdata->nr_outport = pdata->nr_inport = -1;


Please could you remove this hunk and test it ? I forgot to update the
commit before I sent this over.


>>              /* Does it match this newly added device? */
>> @@ -1029,6 +1032,9 @@ static void coresight_fixup_device_conns(struct
>> coresight_device *csdev)
>>          struct coresight_connection *conn = &csdev->pdata->conns[i];
>>          struct device *dev = NULL;
>>
>> +        if (!conn->child_fwnode)
>> +            continue;
>> +
>>          dev = bus_find_device_by_fwnode(&coresight_bustype, 
>> conn->child_fwnode);
>>          if (dev) {
>>              conn->child_dev = to_coresight_device(dev);
>> @@ -1061,7 +1067,7 @@ static int coresight_remove_match(struct device
>> *dev, void *data)
>>      for (i = 0; i < iterator->pdata->nr_outport; i++) {
>>          conn = &iterator->pdata->conns[i];
>>
>> -        if (conn->child_dev == NULL)
>> +        if (conn->child_dev == NULL || conn->child_fwnode == NULL)
>>              continue;
>>
>>          if (csdev->dev.fwnode == conn->child_fwnode) {
> 
> 
> Thanks Suzuki, I don't see the KASAN warning anymore with this patch.
> But somehow tmc_etr probe fails with error -12(ENOMEM).

See the above suggestion.

Cheers
Suzuki

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

[Sai Prakash Ranjan]

Hi Suzuki,

On 2020-04-09 14:47, Suzuki K Poulose wrote:
> Hi Sai,
> 
> 
> Thanks for the quick testing ! Please see below for the
> tmc_etr probe failure.
> 
> On 04/09/2020 08:51 AM, Sai Prakash Ranjan wrote:
>> Hi Suzuki,
>> 
>> On 2020-04-09 04:13, Suzuki K Poulose wrote:
>>> On Tue, Apr 07, 2020 at 08:48:54PM +0530, Sai Prakash Ranjan wrote:
>>> 
>>> Please find the untested patch below.
>>> 
>>> ---8>---
>>> 
>>> [untested] coresight: Fix support for sparse port numbers
>>> 
>>> On some systems the firmware may not describe all the ports
>>> connected to a component (e.g, for security reasons). This
>>> could be especially problematic for "funnels" where we could
>>> end up in modifying memory beyond the allocated space for
>>> refcounts.
>>> 
>>> e.g, for a funnel with input ports listed 0, 3, 5, nr_inport = 3.
>>> However the we could access refcnts[5] while checking for
>>> references.
>>> 
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>  .../hwtracing/coresight/coresight-platform.c  | 74 
>>> ++++++++++++-------
>>>  drivers/hwtracing/coresight/coresight.c       |  8 +-
>>>  2 files changed, 56 insertions(+), 26 deletions(-)
>>> 
>>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c
>>> b/drivers/hwtracing/coresight/coresight-platform.c
>>> index 3c5bee429105..1c610d6e944b 100644
>>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>>> @@ -67,6 +67,7 @@ static void of_coresight_get_ports_legacy(const
> 
> [...]
> 
>>> @@ -684,8 +702,13 @@ static int acpi_coresight_parse_graph(struct
>>> acpi_device *adev,
>>>          return rc;
>>> 
>>>      /* Copy the connection information to the final location */
>>> -    for (i = 0; i < pdata->nr_outport; i++)
>>> -        pdata->conns[i] = conns[i];
>>> +    for (i = 0; conns + i < ptr; i++) {
>>> +        int port = conns[i].outport;
>>> +
>>> +        /* Duplicate output port */
>>> +        WARN_ON(pdata->conns[port].child_fwnode);
>>> +        pdata->conns[port] = conns[i];
>>> +    }
>>> 
>>>      devm_kfree(&adev->dev, conns);
>>>      return 0;
>>> @@ -787,6 +810,7 @@ coresight_get_platform_data(struct device *dev)
>>>          goto error;
>>> 
>>>      pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> +    pdata->nr_outport = pdata->nr_inport = -1;
> 
> 
> Please could you remove this hunk and test it ? I forgot to update the
> commit before I sent this over.
> 

I don't see the ETR probe failure and the KASAN warning anymore with 
this change.

Tested-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>

Thanks,
Sai

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

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

[Sai Prakash Ranjan]

Hi Suzuki,

On 2020-04-07 20:23, Suzuki K Poulose wrote:
> On 04/07/2020 02:56 PM, Sai Prakash Ranjan wrote:
>> Hi Suzuki,
>> 
>> On 2020-04-07 18:38, Suzuki K Poulose wrote:
>>> On 04/07/2020 12:29 PM, Sai Prakash Ranjan wrote:
>>>> Hi Suzuki,
>>>> 
>>>> Thanks for looking into this issue.
>>>> 
>>>> On 2020-04-07 15:54, Suzuki K Poulose wrote:
>>>>> On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
>>>>> 
>>>>> There seems to be two replicators back to back here. What is 
>>>>> connected
>>>>> to the other output of both of them ? Are there any TPIUs ? What 
>>>>> happens
>>>>> if you choose a sink on the other end of "swao_replicator" (ETB ?)
>>>>> 
>>>> 
>>>> The other outport of swao replicator is connected to EUD which is a
>>>> QCOM specific HW which can be used as a sink like USB.
>>>> And the other outport of other replicator(replicator_out) is 
>>>> connected to
>>>> TPIU.
>>>> 
>>>>> After boot, what do the idfilter registers read for both the 
>>>>> replicators ?
>>>>> 
>>>> 
>>>> Added some prints in replicator_probe.
>>>> 
>>>>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
>>>> idfilter1=0x0
>>>>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
>>>> idfilter1=0xff
>>>>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
>>>> idfilter1=0xff
>>> 
>>> Curious to see how the idfilterX is set to 0:
>>>      if that is never used.
>>>         Or
>>>      if the user doesn't reset it back to 0xff.
>>> 
>> 
>> For both replicators, the default value seems to be 0x0.
>> 
>>   replicator probe in res ret=0 devname=6046000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
>> idfilter1=0x0
>>   replicator probe in res ret=0 devname=6b06000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
>> idfilter1=0xff
>>   replicator probe in res ret=0 devname=6046000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
>> idfilter1=0xff
> 
> I am not sure how you have added the debugs, but it looks like the
> drivers set 0xff for both the port filters on a successful probe.
> 

About the earlier mentioned points on:

1) Disallow turning the replicator ON, when it is already turned ON
2) Do what your patch does. i.e, disable the other end while one end
    is turned on.

Do we need 1) and should we go ahead with this?

Thanks,
Sai

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