Message-ID: <CALJ9ZPNExud7ZQ-ZgpVtPJHUsAyJv_O-CH8mCa6gUyu1E1T8zg@mail.gmail.com>
Date: Fri, 25 Apr 2025 15:05:01 -0700
From: Yabin Cui <yabinc@...gle.com>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach <mike.leach@...aro.org>, 
	James Clark <james.clark@...aro.org>, Leo Yan <leo.yan@....com>, 
	Jie Gan <quic_jiegan@...cinc.com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, coresight@...ts.linaro.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] coresight: trbe: Save/restore state across CPU low power state

On Thu, Apr 24, 2025 at 9:16 PM Anshuman Khandual
<anshuman.khandual@....com> wrote:
>
> On 4/24/25 04:30, Yabin Cui wrote:
> > Similar to ETE, TRBE may lose its context when a CPU enters low
> > power state. To make things worse, if ETE state is restored without
> > restoring TRBE state, it can lead to a stuck CPU due to an enabled
> > source device with no enabled sink devices.
>
> Could you please provide some more details about when the cpu gets
> stuck e.g dmesg, traces etc. Also consider adding those details in
> the commit message as well to establish the problem, this patch is
> trying to address.

This is already the best I know. When experimenting TRBE locally
(on Pixel 9), if I don't save TRBE state across low power state, when
recording system wide ETM data using TRBE on a CPU core. In a
few seconds, the CPU state becomes unknown (no message in dmesg).
Since the core may hold locks needed by other cores, it soon locks
other cores and causes a watchdog reset. I found the coresight driver
always carefully enables sink before source, and disables sink after
source. I guess there is some risk in not doing so, like the CPU hang
here. Maybe you know why?


>
> >
> > This patch introduces support for "arm,coresight-loses-context-with-cpu"
> > in the TRBE driver. When present, TRBE registers are saved before
> > and restored after CPU low power state. To prevent CPU hangs, TRBE
> > state is always saved after ETE state and restored after ETE state.
>
> The save and restore order here is
>
> 1. Save ETE
> 2. Save TRBE
> 3. Restore ETE
> 4. Restore TRBE
>
> >
> > Signed-off-by: Yabin Cui <yabinc@...gle.com>
> > ---
> >  .../coresight/coresight-etm4x-core.c          | 13 ++++-
> >  drivers/hwtracing/coresight/coresight-trbe.c  | 53 +++++++++++++++++++
> >  include/linux/coresight.h                     |  6 +++
> >  3 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index e5972f16abff..1bbaa1249206 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1863,6 +1863,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
> >  static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> >  {
> >       int ret = 0;
> > +     struct coresight_device *sink;
> >
> >       /* Save the TRFCR irrespective of whether the ETM is ON */
> >       if (drvdata->trfcr)
> > @@ -1871,8 +1872,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
> >        * Save and restore the ETM Trace registers only if
> >        * the ETM is active.
> >        */
> > -     if (coresight_get_mode(drvdata->csdev) && drvdata->save_state)
> > +     if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) {
> >               ret = __etm4_cpu_save(drvdata);
> > +             if (ret == 0) {
> > +                     sink = coresight_get_percpu_sink(drvdata->cpu);
> > +                     if (sink && sink_ops(sink)->percpu_save)
> > +                             sink_ops(sink)->percpu_save(sink);
> > +             }
> > +     }
> >       return ret;
> >  }
> >
> > @@ -1977,6 +1984,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> >
> >  static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
> >  {
> > +     struct coresight_device *sink = coresight_get_percpu_sink(drvdata->cpu);
> > +
> > +     if (sink && sink_ops(sink)->percpu_restore)
> > +             sink_ops(sink)->percpu_restore(sink);
>
> TRBE gets restored first which contradicts the order mentioned
> earlier in the commit message ?
>
An error in the commit message, should be:
To prevent CPU hangs, TRBE state is always saved after ETE state and
restored before ETE state.

>
> >       if (drvdata->trfcr)
> >               write_trfcr(drvdata->save_trfcr);
> >       if (drvdata->state_needs_restore)
> > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > index fff67aac8418..38bf46951a82 100644
> > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > @@ -115,6 +115,13 @@ static int trbe_errata_cpucaps[] = {
> >   */
> >  #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES       256
> >
> > +struct trbe_save_state {
> > +     u64 trblimitr;
> > +     u64 trbbaser;
> > +     u64 trbptr;
> > +     u64 trbsr;
> > +};
>
> This seems to accommodate all required TRBE registers. Although this is
> very intuitive could you please also add a comment above this structure
> explaining the elements like other existing structures in the file ?

Will do in the v2 patch.
>
> > +
> >  /*
> >   * struct trbe_cpudata: TRBE instance specific data
> >   * @trbe_flag                - TRBE dirty/access flag support
> > @@ -123,6 +130,9 @@ static int trbe_errata_cpucaps[] = {
> >   * @cpu                      - CPU this TRBE belongs to.
> >   * @mode             - Mode of current operation. (perf/disabled)
> >   * @drvdata          - TRBE specific drvdata
> > + * @state_needs_save - Need to save trace registers when entering cpu idle
> > + * @state_needs_restore      - Need to restore trace registers when exiting cpu idle
> > + * @save_state               - Saved trace registers
> >   * @errata           - Bit map for the errata on this TRBE.
> >   */
> >  struct trbe_cpudata {
> > @@ -133,6 +143,9 @@ struct trbe_cpudata {
> >       enum cs_mode mode;
> >       struct trbe_buf *buf;
> >       struct trbe_drvdata *drvdata;
> > +     bool state_needs_save;
>
> This tracks whether coresight_loses_context_with_cpu() is supported
> on the CPU or not.
>
> > +     bool state_needs_restore;
>
> This tracks whether the state has been saved earlier and hence can
> be restored later on when required.

Will update the comment in the v2 patch.

>
> > +     struct trbe_save_state save_state;
> >       DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
> >  };
> >
> > @@ -1187,12 +1200,49 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> >       return IRQ_HANDLED;
> >  }
> >
> > +static void arm_trbe_cpu_save(struct coresight_device *csdev)
> > +{
> > +     struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > +     struct trbe_save_state *state = &cpudata->save_state;
>
> Please move the above statement after the following conditional
> block. Because struct trbe_save_state is not going to be required
> if arm_trbe_cpu_save() returns prematurely from here.
>
Will do in the v2 patch.
> > +
> > +     if (cpudata->mode == CS_MODE_DISABLED || !cpudata->state_needs_save)
> > +             return;> +
> > +     state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
> > +     state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
> > +     state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> > +     state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> > +     cpudata->state_needs_restore = true;
>
> This looks right.
>
> > +}
> > +
> > +static void arm_trbe_cpu_restore(struct coresight_device *csdev)
> > +{
> > +     struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > +     struct trbe_save_state *state = &cpudata->save_state;
> Please move this assignment inside the block where these registers
> actually get restored after all checks are done.
>
Will do in the v2 patch.
> > +
> > +     if (cpudata->state_needs_restore) {
> > +             /*
> > +              * To avoid disruption of normal tracing, restore trace
> > +              * registers only when TRBE lost power (TRBLIMITR == 0).
> > +              */
>
> Although this sounds like a reasonable condition, but what happens
> when TRBE restoration is skipped ?
>
There are cases when a CPU fails to enter a low power state or only enters
a shallow low power state. So TRBE state may not be lost when calling
arm_trbe_cpu_restore(). In this case, if we write the TRBE registers, probably
we lose ETM data written between save and restore. This may not matter,
but could there be a race condition between this restore function and TRBE
interrupt handler (which could also write TRBE registers)? I can't think
this up very well. So I only restore registers when state is lost. If
the state isn't
lost, TRBE should stay in enabled mode, and continue receiving data from ETE.

> > +             if (read_sysreg_s(SYS_TRBLIMITR_EL1) == 0) {
>
>                         state = &cpudata->save_state
>
> > +                     write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
> > +                     write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
> > +                     write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
> > +                     set_trbe_enabled(cpudata, state->trblimitr);
> > +             }
> > +             cpudata->state_needs_restore = false;
>
> That means earlier captured state is dropped without a restoration.
>
> > +     }
> > +}
> > +
> >  static const struct coresight_ops_sink arm_trbe_sink_ops = {
> >       .enable         = arm_trbe_enable,
> >       .disable        = arm_trbe_disable,
> >       .alloc_buffer   = arm_trbe_alloc_buffer,
> >       .free_buffer    = arm_trbe_free_buffer,
> >       .update_buffer  = arm_trbe_update_buffer,
> > +     .percpu_save    = arm_trbe_cpu_save,
> > +     .percpu_restore = arm_trbe_cpu_restore,
>
> Why this percpu_* prefix ?
>
Added percpu_ prefix because it's only for percpu_sink
(coresight_get_percpu_sink).
Please let me know if you have a better name.
> >  };
> >
> >  static const struct coresight_ops arm_trbe_cs_ops = {
> > @@ -1358,6 +1408,9 @@ static void arm_trbe_probe_cpu(void *info)
> >       cpudata->trbe_flag = get_trbe_flag_update(trbidr);
> >       cpudata->cpu = cpu;
> >       cpudata->drvdata = drvdata;
> > +     cpudata->state_needs_save = coresight_loses_context_with_cpu(
> > +             &drvdata->pdev->dev);
> > +     cpudata->state_needs_restore = false;
>
> The init here looks good.
>
> >       return;
> >  cpu_clear:
> >       cpumask_clear_cpu(cpu, &drvdata->supported_cpus);
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index d79a242b271d..fec375d02535 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -362,6 +362,10 @@ enum cs_mode {
> >   * @alloc_buffer:    initialises perf's ring buffer for trace collection.
> >   * @free_buffer:     release memory allocated in @get_config.
> >   * @update_buffer:   update buffer pointers after a trace session.
> > + * @percpu_save:     saves state when CPU enters idle state.
> > + *                   Only set for percpu sink.
> > + * @percpu_restore:  restores state when CPU exits idle state.
> > + *                   only set for percpu sink.
> >   */
> >  struct coresight_ops_sink {
> >       int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> > @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> >       unsigned long (*update_buffer)(struct coresight_device *csdev,
> >                             struct perf_output_handle *handle,
> >                             void *sink_config);
> > +     void (*percpu_save)(struct coresight_device *csdev);
> > +     void (*percpu_restore)(struct coresight_device *csdev);
>
> Again - why this percpu_* prefix ?
>
> >  };
> >
> >  /**

Powered by blists - more mailing lists