Hi Levi,
On Mon, Mar 24, 2025 at 07:17:40PM +0000, Yeoreum Yun wrote:
> While enable active config via cscfg_csdev_enable_active_config(),
> active config could be deactivated via configfs' sysfs interface.
> This could make UAF issue in below scenario:
>
> CPU0 CPU1
> (sysfs enable) load module
> cscfg_load_config_sets()
> activate config. // sysfs
> (sys_active_cnt == 1)
> ...
> cscfg_csdev_enable_active_config()
> lock(csdev->cscfg_csdev_lock)
> // here load config activate by CPU1
> unlock(csdev->cscfg_csdev_lock)
>
> deactivate config // sysfs
> (sys_activec_cnt == 0)
> cscfg_unload_config_sets()
> unload module
>
> // access to config_desc which freed
> // while unloading module.
> cfs_csdev_enable_config
I am not sure if this flow can happen. CoreSight configfs feature is
integrated into the CoreSight core layer, and the other CoreSight
modules are dependent on it.
For example, if the ETM4x module is not removed, the kernel module
management will natually prevent the CoreSight core module from being
removed.
> To address this, use cscfg_config_desc's active_cnt as a reference count
> which will be holded when
> - activate the config.
> - enable the activated config.
> and put the module reference when config_active_cnt == 0.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com>
> ---
> Since v3:
> - Remove enable arguments in cscfg_config_desc_get() (from Mike).
> - https://lore.kernel.org/all/20250109171956.3535294-1-yeoreum.yun@arm.com/
> ---
> .../hwtracing/coresight/coresight-config.h | 2 +-
> .../coresight/coresight-etm4x-core.c | 3 ++
> .../hwtracing/coresight/coresight-syscfg.c | 52 +++++++++++++------
> 3 files changed, 41 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index b9ebc9fcfb7f..90fd937d3bd8 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
> * @feats_csdev:references to the device features to enable.
> */
> struct cscfg_config_csdev {
> - const struct cscfg_config_desc *config_desc;
> + struct cscfg_config_desc *config_desc;
> struct coresight_device *csdev;
> bool enabled;
> struct list_head node;
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e5972f16abff..ef96028fa56b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
>
> raw_spin_unlock(&drvdata->spinlock);
> +
> + cscfg_csdev_disable_active_config(csdev);
> +
In general, we need to split changes into several patches if each
addresses a different issue. From my understanding, the change above is
to fix missing to disable config when disable Sysfs mode.
If so, could we use a seperate patch for this change?
> cpus_read_unlock();
>
> /*
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index a70c1454b410..6d8c212ad434 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
> static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> {
> struct cscfg_config_csdev *config_csdev, *tmp;
> + unsigned long flags;
>
> if (list_empty(&csdev->config_csdev_list))
> return;
>
> + raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
I think we should use spinlock to guard the condition checking
list_empty().
Here the race condition is the 'config_csdev_list' list and
configurations on the list. For atomicity, we should use lock to
protect any operations on the list (read, add, delete, etc).
A side topic, as here it adds locks for protecting 'config_csdev_list',
I am wandering why we do not do the same thing for
'feature_csdev_list' (See cscfg_remove_owned_csdev_features() and
cscfg_get_feat_csdev()).
> list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
> if (config_csdev->config_desc->load_owner == load_owner)
> list_del(&config_csdev->node);
> }
> + raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> }
>
> static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> @@ -867,6 +870,25 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
>
> +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc)
> +{
> + if (!atomic_fetch_inc(&config_desc->active_cnt)) {
> + /* must ensure that config cannot be unloaded in use */
> + if (unlikely(cscfg_owner_get(config_desc->load_owner))) {
> + atomic_dec(&config_desc->active_cnt);
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> +static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
> +{
> + if (!atomic_dec_return(&config_desc->active_cnt))
> + cscfg_owner_put(config_desc->load_owner);
> +}
> +
> /*
> * This activate configuration for either perf or sysfs. Perf can have multiple
> * active configs, selected per event, sysfs is limited to one.
> @@ -890,22 +912,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
> if (config_desc->available == false)
> return -EBUSY;
>
> - /* must ensure that config cannot be unloaded in use */
> - err = cscfg_owner_get(config_desc->load_owner);
> - if (err)
> + if (!cscfg_config_desc_get(config_desc)) {
> + err = -EINVAL;
> break;
> + }
> +
> /*
> * increment the global active count - control changes to
> * active configurations
> */
> atomic_inc(&cscfg_mgr->sys_active_cnt);
Seems to me, it is more reasonable to use 'sys_active_cnt' to acquire
the module reference instead of 'config_desc->active_cnt'. The reason
is 'sys_active_cnt' is a global counter.
> - /*
> - * mark the descriptor as active so enable config on a
> - * device instance will use it
> - */
> - atomic_inc(&config_desc->active_cnt);
> -
> err = 0;
> dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
> break;
> @@ -920,9 +937,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
>
> list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> - atomic_dec(&config_desc->active_cnt);
> atomic_dec(&cscfg_mgr->sys_active_cnt);
> - cscfg_owner_put(config_desc->load_owner);
> + cscfg_config_desc_put(config_desc);
> dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> break;
> }
> @@ -1047,7 +1063,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> unsigned long cfg_hash, int preset)
> {
> struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> - const struct cscfg_config_desc *config_desc;
> + struct cscfg_config_desc *config_desc;
> unsigned long flags;
> int err = 0;
>
> @@ -1062,8 +1078,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> config_desc = config_csdev_item->config_desc;
> - if ((atomic_read(&config_desc->active_cnt)) &&
> - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> + cscfg_config_desc_get(config_desc)) {
This seems to me not right. Why a config descriptor is get in multiple
places? One time getting a config descriptor is in
_cscfg_activate_config(), another is at here.
To be honest, I am not clear what is the difference between 'activate'
config and 'enable active' config. Literally, I think we only need to
acquire the config at its creating phase (maybe match to activate
config?).
> config_csdev_active = config_csdev_item;
> csdev->active_cscfg_ctxt = (void *)config_csdev_active;
> break;
> @@ -1097,7 +1113,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> err = -EBUSY;
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> }
> +
> + if (err)
> + cscfg_config_desc_put(config_desc);
> }
> +
> return err;
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> @@ -1136,8 +1156,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
>
> /* true if there was an enabled active config */
> - if (config_csdev)
> + if (config_csdev) {
> cscfg_csdev_disable_config(config_csdev);
> + cscfg_config_desc_put(config_csdev->config_desc);
> + }
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
>
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>
I've gotten stuck a few times with unusable Coresight after a warm boot
due to lingering claim tags, especially when testing the Coresight
panic patchsets.
This change does some tidy ups, adds some debug messages and clears the
self hosted claim tag on probe. The last two commits are unrelated
tidyups but they touch some of the same functions so to avoid extra
conflicts I'm including them here.
This gets as far as fixing the claim tag issue, but there is some other
state not being cleared on probe that results in the following error.
This can be fixed up as a later change:
coresight tmc_etf0: timeout while waiting for TMC to be Ready
coresight tmc_etf0: Failed to enable : TMC is not ready
Changes in v3:
- Collapse rename and locked/unlocked addition commits of
coresight_clear_self_claim_tag() so we don't change the name twice.
- Make coresight_clear_self_claim_tag() a bit more generic by only
doing UNLOCK for MMIO devices (although there is no use of this right
now)
- Link to v2: https://lore.kernel.org/r/20250318-james-coresight-claim-tags-v2-0-e9c8a9cd…
Changes in v2:
* Revert most of the interface changes, just call
coresight_clear_self_claim_tag() directly. This is possible because
we're not doing the read first, so it has fewer knock on effects.
* Split out the change to add struct cs_access to etm3x
* Add another warning for racing with external debugger
--
2.34.1
---
James Clark (7):
coresight: Convert tag clear function to take a struct cs_access
coresight: Only check bottom two claim bits
coresight: Add claim tag warnings and debug messages
coresight: etm3x: Convert raw base pointer to struct coresight access
coresight: Clear self hosted claim tag on probe
coresight: Remove inlines from static function definitions
coresight: Remove extern from function declarations
drivers/hwtracing/coresight/coresight-catu.c | 12 +--
drivers/hwtracing/coresight/coresight-core.c | 87 ++++++++++++++--------
drivers/hwtracing/coresight/coresight-cti-core.c | 2 +
drivers/hwtracing/coresight/coresight-etb10.c | 4 +-
drivers/hwtracing/coresight/coresight-etm.h | 6 +-
drivers/hwtracing/coresight/coresight-etm3x-core.c | 28 +++----
.../hwtracing/coresight/coresight-etm3x-sysfs.c | 8 +-
drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++-
.../hwtracing/coresight/coresight-etm4x-sysfs.c | 4 +-
drivers/hwtracing/coresight/coresight-funnel.c | 1 +
drivers/hwtracing/coresight/coresight-platform.c | 26 +++----
drivers/hwtracing/coresight/coresight-priv.h | 20 ++---
drivers/hwtracing/coresight/coresight-replicator.c | 3 +-
drivers/hwtracing/coresight/coresight-stm.c | 6 +-
.../coresight/coresight-syscfg-configfs.c | 2 +-
drivers/hwtracing/coresight/coresight-tmc-core.c | 9 ++-
drivers/hwtracing/coresight/coresight-tmc-etr.c | 16 ++--
drivers/hwtracing/coresight/coresight-trbe.c | 18 ++---
include/linux/coresight.h | 40 +++++-----
19 files changed, 168 insertions(+), 134 deletions(-)
---
base-commit: 5442d22da7dbff3ba8c6720fc6f23ea4934d402d
change-id: 20250317-james-coresight-claim-tags-ae1461f1f5e0
Best regards,
--
James Clark <james.clark(a)linaro.org>
I've gotten stuck a few times with unusable Coresight after a warm boot
due to lingering claim tags, especially when testing the Coresight
panic patchsets.
This change does some tidy ups, adds some debug messages and clears the
self hosted claim tag on probe. The last two commits are unrelated
tidyups but they touch some of the same functions so to avoid extra
conflicts I'm including them here.
This gets as far as fixing the claim tag issue, but there is some other
state not being cleared on probe that results in the following error.
This can be fixed up as a later change:
coresight tmc_etf0: timeout while waiting for TMC to be Ready
coresight tmc_etf0: Failed to enable : TMC is not ready
Changes in v4:
- Add _unlocked() suffix for consistency
- s/cs_access/csdev_access/
- Link to v3: https://lore.kernel.org/r/20250320-james-coresight-claim-tags-v3-0-d3145c15…
Changes in v3:
- Collapse rename and locked/unlocked addition commits of
coresight_clear_self_claim_tag() so we don't change the name twice.
- Make coresight_clear_self_claim_tag() a bit more generic by only
doing UNLOCK for MMIO devices (although there is no use of this right
now)
- Link to v2: https://lore.kernel.org/r/20250318-james-coresight-claim-tags-v2-0-e9c8a9cd…
Changes in v2:
* Revert most of the interface changes, just call
coresight_clear_self_claim_tag() directly. This is possible because
we're not doing the read first, so it has fewer knock on effects.
* Split out the change to add struct cs_access to etm3x
* Add another warning for racing with external debugger
--
2.34.1
---
James Clark (7):
coresight: Convert tag clear function to take a struct csdev_access
coresight: Only check bottom two claim bits
coresight: Add claim tag warnings and debug messages
coresight: etm3x: Convert raw base pointer to struct coresight access
coresight: Clear self hosted claim tag on probe
coresight: Remove inlines from static function definitions
coresight: Remove extern from function declarations
drivers/hwtracing/coresight/coresight-catu.c | 12 +--
drivers/hwtracing/coresight/coresight-core.c | 87 ++++++++++++++--------
drivers/hwtracing/coresight/coresight-cti-core.c | 2 +
drivers/hwtracing/coresight/coresight-etb10.c | 4 +-
drivers/hwtracing/coresight/coresight-etm.h | 6 +-
drivers/hwtracing/coresight/coresight-etm3x-core.c | 28 +++----
.../hwtracing/coresight/coresight-etm3x-sysfs.c | 8 +-
drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++-
.../hwtracing/coresight/coresight-etm4x-sysfs.c | 4 +-
drivers/hwtracing/coresight/coresight-funnel.c | 1 +
drivers/hwtracing/coresight/coresight-platform.c | 26 +++----
drivers/hwtracing/coresight/coresight-priv.h | 20 ++---
drivers/hwtracing/coresight/coresight-replicator.c | 3 +-
drivers/hwtracing/coresight/coresight-stm.c | 6 +-
.../coresight/coresight-syscfg-configfs.c | 2 +-
drivers/hwtracing/coresight/coresight-tmc-core.c | 9 ++-
drivers/hwtracing/coresight/coresight-tmc-etr.c | 16 ++--
drivers/hwtracing/coresight/coresight-trbe.c | 18 ++---
include/linux/coresight.h | 40 +++++-----
19 files changed, 168 insertions(+), 134 deletions(-)
---
base-commit: 5442d22da7dbff3ba8c6720fc6f23ea4934d402d
change-id: 20250317-james-coresight-claim-tags-ae1461f1f5e0
Best regards,
--
James Clark <james.clark(a)linaro.org>
Hi,
On Mon, 24 Mar 2025 at 13:59, Yeoreum Yun <yeoreum.yun(a)arm.com> wrote:
>
> Hi Mike,
>
> Please ignore my foremer mail.. and please see my comments for your
> suggestion.
>
> > Hi
> >
> > On Fri, 14 Mar 2025 at 15:25, Yeo Reum Yun <YeoReum.Yun(a)arm.com> wrote:
> > >
> > > Hi, Mike.
> > >
> > > > > static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > > > > @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> > > > >
> > > > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> > > > > +{
> > > > > + if (enable)
> > > > > + return atomic_inc_not_zero(&config_desc->active_cnt);
> > > > > +
> > > >
> > > > Not sure why we have an "enable" parameter here - it completely
> > > > changes the meaning of the function - with no comment at the start.
> > >
> > > Sorry. But what I intended is to distinguish
> > > - activation of config
> > > - enable of activated config.
> > > Because, current coresight doesn't grab the module reference on enable of activate config,
> > > But It grabs that reference only in activation.
> > > That's why I used to "enable" parameter to distinguish this
> > > while I integrate with module_owner count.
> > >
> > > > > list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> > > > > if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > > > > - atomic_dec(&config_desc->active_cnt);
> > > > > atomic_dec(&cscfg_mgr->sys_active_cnt);
> > > > > - cscfg_owner_put(config_desc->load_owner);
> > > > > + cscfg_config_desc_put(config_desc);
> > > > > dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> > > > > break;
> > > > > }
> > > > > @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > > unsigned long cfg_hash, int preset)
> > > > > {
> > > > > struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > > > > - const struct cscfg_config_desc *config_desc;
> > > > > + struct cscfg_config_desc *config_desc;
> > > > > unsigned long flags;
> > > > > int err = 0;
> > > > >
> > > > > @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > > raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > > > > list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> > > > > config_desc = config_csdev_item->config_desc;
> > > > > - if ((atomic_read(&config_desc->active_cnt)) &&
> > > > > - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > > > > + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > > > > + cscfg_config_desc_get(config_desc, true)) {
> > > > >
> > > > This obfuscates the logic of the comparisons without good reason. With
> > > > the true parameter, the function does no "get" operation but just
> > > > replicates the logic being replaced - checking the active_cnt is
> > > > non-zero.
> > > >
> > > > Restore this to the original logic to make it readable again
> > >
> > > It's not a replicates of comparsion logic, but if true,
> >
> > sorry - missed that point .
> >
> > > It get the reference of active_cnt but not get module reference.
> > > The fundemental fault in the UAF becase of just "atomic_read()"
> > > so, it should hold reference in here.
> > >
> > > So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation,
> > > I think there're two way to modfiy.
> > >
> > > 1. cscfg_config_desc_get()/put() always grab/drop the module count.
> > > 2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only
> > > with cscfg_owner_get()/put()
> > >
> > > Any thougt?
> > >
> > > Thanks!
> > >
> > >
> >
> > The get and put functions are asymmetrical w.r.t. owner.
> >
> > The put will put owner if active count decrements to 0,
> > The get if not on enable path will put owner unconditionally.
> >
> > This means that the caller has to work out the correct input conditions.
> >
> > Might be better if:-
> >
> > get_desc()
> > {
> > if (! desc->refcnt) {
> > if (!get_owner())
> > return false;
> > }
> > desc->refcnt++;
> > return true;
> > }
>
> I think This makes another problem when
> it races with _cscfg_deactivate_config().
>
> CPU0 CPU1
> (sysfs enable) load module
> cscfg_load_config_sets()
> activate config. // sysfs
> (sys_active_cnt == 1)
>
> // sysfs
> _cscfg_deactivate_config()
> (sys_active_cnt == 0)
> (config->active_cnt = 0)
> ...
> cscfg_csdev_enable_active_config()
> lock(csdev->cscfg_csdev_lock)
> // here get module reference??
> // even sys_active_cnt == 0 and
> // config->active_cnt == 1.
> get_desc()
> unlock(csdev->cscfg_csdev_lock)
>
>
> // access to config_desc which freed
> // while unloading module.
> cfs_csdev_enable_config
>
>
> Because, the desc->refcnt meaning of zero is different from the context.
> - while activate . it should get module reference if zero.
> - while enable active configuration, if zero, it should be failed.
>
> that means to prevent this race, the core key point is:
> when config->active_cnt == 0, it should be failed in cscfg_csdev_enable_active_config()
>
This is not a failure case, it simple means that this config should
not be activated for this device.
It is possible for a configuration to be active on the system, without
it being active for a particular coresight device.
Having a get/put interface for the config descriptor - which prevents
the config from being unloaded is fine, the key logic here is that we
are searching a list of possible enabled configurations for this
device and taking the necessary action to enable it if we find one -
and there can only ever be a single configuration enabled for a trace
session.
Therefore when the list of loaded configs for a device is > 1, then
all but one is allowed to be active - so the search code will validly
find instances where config->active_cnt == 0.
My objection to the original interface was not the get/put operations
to protect the module from unload, but the fact that the logic
deciding if a config needed to be enabled on the device was hidden
inside the get() operation.
My suggestion is to restore the logic that decides if there is a
config to enable on the device be clear in the enable function itself,
then use get/put as appropriate to prevent module unload.
Regards
Mike
> Because, according to context the handling the zero reference value is
> different, It seems,to integrate the get_desc() interface to handle
> above case together without extra arguments (in case of here is
> "enable").
>
> If this interface is really ugly and unhappy to you,
> I think It should remove get_desc()/put_desc().
> although we can add new interface for cscfg_config_desc_get() for enable
> path. but it makes people more confused.
>
> So my suggestion is:
> - sustain this patch's contents
> - or remove get_desc()/put_desc() interface but use
> atomic_inc_zero(&config_desc->active_cnt) directly in
> cscfg_csdev_enable_active_config()
>
> Any thougt?
>
> Thanks.
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Hi
On Fri, 14 Mar 2025 at 15:25, Yeo Reum Yun <YeoReum.Yun(a)arm.com> wrote:
>
> Hi, Mike.
>
> > > static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > > @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> > > }
> > > EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> > >
> > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> > > +{
> > > + if (enable)
> > > + return atomic_inc_not_zero(&config_desc->active_cnt);
> > > +
> >
> > Not sure why we have an "enable" parameter here - it completely
> > changes the meaning of the function - with no comment at the start.
>
> Sorry. But what I intended is to distinguish
> - activation of config
> - enable of activated config.
> Because, current coresight doesn't grab the module reference on enable of activate config,
> But It grabs that reference only in activation.
> That's why I used to "enable" parameter to distinguish this
> while I integrate with module_owner count.
>
> > > list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> > > if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > > - atomic_dec(&config_desc->active_cnt);
> > > atomic_dec(&cscfg_mgr->sys_active_cnt);
> > > - cscfg_owner_put(config_desc->load_owner);
> > > + cscfg_config_desc_put(config_desc);
> > > dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> > > break;
> > > }
> > > @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > unsigned long cfg_hash, int preset)
> > > {
> > > struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > > - const struct cscfg_config_desc *config_desc;
> > > + struct cscfg_config_desc *config_desc;
> > > unsigned long flags;
> > > int err = 0;
> > >
> > > @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > > list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> > > config_desc = config_csdev_item->config_desc;
> > > - if ((atomic_read(&config_desc->active_cnt)) &&
> > > - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > > + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > > + cscfg_config_desc_get(config_desc, true)) {
> > >
> > This obfuscates the logic of the comparisons without good reason. With
> > the true parameter, the function does no "get" operation but just
> > replicates the logic being replaced - checking the active_cnt is
> > non-zero.
> >
> > Restore this to the original logic to make it readable again
>
> It's not a replicates of comparsion logic, but if true,
sorry - missed that point .
> It get the reference of active_cnt but not get module reference.
> The fundemental fault in the UAF becase of just "atomic_read()"
> so, it should hold reference in here.
>
> So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation,
> I think there're two way to modfiy.
>
> 1. cscfg_config_desc_get()/put() always grab/drop the module count.
> 2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only
> with cscfg_owner_get()/put()
>
> Any thougt?
>
> Thanks!
>
>
The get and put functions are asymmetrical w.r.t. owner.
The put will put owner if active count decrements to 0,
The get if not on enable path will put owner unconditionally.
This means that the caller has to work out the correct input conditions.
Might be better if:-
get_desc()
{
if (! desc->refcnt) {
if (!get_owner())
return false;
}
desc->refcnt++;
return true;
}
put_desc()
{
desc->refcnt--;
if (! desc->refcnt)
put_owner()
}
and then change the enable_active_cfg matching logic to
if ( (desc->refcnt) && (desc->hash == hash) && get_desc()) {
< set active cfg>
}
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Hi,
On Fri, 21 Mar 2025 at 03:26, songchai <quic_songchai(a)quicinc.com> wrote:
>
>
> On 3/7/2025 12:57 AM, Mike Leach wrote:
> > Hi,
> >
> > On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai(a)quicinc.com> wrote:
> >> Provide support for the TGU (Trigger Generation Unit), which can be
> >> utilized to sense a plurality of signals and create a trigger into
> >> the CTI or generate interrupts to processors once the input signal
> >> meets the conditions. We can treat the TGU’s workflow as a flowsheet,
> >> it has some “steps” regions for customization. In each step region,
> >> we can set the signals that we want with priority in priority_group, set
> >> the conditions in each step via condition_decode, and set the resultant
> >> action by condition_select. Meanwhile, some TGUs (not all) also provide
> >> timer/counter functionality. Based on the characteristics described
> >> above, we consider the TGU as a helper in the CoreSight subsystem.
> >> Its master device is the TPDM, which can transmit signals from other
> >> subsystems, and we reuse the existing ports mechanism to link the TPDM to
> >> the connected TGU.
> >>
> > I do not believe that his component is part of the Coresight subsystem.
> >
> > 1) It inputs multiple signals from the SoC to process and create an
> > trigger event - however, it can do this irrespective of CoreSight
> > trace being operational, especially where generating interrupts for
> > processors, or triggers for other non-coresight components. It would
> > appear that the TPDM can send output to more than just the TDPA which
> > generates coresight trace packets - a previously undisclosed feature.
>
> TGU is a part the QPMDA(Qualcomm Performance Monitoring and
>
> Diagnostics Architecture ) library, and the signals it perceives are
> from TPDM and connected
>
> through hardware. So it depends on the coresight-tpdm, and will not
> work if the coresight
>
> tpdm is not operational.
>
As I understand it from reading many of the patches for this and other
of the components,
the QPMDA is a proprietary network of monitoring components across your SoCs,
that can have an endpoint that generates CoreSight trace (from the TPDA).
The nature of this network has not been specified - but it does seem
that the TDPM can output to more than one device - perhaps some sort
of internal bus runs between all the QPMDA devices.
Is it possible that the TDPM can be programmed to output to the TGU,
without trace being generated from the TPDA?
If you enable just the TPDM and TGU, can you program the TGU to output
triggers to CPU and other none-coresight devices, as implied in the
binding descriptions?
> >
> > 2) The ports mechanism is a generic device tree mechanism, not
> > something unique to the Coresight subsystem.
>
> Sure, Mike. As i mentioned above, the signals reach the TGU through
> hardware.So the port
>
> mechanism here is not for the data transmission, but to confirm the
> relationship of the TGU
>
> as a TPDM helper.
>
> >
> > 3) The CTI trigger connection will be defined in devicetree under the
> > CTI component, as this is the interface between this component and
> > coresight.
>
> The TGU is utilized to sense a plurality of signals and create a trigger
> into the CTI.
>
> It should be a trigger in for the targeting CTI. Could we configure the
> targeting CTI's trigger_in
>
> and trigger_out instead of configuring in the devicetree?
>
You would need to make additional device tree entries in the relevant
CTI for the input trigger from the TGU for the CTI driver to correctly
display the source of the trigger.
> >
> > As such this seems more like a general performance and debug
> > component, with optional inputs to the coresight trigger mechanisms,
> > rather than being a coresight component itself. Other SoCs have
> > non-coresight component inputs to CTIs. For example the PL011 serial
> > device on Juno has a signal into one of the system CTIs.
>
> In addition to above, the TGU also have the coresight management
> registers in its register region
>
> which is a character of coresight component.
>
These registers are not exposed in sysfs by your driver, nor are the
values known, so it is difficult to know if they correctly follow the
CoreSight 3.0 Architecture specification - ARM IHI 0029F; which
defines the identification and discovery requirements that all
CoreSight components must follow.
These appear to be missing from your other devices listed in the
coresight driver area - something we appear to have overlooked in
previous driver reviews.
The CTI / ETM and other ARM component coresight drivers all have a
sysfs "mgmt" section that exposes these management registers.
As I mentioned in my comments to the bindings in patch 1 - the
Coresight visible component architecture and ID registers should be
used to discover and identify the capabilities of components, such as
in this case the number of steps/timers/triggers/priorities - and not
need these values to be defined in the device tree. In this way, we
remove the possibility of errors in a device tree leading to driver
failure.
Regards
Mike
> Based on these clarification, could we consider it as a coresight
> component?
> >
> >> Here is a detailed example to explain how to use the TGU:
> >>
> >> In this example, the TGU is configured to use 2 conditions, 2 steps, and
> >> the timer. The goal is to look for one of two patterns which are generated
> >> from TPDM, giving priority to one, and then generate a trigger once the
> >> timer reaches a certain value. In other words, two conditions are used
> >> for the first step to look for the two patterns, where the one with the
> >> highest priority is used in the first condition. Then, in the second step,
> >> the timer is enabled and set to be compared to the given value at each
> >> clock cycle. These steps are better shown below.
> >>
> >>
> >> |-----------------|
> >> | |
> >> | TPDM |
> >> | |
> >> |-----------------|
> >> |
> >> |
> >> --- --- --- --- --- --- --- --- --- --- --- --- --- --- --- --- ------
> >> | | |
> >> | | |--------------------| |
> >> | |---- ---> | | Go to next steps | |
> >> | | | |--- ---> | Enable timer | |
> >> | | v | | | |
> >> | | |-----------------| | |--------------------| |
> >> | | | | Yes | | |
> >> | | | inputs==0xB | ----->| | <-------- |
> >> | | | | | | No | |
> >> | No | |-----------------| | v | |
> >> | | | | |-----------------| | |
> >> | | | | | | | |
> >> | | | | | timer>=3 |-- |
> >> | | v | | | |
> >> | | |-----------------| | |-----------------| |
> >> | | | | Yes | | |
> >> | |--- | inputs==0xA | ----->| | Yes |
> >> | | | | |
> >> | |-----------------| v |
> >> | |-----------------| |
> >> | | | |
> >> | | Trigger | |
> >> | | | |
> >> | |-----------------| |
> >> | TGU | |
> >> |--- --- --- --- --- --- --- --- --- --- --- --- --- --- |--- --- -- |
> >> |
> >> v
> >> |-----------------|
> >> |The controllers |
> >> |which will use |
> >> |triggers further |
> >> |-----------------|
> >>
> >> steps:
> >> 1. Reset TGU /*it will disable tgu and reset dataset*/
> >> - echo 1 > /sys/bus/coresight/devices/<tgu-name>/reset_tgu
> >>
> >> 2. Set the pattern match for priority0 to 0xA = 0b1010 and for
> >> priority 1 to 0xB = 0b1011.
> >> - echo 0x11113232 > /sys/bus/coresight/devices/<tgu-name>/step0_priority0/reg0
> >> - echo 0x11113233 > /sys/bus/coresight/devices/<tgu-name>/step0_priority1/reg0
> >>
> >> Note:
> >> Bit distribution diagram for each priority register
> >> |-------------------------------------------------------------------|
> >> | Bits | Field Nam | Description |
> >> |-------------------------------------------------------------------|
> >> | | | 00 = bypass for OR output |
> >> | 29:28 | SEL_BIT7_TYPE2 | 01 = bypass for AND output |
> >> | | | 10 = sense input '0' is true|
> >> | | | 11 = sense input '1' is true|
> >> |-------------------------------------------------------------------|
> >> | | | 00 = bypass for OR output |
> >> | 25:24 | SEL_BIT6_TYPE2 | 01 = bypass for AND output |
> >> | | | 10 = sense input '0' is true|
> >> | | | 11 = sense input '1' is true|
> >> |-------------------------------------------------------------------|
> >> | | | 00 = bypass for OR output |
> >> | 21:20 | SEL_BIT5_TYPE2 | 01 = bypass for AND output |
> >> | | | 10 = sense input '0' is true|
> >> | | | 11 = sense input '1' is true|
> >> |-------------------------------------------------------------------|
> >> | | | 00 = bypass for OR output |
> >> | 17:16 | SEL_BIT4_TYPE2 | 01 = bypass for AND output |
> >> | | | 10 = sense input '0' is true|
> >> | | | 11 = sense input '1' is true|
> >> |-------------------------------------------------------------------|
> >> | | | 00 = bypass for OR output |
> >> | 13:12 | SEL_BIT3_TYPE2 | 01 = bypass for AND output |
> >> | | | 10 = sense input '0' is true|
> >> | | | 11 = sense input '1' is true|
> >> |-------------------------------------------------------------------|
> >> | | | 00 = bypass for OR output |
> >> | 9:8 | SEL_BIT2_TYPE2 | 01 = bypass for AND output |
> >> | | | 10 = sense input '0' is true|
> >> | | | 11 = sense input '1' is true|
> >> |-------------------------------------------------------------------|
> >> | | | 00 = bypass for OR output |
> >> | 5:4 | SEL_BIT1_TYPE2 | 01 = bypass for AND output |
> >> | | | 10 = sense input '0' is true|
> >> | | | 11 = sense input '1' is true|
> >> |-------------------------------------------------------------------|
> >> | | | 00 = bypass for OR output |
> >> | 1:0 | SEL_BIT0_TYPE2 | 01 = bypass for AND output |
> >> | | | 10 = sense input '0' is true|
> >> | | | 11 = sense input '1' is true|
> >> |-------------------------------------------------------------------|
> >> These bits are used to identify the signals we want to sense, with
> >> a maximum signal number of 140. For example, to sense the signal
> >> 0xA (binary 1010), we set the value of bits 0 to 13 to 3232, which
> >> represents 1010. The remaining bits are set to 1, as we want to use
> >> AND gate to summarize all the signals we want to sense here. For
> >> rising or falling edge detection of any input to the priority, set
> >> the remaining bits to 0 to use an OR gate.
> >>
> >> 3. look for the pattern for priority_i i=0,1.
> >> - echo 0x3 > /sys/bus/coresight/devices/<tgu-name>/step0_condition_decode/reg0
> >> - echo 0x30 > /sys/bus/coresight/devices/<tgu-name>/step0_condition_decode/reg1
> >>
> >> |-------------------------------------------------------------------------------|
> >> | Bits | Field Nam | Description |
> >> |-------------------------------------------------------------------------------|
> >> | | |For each decoded condition, this |
> >> | 24 | NOT |inverts the output. If the condition |
> >> | | |decodes to true, and the NOT field |
> >> | | |is '1', then the output is NOT true. |
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from the associated|
> >> | 21 | BC0_COMP_ACTIVE |comparator will be actively included in|
> >> | | |the decoding of this particular |
> >> | | |condition. |
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from the associated|
> >> | | |comparator will need to be 1 to affect |
> >> | 20 | BC0_COMP_HIGH |the decoding of this condition. |
> >> | | |Conversely, a '0' here requires a '0' |
> >> | | |from the comparator |
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from the associated|
> >> | 17 | |comparator will be actively included in|
> >> | | TC0_COMP_ACTIVE |the decoding of this particular |
> >> | | |condition. |
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from the associated|
> >> | | |comparator will need to be 1 to affect |
> >> | 16 | TC0_COMP_HIGH |the decoding of this particular |
> >> | | |condition.Conversely, a 0 here |
> >> | | |requires a '0' from the comparator |
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from Priority_n |
> >> | | |OR logic will be actively |
> >> | 4n+3 | Priority_n_OR_ACTIVE|included in the decoding of |
> >> | | (n=0,1,2,3) |this particular condition. |
> >> | | | |
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from Priority_n |
> >> | | |will need to be '1' to affect the |
> >> | 4n+2 | Priority_n_OR_HIGH |decoding of this particular |
> >> | | (n=0,1,2,3) |condition. Conversely, a '0' here |
> >> | | |requires a '0' from Priority_n OR logic|
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from Priority_n |
> >> | | |AND logic will be actively |
> >> | 4n+1 |Priority_n_AND_ACTIVE|included in the decoding of this |
> >> | | (n=0,1,2,3) |particular condition. |
> >> | | | |
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from Priority_n |
> >> | | |AND logic will need to be '1' to |
> >> | 4n | Priority_n_AND_HIGH |affect the decoding of this |
> >> | | (n=0,1,2,3) |particular condition. Conversely, |
> >> | | |a '0' here requires a '0' from |
> >> | | |Priority_n AND logic. |
> >> |-------------------------------------------------------------------------------|
> >> Since we use `priority_0` and `priority_1` with an AND output in step 2, we set `0x3`
> >> and `0x30` here to activate them.
> >>
> >> 4. Set NEXT_STEP = 1 and TC0_ENABLE = 1 so that when the conditions
> >> are met then the next step will be step 1 and the timer will be enabled.
> >> - echo 0x20008 > /sys/bus/coresight/devices/<tgu-name>/step0_condition_select/reg0
> >> - echo 0x20008 > /sys/bus/coresight/devices/<tgu-name>/step0_condition_select/reg1
> >>
> >> |-----------------------------------------------------------------------------|
> >> | Bits | Field Nam | Description |
> >> |-----------------------------------------------------------------------------|
> >> | | |This field defines the next step the |
> >> | 18:17 | NEXT_STEP |TGU will 'goto' for the associated |
> >> | | |Condition and Step. |
> >> |-----------------------------------------------------------------------------|
> >> | | |For each possible output trigger |
> >> | 13 | TRIGGER |available, set a '1' if you want |
> >> | | |the trigger to go active for the |
> >> | | |associated condition and Step. |
> >> |-----------------------------------------------------------------------------|
> >> | | |This will cause BC0 to increment if the|
> >> | 9 | BC0_INC |associated Condition is decoded for |
> >> | | |this step. |
> >> |-----------------------------------------------------------------------------|
> >> | | |This will cause BC0 to decrement if the|
> >> | 8 | BC0_DEC |associated Condition is decoded for |
> >> | | |this step. |
> >> |-----------------------------------------------------------------------------|
> >> | | |This will clear BC0 count value to 0 if|
> >> | 7 | BC0_CLEAR |the associated Condition is decoded |
> >> | | |for this step. |
> >> |-----------------------------------------------------------------------------|
> >> | | |This will cause TC0 to increment until |
> >> | 3 | TC0_ENABLE |paused or cleared if the associated |
> >> | | |Condition is decoded for this step. |
> >> |-----------------------------------------------------------------------------|
> >> | | |This will cause TC0 to pause until |
> >> | 2 | TC0_PAUSE |enabled if the associated Condition |
> >> | | |is decoded for this step. |
> >> |-----------------------------------------------------------------------------|
> >> | | |This will clear TC0 count value to 0 |
> >> | 1 | TC0_CLEAR |if the associated Condition is |
> >> | | |decoded for this step. |
> >> |-----------------------------------------------------------------------------|
> >> | | |This will set the done signal to the |
> >> | 0 | DONE |TGU FSM if the associated Condition |
> >> | | |is decoded for this step. |
> >> |-----------------------------------------------------------------------------|
> >> Based on the distribution diagram, we set `0x20008` for `priority0` and `priority1` to
> >> achieve "jump to step 1 and enable TC0" once the signal is sensed.
> >>
> >> 5. activate the timer comparison for this step.
> >> - echo 0x30000 > /sys/bus/coresight/devices/<tgu-name>/step1_condition_decode/reg0
> >>
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from the associated|
> >> | 17 | |comparator will be actively included in|
> >> | | TC0_COMP_ACTIVE |the decoding of this particular |
> >> | | |condition. |
> >> |-------------------------------------------------------------------------------|
> >> | | |When '1' the output from the associated|
> >> | | |comparator will need to be 1 to affect |
> >> | 16 | TC0_COMP_HIGH |the decoding of this particular |
> >> | | |condition.Conversely, a 0 here |
> >> | | |requires a '0' from the comparator |
> >> |-------------------------------------------------------------------------------|
> >> Accroding to the decode distribution diagram , we give 0x30000 here to set 16th&17th bit
> >> to enable timer comparison.
> >>
> >> 6. Set the NEXT_STEP = 0 and TC0_PAUSE = 1 and TC0_CLEAR = 1 once the timer
> >> has reached the given value.
> >> - echo 0x6 > /sys/bus/coresight/devices/<tgu-name>/step1_condition_select/reg0
> >>
> >> 7. Enable Trigger 0 for TGU when the condition 0 is met in step1,
> >> i.e. when the timer reaches 3.
> >> - echo 0x2000 > /sys/bus/coresight/devices/<tgu-name>/step1_condition_select/default
> >>
> >> Note:
> >> 1. 'default' register allows for establishing the resultant action for
> >> the default condition
> >>
> >> 2. Trigger:For each possible output trigger available from
> >> the Design document, there are three triggers: interrupts, CTI,
> >> and Cross-TGU mapping.All three triggers can occur, but
> >> the choice of which trigger to use depends on the user's
> >> needs.
> >>
> >> 8. Compare the timer to 3 in step 1.
> >> - echo 0x3 > /sys/bus/coresight/devices/<tgu-name>/step1_timer/reg0
> >>
> >> 9. enale tgu
> >> - echo 1 > /sys/bus/coresight/devices/<tgu-name>/enable_tgu
> >>
> > If this is version 3 - where is the list of differences from versions 1 - 2?
> My bad. Will add the previous change log in the next version.
> >
> >> Songwei Chai (7):
> >> dt-bindings: arm: Add support for Coresight TGU trace
> >> coresight: Add coresight TGU driver
> >> coresight-tgu: Add signal priority support
> >> coresight-tgu: Add TGU decode support
> >> coresight-tgu: add support to configure next action
> >> coresight-tgu: add timer/counter functionality for TGU
> >> coresight-tgu: add reset node to initialize
> >>
> >> .../testing/sysfs-bus-coresight-devices-tgu | 51 ++
> >> .../bindings/arm/qcom,coresight-tgu.yaml | 135 ++++
> >> drivers/hwtracing/coresight/Kconfig | 11 +
> >> drivers/hwtracing/coresight/Makefile | 1 +
> >> drivers/hwtracing/coresight/coresight-tgu.c | 669 ++++++++++++++++++
> >> drivers/hwtracing/coresight/coresight-tgu.h | 242 +++++++
> >> 6 files changed, 1109 insertions(+)
> >> create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu
> >> create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml
> >> create mode 100644 drivers/hwtracing/coresight/coresight-tgu.c
> >> create mode 100644 drivers/hwtracing/coresight/coresight-tgu.h
> >>
> > Regards
> >
> >
> > Mike
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
On 20/03/2025 13:52, Greg KH wrote:
> On Mon, Mar 17, 2025 at 01:51:10PM +0000, Suzuki K Poulose wrote:
>> Hi Greg,
>>
>> Please find the updates for coresight/hwtracing subsystem targeting v6.15.
>>
>> Kindly pull,
>> Suzuki
>>
>> The following changes since commit 0ad2507d5d93f39619fc42372c347d6006b64319:
>>
>> Linux 6.14-rc3 (2025-02-16 14:02:44 -0800)
>>
>> are available in the Git repository at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-next-v6.15
>
> Pulled and pushed out, thanks.
>
> greg k-h
Thank you Greg !
I've gotten stuck a few times with unusable Coresight after a warm boot
due to lingering claim tags, especially when testing the Coresight
panic patchsets.
This change does some tidy ups, adds some debug messages and clears the
self hosted claim tag on probe. The last two commits are unrelated
tidyups but they touch some of the same functions so to avoid extra
conflicts I'm including them here.
This gets as far as fixing the claim tag issue, but there is some other
state not being cleared on probe that results in the following error.
This can be fixed up as a later change:
coresight tmc_etf0: timeout while waiting for TMC to be Ready
coresight tmc_etf0: Failed to enable : TMC is not ready
Changes in v2:
* Revert most of the interface changes, just call
coresight_clear_self_claim_tag() directly. This is possible because
we're not doing the read first, so it has fewer knock on effects.
* Split out the change to add struct cs_access to etm3x
* Add another warning for racing with external debugger
--
2.34.1
---
James Clark (8):
coresight: Rename coresight_{set,clear}_claim_tags()
coresight: Convert tag clear function to take a struct cs_access
coresight: Only check bottom two claim bits
coresight: Add claim tag warnings and debug messages
coresight: etm3x: Convert raw base pointer to struct coresight access
coresight: Clear self hosted claim tag on probe
coresight: Remove inlines from static function definitions
coresight: Remove extern from function declarations
drivers/hwtracing/coresight/coresight-catu.c | 12 +--
drivers/hwtracing/coresight/coresight-core.c | 85 ++++++++++++++--------
drivers/hwtracing/coresight/coresight-cti-core.c | 2 +
drivers/hwtracing/coresight/coresight-etb10.c | 4 +-
drivers/hwtracing/coresight/coresight-etm.h | 6 +-
drivers/hwtracing/coresight/coresight-etm3x-core.c | 28 +++----
.../hwtracing/coresight/coresight-etm3x-sysfs.c | 8 +-
drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++-
.../hwtracing/coresight/coresight-etm4x-sysfs.c | 4 +-
drivers/hwtracing/coresight/coresight-funnel.c | 1 +
drivers/hwtracing/coresight/coresight-platform.c | 26 +++----
drivers/hwtracing/coresight/coresight-priv.h | 20 ++---
drivers/hwtracing/coresight/coresight-replicator.c | 3 +-
drivers/hwtracing/coresight/coresight-stm.c | 6 +-
.../coresight/coresight-syscfg-configfs.c | 2 +-
drivers/hwtracing/coresight/coresight-tmc-core.c | 9 ++-
drivers/hwtracing/coresight/coresight-tmc-etr.c | 16 ++--
drivers/hwtracing/coresight/coresight-trbe.c | 18 ++---
include/linux/coresight.h | 40 +++++-----
19 files changed, 166 insertions(+), 134 deletions(-)
---
base-commit: 5442d22da7dbff3ba8c6720fc6f23ea4934d402d
change-id: 20250317-james-coresight-claim-tags-ae1461f1f5e0
Best regards,
--
James Clark <james.clark(a)linaro.org>
I've gotten stuck a few times with unusable Coresight after a warm boot
due to lingering claim tags, especially when testing the Coresight
panic patchsets.
This change does some tidy ups, adds some debug messages and clears the
self hosted claim tag on probe. The last two commits are unrelated
tidyups but they touch some of the same functions so to avoid extra
conflicts I'm including them here.
This gets as far as fixing the claim tag issue, but there is some other
state not being cleared on probe that results in the following error.
This can be fixed up as a later change:
coresight tmc_etf0: timeout while waiting for TMC to be Ready
coresight tmc_etf0: Failed to enable : TMC is not ready
James Clark (7):
coresight: Rename coresight_{set,clear}_claim_tags()
coresight: Convert disclaim functions to take a struct cs_access
coresight: Only check bottom two claim bits
coresight: Add claim tag warnings and debug messages
coresight: Clear self hosted claim tag on probe
coresight: Remove inlines from static function definitions
coresight: Remove extern from function declarations
drivers/hwtracing/coresight/coresight-catu.c | 14 +-
drivers/hwtracing/coresight/coresight-core.c | 132 +++++++++++-------
.../hwtracing/coresight/coresight-cti-core.c | 8 +-
drivers/hwtracing/coresight/coresight-etb10.c | 6 +-
drivers/hwtracing/coresight/coresight-etm.h | 6 +-
.../coresight/coresight-etm3x-core.c | 32 ++---
.../coresight/coresight-etm3x-sysfs.c | 8 +-
.../coresight/coresight-etm4x-core.c | 12 +-
.../coresight/coresight-etm4x-sysfs.c | 4 +-
.../hwtracing/coresight/coresight-funnel.c | 4 +-
.../hwtracing/coresight/coresight-platform.c | 26 ++--
drivers/hwtracing/coresight/coresight-priv.h | 20 +--
.../coresight/coresight-replicator.c | 7 +-
drivers/hwtracing/coresight/coresight-stm.c | 6 +-
.../coresight/coresight-syscfg-configfs.c | 2 +-
.../hwtracing/coresight/coresight-tmc-core.c | 9 +-
.../hwtracing/coresight/coresight-tmc-etf.c | 8 +-
.../hwtracing/coresight/coresight-tmc-etr.c | 20 ++-
drivers/hwtracing/coresight/coresight-trbe.c | 18 +--
include/linux/coresight.h | 32 ++---
20 files changed, 209 insertions(+), 165 deletions(-)
--
2.34.1