| Message ID | 20250402011832.2970072-3-yabinc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | coresight: catu: Introduce refcount and spinlock for enabling/disabling | expand |
On Tue, Apr 01, 2025 at 06:18:31PM -0700, Yabin Cui wrote: > A CATU device can be enabled for either PERF mode or SYSFS mode, but > not both simultaneously. Consider the following race condition: > > CPU 1 enables CATU for PERF mode. > CPU 2 enables CATU for SYSFS mode. It only increases refcnt. > CPU 2 enables ETR for SYSFS mode. > CPU 1 fails to enable ETR for PERF mode. > > This results in a CATU being enabled in PERF mode and an ETR enabled > in SYSFS mode, leading to unknown behavior. > > To prevent this situation, this patch checks enabled mode of a > CATU device before increasing its refcnt. Yes. We have also observed the same issue. I am currently working on refactoring the Arm CoreSight locking, my plan is to use coresight_path to maintain mode. Given it is uncommon case to use two modes concurrently in Arm CoreSight, I assume this is not an urgent issue. Could we defer this patch? My understanding is that this issue will be prevented with checking the path mode, rather than checking modes in seperate modules. To be clear, I am fine with other two patches in the series. Thanks, Leo > Signed-off-by: Yabin Cui <yabinc@google.com> > Suggested-by: James Clark <james.clark@linaro.org> > Suggested-by: Leo Yan <leo.yan@arm.com> > --- > drivers/hwtracing/coresight/coresight-catu.c | 6 +++++- > drivers/hwtracing/coresight/coresight-catu.h | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c > index b1d490dd7249..0caf3a3e75ce 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.c > +++ b/drivers/hwtracing/coresight/coresight-catu.c > @@ -466,7 +466,10 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, > CS_UNLOCK(catu_drvdata->base); > rc = catu_enable_hw(catu_drvdata, mode, data); > CS_LOCK(catu_drvdata->base); > - } > + if (!rc) > + catu_drvdata->mode = mode; > + } else if (catu_drvdata->mode != mode) > + rc = -EBUSY; > if (!rc) > csdev->refcnt++; > return rc; > @@ -499,6 +502,7 @@ static int catu_disable(struct coresight_device *csdev, void *__unused) > CS_UNLOCK(catu_drvdata->base); > rc = catu_disable_hw(catu_drvdata); > CS_LOCK(catu_drvdata->base); > + catu_drvdata->mode = CS_MODE_DISABLED; > } else { > rc = -EBUSY; > } > diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h > index 755776cd19c5..ea406464f0a8 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.h > +++ b/drivers/hwtracing/coresight/coresight-catu.h > @@ -66,6 +66,7 @@ struct catu_drvdata { > struct coresight_device *csdev; > int irq; > raw_spinlock_t spinlock; > + enum cs_mode mode; > }; > > #define CATU_REG32(name, offset) \ > -- > 2.49.0.472.ge94155a9ec-goog >
diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c index b1d490dd7249..0caf3a3e75ce 100644 --- a/drivers/hwtracing/coresight/coresight-catu.c +++ b/drivers/hwtracing/coresight/coresight-catu.c @@ -466,7 +466,10 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, CS_UNLOCK(catu_drvdata->base); rc = catu_enable_hw(catu_drvdata, mode, data); CS_LOCK(catu_drvdata->base); - } + if (!rc) + catu_drvdata->mode = mode; + } else if (catu_drvdata->mode != mode) + rc = -EBUSY; if (!rc) csdev->refcnt++; return rc; @@ -499,6 +502,7 @@ static int catu_disable(struct coresight_device *csdev, void *__unused) CS_UNLOCK(catu_drvdata->base); rc = catu_disable_hw(catu_drvdata); CS_LOCK(catu_drvdata->base); + catu_drvdata->mode = CS_MODE_DISABLED; } else { rc = -EBUSY; } diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h index 755776cd19c5..ea406464f0a8 100644 --- a/drivers/hwtracing/coresight/coresight-catu.h +++ b/drivers/hwtracing/coresight/coresight-catu.h @@ -66,6 +66,7 @@ struct catu_drvdata { struct coresight_device *csdev; int irq; raw_spinlock_t spinlock; + enum cs_mode mode; }; #define CATU_REG32(name, offset) \
A CATU device can be enabled for either PERF mode or SYSFS mode, but not both simultaneously. Consider the following race condition: CPU 1 enables CATU for PERF mode. CPU 2 enables CATU for SYSFS mode. It only increases refcnt. CPU 2 enables ETR for SYSFS mode. CPU 1 fails to enable ETR for PERF mode. This results in a CATU being enabled in PERF mode and an ETR enabled in SYSFS mode, leading to unknown behavior. To prevent this situation, this patch checks enabled mode of a CATU device before increasing its refcnt. Signed-off-by: Yabin Cui <yabinc@google.com> Suggested-by: James Clark <james.clark@linaro.org> Suggested-by: Leo Yan <leo.yan@arm.com> --- drivers/hwtracing/coresight/coresight-catu.c | 6 +++++- drivers/hwtracing/coresight/coresight-catu.h | 1 + 2 files changed, 6 insertions(+), 1 deletion(-)