[PATCH 0/1] coresight: fix issue with insufficient claim tags

[PATCH 0/1] coresight: fix issue with insufficient claim tags

[Mike Leach]

All CoreSight compliant components have an implementation defined number
of 0 to 8 claim tag bits in the claim tag registers.

These are used to claim the CoreSight resources by system agents.

ARM recommends implementions have 4 claim tag registers.

The CoreSight drivers implement a 2 claim tag bit protocol to allow
self hosted and external debug agents to manage access to the hardware.

However, if there are less than 2 claim tags available the protocol
incorrectly returns an error on device claim, as no checks are made.

If insufficient claim tags are present in a component then the protocol
must return success on claim / disclaim to allow components to be used
normally.

Changes read the CLAIMSET bits to establish the number of available tags,
and adjust the claim returns accordingly.

Applies to coresignt/next

Mike Leach (1):
  coresight: fix issue where coresight component has no claimtags

 drivers/hwtracing/coresight/coresight-core.c | 26 ++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-priv.h |  8 ++++++
 2 files changed, 34 insertions(+)

-- 
2.32.0

[PATCH 1/1] coresight: fix issue where coresight component has no claimtags

[Mike Leach]

Coresight components have 0 to 8 claim tag bits. ARM recommends 4 and
the implemented claim tag protocol uses two of these.

If a component has insufficient claim tags then the protocol incorrectly
returns an error when attempting to claim a component.

Fix by reading CLAIMSET to establish then actual number of claim tags
and return success when attempting to claim a component were there are
insufficient tags to implement the protocol.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/hwtracing/coresight/coresight-core.c | 26 ++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-priv.h |  8 ++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 3267192f0c1c..ce4eade8a1e3 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -131,6 +131,22 @@ coresight_find_out_connection(struct coresight_device *csdev,
 	return ERR_PTR(-ENODEV);
 }
 
+/*
+ * Reading CLIAMSET returns  a bitfield representing the number of claim tags
+ * implemented from bit 0 to bit nTag-1, valid bits set to 1.
+ *
+ * Claim protocol requires 2 bits so test for highest bit required,
+ * bit 1 -  CORESIGHT_CLAIM_SELF_HOSTED_BIT
+ *
+ * return true if sufficient claim tags implemented for protocol
+ */
+static bool coresight_claim_tags_implemented_unlocked(struct csdev_access *csa)
+{
+	u32 claim_bits_impl = FIELD_GET(CORESIGHT_CLAIM_BITS_MAX_MASK,
+			 csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
+	return ((claim_bits_impl & CORESIGHT_CLAIM_SELF_HOSTED_BIT) != 0);
+}
+
 static u32 coresight_read_claim_tags_unlocked(struct coresight_device *csdev)
 {
 	return FIELD_GET(CORESIGHT_CLAIM_MASK,
@@ -156,6 +172,9 @@ EXPORT_SYMBOL_GPL(coresight_clear_self_claim_tag);
 
 void coresight_clear_self_claim_tag_unlocked(struct csdev_access *csa)
 {
+	if (!coresight_claim_tags_implemented_unlocked(csa))
+		return;
+
 	csdev_access_relaxed_write32(csa, CORESIGHT_CLAIM_SELF_HOSTED,
 				     CORESIGHT_CLAIMCLR);
 	isb();
@@ -181,6 +200,10 @@ int coresight_claim_device_unlocked(struct coresight_device *csdev)
 		return -EINVAL;
 
 	csa = &csdev->access;
+
+	if (!coresight_claim_tags_implemented_unlocked(csa))
+		return 0;
+
 	tag = coresight_read_claim_tags_unlocked(csdev);
 
 	switch (tag) {
@@ -237,6 +260,9 @@ void coresight_disclaim_device_unlocked(struct coresight_device *csdev)
 	if (WARN_ON(!csdev))
 		return;
 
+	if (!coresight_claim_tags_implemented_unlocked(&csdev->access))
+		return;
+
 	if (coresight_read_claim_tags_unlocked(csdev) == CORESIGHT_CLAIM_SELF_HOSTED)
 		coresight_clear_self_claim_tag_unlocked(&csdev->access);
 	else
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 33e22b1ba043..f93cbc9bb36a 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -41,6 +41,14 @@ extern const struct device_type coresight_dev_type[];
 #define CORESIGHT_CLAIM_SELF_HOSTED	2
 #define CORESIGHT_CLAIM_INVALID		3
 
+/*
+ * Coresight specification defines a maximum of 8 claim tag bits.
+ * The precise number is implementation defined, and may be obtained by
+ * reading the CLAIMSET register.
+ */
+#define CORESIGHT_CLAIM_BITS_MAX_MASK	GENMASK(7, 0)
+#define CORESIGHT_CLAIM_SELF_HOSTED_BIT	BIT(1)
+
 #define TIMEOUT_US		100
 #define BMVAL(val, lsb, msb)	((val & GENMASK(msb, lsb)) >> lsb)
 
-- 
2.32.0

Re: [PATCH 1/1] coresight: fix issue where coresight component has no claimtags

[Leo Yan]

On Wed, Oct 22, 2025 at 12:45:20AM +0100, Mike Leach wrote:

[...]

> +/*
> + * Reading CLIAMSET returns  a bitfield representing the number of claim tags
> + * implemented from bit 0 to bit nTag-1, valid bits set to 1.
> + *
> + * Claim protocol requires 2 bits so test for highest bit required,
> + * bit 1 -  CORESIGHT_CLAIM_SELF_HOSTED_BIT
> + *
> + * return true if sufficient claim tags implemented for protocol
> + */
> +static bool coresight_claim_tags_implemented_unlocked(struct csdev_access *csa)
> +{
> +	u32 claim_bits_impl = FIELD_GET(CORESIGHT_CLAIM_BITS_MAX_MASK,
> +			 csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
> +	return ((claim_bits_impl & CORESIGHT_CLAIM_SELF_HOSTED_BIT) != 0);
> +}
> +

The logic looks good to me.

Can we improve a bit with a cached flag to avoid every time checking the
CLAIMSET, we can read it only once at init time instead.

- We can add a new flag ("bool claim_impl" in the struct csdev_access),
  by default the field will be zero.

- The drivers support claim tags call coresight_clear_self_claim_tag()
  in probe (see __catu_probe() as an example), we can call a new
  function coresight_init_claim_tag() to replace it, this function sets
  "claim_impl" properly and clear the tag if supported.

- Afterwards, simply check the "claim_impl" flag.

[...]

> +/*
> + * Coresight specification defines a maximum of 8 claim tag bits.
> + * The precise number is implementation defined, and may be obtained by
> + * reading the CLAIMSET register.
> + */
> +#define CORESIGHT_CLAIM_BITS_MAX_MASK	GENMASK(7, 0)
> +#define CORESIGHT_CLAIM_SELF_HOSTED_BIT	BIT(1)

Now we only care about the self-host bit. Can reuse existed macros ?

  CORESIGHT_CLAIM_MASK
  CORESIGHT_CLAIM_SELF_HOSTED

I don't mind if extend CORESIGHT_CLAIM_MASK to 8 bits and even remove
CORESIGHT_CLAIM_INVALID (as it cannot cover other invalid values). Or we
can defer the extension as needed later.

Thanks,
Leo

Re: [PATCH 1/1] coresight: fix issue where coresight component has no claimtags

[Mike Leach]

Hi Leo

On Wed, 22 Oct 2025 at 10:30, Leo Yan <leo.yan@arm.com> wrote:
>
> On Wed, Oct 22, 2025 at 12:45:20AM +0100, Mike Leach wrote:
>
> [...]
>
> > +/*
> > + * Reading CLIAMSET returns  a bitfield representing the number of claim tags
> > + * implemented from bit 0 to bit nTag-1, valid bits set to 1.
> > + *
> > + * Claim protocol requires 2 bits so test for highest bit required,
> > + * bit 1 -  CORESIGHT_CLAIM_SELF_HOSTED_BIT
> > + *
> > + * return true if sufficient claim tags implemented for protocol
> > + */
> > +static bool coresight_claim_tags_implemented_unlocked(struct csdev_access *csa)
> > +{
> > +     u32 claim_bits_impl = FIELD_GET(CORESIGHT_CLAIM_BITS_MAX_MASK,
> > +                      csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
> > +     return ((claim_bits_impl & CORESIGHT_CLAIM_SELF_HOSTED_BIT) != 0);
> > +}
> > +
>
> The logic looks good to me.
>
> Can we improve a bit with a cached flag to avoid every time checking the
> CLAIMSET, we can read it only once at init time instead.
>

When I considered how often we use the claim tags - compared to all
the other programming of devices, a single extra read did not seem
excessive.

> - We can add a new flag ("bool claim_impl" in the struct csdev_access),
>   by default the field will be zero.
>

I considered a bool - but the correct place for this would be
coresight_device - where we keep all the information about the
hardware features.

> - The drivers support claim tags call coresight_clear_self_claim_tag()
>   in probe (see __catu_probe() as an example), we can call a new
>   function coresight_init_claim_tag() to replace it, this function sets
>   "claim_impl" properly and clear the tag if supported.
>

I considered moving this initialisation to the common coresight code
where we create the coresight_device. Certainly with the new check for
number of claim tags it is safe for all devices, and there would
appear to be no reason to do it as early as it appears in some of the
drivers. However that would be a larger change.

> - Afterwards, simply check the "claim_impl" flag.


>
> [...]
>
> > +/*
> > + * Coresight specification defines a maximum of 8 claim tag bits.
> > + * The precise number is implementation defined, and may be obtained by
> > + * reading the CLAIMSET register.
> > + */
> > +#define CORESIGHT_CLAIM_BITS_MAX_MASK        GENMASK(7, 0)
> > +#define CORESIGHT_CLAIM_SELF_HOSTED_BIT      BIT(1)
>
> Now we only care about the self-host bit. Can reuse existed macros ?
>

I feel that drivers should be written to match the specification - the
macros above are a correct mask value per specification and the
correct bitfield comparison for the MSBit of the protocol.

The ones below are a protocol masks field, and a specific protocol
value that is used in value rather than bit comparisons in the
claimtag code. I felt it clearer to differentiate between the uses
when reading the code.

>   CORESIGHT_CLAIM_MASK
>   CORESIGHT_CLAIM_SELF_HOSTED
>
> I don't mind if extend CORESIGHT_CLAIM_MASK to 8 bits and even remove
> CORESIGHT_CLAIM_INVALID (as it cannot cover other invalid values). Or we
> can defer the extension as needed later.
>

The invalid value is used to as a value comparison to check for a race
condition with both protocol bits set.

Thanks

Mike


> Thanks,
> Leo



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH 1/1] coresight: fix issue where coresight component has no claimtags

[Leo Yan]

Hi Mike,

On Wed, Oct 22, 2025 at 01:35:46PM +0100, Mike Leach wrote:

[...]

> > - We can add a new flag ("bool claim_impl" in the struct csdev_access),
> >   by default the field will be zero.
> >
> 
> I considered a bool - but the correct place for this would be
> coresight_device - where we keep all the information about the
> hardware features.

Maybe coresight_device is suitable. But this might require to update
furthermore for claim function arguments (e.g., need to pass "csdev"
rather than "csdev_access").

> > - The drivers support claim tags call coresight_clear_self_claim_tag()
> >   in probe (see __catu_probe() as an example), we can call a new
> >   function coresight_init_claim_tag() to replace it, this function sets
> >   "claim_impl" properly and clear the tag if supported.
> >
> 
> I considered moving this initialisation to the common coresight code
> where we create the coresight_device.

Seems to me, this is dangerous. If a module is not CoreSight compliant
and claim registers are absent, when access we will get unknown values
or even cause serious result (external abort or bus lockup).

[...]

> > > +/*
> > > + * Coresight specification defines a maximum of 8 claim tag bits.
> > > + * The precise number is implementation defined, and may be obtained by
> > > + * reading the CLAIMSET register.
> > > + */
> > > +#define CORESIGHT_CLAIM_BITS_MAX_MASK        GENMASK(7, 0)
> > > +#define CORESIGHT_CLAIM_SELF_HOSTED_BIT      BIT(1)
> >
> > Now we only care about the self-host bit. Can reuse existed macros ?
> >
> 
> I feel that drivers should be written to match the specification - the
> macros above are a correct mask value per specification and the
> correct bitfield comparison for the MSBit of the protocol.
> 
> The ones below are a protocol masks field, and a specific protocol
> value that is used in value rather than bit comparisons in the
> claimtag code. I felt it clearer to differentiate between the uses
> when reading the code.

How about change CORESIGHT_CLAIM_EXTERNAL and
CORESIGHT_CLAIM_SELF_HOSTED to MSBit ?

Thanks,
Leo

Re: [PATCH 1/1] coresight: fix issue where coresight component has no claimtags

[Mike Leach]

Hi Leo

On Wed, 22 Oct 2025 at 15:04, Leo Yan <leo.yan@arm.com> wrote:
>
> Hi Mike,
>
> On Wed, Oct 22, 2025 at 01:35:46PM +0100, Mike Leach wrote:
>
> [...]
>
> > > - We can add a new flag ("bool claim_impl" in the struct csdev_access),
> > >   by default the field will be zero.
> > >
> >
> > I considered a bool - but the correct place for this would be
> > coresight_device - where we keep all the information about the
> > hardware features.
>
> Maybe coresight_device is suitable. But this might require to update
> furthermore for claim function arguments (e.g., need to pass "csdev"
> rather than "csdev_access").
>
> > > - The drivers support claim tags call coresight_clear_self_claim_tag()
> > >   in probe (see __catu_probe() as an example), we can call a new
> > >   function coresight_init_claim_tag() to replace it, this function sets
> > >   "claim_impl" properly and clear the tag if supported.
> > >
> >
> > I considered moving this initialisation to the common coresight code
> > where we create the coresight_device.
>
> Seems to me, this is dangerous. If a module is not CoreSight compliant
> and claim registers are absent, when access we will get unknown values
> or even cause serious result (external abort or bus lockup).
>

Currently all our drivers assume claim registers are compliant - so
this makes no functional difference to the current situation.

If the hardware is non-compliant, and is bound to the current drivers,
then there will be an access to the registers that could cause the
same issues.

There is little sense in having a call in individual drivers to init
claim tags which are regarded as common management structure.
Any coresight component with no claim tags must still either have
registers indicating no claim tags or as per the specification
unimplemented register locations have to be  RAZ/WI - which amounts to
the same thing.





> [...]
>
> > > > +/*
> > > > + * Coresight specification defines a maximum of 8 claim tag bits.
> > > > + * The precise number is implementation defined, and may be obtained by
> > > > + * reading the CLAIMSET register.
> > > > + */
> > > > +#define CORESIGHT_CLAIM_BITS_MAX_MASK        GENMASK(7, 0)
> > > > +#define CORESIGHT_CLAIM_SELF_HOSTED_BIT      BIT(1)
> > >
> > > Now we only care about the self-host bit. Can reuse existed macros ?
> > >
> >
> > I feel that drivers should be written to match the specification - the
> > macros above are a correct mask value per specification and the
> > correct bitfield comparison for the MSBit of the protocol.
> >
> > The ones below are a protocol masks field, and a specific protocol
> > value that is used in value rather than bit comparisons in the
> > claimtag code. I felt it clearer to differentiate between the uses
> > when reading the code.
>
> How about change CORESIGHT_CLAIM_EXTERNAL and
> CORESIGHT_CLAIM_SELF_HOSTED to MSBit ?
>

The checking code for the protocol does a value compare on the pair of
bits, not individual bit checks.

Regards


Mike


> Thanks,
> Leo



--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH 1/1] coresight: fix issue where coresight component has no claimtags

[Mike Leach]

Hi Leo

On Thu, 23 Oct 2025 at 11:18, Mike Leach <mike.leach@linaro.org> wrote:
>
> Hi Leo
>
> On Wed, 22 Oct 2025 at 15:04, Leo Yan <leo.yan@arm.com> wrote:
> >
> > Hi Mike,
> >
> > On Wed, Oct 22, 2025 at 01:35:46PM +0100, Mike Leach wrote:
> >
> > [...]
> >
> > > > - We can add a new flag ("bool claim_impl" in the struct csdev_access),
> > > >   by default the field will be zero.
> > > >
> > >
> > > I considered a bool - but the correct place for this would be
> > > coresight_device - where we keep all the information about the
> > > hardware features.
> >
> > Maybe coresight_device is suitable. But this might require to update
> > furthermore for claim function arguments (e.g., need to pass "csdev"
> > rather than "csdev_access").
> >
> > > > - The drivers support claim tags call coresight_clear_self_claim_tag()
> > > >   in probe (see __catu_probe() as an example), we can call a new
> > > >   function coresight_init_claim_tag() to replace it, this function sets
> > > >   "claim_impl" properly and clear the tag if supported.
> > > >
> > >
> > > I considered moving this initialisation to the common coresight code
> > > where we create the coresight_device.
> >
> > Seems to me, this is dangerous. If a module is not CoreSight compliant
> > and claim registers are absent, when access we will get unknown values
> > or even cause serious result (external abort or bus lockup).
> >
>
> Currently all our drivers assume claim registers are compliant - so
> this makes no functional difference to the current situation.
>
> If the hardware is non-compliant, and is bound to the current drivers,
> then there will be an access to the registers that could cause the
> same issues.
>
> There is little sense in having a call in individual drivers to init
> claim tags which are regarded as common management structure.
> Any coresight component with no claim tags must still either have
> registers indicating no claim tags or as per the specification
> unimplemented register locations have to be  RAZ/WI - which amounts to
> the same thing.
>
>

Having re-visited this - the cpu bound ETMs use a smp call before
creating the coresight_device so it would be impractical to move this
into the common code or use csdev parameters to make the checks.

The only time we claim  / disclaim is during enable / disable device
when we are writing many 10s of registers anyway. It does not seem too
much of  a performance hit to add one additional read per enable

Regards

Mike

>
>
>
> > [...]
> >
> > > > > +/*
> > > > > + * Coresight specification defines a maximum of 8 claim tag bits.
> > > > > + * The precise number is implementation defined, and may be obtained by
> > > > > + * reading the CLAIMSET register.
> > > > > + */
> > > > > +#define CORESIGHT_CLAIM_BITS_MAX_MASK        GENMASK(7, 0)
> > > > > +#define CORESIGHT_CLAIM_SELF_HOSTED_BIT      BIT(1)
> > > >
> > > > Now we only care about the self-host bit. Can reuse existed macros ?
> > > >
> > >
> > > I feel that drivers should be written to match the specification - the
> > > macros above are a correct mask value per specification and the
> > > correct bitfield comparison for the MSBit of the protocol.
> > >
> > > The ones below are a protocol masks field, and a specific protocol
> > > value that is used in value rather than bit comparisons in the
> > > claimtag code. I felt it clearer to differentiate between the uses
> > > when reading the code.
> >
> > How about change CORESIGHT_CLAIM_EXTERNAL and
> > CORESIGHT_CLAIM_SELF_HOSTED to MSBit ?
> >
>
> The checking code for the protocol does a value compare on the pair of
> bits, not individual bit checks.
>
> Regards
>
>
> Mike
>
>
> > Thanks,
> > Leo
>
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK