Hi all,
On Mon, Mar 31, 2025 at 03:24:46PM +0100, James Clark wrote:
On 28/03/2025 10:38 pm, Yabin Cui wrote:
[...]
diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c index 275cc0d9f505..834a7ffbbdbc 100644 --- a/drivers/hwtracing/coresight/coresight-catu.c +++ b/drivers/hwtracing/coresight/coresight-catu.c @@ -458,12 +458,19 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode, static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, void *data) {
- int rc;
- int rc = 0;
- unsigned long flags; struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
- CS_UNLOCK(catu_drvdata->base);
- rc = catu_enable_hw(catu_drvdata, mode, data);
- CS_LOCK(catu_drvdata->base);
- spin_lock_irqsave(&catu_drvdata->spinlock, flags);
Hi Yabin,
This needs to be a raw_spinlock since [1]. Also you might as well use the new guard() thing to save someone find-and-replacing it later.
But I'm wondering if this is accurate. The ETR's refcount is dependent on the pid of the owner of the trace session:
/* Do not proceed if this device is associated with another session */ if (drvdata->pid != -1 && drvdata->pid != pid) { rc = -EBUSY; goto unlock_out; }
/*
- No HW configuration is needed if the sink is already in
- use for this session.
*/ if (drvdata->pid == pid) { csdev->refcnt++; goto unlock_out; }
If the helpers get enabled first, could this mean that CATU gets associated with a different session than the ETR? Maybe not, but it would be easier to understand if the core code handled the refcounting and locking for linked devices.
Yes, CATU can be enabled in a different session or even is disturbed by another mode (E.g., CATU is enabled in a perf session, then it might be wrongly enabled for Sysfs mode).
But anyway, I think this patch is needed to protect the low level's operations with a raw spin lock.
Thanks, Leo
Thanks James
- if (csdev->refcnt == 0) {
CS_UNLOCK(catu_drvdata->base);rc = catu_enable_hw(catu_drvdata, mode, data);CS_LOCK(catu_drvdata->base);- }
- if (!rc)
csdev->refcnt++;- spin_unlock_irqrestore(&catu_drvdata->spinlock, flags); return rc; }
@@ -486,12 +493,17 @@ static int catu_disable_hw(struct catu_drvdata *drvdata) static int catu_disable(struct coresight_device *csdev, void *__unused) {
- int rc;
- int rc = 0;
- unsigned long flags; struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
- CS_UNLOCK(catu_drvdata->base);
- rc = catu_disable_hw(catu_drvdata);
- CS_LOCK(catu_drvdata->base);
- spin_lock_irqsave(&catu_drvdata->spinlock, flags);
- if (--csdev->refcnt == 0) {
CS_UNLOCK(catu_drvdata->base);rc = catu_disable_hw(catu_drvdata);CS_LOCK(catu_drvdata->base);- }
- spin_unlock_irqrestore(&catu_drvdata->spinlock, flags); return rc; }
@@ -550,6 +562,7 @@ static int __catu_probe(struct device *dev, struct resource *res) dev->platform_data = pdata; drvdata->base = base;
- spin_lock_init(&drvdata->spinlock); catu_desc.access = CSDEV_ACCESS_IOMEM(base); catu_desc.pdata = pdata; catu_desc.dev = dev;
diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h index 141feac1c14b..663282ec6381 100644 --- a/drivers/hwtracing/coresight/coresight-catu.h +++ b/drivers/hwtracing/coresight/coresight-catu.h @@ -65,6 +65,7 @@ struct catu_drvdata { void __iomem *base; struct coresight_device *csdev; int irq;
- spinlock_t spinlock; }; #define CATU_REG32(name, offset) \