Re: [PATCH] coresight: tmc: Read TMC mode only when TMC hw is enabled

[Mathieu Poirier]

On Mon, Apr 13, 2020 at 12:17:21AM +0100, Suzuki K Poulose wrote:
> Hi Sai,
> 
> On 04/09/2020 12:35 PM, Sai Prakash Ranjan wrote:
> > Reading TMC mode register in tmc_read_prepare_etb without
> > enabling the TMC hardware leads to async exceptions like
> > the one in the call trace below. This can happen if the
> > user tries to read the TMC etf data via device node without
> > setting up source and the sink first which enables the TMC
> > hardware in the path. So make sure that the TMC is enabled
> > before we try to read TMC data.
> 
> So, one can trigger the same SError by simply :
> 
> $ cat /sys/bus/coresight/device/tmc_etb0/mgmt/mode

A true TMC-ETB is a rarity nowadays... What boards was this tested on?  I don't
know if it is how the IP behaves or how things are connected on Sai's specific
board but doing echo'ing tmc_etf0/mgmt/mode on my Juno R0 doesn't give rise to
any problems.  That certainly doesn't mean it can't happen on another platform.  

> 
> 
> And also :
> 
> > 
> >   Kernel panic - not syncing: Asynchronous SError Interrupt
> >   CPU: 7 PID: 2605 Comm: hexdump Tainted: G S                5.4.30 #122
> >   Call trace:
> >    dump_backtrace+0x0/0x188
> >    show_stack+0x20/0x2c
> >    dump_stack+0xdc/0x144
> >    panic+0x168/0x36c
> >    panic+0x0/0x36c
> >    arm64_serror_panic+0x78/0x84
> >    do_serror+0x130/0x138
> >    el1_error+0x84/0xf8
> >    tmc_read_prepare_etb+0x88/0xb8
> >    tmc_open+0x40/0xd8
> >    misc_open+0x120/0x158
> >    chrdev_open+0xb8/0x1a4
> >    do_dentry_open+0x268/0x3a0
> >    vfs_open+0x34/0x40
> >    path_openat+0x39c/0xdf4
> >    do_filp_open+0x90/0x10c
> >    do_sys_open+0x150/0x3e8
> >    __arm64_compat_sys_openat+0x28/0x34
> >    el0_svc_common+0xa8/0x160
> >    el0_svc_compat_handler+0x2c/0x38
> >    el0_svc_compat+0x8/0x10
> > 
> > Fixes: 4525412a5046 ("coresight: tmc: making prepare/unprepare functions generic")
> > Reported-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>
> > ---
> >   drivers/hwtracing/coresight/coresight-tmc.c | 5 +++++
> >   drivers/hwtracing/coresight/coresight-tmc.h | 1 +
> >   2 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> > index 1cf82fa58289..7bae69748ab7 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> > @@ -62,11 +62,13 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> >   void tmc_enable_hw(struct tmc_drvdata *drvdata)
> >   {
> > +	drvdata->enable = true;
> >   	writel_relaxed(TMC_CTL_CAPT_EN, drvdata->base + TMC_CTL);
> >   }
> >   void tmc_disable_hw(struct tmc_drvdata *drvdata)
> >   {
> > +	drvdata->enable = false;
> >   	writel_relaxed(0x0, drvdata->base + TMC_CTL);
> >   }
> > @@ -102,6 +104,9 @@ static int tmc_read_prepare(struct tmc_drvdata *drvdata)
> >   {
> >   	int ret = 0;
> > +	if (!drvdata->enable)
> > +		return -EINVAL;
> > +

Proceeding this way would mean that ETB and ETF internal memory buffers can't be
read unless the device is enabled and collecting trace.  That is not practical
because 1) it changes the way the sysfs interface works on all boards where
there isn't a problem and 2) it makes it very difficult to capture the correct
data.

When the device is __not__ enabled, does reading any of the registers under
"/sys/bus/coresight/devices/tmc_etbX/mgmt/" also cause a problem or is it just
'mode'?

Thanks,
Mathieu

> 
> Does this check always guarantee that the TMC is enabled when
> we actually get to reading the MODE ? This needs to be done
> under the spinlock.
> 
> Cheers
> Suzuki
> 
> 
>