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

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

[Sai Prakash Ranjan]

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.

 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@chromium.org>
Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 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;
+
 	switch (drvdata->config_type) {
 	case TMC_CONFIG_TYPE_ETB:
 	case TMC_CONFIG_TYPE_ETF:
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 71de978575f3..7c8712a6ed8d 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -191,6 +191,7 @@ struct tmc_drvdata {
 	struct miscdevice	miscdev;
 	spinlock_t		spinlock;
 	pid_t			pid;
+	bool			enable;
 	bool			reading;
 	union {
 		char		*buf;		/* TMC ETB */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Stephen Boyd]

Quoting Sai Prakash Ranjan (2020-04-09 04:35:38)
> 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.
> 
>  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@chromium.org>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---

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

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

[Suzuki K Poulose]

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


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@chromium.org>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>   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.

Cheers
Suzuki

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

[Sai Prakash Ranjan]

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()")

> 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@chromium.org>
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> ---
>>   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

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@chromium.org>
> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > ---
> >   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
> 
> 
> 

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@chromium.org>
> > > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > > ---
> > >   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

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

[Sai Prakash Ranjan]

Hi Mathieu,

On 2020-04-13 22:44, Mathieu Poirier wrote:
> 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.
> 

This works as well, thanks.

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c 
b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index d0cc3985b72a..7ffe05930984 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -596,6 +596,11 @@ int tmc_read_prepare_etb(struct tmc_drvdata 
*drvdata)
                 goto out;
         }

+       if (drvdata->mode == CS_MODE_DISABLED) {
+               ret = -EINVAL;
+               goto out;
+       }
+
         /* There is no point in reading a TMC in HW FIFO mode */
         mode = readl_relaxed(drvdata->base + TMC_MODE);
         if (mode != TMC_MODE_CIRCULAR_BUFFER) {


Thanks,
Sai

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

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

[Mathieu Poirier]

On Tue, 14 Apr 2020 at 09:47, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi Mathieu,
>
> On 2020-04-13 22:44, Mathieu Poirier wrote:
> > 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.
> >
>
> This works as well, thanks.
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index d0cc3985b72a..7ffe05930984 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -596,6 +596,11 @@ int tmc_read_prepare_etb(struct tmc_drvdata
> *drvdata)
>                  goto out;
>          }
>
> +       if (drvdata->mode == CS_MODE_DISABLED) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +

We are back to your original solution where the ETB buffer can't be
read if the ETB itself is not enabled.  It _is_ possible to read the
buffer of an ETB that has been disabled.

To fix this consider the following [1].  Take the block at line 607
and move it to line 598.  As part of the if() condition at line 619,
read the value of the TMC_MODE register and exit if not in circular
mode.  If it is in circular mode continue with disabling the hardware.

[1]. https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/hwtracing/coresight/coresight-tmc-etf.c

>          /* There is no point in reading a TMC in HW FIFO mode */
>          mode = readl_relaxed(drvdata->base + TMC_MODE);
>          if (mode != TMC_MODE_CIRCULAR_BUFFER) {
>
>
> Thanks,
> Sai
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sai Prakash Ranjan]

Hi Mathieu,

On 2020-04-15 21:26, Mathieu Poirier wrote:
> On Tue, 14 Apr 2020 at 09:47, Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
>> 
>> Hi Mathieu,
>> 
>> On 2020-04-13 22:44, Mathieu Poirier wrote:
>> > 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.
>> >
>> 
>> This works as well, thanks.
>> 
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> index d0cc3985b72a..7ffe05930984 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> @@ -596,6 +596,11 @@ int tmc_read_prepare_etb(struct tmc_drvdata
>> *drvdata)
>>                  goto out;
>>          }
>> 
>> +       if (drvdata->mode == CS_MODE_DISABLED) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>> +
> 
> We are back to your original solution where the ETB buffer can't be
> read if the ETB itself is not enabled.  It _is_ possible to read the
> buffer of an ETB that has been disabled.
> 
> To fix this consider the following [1].  Take the block at line 607
> and move it to line 598.  As part of the if() condition at line 619,
> read the value of the TMC_MODE register and exit if not in circular
> mode.  If it is in circular mode continue with disabling the hardware.
> 
> [1].
> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/hwtracing/coresight/coresight-tmc-etf.c
> 

Thanks, got it now. Posted v2 - 
https://lore.kernel.org/patchwork/patch/1226022/

-Sai

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