[SRU][N][PATCH v2 1/1] coresight: Remove atomic type from refcnt
Noah Wager
noah.wager at canonical.com
Tue Feb 24 18:06:40 UTC 2026
On Tue, Feb 24, 2026 at 09:56:10AM +0100, Stefan Bader wrote:
> On 24/02/2026 08:33, Noah Wager wrote:
> > From: James Clark <james.clark at arm.com>
> >
> > Refcnt is only ever accessed from either inside the coresight_mutex, or
> > the device's spinlock, making the atomic type and atomic_dec_return()
>
> I would say above is the hard question. Is refcnt really ever accessed under
> protection in the 6.8 code base? It is probably hard to validate as access
> is happening around enable / disable operations. Not sure how often that
> takes places. Ensuring the assumption is true is the problematic part and
> usually too complicated to be done within the time limits of upstream stable
> work. I don't want to say no as you spent a lot of time on the backport
> already. It would be good to point out that it has been confirmed that the
> accessors all have some lock protection.
>
> The alternative to this would be to adjust / fix up the upstream stable
> patch. As
>
> coresight: catu: Introduce refcount and spinlock for enabling/disabling
>
> primarily wants to introduce refcount/lock protection it might just be safer
> to adapt the freshly introduced code to what everything around it does
> instead of the other way round. So just replacing direct access to refcnt by
> atomic_* functions.
>
> -Stefan
>
All references of refcnt occur in the same scope as an acquired spinlock
except one, which I mentioned inline below. Going by the author's word,
it seems likely all the references are protected for this tree, but I
can't say for certain.
A SAUCE patch, that adds the missing atomic_read/inc() calls to the
breaking patch, would indeed be the smallest change to the tree. I did
want to avoid a SAUCE patch if possible, but you raise a good point in
that I am putting a lot of trust in the assumption that the commit
message applies to the noble tree.
I guess my reply doesn't have any concrete answers, so I'd like your
advice: do you think the small SAUCE fixup is the way to go, as opposed
to trying to do backport acrobatics for this particular issue?
- Noah
> > calls confusing and unnecessary. The only point of synchronisation
> > outside of these two types of locks is already done with a compare and
> > swap on 'mode', which a comment has been added for.
> >
> > There was one instance of refcnt being used outside of a lock in TPIU,
> > but that can easily be fixed by making it the same as all the other
> > devices and adding a spinlock. Potentially in the future all the
> > refcounting and locking can be moved up into the core code, and all the
> > mostly duplicate code from the individual devices can be removed.
> >
> > Signed-off-by: James Clark <james.clark at arm.com>
> > Link: https://lore.kernel.org/r/20240129154050.569566-8-james.clark@arm.com
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose at arm.com>
> > (backported from commit 4545b38ef004a586295750ea49a505b6396a7c90)
> > [nwager: Context changes due to missing patchset:
> > "coresight: Separate sysfs and Perf usage and some other cleanups".
> > Only this commit was a missing dependency of "coresight: catu: Introduce
> > refcount and spinlock for enabling/disabling", and since this change is
> > simple (basically a find/replace of refcnt atomic_read/inc to int accesses),
> > I just fixed up the context changes instead of porting the five previous
> > patches from the series. Here is the gist of the changes I made:
> > - previous line context changes due to different way of checking CS_MODE_SYSFS
> > - some occurrences were removed before this patch arrived upstream, so
> > replace those occurrences in my fixup
> > - upstream moved some hunks to coresight-sysfs.c, so I needed to
> > replace the occurrences in the original locations
> > - drop the 'mode' comment due to missing commit from series]
> > Signed-off-by: Noah Wager <noah.wager at canonical.com>
> >
> > Signed-off-by: Noah Wager <noah.wager at canonical.com>
> > ---
> > drivers/hwtracing/coresight/coresight-core.c | 9 +++++----
> > drivers/hwtracing/coresight/coresight-etb10.c | 11 +++++-----
> > .../hwtracing/coresight/coresight-tmc-etf.c | 20 ++++++++++---------
> > .../hwtracing/coresight/coresight-tmc-etr.c | 13 ++++++------
> > drivers/hwtracing/coresight/coresight-tpiu.c | 14 +++++++++++--
> > drivers/hwtracing/coresight/ultrasoc-smb.c | 9 +++++----
> > include/linux/coresight.h | 7 +++++--
> > 7 files changed, 51 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index fc0e7f18d95b..52d50545d5c9 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -370,7 +370,7 @@ static void coresight_disable_link(struct coresight_device *csdev,
> > 0)
> > return;
> > } else {
> > - if (atomic_read(&csdev->refcnt) != 0)
> > + if (csdev->refcnt != 0)
> > return;
This is the only use of csdev->refcnt that doesn't explicitly acquire a
lock in the same scope. However, the line just below it is an access of
csdev->enable, which is said to only be used when locked in the upstream
commit that removes the ->enable field (from the same patch series):
d5e83f97eb56 ("coresight: Remove the 'enable' field.")
The 'enable' field has existed from the beginning, so I believe the
author's explanation should hold for 6.8. However, you are right in that
it is unlikely I will be able to independently verify this, as I am not
a coresight expert and it is difficult to make sweeping generalizations
about the control flow of enable/disable events.
> > }
> > @@ -391,7 +391,7 @@ int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
> > csdev->enable = true;
> > }
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt--;
> > return 0;
> > }
> > @@ -473,7 +473,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
> > static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
> > void *data)
> > {
> > - if (atomic_dec_return(&csdev->refcnt) == 0) {
> > + csdev->refcnt--;
> > + if (csdev->refcnt == 0) {
> > coresight_disable_source(csdev, data);
> > csdev->enable = false;
> > }
> > @@ -1142,7 +1143,7 @@ int coresight_enable(struct coresight_device *csdev)
> > * source is already enabled.
> > */
> > if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE)
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > goto out;
> > }
> > diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> > index d83d52dbd1a2..b2d0069bd21c 100644
> > --- a/drivers/hwtracing/coresight/coresight-etb10.c
> > +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> > @@ -163,7 +163,7 @@ static int etb_enable_sysfs(struct coresight_device *csdev)
> > drvdata->mode = CS_MODE_SYSFS;
> > }
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > out:
> > raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> > return ret;
> > @@ -199,7 +199,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
> > * use for this session.
> > */
> > if (drvdata->pid == pid) {
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > goto out;
> > }
> > @@ -217,7 +217,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
> > /* Associate with monitored process. */
> > drvdata->pid = pid;
> > drvdata->mode = CS_MODE_PERF;
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > }
> > out:
> > @@ -356,7 +356,8 @@ static int etb_disable(struct coresight_device *csdev)
> > raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> > - if (atomic_dec_return(&csdev->refcnt)) {
> > + csdev->refcnt--;
> > + if (csdev->refcnt) {
> > raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> > return -EBUSY;
> > }
> > @@ -447,7 +448,7 @@ static unsigned long etb_update_buffer(struct coresight_device *csdev,
> > raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> > /* Don't do anything if another tracer is using this sink */
> > - if (atomic_read(&csdev->refcnt) != 1)
> > + if (csdev->refcnt != 1)
> > goto out;
> > __etb_disable_hw(drvdata);
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > index 7406b65e2cdd..b7ecf660309a 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -206,7 +206,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
> > * touched.
> > */
> > if (drvdata->mode == CS_MODE_SYSFS) {
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > goto out;
> > }
> > @@ -229,7 +229,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
> > ret = tmc_etb_enable_hw(drvdata);
> > if (!ret) {
> > drvdata->mode = CS_MODE_SYSFS;
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > } else {
> > /* Free up the buffer if we failed to enable */
> > used = false;
> > @@ -284,7 +284,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
> > * use for this session.
> > */
> > if (drvdata->pid == pid) {
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > break;
> > }
> > @@ -293,7 +293,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
> > /* Associate with monitored process. */
> > drvdata->pid = pid;
> > drvdata->mode = CS_MODE_PERF;
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > }
> > } while (0);
> > spin_unlock_irqrestore(&drvdata->spinlock, flags);
> > @@ -338,7 +338,8 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
> > return -EBUSY;
> > }
> > - if (atomic_dec_return(&csdev->refcnt)) {
> > + csdev->refcnt--;
> > + if (csdev->refcnt) {
> > spin_unlock_irqrestore(&drvdata->spinlock, flags);
> > return -EBUSY;
> > }
> > @@ -371,7 +372,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
> > return -EBUSY;
> > }
> > - if (atomic_read(&csdev->refcnt) == 0) {
> > + if (csdev->refcnt == 0) {
> > ret = tmc_etf_enable_hw(drvdata);
> > if (!ret) {
> > drvdata->mode = CS_MODE_SYSFS;
> > @@ -379,7 +380,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
> > }
> > }
> > if (!ret)
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > spin_unlock_irqrestore(&drvdata->spinlock, flags);
> > if (first_enable)
> > @@ -401,7 +402,8 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
> > return;
> > }
> > - if (atomic_dec_return(&csdev->refcnt) == 0) {
> > + csdev->refcnt--;
> > + if (csdev->refcnt == 0) {
> > tmc_etf_disable_hw(drvdata);
> > drvdata->mode = CS_MODE_DISABLED;
> > last_disable = true;
> > @@ -489,7 +491,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
> > spin_lock_irqsave(&drvdata->spinlock, flags);
> > /* Don't do anything if another tracer is using this sink */
> > - if (atomic_read(&csdev->refcnt) != 1)
> > + if (csdev->refcnt != 1)
> > goto out;
> > CS_UNLOCK(drvdata->base);
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index 52c55e8172ee..b1c55e7d3d3b 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > @@ -1231,14 +1231,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> > * touched, even if the buffer size has changed.
> > */
> > if (drvdata->mode == CS_MODE_SYSFS) {
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > goto out;
> > }
> > ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
> > if (!ret) {
> > drvdata->mode = CS_MODE_SYSFS;
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > }
> > out:
> > @@ -1564,7 +1564,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
> > spin_lock_irqsave(&drvdata->spinlock, flags);
> > /* Don't do anything if another tracer is using this sink */
> > - if (atomic_read(&csdev->refcnt) != 1) {
> > + if (csdev->refcnt != 1) {
> > spin_unlock_irqrestore(&drvdata->spinlock, flags);
> > goto out;
> > }
> > @@ -1676,7 +1676,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
> > * use for this session.
> > */
> > if (drvdata->pid == pid) {
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > goto unlock_out;
> > }
> > @@ -1686,7 +1686,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
> > drvdata->pid = pid;
> > drvdata->mode = CS_MODE_PERF;
> > drvdata->perf_buf = etr_perf->etr_buf;
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > }
> > unlock_out:
> > @@ -1719,7 +1719,8 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
> > return -EBUSY;
> > }
> > - if (atomic_dec_return(&csdev->refcnt)) {
> > + csdev->refcnt--;
> > + if (csdev->refcnt) {
> > spin_unlock_irqrestore(&drvdata->spinlock, flags);
> > return -EBUSY;
> > }
> > diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
> > index 59eac93fd6bb..c23a6b9b41fe 100644
> > --- a/drivers/hwtracing/coresight/coresight-tpiu.c
> > +++ b/drivers/hwtracing/coresight/coresight-tpiu.c
> > @@ -58,6 +58,7 @@ struct tpiu_drvdata {
> > void __iomem *base;
> > struct clk *atclk;
> > struct coresight_device *csdev;
> > + spinlock_t spinlock;
> > };
> > static void tpiu_enable_hw(struct csdev_access *csa)
> > @@ -72,8 +73,11 @@ static void tpiu_enable_hw(struct csdev_access *csa)
> > static int tpiu_enable(struct coresight_device *csdev, enum cs_mode mode,
> > void *__unused)
> > {
> > + struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > + guard(spinlock)(&drvdata->spinlock);
> > tpiu_enable_hw(&csdev->access);
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > dev_dbg(&csdev->dev, "TPIU enabled\n");
> > return 0;
> > }
> > @@ -96,7 +100,11 @@ static void tpiu_disable_hw(struct csdev_access *csa)
> > static int tpiu_disable(struct coresight_device *csdev)
> > {
> > - if (atomic_dec_return(&csdev->refcnt))
> > + struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > + guard(spinlock)(&drvdata->spinlock);
> > + csdev->refcnt--;
> > + if (csdev->refcnt)
> > return -EBUSY;
> > tpiu_disable_hw(&csdev->access);
> > @@ -132,6 +140,8 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)
> > if (!drvdata)
> > return -ENOMEM;
> > + spin_lock_init(&drvdata->spinlock);
> > +
> > drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */
> > if (!IS_ERR(drvdata->atclk)) {
> > ret = clk_prepare_enable(drvdata->atclk);
> > diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> > index 10e886455b8b..2c1227fd701b 100644
> > --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> > +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> > @@ -103,7 +103,7 @@ static int smb_open(struct inode *inode, struct file *file)
> > if (drvdata->reading)
> > return -EBUSY;
> > - if (atomic_read(&drvdata->csdev->refcnt))
> > + if (drvdata->csdev->refcnt)
> > return -EBUSY;
> > smb_update_data_size(drvdata);
> > @@ -270,7 +270,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
> > if (ret)
> > return ret;
> > - atomic_inc(&csdev->refcnt);
> > + csdev->refcnt++;
> > dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
> > return ret;
> > @@ -285,7 +285,8 @@ static int smb_disable(struct coresight_device *csdev)
> > if (drvdata->reading)
> > return -EBUSY;
> > - if (atomic_dec_return(&csdev->refcnt))
> > + csdev->refcnt--;
> > + if (csdev->refcnt)
> > return -EBUSY;
> > /* Complain if we (somehow) got out of sync */
> > @@ -380,7 +381,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
> > guard(spinlock)(&drvdata->spinlock);
> > /* Don't do anything if another tracer is using this sink. */
> > - if (atomic_read(&csdev->refcnt) != 1)
> > + if (csdev->refcnt != 1)
> > return 0;
> > smb_disable_hw(drvdata);
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index 4c9ff95c2c87..7a908907a652 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -226,7 +226,10 @@ struct coresight_sysfs_link {
> > * by @coresight_ops.
> > * @access: Device i/o access abstraction for this device.
> > * @dev: The device entity associated to this component.
> > - * @refcnt: keep track of what is in use.
> > + * @refcnt: keep track of what is in use. Only access this outside of the
> > + * device's spinlock when the coresight_mutex held and mode ==
> > + * CS_MODE_SYSFS. Otherwise it must be accessed from inside the
> > + * spinlock.
> > * @orphan: true if the component has connections that haven't been linked.
> > * @enable: 'true' if component is currently part of an active path.
> > * @activated: 'true' only if a _sink_ has been activated. A sink can be
> > @@ -250,7 +253,7 @@ struct coresight_device {
> > const struct coresight_ops *ops;
> > struct csdev_access access;
> > struct device dev;
> > - atomic_t refcnt;
> > + int refcnt;
> > bool orphan;
> > bool enable; /* true only if configured as part of a path */
> > /* sink specific fields */
>
More information about the kernel-team
mailing list