| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251113173617.GA3568724@e132581.arm.com>
Date: Thu, 13 Nov 2025 17:36:17 +0000
From: Leo Yan <leo.yan@....com>
To: Mike Leach <mike.leach@...aro.org>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, suzuki.poulose@....com,
james.clark@...aro.org
Subject: Re: [PATCH v2 1/1] coresight: fix issue where coresight component
has no claimtags
On Mon, Oct 27, 2025 at 10:35:45PM +0000, Mike Leach wrote:
> 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@...aro.org>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 46 ++++++++++++++++++--
> drivers/hwtracing/coresight/coresight-priv.h | 10 +++++
> include/linux/coresight.h | 15 +++++++
> 3 files changed, 68 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 3267192f0c1c..61b4902b0ead 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -131,6 +131,39 @@ coresight_find_out_connection(struct coresight_device *csdev,
> return ERR_PTR(-ENODEV);
> }
>
> +/*
> + * Reading CLAiMSET returns a bitfield representing the number of claim tags
s/CLAiMSET/CLAIMSET
> + * implemented from bit 0 to bit nTag-1, valid bits set to 1.
> + *
> + * Claim protocol requires 2 bits so test for MS bit required,
> + * bit 1 - CORESIGHT_CLAIM_BIT_PROTOCOL_HI
> + *
> + * 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_BIT_PROTOCOL_HI) != 0);
How about a simple one?
return !!FIELD_GET(CORESIGHT_CLAIM_BITS_MAX_MASK,
csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
> +}
> +
> +/*
> + * check if we can use the claim tag protocol.
> + *
> + * If currently unknown - read register to determine if sufficient tags and
> + * save value, otherwise return current value.
> + */
> +static bool coresight_check_use_claim_tag_unlocked(struct coresight_device *csdev)
> +{
> + if (csdev->claim_tag_info == CS_CLAIM_TAG_UNKNOWN) {
> + if (coresight_claim_tags_implemented_unlocked(&csdev->access))
> + csdev->claim_tag_info = CS_CLAIM_TAG_STD_PROTOCOL;
> + else
> + csdev->claim_tag_info = CS_CLAIM_TAG_NOT_IMPL;
> + }
> + return (csdev->claim_tag_info == CS_CLAIM_TAG_STD_PROTOCOL);
> +}
> +
> static u32 coresight_read_claim_tags_unlocked(struct coresight_device *csdev)
> {
> return FIELD_GET(CORESIGHT_CLAIM_MASK,
Why do not apply claim tag implemented check in
coresight_read_claim_tags_unlocked() ?
> @@ -156,6 +189,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;
Here it does not call coresight_check_use_claim_tag_unlocked(), which
means the cached flag is not used. I think we should be consistent
across all claim tag functions.
This is why I suggested to put the flag into csdev_access rather than
in csdev. Seems to me, it is nature to use csdev_access to store this
flag, as it presents a attribute for register access (we can consider
csdev presents a device on bus).
If agree with above, then we can create a new function:
void coresight_init_self_claim_tag(struct csdev_access *csa)
{
csa->claim_tag_implemented = !!FIELD_GET(CORESIGHT_CLAIM_BITS_MAX_MASK,
csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
if (csa->claim_tag_implemented)
coresight_clear_self_claim_tag(csa);
}
This function is called by probe()s in CoreSight drivers, afterwards,
simply check the flag "csa->claim_tag_implemented".
A minor benefit is we can drop coresight_check_use_claim_tag_unlocked()
and the enum coresight_claim_tag_info, as we just need to initialize
csa->claim_tag_implemented at the init phase.
> +
> csdev_access_relaxed_write32(csa, CORESIGHT_CLAIM_SELF_HOSTED,
> CORESIGHT_CLAIMCLR);
> isb();
> @@ -175,12 +211,13 @@ EXPORT_SYMBOL_GPL(coresight_clear_self_claim_tag_unlocked);
> int coresight_claim_device_unlocked(struct coresight_device *csdev)
> {
> int tag;
> - struct csdev_access *csa;
>
> if (WARN_ON(!csdev))
> return -EINVAL;
>
> - csa = &csdev->access;
> + if (!coresight_check_use_claim_tag_unlocked(csdev))
> + return 0;
> +
> tag = coresight_read_claim_tags_unlocked(csdev);
>
> switch (tag) {
> @@ -190,7 +227,7 @@ int coresight_claim_device_unlocked(struct coresight_device *csdev)
> return 0;
>
> /* There was a race setting the tag, clean up and fail */
> - coresight_clear_self_claim_tag_unlocked(csa);
> + coresight_clear_self_claim_tag_unlocked(&csdev->access);
> dev_dbg(&csdev->dev, "Busy: Couldn't set self claim tag");
> return -EBUSY;
>
> @@ -237,6 +274,9 @@ void coresight_disclaim_device_unlocked(struct coresight_device *csdev)
> if (WARN_ON(!csdev))
> return;
>
> + if (!coresight_check_use_claim_tag_unlocked(csdev))
> + return;
If use the method mentioned, no need to spread
coresight_check_use_claim_tag_unlocked() in claim and disclaim functions.
Thanks,
Leo
> +
> 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..270f179c8535 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -41,6 +41,16 @@ 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)
> +
> +/* MS Bit required by the protocol */
> +#define CORESIGHT_CLAIM_BIT_PROTOCOL_HI BIT(1)
> +
> #define TIMEOUT_US 100
> #define BMVAL(val, lsb, msb) ((val & GENMASK(msb, lsb)) >> lsb)
>
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 6de59ce8ef8c..fefe4317a793 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -242,6 +242,19 @@ struct coresight_trace_id_map {
> raw_spinlock_t lock;
> };
>
> +/*
> + * Coresight claim tag info:
> + * CS_CLAIM_TAG_UNKNOWN - not yet checked.
> + * CS_CLAIM_TAG_STD_PROTOCOL - using standard claim/release protocol.
> + * CS_CLAIM_TAG_NOT_IMPL - no claim tags available.
> + */
> +enum coresight_claim_tag_info {
> + CS_CLAIM_TAG_UNKNOWN,
> + CS_CLAIM_TAG_STD_PROTOCOL,
> + CS_CLAIM_TAG_NOT_IMPL,
> +};
> +
> +
> /**
> * struct coresight_device - representation of a device as used by the framework
> * @pdata: Platform data with device connections associated to this device.
> @@ -265,6 +278,7 @@ struct coresight_trace_id_map {
> * CS_MODE_SYSFS. Otherwise it must be accessed from inside the
> * spinlock.
> * @orphan: true if the component has connections that haven't been linked.
> + * @claim_tag_info: how the device is using claim tags.
> * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
> * by writing a 1 to the 'enable_sink' file. A sink can be
> * activated but not yet enabled. Enabling for a _sink_ happens
> @@ -291,6 +305,7 @@ struct coresight_device {
> local_t mode;
> int refcnt;
> bool orphan;
> + enum coresight_claim_tag_info claim_tag_info;
> /* sink specific fields */
> bool sysfs_sink_activated;
> struct dev_ext_attribute *ea;
> --
> 2.32.0
>
Powered by blists - more mailing lists