CoreSight

coresight@lists.linaro.org

7 participants
2442 discussions

Re: [PATCH v6 0/3] coresight: prevent deactivate active config while enabling the config

by Suzuki K Poulose

On Wed, 14 May 2025 17:19:48 +0100, 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) > > [...] Applied, thanks! [1/3] coresight/etm4: fix missing disable active config https://git.kernel.org/coresight/c/895b12b7 [2/3] coresight: holding cscfg_csdev_lock while removing cscfg from csdev https://git.kernel.org/coresight/c/53b9e265 [3/3] coresight: prevent deactivate active config while enabling the config https://git.kernel.org/coresight/c/408c97c4 Best regards, -- Suzuki K Poulose <suzuki.poulose(a)arm.com>

3 months

[PATCH v2 0/2] coresight: etm4x: Fix timestamp regression

by Leo Yan

The timestamp appears all zeros with latest kernel, which is caused by wrong bit field setting. The first patch fixes the regression. The second patch refactors the ts_source_show() function. It simplifies the code for directly returning the TS value. Changes from v1: - Retained the older condition check (Suzuki). Leo Yan (2): coresight: etm4x: Fix timestamp bit field handling coresight: etm4x: Refactor returning TS field drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +- drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 16 ++-------------- 2 files changed, 3 insertions(+), 15 deletions(-) -- 2.34.1

3 months

[PATCH v2] coresight: tmc: fix failure to disable/enable ETF after reading

by Mao Jinlong

ETF may fail to re-enable after reading, and driver->reading will not be set to false, this will cause failure to enable/disable to ETF. This change set driver->reading to false even if re-enabling fail. Fixes: 669c4614236a ("coresight: tmc: Don't enable TMC when it's not ready.") Co-developed-by: Yuanfang Zhang <quic_yuanfang(a)quicinc.com> Signed-off-by: Yuanfang Zhang <quic_yuanfang(a)quicinc.com> Signed-off-by: Mao Jinlong <quic_jinlmao(a)quicinc.com> --- drivers/hwtracing/coresight/coresight-tmc-etf.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d858740001c2..c9e2d95ae295 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -747,7 +747,6 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata) char *buf = NULL; enum tmc_mode mode; unsigned long flags; - int rc = 0; /* config types are set a boot time and never change */ if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB && @@ -773,11 +772,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata) * can't be NULL. */ memset(drvdata->buf, 0, drvdata->size); - rc = __tmc_etb_enable_hw(drvdata); - if (rc) { - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); - return rc; - } + __tmc_etb_enable_hw(drvdata); } else { /* * The ETB/ETF is not tracing and the buffer was just read. -- 2.25.1

3 months

[PATCH] coresight: etm4x: Fix timestamp bit field handling

by Leo Yan

Timestamps in the trace data appear as all zeros on recent kernels, although the feature works correctly on old kernels (e.g., v6.12). Since commit c382ee674c8b ("arm64/sysreg/tools: Move TRFCR definitions to sysreg"), the TRFCR_ELx_TS_{VIRTUAL|GUEST_PHYSICAL|PHYSICAL} macros were updated to remove the bit shift. As a result, the driver no longer shifts bits when operates the timestamp field. Fix this by using the FIELD_PREP() and FIELD_GET() helpers. Simplify the ts_source_show() function: return -1 when the value is zero, as this indciates an invalid value; otherwise return the decoded TS value directly. Reported-by: Tamas Zsoldos <tamas.zsoldos(a)arm.com> Fixes: c382ee674c8b ("arm64/sysreg/tools: Move TRFCR definitions to sysreg") Signed-off-by: Leo Yan <leo.yan(a)arm.com> --- .../hwtracing/coresight/coresight-etm4x-core.c | 2 +- .../hwtracing/coresight/coresight-etm4x-sysfs.c | 17 ++--------------- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 6a5898355a83..acb4a58e4bb9 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1237,7 +1237,7 @@ static void cpu_detect_trace_filtering(struct etmv4_drvdata *drvdata) * tracing at the kernel EL and EL0, forcing to use the * virtual time as the timestamp. */ - trfcr = (TRFCR_EL1_TS_VIRTUAL | + trfcr = (FIELD_PREP(TRFCR_EL1_TS_MASK, TRFCR_EL1_TS_VIRTUAL) | TRFCR_EL1_ExTRE | TRFCR_EL1_E0TRE); diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index 49d5fb87a74b..8a2749eeb9a5 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -2315,23 +2315,10 @@ static ssize_t ts_source_show(struct device *dev, int val; struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); - if (!drvdata->trfcr) { + val = FIELD_GET(TRFCR_EL1_TS_MASK, drvdata->trfcr); + if (!val) val = -1; - goto out; - } - - switch (drvdata->trfcr & TRFCR_EL1_TS_MASK) { - case TRFCR_EL1_TS_VIRTUAL: - case TRFCR_EL1_TS_GUEST_PHYSICAL: - case TRFCR_EL1_TS_PHYSICAL: - val = FIELD_GET(TRFCR_EL1_TS_MASK, drvdata->trfcr); - break; - default: - val = -1; - break; - } -out: return sysfs_emit(buf, "%d\n", val); } static DEVICE_ATTR_RO(ts_source); -- 2.34.1

3 months

Re: [PATCH v6 3/3] coresight: prevent deactivate active config while enabling the config

by Leo Yan

On Wed, May 14, 2025 at 05:19:51PM +0100, 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. > cscfg_csdev_enable_config > > 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. > > Fixes: f8cce2ff3c04 ("coresight: syscfg: Add API to activate and enable configurations") > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> Reviewed-by: Leo Yan <leo.yan(a)arm.com> > --- > .../hwtracing/coresight/coresight-config.h | 2 +- > .../hwtracing/coresight/coresight-syscfg.c | 49 +++++++++++++------ > 2 files changed, 35 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-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > index 23017612f2ea..83dad24e0116 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > @@ -869,6 +869,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. > @@ -892,22 +911,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); > > - /* > - * 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; > @@ -922,9 +936,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; > } > @@ -1049,7 +1062,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; > > @@ -1064,8 +1077,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)) { > config_csdev_active = config_csdev_item; > csdev->active_cscfg_ctxt = (void *)config_csdev_active; > break; > @@ -1099,7 +1112,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); > @@ -1138,8 +1155,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} >

3 months, 1 week

Re: [PATCH v5 3/3] coresight: prevent deactivate active config while enabling the config

by Leo Yan

Hi Levi, On Wed, May 14, 2025 at 12:04:39PM +0100, Yeoreum Yun wrote: [...] > > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc) > > > > I would like to change the return type to int, so the error is handled > > within the function. As a result, the caller _cscfg_activate_config() > > does not need to explicitly return an error value. > > I think it's not good since cscfg_config_desc_get() failed only when > try_module_get() failed and its return type is "bool". Understood. I thought it would be easier for later maintenance if we encapsulate error handling in the function, but I don't have strong opinion. It is fine for me to return bool type. Thanks, Leo

3 months, 1 week

Re: [PATCH v5 3/3] coresight: prevent deactivate active config while enabling the config

by Leo Yan

On Tue, May 13, 2025 at 06:06:22PM +0100, 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. > cscfg_csdev_enable_config > > 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> > --- > .../hwtracing/coresight/coresight-config.h | 2 +- > .../hwtracing/coresight/coresight-syscfg.c | 49 +++++++++++++------ > 2 files changed, 35 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-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > index 5d194b9269f5..6d8c212ad434 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > @@ -870,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) I would like to change the return type to int, so the error is handled within the function. As a result, the caller _cscfg_activate_config() does not need to explicitly return an error value. Otherwise, the patch looks good to me. Thanks, Leo > +{ > + 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. > @@ -893,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); > > - /* > - * 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; > @@ -923,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; > } > @@ -1050,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; > > @@ -1065,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)) { > config_csdev_active = config_csdev_item; > csdev->active_cscfg_ctxt = (void *)config_csdev_active; > break; > @@ -1100,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); > @@ -1139,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} >

3 months, 1 week

Re: [PATCH v5 2/3] coresight: holding cscfg_csdev_lock while removing cscfg from csdev

by Leo Yan

On Tue, May 13, 2025 at 06:06:21PM +0100, Yeoreum Yun wrote: > There'll be possible race scenario for coresight config: > > CPU0 CPU1 > (perf enable) load module > cscfg_load_config_sets() > activate config. // sysfs > (sys_active_cnt == 1) > ... > cscfg_csdev_enable_active_config() > lock(csdev->cscfg_csdev_lock) > deactivate config // sysfs > (sys_activec_cnt == 0) > cscfg_unload_config_sets() > <iterating config_csdev_list> cscfg_remove_owned_csdev_configs() > // here load config activate by CPU1 > unlock(csdev->cscfg_csdev_lock) > > iterating config_csdev_list could be raced with config_csdev_list's > entry delete. > > To resolve this race , hold csdev->cscfg_csdev_lock() while > cscfg_remove_owned_csdev_configs() > > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> > Fixes: 02bd588e12df ("coresight: configuration: Update API to permit dynamic load/unload") > --- > drivers/hwtracing/coresight/coresight-syscfg.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > index a70c1454b410..5d194b9269f5 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); Could we use the format: guard(raw_spinlock_irqsave)(&csdev->cscfg_csdev_lock); Sorry I did not mention this in the earlier review. Otherwise: Reviewed-by: Leo Yan <leo.yan(a)arm.com> > 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) > -- > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7} >

3 months, 1 week

Re: [PATCH v5 1/3] coresight/etm4: fix missing disable active config

by Leo Yan

On Tue, May 13, 2025 at 06:06:20PM +0100, Yeoreum Yun wrote: > When etm4 device is disabled via sysfs, it should disable its active > count. > > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> With adding fix tag: Fixes: 7ebd0ec6cf94 ("coresight: configfs: Allow configfs to activate configuration") Reviewed-by: Leo Yan <leo.yan(a)arm.com> > --- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 2b8f10463840..f008f87f0801 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); > + > cpus_read_unlock(); > > /* > -- > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7} >

3 months, 1 week

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

by Leo Yan

Hi Levi, On Fri, May 02, 2025 at 11:34:17AM +0100, Yeoreum Yun wrote: [...] > > > > > --- 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? > > > > > > > > > > It's not a differnt issue. Without this line, the active count wouldn't > > > decrese and it raise another issue -- unloadable moudle for active_cnt :( > > > So I think it should be included in this patch. > > > > I read the code again and concluded the change above is not related to > > locking and would be a separate issue: when we close a Sysfs session, > > we need to disable a config on a CoreSight device. > > Could you clarify what is meaning "unloadable moudle for active_cnt"? > > I only saw "cscfg_mgr->sys_active_cnt" is used for module unloading, > > but have no clue why "active_cnt" impacts module unloading. > > Yes. but it also related "by this patch". > When the load config and "activate" configuration via sysfs, > not only the cscfg_mgr->sys_active_cnt is increase but also > "individual cscfg->active_cnt". > This patch extends the meaning of "cscfg->active_cnt" includes > "enable of configuraiton". so that cscfg_config_desc_put() prevent > decrease "module reference" while holding individual active_cnt. > That's why without this change, the "module reference" couldn't be > decreased. The problem before this change is deactivation doesn't > chekc cscfg->active_cnt but put module reference whenever it calls. Thanks for explanation and it makes sense to me. As we discussed, given this patch is relative big, let us divide into three small patches for easier review. - The first patch is to address that the sysfs interface misses to call cscfg_csdev_disable_active_config() for disabling config. - The second patch fixes the locking issue for "config_csdev_list". As the "config_csdev_list" is protected by cscfg_csdev_lock, the cscfg_remove_owned_csdev_configs() function should use lock for exclusive operations. - The third patch is to fix reference counter of a config module. The patch adds cscfg_config_desc_get() and cscfg_config_desc_put() in the config enabling / disabling flow for acquiring module reference counter. Thanks, Leo

3 months, 1 week

← Newer
1
...
7
8
9
10
11
12
13
...
245
Older →

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight