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

[Suzuki K Poulose]

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)

Suzuki



> +
>   	/*
>   	 * In sysFS mode we can have multiple writers per sink.  Since this
>   	 * sink is already enabled no memory is needed and the HW need not be