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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel