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

[Mathieu Poirier]

On Mon, Apr 13, 2020 at 01:55:30PM +0530, Sai Prakash Ranjan wrote:
> Hi Suzuki,
> 
> On 2020-04-13 04:47, 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
> > 
> 
> I do not see any SError when I run the above command.
> 
> localhost ~ # cat /sys/bus/coresight/devices/tmc_etf0/mgmt/mode
> 0x0
> 
> And this is most likely due to
> 
> commit cd9e3474bb793dc ("coresight: add PM runtime calls to
> coresight_simple_func()")

Ok, so this is related to power management (you can ignore my question in the
previous email).

Regarding function tmc_read_prepare_etb(), the best way to deal with this is
probably make sure drvdata->mode != CS_MODE_DISABLED before reading TMC_MODE.
If there is a buffer to read it will have been copied when the ETB was disabled
and there won't be a need to access the HW.

Mathieu

> 
> > 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;
> > > +
> > 
> > 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.
> > 
> 
> Ok I will make this change.
> 
> Thanks,
> Sai
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation