CoreSight

coresight@lists.linaro.org

4 participants
2607 discussions

Re: [PATCH v3 1/2] dt-bindings: arm: add CTCU device for QCS8300

by Suzuki K Poulose

On 13/10/2025 09:34, Jie Gan wrote: > The CTCU device for QCS8300 shares the same configurations as SA8775p. Add > a fallback to enable the CTCU for QCS8300 to utilize the compitable of the > SA8775p. > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > --- > Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml > index c969c16c21ef..460f38ddbd73 100644 > --- a/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml > @@ -26,8 +26,13 @@ description: | > > properties: > compatible: > - enum: > - - qcom,sa8775p-ctcu > + oneOf: > + - items: > + - enum: > + - qcom,qcs8300-ctcu > + - const: qcom,sa8775p-ctcu > + - enum: > + - qcom,sa8775p-ctcu > > reg: > maxItems: 1 > Acked-by: Suzuki K Poulose <suzuki.poulose(a)arm.com>

2 months, 3 weeks

Re: [PATCH v4 05/11] coresight: etm4x: Ensure context synchronization is not ignored

by Suzuki K Poulose

On 27/10/2025 10:43, Yeoreum Yun wrote: > Hi Suzuki, >> On 24/10/2025 17:45, Leo Yan wrote: >>> As recommended in section 4.3.7 "Synchronization of register updates" of >>> ARM IHI0064H.b, a self-hosted trace analyzer should always executes an >>> ISB instruction after programming the trace unit registers. >>> >>> An ISB works as a context synchronization event; a DSB is not required. >>> Removes the redundant barrier in the enabling flow. >> >> It is required for MMIO based instances and must be retained. > > I think it seems fine. since the the etm4x device mmio is mapped as > Device-nGnRE and according to the section Leo mention: > > Synchronization when using the memory-mapped interface > .. > When disabling or enabling the trace unit, the trace analyzer must poll TRCSTATR to check the trace unit is either > idle or not idle, as described in Use of the trace unit main enable bit on page 4-169: > • When the memory is marked as Device-nGnRE or stronger. > — Write to enable or disable the trace unit. > — Poll TRCSTATR to ensure the previous write has completed. > — Execute an ISB operation. > > Therefore, we can omit the dsb in here. You are right, to be precise, as long as we quote the following, I am fine. Section 4.3.7, Synchronization when using the memory-mapped interface - When using the memory-mapped interface to program the trace unit, the trace analyzer must ensure that writes have completed, to ensure that the trace unit is fully programmed and either enabled or disabled. • If the memory marked is as Device-nGnRE or stronger, read back the value of any register in the trace unit. This relies on peripheral coherence order defined in the Arm architecture. Suzuki > > [...] > > Thanks. > > -- > Sincerely, > Yeoreum Yun

2 months, 3 weeks

Re: [PATCH v3 0/3] add sysfs nodes to configure TPDA's registers

by Suzuki K Poulose

On 27/10/2025 01:14, Jie Gan wrote: > > > On 8/27/2025 6:55 PM, Jie Gan wrote: >> Patchset 1 introduces configuration of the cross-trigger registers with >> appropriate values to enable proper generation of cross-trigger packets. >> >> Patchset 2 introduces a logic to configure the TPDA_SYNCR register, >> which determines the frequency of ASYNC packet generation. These packets >> assist userspace tools in accurately identifying each valid packet. >> >> Patchset 3 introduces a sysfs node to initiate a flush request for the >> specific port, forcing the data to synchronize and be transmitted to the >> sink device. >> > > Gentle reminder. Please could you fix the dates and version to v6.19 ? Suzuki > > Thanks, > Jie > >> Changes in V3: >> 1. Optimizing codes according to James's comment. >> Link to V2 - https://lore.kernel.org/all/20250827042042.6786-1- >> jie.gan(a)oss.qualcomm.com/ >> >> Changes in V2: >> 1. Refactoring the code based on James's comment for optimization. >> Link to V1 - https://lore.kernel.org/all/20250826070150.5603-1- >> jie.gan(a)oss.qualcomm.com/ >> >> Tao Zhang (3): >> coresight: tpda: add sysfs nodes for tpda cross-trigger configuration >> coresight: tpda: add logic to configure TPDA_SYNCR register >> coresight: tpda: add sysfs node to flush specific port >> >> .../testing/sysfs-bus-coresight-devices-tpda | 50 ++++ >> drivers/hwtracing/coresight/coresight-tpda.c | 278 ++++++++++++++++++ >> drivers/hwtracing/coresight/coresight-tpda.h | 33 ++- >> 3 files changed, 360 insertions(+), 1 deletion(-) >> create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight- >> devices-tpda >> >

2 months, 3 weeks

Re: [PATCH v3 2/3] coresight: tpdm: add static tpdm support

by Suzuki K Poulose

On 13/10/2025 07:11, Jie Gan wrote: > The static TPDM function as a dummy source, however, it is essential > to enable the port connected to the TPDA and configure the element size. > Without this, the TPDA cannot correctly receive trace data from the > static TPDM. Since the static TPDM does not require MMIO mapping to > access its registers, a clock controller is not mandatory for its > operation. > > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > --- > drivers/hwtracing/coresight/coresight-tpda.c | 9 ++ > drivers/hwtracing/coresight/coresight-tpdm.c | 148 +++++++++++++++++++++------ > drivers/hwtracing/coresight/coresight-tpdm.h | 8 ++ > 3 files changed, 131 insertions(+), 34 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c > index 333b3cb23685..4e93fa5bace4 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.c > +++ b/drivers/hwtracing/coresight/coresight-tpda.c > @@ -68,6 +68,15 @@ static int tpdm_read_element_size(struct tpda_drvdata *drvdata, > int rc = -EINVAL; > struct tpdm_drvdata *tpdm_data = dev_get_drvdata(csdev->dev.parent); > > + if (coresight_is_static_tpdm(csdev)) { > + rc = fwnode_property_read_u32(dev_fwnode(csdev->dev.parent), > + "qcom,dsb-element-bits", &drvdata->dsb_esize); > + rc &= fwnode_property_read_u32(dev_fwnode(csdev->dev.parent), > + "qcom,cmb-element-bits", &drvdata->cmb_esize); > + This doesn't match the "dynamic" tpdm case ? We mandate that static TPDMs have DSB and CMB. I would rather set the appropriate flags in tpdm_drvdata->dsb/cmb in the TPDM driver for static tpdms and not let the "static" vs "dynamic" creep into the TPDA and other users. e.g., in TPDM driver: if (static_tpdm()) { tpdm_data->dsb = has_dsb_element_bits_property; tpdm_data->cmb = has_cmb_element_bits_property; } > + goto out; > + } > + > if (tpdm_data->dsb) { > rc = fwnode_property_read_u32(dev_fwnode(csdev->dev.parent), > "qcom,dsb-element-bits", &drvdata->dsb_esize); > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 7214e65097ec..1766b0182819 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -495,7 +495,9 @@ static int tpdm_enable(struct coresight_device *csdev, struct perf_event *event, > return -EBUSY; > } > > - __tpdm_enable(drvdata); > + if (!coresight_is_static_tpdm(csdev)) > + __tpdm_enable(drvdata); > + > drvdata->enable = true; > spin_unlock(&drvdata->spinlock); > > @@ -551,7 +553,9 @@ static void tpdm_disable(struct coresight_device *csdev, > return; > } > > - __tpdm_disable(drvdata); > + if (!coresight_is_static_tpdm(csdev)) > + __tpdm_disable(drvdata); minor nit: It is much safer to do this check in __tpdm_xxable() and return early. > + > coresight_set_mode(csdev, CS_MODE_DISABLED); > drvdata->enable = false; > spin_unlock(&drvdata->spinlock); > @@ -1342,10 +1346,9 @@ static const struct attribute_group *tpdm_attr_grps[] = { > NULL, > }; > > -static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > +static int tpdm_probe(struct device *dev, struct resource *res) > { > void __iomem *base; > - struct device *dev = &adev->dev; > struct coresight_platform_data *pdata; > struct tpdm_drvdata *drvdata; > struct coresight_desc desc = { 0 }; > @@ -1354,32 +1357,33 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > pdata = coresight_get_platform_data(dev); > if (IS_ERR(pdata)) > return PTR_ERR(pdata); > - adev->dev.platform_data = pdata; > + dev->platform_data = pdata; > > /* driver data*/ > drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > if (!drvdata) > return -ENOMEM; > - drvdata->dev = &adev->dev; > + drvdata->dev = dev; > dev_set_drvdata(dev, drvdata); > > - base = devm_ioremap_resource(dev, &adev->res); > - if (IS_ERR(base)) > - return PTR_ERR(base); > + if (res) { > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > > - drvdata->base = base; > + drvdata->base = base; > + ret = tpdm_datasets_setup(drvdata); > + if (ret) > + return ret; > > - ret = tpdm_datasets_setup(drvdata); > - if (ret) > - return ret; > + if (drvdata && tpdm_has_dsb_dataset(drvdata)) > + of_property_read_u32(drvdata->dev->of_node, > + "qcom,dsb-msrs-num", &drvdata->dsb_msr_num); > > - if (drvdata && tpdm_has_dsb_dataset(drvdata)) > - of_property_read_u32(drvdata->dev->of_node, > - "qcom,dsb-msrs-num", &drvdata->dsb_msr_num); > - > - if (drvdata && tpdm_has_cmb_dataset(drvdata)) > - of_property_read_u32(drvdata->dev->of_node, > - "qcom,cmb-msrs-num", &drvdata->cmb_msr_num); > + if (drvdata && tpdm_has_cmb_dataset(drvdata)) > + of_property_read_u32(drvdata->dev->of_node, > + "qcom,cmb-msrs-num", &drvdata->cmb_msr_num); > + } > > /* Set up coresight component description */ > desc.name = coresight_alloc_device_name(&tpdm_devs, dev); > @@ -1388,34 +1392,51 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > desc.type = CORESIGHT_DEV_TYPE_SOURCE; > desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM; > desc.ops = &tpdm_cs_ops; > - desc.pdata = adev->dev.platform_data; > - desc.dev = &adev->dev; > + desc.pdata = dev->platform_data; > + desc.dev = dev; > desc.access = CSDEV_ACCESS_IOMEM(base); > - desc.groups = tpdm_attr_grps; > + if (res) > + desc.groups = tpdm_attr_grps; > drvdata->csdev = coresight_register(&desc); > if (IS_ERR(drvdata->csdev)) > return PTR_ERR(drvdata->csdev); > > spin_lock_init(&drvdata->spinlock); > > - /* Decrease pm refcount when probe is done.*/ > - pm_runtime_put(&adev->dev); > - > return 0; > } > > -static void tpdm_remove(struct amba_device *adev) > +static int tpdm_remove(struct device *dev) > { > - struct tpdm_drvdata *drvdata = dev_get_drvdata(&adev->dev); > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev); > > coresight_unregister(drvdata->csdev); > + > + return 0; > +} > + > +static int dynamic_tpdm_probe(struct amba_device *adev, > + const struct amba_id *id) > +{ > + int ret; > + > + ret = tpdm_probe(&adev->dev, &adev->res); > + if (!ret) > + pm_runtime_put(&adev->dev); > + > + return ret; > +} > + > +static void dynamic_tpdm_remove(struct amba_device *adev) > +{ > + tpdm_remove(&adev->dev); > } > > /* > * Different TPDM has different periph id. > * The difference is 0-7 bits' value. So ignore 0-7 bits. > */ > -static const struct amba_id tpdm_ids[] = { > +static const struct amba_id dynamic_tpdm_ids[] = { > { > .id = 0x001f0e00, > .mask = 0x00ffff00, > @@ -1423,17 +1444,76 @@ static const struct amba_id tpdm_ids[] = { > { 0, 0, NULL }, > }; > > -static struct amba_driver tpdm_driver = { > +MODULE_DEVICE_TABLE(amba, dynamic_tpdm_ids); > + > +static struct amba_driver dynamic_tpdm_driver = { > .drv = { > .name = "coresight-tpdm", > .suppress_bind_attrs = true, > }, > - .probe = tpdm_probe, > - .id_table = tpdm_ids, > - .remove = tpdm_remove, > + .probe = dynamic_tpdm_probe, > + .id_table = dynamic_tpdm_ids, > + .remove = dynamic_tpdm_remove, > }; > > -module_amba_driver(tpdm_driver); > +static int tpdm_platform_probe(struct platform_device *pdev) > +{ > + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + int ret; > + > + pm_runtime_get_noresume(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + > + ret = tpdm_probe(&pdev->dev, res); > + pm_runtime_put(&pdev->dev); > + if (ret) > + pm_runtime_disable(&pdev->dev); > + > + return ret; > +} > + > +static void tpdm_platform_remove(struct platform_device *pdev) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(&pdev->dev); > + > + if (WARN_ON(!drvdata)) > + return; > + > + tpdm_remove(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > +} > + > +static const struct of_device_id static_tpdm_match[] = { > + {.compatible = "qcom,coresight-static-tpdm"}, > + {} > +}; > + > +MODULE_DEVICE_TABLE(of, static_tpdm_match); > + > +static struct platform_driver static_tpdm_driver = { > + .probe = tpdm_platform_probe, > + .remove = tpdm_platform_remove, > + .driver = { > + .name = "coresight-static-tpdm", > + .of_match_table = static_tpdm_match, > + .suppress_bind_attrs = true, > + }, > +}; > + > +static int __init tpdm_init(void) > +{ > + return coresight_init_driver("tpdm", &dynamic_tpdm_driver, &static_tpdm_driver, > + THIS_MODULE); > +} > + > +static void __exit tpdm_exit(void) > +{ > + coresight_remove_driver(&dynamic_tpdm_driver, &static_tpdm_driver); > +} > + > +module_init(tpdm_init); > +module_exit(tpdm_exit); > > MODULE_LICENSE("GPL"); > MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Monitor driver"); > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h > index b11754389734..9f52c88ce5c1 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.h > +++ b/drivers/hwtracing/coresight/coresight-tpdm.h > @@ -343,4 +343,12 @@ struct tpdm_dataset_attribute { > enum dataset_mem mem; > u32 idx; > }; > + > +static inline bool coresight_is_static_tpdm(struct coresight_device *csdev) > +{ > + struct device_node *node = csdev->dev.parent->of_node; > + > + return (csdev && > + of_device_is_compatible(node, "qcom,coresight-static-tpdm")); Why do we have to go check the firmware table all the time ? Could we not cache this in drvdata ? Or even better, we can use the csdev->access by using : coresight_device_is_tpdm() return (coresight_device_is_tpdm(csdev) && !csdev->access->base); Suzuki

2 months, 3 weeks

[PATCH 0/1] coresight: fix issue with insufficient claim tags

by Mike Leach

All CoreSight compliant components have an implementation defined number of 0 to 8 claim tag bits in the claim tag registers. These are used to claim the CoreSight resources by system agents. ARM recommends implementions have 4 claim tag registers. The CoreSight drivers implement a 2 claim tag bit protocol to allow self hosted and external debug agents to manage access to the hardware. However, if there are less than 2 claim tags available the protocol incorrectly returns an error on device claim, as no checks are made. If insufficient claim tags are present in a component then the protocol must return success on claim / disclaim to allow components to be used normally. Changes read the CLAIMSET bits to establish the number of available tags, and adjust the claim returns accordingly. Applies to coresignt/next Mike Leach (1): coresight: fix issue where coresight component has no claimtags drivers/hwtracing/coresight/coresight-core.c | 26 ++++++++++++++++++++ drivers/hwtracing/coresight/coresight-priv.h | 8 ++++++ 2 files changed, 34 insertions(+) -- 2.32.0

2 months, 3 weeks

[PATCH] perf cs-etm: Mute enumeration value warning

by Leo Yan

When the OpenCSD library introduces a new enumeration value (for example, in the v1.7.1 release), the perf build fails with an error: util/cs-etm-decoder/cs-etm-decoder.c:600:10: error: enumeration value 'OCSD_GEN_TRC_ELEM_ITMTRACE' not explicitly handled in switch [-Werror, -Wswitch-enum] 600 | switch (elem->elem_type) { | ^~~~~~~~~~~~~~~ 1 error generated. Convert to if-else sentences to mute the enumeration value warning, which can avoid build failures whenever the lib is updated. No functional change. Suggested-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> Signed-off-by: Leo Yan <leo.yan(a)arm.com> --- .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 44 +++++-------------- 1 file changed, 11 insertions(+), 33 deletions(-) diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index b85a8837bddc..3050fe212666 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -588,6 +588,7 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer( const ocsd_generic_trace_elem *elem) { ocsd_datapath_resp_t resp = OCSD_RESP_CONT; + ocsd_gen_trc_elem_t type; struct cs_etm_decoder *decoder = (struct cs_etm_decoder *) context; struct cs_etm_queue *etmq = decoder->data; struct cs_etm_packet_queue *packet_queue; @@ -597,52 +598,29 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer( if (!packet_queue) return OCSD_RESP_FATAL_SYS_ERR; - switch (elem->elem_type) { - case OCSD_GEN_TRC_ELEM_UNKNOWN: - break; - case OCSD_GEN_TRC_ELEM_EO_TRACE: - case OCSD_GEN_TRC_ELEM_NO_SYNC: - case OCSD_GEN_TRC_ELEM_TRACE_ON: + type = elem->elem_type; + + if (type == OCSD_GEN_TRC_ELEM_EO_TRACE || + type == OCSD_GEN_TRC_ELEM_NO_SYNC || + type == OCSD_GEN_TRC_ELEM_TRACE_ON) resp = cs_etm_decoder__buffer_discontinuity(etmq, packet_queue, trace_chan_id); - break; - case OCSD_GEN_TRC_ELEM_INSTR_RANGE: + else if (type == OCSD_GEN_TRC_ELEM_INSTR_RANGE) resp = cs_etm_decoder__buffer_range(etmq, packet_queue, elem, trace_chan_id); - break; - case OCSD_GEN_TRC_ELEM_EXCEPTION: + else if (type == OCSD_GEN_TRC_ELEM_EXCEPTION) resp = cs_etm_decoder__buffer_exception(etmq, packet_queue, elem, trace_chan_id); - break; - case OCSD_GEN_TRC_ELEM_EXCEPTION_RET: + else if (type == OCSD_GEN_TRC_ELEM_EXCEPTION_RET) resp = cs_etm_decoder__buffer_exception_ret(etmq, packet_queue, trace_chan_id); - break; - case OCSD_GEN_TRC_ELEM_TIMESTAMP: + else if (type == OCSD_GEN_TRC_ELEM_TIMESTAMP) resp = cs_etm_decoder__do_hard_timestamp(etmq, elem, trace_chan_id, indx); - break; - case OCSD_GEN_TRC_ELEM_PE_CONTEXT: + else if (type == OCSD_GEN_TRC_ELEM_PE_CONTEXT) resp = cs_etm_decoder__set_tid(etmq, packet_queue, elem, trace_chan_id); - break; - /* Unused packet types */ - case OCSD_GEN_TRC_ELEM_I_RANGE_NOPATH: - case OCSD_GEN_TRC_ELEM_ADDR_NACC: - case OCSD_GEN_TRC_ELEM_CYCLE_COUNT: - case OCSD_GEN_TRC_ELEM_ADDR_UNKNOWN: - case OCSD_GEN_TRC_ELEM_EVENT: - case OCSD_GEN_TRC_ELEM_SWTRACE: - case OCSD_GEN_TRC_ELEM_CUSTOM: - case OCSD_GEN_TRC_ELEM_SYNC_MARKER: - case OCSD_GEN_TRC_ELEM_MEMTRANS: -#if (OCSD_VER_NUM >= 0x010400) - case OCSD_GEN_TRC_ELEM_INSTRUMENTATION: -#endif - default: - break; - } return resp; } -- 2.34.1

2 months, 3 weeks

[PATCH v3 0/5] coresight: Add format attribute for setting the timestamp interval

by James Clark

Do some cleanups then add a new format attribute to set the timestamp interval 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. Although it would be good to make only SYNC timestamps the default and have counter timestamps opt-in, this would be a breaking change. We can always do that later, or disable counter timestamps from Perf. This 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 with 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 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 (5): coresight: Change syncfreq to be a u8 coresight: Repack struct etmv4_drvdata coresight: Refactor etm4_config_timestamp_event() coresight: Add format attribute for setting the timestamp interval coresight: docs: Document etm4x ts_interval Documentation/trace/coresight/coresight.rst | 14 +++ drivers/hwtracing/coresight/coresight-etm-perf.c | 16 ++- drivers/hwtracing/coresight/coresight-etm-perf.h | 7 ++ drivers/hwtracing/coresight/coresight-etm4x-core.c | 110 +++++++++++++-------- drivers/hwtracing/coresight/coresight-etm4x.h | 61 +++++++----- 5 files changed, 143 insertions(+), 65 deletions(-) --- base-commit: 01f96b812526a2c8dcd5c0e510dda37e09ec8bcd change-id: 20250724-james-cs-syncfreq-7c2257a38ed3 Best regards, -- James Clark <james.clark(a)linaro.org>

2 months, 3 weeks

Re: [PATCH] coresight: ETR: Fix ETR buffer use-after-free issue

by Leo Yan

On Tue, Oct 21, 2025 at 09:56:43AM +0800, Jie Gan wrote: [...] > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > > index b07fcdb3fe1a..d0fac958c614 100644 > > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > > @@ -1241,6 +1241,8 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) > > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > > struct etr_buf *sysfs_buf = NULL, *new_buf = NULL, *free_buf = NULL; > > + WARN_ON(coresight_get_mode(csdev) != CS_MODE_SYSFS); > > I think we should check the WARN_ON result and exit if there is an error? When run at here, it should be in Sysfs mode. Here the check is for debugging purpose in case any mismatch. [...] > > +static void tmc_release_mode(struct coresight_device *csdev, enum cs_mode mode) > > +{ > > + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > > + > > + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock); > > + > > + if (WARN_ON(coresight_get_mode(csdev) != mode)) > > + return; > > the mode here could be set to any CS_MODE, so I think it's possible to > encounter the secenario below: > > coresight_get_mode(csdev) == CS_MODE_DISABLED, mode == CS_MODE_DISABLED, > > With the condition, the csdev->refcnt will go to negative number? The parameter "mode" might cause complexity, will drop it. The correctness will be ensured by the callers. Thanks for review! Leo

2 months, 4 weeks

Re: [PATCH] coresight: ETR: Fix ETR buffer use-after-free issue

by Leo Yan

On Mon, Oct 20, 2025 at 05:06:46PM +0800, Xiaoqi Zhuang wrote: > When ETR is enabled as CS_MODE_SYSFS, if the buffer size is changed > and enabled again, currently sysfs_buf will point to the newly > allocated memory(buf_new) and free the old memory(buf_old). But the > etr_buf that is being used by the ETR remains pointed to buf_old, not > updated to buf_new. In this case, it will result in a memory > use-after-free issue. I struggled to understand how to reproduce the issue under the condition "if the buffer size is changed and enabled again." I don't think the flow below where the trace is re-enabled would cause an issue: - Step 1: Enable trace path between ETM0 -> ETR0; - Step 2: Change the buffer size for ETR0; - Step 3: Disable trace path between ETM0 -> ETR0; - Step 4: Enable again trace path between ETM0 -> ETR0. In this case, step3 releases the buffer and update "drvdata->etr_buf" to NULL, and step 4 allocates a new buffer and assign it to "drvdata->etr_buf". The problem should occur when operating on two trace paths, E.g., - Step 1: Enable trace path between ETM0 -> ETR0; - Step 2: Change the buffer size for ETR0; - Step 3: Enable trace path between ETM1 -> ETR0; In step3, the driver releases the existed buffer and allocate a new one. At the meantime, "drvdata->etr_buf" still holds the buffer allocated in step 1. > Fix this by checking ETR's mode before updating and releasing buf_old, > if the mode is CS_MODE_SYSFS, then skip updating and releasing it. Given that we now have a couple of reported issues related to ETR mode, I'd like to refactor the ETR mode handling and its reference counting thoroughly. I've drafted a large change (it's quite big, but we can split it into small patches if we agree to proceed). Thanks for reporting the issue! Leo ---8<--- diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index b07fcdb3fe1a..d0fac958c614 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1241,6 +1241,8 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct etr_buf *sysfs_buf = NULL, *new_buf = NULL, *free_buf = NULL; + WARN_ON(coresight_get_mode(csdev) != CS_MODE_SYSFS); + /* * If we are enabling the ETR from disabled state, we need to make * sure we have a buffer with the right size. The etr_buf is not reset @@ -1263,7 +1265,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; } @@ -1292,30 +1294,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) int ret = 0; unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); - struct etr_buf *sysfs_buf = tmc_etr_get_sysfs_buffer(csdev); + struct etr_buf *sysfs_buf; + sysfs_buf = tmc_etr_get_sysfs_buffer(csdev); if (IS_ERR(sysfs_buf)) return PTR_ERR(sysfs_buf); 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. - */ - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { - csdev->refcnt++; - goto out; - } - ret = tmc_etr_enable_hw(drvdata, sysfs_buf); - if (!ret) { - coresight_set_mode(csdev, CS_MODE_SYSFS); - csdev->refcnt++; - } - -out: raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); if (!ret) @@ -1735,11 +1721,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *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; @@ -1759,18 +1740,14 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) * No HW configuration is needed if the sink is already in * use for this session. */ - if (drvdata->pid == pid) { - csdev->refcnt++; + if (drvdata->pid == pid) goto unlock_out; - } rc = tmc_etr_enable_hw(drvdata, etr_perf->etr_buf); if (!rc) { /* Associate with monitored process. */ drvdata->pid = pid; - coresight_set_mode(csdev, CS_MODE_PERF); drvdata->perf_buf = etr_perf->etr_buf; - csdev->refcnt++; } unlock_out: @@ -1778,17 +1755,76 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) return rc; } +static int tmc_acquire_mode(struct coresight_device *csdev, enum cs_mode mode) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + + if (mode != CS_MODE_SYSFS && mode != CS_MODE_PERF) + return -EINVAL; + + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock); + + if (coresight_get_mode(csdev) == CS_MODE_DISABLED) { + if (!csdev->refcnt) + coresight_set_mode(csdev, mode); + csdev->refcnt++; + } else if (coresight_get_mode(csdev) != mode) { + ret = -EBUSY; + } + + return csdev->refcnt; +} + +static void tmc_release_mode(struct coresight_device *csdev, enum cs_mode mode) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock); + + if (WARN_ON(coresight_get_mode(csdev) != mode)) + return; + + csdev->refcnt--; + if (!csdev->refcnt) + coresight_set_mode(csdev, CS_MODE_DISABLED); +} + static int tmc_enable_etr_sink(struct coresight_device *csdev, enum cs_mode mode, void *data) { + unsigned long flags; + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + int ret; + + ret = tmc_acquire_mode(csdev, mode); + if (ret < 0) + return ret; + + /* + * For sysfs mode, the higher level mutex ensures exclusively + * enabling sink, it is safe to bail out if this is not the + * first time to enable sink. + * + * A perf session can enable the same sink simultaneously, fall + * through to call tmc_enable_etr_sink_perf() to ensure the sink + * has been enabled. + */ + if (mode == CS_MODE_SYSFS && ret > 1) + return 0; + switch (mode) { case CS_MODE_SYSFS: - return tmc_enable_etr_sink_sysfs(csdev); + ret = tmc_enable_etr_sink_sysfs(csdev); case CS_MODE_PERF: - return tmc_enable_etr_sink_perf(csdev, data); + ret = tmc_enable_etr_sink_perf(csdev, data); default: - return -EINVAL; + ret = -EINVAL; } + + if (ret) + tmc_release_mode(csdev, mode); + + return ret; } static int tmc_disable_etr_sink(struct coresight_device *csdev) @@ -1796,30 +1832,20 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); - raw_spin_lock_irqsave(&drvdata->spinlock, flags); + tmc_release_mode(csdev, mode); - if (drvdata->reading) { - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); - return -EBUSY; - } + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock); - csdev->refcnt--; - if (csdev->refcnt) { - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); + if (csdev->refcnt || drvdata->reading) return -EBUSY; - } - /* Complain if we (somehow) got out of sync */ - WARN_ON_ONCE(coresight_get_mode(csdev) == CS_MODE_DISABLED); + if (drvdata->pid == -1) + return 0; + tmc_etr_disable_hw(drvdata); - /* Dissociate from monitored process. */ - drvdata->pid = -1; - coresight_set_mode(csdev, CS_MODE_DISABLED); /* Reset perf specific data */ drvdata->perf_buf = NULL; - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); - dev_dbg(&csdev->dev, "TMC-ETR disabled\n"); return 0; }

2 months, 4 weeks

Re: [PATCH] coresight: ETR: Fix ETR buffer use-after-free issue

by Suzuki K Poulose

On 20/10/2025 11:18, Jie Gan wrote: > > > On 10/20/2025 5:06 PM, Xiaoqi Zhuang wrote: >> When ETR is enabled as CS_MODE_SYSFS, if the buffer size is changed >> and enabled again, currently sysfs_buf will point to the newly >> allocated memory(buf_new) and free the old memory(buf_old). But the >> etr_buf that is being used by the ETR remains pointed to buf_old, not >> updated to buf_new. In this case, it will result in a memory >> use-after-free issue. >> >> Fix this by checking ETR's mode before updating and releasing buf_old, >> if the mode is CS_MODE_SYSFS, then skip updating and releasing it. >> > > I think we need a fix tag for the fix patch: > Fixes: de5461970b3e ("coresight: tmc: allocating memory when needed") > > minor comment: > Since ETR is enabled, we can't switch the sysfs buffer, can we exit > earlier by checking the CS_MODE to avoid allocating memory unnecessarily? +1 Something like this: diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index b07fcdb3fe1a..a9ddb05c10d9 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1252,6 +1252,12 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) raw_spin_lock_irqsave(&drvdata->spinlock, flags); sysfs_buf = READ_ONCE(drvdata->sysfs_buf); if (!sysfs_buf || (sysfs_buf->size != drvdata->size)) { + /* + * If the ETR is already enabled, continue with the existing + * buffer. + */ + if (coresight_get_mode(csdev) == CS_MODE_SYSFS) + goto out; raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); /* Allocate memory with the locks released */ Suzuki> > Besides, > Looks good to me. > > Thanks, > Jie > >> Signed-off-by: Xiaoqi Zhuang <xiaoqi.zhuang(a)oss.qualcomm.com> >> --- >> drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ >> drivers/hwtracing/coresight/coresight-tmc-etr.c >> index b07fcdb3fe1a..3e73cf2c38a3 100644 >> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> @@ -1268,6 +1268,13 @@ static struct etr_buf >> *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) >> goto out; >> } >> + /* >> + * Since this sink is already enabled, the existing buffer should >> not >> + * be released even if the buffer size has changed. >> + */ >> + if (coresight_get_mode(csdev) == CS_MODE_SYSFS) >> + goto out; >> + >> /* >> * If we don't have a buffer or it doesn't match the requested >> size, >> * use the buffer allocated above. Otherwise reuse the existing >> buffer. >> >> --- >> base-commit: 98ac9cc4b4452ed7e714eddc8c90ac4ae5da1a09 >> change-id: 20251020-fix_etr_issue-02c706dbc899 >> >> Best regards, >

2 months, 4 weeks

← Newer
1
...
8
9
10
11
12
13
14
...
261
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight