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