CoreSight

coresight@lists.linaro.org

7 participants
2545 discussions

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 >

2 months, 1 week

[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

2 months, 1 week

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

2 months, 1 week

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) >>

2 months, 1 week

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

2 months, 1 week

[PATCH 0/2] perf: Fix overflow in PERF_IDX2OFF() for Arm SPE and TRBE drivers

by Leo Yan

If the AUX buffer size is specified as 2 GiB or larger, the expression "(buf)->nr_pages << PAGE_SHIFT" may exceed 0x8000_0000. Since (buf)->nr_pages is a signed integer, the shift can overflow and produce a negative value. As a result, PERF_IDX2OFF() fails to work correctly. Fix this by casting (buf)->nr_pages to unsigned long before the shift, which allows PERF_IDX2OFF() to handle large buffers properly. Signed-off-by: Leo Yan <leo.yan(a)arm.com> --- Leo Yan (2): coresight: trbe: Prevent overflow in PERF_IDX2OFF() perf: arm_spe: Prevent overflow in PERF_IDX2OFF() drivers/hwtracing/coresight/coresight-trbe.c | 3 ++- drivers/perf/arm_spe_pmu.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) --- base-commit: 5aca7966d2a7255ba92fd5e63268dd767b223aa5 change-id: 20250917-fix_aux_trace_index-9745674f5061 Best regards, -- Leo Yan <leo.yan(a)arm.com>

2 months, 1 week

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

by Leo Yan

On Wed, Sep 17, 2025 at 09:35:55AM +0800, Jie Gan wrote: [...] > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > > index fa758cc21827..b1077d73c988 100644 > > --- a/drivers/hwtracing/coresight/coresight-core.c > > +++ b/drivers/hwtracing/coresight/coresight-core.c > > @@ -510,7 +510,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, sink_data); > > I dont think we can change back to sink_data since we introduced > coresight_path to wrap 'data' which is needed by the path. This change can fix catu issue but will cause regression for ctcu driver. > 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. > > before: > struct perf_output_handle *handle = data; > > after: > struct coresight_path *path = data; > struct perf_output_handle *handle = path->perf_handle; > > We can assign the perf_output_handle to the coresight_path after we > constructed the coresight_path in perf mode. The suggestion looks good to me. Thanks, Leo

2 months, 1 week

Re: [PATCH v3] coresight: Fix possible deadlock in coresight_panic_cb

by Leo Yan

On Tue, Sep 16, 2025 at 12:51:11PM -0400, Sean Anderson wrote: > On 9/16/25 12:48, Leo Yan wrote: > > On Tue, Sep 16, 2025 at 12:14:40PM -0400, Sean Anderson wrote: > > > > [...] > > > >> > Could you check if the drafted patch below looks good to you? If so, I > >> > >> As stated above I disagree with a half-hearted removal. If you want to do that, > >> then I will resend v2 done with an rcu list and you can make your own follow-up. > > > > It is fine to disagree, but please don't resend v2 :) > > > > We have plan to refactor locking in CoreSight driver, I will try my > > best to avoid adding new lock unless with a strong reason. > > As said above it will be done with an rcu list, so no new lock. > > Or I can do this patch but stick the notifier block in csdev as suggested by Suzuki. I am fine for adding the notifier block in csdev. Suzuki, could you confirm if this is the right way to move forward? Thanks, Leo

2 months, 1 week

[PATCH v4 0/3] coresight-tnoc: Add support for Interconnect TNOC

by Yuanfang Zhang

This patch series adds support for the Qualcomm CoreSight Interconnect TNOC (Trace Network On Chip) block, which acts as a CoreSight graph link forwarding trace data from subsystems to the Aggregator TNOC. Unlike the Aggregator TNOC, this block does not support aggregation or ATID assignment. Signed-off-by: Yuanfang Zhang <yuanfang.zhang(a)oss.qualcomm.com> --- Changes in v4: - Fix unintended blank line removals in trace_noc_enable_hw. - Link to v3: https://lore.kernel.org/r/20250828-itnoc-v3-0-f1b55dea7a27@oss.qualcomm.com Changes in v3: - Add detail for changes in V2. - Remove '#address-cells' and '#size-cells' properties from in-ports field. - Fix comment indentation for packet description. - Link to v2: https://lore.kernel.org/r/20250819-itnoc-v2-0-2d0e6be44e2f@oss.qualcomm.com Changes in v2: - Removed the trailing '|' after the description in qcom,coresight-itnoc.yaml. - Dropped the 'select' section from the YAML file. - Updated node name to use a more generic naming convention. - Removed the 'items' property from the compatible field. - Deleted the description for the reg property. - Dropped clock-names and adjusted the order of clock-names and clocks. - Moved additionalProperties to follow the $ref of out-ports. - Change "atid" type from u32 to int, set it as "-EOPNOTSUPP" for non-AMBA device. - Link to v1: https://lore.kernel.org/r/20250815-itnoc-v1-0-62c8e4f7ad32@oss.qualcomm.com --- Yuanfang Zhang (3): dt-bindings: arm: qcom: Add Coresight Interconnect TNOC coresight-tnoc: add platform driver to support Interconnect TNOC coresight-tnoc: Add runtime PM support for Interconnect TNOC .../bindings/arm/qcom,coresight-itnoc.yaml | 90 ++++++++++++++ drivers/hwtracing/coresight/coresight-tnoc.c | 136 +++++++++++++++++++-- 2 files changed, 215 insertions(+), 11 deletions(-) --- base-commit: 2b52cf338d39d684a1c6af298e8204902c026aca change-id: 20250815-itnoc-460273d1b80c Best regards, -- Yuanfang Zhang <yuanfang.zhang(a)oss.qualcomm.com>

2 months, 1 week

Re: [PATCH v3] coresight: Fix possible deadlock in coresight_panic_cb

by Leo Yan

On Mon, Sep 15, 2025 at 10:31:24AM -0400, Sean Anderson wrote: > On 9/15/25 05:58, Leo Yan wrote: > > On Fri, Sep 12, 2025 at 11:13:14AM -0400, Sean Anderson wrote: > >> coresight_panic_cb is called with interrupts disabled during panics. > >> However, bus_for_each_dev calls bus_to_subsys which takes > >> bus_kset->list_lock without disabling IRQs. This may cause a deadlock. > > > > I would rephrase it to make it clearer for anyone reading it later: > > > > coresight_panic_cb() is called during panics, which can preempt a flow > > that triggers exceptions (such as data or instruction aborts). > > I don't see what exceptions have to do with it. You can also panic > during a regular interrupt. The commit mentioned "without disabling IRQs" gives the impression that the deadlock is caused by IRQ-unsafe locking, which might mislead into thinking why the issue cannot be fixed with IRQ-safe locking. Regardless of whether IRQs are disabled, and regardless of the context (interrupt, bottom-half, or normal thread), the conditions for the deadlock are only about: (a) The bus lock has been acquired; (b) A panic is triggered to try to acquire the same lock. [...] > > When I review this patch, I recognize we can consolidate panic notifier > > in coresight-tmc-core.c, so we don't need to distribute the changes > > into ETF and ETR drivers (sorry if I misled you in my previous reply). > > And this kind of thing is why I went with the straightforward fix > initially. I do not want to bikeshed the extent that this gets removed. > IMO the whole "panic ops" stuff should be done directly with the panic > notifier, hence this patch. If you do not agree with that, then ack v2 > and send a follow up of your own to fix it how you see fit. I would fix it in one go. I agree with you that "the whole panic ops stuff should be done directly with the panic". The only difference between us is that I would keep the `panic_ops` callback. To me, this encapsulates panic callbacks into different modules, to make the code more general. Could you check if the drafted patch below looks good to you? If so, I will send out a formal patch. ---8<--- From ea78dd22cbdd97f709c5991d5bd3be97be6e137e Mon Sep 17 00:00:00 2001 From: Sean Anderson <sean.anderson(a)linux.dev> Date: Tue, 16 Sep 2025 16:03:58 +0100 Subject: [PATCH] coresight: Fix possible deadlock in coresight_panic_cb() coresight_panic_cb() is called during a panic. It invokes bus_for_each_dev(), which then calls bus_to_subsys() and takes the 'bus_kset->list_lock'. If a panic occurs after the lock has been acquired, it can lead to a deadlock. Instead of using a common panic notifier to iterate the bus, this commit directly registers the TMC device's panic notifier. This avoids bus iteration and effectively eliminates the race condition that could cause the deadlock. Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks") Signed-off-by: Sean Anderson <sean.anderson(a)linux.dev> Signed-off-by: Leo Yan <leo.yan(a)arm.com> --- drivers/hwtracing/coresight/coresight-core.c | 42 ------------------- .../hwtracing/coresight/coresight-tmc-core.c | 26 ++++++++++++ drivers/hwtracing/coresight/coresight-tmc.h | 2 + 3 files changed, 28 insertions(+), 42 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 3267192f0c1c..cb0cc8d77056 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -21,7 +21,6 @@ #include <linux/property.h> #include <linux/delay.h> #include <linux/pm_runtime.h> -#include <linux/panic_notifier.h> #include "coresight-etm-perf.h" #include "coresight-priv.h" @@ -1566,36 +1565,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; @@ -1608,20 +1577,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); @@ -1631,8 +1591,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/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 36599c431be6..108ed9daf56d 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -21,6 +21,7 @@ #include <linux/slab.h> #include <linux/dma-mapping.h> #include <linux/spinlock.h> +#include <linux/panic_notifier.h> #include <linux/pm_runtime.h> #include <linux/of.h> #include <linux/of_address.h> @@ -769,6 +770,21 @@ static void register_crash_dev_interface(struct tmc_drvdata *drvdata, "Valid crash tracedata found\n"); } +static int tmc_panic_cb(struct notifier_block *nb, unsigned long v, void *p) +{ + struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata, + panic_notifier); + struct coresight_device *csdev = drvdata->csdev; + + if (coresight_get_mode(csdev) == CS_MODE_DISABLED) + return 0; + + if (panic_ops(csdev)) + panic_ops(csdev)->sync(csdev); + + return 0; +} + static int __tmc_probe(struct device *dev, struct resource *res) { int ret = 0; @@ -885,6 +901,12 @@ static int __tmc_probe(struct device *dev, struct resource *res) goto out; } + if (panic_ops(drvdata->csdev)) { + drvdata->panic_notifier.notifier_call = tmc_panic_cb; + atomic_notifier_chain_register(&panic_notifier_list, + &drvdata->panic_notifier); + } + out: if (is_tmc_crashdata_valid(drvdata) && !tmc_prepare_crashdata(drvdata)) @@ -929,6 +951,10 @@ static void __tmc_remove(struct device *dev) { struct tmc_drvdata *drvdata = dev_get_drvdata(dev); + if (panic_ops(drvdata->csdev)) + atomic_notifier_chain_unregister(&panic_notifier_list, + &drvdata->panic_notifier); + /* * Since misc_open() holds a refcount on the f_ops, which is * etb fops in this case, device is there until last file diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index cbb4ba439158..873c5427673c 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -243,6 +243,7 @@ struct tmc_resrv_buf { * (after crash) by default. * @crash_mdata: Reserved memory for storing tmc crash metadata. * Used by ETR/ETF. + * @panic_notifier: Notifier used to clean up during a panic */ struct tmc_drvdata { struct clk *atclk; @@ -273,6 +274,7 @@ struct tmc_drvdata { struct etr_buf *perf_buf; struct tmc_resrv_buf resrv_buf; struct tmc_resrv_buf crash_mdata; + struct notifier_block panic_notifier; }; struct etr_buf_operations { -- 2.34.1

2 months, 1 week

← Newer
1
...
5
6
7
8
9
10
11
...
255
Older →

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight