Message-ID: <731dbb1d-e804-4678-9b8c-40f6395e94a7@arm.com>
Date: Fri, 25 Apr 2025 09:46:31 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Yabin Cui <yabinc@...gle.com>, 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>
Cc: 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 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 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 ?


>  	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 ?

> +
>  /*
>   * 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.

> +	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.

> +
> +	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.

> +
> +	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 ?

> +		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 ?

>  };
>  
>  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