| Message ID | 20250418055820.3689408-4-hejunhao3@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Coresight TMC-ETR some bugfixes and cleanups | expand |
On 4/18/2025 1:58 PM, Junhao He wrote: > 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 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) > > To resolve this, configure the tmc-etr mode before invoking > `enable_perf()` or sysfs interfaces. Prior to mode configuration, > explicitly check if the tmc-etr sink is already enabled in a > different mode to prevent race conditions between mode transitions. > Furthermore, enforce spinlock protection around the critical > sections to serialize concurrent accesses from sysfs and perf > subsystems. Is any issue observed during this race condition? The etr_buf is checked before assign it to sysfs_buf or perf_buf. In my understanding, the warning raised to indicate the etr already enabled with sysfs mode or perf mode, so terminate current enable session, right? Thanks, Jie > > Fixes: 296b01fd106e ("coresight: Refactor out buffer allocation function for ETR") > Reported-by: Yicong Yang <yangyicong@hisilicon.com> > Closes: https://lore.kernel.org/linux-arm-kernel/20241202092419.11777-2-yangyicong@huawei.com/ > Signed-off-by: Junhao He <hejunhao3@huawei.com> > --- > .../hwtracing/coresight/coresight-tmc-etr.c | 77 +++++++++++-------- > 1 file changed, 47 insertions(+), 30 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index a48bb85d0e7f..3d94d64cacaa 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1190,11 +1190,6 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) > spin_lock_irqsave(&drvdata->spinlock, flags); > } > > - if (drvdata->reading || coresight_get_mode(csdev) == CS_MODE_PERF) { > - ret = -EBUSY; > - goto out; > - } > - > /* > * If we don't have a buffer or it doesn't match the requested size, > * use the buffer allocated above. Otherwise reuse the existing buffer. > @@ -1205,7 +1200,6 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) > drvdata->sysfs_buf = new_buf; > } > > -out: > spin_unlock_irqrestore(&drvdata->spinlock, flags); > > /* Free memory outside the spinlock if need be */ > @@ -1216,7 +1210,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) > > static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) > { > - int ret = 0; > + int ret; > unsigned long flags; > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > struct etr_buf *sysfs_buf = tmc_etr_get_sysfs_buffer(csdev); > @@ -1226,23 +1220,10 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) > > spin_lock_irqsave(&drvdata->spinlock, flags); > > - /* > - * 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 > - * touched, even if the buffer size has changed. > - */ > - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { > - csdev->refcnt++; > - goto out; > - } > - > ret = tmc_etr_enable_hw(drvdata, sysfs_buf); > - if (!ret) { > - coresight_set_mode(csdev, CS_MODE_SYSFS); > + if (!ret) > csdev->refcnt++; > - } > > -out: > spin_unlock_irqrestore(&drvdata->spinlock, flags); > > if (!ret) > @@ -1652,11 +1633,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) > struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle); > > spin_lock_irqsave(&drvdata->spinlock, flags); > - /* Don't use this sink if it is already claimed by sysFS */ > - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { > - rc = -EBUSY; > - goto unlock_out; > - } > > if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) { > rc = -EINVAL; > @@ -1685,7 +1661,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) > if (!rc) { > /* Associate with monitored process. */ > drvdata->pid = pid; > - coresight_set_mode(csdev, CS_MODE_PERF); > drvdata->perf_buf = etr_perf->etr_buf; > csdev->refcnt++; > } > @@ -1698,14 +1673,56 @@ 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; > + > + scoped_guard(spinlock_irqsave, &drvdata->spinlock) { > + 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. Since this > + * sink is already enabled no memory is needed and the HW need not be > + * touched, even if the buffer size has changed. > + */ > + if (old_mode == CS_MODE_SYSFS) { > + csdev->refcnt++; > + return 0; > + } > + > + /* > + * minor note: > + * When sysfs-task1 get locked, it setup the mode first. Then > + * sysfs-task2 gets locked，it will directly return success even > + * when the tmc-etr is not enabled at this moment. Ultimately, > + * sysfs-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; > } > + > + scoped_guard(spinlock_irqsave, &drvdata->spinlock) { > + if (rc && old_mode != mode) > + coresight_set_mode(csdev, old_mode); > + } > + > + return rc; > } > > static int tmc_disable_etr_sink(struct coresight_device *csdev)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index a48bb85d0e7f..3d94d64cacaa 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1190,11 +1190,6 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags); } - if (drvdata->reading || coresight_get_mode(csdev) == CS_MODE_PERF) { - ret = -EBUSY; - goto out; - } - /* * If we don't have a buffer or it doesn't match the requested size, * use the buffer allocated above. Otherwise reuse the existing buffer. @@ -1205,7 +1200,6 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) drvdata->sysfs_buf = new_buf; } -out: spin_unlock_irqrestore(&drvdata->spinlock, flags); /* Free memory outside the spinlock if need be */ @@ -1216,7 +1210,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) { - int ret = 0; + int ret; unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct etr_buf *sysfs_buf = tmc_etr_get_sysfs_buffer(csdev); @@ -1226,23 +1220,10 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags); - /* - * 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 - * touched, even if the buffer size has changed. - */ - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { - csdev->refcnt++; - goto out; - } - ret = tmc_etr_enable_hw(drvdata, sysfs_buf); - if (!ret) { - coresight_set_mode(csdev, CS_MODE_SYSFS); + if (!ret) csdev->refcnt++; - } -out: spin_unlock_irqrestore(&drvdata->spinlock, flags); if (!ret) @@ -1652,11 +1633,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle); spin_lock_irqsave(&drvdata->spinlock, flags); - /* Don't use this sink if it is already claimed by sysFS */ - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { - rc = -EBUSY; - goto unlock_out; - } if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) { rc = -EINVAL; @@ -1685,7 +1661,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) if (!rc) { /* Associate with monitored process. */ drvdata->pid = pid; - coresight_set_mode(csdev, CS_MODE_PERF); drvdata->perf_buf = etr_perf->etr_buf; csdev->refcnt++; } @@ -1698,14 +1673,56 @@ 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; + + scoped_guard(spinlock_irqsave, &drvdata->spinlock) { + 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. Since this + * sink is already enabled no memory is needed and the HW need not be + * touched, even if the buffer size has changed. + */ + if (old_mode == CS_MODE_SYSFS) { + csdev->refcnt++; + return 0; + } + + /* + * minor note: + * When sysfs-task1 get locked, it setup the mode first. Then + * sysfs-task2 gets locked，it will directly return success even + * when the tmc-etr is not enabled at this moment. Ultimately, + * sysfs-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; } + + scoped_guard(spinlock_irqsave, &drvdata->spinlock) { + if (rc && old_mode != mode) + coresight_set_mode(csdev, old_mode); + } + + return rc; } static int tmc_disable_etr_sink(struct coresight_device *csdev)
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 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) To resolve this, configure the tmc-etr mode before invoking `enable_perf()` or sysfs interfaces. Prior to mode configuration, explicitly check if the tmc-etr sink is already enabled in a different mode to prevent race conditions between mode transitions. Furthermore, enforce spinlock protection around the critical sections to serialize concurrent accesses from sysfs and perf subsystems. Fixes: 296b01fd106e ("coresight: Refactor out buffer allocation function for ETR") Reported-by: Yicong Yang <yangyicong@hisilicon.com> Closes: https://lore.kernel.org/linux-arm-kernel/20241202092419.11777-2-yangyicong@huawei.com/ Signed-off-by: Junhao He <hejunhao3@huawei.com> --- .../hwtracing/coresight/coresight-tmc-etr.c | 77 +++++++++++-------- 1 file changed, 47 insertions(+), 30 deletions(-)