Re: [PATCH v2 02/28] coresight: etm4x: Always set tracer's device mode on target CPU

[James Clark]

On 01/07/2025 3:53 pm, Leo Yan wrote:
> When enabling a tracer via SysFS interface, the device mode may be set
> by any CPU - not necessarily the target CPU. This can lead to race
> condition in SMP, and may result in incorrect mode values being read.
>
> Consider the following example, where CPU0 attempts to enable the tracer
> on CPU1 (the target CPU):
>
>   CPU0                                    CPU1
>   etm4_enable()
>    ` coresight_take_mode(SYSFS)
>    ` etm4_enable_sysfs()
>       ` smp_call_function_single() ---->  etm4_enable_hw_smp_call()
>       			                /
>                                         /  CPU idle:
>                                        /   etm4_cpu_save()
>                                       /     ` coresight_get_mode()
> 	       Failed to enable h/w /         ^^^
>    ` coresight_set_mode(DISABLED) <-'          Read the intermediate SYSFS mode


> In this case, CPU0 initiates the operation by taking the SYSFS mode to
> avoid conflicts with the Perf mode. It then sends an IPI to CPU1 to
> configure the tracer registers. If any error occurs during this process,
> CPU0 rolls back by setting the mode to DISABLED.
>
> However, if CPU1 enters an idle state during this time, it might read
> the intermediate SYSFS mode. As a result, the CPU PM flow could wrongly
> save and restore tracer context that is actually disabled.
>
> To resolve the issue, this commit moves the device mode setting logic on
> the target CPU. This ensures that the device mode is only modified by
> the target CPU, eliminating race condition between mode writes and reads
> across CPUs.
>
> An additional change introduces the etm4_disable_hw_smp_call() function
> for SMP calls, which disables the tracer and explicitly set the mode to
> DISABLED during SysFS operations.
>
> The flow is updated with this change:
>
>   CPU0                                    CPU1
>   etm4_enable()
>    ` etm4_enable_sysfs()
>       ` smp_call_function_single() ---->  etm4_enable_hw_smp_call()
>                                            ` coresight_take_mode(SYSFS)
> 	                                    Failed, set back to DISABLED
>                                            ` coresight_set_mode(DISABLED)
>
>                                            CPU idle:
>                                            etm4_cpu_save()
>                                             ` coresight_get_mode()
>                                                ^^^
>                                                Read out the DISABLED mode

There's no lock though, so can't it do this:

 CPU0                                    CPU1
 etm4_enable()
   ` etm4_enable_sysfs()
      ` smp_call_function_single() --->  etm4_enable_hw_smp_call()
                                             `coresight_take_mode(SYSFS)

                                         CPU idle:
                                         etm4_cpu_save()
                                           ` coresight_get_mode()
                                          ^^ Intermediate SYSFS mode

This is why I was voting for changing the compare and swap mode stuff to 
spinlocks so we can be sure it's correct. The lock shouldn't be released 
until the mode is at its final value.