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

[Leo Yan]

Hi Mike,

On Wed, Nov 12, 2025 at 09:16:46AM +0000, Mike Leach wrote:
> 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.

Yes, Levi reminded me he already has some patches [1] to address the
issue.  Basically, the series returns -EBUSY if trying to write sysfs
knobs but the device has been enabled.

Actually, we have another solution (credits to Levi who has worked on
it with some internal discussion).  Let me elaberate at here:

Similiar to you suggested "isolating sysfs and perf config setting",
but isolate in a different way:

  User's config: it is a set parameters stored for setting from user
  space. It can come from sysfs knobs or from cscfs parameters.

  Hardware's config: it is a set parameters for setting hardware
  registers (we can simply use drvdata->config for this purpose).

As a result, every time when a session is enabled (it can be a sysfs
session or perf session), we apply user's config for updating
hardware's config (e.g., in etm4_enable_sysfs() / etm4_enable_perf()),
afterwards, when invoke etm4_disable_hw() / etm4_enable_hw(), we
only use hardware's config for register setting.

This can naturally resolve the mismatched issue you mentioned in CPU
PM save and restore, and this can benefit us for later consolidating
cscfg parameters.

I prefer we can use a separate patch series to address this issue (I
should review Levi's patch series so can make things proceed). At the
meantime, I would say this patch is still valid.

As a side topic, actually we have found issues for the config is
reset when start a session so the user's config does not take affect at
all (see etm4_set_default_config()).

[1] https://lore.kernel.org/linux-arm-kernel/20241221165934.1161856-2-yeoreum.yun@arm.com/

> On Tue, 11 Nov 2025 at 18:59, Leo Yan <leo.yan at 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.

Sorry for confusion.
 
I will update the commit log:

  Split the hardware disable sequence into a new function
  __etm4_disable_hw(), which performs the common disable
  logic without the CoreSight lock.

  __etm4_cpu_save() has its own CoreSight lock handling, it
  calls __etm4_disable_hw() so without duplicate lock issues.

Thanks,
Leo