[PATCH v2 2/3] coresight: tmc: refactor the tmc-etr mode setting to avoid race conditions
hejunhao
hejunhao3 at huawei.com
Sat Jul 19 04:09:40 PDT 2025
On 2025/7/3 0:47, Leo Yan wrote:
> On Fri, Jun 20, 2025 at 03:54:11PM +0800, Junhao He wrote:
>
> [...]
>
>> To fix this, configure the tmc-etr mode before invoking enable_etr_perf()
>> or enable_etr_sysfs(), explicitly check if the tmc-etr sink is already
>> enabled in a different mode, and simplily the setup and checks for "mode".
>> To prevent race conditions between mode transitions.
> I have a similiar fixing for CTI driver, see:
> https://lore.kernel.org/linux-arm-kernel/20250701-arm_cs_pm_fix_v3-v2-0-23ebb864fcc1@arm.com/T/#maccd68b460fb8d74ccdd3504163d8f486f04178b
>
> [...]
>
>> @@ -1781,14 +1756,52 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>> static int tmc_enable_etr_sink(struct coresight_device *csdev,
>> enum cs_mode mode, void *data)
>> {
>> + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> + enum cs_mode old_mode;
>> + int rc = -EINVAL;
>> +
>> + scoped_guard(spinlock_irqsave, &drvdata->spinlock) {
> scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {
Ok, will fix it.
> I am concerned that the spinlock is acquired and released for several
> times in a single flow. It is possible to hit some corner case, e.g.,
>
> - CPU0 acquires the lock, set sink as SYSFS mode, and releases the lock;
> - CPU1 acquires the lock, detects the SYSFS mode has been set,
> directly bail out, then enable ETM.
>
> The problem is the sink has not been enabled yet. This leads to CPU1
> to enable the tracer but prior to sink's enabling.
Yes, you are right. So I added a minor note for that. The impact of this
corner case may not have been fully considered.
> The key piont is we should ensure the mode is consistent to the
> hardware state. I will take a bit time for if we can have a better idea
> for this.
>
>> + old_mode = coresight_get_mode(csdev);
>> + if (old_mode != CS_MODE_DISABLED && old_mode != mode)
>> + return -EBUSY;
>> +
>> + if (drvdata->reading)
>> + return -EBUSY;
>> +
>> + /* In sysFS mode we can have multiple writers per sink. */
>> + if (old_mode == CS_MODE_SYSFS) {
>> + csdev->refcnt++;
> I am just wandering if we can unify the "refcnt" code for both SYSFS
> mode and Perf mode, and as a result, we can consolidate the code cross
> different drivers.
>
> Something like:
>
> if (!csdev->refcnt) {
> coresight_set_mode(csdev, mode);
> } else {
> /* The device has been configured with an incompatible mode */
> if (coresight_get_mode(csdev) != mode)
> return -EBUSY;
>
> csdev->refcnt++;
we need to check the mode, cannot directly increase the reference count
In performance mode.
> }
>
> Then when disable the device:
>
> csdev->refcnt--;
> if (!csdev->refcnt)
> coresight_set_mode(csdev, CS_MODE_DISABLED);
>
> We should not check "drvdata->reading" when enable or disable sink, as
> it is a flag to track if reading the trace buffer, it is irrelevant to
> the sink device's enabling or disabling.
In sysfs mode, it is necessary to check drvdata->reading.
>
> Please verify perf mode (especially for system wide session) to avoid
> anything missed.
I will refactor this code and upload a new version that includes all of
your suggested solutions.
Thanks,
Junhao
> Thanks,
> Leo
>
>> + return 0;
>> + }
>> +
>> + /*
>> + * minor note: In sysFS mode, the task1 get locked first, it setup
>> + * etr mode to SYSFS. Then task2 get locked,it will directly return
>> + * success even when the tmc-etr is not enabled at this moment.
>> + * Ultimately, task1 will still successfully enable tmc-etr.
>> + * This is a transient state and does not cause an anomaly.
>> + */
>> + coresight_set_mode(csdev, mode);
>> + }
>> +
>> switch (mode) {
>> case CS_MODE_SYSFS:
>> - return tmc_enable_etr_sink_sysfs(csdev);
>> + rc = tmc_enable_etr_sink_sysfs(csdev);
>> + break;
>> case CS_MODE_PERF:
>> - return tmc_enable_etr_sink_perf(csdev, data);
>> + rc = tmc_enable_etr_sink_perf(csdev, data);
>> + break;
>> default:
>> - return -EINVAL;
>> + rc = -EINVAL;
>> }
>> +
>> + if (rc && old_mode != mode) {
>> + scoped_guard(spinlock_irqsave, &drvdata->spinlock) {
>> + coresight_set_mode(csdev, old_mode);
>> + }
>> + }
>> +
>> + return rc;
>> }
>>
>> static int tmc_disable_etr_sink(struct coresight_device *csdev)
>> --
>> 2.33.0
>>
> .
>
More information about the linux-arm-kernel
mailing list