[PATCH 3/4] coresight: tmc-etr: Fix race condition between sysfs and perf mode

[Yicong Yang]

On 2024/12/2 17:46, Suzuki K Poulose wrote:
> Hi Yicong
> 
> On 02/12/2024 09:24, Yicong Yang wrote:
>> From: Yicong Yang <yangyicong at hisilicon.com>
>>
>> When trying to run perf and sysfs mode simultaneously, the WARN_ON()
>> in tmc_etr_enable_hw() is triggered sometimes:
>>
>>   WARNING: CPU: 42 PID: 3911571 at drivers/hwtracing/coresight/coresight-tmc-etr.c:1060 tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc]
>>   [..snip..]
>>   Call trace:
>>    tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] (P)
>>    tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] (L)
>>    tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc]
>>    coresight_enable_path+0x1c8/0x218 [coresight]
>>    coresight_enable_sysfs+0xa4/0x228 [coresight]
>>    enable_source_store+0x58/0xa8 [coresight]
>>    dev_attr_store+0x20/0x40
>>    sysfs_kf_write+0x4c/0x68
>>    kernfs_fop_write_iter+0x120/0x1b8
>>    vfs_write+0x2c8/0x388
>>    ksys_write+0x74/0x108
>>    __arm64_sys_write+0x24/0x38
>>    el0_svc_common.constprop.0+0x64/0x148
>>    do_el0_svc+0x24/0x38
>>    el0_svc+0x3c/0x130
>>    el0t_64_sync_handler+0xc8/0xd0
>>    el0t_64_sync+0x1ac/0x1b0
>>   ---[ end trace 0000000000000000 ]---
>>
>> Since the enablement of sysfs mode is separeted into two critical regions,
>> one for sysfs buffer allocation and another for hardware enablement, it's
>> possible to race with the perf mode. Fix this by double check whether
>> the perf mode's been used before enabling the hardware in sysfs mode.
> 
> Thanks for the fix. Some minor comments below.
> 
> It needs a Fixes tag.
> 
>>
>> Signed-off-by: Yicong Yang <yangyicong at hisilicon.com>
>> ---
>>   .../hwtracing/coresight/coresight-tmc-etr.c   | 30 +++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index ad83714ca4dc..d382d95da5ff 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -1230,6 +1230,36 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>>         spin_lock_irqsave(&drvdata->spinlock, flags);
>>   +    /*
>> +     * Since the sysfs buffer allocation and the hardware enablement is not
>> +     * in the same critical region, it's possible to race with the perf
>> +     * mode:
>> +     *   [sysfs mode]                   [perf mode]
>> +     *   tmc_etr_get_sysfs_buffer()
>> +     *     spin_lock(&drvdata->spinlock)
>> +     *     [sysfs buffer allocation]
>> +     *     spin_unlock(&drvdata->spinlock)
>> +     *                                  spin_lock(&drvdata->spinlock)
>> +     *                                  tmc_etr_enable_hw()
>> +     *                                    drvdata->etr_buf = etr_perf->etr_buf
>> +     *                                  spin_unlock(&drvdata->spinlock)
>> +     *   spin_lock(&drvdata->spinlock)
>> +     *   tmc_etr_enable_hw()
>> +     *     WARN_ON(drvdata->etr_buf) // WARN sicne etr_buf initialized at
>> +     *                                  the perf side
>> +     *   spin_unlock(&drvdata->spinlock)
>> +     *
>> +     * So check here before continue.
>> +     */
>> +    if (coresight_get_mode(csdev) == CS_MODE_PERF) {
>> +        drvdata->sysfs_buf = NULL;
>> +        spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +
>> +        /* Free allocated memory out side of the spinlock */
>> +        tmc_etr_free_sysfs_buf(sysfs_buf);
>> +        return -EBUSY;
>> +    }
> 
> With this in place, I think we should remove the check for CS_MODE_PERF in get_etr_sysfs_buf() to avoid confusion (which I believe opened up this race)
> 

ok, considering that maybe we can optimize it a bit more in the tmc_etr_get_sysfs_buffer().
Check whether the perf mode's already running before we actually allocate the buffer.
Then we can save the time of allocating/freeing the sysfs buffer if race with the perf mode.

Thanks.