CoreSight

coresight@lists.linaro.org

6 participants
2630 discussions

Re: [PATCH v4 2/2] coresight: Fix possible deadlock in coresight_panic_cb

by Leo Yan

On Fri, Sep 19, 2025 at 12:06:53PM -0400, Sean Anderson wrote: [...] > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 022c8384b98d..6dfb1198c16e 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -1046,8 +1046,11 @@ static void coresight_device_release(struct device *dev) > { > struct coresight_device *csdev = to_coresight_device(dev); > > - fwnode_handle_put(csdev->dev.fwnode); > + if (panic_ops(csdev)) > + atomic_notifier_chain_unregister(&panic_notifier_list, > + &csdev->panic_notifier); > free_percpu(csdev->perf_sink_id_map.cpu_map); > + fwnode_handle_put(csdev->dev.fwnode); The moving fwnode_handle_put() is irrelvant to panic notifier fix, should be moved out from this patch. > kfree(csdev); > } > > @@ -1315,6 +1318,16 @@ void coresight_release_platform_data(struct coresight_device *csdev, > coresight_remove_conns_sysfs_group(csdev); > } > > +static int coresight_panic_notifier(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct coresight_device *csdev = > + container_of(nb, struct coresight_device, panic_notifier); > + Need to check device mode: if (coresight_get_mode(csdev) == CS_MODE_DISABLED) return NOTIFY_DONE; The rest is fine for me. Thanks, Leo > + panic_ops(csdev)->sync(csdev); > + return NOTIFY_DONE; > +} > + > struct coresight_device *coresight_register(struct coresight_desc *desc) > { > int ret; > @@ -1357,6 +1370,17 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) > goto err_out; > } > } > + > + if (panic_ops(csdev)) { > + csdev->panic_notifier.notifier_call = coresight_panic_notifier; > + ret = atomic_notifier_chain_register(&panic_notifier_list, > + &csdev->panic_notifier); > + if (ret) { > + coresight_device_release(&csdev->dev); > + goto err_out; > + } > + } > + > /* > * Make sure the device registration and the connection fixup > * are synchronised, so that we don't see uninitialised devices > @@ -1563,36 +1587,6 @@ const struct bus_type coresight_bustype = { > .name = "coresight", > }; > > -static int coresight_panic_sync(struct device *dev, void *data) > -{ > - int mode; > - struct coresight_device *csdev; > - > - /* Run through panic sync handlers for all enabled devices */ > - csdev = container_of(dev, struct coresight_device, dev); > - mode = coresight_get_mode(csdev); > - > - if ((mode == CS_MODE_SYSFS) || (mode == CS_MODE_PERF)) { > - if (panic_ops(csdev)) > - panic_ops(csdev)->sync(csdev); > - } > - > - return 0; > -} > - > -static int coresight_panic_cb(struct notifier_block *self, > - unsigned long v, void *p) > -{ > - bus_for_each_dev(&coresight_bustype, NULL, NULL, > - coresight_panic_sync); > - > - return 0; > -} > - > -static struct notifier_block coresight_notifier = { > - .notifier_call = coresight_panic_cb, > -}; > - > static int __init coresight_init(void) > { > int ret; > @@ -1605,20 +1599,11 @@ static int __init coresight_init(void) > if (ret) > goto exit_bus_unregister; > > - /* Register function to be called for panic */ > - ret = atomic_notifier_chain_register(&panic_notifier_list, > - &coresight_notifier); > - if (ret) > - goto exit_perf; > - > /* initialise the coresight syscfg API */ > ret = cscfg_init(); > if (!ret) > return 0; > > - atomic_notifier_chain_unregister(&panic_notifier_list, > - &coresight_notifier); > -exit_perf: > etm_perf_exit(); > exit_bus_unregister: > bus_unregister(&coresight_bustype); > @@ -1628,8 +1613,6 @@ static int __init coresight_init(void) > static void __exit coresight_exit(void) > { > cscfg_exit(); > - atomic_notifier_chain_unregister(&panic_notifier_list, > - &coresight_notifier); > etm_perf_exit(); > bus_unregister(&coresight_bustype); > } > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 4ac65c68bbf4..a7aaf9d3d01d 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -280,6 +280,7 @@ struct coresight_trace_id_map { > * @config_csdev_list: List of system configurations added to the device. > * @cscfg_csdev_lock: Protect the lists of configurations and features. > * @active_cscfg_ctxt: Context information for current active system configuration. > + * @panic_notifier: Notifier block used to clean up during a panic > */ > struct coresight_device { > struct coresight_platform_data *pdata; > @@ -304,6 +305,7 @@ struct coresight_device { > struct list_head config_csdev_list; > raw_spinlock_t cscfg_csdev_lock; > void *active_cscfg_ctxt; > + struct notifier_block panic_notifier; > }; > > /* > -- > 2.35.1.1320.gc452695387.dirty >

4 months, 3 weeks

Re: [PATCH v4 1/2] coresight: Fix fwnode leak in coresight_register error path

by Leo Yan

On Fri, Sep 19, 2025 at 12:06:52PM -0400, Sean Anderson wrote: > If registering the CPU map fails, we need to put the fwnode. free_percpu > works when called with a NULL pointer, so just use > coresight_device_release. > > Fixes: 5ad628a76176 ("coresight: Use per-sink trace ID maps for Perf sessions") > Signed-off-by: Sean Anderson <sean.anderson(a)linux.dev> I have a patch that fixes the same issue: https://lore.kernel.org/linux-arm-kernel/20250512154108.23920-2-leo.yan@arm… The difference in my patch is about the sequence: first it allocates the resource, then increases the fw node's reference count. During release, it first decreases the reference count, and then safely releases the resource. After comparing your patch, I still think the above reason is valid. That said, I agree we should put this fixing before the panic notifier fix. This would be friendly for backporting. Thanks, Leo > --- > > Changes in v4: > - New > > drivers/hwtracing/coresight/coresight-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index fa758cc21827..022c8384b98d 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -1352,7 +1352,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) > raw_spin_lock_init(&csdev->perf_sink_id_map.lock); > csdev->perf_sink_id_map.cpu_map = alloc_percpu(atomic_t); > if (!csdev->perf_sink_id_map.cpu_map) { > - kfree(csdev); > + coresight_device_release(&csdev->dev); > ret = -ENOMEM; > goto err_out; > } > -- > 2.35.1.1320.gc452695387.dirty >

4 months, 3 weeks

Re: [PATCH RFC 3/3] coresight: change the sink_ops to accept coresight_path

by Leo Yan

On Mon, Sep 22, 2025 at 03:31:41PM +0800, Jie Gan wrote: > Update the sink_enable functions to accept coresight_path instead of > a generic void *data, as coresight_path encapsulates all the necessary > data required by devices along the path. > > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> Good refactoring for me, thanks! Reviewed-by: Leo Yan <leo.yan(a)arm.com>

4 months, 3 weeks

Re: [PATCH RFC 2/3] coresight: change helper_ops to accept coresight_path

by Leo Yan

On Mon, Sep 22, 2025 at 03:31:40PM +0800, Jie Gan wrote: > Update the helper_enable and helper_disable functions to accept > coresight_path instead of a generic void *data, as coresight_path > encapsulates all the necessary data required by devices along the path. > > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> Reviewed-by: Leo Yan <leo.yan(a)arm.com>

4 months, 3 weeks

Re: [PATCH RFC 1/3] coresight: tmc: add the handle of the event to the path

by Leo Yan

On Mon, Sep 22, 2025 at 03:31:39PM +0800, Jie Gan wrote: > From: Carl Worth <carl(a)os.amperecomputing.com> > > The handle is essential for retrieving the AUX_EVENT of each CPU and is > required in perf mode. It has been added to the coresight_path so that > dependent devices can access it from the path when needed. > > Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path") > Signed-off-by: Carl Worth <carl(a)os.amperecomputing.com> > Co-developed-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > --- > drivers/hwtracing/coresight/coresight-etm-perf.c | 1 + > drivers/hwtracing/coresight/coresight-tmc-etr.c | 3 ++- > include/linux/coresight.h | 10 ++++++---- > 3 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index f677c08233ba..5c256af6e54a 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -520,6 +520,7 @@ static void etm_event_start(struct perf_event *event, int flags) > goto out; > > path = etm_event_cpu_path(event_data, cpu); > + path->handle = handle; > /* We need a sink, no need to continue without one */ > sink = coresight_get_sink(path); > if (WARN_ON_ONCE(!sink)) > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index b07fcdb3fe1a..1040f73f0537 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1327,7 +1327,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) > struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, > enum cs_mode mode, void *data) > { > - struct perf_output_handle *handle = data; > + struct coresight_path *path = data; > + struct perf_output_handle *handle = path->handle; > struct etr_perf_buffer *etr_perf; > > switch (mode) { > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 6de59ce8ef8c..4591121ae1d4 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -332,12 +332,14 @@ static struct coresight_dev_list (var) = { \ > > /** > * struct coresight_path - data needed by enable/disable path > - * @path_list: path from source to sink. > - * @trace_id: trace_id of the whole path. > + * @path_list: path from source to sink. > + * @trace_id: trace_id of the whole path. > + * struct perf_output_handle: handle of the aux_event. s/struct perf_output_handle/@handle/ Otherwise, LGTM: Reviewed-by: Leo Yan <leo.yan(a)arm.com> > */ > struct coresight_path { > - struct list_head path_list; > - u8 trace_id; > + struct list_head path_list; > + u8 trace_id; > + struct perf_output_handle *handle; > }; > > enum cs_mode { > > -- > 2.34.1 > >

4 months, 3 weeks

Re: [PATCH] coresight: Fix data argument to coresight_enable_helpers

by Suzuki K Poulose

On 19/09/2025 02:20, Jie Gan wrote: > > > On 9/19/2025 6:18 AM, Carl Worth wrote: >> Jie Gan <jie.gan(a)oss.qualcomm.com> writes: >>> I dont think we can change back to sink_data since we introduced >>> coresight_path to wrap 'data' which is needed by the path. >>> >>> I suggest you to add the struct perf_output_handle to the >>> coresight_path, then retrieving it with data->perf_handle in >>> tmc_etr_get_buffer. >> ... >>> We can assign the perf_output_handle to the coresight_path after we >>> constructed the coresight_path in perf mode. >> >> Thanks. That much makes sense to me, and I'll put together a patch along >> those lines. >> >> But, further: with core coresight code assembling into the path all the >> data that is necessary, is there any reason to be using void* in these >> enable/disable functions? > > In my opinion, yes, we can change void * to coresight_path * for > helper's enable/disable functions since we have everything in path so > the cast is not necessary now. > >> >> Could we also change these functions to accept a coresight_path* and >> actually get some compiler help at finding mistakes like the one we're >> fixing here? > Yes, please. I was going to suggest that. May be we could do that as a separate patch after fixing the problem here first (so that it can be back ported). This was initially a perf_handle only used for the perf mode, and it didn't make sens to have a "perf" argument to "enable" which could be used for both sysfs and perf. Now that the path is a generic data structure, it makes sense to move everything to accept the path. Suzuki > That's the only benefit in my mind so far. > > Thanks, > Jie > >> >> -Carl >

4 months, 3 weeks

[PATCH v2] coresight: Fix data arguments to coresight enable/disable helpers

by Carl Worth

In the commit being fixed, coresight_enable_path() was changed to call coresight_enable_helpers() by passing a struct coresight_path* as the final void* 'path' argument, rather passing 'sink_data' as was being passed previously. This set the groundwork for the subsequent addition of the ctcu_enable function which interprets void* data as a struct coresight_path*, but it also broke the existing catu_enable function which interprets void* data as a struct perf_output_handle*. The compiler could not flag the error since there are several layers of function calls treating the pointer as void*. Fix both users simultaneously by adding an additional argument to the enable helper interface. So now both the struct coresight_path and the struct perf_output_handle pointers are passed. Also, eliminate all usages of the void* from these code paths and use explicit types instead to make all of this code more robust. Note: The disable function is also changed from void* to struct coresight_path* but no implementations of this function need the struct perf_output_handle* so it is not added to the interface. The existing bug can be reproduced with: # perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null Showing an oops as follows: Unable to handle kernel paging request at virtual address 000f6e84934ed19e Call trace: tmc_etr_get_buffer+0x30/0x80 [coresight_tmc] (P) catu_enable_hw+0xbc/0x3d0 [coresight_catu] catu_enable+0x70/0xe0 [coresight_catu] coresight_enable_path+0xb0/0x258 [coresight] Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path") Signed-off-by: Carl Worth <carl(a)os.amperecomputing.com> --- Jie, I looked into your suggestion of adding the struct perf_output_handle* to the struct coresight_path, but I couldn't justify adding event-related data to the path structure, which has nothing to do with it. So, instead I took the approach in this patch to plumb both arguments through the enable path, (and changed away from void* as you agreed to). Note: I've tested that this fixes the bug for catu_enable. This patch also obviously touches ctcu_enable, which I believe is correct, but I have not tested since I don't have ready access to the necessary hardware. I will appreciate other testing. -Carl drivers/hwtracing/coresight/coresight-catu.c | 12 ++++++---- drivers/hwtracing/coresight/coresight-core.c | 23 +++++++++++-------- .../hwtracing/coresight/coresight-ctcu-core.c | 11 ++++----- .../hwtracing/coresight/coresight-cti-core.c | 7 ++++-- .../hwtracing/coresight/coresight-cti-sysfs.c | 2 +- drivers/hwtracing/coresight/coresight-cti.h | 6 +++-- drivers/hwtracing/coresight/coresight-priv.h | 2 +- .../hwtracing/coresight/coresight-tmc-etr.c | 4 ++-- drivers/hwtracing/coresight/coresight-tmc.h | 3 ++- include/linux/coresight.h | 6 +++-- 10 files changed, 45 insertions(+), 31 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c index 5058432233da..724c25d0afa4 100644 --- a/drivers/hwtracing/coresight/coresight-catu.c +++ b/drivers/hwtracing/coresight/coresight-catu.c @@ -397,7 +397,7 @@ static int catu_wait_for_ready(struct catu_drvdata *drvdata) } static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode, - void *data) + struct perf_output_handle *handle) { int rc; u32 control, mode; @@ -425,7 +425,7 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode, etrdev = coresight_find_input_type( csdev->pdata, CORESIGHT_DEV_TYPE_SINK, etr_subtype); if (etrdev) { - etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data); + etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, handle); if (IS_ERR(etr_buf)) return PTR_ERR(etr_buf); } @@ -455,7 +455,8 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode, } static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, - void *data) + struct coresight_path *path, + struct perf_output_handle *handle) { int rc = 0; struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); @@ -463,7 +464,7 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); if (csdev->refcnt == 0) { CS_UNLOCK(catu_drvdata->base); - rc = catu_enable_hw(catu_drvdata, mode, data); + rc = catu_enable_hw(catu_drvdata, mode, handle); CS_LOCK(catu_drvdata->base); } if (!rc) @@ -488,7 +489,8 @@ static int catu_disable_hw(struct catu_drvdata *drvdata) return rc; } -static int catu_disable(struct coresight_device *csdev, void *__unused) +static int catu_disable(struct coresight_device *csdev, + struct coresight_path *__unused) { int rc = 0; struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index fa758cc21827..cc449d5196a4 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -353,14 +353,17 @@ static bool coresight_is_helper(struct coresight_device *csdev) } static int coresight_enable_helper(struct coresight_device *csdev, - enum cs_mode mode, void *data) + enum cs_mode mode, + struct coresight_path *path, + struct perf_output_handle *handle) { - return helper_ops(csdev)->enable(csdev, mode, data); + return helper_ops(csdev)->enable(csdev, mode, path, handle); } -static void coresight_disable_helper(struct coresight_device *csdev, void *data) +static void coresight_disable_helper(struct coresight_device *csdev, + struct coresight_path *path) { - helper_ops(csdev)->disable(csdev, data); + helper_ops(csdev)->disable(csdev, path); } static void coresight_disable_helpers(struct coresight_device *csdev, void *data) @@ -477,7 +480,9 @@ void coresight_disable_path(struct coresight_path *path) EXPORT_SYMBOL_GPL(coresight_disable_path); static int coresight_enable_helpers(struct coresight_device *csdev, - enum cs_mode mode, void *data) + enum cs_mode mode, + struct coresight_path *path, + struct perf_output_handle *handle) { int i, ret = 0; struct coresight_device *helper; @@ -487,7 +492,7 @@ static int coresight_enable_helpers(struct coresight_device *csdev, if (!helper || !coresight_is_helper(helper)) continue; - ret = coresight_enable_helper(helper, mode, data); + ret = coresight_enable_helper(helper, mode, path, handle); if (ret) return ret; } @@ -496,7 +501,7 @@ static int coresight_enable_helpers(struct coresight_device *csdev, } int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, - void *sink_data) + struct perf_output_handle *handle) { int ret = 0; u32 type; @@ -510,7 +515,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, type = csdev->type; /* Enable all helpers adjacent to the path first */ - ret = coresight_enable_helpers(csdev, mode, path); + ret = coresight_enable_helpers(csdev, mode, path, handle); if (ret) goto err_disable_path; /* @@ -526,7 +531,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, switch (type) { case CORESIGHT_DEV_TYPE_SINK: - ret = coresight_enable_sink(csdev, mode, sink_data); + ret = coresight_enable_sink(csdev, mode, handle); /* * Sink is the first component turned on. If we * failed to enable the sink, there are no components diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c index c6bafc96db96..9f6d71c59d4e 100644 --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c @@ -156,17 +156,16 @@ static int ctcu_set_etr_traceid(struct coresight_device *csdev, struct coresight return __ctcu_set_etr_traceid(csdev, traceid, port_num, enable); } -static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode, void *data) +static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode, + struct coresight_path *path, + struct perf_output_handle *handle) { - struct coresight_path *path = (struct coresight_path *)data; - return ctcu_set_etr_traceid(csdev, path, true); } -static int ctcu_disable(struct coresight_device *csdev, void *data) +static int ctcu_disable(struct coresight_device *csdev, + struct coresight_path *path) { - struct coresight_path *path = (struct coresight_path *)data; - return ctcu_set_etr_traceid(csdev, path, false); } diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index 8fb30dd73fd2..f92e3be4607c 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -799,14 +799,17 @@ static void cti_pm_release(struct cti_drvdata *drvdata) } /** cti ect operations **/ -int cti_enable(struct coresight_device *csdev, enum cs_mode mode, void *data) +int cti_enable(struct coresight_device *csdev, enum cs_mode mode, + struct coresight_path *path, + struct perf_output_handle *handle) { struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev); return cti_enable_hw(drvdata); } -int cti_disable(struct coresight_device *csdev, void *data) +int cti_disable(struct coresight_device *csdev, + struct coresight_path *path) { struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev); diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 572b80ee96fb..2bb6929eeb19 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -112,7 +112,7 @@ static ssize_t enable_store(struct device *dev, ret = pm_runtime_resume_and_get(dev->parent); if (ret) return ret; - ret = cti_enable(drvdata->csdev, CS_MODE_SYSFS, NULL); + ret = cti_enable(drvdata->csdev, CS_MODE_SYSFS, NULL, NULL); if (ret) pm_runtime_put(dev->parent); } else { diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index 8362a47c939c..73954369654c 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -216,8 +216,10 @@ int cti_add_connection_entry(struct device *dev, struct cti_drvdata *drvdata, const char *assoc_dev_name); struct cti_trig_con *cti_allocate_trig_con(struct device *dev, int in_sigs, int out_sigs); -int cti_enable(struct coresight_device *csdev, enum cs_mode mode, void *data); -int cti_disable(struct coresight_device *csdev, void *data); +int cti_enable(struct coresight_device *csdev, enum cs_mode mode, + struct coresight_path *path, + struct perf_output_handle *handle); +int cti_disable(struct coresight_device *csdev, struct coresight_path *path); void cti_write_all_hw_regs(struct cti_drvdata *drvdata); void cti_write_intack(struct device *dev, u32 ackval); void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value); diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 33e22b1ba043..65c4984d98c0 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -136,7 +136,7 @@ static inline void CS_UNLOCK(void __iomem *addr) void coresight_disable_path(struct coresight_path *path); int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, - void *sink_data); + struct perf_output_handle *handle); struct coresight_device *coresight_get_sink(struct coresight_path *path); struct coresight_device *coresight_get_sink_by_id(u32 id); struct coresight_device * diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index b07fcdb3fe1a..2ed7fa2366ce 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1325,9 +1325,9 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) } struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, - enum cs_mode mode, void *data) + enum cs_mode mode, + struct perf_output_handle *handle) { - struct perf_output_handle *handle = data; struct etr_perf_buffer *etr_perf; switch (mode) { diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 6541a27a018e..b6e2b00d393a 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -440,7 +440,8 @@ struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata); void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu); void tmc_etr_remove_catu_ops(void); struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, - enum cs_mode mode, void *data); + enum cs_mode mode, + struct perf_output_handle *handle); extern const struct attribute_group coresight_etr_group; #endif diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 4ac65c68bbf4..450cccd46f38 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -422,8 +422,10 @@ struct coresight_ops_source { */ struct coresight_ops_helper { int (*enable)(struct coresight_device *csdev, enum cs_mode mode, - void *data); - int (*disable)(struct coresight_device *csdev, void *data); + struct coresight_path *path, + struct perf_output_handle *data); + int (*disable)(struct coresight_device *csdev, + struct coresight_path *path); }; base-commit: 46a51f4f5edade43ba66b3c151f0e25ec8b69cb6 -- 2.39.5

4 months, 3 weeks

Re: [PATCH] coresight: Fix data argument to coresight_enable_helpers

by Carl Worth

Jie Gan <jie.gan(a)oss.qualcomm.com> writes: > I dont think we can change back to sink_data since we introduced > coresight_path to wrap 'data' which is needed by the path. > > I suggest you to add the struct perf_output_handle to the > coresight_path, then retrieving it with data->perf_handle in > tmc_etr_get_buffer. ... > We can assign the perf_output_handle to the coresight_path after we > constructed the coresight_path in perf mode. Thanks. That much makes sense to me, and I'll put together a patch along those lines. But, further: with core coresight code assembling into the path all the data that is necessary, is there any reason to be using void* in these enable/disable functions? Could we also change these functions to accept a coresight_path* and actually get some compiler help at finding mistakes like the one we're fixing here? -Carl

4 months, 3 weeks

Re: [PATCH v3 2/3] coresight: tmc: refactor the tmc-etr mode setting to avoid race conditions

by Suzuki K Poulose

On 18/09/2025 10:15, Yicong Yang wrote: > On 2025/8/18 16:05, Junhao He wrote: >> When trying to run perf and sysfs mode simultaneously, the WARN_ON() >> in tmc_etr_enable_hw() is triggered sometimes: >> >> WARNING: CPU: 42 PID: 3911571 at drivers/hwtracing/coresight/coresight-tmc-etr.c:1060 tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] >> [..snip..] >> Call trace: >> tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] (P) >> tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] (L) >> tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] >> coresight_enable_path+0x1c8/0x218 [coresight] >> coresight_enable_sysfs+0xa4/0x228 [coresight] >> enable_source_store+0x58/0xa8 [coresight] >> dev_attr_store+0x20/0x40 >> sysfs_kf_write+0x4c/0x68 >> kernfs_fop_write_iter+0x120/0x1b8 >> vfs_write+0x2c8/0x388 >> ksys_write+0x74/0x108 >> __arm64_sys_write+0x24/0x38 >> el0_svc_common.constprop.0+0x64/0x148 >> do_el0_svc+0x24/0x38 >> el0_svc+0x3c/0x130 >> el0t_64_sync_handler+0xc8/0xd0 >> el0t_64_sync+0x1ac/0x1b0 >> ---[ end trace 0000000000000000 ]--- >> >> Since the sysfs buffer allocation and the hardware enablement is not >> in the same critical region, it's possible to race with the perf >> >> mode: >> [sysfs mode] [perf mode] >> tmc_etr_get_sysfs_buffer() >> spin_lock(&drvdata->spinlock) >> [sysfs buffer allocation] >> spin_unlock(&drvdata->spinlock) >> spin_lock(&drvdata->spinlock) >> tmc_etr_enable_hw() >> drvdata->etr_buf = etr_perf->etr_buf >> spin_unlock(&drvdata->spinlock) >> spin_lock(&drvdata->spinlock) >> tmc_etr_enable_hw() >> WARN_ON(drvdata->etr_buf) // WARN sicne etr_buf initialized at >> the perf side >> spin_unlock(&drvdata->spinlock) >> >> A race condition is introduced here, perf always prioritizes execution, and >> warnings can lead to concerns about potential hidden bugs, such as getting >> out of sync. >> >> To fix this, configure the tmc-etr mode before invoking enable_etr_perf() >> or enable_etr_sysfs(), explicitly check if the tmc-etr sink is already >> enabled in a different mode, and simplily the setup and checks for "mode". >> To prevent race conditions between mode transitions. >> >> Fixes: 296b01fd106e ("coresight: Refactor out buffer allocation function for ETR") >> Reported-by: Yicong Yang <yangyicong(a)hisilicon.com> >> Closes: https://lore.kernel.org/linux-arm-kernel/20241202092419.11777-2-yangyicong@… >> Signed-off-by: Junhao He <hejunhao3(a)huawei.com> > > Tested by running perf and sysfs mode simultaneously, no warning reproduced. > > Tested-by: Yicong Yang <yangyicong(a)hisilicon.com> > >> --- >> .../hwtracing/coresight/coresight-tmc-etr.c | 80 ++++++++++--------- >> 1 file changed, 42 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> index b07fcdb3fe1a..06c74717be19 100644 >> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> @@ -1263,7 +1263,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) >> raw_spin_lock_irqsave(&drvdata->spinlock, flags); >> } >> >> - if (drvdata->reading || coresight_get_mode(csdev) == CS_MODE_PERF) { >> + if (drvdata->reading) { >> ret = -EBUSY; >> goto out; >> } >> @@ -1300,20 +1300,18 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) >> raw_spin_lock_irqsave(&drvdata->spinlock, flags); >> >> /* >> - * In sysFS mode we can have multiple writers per sink. Since this >> - * sink is already enabled no memory is needed and the HW need not be >> - * touched, even if the buffer size has changed. >> + * When two sysfs sessions race to acquire an idle sink, both may enter >> + * this function. We need to recheck if the sink is already in use to >> + * prevent duplicate hardware configuration. >> */ >> - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { >> + if (csdev->refcnt) { >> csdev->refcnt++; >> goto out; >> } >> >> ret = tmc_etr_enable_hw(drvdata, sysfs_buf); >> - if (!ret) { >> - coresight_set_mode(csdev, CS_MODE_SYSFS); >> + if (!ret) >> csdev->refcnt++; >> - } >> >> out: >> raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); >> @@ -1729,39 +1727,24 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) >> { >> int rc = 0; >> pid_t pid; >> - unsigned long flags; >> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >> struct perf_output_handle *handle = data; >> struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle); >> >> - raw_spin_lock_irqsave(&drvdata->spinlock, flags); >> - /* Don't use this sink if it is already claimed by sysFS */ >> - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { >> - rc = -EBUSY; >> - goto unlock_out; >> - } >> - >> - if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) { >> - rc = -EINVAL; >> - goto unlock_out; >> - } >> + if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) >> + return -EINVAL; >> >> /* Get a handle on the pid of the session owner */ >> pid = etr_perf->pid; >> >> /* Do not proceed if this device is associated with another session */ >> - if (drvdata->pid != -1 && drvdata->pid != pid) { >> - rc = -EBUSY; >> - goto unlock_out; >> - } >> + if (drvdata->pid != -1 && drvdata->pid != pid) >> + return -EBUSY; >> >> - /* >> - * No HW configuration is needed if the sink is already in >> - * use for this session. >> - */ >> + /* The sink is already in use for this session */ >> if (drvdata->pid == pid) { >> csdev->refcnt++; >> - goto unlock_out; >> + return rc; >> } >> >> rc = tmc_etr_enable_hw(drvdata, etr_perf->etr_buf); >> @@ -1773,22 +1756,43 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) >> csdev->refcnt++; >> } >> >> -unlock_out: >> - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); >> return rc; >> } >> >> static int tmc_enable_etr_sink(struct coresight_device *csdev, >> enum cs_mode mode, void *data) >> { >> - switch (mode) { >> - case CS_MODE_SYSFS: >> - return tmc_enable_etr_sink_sysfs(csdev); >> - case CS_MODE_PERF: >> - return tmc_enable_etr_sink_perf(csdev, data); >> - default: >> - return -EINVAL; >> + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >> + int rc; >> + >> + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { >> + if (coresight_get_mode(csdev) != CS_MODE_DISABLED && >> + coresight_get_mode(csdev) != mode) >> + return -EBUSY; >> + >> + switch (mode) { >> + case CS_MODE_SYSFS: >> + if (csdev->refcnt) { >> + /* The sink is already enabled */ >> + csdev->refcnt++; >> + return 0; >> + } >> + coresight_set_mode(csdev, mode); Why are we spilling bits here in the common code for sysfs ? More on this, see below. >> + break; >> + case CS_MODE_PERF: >> + return tmc_enable_etr_sink_perf(csdev, data); >> + default: >> + return -EINVAL; >> + } >> + } >> + >> + rc = tmc_enable_etr_sink_sysfs(csdev); We now call the above function without the spinlock and the refcnt is managed with and without the spinlock by the users. This is problematic, with refcnt being a non-atomic type. Please fix. I don't see why we can't set the mode in tmc_enable_etr_sink_sysfs() with the locks held and reset the mode if we failed enable it properly. Suzuki >> + if (rc) { >> + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) >> + coresight_set_mode(csdev, CS_MODE_DISABLED); >> } >> + >> + return rc; >> } >> >> static int tmc_disable_etr_sink(struct coresight_device *csdev) >>

4 months, 3 weeks

ITM support in OpenCSD

by Mike Leach

Hi, I will be making the OpenCSD ITM support in the development branch (itm-decoder-dev) part of the main upstream branch. If anyone has any feedback on this support please let me know. Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months, 3 weeks

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight