Re: [PATCH v6 11/11] coresight: etm4x: Reuse normal enable and disable logic in CPU idle

Hi Leo,

The save/restore sequence is only used if the ETM is active at the time of power down.

However, by removing the specific save/restore structure, and using the existing drvdata->config structure, the actual state can be changed by sysfs from another PE, as the sysfs accesses only write to the drvdata->config structure and not the actual hardware. Thus this change permits the corruption of the saved state, which will result in restore != saved..

While using the same code is a good idea avoiding lots of duplication, it may be necessary to have a copy of the drvdata->config passed in to the relevant functions.

We have considered in the past if it might be worthwhile isolating the sysfs and perf config settings in the etm - it may be worth re-visiting this again with a save restore copy in mind to.

On Tue, 11 Nov 2025 at 18:59, Leo Yan leo.yan@arm.com wrote:

The etm4_enable_trace_unit() function is almost identical to the restore flow, with a few differences listed below:

1. TRCQCTLR register

   The TRCQCTLR register is saved and restored during CPU idle, but it is never touched in the normal flow. Given the Q element is not enabled (TRCCONFIGR.QE bits), it is acceptable to omit saving and restoring this register during idle.

2. TRCSSCSRn.STATUS bit

   The restore flow does not explicitly clear the TRCSSCSRn.STATUS bit but instead directly loads the saved value. In contrast, the normal flow clears this bit to 0 when re-enabling single-shot control.

   Therefore, the restore function passes the restart_ss argument as false to etm4_enable_hw() to avoid re-enabling single-shot mode.

3. Claim Tag Handling

   The claim tag is acquired and released in normal flow, on the other hand, the claim protocol is not applied in CPU idle flow - it simply restores the saved value.

   The claim bits serve two purpose:

   • Exclusive access between the kernel driver and an external debugger. During CPU idle, the kernel driver locks the OS Lock, ensuring that the external debugger cannot access the trace unit. Therefore, it is safe to release the claim tag during idle.

   • Notification to supervisory software to save/restore context for external debuggers. The kernel driver does not touch the external debugger's claim bit (ETMCLAIM[0]).

   Based on this, it is safe to acquire and release claim tag in the idle sequence.

4. OS Lock Behavior

   The OS Lock should be locked during CPU idle states. This differs from the normal flow, which unlock it. This special handling remains in the CPU save path.

This commit reuses the normal enable and disable logic in the CPU idle path. The common disable logic is moved into a new function __etm4_disable_hw(), which omits the etm4_cs_{unlock|lock}() pairs and is convenient to call from the save callback.

Why omit cs lock/unlock? Not unlocking the software lock prevents the claim tag registers from being written, which contradicts 3) above, and prevents other writes during the disable sequence.

Regards

Mike.

The resume callback sets the 'retain_ss_status' flag to avoid restarting the single-shot mode, and then directly calls etm4_enable_hw().

The save context in the driver data is no longer needed and has been removed.

Tested-by: James Clark james.clark@linaro.org Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Signed-off-by: Leo Yan leo.yan@arm.com

---

drivers/hwtracing/coresight/coresight-etm4x-core.c | 215 ++------------------- drivers/hwtracing/coresight/coresight-etm4x.h | 57 ------ 2 files changed, 16 insertions(+), 256 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index afee54ac0aec0c0e521fa1885a95f48555a369de..1c17d54729200671ec6339f43755ea4267dd61dc 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1002,7 +1002,7 @@ static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata) isb(); }

-static void etm4_disable_hw(struct etmv4_drvdata *drvdata) +static void __etm4_disable_hw(struct etmv4_drvdata *drvdata) { u32 control; struct etmv4_config *config = &drvdata->config; @@ -1010,7 +1010,6 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) struct csdev_access *csa = &csdev->access; int i;

•   etm4_cs_unlock(drvdata, csa);
    etm4_disable_arch_specific(drvdata);
  
    if (!drvdata->skip_power_up) {
  

@@ -1039,12 +1038,21 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) config->seq_state = etm4x_relaxed_read32(csa, TRCSEQSTR);

    coresight_disclaim_device_unlocked(csdev);

•   etm4_cs_lock(drvdata, csa);
  
    dev_dbg(&drvdata->csdev->dev,
            "cpu: %d disable smp call done\n", drvdata->cpu);
  

}

+static void etm4_disable_hw(struct etmv4_drvdata *drvdata) +{

•   struct coresight_device *csdev = drvdata->csdev;
  

•   struct csdev_access *csa = &csdev->access;
  

•   etm4_cs_unlock(drvdata, csa);
  

•   __etm4_disable_hw(drvdata);
  

•   etm4_cs_lock(drvdata, csa);
  

+}

static void etm4_disable_sysfs_smp_call(void *info) { struct etmv4_drvdata *drvdata = info; @@ -1852,8 +1860,7 @@ static int etm4_dying_cpu(unsigned int cpu)

static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) {

•   int i, ret = 0;
  

•   struct etmv4_save_state *state;
  

•   int ret = 0;
    struct coresight_device *csdev = drvdata->csdev;
    struct csdev_access *csa;
    struct device *etm_dev;
  

@@ -1884,91 +1891,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) goto out; }

•   if (!drvdata->paused)
  

•           etm4_disable_trace_unit(drvdata);
  

•   state = drvdata->save_state;
  

•   if (drvdata->nr_pe)
  

•           state->trcprocselr = etm4x_read32(csa, TRCPROCSELR);
  

•   state->trcconfigr = etm4x_read32(csa, TRCCONFIGR);
  

•   state->trcauxctlr = etm4x_read32(csa, TRCAUXCTLR);
  

•   state->trceventctl0r = etm4x_read32(csa, TRCEVENTCTL0R);
  

•   state->trceventctl1r = etm4x_read32(csa, TRCEVENTCTL1R);
  

•   if (drvdata->stallctl)
  

•           state->trcstallctlr = etm4x_read32(csa, TRCSTALLCTLR);
  

•   state->trctsctlr = etm4x_read32(csa, TRCTSCTLR);
  

•   state->trcsyncpr = etm4x_read32(csa, TRCSYNCPR);
  

•   state->trcccctlr = etm4x_read32(csa, TRCCCCTLR);
  

•   state->trcbbctlr = etm4x_read32(csa, TRCBBCTLR);
  

•   state->trctraceidr = etm4x_read32(csa, TRCTRACEIDR);
  

•   if (drvdata->q_filt)
  

•           state->trcqctlr = etm4x_read32(csa, TRCQCTLR);
  

•   state->trcvictlr = etm4x_read32(csa, TRCVICTLR);
  

•   state->trcviiectlr = etm4x_read32(csa, TRCVIIECTLR);
  

•   state->trcvissctlr = etm4x_read32(csa, TRCVISSCTLR);
  

•   if (drvdata->nr_pe_cmp)
  

•           state->trcvipcssctlr = etm4x_read32(csa, TRCVIPCSSCTLR);
  

•   for (i = 0; i < drvdata->nrseqstate - 1; i++)
  

•           state->trcseqevr[i] = etm4x_read32(csa, TRCSEQEVRn(i));
  

•   if (drvdata->nrseqstate) {
  

•           state->trcseqrstevr = etm4x_read32(csa, TRCSEQRSTEVR);
  

•           state->trcseqstr = etm4x_read32(csa, TRCSEQSTR);
  

•   }
  

•   if (drvdata->numextinsel)
  

•           state->trcextinselr = etm4x_read32(csa, TRCEXTINSELR);
  

•   for (i = 0; i < drvdata->nr_cntr; i++) {
  

•           state->trccntrldvr[i] = etm4x_read32(csa, TRCCNTRLDVRn(i));
  

•           state->trccntctlr[i] = etm4x_read32(csa, TRCCNTCTLRn(i));
  

•           state->trccntvr[i] = etm4x_read32(csa, TRCCNTVRn(i));
  

•   }
  

•   /* Resource selector pair 0 is reserved */
  

•   for (i = 2; i < drvdata->nr_resource * 2; i++)
  

•           state->trcrsctlr[i] = etm4x_read32(csa, TRCRSCTLRn(i));
  

•   for (i = 0; i < drvdata->nr_ss_cmp; i++) {
  

•           state->trcssccr[i] = etm4x_read32(csa, TRCSSCCRn(i));
  

•           state->trcsscsr[i] = etm4x_read32(csa, TRCSSCSRn(i));
  

•           if (etm4x_sspcicrn_present(drvdata, i))
  

•                   state->trcsspcicr[i] = etm4x_read32(csa, TRCSSPCICRn(i));
  

•   }
  

•   for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
  

•           state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));
  

•           state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));
  

•   }
  

•   /*
  

•    * Data trace stream is architecturally prohibited for A profile cores
  

•    * so we don't save (or later restore) trcdvcvr and trcdvcmr - As per
  

•    * section 1.3.4 ("Possible functional configurations of an ETMv4 trace
  

•    * unit") of ARM IHI 0064D.
  

•    */
  

•   for (i = 0; i < drvdata->numcidc; i++)
  

•           state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));
  

•   for (i = 0; i < drvdata->numvmidc; i++)
  

•           state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));
  

•   state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0);
  

•   if (drvdata->numcidc > 4)
  

•           state->trccidcctlr1 = etm4x_read32(csa, TRCCIDCCTLR1);
  

•   state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR0);
  

•   if (drvdata->numvmidc > 4)
  

•           state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR1);
  

•   state->trcclaimset = etm4x_read32(csa, TRCCLAIMCLR);
  

•   if (!drvdata->skip_power_up)
  

•           state->trcpdcr = etm4x_read32(csa, TRCPDCR);
  

•   __etm4_disable_hw(drvdata);
  
    /* wait for TRCSTATR.IDLE to go up */
    if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1)) {
  

@@ -1976,17 +1899,8 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) "timeout while waiting for Idle Trace Status\n"); etm4_os_unlock(drvdata); ret = -EBUSY;

•           goto out;
    }
  
  

•   /*
  

•    * Power can be removed from the trace unit now. We do this to
  

•    * potentially save power on systems that respect the TRCPDCR_PU
  

•    * despite requesting software to save/restore state.
  

•    */
  

•   if (!drvdata->skip_power_up)
  

•           etm4x_relaxed_write32(csa, (state->trcpdcr & ~TRCPDCR_PU),
  

•                                 TRCPDCR);
  

out: etm4_cs_lock(drvdata, csa); return ret; @@ -2010,103 +1924,13 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)

static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) {

•   int i;
  

•   struct etmv4_save_state *state = drvdata->save_state;
  

•   struct csdev_access *csa = &drvdata->csdev->access;
  

•   if (WARN_ON(!drvdata->csdev))
            return;
  
  

•   etm4_cs_unlock(drvdata, csa);
  

•   etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
  

•   if (drvdata->nr_pe)
  

•           etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR);
  

•   etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR);
  

•   etm4x_relaxed_write32(csa, state->trcauxctlr, TRCAUXCTLR);
  

•   etm4x_relaxed_write32(csa, state->trceventctl0r, TRCEVENTCTL0R);
  

•   etm4x_relaxed_write32(csa, state->trceventctl1r, TRCEVENTCTL1R);
  

•   if (drvdata->stallctl)
  

•           etm4x_relaxed_write32(csa, state->trcstallctlr, TRCSTALLCTLR);
  

•   etm4x_relaxed_write32(csa, state->trctsctlr, TRCTSCTLR);
  

•   etm4x_relaxed_write32(csa, state->trcsyncpr, TRCSYNCPR);
  

•   etm4x_relaxed_write32(csa, state->trcccctlr, TRCCCCTLR);
  

•   etm4x_relaxed_write32(csa, state->trcbbctlr, TRCBBCTLR);
  

•   etm4x_relaxed_write32(csa, state->trctraceidr, TRCTRACEIDR);
  

•   if (drvdata->q_filt)
  

•           etm4x_relaxed_write32(csa, state->trcqctlr, TRCQCTLR);
  

•   etm4x_relaxed_write32(csa, state->trcvictlr, TRCVICTLR);
  

•   etm4x_relaxed_write32(csa, state->trcviiectlr, TRCVIIECTLR);
  

•   etm4x_relaxed_write32(csa, state->trcvissctlr, TRCVISSCTLR);
  

•   if (drvdata->nr_pe_cmp)
  

•           etm4x_relaxed_write32(csa, state->trcvipcssctlr, TRCVIPCSSCTLR);
  

•   for (i = 0; i < drvdata->nrseqstate - 1; i++)
  

•           etm4x_relaxed_write32(csa, state->trcseqevr[i], TRCSEQEVRn(i));
  

•   if (drvdata->nrseqstate) {
  

•           etm4x_relaxed_write32(csa, state->trcseqrstevr, TRCSEQRSTEVR);
  

•           etm4x_relaxed_write32(csa, state->trcseqstr, TRCSEQSTR);
  

•   }
  

•   if (drvdata->numextinsel)
  

•           etm4x_relaxed_write32(csa, state->trcextinselr, TRCEXTINSELR);
  

•   /* Instruct etm4_enable_hw() not to restart the single-shot mode */
  

•   drvdata->config.retain_ss_status = 1;
  
  

•   for (i = 0; i < drvdata->nr_cntr; i++) {
  

•           etm4x_relaxed_write32(csa, state->trccntrldvr[i], TRCCNTRLDVRn(i));
  

•           etm4x_relaxed_write32(csa, state->trccntctlr[i], TRCCNTCTLRn(i));
  

•           etm4x_relaxed_write32(csa, state->trccntvr[i], TRCCNTVRn(i));
  

•   }
  

•   /* Resource selector pair 0 is reserved */
  

•   for (i = 2; i < drvdata->nr_resource * 2; i++)
  

•           etm4x_relaxed_write32(csa, state->trcrsctlr[i], TRCRSCTLRn(i));
  

•   for (i = 0; i < drvdata->nr_ss_cmp; i++) {
  

•           etm4x_relaxed_write32(csa, state->trcssccr[i], TRCSSCCRn(i));
  

•           etm4x_relaxed_write32(csa, state->trcsscsr[i], TRCSSCSRn(i));
  

•           if (etm4x_sspcicrn_present(drvdata, i))
  

•                   etm4x_relaxed_write32(csa, state->trcsspcicr[i], TRCSSPCICRn(i));
  

•   }
  

•   for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
  

•           etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));
  

•           etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));
  

•   }
  

•   for (i = 0; i < drvdata->numcidc; i++)
  

•           etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));
  

•   for (i = 0; i < drvdata->numvmidc; i++)
  

•           etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));
  

•   etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0);
  

•   if (drvdata->numcidc > 4)
  

•           etm4x_relaxed_write32(csa, state->trccidcctlr1, TRCCIDCCTLR1);
  

•   etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR0);
  

•   if (drvdata->numvmidc > 4)
  

•           etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR1);
  

•   etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
  

•   if (!drvdata->skip_power_up)
  

•           etm4x_relaxed_write32(csa, state->trcpdcr, TRCPDCR);
  

•   /*
  

•    * As recommended by section 4.3.7 ("Synchronization when using the
  

•    * memory-mapped interface") of ARM IHI 0064D
  

•    */
  

•   dsb(sy);
  

•   isb();
  

•   /* Unlock the OS lock to re-enable trace and external debug access */
  

•   etm4_os_unlock(drvdata);
  

•   if (!drvdata->paused)
  

•           etm4_enable_trace_unit(drvdata);
  

•   etm4_cs_lock(drvdata, csa);
  

•   etm4_enable_hw(drvdata);
  

}

static void etm4_cpu_restore(struct etmv4_drvdata *drvdata) @@ -2293,13 +2117,6 @@ static int etm4_probe(struct device *dev) pm_save_enable = coresight_loses_context_with_cpu(dev) ? PARAM_PM_SAVE_SELF_HOSTED : PARAM_PM_SAVE_NEVER;

•   if (pm_save_enable != PARAM_PM_SAVE_NEVER) {
  

•           drvdata->save_state = devm_kmalloc(dev,
  

•                           sizeof(struct etmv4_save_state), GFP_KERNEL);
  

•           if (!drvdata->save_state)
  

•                   return -ENOMEM;
  

•   }
  

•   raw_spin_lock_init(&drvdata->spinlock);
  
    drvdata->cpu = coresight_get_cpu(dev);
  

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index d82ec1559b34787dffc7debf4643d948e3850b2a..8d4a1f0f1e52a38a280b7fbe978129287a8ba60c 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -864,61 +864,6 @@ struct etmv4_config { u8 s_ex_level; };

-/**

• • struct etm4_save_state - state to be preserved when ETM is without power
• */

-struct etmv4_save_state {

•   u32     trcprocselr;
  

•   u32     trcconfigr;
  

•   u32     trcauxctlr;
  

•   u32     trceventctl0r;
  

•   u32     trceventctl1r;
  

•   u32     trcstallctlr;
  

•   u32     trctsctlr;
  

•   u32     trcsyncpr;
  

•   u32     trcccctlr;
  

•   u32     trcbbctlr;
  

•   u32     trctraceidr;
  

•   u32     trcqctlr;
  

•   u32     trcvictlr;
  

•   u32     trcviiectlr;
  

•   u32     trcvissctlr;
  

•   u32     trcvipcssctlr;
  

•   u32     trcseqevr[ETM_MAX_SEQ_STATES];
  

•   u32     trcseqrstevr;
  

•   u32     trcseqstr;
  

•   u32     trcextinselr;
  

•   u32     trccntrldvr[ETMv4_MAX_CNTR];
  

•   u32     trccntctlr[ETMv4_MAX_CNTR];
  

•   u32     trccntvr[ETMv4_MAX_CNTR];
  

•   u32     trcrsctlr[ETM_MAX_RES_SEL];
  

•   u32     trcssccr[ETM_MAX_SS_CMP];
  

•   u32     trcsscsr[ETM_MAX_SS_CMP];
  

•   u32     trcsspcicr[ETM_MAX_SS_CMP];
  

•   u64     trcacvr[ETM_MAX_SINGLE_ADDR_CMP];
  

•   u64     trcacatr[ETM_MAX_SINGLE_ADDR_CMP];
  

•   u64     trccidcvr[ETMv4_MAX_CTXID_CMP];
  

•   u64     trcvmidcvr[ETM_MAX_VMID_CMP];
  

•   u32     trccidcctlr0;
  

•   u32     trccidcctlr1;
  

•   u32     trcvmidcctlr0;
  

•   u32     trcvmidcctlr1;
  

•   u32     trcclaimset;
  

•   u32     cntr_val[ETMv4_MAX_CNTR];
  

•   u32     seq_state;
  

•   u32     vinst_ctrl;
  

•   u32     ss_status[ETM_MAX_SS_CMP];
  

•   u32     trcpdcr;
  

-};

/**

• struct etm4_drvdata - specifics associated to an ETM component
• @pclk: APB clock if present, otherwise NULL

@@ -981,7 +926,6 @@ struct etmv4_save_state {

•         at runtime, due to the additional setting of TRFCR_CX when
  

•         in EL2. Otherwise, 0.
  

• @config: structure holding configuration parameters.

• • @save_state: State to be preserved across power loss
  • @skip_power_up: Indicates if an implementation can skip powering up

  •            the trace unit.
    

  • @paused: Indicates if the trace unit is paused.

@@ -1036,7 +980,6 @@ struct etmv4_drvdata { bool lpoverride; u64 trfcr; struct etmv4_config config;

•   struct etmv4_save_state         *save_state;
    bool                            skip_power_up;
    bool                            paused;
    DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
  
  

-- 2.34.1

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK