CoreSight

coresight@lists.linaro.org

6 participants
2630 discussions

Re: [PATCH v6 4/9] coresight: tmc: add create/clean functions for etr_buf_list

by Mike Leach

On Mon, 8 Sept 2025 at 03:02, Jie Gan <jie.gan(a)oss.qualcomm.com> wrote: > > Create and insert or remove the etr_buf_node to/from the etr_buf_list. > > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > --- > drivers/hwtracing/coresight/coresight-tmc-etr.c | 94 +++++++++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tmc.h | 2 + > 2 files changed, 96 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index b07fcdb3fe1a..ed15991b3217 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1909,6 +1909,100 @@ const struct coresight_ops tmc_etr_cs_ops = { > .panic_ops = &tmc_etr_sync_ops, > }; > > +/** > + * tmc_clean_etr_buf_list - clean the etr_buf_list. > + * @drvdata: driver data of the TMC device. > + * > + * Remove the allocated node from the list and free the extra buffer. > + */ > +void tmc_clean_etr_buf_list(struct tmc_drvdata *drvdata) > +{ > + struct etr_buf_node *nd, *next; > + > + list_for_each_entry_safe(nd, next, &drvdata->etr_buf_list, node) { > + if (nd->sysfs_buf == drvdata->sysfs_buf) { > + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) { > + drvdata->sysfs_buf = NULL; > + tmc_free_etr_buf(nd->sysfs_buf); > + nd->sysfs_buf = NULL; > + } > + list_del(&nd->node); > + kfree(nd); > + } else { > + /* Free allocated buffers which are not utilized by ETR */ > + list_del(&nd->node); > + tmc_free_etr_buf(nd->sysfs_buf); > + nd->sysfs_buf = NULL; > + kfree(nd); > + } > + } > +} > +EXPORT_SYMBOL_GPL(tmc_clean_etr_buf_list); > + > +/** > + * tmc_create_etr_buf_list - create a list to manage the etr_buf_node. > + * @drvdata: driver data of the TMC device. > + * @num_nodes: number of nodes want to create with the list. > + * > + * Return 0 upon success and return the error number if fail. > + */ > +int tmc_create_etr_buf_list(struct tmc_drvdata *drvdata, int num_nodes) > +{ > + struct etr_buf_node *new_node; > + struct etr_buf *sysfs_buf; > + int i = 0, ret = 0; > + > + /* We dont need a list if there is only one node */ > + if (num_nodes < 2) > + return -EINVAL; > + > + /* We expect that sysfs_buf in drvdata has already been allocated. */ > + if (drvdata->sysfs_buf) { > + /* Directly insert the allocated sysfs_buf into the list first */ > + new_node = kzalloc(sizeof(struct etr_buf_node), GFP_KERNEL); > + if (IS_ERR(new_node)) > + return PTR_ERR(new_node); > + > + new_node->sysfs_buf = drvdata->sysfs_buf; > + new_node->is_free = false; > + list_add(&new_node->node, &drvdata->etr_buf_list); > + i++; > + } > + > + while (i < num_nodes) { > + new_node = kzalloc(sizeof(struct etr_buf_node), GFP_KERNEL); > + if (IS_ERR(new_node)) { > + ret = PTR_ERR(new_node); > + break; > + } > + > + sysfs_buf = tmc_alloc_etr_buf(drvdata, drvdata->size, 0, cpu_to_node(0), NULL); > + if (IS_ERR(sysfs_buf)) { > + kfree(new_node); > + ret = PTR_ERR(new_node); > + break; > + } > + > + /* We dont have a available sysfs_buf in drvdata, setup one */ > + if (!drvdata->sysfs_buf) { > + drvdata->sysfs_buf = sysfs_buf; > + new_node->is_free = false; > + } else > + new_node->is_free = true; > + > + new_node->sysfs_buf = sysfs_buf; > + list_add(&new_node->node, &drvdata->etr_buf_list); > + i++; > + } > + > + /* Clean the list if there is an error */ > + if (ret) > + tmc_clean_etr_buf_list(drvdata); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(tmc_create_etr_buf_list); > + > int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) > { > int ret = 0; > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h > index 292e25d82b62..ca0cba860d5f 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc.h > +++ b/drivers/hwtracing/coresight/coresight-tmc.h > @@ -459,5 +459,7 @@ void tmc_etr_remove_catu_ops(void); > struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, > enum cs_mode mode, void *data); > extern const struct attribute_group coresight_etr_group; > +void tmc_clean_etr_buf_list(struct tmc_drvdata *drvdata); > +int tmc_create_etr_buf_list(struct tmc_drvdata *drvdata, int num_nodes); > > #endif > > -- > 2.34.1 > Reviewed-by: Mike Leach <mike.leach(a)linaro.org> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

2 months, 1 week

Re: [PATCH v6 3/9] coresight: tmc: add etr_buf_list to store allocated etr_buf

by Mike Leach

On Mon, 8 Sept 2025 at 03:02, Jie Gan <jie.gan(a)oss.qualcomm.com> wrote: > > Add a list to store allocated etr_buf. > > The byte-cntr functionality requires two etr_buf to receive trace data. > The active etr_buf collects the trace data from source device, while the > byte-cntr reading function accesses the deactivated etr_buf after is > has been filled and synced, transferring data to the userspace. > > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > --- > drivers/hwtracing/coresight/coresight-tmc-core.c | 1 + > drivers/hwtracing/coresight/coresight-tmc.h | 17 +++++++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c > index be964656be93..4d249af93097 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c > @@ -830,6 +830,7 @@ static int __tmc_probe(struct device *dev, struct resource *res) > idr_init(&drvdata->idr); > mutex_init(&drvdata->idr_mutex); > dev_list = &etr_devs; > + INIT_LIST_HEAD(&drvdata->etr_buf_list); > break; > case TMC_CONFIG_TYPE_ETF: > desc.groups = coresight_etf_groups; > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h > index 6541a27a018e..292e25d82b62 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc.h > +++ b/drivers/hwtracing/coresight/coresight-tmc.h > @@ -208,6 +208,19 @@ struct tmc_resrv_buf { > s64 len; > }; > > +/** > + * @sysfs_buf: Allocated sysfs_buf. > + * @is_free: Indicates whether the buffer is free to choose. > + * @pos: Position of the buffer. > + * @node: Node in etr_buf_list. > + */ > +struct etr_buf_node { > + struct etr_buf *sysfs_buf; > + bool is_free; > + loff_t pos; > + struct list_head node; > +}; > + > /** > * struct tmc_drvdata - specifics associated to an TMC component > * @pclk: APB clock if present, otherwise NULL > @@ -242,6 +255,8 @@ struct tmc_resrv_buf { > * (after crash) by default. > * @crash_mdata: Reserved memory for storing tmc crash metadata. > * Used by ETR/ETF. > + * @etr_buf_list: List that is used to manage allocated etr_buf. > + * @reading_node: Available buffer for byte-cntr reading. > */ > struct tmc_drvdata { > struct clk *pclk; > @@ -271,6 +286,8 @@ struct tmc_drvdata { > struct etr_buf *perf_buf; > struct tmc_resrv_buf resrv_buf; > struct tmc_resrv_buf crash_mdata; > + struct list_head etr_buf_list; > + struct etr_buf_node *reading_node; > }; > > struct etr_buf_operations { > > -- > 2.34.1 > Reviewed-by: Mike Leach <mike.leach(a)linaro.org> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

2 months, 1 week

Re: [PATCH v3] coresight: etm3x: Fix cntr_val_show() to match cntr_val_store() behavior

by James Clark

On 02/12/2025 8:26 am, Kuan-Wei Chiu wrote: > The cntr_val_show() function was intended to print the values of all > counters using a loop. However, due to a buffer overwrite issue with > sprintf(), it effectively only displayed the value of the last counter. > > The companion function, cntr_val_store(), allows users to modify a > specific counter selected by 'cntr_idx'. To maintain consistency > between read and write operations and to align with the ETM4x driver > behavior, modify cntr_val_show() to report only the value of the > currently selected counter. > > This change removes the loop and the "counter %d:" prefix, printing > only the hexadecimal value. It also adopts sysfs_emit() for standard > sysfs output formatting. > > Fixes: a939fc5a71ad ("coresight-etm: add CoreSight ETM/PTM driver") > Cc: stable(a)vger.kernel.org > Signed-off-by: Kuan-Wei Chiu <visitorckw(a)gmail.com> > --- > Build test only. > > Changes in v3: > - Switch format specifier to %#x to include the 0x prefix. > - Add Cc stable > > v2: https://lore.kernel.org/lkml/20251201095228.1905489-1-visitorckw@gmail.com/ > > .../hwtracing/coresight/coresight-etm3x-sysfs.c | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > index 762109307b86..b3c67e96a82a 100644 > --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > @@ -717,26 +717,19 @@ static DEVICE_ATTR_RW(cntr_rld_event); > static ssize_t cntr_val_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - int i, ret = 0; > u32 val; > struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct etm_config *config = &drvdata->config; > > if (!coresight_get_mode(drvdata->csdev)) { > spin_lock(&drvdata->spinlock); > - for (i = 0; i < drvdata->nr_cntr; i++) > - ret += sprintf(buf, "counter %d: %x\n", > - i, config->cntr_val[i]); > + val = config->cntr_val[config->cntr_idx]; > spin_unlock(&drvdata->spinlock); > - return ret; > - } > - > - for (i = 0; i < drvdata->nr_cntr; i++) { > - val = etm_readl(drvdata, ETMCNTVRn(i)); > - ret += sprintf(buf, "counter %d: %x\n", i, val); > + } else { > + val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx)); > } > > - return ret; > + return sysfs_emit(buf, "%#x\n", val); > } > > static ssize_t cntr_val_store(struct device *dev, Reviewed-by: James Clark <james.clark(a)linaro.org>

2 months, 1 week

Re: [PATCH v2] coresight: etm3x: Fix cntr_val_show() to match cntr_val_store() behavior

by James Clark

On 01/12/2025 9:52 am, Kuan-Wei Chiu wrote: > The cntr_val_show() function was intended to print the values of all > counters using a loop. However, due to a buffer overwrite issue with > sprintf(), it effectively only displayed the value of the last counter. > > The companion function, cntr_val_store(), allows users to modify a > specific counter selected by 'cntr_idx'. To maintain consistency > between read and write operations and to align with the ETM4x driver > behavior, modify cntr_val_show() to report only the value of the > currently selected counter. > > This change removes the loop and the "counter %d:" prefix, printing > only the hexadecimal value. It also adopts sysfs_emit() for standard > sysfs output formatting. > > Fixes: a939fc5a71ad ("coresight-etm: add CoreSight ETM/PTM driver") > Signed-off-by: Kuan-Wei Chiu <visitorckw(a)gmail.com> > --- > Build test only. > > Changes in v2: > - Redesigned the fix to match cntr_val_store() logic (display only the > currently selected counter) instead of attempting to list all > counters. > - Removed the loop and the "counter %d:" prefix. > - Switched from sprintf() to sysfs_emit(). > - Refactored control flow to use a single return point. > > v1: https://lore.kernel.org/lkml/20251121002350.1166758-1-visitorckw@gmail.com/ > > .../hwtracing/coresight/coresight-etm3x-sysfs.c | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > index 762109307b86..a6a650331196 100644 > --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > @@ -717,26 +717,19 @@ static DEVICE_ATTR_RW(cntr_rld_event); > static ssize_t cntr_val_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - int i, ret = 0; > u32 val; > struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct etm_config *config = &drvdata->config; > > if (!coresight_get_mode(drvdata->csdev)) { > spin_lock(&drvdata->spinlock); > - for (i = 0; i < drvdata->nr_cntr; i++) > - ret += sprintf(buf, "counter %d: %x\n", > - i, config->cntr_val[i]); > + val = config->cntr_val[config->cntr_idx]; > spin_unlock(&drvdata->spinlock); > - return ret; > - } > - > - for (i = 0; i < drvdata->nr_cntr; i++) { > - val = etm_readl(drvdata, ETMCNTVRn(i)); > - ret += sprintf(buf, "counter %d: %x\n", i, val); > + } else { > + val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx)); > } > > - return ret; > + return sysfs_emit(buf, "%x\n", val); Sorry I missed this on the previous version, but this should be %#x to add the "0x" prefix. All the other ones in this file have it. Otherwise you can't tell the difference between decimal and hex. > } > > static ssize_t cntr_val_store(struct device *dev,

2 months, 1 week

Re: [PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val_show()

by Leo Yan

On Fri, Nov 21, 2025 at 12:23:50AM +0000, Kuan-Wei Chiu wrote: [...] > I noticed this issue while browsing the coresight code after attending > a technical talk on the subject. This code dates back to the initial > driver submission over 10 years ago, so I was surprised it hadn't been > caught earlier. Although I cannot perform runtime testing, the logic > error seems obvious to me, so I still decided to submit this patch. I have a question for maintainers. The ETMv4 architecture specification shows that ETMv4 was released as a non-confidential module in May 2013 (with the confidential release even a year earlier). So ETMv4 has been a public IP for more than 12+ years, and ETMv3 has been gradually retired since then. This fix can still be applied to older kernels, but seems to me that now might be an appropriate time to consider removing the ETMv3 driver from the mainline kernel? Thanks, Leo

2 months, 2 weeks

[PATCH v7 00/13] coresight: Update timestamp attribute to be an interval instead of bool

by James Clark

Do some cleanups then expand the timestamp format attribute from 1 bit to 4 bits for ETMv4 in Perf mode. The current interval is too high for most use cases, and particularly on the FVP the number of timestamps generated is excessive. This change not only still allows disabling or enabling timestamps, but also allows the interval to be configured. The old bit is kept deprecated and undocumented for now. There are known broken versions of Perf that don't read the format attribute positions from sysfs and instead hard code the timestamp bit. We can leave the old bit in the driver until we need the bit for another feature or enough time has passed that these old Perfs are unlikely to be used. The interval option is added as an event format attribute, rather than a Coresight config because it's something that the driver is already configuring automatically in Perf mode using any unused counter, so it's not possible to modify this with a config. Applies to coresight/next Signed-off-by: James Clark <james.clark(a)linaro.org> --- Changes in v7: - Change TRCCNTCTLRn and TRCTSCTLR register definitions to use a combined field for TYPE and SEL (EVENT) so that they can be used with the new utilities. - Add utility functions for creating resource selectors that do compile and runtime checking of the resource selector ID. - Link to v6: https://lore.kernel.org/r/20251119-james-cs-syncfreq-v6-0-740d24a29e51@lina… Changes in v6: - #ifdef out format attributes for ETMv3 instead of using is_visible(). Then the same block can be used to define format_attr_contextid_show() which avoids an awkward WARN_ONCE() and comments in arm32 for a function that's never called. - Link to v5: https://lore.kernel.org/r/20251118-james-cs-syncfreq-v5-0-82efd7b1a751@lina… Changes in v5: - Add parens to interval calculation in docs (Randy) - Swap "minimum interval" and "maximum interval" in docs. (Leo) - Add TRCSYNCPR.PERIOD to docs (Leo) - Use CONFIG_ARM64 to avoid is_kernel_in_hyp_mode() (Leo) - Add a comment for hidden ETMv3 format attributes (Leo) - Hide configid for ETMv3 (Leo) - Link to v4: https://lore.kernel.org/r/20251112-james-cs-syncfreq-v4-0-165ba21401dc@lina… Changes in v4: - Add #defines for true and false resources ETM_RES_SEL_TRUE/FALSE - Reword comment about finding a counter to say if there are no resources there are no counters. - Extend existing timestamp format attribute instead of adding a new one - Refactor all the config definitions and parsing to use GEN_PMU_FORMAT_ATTR()/ATTR_CFG_GET_FLD() so we can see where the unused bits are. - Link to v3: https://lore.kernel.org/r/20251002-james-cs-syncfreq-v3-0-fe5df2bf91d1@lina… Changes in v3: - Move the format attr definitions to coresight-etm-perf.h we can compile on arm32 without #ifdefs - (Leo) - Convert the new #ifdefs to a single one in an is_visible() function so that the code is cleaner - (Leo) - Drop the change to remove the holes in struct etmv4_config as they were grouped by function - (Mike) - Link to v2: https://lore.kernel.org/r/20250814-james-cs-syncfreq-v2-0-c76fcb87696d@lina… Changes in v2: - Only show the attribute for ETMv4 to improve usability and fix the arm32 build error. Wrapping everything in IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) isn't ideal, but the -perf.c file is shared between ETMv3 and ETMv4, and there is already precedent for doing it this way. - Link to v1: https://lore.kernel.org/r/20250811-james-cs-syncfreq-v1-0-b001cd6e3404@lina… --- James Clark (13): coresight: Change syncfreq to be a u8 coresight: Repack struct etmv4_drvdata coresight: Refactor etm4_config_timestamp_event() coresight: Hide unused ETMv3 format attributes coresight: Define format attributes with GEN_PMU_FORMAT_ATTR() coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD() coresight: Don't reject unrecognized ETMv3 format attributes coresight: Interpret perf config with ATTR_CFG_GET_FLD() coresight: Interpret ETMv4 config with ATTR_CFG_GET_FLD() coresight: Remove misleading definitions coresight: Extend width of timestamp format attribute coresight: Allow setting the timestamp interval coresight: docs: Document etm4x timestamp interval option Documentation/trace/coresight/coresight.rst | 16 +- drivers/hwtracing/coresight/coresight-etm-perf.c | 68 ++++----- drivers/hwtracing/coresight/coresight-etm-perf.h | 39 +++++ drivers/hwtracing/coresight/coresight-etm3x-core.c | 36 ++--- drivers/hwtracing/coresight/coresight-etm4x-core.c | 164 +++++++++++++-------- drivers/hwtracing/coresight/coresight-etm4x.h | 92 +++++++++--- include/linux/coresight-pmu.h | 24 --- 7 files changed, 275 insertions(+), 164 deletions(-) --- base-commit: 9e9182cab5ebc3ee7544e60ef08ba19fdf216920 change-id: 20250724-james-cs-syncfreq-7c2257a38ed3 Best regards, -- James Clark <james.clark(a)linaro.org>

2 months, 2 weeks

Re: [PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val_show()

by Leo Yan

On Thu, Nov 27, 2025 at 04:44:53PM +0800, Kuan-Wei Chiu wrote: [...] > I don't feel it is my place to say whether the etm3x driver should be > removed entirely. Sorry for confusion. Your fix patch is welcome, this is useful no matter if remove the ETMv3 driver or not. > However, if we decide to keep it, I agree that aligning cntr_val_show > with the cntr_val_store behavior (using cntr_idx) makes more sense. > > Here is my plan for v2: > > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > index 762109307b86..77578885e8f3 100644 > --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > @@ -717,26 +717,19 @@ static DEVICE_ATTR_RW(cntr_rld_event); > static ssize_t cntr_val_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - int i, ret = 0; > u32 val; > struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct etm_config *config = &drvdata->config; > > if (!coresight_get_mode(drvdata->csdev)) { > spin_lock(&drvdata->spinlock); > - for (i = 0; i < drvdata->nr_cntr; i++) > - ret += sprintf(buf, "counter %d: %x\n", > - i, config->cntr_val[i]); > + val = config->cntr_val[config->cntr_idx]; > spin_unlock(&drvdata->spinlock); > - return ret; > - } > - > - for (i = 0; i < drvdata->nr_cntr; i++) { > - val = etm_readl(drvdata, ETMCNTVRn(i)); > - ret += sprintf(buf, "counter %d: %x\n", i, val); > + return sprintf(buf, "%x\n", val); > } > > - return ret; > + val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx)); It is not right to read register at here (it cannot promise to read the CPU (cp14) register on the target CPU). Please refer to the same function in coresight-etm4x-sysfs.c. I think we can do the same thing at here. > + return sprintf(buf, "%x\n", val); > } > > static ssize_t cntr_val_store(struct device *dev, > > > Given the upcoming merge window, I plan to submit this v2 after -rc1 > is released. > > Alternatively, if the consensus is to drop the driver, I am happy to > submit a patch for that instead. Please continue this patch. Thanks a lot! Leo

2 months, 2 weeks

Re: [PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val_show()

by James Clark

On 27/11/2025 8:44 am, Kuan-Wei Chiu wrote: > On Wed, Nov 26, 2025 at 10:57:23AM +0000, James Clark wrote: >> >> >> On 26/11/2025 10:49 am, Mike Leach wrote: >>> Hi, >>> >>> On Mon, 24 Nov 2025 at 16:12, James Clark <james.clark(a)linaro.org> wrote: >>>> >>>> >>>> >>>> On 21/11/2025 5:02 pm, Kuan-Wei Chiu wrote: >>>>> Hi James, >>>>> >>>>> On Fri, Nov 21, 2025 at 09:50:03AM +0000, James Clark wrote: >>>>>> >>>>>> >>>>>> On 21/11/2025 12:23 am, Kuan-Wei Chiu wrote: >>>>>>> The cntr_val_show() function is meant to display the values of all >>>>>>> available counters. However, the sprintf() call inside the loop was >>>>>>> always writing to the beginning of the buffer, causing the output of >>>>>>> previous iterations to be overwritten. As a result, only the value of >>>>>>> the last counter was actually returned to the user. >>>>>>> >>>>>>> Fix this by using the return value of sprintf() to calculate the >>>>>>> correct offset into the buffer for the next write, ensuring that all >>>>>>> counter values are appended sequentially. >>>>>>> >>>>>>> Fixes: a939fc5a71ad ("coresight-etm: add CoreSight ETM/PTM driver") >>>>>>> Signed-off-by: Kuan-Wei Chiu <visitorckw(a)gmail.com> >>>>>>> --- >>>>>>> Build tested only. I do not have the hardware to run the etm3x driver, >>>>>>> so I would be grateful if someone could verify this on actual hardware. >>>>>>> >>>>>>> I noticed this issue while browsing the coresight code after attending >>>>>>> a technical talk on the subject. This code dates back to the initial >>>>>>> driver submission over 10 years ago, so I was surprised it hadn't been >>>>>>> caught earlier. Although I cannot perform runtime testing, the logic >>>>>>> error seems obvious to me, so I still decided to submit this patch. >>>>>> >>>>>> Nice find. I think the point that it wasn't caught changes how we fix it. >>>>>> Either nobody used it ever - so we can just delete it. Or someone was using >>>>>> it and they expect it to always return a single entry with the value of the >>>>>> last counter and this is a potentially breaking change. So maybe instead of >>>>>> fixing this we should add a new cntr_vals_show() which works correctly. But >>>>>> then again if nobody is using it we shouldn't do that either. >>>>> >>>>> Thanks for your feedback. >>>>> >>>>> I agree that if any tool relies on the current behavior, this patch >>>>> would break the ABI and violate the hard rule that we must never break >>>>> userspace. >>>>> >>>>> However, I am not sure how to determine if any userspace tools are >>>>> actually using this sysfs interface. Is there a recommended way to >>>>> verify this, or a standard procedure/convention to follow in this >>>>> situation? >>>>> >>>>> Regards, >>>>> Kuan-Wei >>>>> >>>> >>>> There's no way of knowing apart from deducing that there are 0 users of >>>> the 'correct' version of the API because it never existed. This isn't >>>> even a regression, it was broken from the beginning. >>>> >>>> I suppose it does work when hardware only has one counter, but I don't >>>> know how likely that is? >>>> >>>> Looking at cntr_val_store() I think I was wrong before when I said it >>>> should be a separate file for each counter. Writing to it already writes >>>> to the counter currently selected by cntr_idx and splitting it out to >>>> separate files would have to break that too. So we can fix the display >>>> bug by making show operate on the currently selected counter in the same >>>> way as store. Then it makes a bit more sense and we can delete the >>>> "counter %d: " prefix. >>>> >>>> ETM4 already does it this way too. >>>> >>> I agree with this. >>> >>> The difficulty in having a file per counter is that different >>> implementations have different numbers of counters. Rather than >>> complicate the driver by dynamically generating the sysfs files, the >> >> It wouldn't be that hard because there is a maximum number of counters. >> You'd generate them all and then use the is_visible() thing to hide ones >> that didn't exist on that device. >> >> But it looks like we don't want to do it that way for other reasons anyway. >> > > I don't feel it is my place to say whether the etm3x driver should be > removed entirely. > > However, if we decide to keep it, I agree that aligning cntr_val_show > with the cntr_val_store behavior (using cntr_idx) makes more sense. > > Here is my plan for v2: > > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > index 762109307b86..77578885e8f3 100644 > --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c > @@ -717,26 +717,19 @@ static DEVICE_ATTR_RW(cntr_rld_event); > static ssize_t cntr_val_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - int i, ret = 0; > u32 val; > struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct etm_config *config = &drvdata->config; > > if (!coresight_get_mode(drvdata->csdev)) { > spin_lock(&drvdata->spinlock); > - for (i = 0; i < drvdata->nr_cntr; i++) > - ret += sprintf(buf, "counter %d: %x\n", > - i, config->cntr_val[i]); > + val = config->cntr_val[config->cntr_idx]; > spin_unlock(&drvdata->spinlock); > - return ret; > - } > - > - for (i = 0; i < drvdata->nr_cntr; i++) { > - val = etm_readl(drvdata, ETMCNTVRn(i)); > - ret += sprintf(buf, "counter %d: %x\n", i, val); > + return sprintf(buf, "%x\n", val); > } > > - return ret; > + val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx)); > + return sprintf(buf, "%x\n", val); > } > > static ssize_t cntr_val_store(struct device *dev, > > > Given the upcoming merge window, I plan to submit this v2 after -rc1 > is released. > > Alternatively, if the consensus is to drop the driver, I am happy to > submit a patch for that instead. > > Regards, > Kuan-Wei > > The change looks good. I wouldn't wait for rc1, nobody else is touching these files so nothing will change, so makes sense to send it now. >>> single file + selector paradigm makes things easier. The index is >>> validated against the actual number of counters for this instance, and >>> read/write occurs for the selected one. >>> >>> Regards >>> >>> Mike >>> >>>>>> >>>>>> The interface isn't even that great, it should be a separate file per >>>>>> counter. You don't want to be parsing strings and colons to try to read a >>>>>> single value, especially in C. Separate files allows you to read it directly >>>>>> without any hassle. >>>>>> >>>>>>> >>>>>>> drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 4 ++-- >>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c >>>>>>> index 762109307b86..312033e74b7a 100644 >>>>>>> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c >>>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c >>>>>>> @@ -725,7 +725,7 @@ static ssize_t cntr_val_show(struct device *dev, >>>>>>> if (!coresight_get_mode(drvdata->csdev)) { >>>>>>> spin_lock(&drvdata->spinlock); >>>>>>> for (i = 0; i < drvdata->nr_cntr; i++) >>>>>>> - ret += sprintf(buf, "counter %d: %x\n", >>>>>>> + ret += sprintf(buf + ret, "counter %d: %x\n", >>>>>>> i, config->cntr_val[i]); >>>>>>> spin_unlock(&drvdata->spinlock); >>>>>>> return ret; >>>>>>> @@ -733,7 +733,7 @@ static ssize_t cntr_val_show(struct device *dev, >>>>>>> for (i = 0; i < drvdata->nr_cntr; i++) { >>>>>>> val = etm_readl(drvdata, ETMCNTVRn(i)); >>>>>>> - ret += sprintf(buf, "counter %d: %x\n", i, val); >>>>>>> + ret += sprintf(buf + ret, "counter %d: %x\n", i, val); >>>>>>> } >>>>>>> return ret; >>>>>> >>>> >>> >>> >>

2 months, 2 weeks

Re: [PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val_show()

by James Clark

On 21/11/2025 5:02 pm, Kuan-Wei Chiu wrote: > Hi James, > > On Fri, Nov 21, 2025 at 09:50:03AM +0000, James Clark wrote: >> >> >> On 21/11/2025 12:23 am, Kuan-Wei Chiu wrote: >>> The cntr_val_show() function is meant to display the values of all >>> available counters. However, the sprintf() call inside the loop was >>> always writing to the beginning of the buffer, causing the output of >>> previous iterations to be overwritten. As a result, only the value of >>> the last counter was actually returned to the user. >>> >>> Fix this by using the return value of sprintf() to calculate the >>> correct offset into the buffer for the next write, ensuring that all >>> counter values are appended sequentially. >>> >>> Fixes: a939fc5a71ad ("coresight-etm: add CoreSight ETM/PTM driver") >>> Signed-off-by: Kuan-Wei Chiu <visitorckw(a)gmail.com> >>> --- >>> Build tested only. I do not have the hardware to run the etm3x driver, >>> so I would be grateful if someone could verify this on actual hardware. >>> >>> I noticed this issue while browsing the coresight code after attending >>> a technical talk on the subject. This code dates back to the initial >>> driver submission over 10 years ago, so I was surprised it hadn't been >>> caught earlier. Although I cannot perform runtime testing, the logic >>> error seems obvious to me, so I still decided to submit this patch. >> >> Nice find. I think the point that it wasn't caught changes how we fix it. >> Either nobody used it ever - so we can just delete it. Or someone was using >> it and they expect it to always return a single entry with the value of the >> last counter and this is a potentially breaking change. So maybe instead of >> fixing this we should add a new cntr_vals_show() which works correctly. But >> then again if nobody is using it we shouldn't do that either. > > Thanks for your feedback. > > I agree that if any tool relies on the current behavior, this patch > would break the ABI and violate the hard rule that we must never break > userspace. > > However, I am not sure how to determine if any userspace tools are > actually using this sysfs interface. Is there a recommended way to > verify this, or a standard procedure/convention to follow in this > situation? > > Regards, > Kuan-Wei > There's no way of knowing apart from deducing that there are 0 users of the 'correct' version of the API because it never existed. This isn't even a regression, it was broken from the beginning. I suppose it does work when hardware only has one counter, but I don't know how likely that is? Looking at cntr_val_store() I think I was wrong before when I said it should be a separate file for each counter. Writing to it already writes to the counter currently selected by cntr_idx and splitting it out to separate files would have to break that too. So we can fix the display bug by making show operate on the currently selected counter in the same way as store. Then it makes a bit more sense and we can delete the "counter %d: " prefix. ETM4 already does it this way too. >> >> The interface isn't even that great, it should be a separate file per >> counter. You don't want to be parsing strings and colons to try to read a >> single value, especially in C. Separate files allows you to read it directly >> without any hassle. >> >>> >>> drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c >>> index 762109307b86..312033e74b7a 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c >>> @@ -725,7 +725,7 @@ static ssize_t cntr_val_show(struct device *dev, >>> if (!coresight_get_mode(drvdata->csdev)) { >>> spin_lock(&drvdata->spinlock); >>> for (i = 0; i < drvdata->nr_cntr; i++) >>> - ret += sprintf(buf, "counter %d: %x\n", >>> + ret += sprintf(buf + ret, "counter %d: %x\n", >>> i, config->cntr_val[i]); >>> spin_unlock(&drvdata->spinlock); >>> return ret; >>> @@ -733,7 +733,7 @@ static ssize_t cntr_val_show(struct device *dev, >>> for (i = 0; i < drvdata->nr_cntr; i++) { >>> val = etm_readl(drvdata, ETMCNTVRn(i)); >>> - ret += sprintf(buf, "counter %d: %x\n", i, val); >>> + ret += sprintf(buf + ret, "counter %d: %x\n", i, val); >>> } >>> return ret; >>

2 months, 2 weeks

[PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool

by James Clark

Do some cleanups then expand the timestamp format attribute from 1 bit to 4 bits for ETMv4 in Perf mode. The current interval is too high for most use cases, and particularly on the FVP the number of timestamps generated is excessive. This change not only still allows disabling or enabling timestamps, but also allows the interval to be configured. The old bit is kept deprecated and undocumented for now. There are known broken versions of Perf that don't read the format attribute positions from sysfs and instead hard code the timestamp bit. We can leave the old bit in the driver until we need the bit for another feature or enough time has passed that these old Perfs are unlikely to be used. The interval option is added as an event format attribute, rather than a Coresight config because it's something that the driver is already configuring automatically in Perf mode using any unused counter, so it's not possible to modify this with a config. Applies to coresight/next Signed-off-by: James Clark <james.clark(a)linaro.org> --- Changes in v5: - Add parens to interval calculation in docs (Randy) - Swap "minimum interval" and "maximum interval" in docs. (Leo) - Add TRCSYNCPR.PERIOD to docs (Leo) - Use CONFIG_ARM64 to avoid is_kernel_in_hyp_mode() (Leo) - Add a comment for hidden ETMv3 format attributes (Leo) - Hide configid for ETMv3 (Leo) - Link to v4: https://lore.kernel.org/r/20251112-james-cs-syncfreq-v4-0-165ba21401dc@lina… Changes in v4: - Add #defines for true and false resources ETM_RES_SEL_TRUE/FALSE - Reword comment about finding a counter to say if there are no resources there are no counters. - Extend existing timestamp format attribute instead of adding a new one - Refactor all the config definitions and parsing to use GEN_PMU_FORMAT_ATTR()/ATTR_CFG_GET_FLD() so we can see where the unused bits are. - Link to v3: https://lore.kernel.org/r/20251002-james-cs-syncfreq-v3-0-fe5df2bf91d1@lina… Changes in v3: - Move the format attr definitions to coresight-etm-perf.h we can compile on arm32 without #ifdefs - (Leo) - Convert the new #ifdefs to a single one in an is_visible() function so that the code is cleaner - (Leo) - Drop the change to remove the holes in struct etmv4_config as they were grouped by function - (Mike) - Link to v2: https://lore.kernel.org/r/20250814-james-cs-syncfreq-v2-0-c76fcb87696d@lina… Changes in v2: - Only show the attribute for ETMv4 to improve usability and fix the arm32 build error. Wrapping everything in IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) isn't ideal, but the -perf.c file is shared between ETMv3 and ETMv4, and there is already precedent for doing it this way. - Link to v1: https://lore.kernel.org/r/20250811-james-cs-syncfreq-v1-0-b001cd6e3404@lina… --- James Clark (13): coresight: Change syncfreq to be a u8 coresight: Repack struct etmv4_drvdata coresight: Refactor etm4_config_timestamp_event() coresight: Hide unused ETMv3 format attributes coresight: Define format attributes with GEN_PMU_FORMAT_ATTR() coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD() coresight: Don't reject unrecognized ETMv3 format attributes coresight: Interpret perf config with ATTR_CFG_GET_FLD() coresight: Interpret ETMv4 config with ATTR_CFG_GET_FLD() coresight: Remove misleading definitions coresight: Extend width of timestamp format attribute coresight: Allow setting the timestamp interval coresight: docs: Document etm4x timestamp interval option Documentation/trace/coresight/coresight.rst | 16 +- drivers/hwtracing/coresight/coresight-etm-perf.c | 70 ++++++--- drivers/hwtracing/coresight/coresight-etm-perf.h | 39 +++++ drivers/hwtracing/coresight/coresight-etm3x-core.c | 36 ++--- drivers/hwtracing/coresight/coresight-etm4x-core.c | 164 +++++++++++++-------- drivers/hwtracing/coresight/coresight-etm4x.h | 61 +++++--- include/linux/coresight-pmu.h | 24 --- 7 files changed, 258 insertions(+), 152 deletions(-) --- base-commit: 9e9182cab5ebc3ee7544e60ef08ba19fdf216920 change-id: 20250724-james-cs-syncfreq-7c2257a38ed3 Best regards, -- James Clark <james.clark(a)linaro.org>

2 months, 2 weeks

← Newer
1
...
6
7
8
9
10
11
12
...
263
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight