| Message ID | 20250328223837.2314277-1-yabinc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | coresight: catu: Introduce refcount and spinlock for enabling/disabling | expand |
On 28/03/2025 10:38 pm, Yabin Cui wrote: > When tracing ETM data on multiple CPUs concurrently via the > perf interface, the CATU device is shared across different CPU > paths. This can lead to race conditions when multiple CPUs attempt > to enable or disable the CATU device simultaneously. > > To address these race conditions, this patch introduces the > following changes: > > 1. The enable and disable operations for the CATU device are not > reentrant. Therefore, a spinlock is added to ensure that only > one CPU can enable or disable a given CATU device at any point > in time. > > 2. A reference counter is used to manage the enable/disable state > of the CATU device. The device is enabled when the first CPU > requires it and is only disabled when the last CPU finishes > using it. This ensures the device remains active as long as at > least one CPU needs it. > > Signed-off-by: Yabin Cui <yabinc@google.com> > --- > drivers/hwtracing/coresight/coresight-catu.c | 29 ++++++++++++++------ > drivers/hwtracing/coresight/coresight-catu.h | 1 + > 2 files changed, 22 insertions(+), 8 deletions(-) > > 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. [1]: https://lore.kernel.org/all/20250306121110.1647948-3-yeoreum.yun@arm.com/ 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) \
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 > [1]: > https://lore.kernel.org/all/20250306121110.1647948-3-yeoreum.yun@arm.com/ > > 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) \ >
On 31/03/2025 15:24, James Clark wrote: > > > On 28/03/2025 10:38 pm, Yabin Cui wrote: >> When tracing ETM data on multiple CPUs concurrently via the >> perf interface, the CATU device is shared across different CPU >> paths. This can lead to race conditions when multiple CPUs attempt >> to enable or disable the CATU device simultaneously. >> >> To address these race conditions, this patch introduces the >> following changes: >> >> 1. The enable and disable operations for the CATU device are not >> reentrant. Therefore, a spinlock is added to ensure that only >> one CPU can enable or disable a given CATU device at any point >> in time. >> >> 2. A reference counter is used to manage the enable/disable state >> of the CATU device. The device is enabled when the first CPU >> requires it and is only disabled when the last CPU finishes >> using it. This ensures the device remains active as long as at >> least one CPU needs it. >> >> Signed-off-by: Yabin Cui <yabinc@google.com> >> --- >> drivers/hwtracing/coresight/coresight-catu.c | 29 ++++++++++++++------ >> drivers/hwtracing/coresight/coresight-catu.h | 1 + >> 2 files changed, 22 insertions(+), 8 deletions(-) >> >> 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? Thats a good point, I looked at the code and : I think we have a bug here, w.r.t helpers and Sinks. We enable the helpers (e.g. CATU) and then we try the actual sink (e.g. ETR). And if we fail to enable ETR, we don't disable the helpers. We have this today : case CORESIGHT_DEV_TYPE_SINK: ret = coresight_enable_sink(csdev, mode, sink_data); /* * Sink is the first component turned on. If we * failed to enable the sink, there are no components * that need disabling. Disabling the path here * would mean we could disrupt an existing session. */ if (ret) goto out; ... out: return ret; err: coresight_disable_path_from(path, nd); goto out; } Suzuki Maybe not, but it > would be easier to understand if the core code handled the refcounting > and locking for linked devices. > > [1]: https://lore.kernel.org/all/20250306121110.1647948-3- > yeoreum.yun@arm.com/ > > 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) \ >
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); + 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) \
When tracing ETM data on multiple CPUs concurrently via the perf interface, the CATU device is shared across different CPU paths. This can lead to race conditions when multiple CPUs attempt to enable or disable the CATU device simultaneously. To address these race conditions, this patch introduces the following changes: 1. The enable and disable operations for the CATU device are not reentrant. Therefore, a spinlock is added to ensure that only one CPU can enable or disable a given CATU device at any point in time. 2. A reference counter is used to manage the enable/disable state of the CATU device. The device is enabled when the first CPU requires it and is only disabled when the last CPU finishes using it. This ensures the device remains active as long as at least one CPU needs it. Signed-off-by: Yabin Cui <yabinc@google.com> --- drivers/hwtracing/coresight/coresight-catu.c | 29 ++++++++++++++------ drivers/hwtracing/coresight/coresight-catu.h | 1 + 2 files changed, 22 insertions(+), 8 deletions(-)