* [PATCH v5 01/14] dt-bindings: Add RISC-V trace component bindings
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:33 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 02/14] hwtracing: gtrace: Initial implementation of gtrace framework Mayuresh Chitale
` (12 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Anup Patel, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel,
Mayuresh Chitale
From: Anup Patel <anup.patel@oss.qualcomm.com>
Add device tree bindings for the memory mapped RISC-V trace components
which support both the RISC-V efficient trace (E-trace) protocol and
the RISC-V Nexus-based trace (N-trace) protocol.
The RISC-V trace components are defined by the RISC-V trace control
interface specification.
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
.../bindings/riscv/riscv,trace-component.yaml | 120 ++++++++++++++++++
1 file changed, 120 insertions(+)
create mode 100644 Documentation/devicetree/bindings/riscv/riscv,trace-component.yaml
diff --git a/Documentation/devicetree/bindings/riscv/riscv,trace-component.yaml b/Documentation/devicetree/bindings/riscv/riscv,trace-component.yaml
new file mode 100644
index 000000000000..bb519bc4a163
--- /dev/null
+++ b/Documentation/devicetree/bindings/riscv/riscv,trace-component.yaml
@@ -0,0 +1,120 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/riscv/riscv,trace-component.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V Trace Component
+
+maintainers:
+ - Anup Patel <anup@brainfault.org>
+
+description:
+ The RISC-V trace control interface specification standard memory mapped
+ components (or devices) which support both the RISC-V efficient trace
+ (E-trace) protocol and the RISC-V Nexus-based trace (N-trace) protocol.
+ The RISC-V trace components have implementation specific directed acyclic
+ graph style interdependency where output of one component serves as input
+ to another component and certain components (such as funnel) can take inputs
+ from multiple components. The type and version of a RISC-V trace component
+ can be discovered from it's IMPL memory mapped register hence component
+ specific compatible strings are not needed.
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - qemu,trace-component
+ - const: riscv,trace-component
+
+ reg:
+ maxItems: 1
+
+ cpus:
+ maxItems: 1
+ description:
+ phandle to the cpu to which the RISC-V trace component is bound.
+
+ in-ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+ patternProperties:
+ '^port(@[0-7])?$':
+ description: Input connections from RISC-V trace component
+ $ref: /schemas/graph.yaml#/properties/port
+
+ out-ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+ patternProperties:
+ '^port(@[0-7])?$':
+ description: Output connections from RISC-V trace component
+ $ref: /schemas/graph.yaml#/properties/port
+
+required:
+ - compatible
+ - reg
+
+anyOf:
+ - required: [ in-ports ]
+ - required: [ out-ports ]
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ // Example 1 (Per-hart encoder and ramsink components):
+
+ trace@c000000 {
+ compatible = "qemu,trace-component", "riscv,trace-component";
+ reg = <0xc000000 0x1000>;
+ cpus = <&CPU0>;
+
+ out-ports {
+ port {
+ CPU0_ENCODER_OUTPUT: endpoint {
+ remote-endpoint = <&CPU0_RAMSINK_INPUT>;
+ };
+ };
+ };
+ };
+
+ trace@c001000 {
+ compatible = "qemu,trace-component", "riscv,trace-component";
+ reg = <0xc001000 0x1000>;
+ cpus = <&CPU0>;
+
+ in-ports {
+ port {
+ CPU0_RAMSINK_INPUT: endpoint {
+ };
+ };
+ };
+ };
+
+ trace@c002000 {
+ compatible = "qemu,trace-component", "riscv,trace-component";
+ reg = <0xc002000 0x1000>;
+ cpus = <&CPU1>;
+
+ out-ports {
+ port {
+ CPU1_ENCODER_OUTPUT: endpoint {
+ remote-endpoint = <&CPU1_RAMSINK_INPUT>;
+ };
+ };
+ };
+ };
+
+ trace@c003000 {
+ compatible = "qemu,trace-component", "riscv,trace-component";
+ reg = <0xc003000 0x1000>;
+ cpus = <&CPU1>;
+
+ in-ports {
+ port {
+ CPU1_RAMSINK_INPUT: endpoint {
+ };
+ };
+ };
+ };
+
+...
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 01/14] dt-bindings: Add RISC-V trace component bindings
2026-08-10 15:22 ` [PATCH v5 01/14] dt-bindings: Add RISC-V trace component bindings Mayuresh Chitale
@ 2026-08-10 15:33 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:33 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The schema mandates the `qemu,trace-component` compatible string for all instances, directly contradicting the commit message's claim that component-specific strings are not needed.
- [Medium] The generic binding sets `unevaluatedProperties: false` but completely omits standard physical hardware properties such as `clocks`, `resets`, and `power-domains`.
--
--- Patch [1]: [PATCH v5 01/14] dt-bindings: Add RISC-V trace component bindings ---
commit 48aff1e54e6f77b7c90ec16a8b7b4fc01e4b287f
Author: Anup Patel <anup.patel@oss.qualcomm.com>
dt-bindings: Add RISC-V trace component bindings
This commit adds device tree bindings for memory mapped RISC-V trace components
that support both the E-trace and N-trace protocols. The commit message states
that component-specific compatible strings are not needed as the type and
version can be discovered from the IMPL register.
> diff --git a/Documentation/devicetree/bindings/riscv/riscv,trace-component.yaml b/Documentation/devicetree/bindings/riscv/riscv,trace-component.yaml
> new file mode 100644
> index 0000000000000..bb519bc4a163e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/riscv/riscv,trace-component.yaml
> @@ -0,0 +1,120 @@
[ ... ]
> +properties:
> + compatible:
> + items:
> + - enum:
> + - qemu,trace-component
> + - const: riscv,trace-component
[Severity: Medium]
The commit message states that component-specific compatible strings are not
needed, but does this schema enforce qemu,trace-component for all instances?
If a physical hardware device uses just riscv,trace-component, will it fail
schema validation because of the strict two-element array requirement defined
here?
[ ... ]
> +required:
> + - compatible
> + - reg
> +
> +anyOf:
> + - required: [ in-ports ]
> + - required: [ out-ports ]
> +
> +unevaluatedProperties: false
[Severity: Medium]
Since unevaluatedProperties is set to false, how should physical hardware
instances declare standard properties like clocks, resets, or power-domains?
Because they are omitted from the properties block above, won't the schema
validator unconditionally reject device trees that attempt to provide these
standard physical hardware properties?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 02/14] hwtracing: gtrace: Initial implementation of gtrace framework
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
2026-08-10 15:22 ` [PATCH v5 01/14] dt-bindings: Add RISC-V trace component bindings Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:36 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 03/14] gtrace: Add RISC-V platform driver for the " Mayuresh Chitale
` (11 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Anup Patel, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel,
Mayuresh Chitale
From: Anup Patel <anup.patel@oss.qualcomm.com>
Implement a generic and architecture neutral trace framework (gtrace)
in which trace components are organized in a graph-like topology. The
framework provides a bus for the trace components along with helpers to
register components, build the topology and access the hardware registers.
Co-developed-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
drivers/Makefile | 1 +
drivers/hwtracing/Kconfig | 2 +
drivers/hwtracing/gtrace/Kconfig | 15 +
drivers/hwtracing/gtrace/Makefile | 4 +
drivers/hwtracing/gtrace/gtrace-core.c | 442 +++++++++++++++++++++++++
include/linux/gtrace.h | 266 +++++++++++++++
6 files changed, 730 insertions(+)
create mode 100644 drivers/hwtracing/gtrace/Kconfig
create mode 100644 drivers/hwtracing/gtrace/Makefile
create mode 100644 drivers/hwtracing/gtrace/gtrace-core.c
create mode 100644 include/linux/gtrace.h
diff --git a/drivers/Makefile b/drivers/Makefile
index 0841ea851847..e018cc915f62 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -178,6 +178,7 @@ obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/
obj-y += hwtracing/intel_th/
obj-$(CONFIG_STM) += hwtracing/stm/
obj-$(CONFIG_HISI_PTT) += hwtracing/ptt/
+obj-$(CONFIG_GTRACE) += hwtracing/gtrace/
obj-y += android/
obj-$(CONFIG_NVMEM) += nvmem/
obj-$(CONFIG_FPGA) += fpga/
diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig
index 911ee977103c..ef53f5c8429f 100644
--- a/drivers/hwtracing/Kconfig
+++ b/drivers/hwtracing/Kconfig
@@ -7,4 +7,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
source "drivers/hwtracing/ptt/Kconfig"
+source "drivers/hwtracing/gtrace/Kconfig"
+
endmenu
diff --git a/drivers/hwtracing/gtrace/Kconfig b/drivers/hwtracing/gtrace/Kconfig
new file mode 100644
index 000000000000..bcc3b4169a76
--- /dev/null
+++ b/drivers/hwtracing/gtrace/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+menuconfig GTRACE
+ tristate "Generic Hardware Trace Support"
+ depends on OF
+ help
+ This framework provides an architecture-neutral kernel interface
+ for hardware trace drivers. It builds a topological view of the
+ trace components and configures the correct series of components
+ when trace is enabled. It is intended to be shared across RISC-V
+ and ARM trace components so that a single trace path may span
+ components of different architectures.
+
+ To compile this driver as a module, choose M here: the module
+ will be called gtrace.
diff --git a/drivers/hwtracing/gtrace/Makefile b/drivers/hwtracing/gtrace/Makefile
new file mode 100644
index 000000000000..3f90fb99c514
--- /dev/null
+++ b/drivers/hwtracing/gtrace/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_GTRACE) += gtrace.o
+gtrace-y := gtrace-core.o
diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
new file mode 100644
index 000000000000..97398f2b7cbb
--- /dev/null
+++ b/drivers/hwtracing/gtrace/gtrace-core.c
@@ -0,0 +1,442 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ *
+ */
+
+#include <linux/cpumask.h>
+#include <linux/delay.h>
+#include <linux/export.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/percpu.h>
+#include <linux/xarray.h>
+#include <linux/gtrace.h>
+
+/* Mutex to serialize component registration/unregistration */
+static DEFINE_MUTEX(gtrace_mutex);
+
+/* Per-CPU source instances */
+static DEFINE_PER_CPU(struct gtrace_component *, gtrace_cpu_source_comp);
+
+/* Component type based id generator */
+struct gtrace_type_idx {
+ /* Lock to protect the type ID generator */
+ struct mutex lock;
+ struct xarray xa;
+};
+
+/* Array of component type based id generator */
+static struct gtrace_type_idx gtrace_type_idx_array[GTRACE_COMPONENT_TYPE_MAX];
+
+static int gtrace_alloc_type_idx(struct gtrace_component *comp)
+{
+ struct gtrace_type_idx *gidx;
+ u32 idx;
+ int ret;
+
+ if (comp->id.type >= GTRACE_COMPONENT_TYPE_MAX)
+ return -EINVAL;
+
+ gidx = >race_type_idx_array[comp->id.type];
+ mutex_lock(&gidx->lock);
+ ret = xa_alloc(&gidx->xa, &idx, comp, xa_limit_32b, GFP_KERNEL);
+ mutex_unlock(&gidx->lock);
+ if (ret)
+ return ret;
+
+ comp->type_idx = idx;
+ return 0;
+}
+
+static void gtrace_free_type_idx(struct gtrace_component *comp)
+{
+ struct gtrace_type_idx *gidx;
+
+ if (comp->id.type >= GTRACE_COMPONENT_TYPE_MAX)
+ return;
+
+ gidx = >race_type_idx_array[comp->id.type];
+ mutex_lock(&gidx->lock);
+ xa_erase(&gidx->xa, comp->type_idx);
+ mutex_unlock(&gidx->lock);
+}
+
+static void __init gtrace_init_type_idx(void)
+{
+ struct gtrace_type_idx *gidx;
+ int i;
+
+ for (i = 0; i < GTRACE_COMPONENT_TYPE_MAX; i++) {
+ gidx = >race_type_idx_array[i];
+ mutex_init(&gidx->lock);
+ xa_init_flags(&gidx->xa, XA_FLAGS_ALLOC);
+ }
+}
+
+const struct gtrace_component_id *gtrace_match_id(struct gtrace_component *comp,
+ const struct gtrace_component_id *ids)
+{
+ const struct gtrace_component_id *id;
+
+ for (id = ids; id->version; id++) {
+ if (comp->id.type != id->type)
+ continue;
+
+ return id;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(gtrace_match_id);
+
+static int gtrace_match_device(struct device *dev, const struct device_driver *drv)
+{
+ const struct gtrace_driver *gtdrv = to_gtrace_driver(drv);
+ struct gtrace_component *comp = to_gtrace_component(dev);
+
+ return gtrace_match_id(comp, gtdrv->id_table) ? 1 : 0;
+}
+
+static int gtrace_probe(struct device *dev)
+{
+ const struct gtrace_driver *gtdrv = to_gtrace_driver(dev->driver);
+ struct gtrace_component *comp = to_gtrace_component(dev);
+ int ret = 0;
+
+ if (gtdrv->probe)
+ ret = gtdrv->probe(comp);
+
+ if (!ret)
+ comp->ready = true;
+
+ return ret;
+}
+
+static void gtrace_remove(struct device *dev)
+{
+ const struct gtrace_driver *gtdrv = to_gtrace_driver(dev->driver);
+ struct gtrace_component *comp = to_gtrace_component(dev);
+
+ comp->ready = false;
+ if (gtdrv->remove)
+ gtdrv->remove(comp);
+}
+
+static const struct bus_type gtrace_bustype = {
+ .name = "gtrace",
+ .match = gtrace_match_device,
+ .probe = gtrace_probe,
+ .remove = gtrace_remove,
+};
+
+struct gtrace_fwnode_match_data {
+ struct fwnode_handle *fwnode;
+ struct gtrace_component *match;
+};
+
+static int gtrace_match_fwnode(struct device *dev, void *data)
+{
+ struct gtrace_component *comp = to_gtrace_component(dev);
+ struct gtrace_fwnode_match_data *d = data;
+
+ if (device_match_fwnode(&comp->dev, d->fwnode)) {
+ d->match = comp;
+ return 1;
+ }
+
+ return 0;
+}
+
+struct gtrace_component *gtrace_find_by_fwnode(struct fwnode_handle *fwnode)
+{
+ struct gtrace_fwnode_match_data d = { .fwnode = fwnode, .match = NULL };
+ int ret;
+
+ ret = bus_for_each_dev(>race_bustype, NULL, &d, gtrace_match_fwnode);
+ if (ret < 0)
+ return ERR_PTR(ret);
+
+ return d.match;
+}
+EXPORT_SYMBOL_GPL(gtrace_find_by_fwnode);
+
+int gtrace_poll_bit(struct gtrace_platform_data *pdata, int offset,
+ int bit, int bitval, int timeout)
+{
+ u32 val;
+
+ while (timeout--) {
+ val = gtrace_read32(pdata, offset);
+ if (((val >> bit) & 0x1) == bitval)
+ break;
+ udelay(1);
+ }
+
+ return (timeout < 0) ? -ETIMEDOUT : 0;
+}
+EXPORT_SYMBOL_GPL(gtrace_poll_bit);
+
+int gtrace_enable_component(struct gtrace_component *comp)
+{
+ const struct gtrace_hw_ops *ops = comp->pdata->hw_ops;
+
+ if (!ops || !ops->enable)
+ return -EOPNOTSUPP;
+
+ return ops->enable(comp->pdata);
+}
+EXPORT_SYMBOL_GPL(gtrace_enable_component);
+
+int gtrace_disable_component(struct gtrace_component *comp)
+{
+ const struct gtrace_hw_ops *ops = comp->pdata->hw_ops;
+
+ if (!ops || !ops->disable)
+ return -EOPNOTSUPP;
+
+ return ops->disable(comp->pdata);
+}
+EXPORT_SYMBOL_GPL(gtrace_disable_component);
+
+int gtrace_reset_component(struct gtrace_component *comp)
+{
+ const struct gtrace_hw_ops *ops = comp->pdata->hw_ops;
+
+ if (!ops || !ops->reset)
+ return -EOPNOTSUPP;
+
+ return ops->reset(comp->pdata);
+}
+EXPORT_SYMBOL_GPL(gtrace_reset_component);
+
+struct gtrace_component *gtrace_cpu_source(unsigned int cpu)
+{
+ if (!cpu_present(cpu))
+ return NULL;
+
+ return per_cpu(gtrace_cpu_source_comp, cpu);
+}
+EXPORT_SYMBOL_GPL(gtrace_cpu_source);
+
+static int gtrace_cleanup_inconn(struct device *dev, void *data)
+{
+ struct gtrace_component *comp = to_gtrace_component(dev);
+ struct gtrace_platform_data *pdata = comp->pdata;
+ struct gtrace_connection *conn = data;
+ int i;
+
+ if (device_match_fwnode(&comp->dev, conn->dest_fwnode)) {
+ for (i = 0; i < pdata->nr_inconns; i++) {
+ if (pdata->inconns[i] != conn)
+ continue;
+ pdata->inconns[i] = NULL;
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
+static void gtrace_cleanup_inconns_from_outconns(struct gtrace_component *comp)
+{
+ struct gtrace_platform_data *pdata = comp->pdata;
+ struct gtrace_connection *conn;
+ int i;
+
+ lockdep_assert_held(>race_mutex);
+
+ for (i = 0; i < pdata->nr_outconns; i++) {
+ conn = pdata->outconns[i];
+ bus_for_each_dev(>race_bustype, NULL, conn, gtrace_cleanup_inconn);
+ }
+}
+
+static int gtrace_setup_inconn(struct device *dev, void *data)
+{
+ struct gtrace_component *comp = to_gtrace_component(dev);
+ struct gtrace_platform_data *pdata = comp->pdata;
+ struct gtrace_connection *conn = data;
+ int i;
+
+ if (device_match_fwnode(&comp->dev, conn->dest_fwnode)) {
+ for (i = 0; i < pdata->nr_inconns; i++) {
+ if (pdata->inconns[i])
+ continue;
+ pdata->inconns[i] = conn;
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
+static int gtrace_setup_inconns_from_outconns(struct gtrace_component *comp)
+{
+ struct gtrace_platform_data *pdata = comp->pdata;
+ struct gtrace_connection *conn;
+ int i, ret;
+
+ lockdep_assert_held(>race_mutex);
+
+ for (i = 0; i < pdata->nr_outconns; i++) {
+ conn = pdata->outconns[i];
+ ret = bus_for_each_dev(>race_bustype, NULL, conn, gtrace_setup_inconn);
+ if (ret < 0) {
+ gtrace_cleanup_inconns_from_outconns(comp);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void gtrace_component_release(struct device *dev)
+{
+ struct gtrace_component *comp = to_gtrace_component(dev);
+
+ fwnode_handle_put(comp->dev.fwnode);
+ gtrace_free_type_idx(comp);
+ kfree(comp);
+}
+
+struct gtrace_component *gtrace_register_component(struct gtrace_component_id *id,
+ const char *name,
+ struct gtrace_platform_data *pdata)
+{
+ struct gtrace_connection *conn;
+ struct gtrace_component *comp;
+ int i, ret = 0;
+
+ if (!id || id->type >= GTRACE_COMPONENT_TYPE_MAX) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ if (!pdata || !pdata->dev) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ for (i = 0; i < pdata->nr_inconns; i++) {
+ if (pdata->inconns[i]) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+ }
+
+ for (i = 0; i < pdata->nr_outconns; i++) {
+ conn = pdata->outconns[i];
+ if (!conn || conn->src_port < 0 || conn->src_comp ||
+ !device_match_fwnode(pdata->dev, conn->src_fwnode) ||
+ conn->dest_port < 0 || !conn->dest_fwnode || !conn->dest_comp) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+ }
+
+ if (pdata->bound_cpu >= 0 && !cpu_present(pdata->bound_cpu)) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ comp = kzalloc(sizeof(*comp), GFP_KERNEL);
+ if (!comp) {
+ ret = -ENOMEM;
+ goto err_out;
+ }
+ comp->pdata = pdata;
+ comp->id = *id;
+ ret = gtrace_alloc_type_idx(comp);
+ if (ret) {
+ kfree(comp);
+ goto err_out;
+ }
+
+ comp->dev.parent = pdata->dev;
+ comp->dev.coherent_dma_mask = pdata->dev->coherent_dma_mask;
+ comp->dev.release = gtrace_component_release;
+ comp->dev.bus = >race_bustype;
+ comp->dev.fwnode = fwnode_handle_get(dev_fwnode(pdata->dev));
+ dev_set_name(&comp->dev, "%s-%u", name ? name : "comp", comp->type_idx);
+
+ mutex_lock(>race_mutex);
+
+ ret = device_register(&comp->dev);
+ if (ret) {
+ put_device(&comp->dev);
+ goto err_out_unlock;
+ }
+
+ for (i = 0; i < pdata->nr_outconns; i++) {
+ conn = pdata->outconns[i];
+ conn->src_comp = comp;
+ }
+
+ ret = gtrace_setup_inconns_from_outconns(comp);
+ if (ret < 0) {
+ device_unregister(&comp->dev);
+ goto err_out_unlock;
+ }
+
+ if (comp->pdata->bound_cpu >= 0) {
+ gtrace_get_component(comp);
+ per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu) = comp;
+ }
+
+ mutex_unlock(>race_mutex);
+
+ return comp;
+
+err_out_unlock:
+ mutex_unlock(>race_mutex);
+err_out:
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(gtrace_register_component);
+
+void gtrace_unregister_component(struct gtrace_component *comp)
+{
+ struct gtrace_component *c;
+
+ mutex_lock(>race_mutex);
+
+ if (comp->pdata->bound_cpu >= 0) {
+ c = per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu);
+ per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu) = NULL;
+ gtrace_put_component(c);
+ }
+
+ gtrace_cleanup_inconns_from_outconns(comp);
+ device_unregister(&comp->dev);
+
+ mutex_unlock(>race_mutex);
+}
+EXPORT_SYMBOL_GPL(gtrace_unregister_component);
+
+int __gtrace_register_driver(struct module *owner, struct gtrace_driver *gtdrv)
+{
+ gtdrv->driver.owner = owner;
+ gtdrv->driver.bus = >race_bustype;
+
+ return driver_register(>drv->driver);
+}
+EXPORT_SYMBOL_GPL(__gtrace_register_driver);
+
+static int __init gtrace_init(void)
+{
+ gtrace_init_type_idx();
+
+ return bus_register(>race_bustype);
+}
+
+static void __exit gtrace_exit(void)
+{
+ bus_unregister(>race_bustype);
+}
+
+subsys_initcall(gtrace_init);
+module_exit(gtrace_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Generic hardware trace (gtrace) framework core");
diff --git a/include/linux/gtrace.h b/include/linux/gtrace.h
new file mode 100644
index 000000000000..e8ca2bd46e67
--- /dev/null
+++ b/include/linux/gtrace.h
@@ -0,0 +1,266 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ */
+
+#ifndef __LINUX_GTRACE_H__
+#define __LINUX_GTRACE_H__
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/types.h>
+
+/*
+ * Global list of trace component types across all architectures. Each backend
+ * uses the entries relevant to it.
+ */
+enum gtrace_component_type {
+ GTRACE_RVTRACE_ENCODER,
+ GTRACE_RVTRACE_FUNNEL,
+ GTRACE_RVTRACE_RAMSINK,
+ GTRACE_RVTRACE_PIBSINK,
+ GTRACE_RVTRACE_ATBBRIDGE,
+ GTRACE_COMPONENT_TYPE_MAX
+};
+
+/* Supported usage modes for trace components */
+enum gtrace_component_mode {
+ GTRACE_COMPONENT_MODE_PERF,
+ GTRACE_COMPONENT_MODE_MAX
+};
+
+/**
+ * struct gtrace_connection - Physical connection between two trace components.
+ * @src_port: A connection's source port number.
+ * @src_fwnode: Source component's fwnode handle.
+ * @src_comp: Source component's pointer.
+ * @dest_port: A connection's destination port number.
+ * @dest_fwnode: Destination component's fwnode handle.
+ * @dest_comp: Destination component's pointer.
+ */
+struct gtrace_connection {
+ int src_port;
+ struct fwnode_handle *src_fwnode;
+ int dest_port;
+ struct fwnode_handle *dest_fwnode;
+ struct gtrace_component *src_comp;
+ struct gtrace_component *dest_comp;
+};
+
+struct gtrace_platform_data;
+
+/**
+ * struct gtrace_hw_ops - Architecture-specific low level control of a component.
+ * @enable: Enable the component's hardware block.
+ * @disable: Disable the component's hardware block.
+ * @reset: Reset the component's hardware block.
+ *
+ * These operate on the component's platform data rather than a full
+ * gtrace_component so that they can be invoked before the component is
+ * registered with the gtrace core (e.g. to reset the hardware during probe).
+ *
+ * These abstract the register-level programming that differs between
+ * architectures (RISC-V Trace Control registers vs ARM CoreSight, etc.). The
+ * core never touches component registers directly; it invokes these ops.
+ */
+struct gtrace_hw_ops {
+ int (*enable)(struct gtrace_platform_data *pdata);
+ int (*disable)(struct gtrace_platform_data *pdata);
+ int (*reset)(struct gtrace_platform_data *pdata);
+};
+
+/**
+ * struct gtrace_platform_data - Platform-level data for a trace component
+ * discovered from DT or ACPI.
+ * @dev: Parent device.
+ * @impid: Opaque, architecture-specific implementation ID.
+ * @hw_ops: Architecture-specific low level control ops.
+ * @io_mem: Flag showing whether component registers are memory mapped.
+ * @base: If io_mem == true then base address of the mapped registers.
+ * @read: If io_mem == false then read register from the given offset.
+ * @write: If io_mem == false then write register to the given offset.
+ * @bound_cpu: CPU to which the component is bound, or -1 if unbound. For a
+ * source component bound to a CPU this must not be -1.
+ * @control_poll_timeout_usecs: Delay in usecs when polling control bits.
+ * @nr_inconns: Number of input connections.
+ * @inconns: Array of pointers to input connections.
+ * @nr_outconns: Number of output connections.
+ * @outconns: Array of pointers to output connections.
+ */
+struct gtrace_platform_data {
+ struct device *dev;
+
+ u32 impid;
+
+ const struct gtrace_hw_ops *hw_ops;
+
+ bool io_mem;
+ union {
+ void __iomem *base;
+ struct {
+ u32 (*read)(struct gtrace_platform_data *pdata,
+ u32 offset, bool relaxed);
+ void (*write)(struct gtrace_platform_data *pdata,
+ u32 val, u32 offset, bool relaxed);
+ };
+ };
+
+ int bound_cpu;
+
+ /* Delay in microseconds when polling control register bits */
+ int control_poll_timeout_usecs;
+
+ /*
+ * Platform driver must only populate empty pointer array without
+ * any actual input connections.
+ */
+ unsigned int nr_inconns;
+ struct gtrace_connection **inconns;
+
+ /*
+ * Platform driver must fully populate pointer array with individual
+ * array elements pointing to actual output connections. The src_comp
+ * of each output connection is automatically updated at the time of
+ * registering component.
+ */
+ unsigned int nr_outconns;
+ struct gtrace_connection **outconns;
+};
+
+static inline u32 gtrace_read32(struct gtrace_platform_data *pdata, u32 offset)
+{
+ if (likely(pdata->io_mem))
+ return readl(pdata->base + offset);
+
+ return pdata->read(pdata, offset, false);
+}
+
+static inline u32 gtrace_relaxed_read32(struct gtrace_platform_data *pdata, u32 offset)
+{
+ if (likely(pdata->io_mem))
+ return readl_relaxed(pdata->base + offset);
+
+ return pdata->read(pdata, offset, true);
+}
+
+static inline void gtrace_write32(struct gtrace_platform_data *pdata, u32 val, u32 offset)
+{
+ if (likely(pdata->io_mem))
+ writel(val, pdata->base + offset);
+ else
+ pdata->write(pdata, val, offset, false);
+}
+
+static inline void gtrace_relaxed_write32(struct gtrace_platform_data *pdata,
+ u32 val, u32 offset)
+{
+ if (likely(pdata->io_mem))
+ writel_relaxed(val, pdata->base + offset);
+ else
+ pdata->write(pdata, val, offset, true);
+}
+
+static inline bool gtrace_is_source(struct gtrace_platform_data *pdata)
+{
+ return !pdata->nr_inconns ? true : false;
+}
+
+static inline bool gtrace_is_sink(struct gtrace_platform_data *pdata)
+{
+ return !pdata->nr_outconns ? true : false;
+}
+
+/**
+ * struct gtrace_component_id - Details to identify or match a trace component.
+ * @type: Component type from the global gtrace_component_type list.
+ * @version: Architecture-specific version (opaque to the core).
+ * @data: Data pointer for driver use.
+ */
+struct gtrace_component_id {
+ enum gtrace_component_type type;
+ u32 version;
+ void *data;
+};
+
+/**
+ * struct gtrace_component - Representation of a trace component.
+ * @pdata: Pointer to underlying platform data.
+ * @id: Details to match the component.
+ * @type_idx: Unique number based on component type.
+ * @dev: Device instance.
+ * @ready: Flag showing whether the driver was probed successfully.
+ */
+struct gtrace_component {
+ struct gtrace_platform_data *pdata;
+ struct gtrace_component_id id;
+ u32 type_idx;
+ struct device dev;
+ bool ready;
+};
+
+#define to_gtrace_component(__dev) container_of_const(__dev, struct gtrace_component, dev)
+
+static inline void gtrace_get_component(struct gtrace_component *comp)
+{
+ get_device(&comp->dev);
+}
+
+static inline void gtrace_put_component(struct gtrace_component *comp)
+{
+ put_device(&comp->dev);
+}
+
+const struct gtrace_component_id *gtrace_match_id(struct gtrace_component *comp,
+ const struct gtrace_component_id *ids);
+struct gtrace_component *gtrace_find_by_fwnode(struct fwnode_handle *fwnode);
+
+int gtrace_poll_bit(struct gtrace_platform_data *pdata, int offset,
+ int bit, int bitval, int timeout);
+int gtrace_enable_component(struct gtrace_component *comp);
+int gtrace_disable_component(struct gtrace_component *comp);
+int gtrace_reset_component(struct gtrace_component *comp);
+
+struct gtrace_component *gtrace_cpu_source(unsigned int cpu);
+
+struct gtrace_component *gtrace_register_component(struct gtrace_component_id *id,
+ const char *name,
+ struct gtrace_platform_data *pdata);
+void gtrace_unregister_component(struct gtrace_component *comp);
+
+/**
+ * struct gtrace_driver - Representation of a trace driver.
+ * @id_table: Table to match components handled by the driver.
+ * @probe: Driver probe() function.
+ * @remove: Driver remove() function.
+ * @get_trace_id: Get/allocate a trace ID.
+ * @put_trace_id: Put/free a trace ID.
+ * @driver: Device driver instance.
+ */
+struct gtrace_driver {
+ const struct gtrace_component_id *id_table;
+ int (*probe)(struct gtrace_component *comp);
+ void (*remove)(struct gtrace_component *comp);
+ int (*get_trace_id)(struct gtrace_component *comp,
+ enum gtrace_component_mode mode);
+ void (*put_trace_id)(struct gtrace_component *comp,
+ enum gtrace_component_mode mode,
+ u32 trace_id);
+ struct device_driver driver;
+};
+
+#define to_gtrace_driver(__drv) \
+ ((__drv) ? container_of_const((__drv), struct gtrace_driver, driver) : NULL)
+
+int __gtrace_register_driver(struct module *owner, struct gtrace_driver *gtdrv);
+#define gtrace_register_driver(driver) __gtrace_register_driver(THIS_MODULE, driver)
+static inline void gtrace_unregister_driver(struct gtrace_driver *gtdrv)
+{
+ if (gtdrv)
+ driver_unregister(>drv->driver);
+}
+
+#endif /* __LINUX_GTRACE_H__ */
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 02/14] hwtracing: gtrace: Initial implementation of gtrace framework
2026-08-10 15:22 ` [PATCH v5 02/14] hwtracing: gtrace: Initial implementation of gtrace framework Mayuresh Chitale
@ 2026-08-10 15:36 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:36 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: robh, devicetree, conor+dt
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Use-After-Free due to missing cleanup of upstream connections upon component unregistration.
- [High] Topology validation failure and missing `dest_comp` assignment lead to registration rejection and NULL pointer dereferences.
- [High] Exported lookup APIs return unreferenced device pointers, causing Use-After-Free risks.
- [High] Unbalanced refcounts and memory leak when multiple components bind to the same CPU.
- [High] Strict probe order dependency silently breaks topology building.
- [High] Infinite recursion during graph traversal leads to kernel stack overflow.
- [High] ID matching logic ignores version comparisons and terminates prematurely.
- [High] Unconditional NULL pointer dereference if driver lacks `id_table`.
- [Medium] Deadlock risk by holding subsystem mutex during device_register.
--
--- Patch [2]: [PATCH v5 02/14] hwtracing: gtrace: Initial implementation of gtrace framework ---
commit 9dea7b9c99e06842582e974d2b7070319605798a
Author: Anup Patel <anup.patel@oss.qualcomm.com>
hwtracing: gtrace: Initial implementation of gtrace framework
This patch introduces a generic hardware trace framework (gtrace). It provides
a bus and topology management helpers for trace components to interact across
different architectures.
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c
[ ... ]
> +const struct gtrace_component_id *gtrace_match_id(struct gtrace_component *comp,
> + const struct gtrace_component_id *ids)
> +{
> + const struct gtrace_component_id *id;
> +
> + for (id = ids; id->version; id++) {
> + if (comp->id.type != id->type)
> + continue;
> +
> + return id;
> + }
> +
> + return NULL;
> +}
[Severity: High]
Does this loop use the version field as a termination sentinel instead of
evaluating it? If a driver defines a component with version 0, it appears
the loop will terminate prematurely.
Also, it looks like a version comparison against comp->id.version is missing
entirely, which might cause it to match any version of a given type.
[ ... ]
> +static int gtrace_match_device(struct device *dev, const struct device_driver *drv)
> +{
> + const struct gtrace_driver *gtdrv = to_gtrace_driver(drv);
> + struct gtrace_component *comp = to_gtrace_component(dev);
> +
> + return gtrace_match_id(comp, gtdrv->id_table) ? 1 : 0;
> +}
[Severity: High]
If a driver is registered on this bus without an id_table (for example, matching
solely by ACPI or OF tables), will passing a NULL id_table here lead to a
NULL pointer dereference when gtrace_match_id() evaluates its loop condition?
[ ... ]
> +struct gtrace_component *gtrace_find_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct gtrace_fwnode_match_data d = { .fwnode = fwnode, .match = NULL };
> + int ret;
> +
> + ret = bus_for_each_dev(>race_bustype, NULL, &d, gtrace_match_fwnode);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + return d.match;
> +}
[Severity: High]
Since bus_for_each_dev() drops its temporary reference upon returning, does
returning d.match directly leave the caller with an unreferenced raw pointer?
Could this lead to a use-after-free if the component is unregistered
concurrently?
[ ... ]
> +struct gtrace_component *gtrace_cpu_source(unsigned int cpu)
> +{
> + if (!cpu_present(cpu))
> + return NULL;
> +
> + return per_cpu(gtrace_cpu_source_comp, cpu);
> +}
[Severity: High]
Similar to the fwnode lookup above, should this function return a referenced
device pointer? Exposing the unreferenced component might cause a
use-after-free if the component is suddenly unregistered by another thread.
[ ... ]
> +static int gtrace_setup_inconn(struct device *dev, void *data)
> +{
> + struct gtrace_component *comp = to_gtrace_component(dev);
> + struct gtrace_platform_data *pdata = comp->pdata;
> + struct gtrace_connection *conn = data;
> + int i;
> +
> + if (device_match_fwnode(&comp->dev, conn->dest_fwnode)) {
> + for (i = 0; i < pdata->nr_inconns; i++) {
> + if (pdata->inconns[i])
> + continue;
> + pdata->inconns[i] = conn;
> + return 1;
> + }
> + }
[Severity: High]
While this properly links the connection into the destination's inconns array,
should it also assign conn->dest_comp = comp? If conn->dest_comp remains
unassigned (NULL), won't later graph traversals encounter a NULL pointer
dereference?
[ ... ]
> +static int gtrace_setup_inconns_from_outconns(struct gtrace_component *comp)
> +{
> + struct gtrace_platform_data *pdata = comp->pdata;
> + struct gtrace_connection *conn;
> + int i, ret;
> +
> + lockdep_assert_held(>race_mutex);
> +
> + for (i = 0; i < pdata->nr_outconns; i++) {
> + conn = pdata->outconns[i];
> + ret = bus_for_each_dev(>race_bustype, NULL, conn, gtrace_setup_inconn);
> + if (ret < 0) {
> + gtrace_cleanup_inconns_from_outconns(comp);
> + return ret;
> + }
> + }
[Severity: High]
If a destination component has not probed yet, bus_for_each_dev() will return 0.
Will treating 0 as a success leave this connection permanently incomplete,
since the framework does not appear to re-evaluate bidirectional matches or
request probe deferrals?
[ ... ]
> +struct gtrace_component *gtrace_register_component(struct gtrace_component_id *id,
> + const char *name,
> + struct gtrace_platform_data *pdata)
> +{
> + struct gtrace_connection *conn;
> + struct gtrace_component *comp;
> + int i, ret = 0;
> +
> + if (!id || id->type >= GTRACE_COMPONENT_TYPE_MAX) {
> + ret = -EINVAL;
> + goto err_out;
> + }
> +
> + if (!pdata || !pdata->dev) {
> + ret = -EINVAL;
> + goto err_out;
> + }
> +
> + for (i = 0; i < pdata->nr_inconns; i++) {
> + if (pdata->inconns[i]) {
> + ret = -EINVAL;
> + goto err_out;
> + }
> + }
> +
> + for (i = 0; i < pdata->nr_outconns; i++) {
> + conn = pdata->outconns[i];
> + if (!conn || conn->src_port < 0 || conn->src_comp ||
> + !device_match_fwnode(pdata->dev, conn->src_fwnode) ||
> + conn->dest_port < 0 || !conn->dest_fwnode || !conn->dest_comp) {
> + ret = -EINVAL;
> + goto err_out;
> + }
> + }
[Severity: High]
During component registration, won't the !conn->dest_comp check always fail
here since the dynamic discovery hasn't had a chance to run yet? This appears
to reject registration for any component that has output connections.
[ ... ]
> + mutex_lock(>race_mutex);
> +
> + ret = device_register(&comp->dev);
[Severity: Medium]
Calling device_register() triggers a synchronous driver probe. If the driver's
probe routine calls back into an exported framework API that also attempts to
acquire gtrace_mutex, could this result in a deadlock?
[ ... ]
> + if (comp->pdata->bound_cpu >= 0) {
> + gtrace_get_component(comp);
> + per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu) = comp;
> + }
[Severity: High]
If multiple components bind to the same CPU, does blindly overwriting this
per-CPU pointer without releasing the previously bound component lead to a
memory leak?
[ ... ]
> +void gtrace_unregister_component(struct gtrace_component *comp)
> +{
> + struct gtrace_component *c;
> +
> + mutex_lock(>race_mutex);
> +
> + if (comp->pdata->bound_cpu >= 0) {
> + c = per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu);
> + per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu) = NULL;
> + gtrace_put_component(c);
> + }
[Severity: High]
If an old component's CPU slot was overwritten by a newer component (leaking
the original reference as mentioned above), could putting the current occupant
here cause us to prematurely free the newer component?
> +
> + gtrace_cleanup_inconns_from_outconns(comp);
> + device_unregister(&comp->dev);
[Severity: High]
This cleans up downstream outconns, but what about upstream components that
connect to this component? If upstream nodes retain dangling pointers to this
freed component, could it cause a use-after-free during graph traversal?
One additional question regarding graph traversals: when recursively walking the
graph topology across connections, it doesn't appear that visited nodes are
tracked. If hardware topology tables are misconfigured with a cycle
(e.g., A -> B -> A), could this trigger infinite recursion and overflow the
kernel stack?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 03/14] gtrace: Add RISC-V platform driver for the gtrace framework
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
2026-08-10 15:22 ` [PATCH v5 01/14] dt-bindings: Add RISC-V trace component bindings Mayuresh Chitale
2026-08-10 15:22 ` [PATCH v5 02/14] hwtracing: gtrace: Initial implementation of gtrace framework Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:38 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 04/14] gtrace: Add functions to create/destroy a trace component path Mayuresh Chitale
` (10 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Anup Patel, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel,
Mayuresh Chitale
From: Anup Patel <anup.patel@oss.qualcomm.com>
The RISC-V trace components defined by the RISC-V Trace Control Interface
Specification are memory mapped devices which are organized in a
graph-like topology and described in the device tree.
Add the RISC-V platform driver for the gtrace framework. The driver
parses the RISC-V component topology from the device tree, discovers the
type and version of each component from its IMPL register, provides the
RISC-V register level control ops and registers each component with the
gtrace core.
Co-developed-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
drivers/hwtracing/gtrace/Kconfig | 15 +
drivers/hwtracing/gtrace/Makefile | 2 +
drivers/hwtracing/gtrace/rvtrace-platform.c | 286 ++++++++++++++++++++
drivers/hwtracing/gtrace/rvtrace.h | 121 +++++++++
4 files changed, 424 insertions(+)
create mode 100644 drivers/hwtracing/gtrace/rvtrace-platform.c
create mode 100644 drivers/hwtracing/gtrace/rvtrace.h
diff --git a/drivers/hwtracing/gtrace/Kconfig b/drivers/hwtracing/gtrace/Kconfig
index bcc3b4169a76..04986f216549 100644
--- a/drivers/hwtracing/gtrace/Kconfig
+++ b/drivers/hwtracing/gtrace/Kconfig
@@ -13,3 +13,18 @@ menuconfig GTRACE
To compile this driver as a module, choose M here: the module
will be called gtrace.
+
+config RVTRACE
+ tristate "RISC-V Trace Support"
+ depends on RISCV
+ depends on OF
+ depends on GTRACE
+ default RISCV
+ help
+ This provides the RISC-V backend for the generic hardware trace
+ (gtrace) framework. It parses the RISC-V trace component topology,
+ provides the RISC-V register-level control ops and registers each
+ component with the gtrace core.
+
+ To compile this driver as a module, choose M here: the module
+ will be called rvtrace.
diff --git a/drivers/hwtracing/gtrace/Makefile b/drivers/hwtracing/gtrace/Makefile
index 3f90fb99c514..cf02316c1bbf 100644
--- a/drivers/hwtracing/gtrace/Makefile
+++ b/drivers/hwtracing/gtrace/Makefile
@@ -2,3 +2,5 @@
obj-$(CONFIG_GTRACE) += gtrace.o
gtrace-y := gtrace-core.o
+
+obj-$(CONFIG_RVTRACE) += rvtrace-platform.o
diff --git a/drivers/hwtracing/gtrace/rvtrace-platform.c b/drivers/hwtracing/gtrace/rvtrace-platform.c
new file mode 100644
index 000000000000..f34de9183d9a
--- /dev/null
+++ b/drivers/hwtracing/gtrace/rvtrace-platform.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ */
+
+#include <linux/device.h>
+#include <linux/gtrace.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/types.h>
+#include "rvtrace.h"
+
+static int rvtrace_hw_enable(struct gtrace_platform_data *pdata)
+{
+ u32 val;
+
+ val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
+ val |= BIT(RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT);
+ gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
+ return gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
+ RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT, 1,
+ pdata->control_poll_timeout_usecs);
+}
+
+static int rvtrace_hw_disable(struct gtrace_platform_data *pdata)
+{
+ u32 val;
+
+ val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
+ val &= ~BIT(RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT);
+ gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
+ return gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
+ RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT, 0,
+ pdata->control_poll_timeout_usecs);
+}
+
+static int rvtrace_hw_reset(struct gtrace_platform_data *pdata)
+{
+ int ret;
+
+ gtrace_write32(pdata, 0, RVTRACE_COMPONENT_CTRL_OFFSET);
+ ret = gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
+ RVTRACE_COMPONENT_CTRL_ACTIVE_SHIFT, 0,
+ pdata->control_poll_timeout_usecs);
+ if (ret)
+ return ret;
+
+ gtrace_write32(pdata, RVTRACE_COMPONENT_CTRL_ACTIVE_MASK,
+ RVTRACE_COMPONENT_CTRL_OFFSET);
+ return gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
+ RVTRACE_COMPONENT_CTRL_ACTIVE_SHIFT, 1,
+ pdata->control_poll_timeout_usecs);
+}
+
+const struct gtrace_hw_ops rvtrace_hw_ops = {
+ .enable = rvtrace_hw_enable,
+ .disable = rvtrace_hw_disable,
+ .reset = rvtrace_hw_reset,
+};
+
+static int rvtrace_of_parse_outconns(struct gtrace_platform_data *pdata)
+{
+ struct device_node *parent, *ep_node, *rep_node, *rdev_node;
+ struct gtrace_connection *conn;
+ struct of_endpoint ep, rep;
+ int ret = 0, i = 0;
+
+ parent = of_get_child_by_name(dev_of_node(pdata->dev), "out-ports");
+ if (!parent)
+ return 0;
+
+ pdata->nr_outconns = of_graph_get_endpoint_count(parent);
+ pdata->outconns = devm_kcalloc(pdata->dev, pdata->nr_outconns,
+ sizeof(*pdata->outconns), GFP_KERNEL);
+ if (!pdata->outconns) {
+ ret = -ENOMEM;
+ goto done;
+ }
+
+ for_each_endpoint_of_node(parent, ep_node) {
+ conn = devm_kzalloc(pdata->dev, sizeof(*conn), GFP_KERNEL);
+ if (!conn) {
+ of_node_put(ep_node);
+ ret = -ENOMEM;
+ break;
+ }
+
+ ret = of_graph_parse_endpoint(ep_node, &ep);
+ if (ret) {
+ of_node_put(ep_node);
+ break;
+ }
+
+ rep_node = of_graph_get_remote_endpoint(ep_node);
+ if (!rep_node) {
+ ret = -ENODEV;
+ of_node_put(ep_node);
+ break;
+ }
+ rdev_node = of_graph_get_port_parent(rep_node);
+
+ ret = of_graph_parse_endpoint(rep_node, &rep);
+ if (ret) {
+ of_node_put(ep_node);
+ of_node_put(rep_node);
+ of_node_put(rdev_node);
+ break;
+ }
+
+ conn->src_port = ep.port;
+ conn->src_fwnode = dev_fwnode(pdata->dev);
+ /* The 'src_comp' is set by gtrace_register_component() */
+ conn->src_comp = NULL;
+ conn->dest_port = rep.port;
+ conn->dest_fwnode = of_fwnode_handle(rdev_node);
+ fwnode_handle_get(conn->dest_fwnode);
+ conn->dest_comp = gtrace_find_by_fwnode(conn->dest_fwnode);
+ if (!conn->dest_comp) {
+ ret = -EPROBE_DEFER;
+ of_node_put(ep_node);
+ of_node_put(rep_node);
+ of_node_put(rdev_node);
+ break;
+ }
+
+ pdata->outconns[i] = conn;
+ i++;
+ }
+
+done:
+ if (ret) {
+ for (i = 0; i < pdata->nr_outconns && pdata->outconns; i++) {
+ conn = pdata->outconns[i];
+ if (conn && conn->dest_fwnode)
+ fwnode_handle_put(conn->dest_fwnode);
+ }
+ }
+ of_node_put(parent);
+ return ret;
+}
+
+static int rvtrace_of_parse_inconns(struct gtrace_platform_data *pdata)
+{
+ struct device_node *parent;
+ int ret = 0;
+
+ parent = of_get_child_by_name(dev_of_node(pdata->dev), "in-ports");
+ if (!parent)
+ return 0;
+
+ pdata->nr_inconns = of_graph_get_endpoint_count(parent);
+ pdata->inconns = devm_kcalloc(pdata->dev, pdata->nr_inconns,
+ sizeof(*pdata->inconns), GFP_KERNEL);
+ if (!pdata->inconns)
+ ret = -ENOMEM;
+
+ of_node_put(parent);
+ return ret;
+}
+
+static int rvtrace_platform_probe(struct platform_device *pdev)
+{
+ struct gtrace_platform_data *pdata;
+ struct device *dev = &pdev->dev;
+ struct gtrace_component_id id;
+ struct gtrace_component *comp;
+ u32 impl, type, major, minor;
+ struct device_node *node;
+ struct resource *res;
+ int gtype;
+ int ret;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+ pdata->dev = dev;
+ pdata->impid = RVTRACE_COMPONENT_IMPID_UNKNOWN;
+ pdata->hw_ops = &rvtrace_hw_ops;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ pdata->io_mem = true;
+ pdata->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+ if (!pdata->base)
+ return dev_err_probe(dev, -ENOMEM, "failed to ioremap %pR\n", res);
+
+ pdata->bound_cpu = -1;
+ node = of_parse_phandle(dev_of_node(dev), "cpus", 0);
+ if (node) {
+ ret = of_cpu_node_to_id(node);
+ of_node_put(node);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to get CPU id for %pOF\n", node);
+ pdata->bound_cpu = ret;
+ }
+
+ /* Default control poll timeout */
+ pdata->control_poll_timeout_usecs = 10;
+
+ ret = rvtrace_of_parse_outconns(pdata);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to parse output connections\n");
+
+ ret = rvtrace_of_parse_inconns(pdata);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to parse input connections\n");
+
+ /* Reset the component before it is registered with the gtrace core. */
+ ret = rvtrace_hw_reset(pdata);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to reset component\n");
+
+ impl = gtrace_read32(pdata, RVTRACE_COMPONENT_IMPL_OFFSET);
+ type = (impl >> RVTRACE_COMPONENT_IMPL_TYPE_SHIFT) &
+ RVTRACE_COMPONENT_IMPL_TYPE_MASK;
+ major = (impl >> RVTRACE_COMPONENT_IMPL_VERMAJOR_SHIFT) &
+ RVTRACE_COMPONENT_IMPL_VERMAJOR_MASK;
+ minor = (impl >> RVTRACE_COMPONENT_IMPL_VERMINOR_SHIFT) &
+ RVTRACE_COMPONENT_IMPL_VERMINOR_MASK;
+
+ gtype = rvtrace_type_to_gtrace(type);
+ if (gtype < 0)
+ return dev_err_probe(dev, -ENODEV, "unsupported component type %u\n", type);
+
+ id.type = gtype;
+ id.version = rvtrace_component_mkversion(major, minor);
+
+ comp = gtrace_register_component(&id, rvtrace_type_name(gtype), pdata);
+ if (IS_ERR(comp))
+ return PTR_ERR(comp);
+
+ platform_set_drvdata(pdev, comp);
+ return 0;
+}
+
+static void rvtrace_platform_remove(struct platform_device *pdev)
+{
+ struct gtrace_component *comp = platform_get_drvdata(pdev);
+ struct gtrace_platform_data *pdata = comp->pdata;
+ struct gtrace_connection *conn;
+ int i;
+
+ for (i = 0; i < pdata->nr_outconns; i++) {
+ conn = pdata->outconns[i];
+ if (conn && conn->dest_fwnode)
+ fwnode_handle_put(conn->dest_fwnode);
+ }
+
+ gtrace_unregister_component(comp);
+}
+
+static const struct of_device_id rvtrace_platform_match[] = {
+ { .compatible = "riscv,trace-component" },
+ {}
+};
+
+static struct platform_driver rvtrace_platform_driver = {
+ .driver = {
+ .name = "rvtrace",
+ .of_match_table = rvtrace_platform_match,
+ },
+ .probe = rvtrace_platform_probe,
+ .remove = rvtrace_platform_remove,
+};
+
+static int __init rvtrace_platform_init(void)
+{
+ return platform_driver_register(&rvtrace_platform_driver);
+}
+
+static void __exit rvtrace_platform_exit(void)
+{
+ platform_driver_unregister(&rvtrace_platform_driver);
+}
+
+module_init(rvtrace_platform_init);
+module_exit(rvtrace_platform_exit);
+
+MODULE_AUTHOR("Anup Patel");
+MODULE_DESCRIPTION("RISC-V backend for the generic trace framework");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwtracing/gtrace/rvtrace.h b/drivers/hwtracing/gtrace/rvtrace.h
new file mode 100644
index 000000000000..e6be1ce303cc
--- /dev/null
+++ b/drivers/hwtracing/gtrace/rvtrace.h
@@ -0,0 +1,121 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ *
+ * RISC-V trace component register layout and helpers for the RISC-V backend
+ * of the generic trace (gtrace) framework. These definitions are specific to
+ * the RISC-V Trace Control Interface and must not be used by the generic core.
+ */
+
+#ifndef __LINUX_RVTRACE_H__
+#define __LINUX_RVTRACE_H__
+
+#include <linux/gtrace.h>
+#include <linux/types.h>
+
+/* Control register common across all RISC-V trace components */
+#define RVTRACE_COMPONENT_CTRL_OFFSET 0x000
+#define RVTRACE_COMPONENT_CTRL_ACTIVE_MASK 0x1
+#define RVTRACE_COMPONENT_CTRL_ACTIVE_SHIFT 0
+#define RVTRACE_COMPONENT_CTRL_ENABLE_MASK 0x1
+#define RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT 1
+#define RVTRACE_COMPONENT_CTRL_EMPTY_SHIFT 3
+
+/* Implementation register common across all RISC-V trace components */
+#define RVTRACE_COMPONENT_IMPL_OFFSET 0x004
+#define RVTRACE_COMPONENT_IMPL_VERMAJOR_MASK 0xf
+#define RVTRACE_COMPONENT_IMPL_VERMAJOR_SHIFT 0
+#define RVTRACE_COMPONENT_IMPL_VERMINOR_MASK 0xf
+#define RVTRACE_COMPONENT_IMPL_VERMINOR_SHIFT 4
+#define RVTRACE_COMPONENT_IMPL_TYPE_MASK 0xf
+#define RVTRACE_COMPONENT_IMPL_TYPE_SHIFT 8
+
+/* Component types defined by the RISC-V Trace Control Interface */
+enum rvtrace_component_type {
+ RVTRACE_COMPONENT_TYPE_RESV0,
+ RVTRACE_COMPONENT_TYPE_ENCODER, /* 0x1 */
+ RVTRACE_COMPONENT_TYPE_RESV2,
+ RVTRACE_COMPONENT_TYPE_RESV3,
+ RVTRACE_COMPONENT_TYPE_RESV4,
+ RVTRACE_COMPONENT_TYPE_RESV5,
+ RVTRACE_COMPONENT_TYPE_RESV6,
+ RVTRACE_COMPONENT_TYPE_RESV7,
+ RVTRACE_COMPONENT_TYPE_FUNNEL, /* 0x8 */
+ RVTRACE_COMPONENT_TYPE_RAMSINK, /* 0x9 */
+ RVTRACE_COMPONENT_TYPE_PIBSINK, /* 0xA */
+ RVTRACE_COMPONENT_TYPE_RESV11,
+ RVTRACE_COMPONENT_TYPE_RESV12,
+ RVTRACE_COMPONENT_TYPE_RESV13,
+ RVTRACE_COMPONENT_TYPE_ATBBRIDGE, /* 0xE */
+ RVTRACE_COMPONENT_TYPE_RESV15,
+ RVTRACE_COMPONENT_TYPE_MAX
+};
+
+/*
+ * Possible component implementation IDs discovered from DT or ACPI
+ * shared across the RISC-V trace drivers to infer trace parameters,
+ * quirks, and work-arounds. These component implementation IDs are
+ * internal to Linux and must not be exposed to user-space.
+ *
+ * The component implementation ID should be named as follows:
+ * RVTRACE_COMPONENT_IMPID_<vendor>_<part>
+ */
+enum rvtrace_component_impid {
+ RVTRACE_COMPONENT_IMPID_UNKNOWN,
+ RVTRACE_COMPONENT_IMPID_MAX
+};
+
+#define rvtrace_component_version_major(__version) \
+ (((__version) >> 16) & 0xffff)
+#define rvtrace_component_version_minor(__version) \
+ ((__version) & 0xffff)
+#define rvtrace_component_mkversion(__major, __minor) \
+ ((((__major) & 0xffff) << 16) | ((__minor) & 0xffff))
+
+/* Map a RISC-V hardware component type to the global gtrace type */
+static inline int rvtrace_type_to_gtrace(enum rvtrace_component_type type)
+{
+ switch (type) {
+ case RVTRACE_COMPONENT_TYPE_ENCODER:
+ return GTRACE_RVTRACE_ENCODER;
+ case RVTRACE_COMPONENT_TYPE_FUNNEL:
+ return GTRACE_RVTRACE_FUNNEL;
+ case RVTRACE_COMPONENT_TYPE_RAMSINK:
+ return GTRACE_RVTRACE_RAMSINK;
+ case RVTRACE_COMPONENT_TYPE_PIBSINK:
+ return GTRACE_RVTRACE_PIBSINK;
+ case RVTRACE_COMPONENT_TYPE_ATBBRIDGE:
+ return GTRACE_RVTRACE_ATBBRIDGE;
+ default:
+ return -EINVAL;
+ }
+}
+
+static inline const char *rvtrace_type_name(enum gtrace_component_type type)
+{
+ switch (type) {
+ case GTRACE_RVTRACE_ENCODER:
+ return "encoder";
+ case GTRACE_RVTRACE_FUNNEL:
+ return "funnel";
+ case GTRACE_RVTRACE_RAMSINK:
+ return "ramsink";
+ case GTRACE_RVTRACE_PIBSINK:
+ return "pibsink";
+ case GTRACE_RVTRACE_ATBBRIDGE:
+ return "atbbridge";
+ default:
+ return NULL;
+ }
+}
+
+static inline int rvtrace_comp_poll_empty(struct gtrace_component *comp)
+{
+ return gtrace_poll_bit(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
+ RVTRACE_COMPONENT_CTRL_EMPTY_SHIFT, 1,
+ comp->pdata->control_poll_timeout_usecs);
+}
+
+extern const struct gtrace_hw_ops rvtrace_hw_ops;
+
+#endif /* __LINUX_RVTRACE_H__ */
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 03/14] gtrace: Add RISC-V platform driver for the gtrace framework
2026-08-10 15:22 ` [PATCH v5 03/14] gtrace: Add RISC-V platform driver for the " Mayuresh Chitale
@ 2026-08-10 15:38 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:38 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues:
- [High] Missing `IS_ERR` check for `gtrace_find_by_fwnode()` return value.
- [High] The CPU device node is used in a format string after its reference count has been decremented.
- [High] Dropping fwnode references before unregistering the component causes a use-after-free in the core subsystem.
- [Medium] Device node references `rep_node` and `rdev_node` are leaked on the success path in `rvtrace_of_parse_outconns()`.
- [Medium] Leak of destination fwnode reference when component lookup fails in `rvtrace_of_parse_outconns()`.
- [Medium] Output connection fwnode references are leaked if `rvtrace_platform_probe()` fails after parsing connections.
- [Medium] The driver lacks a `MODULE_DEVICE_TABLE` declaration for its OF match table, preventing automatic module loading.
- [Medium] Unprotected read-modify-write operations on hardware control registers.
Pre-existing issues:
- [High] `gtrace_find_by_fwnode()` returns a component pointer without acquiring a reference.
--
--- Patch [3]: [PATCH v5 03/14] gtrace: Add RISC-V platform driver for the gtrace framework ---
commit d15655615f1b10f39198151874d1d481ed52c740
Author: Anup Patel <anup.patel@oss.qualcomm.com>
gtrace: Add RISC-V platform driver for the gtrace framework
The RISC-V trace components defined by the RISC-V Trace Control Interface
Specification are memory mapped devices which are organized in a
graph-like topology and described in the device tree.
Add the RISC-V platform driver for the gtrace framework. The driver
parses the RISC-V component topology from the device tree, discovers the
type and version of each component from its IMPL register, provides the
RISC-V register level control ops and registers each component with the
gtrace core.
Co-developed-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
> diff --git a/drivers/hwtracing/gtrace/rvtrace-platform.c b/drivers/hwtracing/gtrace/rvtrace-platform.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/rvtrace-platform.c
> @@ -0,0 +1,286 @@
[ ... ]
> +static int rvtrace_hw_enable(struct gtrace_platform_data *pdata)
> +{
> + u32 val;
> +
> + val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> + val |= BIT(RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT);
> + gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
[Severity: Medium]
Are these read-modify-write operations safe from concurrent access?
Looking at rvtrace_hw_enable() and rvtrace_hw_disable(), the sequences
gtrace_read32() followed by gtrace_write32() execute locklessly.
If the tracing framework or userspace allows concurrent control, could this
corrupt the RVTRACE_COMPONENT_CTRL_OFFSET register state?
> + return gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
> + RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT, 1,
> + pdata->control_poll_timeout_usecs);
> +}
> +
> +static int rvtrace_hw_disable(struct gtrace_platform_data *pdata)
> +{
> + u32 val;
> +
> + val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> + val &= ~BIT(RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT);
> + gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
> + return gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
> + RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT, 0,
> + pdata->control_poll_timeout_usecs);
> +}
[ ... ]
> +static int rvtrace_of_parse_outconns(struct gtrace_platform_data *pdata)
> +{
[ ... ]
> + conn->dest_comp = gtrace_find_by_fwnode(conn->dest_fwnode);
> + if (!conn->dest_comp) {
[Severity: High]
Does this check properly handle error pointers?
gtrace_find_by_fwnode() returns an ERR_PTR on failure. Since the check is
only for NULL, will an error pointer bypass this condition and be
assigned to conn->dest_comp as a valid pointer, potentially causing a
kernel panic later when dereferenced?
[Severity: High]
This isn't a bug introduced by this patch, but does gtrace_find_by_fwnode()
return a component pointer without acquiring a reference to it?
If we look at gtrace_match_fwnode() in the core subsystem:
drivers/hwtracing/gtrace/gtrace-core.c:gtrace_match_fwnode() {
if (device_match_fwnode(&comp->dev, d->fwnode)) {
d->match = comp;
return 1;
}
}
The component pointer is returned without incrementing its reference count.
Could this lead to a use-after-free if the matched component is
concurrently unregistered before we use it here?
[Severity: Medium]
Does this error path leak the dest_fwnode reference?
We acquired a reference to conn->dest_fwnode earlier with
fwnode_handle_get(). If gtrace_find_by_fwnode() fails and we break out
of the loop here, conn is not yet assigned to pdata->outconns.
The cleanup block at the end only frees references that were already
assigned to the pdata->outconns array.
Will this unassigned conn's fwnode reference be permanently leaked?
> + ret = -EPROBE_DEFER;
> + of_node_put(ep_node);
> + of_node_put(rep_node);
> + of_node_put(rdev_node);
> + break;
> + }
> +
> + pdata->outconns[i] = conn;
> + i++;
> + }
[Severity: Medium]
Are the device node references for rep_node and rdev_node leaked on the
success path?
of_graph_get_remote_endpoint() and of_graph_get_port_parent() return
device nodes with incremented reference counts.
When the loop continues to the next iteration after a successful parse,
it doesn't appear that of_node_put() is called for these nodes.
[ ... ]
> +static int rvtrace_platform_probe(struct platform_device *pdev)
> +{
[ ... ]
> + node = of_parse_phandle(dev_of_node(dev), "cpus", 0);
> + if (node) {
> + ret = of_cpu_node_to_id(node);
> + of_node_put(node);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to get CPU id for %pOF\n", node);
[Severity: High]
Does this error path trigger a use-after-free on the device node?
of_node_put(node) is called to drop the reference to the CPU device node.
Immediately afterward on the error path, the same node pointer is passed
to dev_err_probe() using the %pOF format specifier.
Could the format parsing dereference the freed node memory?
> + pdata->bound_cpu = ret;
> + }
> +
> + /* Default control poll timeout */
> + pdata->control_poll_timeout_usecs = 10;
> +
> + ret = rvtrace_of_parse_outconns(pdata);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to parse output connections\n");
> +
> + ret = rvtrace_of_parse_inconns(pdata);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to parse input connections\n");
[Severity: Medium]
Will the fwnode references acquired in rvtrace_of_parse_outconns() be
leaked if initialization fails here?
If rvtrace_of_parse_inconns() or any subsequent initialization step fails,
rvtrace_platform_probe() returns directly.
Since there is no cleanup path to iterate through pdata->outconns and call
fwnode_handle_put(), will all the acquired fwnode references be permanently
leaked?
[ ... ]
> +static void rvtrace_platform_remove(struct platform_device *pdev)
> +{
> + struct gtrace_component *comp = platform_get_drvdata(pdev);
> + struct gtrace_platform_data *pdata = comp->pdata;
> + struct gtrace_connection *conn;
> + int i;
> +
> + for (i = 0; i < pdata->nr_outconns; i++) {
> + conn = pdata->outconns[i];
> + if (conn && conn->dest_fwnode)
> + fwnode_handle_put(conn->dest_fwnode);
> + }
> +
> + gtrace_unregister_component(comp);
> +}
[Severity: High]
Could dropping the fwnode references before unregistering the component
cause a use-after-free in the core subsystem?
Here, the driver drops the references to the destination fwnodes before
calling gtrace_unregister_component(). The core unregistration function
iterates over the connections and accesses the fwnodes via
device_match_fwnode().
Since the references were already dropped, could the memory have been freed,
causing a use-after-free during cleanup?
> +static const struct of_device_id rvtrace_platform_match[] = {
> + { .compatible = "riscv,trace-component" },
> + {}
> +};
[Severity: Medium]
Is a MODULE_DEVICE_TABLE declaration missing for this OF match table?
Platform drivers configured as modules typically expose their device ID
tables using MODULE_DEVICE_TABLE(of, rvtrace_platform_match).
Without it, modaliases won't be exported, which prevents udev from
automatically loading the driver when matching hardware is present.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 04/14] gtrace: Add functions to create/destroy a trace component path
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (2 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 03/14] gtrace: Add RISC-V platform driver for the " Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:34 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 05/14] gtrace: Add functions to start/stop tracing on a " Mayuresh Chitale
` (9 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Anup Patel, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel,
Mayuresh Chitale
From: Anup Patel <anup.patel@oss.qualcomm.com>
Trace needs to be configured on a chain of trace components which are
connected to each other. These chain of components is also referred
to as trace component path. Add functions to create/destroy a trace
component path which will be later used by gtrace perf support.
Co-developed-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
drivers/hwtracing/gtrace/gtrace-core.c | 223 +++++++++++++++++++++++++
include/linux/gtrace.h | 24 +++
2 files changed, 247 insertions(+)
diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
index 97398f2b7cbb..a89d195ab5b8 100644
--- a/drivers/hwtracing/gtrace/gtrace-core.c
+++ b/drivers/hwtracing/gtrace/gtrace-core.c
@@ -210,6 +210,53 @@ int gtrace_reset_component(struct gtrace_component *comp)
}
EXPORT_SYMBOL_GPL(gtrace_reset_component);
+static int __gtrace_walk_output_components(struct gtrace_component *comp,
+ bool *stop, void *priv,
+ int (*fn)(struct gtrace_component *comp, bool *stop,
+ struct gtrace_connection *stop_conn,
+ void *priv))
+{
+ struct gtrace_connection *conn, *stop_conn = NULL;
+ struct gtrace_platform_data *pdata = comp->pdata;
+ int i, ret;
+
+ for (i = 0; i < pdata->nr_outconns; i++) {
+ conn = pdata->outconns[i];
+ ret = __gtrace_walk_output_components(conn->dest_comp, stop, priv, fn);
+ if (ret)
+ return ret;
+ if (*stop) {
+ stop_conn = conn;
+ break;
+ }
+ }
+
+ ret = fn(comp, stop, stop_conn, priv);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+int gtrace_walk_output_components(struct gtrace_component *comp, void *priv,
+ int (*fn)(struct gtrace_component *comp, bool *stop,
+ struct gtrace_connection *stop_conn,
+ void *priv))
+{
+ bool stop = false;
+ int ret;
+
+ if (!comp || !fn)
+ return -EINVAL;
+
+ mutex_lock(>race_mutex);
+ ret = __gtrace_walk_output_components(comp, &stop, priv, fn);
+ mutex_unlock(>race_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gtrace_walk_output_components);
+
struct gtrace_component *gtrace_cpu_source(unsigned int cpu)
{
if (!cpu_present(cpu))
@@ -414,6 +461,182 @@ void gtrace_unregister_component(struct gtrace_component *comp)
}
EXPORT_SYMBOL_GPL(gtrace_unregister_component);
+struct gtrace_path_node {
+ struct list_head head;
+ struct gtrace_component *comp;
+ struct gtrace_connection *conn;
+};
+
+struct gtrace_component *gtrace_path_source(struct gtrace_path *path)
+{
+ struct gtrace_path_node *node;
+
+ node = list_first_entry(&path->comp_list, struct gtrace_path_node, head);
+ return node->comp;
+}
+EXPORT_SYMBOL_GPL(gtrace_path_source);
+
+struct gtrace_component *gtrace_path_sink(struct gtrace_path *path)
+{
+ struct gtrace_path_node *node;
+
+ node = list_last_entry(&path->comp_list, struct gtrace_path_node, head);
+ return node->comp;
+}
+EXPORT_SYMBOL_GPL(gtrace_path_sink);
+
+static int gtrace_assign_trace_id(struct gtrace_path *path)
+{
+ const struct gtrace_driver *gtdrv;
+ struct gtrace_component *comp;
+ struct gtrace_path_node *node;
+ int trace_id;
+
+ list_for_each_entry(node, &path->comp_list, head) {
+ comp = node->comp;
+ gtdrv = to_gtrace_driver(comp->dev.driver);
+
+ if (!gtdrv->get_trace_id)
+ continue;
+
+ trace_id = gtdrv->get_trace_id(comp, path->mode);
+ if (trace_id > 0) {
+ path->trace_id = trace_id;
+ return 0;
+ } else if (trace_id < 0) {
+ return trace_id;
+ }
+ }
+
+ return 0;
+}
+
+static void gtrace_unassign_trace_id(struct gtrace_path *path)
+{
+ const struct gtrace_driver *gtdrv;
+ struct gtrace_component *comp;
+ struct gtrace_path_node *node;
+
+ list_for_each_entry(node, &path->comp_list, head) {
+ comp = node->comp;
+ gtdrv = to_gtrace_driver(comp->dev.driver);
+
+ if (!gtdrv->put_trace_id)
+ continue;
+
+ gtdrv->put_trace_id(comp, path->mode, path->trace_id);
+ }
+}
+
+static bool gtrace_path_ready(struct gtrace_path *path)
+{
+ struct gtrace_path_node *node;
+
+ list_for_each_entry(node, &path->comp_list, head) {
+ if (!node->comp->ready)
+ return false;
+ }
+
+ return true;
+}
+
+struct build_path_walk_priv {
+ struct gtrace_path *path;
+ struct gtrace_component *sink;
+};
+
+static int build_path_walk_fn(struct gtrace_component *comp, bool *stop,
+ struct gtrace_connection *stop_conn,
+ void *priv)
+{
+ struct build_path_walk_priv *ppriv = priv;
+ struct gtrace_path *path = ppriv->path;
+ struct gtrace_path_node *node;
+
+ if ((!ppriv->sink && gtrace_is_sink(comp->pdata)) ||
+ (ppriv->sink && ppriv->sink == comp))
+ *stop = true;
+
+ if (*stop) {
+ node = kzalloc_obj(*node);
+ if (!node)
+ return -ENOMEM;
+ INIT_LIST_HEAD(&node->head);
+ gtrace_get_component(comp);
+ node->comp = comp;
+ node->conn = stop_conn;
+ list_add(&node->head, &path->comp_list);
+ }
+
+ return 0;
+}
+
+static void gtrace_release_path_nodes(struct gtrace_path *path)
+{
+ struct gtrace_path_node *node, *node1;
+
+ list_for_each_entry_safe(node, node1, &path->comp_list, head) {
+ list_del(&node->head);
+ gtrace_put_component(node->comp);
+ kfree(node);
+ }
+}
+
+struct gtrace_path *gtrace_create_path(struct gtrace_component *source,
+ struct gtrace_component *sink,
+ enum gtrace_component_mode mode)
+{
+ struct build_path_walk_priv priv;
+ struct gtrace_path *path;
+ int ret = 0;
+
+ if (!source || mode >= GTRACE_COMPONENT_MODE_MAX) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ path = kzalloc(sizeof(*path), GFP_KERNEL);
+ if (!path) {
+ ret = -ENOMEM;
+ goto err_out;
+ }
+ INIT_LIST_HEAD(&path->comp_list);
+ path->mode = mode;
+ path->trace_id = GTRACE_INVALID_TRACE_ID;
+
+ priv.path = path;
+ priv.sink = sink;
+ ret = gtrace_walk_output_components(source, &priv, build_path_walk_fn);
+ if (ret < 0)
+ goto err_release_path_nodes;
+
+ if (!gtrace_path_ready(path)) {
+ ret = -EOPNOTSUPP;
+ goto err_release_path_nodes;
+ }
+
+ ret = gtrace_assign_trace_id(path);
+ if (ret < 0)
+ goto err_release_path_nodes;
+
+ return path;
+
+err_release_path_nodes:
+ gtrace_release_path_nodes(path);
+ kfree(path);
+err_out:
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(gtrace_create_path);
+
+void gtrace_destroy_path(struct gtrace_path *path)
+{
+ gtrace_unassign_trace_id(path);
+ gtrace_release_path_nodes(path);
+ kfree(path);
+}
+EXPORT_SYMBOL_GPL(gtrace_destroy_path);
+
int __gtrace_register_driver(struct module *owner, struct gtrace_driver *gtdrv)
{
gtdrv->driver.owner = owner;
diff --git a/include/linux/gtrace.h b/include/linux/gtrace.h
index e8ca2bd46e67..b74d4b83b141 100644
--- a/include/linux/gtrace.h
+++ b/include/linux/gtrace.h
@@ -224,6 +224,10 @@ int gtrace_enable_component(struct gtrace_component *comp);
int gtrace_disable_component(struct gtrace_component *comp);
int gtrace_reset_component(struct gtrace_component *comp);
+int gtrace_walk_output_components(struct gtrace_component *comp, void *priv,
+ int (*fn)(struct gtrace_component *comp, bool *stop,
+ struct gtrace_connection *stop_conn,
+ void *priv));
struct gtrace_component *gtrace_cpu_source(unsigned int cpu);
struct gtrace_component *gtrace_register_component(struct gtrace_component_id *id,
@@ -231,6 +235,26 @@ struct gtrace_component *gtrace_register_component(struct gtrace_component_id *i
struct gtrace_platform_data *pdata);
void gtrace_unregister_component(struct gtrace_component *comp);
+/**
+ * struct gtrace_path - Representation of a trace path from source to sink.
+ * @comp_list: List of trace components in the path.
+ * @mode: Usage mode for trace components.
+ * @trace_id: ID of the trace source (typically hart/CPU id).
+ */
+struct gtrace_path {
+ struct list_head comp_list;
+ enum gtrace_component_mode mode;
+ u32 trace_id;
+#define GTRACE_INVALID_TRACE_ID 0
+};
+
+struct gtrace_component *gtrace_path_source(struct gtrace_path *path);
+struct gtrace_component *gtrace_path_sink(struct gtrace_path *path);
+struct gtrace_path *gtrace_create_path(struct gtrace_component *source,
+ struct gtrace_component *sink,
+ enum gtrace_component_mode mode);
+void gtrace_destroy_path(struct gtrace_path *path);
+
/**
* struct gtrace_driver - Representation of a trace driver.
* @id_table: Table to match components handled by the driver.
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 04/14] gtrace: Add functions to create/destroy a trace component path
2026-08-10 15:22 ` [PATCH v5 04/14] gtrace: Add functions to create/destroy a trace component path Mayuresh Chitale
@ 2026-08-10 15:34 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:34 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Asymmetric trace ID lifecycle causes `put_trace_id` to be called on components that did not allocate an ID.
- [High] `gtrace_create_path` returns a path object with an empty component list when the sink is unreachable, leading to out-of-bounds pointer dereferences.
- [High] Missing NULL checks on component driver pointers (`gtdrv`) leads to NULL pointer dereferences.
- [High] Recursive graph traversal crashes on unresolved output connections where the destination component is NULL.
- [High] Unbounded recursive depth-first search (DFS) graph traversal lacks cycle detection, leading to kernel stack overflow.
- [High] Lockless pointer return in `gtrace_cpu_source` leads to a Use-After-Free.
--
--- Patch [4]: [PATCH v5 04/14] gtrace: Add functions to create/destroy a trace component path ---
commit a8e1889600bd5a16b5c7f9f06b462a376b8f943f
Author: Anup Patel <anup.patel@oss.qualcomm.com>
gtrace: Add functions to create/destroy a trace component path
Trace needs to be configured on a chain of trace components which are
connected to each other. These chain of components is also referred
to as trace component path. Add functions to create/destroy a trace
component path which will be later used by gtrace perf support.
> diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
> --- a/drivers/hwtracing/gtrace/gtrace-core.c
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c
> @@ -210,6 +210,53 @@ int gtrace_reset_component(struct gtrace_component *comp)
> }
> EXPORT_SYMBOL_GPL(gtrace_reset_component);
>
> +static int __gtrace_walk_output_components(struct gtrace_component *comp,
> + bool *stop, void *priv,
> + int (*fn)(struct gtrace_component *comp, bool *stop,
> + struct gtrace_connection *stop_conn,
> + void *priv))
> +{
> + struct gtrace_connection *conn, *stop_conn = NULL;
> + struct gtrace_platform_data *pdata = comp->pdata;
[Severity: High]
Does this crash if comp is NULL?
If a destination component is missing or unregistered, conn->dest_comp will be
NULL. When the recursive call is made, comp->pdata is evaluated immediately
without verifying comp, which would cause a NULL pointer dereference.
> + int i, ret;
> +
> + for (i = 0; i < pdata->nr_outconns; i++) {
> + conn = pdata->outconns[i];
> + ret = __gtrace_walk_output_components(conn->dest_comp, stop, priv, fn);
[Severity: High]
Is there any cycle detection in this graph traversal?
If a misconfigured Device Tree provides a cyclical graph layout
(e.g., A -> B -> A), wouldn't this DFS recurse indefinitely and exhaust the
kernel stack?
[ ... ]
> struct gtrace_component *gtrace_cpu_source(unsigned int cpu)
> {
> if (!cpu_present(cpu))
> return NULL;
>
> return per_cpu(gtrace_cpu_source_comp, cpu);
> }
[Severity: High]
Is it safe to return this pointer without taking a reference?
If a caller obtains the component pointer while gtrace_unregister_component()
is invoked concurrently, the entry could be cleared and the component freed.
This would leave the caller with a dangling pointer.
Should this use gtrace_get_component() or RCU read locks before returning?
[ ... ]
> +struct gtrace_component *gtrace_path_source(struct gtrace_path *path)
> +{
> + struct gtrace_path_node *node;
> +
> + node = list_first_entry(&path->comp_list, struct gtrace_path_node, head);
> + return node->comp;
> +}
> +EXPORT_SYMBOL_GPL(gtrace_path_source);
[Severity: High]
Does list_first_entry() resolve to an invalid offset if the list is empty?
If gtrace_create_path() successfully returns a path with an empty list,
calling gtrace_path_source() maps the pointer over the list head itself,
causing node->comp to read garbage memory.
[ ... ]
> +static int gtrace_assign_trace_id(struct gtrace_path *path)
> +{
> + const struct gtrace_driver *gtdrv;
> + struct gtrace_component *comp;
> + struct gtrace_path_node *node;
> + int trace_id;
> +
> + list_for_each_entry(node, &path->comp_list, head) {
> + comp = node->comp;
> + gtdrv = to_gtrace_driver(comp->dev.driver);
> +
> + if (!gtdrv->get_trace_id)
[Severity: High]
What happens if the component device lacks a bound driver?
The to_gtrace_driver() macro handles NULL inputs by returning NULL. If gtdrv
is NULL, evaluating !gtdrv->get_trace_id will dereference a NULL pointer.
> + continue;
> +
> + trace_id = gtdrv->get_trace_id(comp, path->mode);
> + if (trace_id > 0) {
> + path->trace_id = trace_id;
> + return 0;
[Severity: High]
This function stops and returns at the first component that successfully
allocates a trace ID.
[ ... ]
> +static void gtrace_unassign_trace_id(struct gtrace_path *path)
> +{
> + const struct gtrace_driver *gtdrv;
> + struct gtrace_component *comp;
> + struct gtrace_path_node *node;
> +
> + list_for_each_entry(node, &path->comp_list, head) {
> + comp = node->comp;
> + gtdrv = to_gtrace_driver(comp->dev.driver);
> +
> + if (!gtdrv->put_trace_id)
> + continue;
> +
> + gtdrv->put_trace_id(comp, path->mode, path->trace_id);
> + }
> +}
[Severity: High]
Since gtrace_assign_trace_id() only allocates an ID from the first capable
component, wouldn't this loop unconditionally call put_trace_id() for every
component in the path?
This could ask components to free a trace ID they never allocated,
potentially corrupting their allocation bitmaps or driver state.
[ ... ]
> +struct gtrace_path *gtrace_create_path(struct gtrace_component *source,
> + struct gtrace_component *sink,
> + enum gtrace_component_mode mode)
> +{
> + struct build_path_walk_priv priv;
> + struct gtrace_path *path;
> + int ret = 0;
> +
> + if (!source || mode >= GTRACE_COMPONENT_MODE_MAX) {
> + ret = -EINVAL;
> + goto err_out;
> + }
> +
> + path = kzalloc(sizeof(*path), GFP_KERNEL);
> + if (!path) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + INIT_LIST_HEAD(&path->comp_list);
> + path->mode = mode;
> + path->trace_id = GTRACE_INVALID_TRACE_ID;
> +
> + priv.path = path;
> + priv.sink = sink;
> + ret = gtrace_walk_output_components(source, &priv, build_path_walk_fn);
> + if (ret < 0)
> + goto err_release_path_nodes;
[Severity: High]
If the destination sink is unreachable, gtrace_walk_output_components()
completes without adding any nodes to path->comp_list and returns 0.
Since gtrace_path_ready() and gtrace_assign_trace_id() will vacuously pass
for an empty list, won't this return a successful path object with an empty
component list?
This leads to the invalid list head dereferences mentioned earlier in
gtrace_path_source() and gtrace_path_sink().
> +
> + if (!gtrace_path_ready(path)) {
> + ret = -EOPNOTSUPP;
> + goto err_release_path_nodes;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=4
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 05/14] gtrace: Add functions to start/stop tracing on a component path
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (3 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 04/14] gtrace: Add functions to create/destroy a trace component path Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:33 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 06/14] gtrace: Add RISC-V Trace encoder driver Mayuresh Chitale
` (8 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Mayuresh Chitale, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel, Anup Patel
The perf driver framework needs to be able to start / stop all components
in a trace component path during its operation. Add gtrace_path_start()
and gtrace_path_stop() functions for this purpose.
Co-developed-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
---
drivers/hwtracing/gtrace/gtrace-core.c | 44 ++++++++++++++++++++++++++
include/linux/gtrace.h | 6 ++++
2 files changed, 50 insertions(+)
diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
index a89d195ab5b8..6ff1b3087947 100644
--- a/drivers/hwtracing/gtrace/gtrace-core.c
+++ b/drivers/hwtracing/gtrace/gtrace-core.c
@@ -582,6 +582,50 @@ static void gtrace_release_path_nodes(struct gtrace_path *path)
}
}
+int gtrace_path_start(struct gtrace_path *path)
+{
+ const struct gtrace_driver *gtdrv;
+ struct gtrace_component *comp;
+ struct gtrace_path_node *node;
+ int ret;
+
+ list_for_each_entry_reverse(node, &path->comp_list, head) {
+ comp = node->comp;
+ gtdrv = to_gtrace_driver(comp->dev.driver);
+ if (!gtdrv->start)
+ continue;
+
+ ret = gtdrv->start(comp);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(gtrace_path_start);
+
+int gtrace_path_stop(struct gtrace_path *path)
+{
+ const struct gtrace_driver *gtdrv;
+ struct gtrace_component *comp;
+ struct gtrace_path_node *node;
+ int ret;
+
+ list_for_each_entry(node, &path->comp_list, head) {
+ comp = node->comp;
+ gtdrv = to_gtrace_driver(comp->dev.driver);
+ if (!gtdrv->stop)
+ continue;
+
+ ret = gtdrv->stop(comp);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(gtrace_path_stop);
+
struct gtrace_path *gtrace_create_path(struct gtrace_component *source,
struct gtrace_component *sink,
enum gtrace_component_mode mode)
diff --git a/include/linux/gtrace.h b/include/linux/gtrace.h
index b74d4b83b141..78e2ead3db8e 100644
--- a/include/linux/gtrace.h
+++ b/include/linux/gtrace.h
@@ -254,10 +254,14 @@ struct gtrace_path *gtrace_create_path(struct gtrace_component *source,
struct gtrace_component *sink,
enum gtrace_component_mode mode);
void gtrace_destroy_path(struct gtrace_path *path);
+int gtrace_path_start(struct gtrace_path *path);
+int gtrace_path_stop(struct gtrace_path *path);
/**
* struct gtrace_driver - Representation of a trace driver.
* @id_table: Table to match components handled by the driver.
+ * @start: Callback to start tracing.
+ * @stop: Callback to stop tracing.
* @probe: Driver probe() function.
* @remove: Driver remove() function.
* @get_trace_id: Get/allocate a trace ID.
@@ -266,6 +270,8 @@ void gtrace_destroy_path(struct gtrace_path *path);
*/
struct gtrace_driver {
const struct gtrace_component_id *id_table;
+ int (*start)(struct gtrace_component *comp);
+ int (*stop)(struct gtrace_component *comp);
int (*probe)(struct gtrace_component *comp);
void (*remove)(struct gtrace_component *comp);
int (*get_trace_id)(struct gtrace_component *comp,
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 05/14] gtrace: Add functions to start/stop tracing on a component path
2026-08-10 15:22 ` [PATCH v5 05/14] gtrace: Add functions to start/stop tracing on a " Mayuresh Chitale
@ 2026-08-10 15:33 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:33 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing rollback in gtrace_path_start() leaves components in a partially started state on error.
- [High] Early abort in gtrace_path_stop() terminates teardown on the first error, leaving remaining components active.
- [High] Unsafe dereference of gtdrv without NULL check and lockless access to dev->driver can cause a kernel panic or memory corruption.
--
--- Patch [5]: [PATCH v5 05/14] gtrace: Add functions to start/stop tracing on a component path ---
commit 24fa9f699451874fed23109ef92badb57913e17b
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
gtrace: Add functions to start/stop tracing on a component path
The perf driver framework needs to be able to start / stop all components
in a trace component path during its operation. Add gtrace_path_start()
and gtrace_path_stop() functions for this purpose.
> diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
> index a89d195ab5b89..6ff1b30879472 100644
> --- a/drivers/hwtracing/gtrace/gtrace-core.c
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c
> @@ -582,6 +582,50 @@ static void gtrace_release_path_nodes(struct gtrace_path *path)
> }
> }
>
> +int gtrace_path_start(struct gtrace_path *path)
> +{
> + const struct gtrace_driver *gtdrv;
> + struct gtrace_component *comp;
> + struct gtrace_path_node *node;
> + int ret;
> +
> + list_for_each_entry_reverse(node, &path->comp_list, head) {
> + comp = node->comp;
> + gtdrv = to_gtrace_driver(comp->dev.driver);
[Severity: High]
Does this code risk a NULL pointer dereference or memory corruption?
If a driver is unbound concurrently, comp->dev.driver could become NULL.
The to_gtrace_driver() macro evaluates its argument twice locklessly:
#define to_gtrace_driver(__drv) \
((__drv) ? container_of_const((__drv), struct gtrace_driver, driver) : NULL)
If comp->dev.driver is set to NULL between the two evaluations,
container_of_const(NULL) returns a bogus negative offset instead of NULL.
Even if it safely returns NULL, there is no NULL check for gtdrv before
dereferencing it on the next line.
> + if (!gtdrv->start)
> + continue;
> +
> + ret = gtdrv->start(comp);
> + if (ret)
> + return ret;
[Severity: High]
If a component fails to start and returns an error here, do we need to
unwind and stop the components that were already successfully started
in previous loop iterations?
Returning immediately leaves the hardware components in a partially started
state, which could lead to hardware inconsistencies, resource leaks, or
unexpected DMA operations.
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(gtrace_path_start);
> +
> +int gtrace_path_stop(struct gtrace_path *path)
> +{
> + const struct gtrace_driver *gtdrv;
> + struct gtrace_component *comp;
> + struct gtrace_path_node *node;
> + int ret;
> +
> + list_for_each_entry(node, &path->comp_list, head) {
> + comp = node->comp;
> + gtdrv = to_gtrace_driver(comp->dev.driver);
[Severity: High]
This shares the same risk as gtrace_path_start(). A concurrent driver unbind
or a missing driver could result in gtdrv being NULL or an invalid pointer,
leading to a crash when gtdrv->stop is checked below.
> + if (!gtdrv->stop)
> + continue;
> +
> + ret = gtdrv->stop(comp);
> + if (ret)
> + return ret;
[Severity: High]
If a component fails to stop, is it intentional to abort the entire teardown
sequence early?
Returning on the first error prevents the loop from stopping the remaining
components in the path, leaving downstream components actively running and
consuming hardware resources indefinitely. Should teardown paths use a
best-effort approach to disable as many components as possible?
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(gtrace_path_stop);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=5
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 06/14] gtrace: Add RISC-V Trace encoder driver
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (4 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 05/14] gtrace: Add functions to start/stop tracing on a " Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:40 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 07/14] gtrace: Add function to copy into perf AUX buffer Mayuresh Chitale
` (7 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Mayuresh Chitale, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel, Anup Patel
Add initial implementation of RISC-V E-Trace encoder driver. The encoder
is defined in the RISC-V Trace Control Interface specification.
Co-developed-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
---
drivers/hwtracing/gtrace/Kconfig | 13 +++
drivers/hwtracing/gtrace/Makefile | 1 +
drivers/hwtracing/gtrace/rvtrace-encoder.c | 99 ++++++++++++++++++++++
3 files changed, 113 insertions(+)
create mode 100644 drivers/hwtracing/gtrace/rvtrace-encoder.c
diff --git a/drivers/hwtracing/gtrace/Kconfig b/drivers/hwtracing/gtrace/Kconfig
index 04986f216549..e3a621b59913 100644
--- a/drivers/hwtracing/gtrace/Kconfig
+++ b/drivers/hwtracing/gtrace/Kconfig
@@ -28,3 +28,16 @@ config RVTRACE
To compile this driver as a module, choose M here: the module
will be called rvtrace.
+
+config RVTRACE_ENCODER
+ tristate "RISC-V Trace Encoder driver"
+ depends on RVTRACE
+ default y
+ help
+ This driver provides support for the RISC-V trace encoder component
+ which is defined by the RISC-V Trace Control Interface specification.
+ The encoder is bound to a CPU and generates the trace data for that
+ CPU.
+
+ To compile this driver as a module, choose M here: the module
+ will be called rvtrace-encoder.
diff --git a/drivers/hwtracing/gtrace/Makefile b/drivers/hwtracing/gtrace/Makefile
index cf02316c1bbf..10a74f852115 100644
--- a/drivers/hwtracing/gtrace/Makefile
+++ b/drivers/hwtracing/gtrace/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_GTRACE) += gtrace.o
gtrace-y := gtrace-core.o
obj-$(CONFIG_RVTRACE) += rvtrace-platform.o
+obj-$(CONFIG_RVTRACE_ENCODER) += rvtrace-encoder.o
diff --git a/drivers/hwtracing/gtrace/rvtrace-encoder.c b/drivers/hwtracing/gtrace/rvtrace-encoder.c
new file mode 100644
index 000000000000..9ff3413d1aa4
--- /dev/null
+++ b/drivers/hwtracing/gtrace/rvtrace-encoder.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ */
+
+#include <linux/device.h>
+#include <linux/gtrace.h>
+#include <linux/types.h>
+#include "rvtrace.h"
+
+#define RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT 2
+#define RVTRACE_COMPONENT_CTRL_INSTMODE_SHIFT 4
+#define RVTRACE_COMPONENT_CTRL_INSTMODE_OPIT 0x6
+
+static int rvtrace_encoder_start(struct gtrace_component *comp)
+{
+ struct gtrace_platform_data *pdata = comp->pdata;
+ int ret;
+ u32 val;
+
+ ret = gtrace_enable_component(comp);
+ if (ret) {
+ dev_err(&comp->dev, "failed to enable encoder.\n");
+ return ret;
+ }
+
+ /* set mode */
+ val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
+ val |= (RVTRACE_COMPONENT_CTRL_INSTMODE_OPIT << RVTRACE_COMPONENT_CTRL_INSTMODE_SHIFT);
+ gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
+
+ val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
+ val |= BIT(RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT);
+ gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
+ ret = gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
+ RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT, 1,
+ pdata->control_poll_timeout_usecs);
+ if (ret)
+ dev_err(&comp->dev, "failed to enable tracing.\n");
+
+ return ret;
+}
+
+static int rvtrace_encoder_stop(struct gtrace_component *comp)
+{
+ struct gtrace_platform_data *pdata = comp->pdata;
+ int ret;
+ u32 val;
+
+ val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
+ val &= ~BIT(RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT);
+ gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
+ ret = gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
+ RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT, 0,
+ pdata->control_poll_timeout_usecs);
+ if (ret) {
+ dev_err(&comp->dev, "failed to stop tracing.\n");
+ return ret;
+ }
+
+ ret = gtrace_disable_component(comp);
+ if (ret)
+ dev_err(&comp->dev, "failed to disable encoder.\n");
+
+ return ret;
+}
+
+static const struct gtrace_component_id rvtrace_encoder_ids[] = {
+ { .type = GTRACE_RVTRACE_ENCODER,
+ .version = rvtrace_component_mkversion(1, 0), },
+ {},
+};
+
+static struct gtrace_driver rvtrace_encoder_driver = {
+ .id_table = rvtrace_encoder_ids,
+ .start = rvtrace_encoder_start,
+ .stop = rvtrace_encoder_stop,
+ .driver = {
+ .name = "rvtrace-encoder",
+ },
+};
+
+static int __init rvtrace_encoder_init(void)
+{
+ return gtrace_register_driver(&rvtrace_encoder_driver);
+}
+
+static void __exit rvtrace_encoder_exit(void)
+{
+ gtrace_unregister_driver(&rvtrace_encoder_driver);
+}
+
+module_init(rvtrace_encoder_init);
+module_exit(rvtrace_encoder_exit);
+
+/* Module information */
+MODULE_AUTHOR("Mayuresh Chitale");
+MODULE_DESCRIPTION("RISC-V Trace Encoder Driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 06/14] gtrace: Add RISC-V Trace encoder driver
2026-08-10 15:22 ` [PATCH v5 06/14] gtrace: Add RISC-V Trace encoder driver Mayuresh Chitale
@ 2026-08-10 15:40 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:40 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing component disable (resource leak) on error paths in rvtrace_encoder_start() and rvtrace_encoder_stop().
- [High] Hardware state corruption due to setting the multi-bit INSTMODE field without clearing existing bits.
- [Medium] gtrace_poll_bit() evaluates incorrectly and fails immediately when timeout is 0.
--
--- Patch [6]: [PATCH v5 06/14] gtrace: Add RISC-V Trace encoder driver ---
commit a4adb209cac509aff6293c009b2c4a5f828df5f5
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
gtrace: Add RISC-V Trace encoder driver
Add initial implementation of RISC-V E-Trace encoder driver. The encoder
is defined in the RISC-V Trace Control Interface specification.
> diff --git a/drivers/hwtracing/gtrace/rvtrace-encoder.c b/drivers/hwtracing/gtrace/rvtrace-encoder.c
> new file mode 100644
> index 0000000000000..9ff3413d1aa41
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/rvtrace-encoder.c
[ ... ]
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2026 Qualcomm Technologies, Inc.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/gtrace.h>
> +#include <linux/types.h>
> +#include "rvtrace.h"
> +
> +#define RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT 2
> +#define RVTRACE_COMPONENT_CTRL_INSTMODE_SHIFT 4
> +#define RVTRACE_COMPONENT_CTRL_INSTMODE_OPIT 0x6
> +
> +static int rvtrace_encoder_start(struct gtrace_component *comp)
> +{
> + struct gtrace_platform_data *pdata = comp->pdata;
> + int ret;
> + u32 val;
> +
> + ret = gtrace_enable_component(comp);
> + if (ret) {
> + dev_err(&comp->dev, "failed to enable encoder.\n");
> + return ret;
> + }
> +
> + /* set mode */
> + val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> + val |= (RVTRACE_COMPONENT_CTRL_INSTMODE_OPIT << RVTRACE_COMPONENT_CTRL_INSTMODE_SHIFT);
[Severity: High]
Is a bitmask needed before ORing the new mode here?
Since RVTRACE_COMPONENT_CTRL_INSTMODE_OPIT is 0x6 (spanning 3 bits), if the
hardware register already contains non-zero bits from a previous state or the
bootloader (e.g., 0x1), won't the bitwise OR result in a corrupted state
(0x1 | 0x6 = 0x7)?
> + gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
> +
> + val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> + val |= BIT(RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT);
> + gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
> + ret = gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
> + RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT, 1,
> + pdata->control_poll_timeout_usecs);
[Severity: Medium]
Does the gtrace_poll_bit() implementation in gtrace-core.c handle a 0 timeout
correctly?
Looking at gtrace_poll_bit():
while (timeout--) {
val = gtrace_read32(pdata, offset);
If a caller passes a timeout of 0, the postfix decrement causes it to bypass
the loop entirely and immediately return -ETIMEDOUT without reading the
register even once. Should standard polling semantics perform at least one
non-blocking read when timeout is 0?
> + if (ret)
> + dev_err(&comp->dev, "failed to enable tracing.\n");
> +
> + return ret;
[Severity: High]
If gtrace_poll_bit() times out, this returns the error but skips disabling the
component. Will this leak the hardware resources (like clocks or power
domains) that were enabled earlier in gtrace_enable_component()?
> +}
> +
> +static int rvtrace_encoder_stop(struct gtrace_component *comp)
> +{
> + struct gtrace_platform_data *pdata = comp->pdata;
> + int ret;
> + u32 val;
> +
> + val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> + val &= ~BIT(RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT);
> + gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
> + ret = gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
> + RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT, 0,
> + pdata->control_poll_timeout_usecs);
> + if (ret) {
> + dev_err(&comp->dev, "failed to stop tracing.\n");
> + return ret;
> + }
[Severity: High]
If stopping the trace times out, this returns early. Does this intentionally
bypass gtrace_disable_component() and leak the resource, or should it still
attempt to disable the encoder even on a timeout?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=6
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 07/14] gtrace: Add function to copy into perf AUX buffer
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (5 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 06/14] gtrace: Add RISC-V Trace encoder driver Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:42 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 08/14] perf: Add gtrace AUX buffer trace format type Mayuresh Chitale
` (6 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Anup Patel, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel,
Mayuresh Chitale
From: Anup Patel <anup.patel@oss.qualcomm.com>
The RISC-V trace ramsink will need a mechanism to copy trace data
into the perf AUX buffer. Add gtrace_path_copyto_auxbuf() function
and corresponding trace driver callback copyto_auxbuf() for this
purpose.
Co-developed-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
drivers/hwtracing/gtrace/gtrace-core.c | 22 ++++++++++++++++++++++
include/linux/gtrace.h | 24 ++++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
index 6ff1b3087947..53a3679ba69f 100644
--- a/drivers/hwtracing/gtrace/gtrace-core.c
+++ b/drivers/hwtracing/gtrace/gtrace-core.c
@@ -626,6 +626,28 @@ int gtrace_path_stop(struct gtrace_path *path)
}
EXPORT_SYMBOL_GPL(gtrace_path_stop);
+int gtrace_path_copyto_auxbuf(struct gtrace_path *path,
+ struct gtrace_perf_auxbuf *buf,
+ size_t *bytes_copied, u64 *format)
+{
+ const struct gtrace_driver *gtdrv;
+ struct gtrace_component *comp;
+ struct gtrace_path_node *node;
+
+ list_for_each_entry(node, &path->comp_list, head) {
+ comp = node->comp;
+ gtdrv = to_gtrace_driver(comp->dev.driver);
+ if (!gtdrv->copyto_auxbuf)
+ continue;
+
+ *bytes_copied = gtdrv->copyto_auxbuf(comp, buf, format);
+ return 0;
+ }
+
+ return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL_GPL(gtrace_path_copyto_auxbuf);
+
struct gtrace_path *gtrace_create_path(struct gtrace_component *source,
struct gtrace_component *sink,
enum gtrace_component_mode mode)
diff --git a/include/linux/gtrace.h b/include/linux/gtrace.h
index 78e2ead3db8e..bc82814e2738 100644
--- a/include/linux/gtrace.h
+++ b/include/linux/gtrace.h
@@ -257,9 +257,30 @@ void gtrace_destroy_path(struct gtrace_path *path);
int gtrace_path_start(struct gtrace_path *path);
int gtrace_path_stop(struct gtrace_path *path);
+/**
+ * struct gtrace_perf_auxbuf - Representation of the perf AUX buffer.
+ * @length: Size of the AUX buffer.
+ * @nr_pages: Number of pages of the AUX buffer.
+ * @base: Start address of AUX buffer.
+ * @pos: Position in the AUX buffer to commit traced data.
+ */
+struct gtrace_perf_auxbuf {
+ size_t length;
+ int nr_pages;
+ void *base;
+ long pos;
+};
+
+int gtrace_path_copyto_auxbuf(struct gtrace_path *path,
+ struct gtrace_perf_auxbuf *buf,
+ size_t *bytes_copied, u64 *format);
+
/**
* struct gtrace_driver - Representation of a trace driver.
* @id_table: Table to match components handled by the driver.
+ * @copyto_auxbuf: Callback to copy data into perf AUX buffer. The driver
+ * reports the PMU specific trace format of the copied data
+ * via @format (see PERF_AUX_FLAG_PMU_FORMAT_TYPE_MASK).
* @start: Callback to start tracing.
* @stop: Callback to stop tracing.
* @probe: Driver probe() function.
@@ -270,6 +291,9 @@ int gtrace_path_stop(struct gtrace_path *path);
*/
struct gtrace_driver {
const struct gtrace_component_id *id_table;
+ size_t (*copyto_auxbuf)(struct gtrace_component *comp,
+ struct gtrace_perf_auxbuf *buf,
+ u64 *format);
int (*start)(struct gtrace_component *comp);
int (*stop)(struct gtrace_component *comp);
int (*probe)(struct gtrace_component *comp);
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 07/14] gtrace: Add function to copy into perf AUX buffer
2026-08-10 15:22 ` [PATCH v5 07/14] gtrace: Add function to copy into perf AUX buffer Mayuresh Chitale
@ 2026-08-10 15:42 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:42 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added `gtrace_path_copyto_auxbuf` function dereferences `gtdrv` without a NULL check, leading to a kernel panic if the component's driver is unbound.
- [High] Lack of module pinning or device locking in `gtrace_path_copyto_auxbuf` allows concurrent driver unbind or module unloading, leading to a Use-After-Free and kernel panic.
--
--- Patch [7]: [PATCH v5 07/14] gtrace: Add function to copy into perf AUX buffer ---
commit 175afd4c59c793de1c5fef3899d7379cf730bece
Author: Anup Patel <anup.patel@oss.qualcomm.com>
gtrace: Add function to copy into perf AUX buffer
This commit adds gtrace_path_copyto_auxbuf() and a new driver callback
copyto_auxbuf to allow the RISC-V trace ramsink to copy trace data into a
perf AUX buffer.
> diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
> index 6ff1b30879472..53a3679ba69fa 100644
> --- a/drivers/hwtracing/gtrace/gtrace-core.c
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c
> @@ -626,6 +626,28 @@ int gtrace_path_stop(struct gtrace_path *path)
> }
> EXPORT_SYMBOL_GPL(gtrace_path_stop);
>
> +int gtrace_path_copyto_auxbuf(struct gtrace_path *path,
> + struct gtrace_perf_auxbuf *buf,
> + size_t *bytes_copied, u64 *format)
> +{
> + const struct gtrace_driver *gtdrv;
> + struct gtrace_component *comp;
> + struct gtrace_path_node *node;
> +
> + list_for_each_entry(node, &path->comp_list, head) {
> + comp = node->comp;
> + gtdrv = to_gtrace_driver(comp->dev.driver);
> + if (!gtdrv->copyto_auxbuf)
[Severity: High]
Does this sequence risk a null pointer dereference if a component's driver is
unbound?
If a driver is dynamically unbound via sysfs while the perf session is active,
comp->dev.driver becomes null. The to_gtrace_driver() macro will then return
null, causing the evaluation of gtdrv->copyto_auxbuf to panic.
> + continue;
> +
> + *bytes_copied = gtdrv->copyto_auxbuf(comp, buf, format);
[Severity: High]
Can this indirect function call race with a concurrent module unload?
If the component's module is unloaded immediately after the driver pointer is
checked but before the callback executes, jumping to gtdrv->copyto_auxbuf()
could execute freed memory.
Could this be prevented by holding a module reference (try_module_get()) or
taking the device lock (device_lock()) while reading the driver structure and
executing the callback?
> + return 0;
> + }
> +
> + return -EOPNOTSUPP;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=7
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 08/14] perf: Add gtrace AUX buffer trace format type
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (6 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 07/14] gtrace: Add function to copy into perf AUX buffer Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:37 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 09/14] gtrace: Add RISC-V Trace ramsink driver Mayuresh Chitale
` (5 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Mayuresh Chitale, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel
The gtrace PMU can host trace components of different architectures and
trace protocols, so the recorded AUX data needs to carry its format as
well. Define PERF_AUX_FLAG_GTRACE_FORMAT_ETRACE for the RISC-V E-trace
format which the perf tool can use to decode the payload.
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
---
include/uapi/linux/perf_event.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index fd10aa8d697f..ca2d8614a780 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -1311,6 +1311,10 @@ enum perf_callchain_context {
#define PERF_AUX_FLAG_CORESIGHT_FORMAT_CORESIGHT 0x0000 /* Default for backward compatibility */
#define PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW 0x0100 /* Raw format of the source */
+/* gtrace PMU AUX buffer formats */
+#define PERF_AUX_FLAG_GTRACE_FORMAT_UNKNOWN 0x0000
+#define PERF_AUX_FLAG_GTRACE_FORMAT_ETRACE 0x0100 /* RISC-V E-trace format */
+
#define PERF_FLAG_FD_NO_GROUP (1UL << 0)
#define PERF_FLAG_FD_OUTPUT (1UL << 1)
#define PERF_FLAG_PID_CGROUP (1UL << 2) /* pid=cgroup ID, per-CPU mode only */
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 08/14] perf: Add gtrace AUX buffer trace format type
2026-08-10 15:22 ` [PATCH v5 08/14] perf: Add gtrace AUX buffer trace format type Mayuresh Chitale
@ 2026-08-10 15:37 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:37 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The commit modifies the kernel UAPI header `include/uapi/linux/perf_event.h` but fails to mirror these changes to `tools/include/uapi/linux/perf_event.h`.
--
--- Patch [8]: [PATCH v5 08/14] perf: Add gtrace AUX buffer trace format type ---
commit 6637722bfb1d15a6e1471e7ca1494775099312c6
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
perf: Add gtrace AUX buffer trace format type
This commit adds definitions for the gtrace PMU AUX buffer formats to the
perf event UAPI header. It specifically defines the RISC-V E-trace format flag
so the perf tool can decode the recorded payload.
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index fd10aa8d697f2..ca2d8614a7804 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -1311,6 +1311,10 @@ enum perf_callchain_context {
> #define PERF_AUX_FLAG_CORESIGHT_FORMAT_CORESIGHT 0x0000 /* Default for backward compatibility */
> #define PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW 0x0100 /* Raw format of the source */
>
> +/* gtrace PMU AUX buffer formats */
> +#define PERF_AUX_FLAG_GTRACE_FORMAT_UNKNOWN 0x0000
> +#define PERF_AUX_FLAG_GTRACE_FORMAT_ETRACE 0x0100 /* RISC-V E-trace format */
> +
[Severity: Low]
Does this change also need to be mirrored in
tools/include/uapi/linux/perf_event.h?
When building the perf tool, missing the synchronization of UAPI headers
typically triggers a warning that the kernel ABI header differs from the latest
version. Updating the tools directory copy would also ensure the new
PERF_AUX_FLAG_GTRACE_FORMAT_ETRACE macro is accessible to the perf userspace
code within the same tree.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=8
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 09/14] gtrace: Add RISC-V Trace ramsink driver
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (7 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 08/14] perf: Add gtrace AUX buffer trace format type Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:47 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 10/14] riscv: Enable DMA_RESTRICTED_POOL in defconfig Mayuresh Chitale
` (4 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Mayuresh Chitale, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel, Anup Patel
Add initial implementation of RISC-V E-Trace ramsink driver. The ramsink
is defined in the RISC-V Trace Control Interface specification.
Co-developed-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
---
drivers/hwtracing/gtrace/Kconfig | 15 +
drivers/hwtracing/gtrace/Makefile | 1 +
drivers/hwtracing/gtrace/rvtrace-ramsink.c | 320 +++++++++++++++++++++
3 files changed, 336 insertions(+)
create mode 100644 drivers/hwtracing/gtrace/rvtrace-ramsink.c
diff --git a/drivers/hwtracing/gtrace/Kconfig b/drivers/hwtracing/gtrace/Kconfig
index e3a621b59913..de4a3b533591 100644
--- a/drivers/hwtracing/gtrace/Kconfig
+++ b/drivers/hwtracing/gtrace/Kconfig
@@ -41,3 +41,18 @@ config RVTRACE_ENCODER
To compile this driver as a module, choose M here: the module
will be called rvtrace-encoder.
+
+config RVTRACE_RAMSINK
+ tristate "RISC-V Trace Ramsink driver"
+ depends on RVTRACE
+ select DMA_SHARED_BUFFER
+ default y
+ help
+ This driver provides support for the RISC-V trace ramsink component
+ which is defined by the RISC-V Trace Control Interface specification.
+ The ramsink is a trace sink which stores the trace data generated by
+ the upstream components into a system memory buffer. The buffer is
+ then copied to the perf AUX buffer when tracing using the perf tool.
+
+ To compile this driver as a module, choose M here: the module
+ will be called rvtrace-ramsink.
diff --git a/drivers/hwtracing/gtrace/Makefile b/drivers/hwtracing/gtrace/Makefile
index 10a74f852115..3e0c68d1b632 100644
--- a/drivers/hwtracing/gtrace/Makefile
+++ b/drivers/hwtracing/gtrace/Makefile
@@ -5,3 +5,4 @@ gtrace-y := gtrace-core.o
obj-$(CONFIG_RVTRACE) += rvtrace-platform.o
obj-$(CONFIG_RVTRACE_ENCODER) += rvtrace-encoder.o
+obj-$(CONFIG_RVTRACE_RAMSINK) += rvtrace-ramsink.o
diff --git a/drivers/hwtracing/gtrace/rvtrace-ramsink.c b/drivers/hwtracing/gtrace/rvtrace-ramsink.c
new file mode 100644
index 000000000000..7ed25cf364a7
--- /dev/null
+++ b/drivers/hwtracing/gtrace/rvtrace-ramsink.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ */
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/dma-mapping.h>
+#include <linux/gtrace.h>
+#include <linux/types.h>
+#include <linux/sizes.h>
+#include <uapi/linux/perf_event.h>
+#include "rvtrace.h"
+
+#define RVTRACE_RAMSINK_STARTLOW_OFF 0x010
+#define RVTRACE_RAMSINK_STARTHIGH_OFF 0x014
+#define RVTRACE_RAMSINK_LIMITLOW_OFF 0x018
+#define RVTRACE_RAMSINK_LIMITHIGH_OFF 0x01c
+#define RVTRACE_RAMSINK_WPLOW_OFF 0x020
+#define RVTRACE_RAMSINK_WPHIGH_OFF 0x024
+#define RVTRACE_RAMSINK_WPLOW_WRAP 0x1
+#define RVTRACE_RAMSINK_CTRL_MODE_SHIFT 0x4
+#define RVTRACE_RAMSINK_CTRL_STP_WRAP_SHIFT 0x8
+
+enum rvtrace_ramsink_mode {
+ MODE_SRAM,
+ MODE_SMEM
+};
+
+struct rvtrace_ramsink_priv {
+ size_t size;
+ void *va;
+ dma_addr_t start;
+ dma_addr_t end;
+ enum rvtrace_ramsink_mode mode;
+ bool stop_on_wrap;
+ int mem_acc_width;
+ u64 prev_wp;
+};
+
+struct trace_buf {
+ void *base;
+ size_t cur;
+ size_t len;
+};
+
+static int rvtrace_ramsink_start(struct gtrace_component *comp)
+{
+ int ret;
+
+ ret = gtrace_enable_component(comp);
+ if (ret)
+ dev_err(&comp->dev, "failed to start ramsink.\n");
+
+ return ret;
+}
+
+static int rvtrace_ramsink_stop(struct gtrace_component *comp)
+{
+ int ret;
+
+ ret = gtrace_disable_component(comp);
+ if (ret) {
+ dev_err(&comp->dev, "failed to stop ramsink.\n");
+ return ret;
+ }
+
+ return rvtrace_comp_poll_empty(comp);
+}
+
+static void tbuf_to_pbuf_copy(struct trace_buf *src, struct trace_buf *dst, size_t size)
+{
+ int bytes_dst, bytes_src, bytes;
+ void *dst_addr, *src_addr;
+
+ /* If destination cannot hold entire source buffer then write only the latest data. */
+ if (dst->len < size) {
+ src->cur = (src->cur + size - dst->len) % src->len;
+ size = dst->len;
+ }
+
+ while (size) {
+ src_addr = src->base + src->cur;
+ dst_addr = dst->base + dst->cur;
+
+ /* Ensure that there are no OOB memory accesses */
+ if (dst->len - dst->cur < size)
+ bytes_dst = dst->len - dst->cur;
+ else
+ bytes_dst = size;
+
+ if (src->len - src->cur < size)
+ bytes_src = src->len - src->cur;
+ else
+ bytes_src = size;
+ bytes = bytes_dst < bytes_src ? bytes_dst : bytes_src;
+ memcpy(dst_addr, src_addr, bytes);
+ dst->cur = (dst->cur + bytes) % dst->len;
+ src->cur = (src->cur + bytes) % src->len;
+ size -= bytes;
+ }
+}
+
+static size_t rvtrace_ramsink_copyto_auxbuf(struct gtrace_component *comp,
+ struct gtrace_perf_auxbuf *buf,
+ u64 *format)
+{
+ struct rvtrace_ramsink_priv *priv = dev_get_drvdata(&comp->dev);
+ struct trace_buf src, dst;
+ u32 wp_low, wp_high;
+ size_t bytes = 0;
+ bool wrap;
+ u64 wp;
+
+ dst.base = buf->base;
+ dst.len = buf->length;
+ dst.cur = buf->pos;
+ src.base = priv->va;
+ src.len = priv->size;
+ wp_low = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_WPLOW_OFF);
+ wp_high = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_WPHIGH_OFF);
+ wp = (u64)(wp_high) << 32 | wp_low;
+ wrap = wp & RVTRACE_RAMSINK_WPLOW_WRAP;
+ wp &= ~RVTRACE_RAMSINK_WPLOW_WRAP;
+ if (wrap) {
+ gtrace_write32(comp->pdata, lower_32_bits(priv->start),
+ RVTRACE_RAMSINK_WPLOW_OFF);
+ gtrace_write32(comp->pdata, upper_32_bits(priv->start),
+ RVTRACE_RAMSINK_WPHIGH_OFF);
+ src.cur = wp - priv->start;
+ priv->prev_wp = priv->start;
+ /*
+ * There is no way to tell if trRamWp wrapped around more than once. As a
+ * result priv->prev_wp can't be used and the entire buffer must be copied
+ * even though some data might be duplicated.
+ */
+ bytes = priv->size;
+ } else {
+ src.cur = priv->prev_wp - priv->start;
+ bytes = wp - priv->prev_wp;
+ priv->prev_wp = wp;
+ }
+
+ tbuf_to_pbuf_copy(&src, &dst, bytes);
+ dev_dbg(&comp->dev, "Copied %zu bytes\n", bytes);
+ *format = PERF_AUX_FLAG_GTRACE_FORMAT_ETRACE;
+ return bytes;
+}
+
+static int rvtrace_ramsink_setup_buf(struct gtrace_component *comp,
+ struct rvtrace_ramsink_priv *priv)
+{
+ struct device *pdev = comp->pdata->dev;
+ u64 start_min, limit_max, end;
+ u32 low, high;
+ int ret;
+
+ /* Probe min and max values for start and limit registers */
+ gtrace_write32(comp->pdata, 0, RVTRACE_RAMSINK_STARTLOW_OFF);
+ gtrace_write32(comp->pdata, 0, RVTRACE_RAMSINK_STARTHIGH_OFF);
+ low = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_STARTLOW_OFF);
+ high = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_STARTHIGH_OFF);
+ start_min = (u64)(high) << 32 | low;
+
+ gtrace_write32(comp->pdata, 0xffffffff, RVTRACE_RAMSINK_LIMITLOW_OFF);
+ gtrace_write32(comp->pdata, 0xffffffff, RVTRACE_RAMSINK_LIMITHIGH_OFF);
+ low = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_LIMITLOW_OFF);
+ high = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_LIMITHIGH_OFF);
+ limit_max = (u64)(high) << 32 | low;
+
+ /* Set DMA mask based on the maximum allowed limit address */
+ ret = dma_set_mask_and_coherent(pdev, DMA_BIT_MASK(fls64(limit_max)));
+ if (ret)
+ return ret;
+
+ priv->va = dma_alloc_coherent(pdev, priv->size, &priv->start, GFP_KERNEL);
+ if (!priv->va)
+ return -ENOMEM;
+
+ priv->end = priv->start + priv->size;
+ priv->prev_wp = priv->start;
+ if (priv->end <= start_min || priv->start >= limit_max) {
+ dma_free_coherent(pdev, priv->size, priv->va, priv->start);
+ dev_err(&comp->dev, "DMA memory not addressable by device\n");
+ return -EINVAL;
+ }
+
+ /* Setup ram sink start addresses */
+ if (priv->start < start_min) {
+ dev_warn(&comp->dev, "Ramsink start address updated from %pad to %pad\n",
+ &priv->start, &start_min);
+ priv->va += start_min - priv->start;
+ priv->start = start_min;
+ }
+
+ gtrace_write32(comp->pdata, lower_32_bits(priv->start), RVTRACE_RAMSINK_STARTLOW_OFF);
+ gtrace_write32(comp->pdata, upper_32_bits(priv->start), RVTRACE_RAMSINK_STARTHIGH_OFF);
+ gtrace_write32(comp->pdata, lower_32_bits(priv->start), RVTRACE_RAMSINK_WPLOW_OFF);
+ gtrace_write32(comp->pdata, upper_32_bits(priv->start), RVTRACE_RAMSINK_WPHIGH_OFF);
+ /* Setup ram sink limit addresses */
+ if (priv->end > limit_max) {
+ dev_warn(&comp->dev, "Ramsink limit address updated from %pad to %pad\n",
+ &priv->end, &limit_max);
+ priv->end = limit_max;
+ priv->size = priv->end - priv->start;
+ }
+
+ /* Limit address needs to be set to end - mem_access_width to avoid overflow */
+ end = priv->end - priv->mem_acc_width;
+ gtrace_write32(comp->pdata, lower_32_bits(end), RVTRACE_RAMSINK_LIMITLOW_OFF);
+ gtrace_write32(comp->pdata, upper_32_bits(end), RVTRACE_RAMSINK_LIMITHIGH_OFF);
+ low = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_LIMITLOW_OFF);
+ high = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_LIMITHIGH_OFF);
+ end = (u64)(high) << 32 | low;
+ if (end != (priv->end - 4)) {
+ dev_warn(&comp->dev, "Ramsink limit address updated from %pad to %pad\n",
+ &priv->end, &end);
+ priv->end = end;
+ priv->size = priv->end - priv->start;
+ }
+
+ return 0;
+}
+
+static int rvtrace_ramsink_setup(struct gtrace_component *comp)
+{
+ struct rvtrace_ramsink_priv *priv;
+ u32 trram_ctrl;
+ int ret;
+
+ priv = devm_kzalloc(&comp->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ /* Derive RAM sink memory size based on component implementation ID */
+ switch (comp->pdata->impid) {
+ default:
+ priv->size = SZ_1M;
+ priv->mode = MODE_SMEM;
+ priv->stop_on_wrap = false;
+ priv->mem_acc_width = 4;
+ break;
+ }
+
+ trram_ctrl = gtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
+ trram_ctrl |= priv->mode << RVTRACE_RAMSINK_CTRL_MODE_SHIFT;
+ gtrace_write32(comp->pdata, trram_ctrl, RVTRACE_COMPONENT_CTRL_OFFSET);
+ trram_ctrl = gtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
+ dev_dbg(&comp->dev, "mode: %s\n", (trram_ctrl >> RVTRACE_RAMSINK_CTRL_MODE_SHIFT) & 0x1 ?
+ "SMEM" : "SRAM");
+
+ trram_ctrl |= priv->stop_on_wrap << RVTRACE_RAMSINK_CTRL_STP_WRAP_SHIFT;
+ gtrace_write32(comp->pdata, trram_ctrl, RVTRACE_COMPONENT_CTRL_OFFSET);
+
+ ret = rvtrace_ramsink_setup_buf(comp, priv);
+ if (!ret)
+ dev_set_drvdata(&comp->dev, priv);
+
+ return ret;
+}
+
+static void rvtrace_ramsink_cleanup(struct gtrace_component *comp)
+{
+ struct rvtrace_ramsink_priv *priv = dev_get_drvdata(&comp->dev);
+
+ dma_free_coherent(comp->pdata->dev, priv->size, priv->va, priv->start);
+}
+
+static int rvtrace_ramsink_probe(struct gtrace_component *comp)
+{
+ int ret;
+
+ ret = rvtrace_ramsink_setup(comp);
+ if (ret)
+ return dev_err_probe(&comp->dev, ret, "failed to setup ramsink.\n");
+
+ return ret;
+}
+
+static void rvtrace_ramsink_remove(struct gtrace_component *comp)
+{
+ rvtrace_ramsink_cleanup(comp);
+}
+
+static const struct gtrace_component_id rvtrace_ramsink_ids[] = {
+ { .type = GTRACE_RVTRACE_RAMSINK,
+ .version = rvtrace_component_mkversion(1, 0), },
+ {},
+};
+
+static struct gtrace_driver rvtrace_ramsink_driver = {
+ .id_table = rvtrace_ramsink_ids,
+ .copyto_auxbuf = rvtrace_ramsink_copyto_auxbuf,
+ .stop = rvtrace_ramsink_stop,
+ .start = rvtrace_ramsink_start,
+ .probe = rvtrace_ramsink_probe,
+ .remove = rvtrace_ramsink_remove,
+ .driver = {
+ .name = "rvtrace-ramsink",
+ },
+};
+
+static int __init rvtrace_ramsink_init(void)
+{
+ return gtrace_register_driver(&rvtrace_ramsink_driver);
+}
+
+static void __exit rvtrace_ramsink_exit(void)
+{
+ gtrace_unregister_driver(&rvtrace_ramsink_driver);
+}
+
+module_init(rvtrace_ramsink_init);
+module_exit(rvtrace_ramsink_exit);
+
+/* Module information */
+MODULE_AUTHOR("Mayuresh Chitale");
+MODULE_DESCRIPTION("RISC-V E-Trace Ramsink Driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 09/14] gtrace: Add RISC-V Trace ramsink driver
2026-08-10 15:22 ` [PATCH v5 09/14] gtrace: Add RISC-V Trace ramsink driver Mayuresh Chitale
@ 2026-08-10 15:47 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:47 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Modifying DMA pointers and size after allocation causes a DMA API violation upon freeing and out-of-bounds memory accesses.
- [High] Missing bounds check on the hardware write pointer (`wp`) allows arbitrary out-of-bounds kernel memory reads.
- [High] Trusting hardware limit registers unconditionally overwrites `priv->size`, allowing hardware to inflate buffer bounds.
- [High] Trusting hardware limit registers can result in a zero-sized buffer, triggering a division-by-zero panic.
- [High] Using signed `int` for lengths in `tbuf_to_pbuf_copy` causes an integer overflow when bounds exceed 2GB.
- [Medium] Bitwise OR operations update configuration fields without clearing previous bit values, leaving old configurations active.
- [Medium] The driver does not export its component ID table, preventing automatic module loading.
- [Medium] Buffer truncation logic error when hardware clamps the limit address.
- [Medium] Missing synchronization protecting shared ramsink state in `copyto_auxbuf`.
- [Low] Format String Type Mismatch using `%pad` for `u64` variables.
--
--- Patch [9]: [PATCH v5 09/14] gtrace: Add RISC-V Trace ramsink driver ---
commit 07a5e011c3c5f12851ee9400f1bd383cb0538970
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
gtrace: Add RISC-V Trace ramsink driver
This patch adds the initial implementation of the RISC-V E-Trace ramsink
driver. The ramsink stores trace data into a system memory buffer for perf
AUX buffer usage.
> diff --git a/drivers/hwtracing/gtrace/rvtrace-ramsink.c b/drivers/hwtracing/gtrace/rvtrace-ramsink.c
> new file mode 100644
> index 0000000000000..7ed25cf364a71
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/rvtrace-ramsink.c
[ ... ]
> +static void tbuf_to_pbuf_copy(struct trace_buf *src, struct trace_buf *dst, size_t size)
> +{
> + int bytes_dst, bytes_src, bytes;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this code risk integer overflow if the lengths exceed 2GB?
The variables bytes, bytes_dst, and bytes_src are declared as signed int,
while the lengths they represent (like dst->len and src->len) are size_t
and can legally exceed 2GB. If dst->len - dst->cur exceeds 2GB, the
subtraction overflows the 32-bit int into a negative number.
Could this negative number pass the bytes_dst < bytes_src check and be
supplied to memcpy(), which implicitly casts it to a huge size_t value
causing a fatal out-of-bounds copy?
> + void *dst_addr, *src_addr;
> +
> + /* If destination cannot hold entire source buffer then write only the latest data. */
> + if (dst->len < size) {
> + src->cur = (src->cur + size - dst->len) % src->len;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Could this result in a division-by-zero panic?
If the hardware limit register readback exactly equals priv->start,
rvtrace_ramsink_setup_buf() will update priv->end = priv->start, making
priv->size = 0.
Upon the next perf AUX buffer read, rvtrace_ramsink_copyto_auxbuf() will pass
priv->size (0) as src.len to tbuf_to_pbuf_copy(). This would cause the modulo
operation here to divide by zero.
> + size = dst->len;
> + }
> +
> + while (size) {
[ ... ]
> + if (src->len - src->cur < size)
> + bytes_src = src->len - src->cur;
> + else
> + bytes_src = size;
> + bytes = bytes_dst < bytes_src ? bytes_dst : bytes_src;
> + memcpy(dst_addr, src_addr, bytes);
> + dst->cur = (dst->cur + bytes) % dst->len;
> + src->cur = (src->cur + bytes) % src->len;
> + size -= bytes;
> + }
> +}
> +
> +static size_t rvtrace_ramsink_copyto_auxbuf(struct gtrace_component *comp,
> + struct gtrace_perf_auxbuf *buf,
> + u64 *format)
> +{
> + struct rvtrace_ramsink_priv *priv = dev_get_drvdata(&comp->dev);
> + struct trace_buf src, dst;
> + u32 wp_low, wp_high;
> + size_t bytes = 0;
> + bool wrap;
> + u64 wp;
> +
> + dst.base = buf->base;
> + dst.len = buf->length;
> + dst.cur = buf->pos;
> + src.base = priv->va;
> + src.len = priv->size;
> + wp_low = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_WPLOW_OFF);
> + wp_high = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_WPHIGH_OFF);
> + wp = (u64)(wp_high) << 32 | wp_low;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this implicitly trust the hardware read value without bounds validation?
If a faulty hardware component returns a value outside the physical buffer
bounds, or if concurrent accesses cause a race, the calculations like
bytes = wp - priv->prev_wp could underflow or overflow.
This would allow tbuf_to_pbuf_copy() to copy arbitrary out-of-bounds kernel
memory directly into the perf AUX buffer, leaking kernel memory to userspace.
> + wrap = wp & RVTRACE_RAMSINK_WPLOW_WRAP;
> + wp &= ~RVTRACE_RAMSINK_WPLOW_WRAP;
> + if (wrap) {
[ ... ]
> + } else {
> + src.cur = priv->prev_wp - priv->start;
> + bytes = wp - priv->prev_wp;
> + priv->prev_wp = wp;
^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Is there missing synchronization protecting this shared ramsink state?
If rvtrace_ramsink_copyto_auxbuf() is executed concurrently, data races
on priv->prev_wp could result in length underflows and corrupted hardware
write pointers. Is a spinlock necessary to protect this component's
shared state?
> + }
> +
> + tbuf_to_pbuf_copy(&src, &dst, bytes);
> + dev_dbg(&comp->dev, "Copied %zu bytes\n", bytes);
> + *format = PERF_AUX_FLAG_GTRACE_FORMAT_ETRACE;
> + return bytes;
> +}
> +
> +static int rvtrace_ramsink_setup_buf(struct gtrace_component *comp,
> + struct rvtrace_ramsink_priv *priv)
> +{
> + struct device *pdev = comp->pdata->dev;
> + u64 start_min, limit_max, end;
> + u32 low, high;
> + int ret;
[ ... ]
> + priv->va = dma_alloc_coherent(pdev, priv->size, &priv->start, GFP_KERNEL);
> + if (!priv->va)
> + return -ENOMEM;
> +
> + priv->end = priv->start + priv->size;
> + priv->prev_wp = priv->start;
> + if (priv->end <= start_min || priv->start >= limit_max) {
> + dma_free_coherent(pdev, priv->size, priv->va, priv->start);
> + dev_err(&comp->dev, "DMA memory not addressable by device\n");
> + return -EINVAL;
> + }
> +
> + /* Setup ram sink start addresses */
> + if (priv->start < start_min) {
> + dev_warn(&comp->dev, "Ramsink start address updated from %pad to %pad\n",
> + &priv->start, &start_min);
^^^^^^^^^^
[Severity: Low]
Does this code use an incorrect format string for start_min?
start_min is of type u64, but %pad requires a pointer to dma_addr_t or
phys_addr_t. On 32-bit architectures where dma_addr_t is 32-bit, printk
will dereference the pointer as a 32-bit type, reading only half of the
u64 variable, resulting in incorrect log output.
> + priv->va += start_min - priv->start;
> + priv->start = start_min;
^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Can modifying these DMA pointers cause a DMA API violation later?
When rvtrace_ramsink_cleanup() later passes these modified pointers to
dma_free_coherent(), it violates the DMA API, which strictly requires the
exact virtual and physical addresses originally returned by
dma_alloc_coherent().
This can lead to a kernel panic on driver unbind. Also, because priv->size
is not reduced, operations accessing priv->start + priv->size will overflow
the allocated DMA buffer bounds.
> + }
> +
> + gtrace_write32(comp->pdata, lower_32_bits(priv->start), RVTRACE_RAMSINK_STARTLOW_OFF);
> + gtrace_write32(comp->pdata, upper_32_bits(priv->start), RVTRACE_RAMSINK_STARTHIGH_OFF);
> + gtrace_write32(comp->pdata, lower_32_bits(priv->start), RVTRACE_RAMSINK_WPLOW_OFF);
> + gtrace_write32(comp->pdata, upper_32_bits(priv->start), RVTRACE_RAMSINK_WPHIGH_OFF);
> + /* Setup ram sink limit addresses */
> + if (priv->end > limit_max) {
> + dev_warn(&comp->dev, "Ramsink limit address updated from %pad to %pad\n",
> + &priv->end, &limit_max);
> + priv->end = limit_max;
> + priv->size = priv->end - priv->start;
> + }
> +
> + /* Limit address needs to be set to end - mem_access_width to avoid overflow */
> + end = priv->end - priv->mem_acc_width;
> + gtrace_write32(comp->pdata, lower_32_bits(end), RVTRACE_RAMSINK_LIMITLOW_OFF);
> + gtrace_write32(comp->pdata, upper_32_bits(end), RVTRACE_RAMSINK_LIMITHIGH_OFF);
> + low = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_LIMITLOW_OFF);
> + high = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_LIMITHIGH_OFF);
> + end = (u64)(high) << 32 | low;
> + if (end != (priv->end - 4)) {
> + dev_warn(&comp->dev, "Ramsink limit address updated from %pad to %pad\n",
> + &priv->end, &end);
^^^^
[Severity: Low]
Does this code use an incorrect format string for end?
end is of type u64, but %pad requires a pointer to dma_addr_t or
phys_addr_t.
> + priv->end = end;
> + priv->size = priv->end - priv->start;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this trust the hardware limit registers to unconditionally overwrite
priv->size?
If the hardware returns a value much larger than the original 1MB DMA
allocation, the software size limits are inflated. Subsequent trace data
copies will read past the 1MB physical buffer, leaking surrounding kernel
memory into the user perf stream.
[Severity: Medium]
Additionally, does this introduce a buffer truncation logic error?
The driver subtracted 4 (priv->mem_acc_width) to avoid hardware wrap
overflow. If the hardware clamps this limit and returns a different value,
shrinking priv->end to the raw hardware return value recalculates
priv->size without adding those 4 bytes back.
This can cause the software ring buffer math to wrap before reading the
final 4 bytes of trace data written by the hardware, leading to data loss.
> + }
> +
> + return 0;
> +}
> +
> +static int rvtrace_ramsink_setup(struct gtrace_component *comp)
> +{
> + struct rvtrace_ramsink_priv *priv;
> + u32 trram_ctrl;
> + int ret;
[ ... ]
> + trram_ctrl = gtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> + trram_ctrl |= priv->mode << RVTRACE_RAMSINK_CTRL_MODE_SHIFT;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Should the existing bits be cleared before ORing the new values?
Because the driver doesn't clear the existing bits, a previously set bit
might remain set even if the new mode evaluates to 0, leaving the hardware
improperly configured.
> + gtrace_write32(comp->pdata, trram_ctrl, RVTRACE_COMPONENT_CTRL_OFFSET);
> + trram_ctrl = gtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> + dev_dbg(&comp->dev, "mode: %s\n", (trram_ctrl >> RVTRACE_RAMSINK_CTRL_MODE_SHIFT) & 0x1 ?
> + "SMEM" : "SRAM");
> +
> + trram_ctrl |= priv->stop_on_wrap << RVTRACE_RAMSINK_CTRL_STP_WRAP_SHIFT;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Similarly, does the stop-on-wrap bitfield need to be cleared before
being updated?
> + gtrace_write32(comp->pdata, trram_ctrl, RVTRACE_COMPONENT_CTRL_OFFSET);
> +
> + ret = rvtrace_ramsink_setup_buf(comp, priv);
> + if (!ret)
> + dev_set_drvdata(&comp->dev, priv);
> +
> + return ret;
> +}
> +
> +static void rvtrace_ramsink_cleanup(struct gtrace_component *comp)
> +{
> + struct rvtrace_ramsink_priv *priv = dev_get_drvdata(&comp->dev);
> +
> + dma_free_coherent(comp->pdata->dev, priv->size, priv->va, priv->start);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
As mentioned earlier, if priv->va, priv->start, or priv->size were modified
in rvtrace_ramsink_setup_buf(), won't this pass illegally shifted pointers
to the DMA API and cause a kernel panic?
> +}
[ ... ]
> +static const struct gtrace_component_id rvtrace_ramsink_ids[] = {
> + { .type = GTRACE_RVTRACE_RAMSINK,
> + .version = rvtrace_component_mkversion(1, 0), },
> + {},
> +};
^^
[Severity: Medium]
Should this ID table be exported?
Without a MODULE_DEVICE_TABLE declaration, the driver will not be
autoloaded when compatible hardware is present on the system.
> +
> +static struct gtrace_driver rvtrace_ramsink_driver = {
> + .id_table = rvtrace_ramsink_ids,
> + .copyto_auxbuf = rvtrace_ramsink_copyto_auxbuf,
> + .stop = rvtrace_ramsink_stop,
> + .start = rvtrace_ramsink_start,
> + .probe = rvtrace_ramsink_probe,
> + .remove = rvtrace_ramsink_remove,
> + .driver = {
> + .name = "rvtrace-ramsink",
> + },
> +};
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=9
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 10/14] riscv: Enable DMA_RESTRICTED_POOL in defconfig
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (8 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 09/14] gtrace: Add RISC-V Trace ramsink driver Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:22 ` [PATCH v5 11/14] gtrace: Add perf driver for tracing using perf tool Mayuresh Chitale
` (3 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Anup Patel, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel
From: Anup Patel <anup.patel@oss.qualcomm.com>
The RISC-V ramsink trace component may have implementation specific
restrictions such that the component can only write trace data in
particular parts of DRAM.
Enable DMA_RESTRICTED_POOL in the defconfig so that dma_alloc_*()
and dma_free_*() APIs work for devices with DMA address restrictions.
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
arch/riscv/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index ed605b5e3162..57692fcdbb89 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -304,6 +304,7 @@ CONFIG_SECURITY_APPARMOR=y
CONFIG_DEFAULT_SECURITY_DAC=y
CONFIG_CRYPTO_USER_API_HASH=y
CONFIG_CRYPTO_DEV_VIRTIO=y
+CONFIG_DMA_RESTRICTED_POOL=y
CONFIG_PRINTK_TIME=y
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_FS=y
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 11/14] gtrace: Add perf driver for tracing using perf tool
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (9 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 10/14] riscv: Enable DMA_RESTRICTED_POOL in defconfig Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:48 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 12/14] perf tools: Add RISC-V trace PMU record capabilities Mayuresh Chitale
` (2 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Mayuresh Chitale, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel, Anup Patel
Add perf driver for the gtrace similar to ARM Coresight and Hisilicon
PTT drivers. The driver adds 'gtrace' event descriptor which can be used
by the perf tool to record the trace data. The actual data format depends
on type of encoder device and optionally the PMU specific trace format
type.
Co-developed-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
---
drivers/hwtracing/gtrace/Kconfig | 1 +
drivers/hwtracing/gtrace/Makefile | 2 +-
drivers/hwtracing/gtrace/gtrace-core.c | 15 +-
drivers/hwtracing/gtrace/gtrace-perf.c | 347 +++++++++++++++++++++++++
include/linux/gtrace.h | 3 +
5 files changed, 366 insertions(+), 2 deletions(-)
create mode 100644 drivers/hwtracing/gtrace/gtrace-perf.c
diff --git a/drivers/hwtracing/gtrace/Kconfig b/drivers/hwtracing/gtrace/Kconfig
index de4a3b533591..e5b62d65e2fd 100644
--- a/drivers/hwtracing/gtrace/Kconfig
+++ b/drivers/hwtracing/gtrace/Kconfig
@@ -3,6 +3,7 @@
menuconfig GTRACE
tristate "Generic Hardware Trace Support"
depends on OF
+ select PERF_EVENTS
help
This framework provides an architecture-neutral kernel interface
for hardware trace drivers. It builds a topological view of the
diff --git a/drivers/hwtracing/gtrace/Makefile b/drivers/hwtracing/gtrace/Makefile
index 3e0c68d1b632..aa044fdc787b 100644
--- a/drivers/hwtracing/gtrace/Makefile
+++ b/drivers/hwtracing/gtrace/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_GTRACE) += gtrace.o
-gtrace-y := gtrace-core.o
+gtrace-y := gtrace-core.o gtrace-perf.o
obj-$(CONFIG_RVTRACE) += rvtrace-platform.o
obj-$(CONFIG_RVTRACE_ENCODER) += rvtrace-encoder.o
diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
index 53a3679ba69f..102251f6a5a7 100644
--- a/drivers/hwtracing/gtrace/gtrace-core.c
+++ b/drivers/hwtracing/gtrace/gtrace-core.c
@@ -714,13 +714,26 @@ EXPORT_SYMBOL_GPL(__gtrace_register_driver);
static int __init gtrace_init(void)
{
+ int ret;
+
gtrace_init_type_idx();
- return bus_register(>race_bustype);
+ ret = bus_register(>race_bustype);
+ if (ret)
+ return ret;
+
+ ret = gtrace_perf_init();
+ if (ret) {
+ bus_unregister(>race_bustype);
+ return ret;
+ }
+
+ return 0;
}
static void __exit gtrace_exit(void)
{
+ gtrace_perf_exit();
bus_unregister(>race_bustype);
}
diff --git a/drivers/hwtracing/gtrace/gtrace-perf.c b/drivers/hwtracing/gtrace/gtrace-perf.c
new file mode 100644
index 000000000000..ed9155bbbdb7
--- /dev/null
+++ b/drivers/hwtracing/gtrace/gtrace-perf.c
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/cpumask.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/init.h>
+#include <linux/perf_event.h>
+#include <linux/vmalloc.h>
+#include <linux/percpu-defs.h>
+#include <linux/slab.h>
+#include <linux/stringhash.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+#include <linux/gtrace.h>
+
+#define GTRACE_PMU_NAME "gtrace"
+static struct pmu gtrace_pmu;
+static DEFINE_SPINLOCK(perf_buf_lock);
+
+/**
+ * struct gtrace_event_data - generic hardware trace perf event data
+ * @work: Handle to free allocated memory outside IRQ context.
+ * @mask: Hold the CPU(s) this event was set for.
+ * @aux_hwid_done: Whether a CPU has emitted the TraceID packet or not.
+ * @path: An array of path, each slot for one CPU.
+ * @buf: Aux buffer / pages allocated by perf framework.
+ */
+struct gtrace_event_data {
+ struct work_struct work;
+ cpumask_t mask;
+ cpumask_t aux_hwid_done;
+ struct gtrace_path * __percpu *path;
+ struct gtrace_perf_auxbuf buf;
+};
+
+struct gtrace_ctxt {
+ struct perf_output_handle handle;
+ struct gtrace_event_data *event_data;
+};
+
+static DEFINE_PER_CPU(struct gtrace_ctxt, gtrace_ctxt);
+
+static void *alloc_event_data(int cpu)
+{
+ struct gtrace_event_data *event_data;
+ cpumask_t *mask;
+
+ event_data = kzalloc_obj(*event_data);
+ if (!event_data)
+ return NULL;
+
+ /* Update mask as per selected CPUs */
+ mask = &event_data->mask;
+ if (cpu != -1)
+ cpumask_set_cpu(cpu, mask);
+ else
+ cpumask_copy(mask, cpu_present_mask);
+
+ event_data->path = alloc_percpu(struct gtrace_path *);
+ return event_data;
+}
+
+static void gtrace_free_aux(void *data)
+{
+ struct gtrace_event_data *event_data = data;
+
+ schedule_work(&event_data->work);
+}
+
+static struct gtrace_path **gtrace_event_cpu_path_ptr(struct gtrace_event_data *data,
+ int cpu)
+{
+ return per_cpu_ptr(data->path, cpu);
+}
+
+static void free_event_data(struct work_struct *work)
+{
+ struct gtrace_event_data *event_data;
+ struct gtrace_path *path;
+ cpumask_t *mask;
+ int cpu;
+
+ event_data = container_of(work, struct gtrace_event_data, work);
+ mask = &event_data->mask;
+ for_each_cpu(cpu, mask) {
+ path = *gtrace_event_cpu_path_ptr(event_data, cpu);
+ gtrace_destroy_path(path);
+ }
+ free_percpu(event_data->path);
+ kfree(event_data);
+}
+
+static void *gtrace_setup_aux(struct perf_event *event, void **pages,
+ int nr_pages, bool overwrite)
+{
+ struct gtrace_event_data *event_data = NULL;
+ struct page **pagelist;
+ int cpu = event->cpu, i;
+ cpumask_t *mask;
+
+ event_data = alloc_event_data(cpu);
+ if (!event_data)
+ return NULL;
+
+ INIT_WORK(&event_data->work, free_event_data);
+ mask = &event_data->mask;
+ /*
+ * Create the path for each CPU in the mask. In case of any failure skip the CPU
+ */
+ for_each_cpu(cpu, mask) {
+ struct gtrace_component *src;
+ struct gtrace_path *path;
+
+ src = gtrace_cpu_source(cpu);
+ if (!src)
+ continue;
+
+ path = gtrace_create_path(src, NULL, GTRACE_COMPONENT_MODE_PERF);
+ if (!path)
+ continue;
+
+ *gtrace_event_cpu_path_ptr(event_data, cpu) = path;
+ }
+
+ /* If we don't have any CPUs ready for tracing, abort */
+ cpu = cpumask_first(&event_data->mask);
+ if (cpu >= nr_cpu_ids)
+ goto err;
+
+ pagelist = kcalloc(nr_pages, sizeof(*pagelist), GFP_KERNEL);
+ if (!pagelist)
+ goto err;
+
+ for (i = 0; i < nr_pages; i++)
+ pagelist[i] = virt_to_page(pages[i]);
+
+ event_data->buf.base = vmap(pagelist, nr_pages, VM_MAP, PAGE_KERNEL);
+ if (!event_data->buf.base) {
+ kfree(pagelist);
+ goto err;
+ }
+
+ event_data->buf.nr_pages = nr_pages;
+ event_data->buf.length = nr_pages * PAGE_SIZE;
+ event_data->buf.pos = 0;
+ return event_data;
+err:
+ gtrace_free_aux(event_data);
+ return NULL;
+}
+
+static void gtrace_event_read(struct perf_event *event)
+{
+}
+
+static void gtrace_event_destroy(struct perf_event *event)
+{
+}
+
+static int gtrace_event_init(struct perf_event *event)
+{
+ if (event->attr.type != gtrace_pmu.type)
+ return -EINVAL;
+
+ event->destroy = gtrace_event_destroy;
+ return 0;
+}
+
+static void gtrace_event_start(struct perf_event *event, int flags)
+{
+ struct gtrace_ctxt *ctxt = this_cpu_ptr(>race_ctxt);
+ struct perf_output_handle *handle = &ctxt->handle;
+ struct gtrace_event_data *event_data;
+ int cpu = smp_processor_id();
+ struct gtrace_path *path;
+
+ if (WARN_ON(ctxt->event_data))
+ goto fail;
+
+ /*
+ * Deal with the ring buffer API and get a handle on the
+ * session's information.
+ */
+ event_data = perf_aux_output_begin(handle, event);
+ if (!event_data)
+ goto fail;
+
+ if (!cpumask_test_cpu(cpu, &event_data->mask))
+ goto out;
+
+ event_data->buf.pos = handle->head % event_data->buf.length;
+ path = *gtrace_event_cpu_path_ptr(event_data, cpu);
+ if (!path) {
+ pr_err("Error. Path not found\n");
+ return;
+ }
+
+ if (gtrace_path_start(path)) {
+ pr_err("Error. Tracing not started\n");
+ return;
+ }
+
+ /*
+ * output cpu / trace ID in perf record, once for the lifetime
+ * of the event.
+ */
+ if (!cpumask_test_cpu(cpu, &event_data->aux_hwid_done)) {
+ cpumask_set_cpu(cpu, &event_data->aux_hwid_done);
+ perf_report_aux_output_id(event, cpu);
+ }
+
+out:
+ /* Tell the perf core the event is alive */
+ event->hw.state = 0;
+ ctxt->event_data = event_data;
+ return;
+fail:
+ event->hw.state = PERF_HES_STOPPED;
+}
+
+static void gtrace_event_stop(struct perf_event *event, int mode)
+{
+ struct gtrace_ctxt *ctxt = this_cpu_ptr(>race_ctxt);
+ struct perf_output_handle *handle = &ctxt->handle;
+ struct gtrace_event_data *event_data;
+ int ret, cpu = smp_processor_id();
+ struct gtrace_path *path;
+ size_t size;
+ u64 format;
+
+ if (event->hw.state == PERF_HES_STOPPED)
+ return;
+
+ if (handle->event &&
+ WARN_ON(perf_get_aux(handle) != ctxt->event_data))
+ return;
+
+ event_data = ctxt->event_data;
+ ctxt->event_data = NULL;
+
+ if (WARN_ON(!event_data))
+ return;
+
+ if (handle->event && (mode & PERF_EF_UPDATE) && !cpumask_test_cpu(cpu, &event_data->mask)) {
+ event->hw.state = PERF_HES_STOPPED;
+ perf_aux_output_end(handle, 0);
+ return;
+ }
+
+ /* stop tracing */
+ path = *gtrace_event_cpu_path_ptr(event_data, cpu);
+ if (!path) {
+ pr_err("Error. Path not found\n");
+ return;
+ }
+
+ if (gtrace_path_stop(path)) {
+ pr_err("Error. Tracing not stopped\n");
+ return;
+ }
+
+ event->hw.state = PERF_HES_STOPPED;
+ if (handle->event && (mode & PERF_EF_UPDATE)) {
+ if (WARN_ON_ONCE(handle->event != event))
+ return;
+ spin_lock(&perf_buf_lock);
+ ret = gtrace_path_copyto_auxbuf(path, &event_data->buf, &size, &format);
+ spin_unlock(&perf_buf_lock);
+ WARN_ON_ONCE(ret);
+ if (READ_ONCE(handle->event)) {
+ /* Tag the AUX data with the format reported by the sink. */
+ perf_aux_output_flag(handle, format);
+ if (size > handle->size) {
+ size = handle->size;
+ perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+ }
+ perf_aux_output_end(handle, size);
+ } else
+ WARN_ON(size);
+ }
+}
+
+static int gtrace_event_add(struct perf_event *event, int mode)
+{
+ struct hw_perf_event *hwc = &event->hw;
+ int ret = 0;
+
+ if (mode & PERF_EF_START) {
+ gtrace_event_start(event, 0);
+ if (hwc->state & PERF_HES_STOPPED)
+ ret = -EINVAL;
+ } else {
+ hwc->state = PERF_HES_STOPPED;
+ }
+
+ return ret;
+}
+
+static void gtrace_event_del(struct perf_event *event, int mode)
+{
+ gtrace_event_stop(event, PERF_EF_UPDATE);
+}
+
+PMU_FORMAT_ATTR(event, "config:0-0");
+
+static struct attribute *gtrace_pmu_formats_attr[] = {
+ &format_attr_event.attr,
+ NULL,
+};
+
+static struct attribute_group gtrace_pmu_format_group = {
+ .name = "format",
+ .attrs = gtrace_pmu_formats_attr,
+};
+
+static const struct attribute_group *gtrace_pmu_attr_groups[] = {
+ >race_pmu_format_group,
+ NULL,
+};
+
+int __init gtrace_perf_init(void)
+{
+ gtrace_pmu.capabilities = (PERF_PMU_CAP_EXCLUSIVE | PERF_PMU_CAP_ITRACE);
+ gtrace_pmu.attr_groups = gtrace_pmu_attr_groups;
+ gtrace_pmu.task_ctx_nr = perf_sw_context;
+ gtrace_pmu.read = gtrace_event_read;
+ gtrace_pmu.event_init = gtrace_event_init;
+ gtrace_pmu.setup_aux = gtrace_setup_aux;
+ gtrace_pmu.free_aux = gtrace_free_aux;
+ gtrace_pmu.start = gtrace_event_start;
+ gtrace_pmu.stop = gtrace_event_stop;
+ gtrace_pmu.add = gtrace_event_add;
+ gtrace_pmu.del = gtrace_event_del;
+ gtrace_pmu.module = THIS_MODULE;
+
+ return perf_pmu_register(>race_pmu, GTRACE_PMU_NAME, -1);
+}
+
+void __exit gtrace_perf_exit(void)
+{
+ perf_pmu_unregister(>race_pmu);
+}
diff --git a/include/linux/gtrace.h b/include/linux/gtrace.h
index bc82814e2738..439e2f091d4d 100644
--- a/include/linux/gtrace.h
+++ b/include/linux/gtrace.h
@@ -317,4 +317,7 @@ static inline void gtrace_unregister_driver(struct gtrace_driver *gtdrv)
driver_unregister(>drv->driver);
}
+int gtrace_perf_init(void);
+void gtrace_perf_exit(void);
+
#endif /* __LINUX_GTRACE_H__ */
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 11/14] gtrace: Add perf driver for tracing using perf tool
2026-08-10 15:22 ` [PATCH v5 11/14] gtrace: Add perf driver for tracing using perf tool Mayuresh Chitale
@ 2026-08-10 15:48 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:48 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Unchecked return value of alloc_percpu() leads to NULL pointer dereference on allocation failure.
- [High] gtrace_setup_aux() checks for !path instead of IS_ERR(path), storing error pointers that cause crashes later.
- [High] Skipped CPUs are not cleared from event_data->mask, causing validation bypass and NULL pointer dereferences in cleanup.
- [High] The virtual memory mapped via vmap() for the AUX buffer is never unmapped, permanently leaking vmalloc space.
- [High] Error paths in gtrace_event_start() and gtrace_event_stop() return directly without calling perf_aux_output_end(), leaking the active AUX buffer handle.
- [High] gtrace_event_stop() passes uninitialized size and format variables to the perf core if gtrace_path_copyto_auxbuf() fails.
- [High] gtrace_path_start and gtrace_path_stop abort on the first component error without rolling back already-modified components.
- [High] Lock ordering violation: gtrace_event_stop() acquires a spinlock_t inside the PMU stop callback, violating wait-context nesting.
- [High] Module unload race in gtrace_perf_exit() with pending asynchronous work leads to use-after-free of module text.
- [Medium] The dynamically allocated pagelist array is permanently leaked on the success path of gtrace_setup_aux().
--
--- Patch [11]: [PATCH v5 11/14] gtrace: Add perf driver for tracing using perf tool ---
commit 7444340c1bfa224d9b35f7fb63e614992d6414e2
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
gtrace: Add perf driver for tracing using perf tool
Add perf driver for the gtrace similar to ARM Coresight and Hisilicon
PTT drivers. The driver adds 'gtrace' event descriptor which can be used
by the perf tool to record the trace data. The actual data format depends
on type of encoder device and optionally the PMU specific trace format
type.
> diff --git a/drivers/hwtracing/gtrace/gtrace-perf.c b/drivers/hwtracing/gtrace/gtrace-perf.c
> new file mode 100644
> index 0000000000000..ed9155bbbdb7a
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/gtrace-perf.c
> @@ -0,0 +1,347 @@
[ ... ]
> +static void *alloc_event_data(int cpu)
> +{
> + struct gtrace_event_data *event_data;
> + cpumask_t *mask;
> +
> + event_data = kzalloc_obj(*event_data);
> + if (!event_data)
> + return NULL;
> +
> + /* Update mask as per selected CPUs */
> + mask = &event_data->mask;
> + if (cpu != -1)
> + cpumask_set_cpu(cpu, mask);
> + else
> + cpumask_copy(mask, cpu_present_mask);
> +
> + event_data->path = alloc_percpu(struct gtrace_path *);
> + return event_data;
> +}
[Severity: High]
Can alloc_percpu() fail here?
If it returns a NULL pointer, event_data->path will be NULL. Later in
gtrace_setup_aux(), this pointer is passed to per_cpu_ptr(), which blindly
adds the CPU offset to the NULL pointer and generates an invalid kernel
address, resulting in an oops on dereference.
[ ... ]
> +static void free_event_data(struct work_struct *work)
> +{
> + struct gtrace_event_data *event_data;
> + struct gtrace_path *path;
> + cpumask_t *mask;
> + int cpu;
> +
> + event_data = container_of(work, struct gtrace_event_data, work);
> + mask = &event_data->mask;
> + for_each_cpu(cpu, mask) {
> + path = *gtrace_event_cpu_path_ptr(event_data, cpu);
> + gtrace_destroy_path(path);
> + }
[Severity: High]
When gtrace_setup_aux() skips CPUs due to initialization failures (e.g., if
gtrace_cpu_source() returns NULL), it leaves those CPUs in the mask.
Will this loop blindly pass uninitialized or NULL paths to
gtrace_destroy_path(), causing a NULL pointer dereference when accessing
path->comp_list?
> + free_percpu(event_data->path);
> + kfree(event_data);
> +}
[Severity: High]
Looking at the setup path in gtrace_setup_aux(), the buffer is mapped using
vmap(), but this cleanup function does not call vunmap(event_data->buf.base).
Will this permanently leak the vmalloc space each time a session is destroyed?
[ ... ]
> +static void *gtrace_setup_aux(struct perf_event *event, void **pages,
> + int nr_pages, bool overwrite)
> +{
[ ... ]
> + /*
> + * Create the path for each CPU in the mask. In case of any failure skip the CPU
> + */
> + for_each_cpu(cpu, mask) {
> + struct gtrace_component *src;
> + struct gtrace_path *path;
> +
> + src = gtrace_cpu_source(cpu);
> + if (!src)
> + continue;
> +
> + path = gtrace_create_path(src, NULL, GTRACE_COMPONENT_MODE_PERF);
> + if (!path)
> + continue;
[Severity: High]
When skipping a CPU due to these error paths, should the CPU be cleared from
event_data->mask?
Leaving it in the mask bypasses the check for valid CPUs below and causes
the cleanup function to operate on uninitialized pointers.
[Severity: High]
Does gtrace_create_path() return NULL on failure? Looking at its
implementation, it returns an error pointer like ERR_PTR(-ENOMEM).
Using !path here evaluates to false for error pointers, storing an invalid
pointer that will later cause a kernel panic when dereferenced.
> +
> + *gtrace_event_cpu_path_ptr(event_data, cpu) = path;
> + }
[ ... ]
> + pagelist = kcalloc(nr_pages, sizeof(*pagelist), GFP_KERNEL);
> + if (!pagelist)
> + goto err;
> +
> + for (i = 0; i < nr_pages; i++)
> + pagelist[i] = virt_to_page(pages[i]);
> +
> + event_data->buf.base = vmap(pagelist, nr_pages, VM_MAP, PAGE_KERNEL);
> + if (!event_data->buf.base) {
> + kfree(pagelist);
> + goto err;
> + }
> +
> + event_data->buf.nr_pages = nr_pages;
> + event_data->buf.length = nr_pages * PAGE_SIZE;
> + event_data->buf.pos = 0;
> + return event_data;
[Severity: Medium]
Is the pagelist array leaked on this success path?
The error path correctly frees it, but it appears vmap() only maps the pages
without taking ownership of the array itself.
[ ... ]
> +static void gtrace_event_start(struct perf_event *event, int flags)
> +{
[ ... ]
> + event_data = perf_aux_output_begin(handle, event);
> + if (!event_data)
> + goto fail;
> +
> + if (!cpumask_test_cpu(cpu, &event_data->mask))
> + goto out;
> +
> + event_data->buf.pos = handle->head % event_data->buf.length;
> + path = *gtrace_event_cpu_path_ptr(event_data, cpu);
> + if (!path) {
> + pr_err("Error. Path not found\n");
> + return;
> + }
[Severity: High]
Because perf_aux_output_begin() successfully acquired a handle to the AUX
ring buffer, does this error path need to call perf_aux_output_end() before
returning? Returning directly leaves the ring buffer permanently locked.
> +
> + if (gtrace_path_start(path)) {
> + pr_err("Error. Tracing not started\n");
> + return;
> + }
[Severity: High]
Looking at gtrace_path_start(), if a component fails to start in the middle
of its initialization loop, it returns an error immediately.
Does this lack of rollback strand previously started hardware components in an
active state?
[ ... ]
> +static void gtrace_event_stop(struct perf_event *event, int mode)
> +{
[ ... ]
> + /* stop tracing */
> + path = *gtrace_event_cpu_path_ptr(event_data, cpu);
> + if (!path) {
> + pr_err("Error. Path not found\n");
> + return;
> + }
[Severity: High]
If we hit this error path after an active handle is established, do we need
to call perf_aux_output_end() to properly close the session?
> +
> + if (gtrace_path_stop(path)) {
> + pr_err("Error. Tracing not stopped\n");
> + return;
> + }
[Severity: High]
Similar to gtrace_path_start(), gtrace_path_stop() aborts its loop early
if a single component fails to stop. Could this leave hardware components
running indefinitely?
> +
> + event->hw.state = PERF_HES_STOPPED;
> + if (handle->event && (mode & PERF_EF_UPDATE)) {
> + if (WARN_ON_ONCE(handle->event != event))
> + return;
> + spin_lock(&perf_buf_lock);
[Severity: High]
Is it safe to acquire perf_buf_lock here?
The perf events core invokes PMU stop callbacks with a raw_spinlock_t held.
Acquiring a regular spinlock_t while holding a raw lock is an invalid
wait-context nesting order and will trigger a BUG on PREEMPT_RT kernels.
> + ret = gtrace_path_copyto_auxbuf(path, &event_data->buf, &size, &format);
> + spin_unlock(&perf_buf_lock);
> + WARN_ON_ONCE(ret);
> + if (READ_ONCE(handle->event)) {
> + /* Tag the AUX data with the format reported by the sink. */
> + perf_aux_output_flag(handle, format);
> + if (size > handle->size) {
> + size = handle->size;
> + perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> + }
> + perf_aux_output_end(handle, size);
[Severity: High]
If gtrace_path_copyto_auxbuf() returns an error, size and format might
remain uninitialized.
Since this code emits a warning but continues execution, will it pass
stack-garbage values to perf_aux_output_flag() and perf_aux_output_end(),
corrupting the ring buffer head pointer?
[ ... ]
> +void __exit gtrace_perf_exit(void)
> +{
> + perf_pmu_unregister(>race_pmu);
> +}
[Severity: High]
The driver schedules asynchronous work items via schedule_work(&event_data->work)
to clean up event data.
Does this exit function need to call flush_work() or flush_scheduled_work()
before returning? If the module unloads while work is pending, the workqueue
thread will execute freed memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=11
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 12/14] perf tools: Add RISC-V trace PMU record capabilities
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (10 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 11/14] gtrace: Add perf driver for tracing using perf tool Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:55 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 13/14] perf tools: Initial support for gtrace decoder Mayuresh Chitale
2026-08-10 15:22 ` [PATCH v5 14/14] MAINTAINERS: Add entry for RISC-V trace framework Mayuresh Chitale
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Mayuresh Chitale, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel, Anup Patel,
Nutty Liu
Introduce the required auxiliary API functions allowing the perf core
to interact with gtrace perf driver. On RISC-V systems the gtrace
PMU uses the RISC-V E-Trace components and optionally an ATB Bridge
component for connecting to ARM-side sinks, if it is present on the SoC.
Co-developed-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
---
tools/perf/arch/riscv/util/Build | 1 +
tools/perf/arch/riscv/util/auxtrace.c | 217 ++++++++++++++++++++++++++
tools/perf/util/auxtrace.c | 1 +
tools/perf/util/auxtrace.h | 1 +
tools/perf/util/gtrace.h | 12 ++
5 files changed, 232 insertions(+)
create mode 100644 tools/perf/arch/riscv/util/auxtrace.c
create mode 100644 tools/perf/util/gtrace.h
diff --git a/tools/perf/arch/riscv/util/Build b/tools/perf/arch/riscv/util/Build
index 2328fb9a30a3..e07d5525ece6 100644
--- a/tools/perf/arch/riscv/util/Build
+++ b/tools/perf/arch/riscv/util/Build
@@ -1 +1,2 @@
perf-util-y += header.o
+perf-util-y += auxtrace.o
diff --git a/tools/perf/arch/riscv/util/auxtrace.c b/tools/perf/arch/riscv/util/auxtrace.c
new file mode 100644
index 000000000000..39c31be9f896
--- /dev/null
+++ b/tools/perf/arch/riscv/util/auxtrace.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Risc-V E-Trace support
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/bitops.h>
+#include <linux/log2.h>
+#include <linux/zalloc.h>
+#include <errno.h>
+#include <time.h>
+
+#include <internal/lib.h>
+#include "../../../util/auxtrace.h"
+#include "../../../util/cpumap.h"
+#include "../../../util/debug.h"
+#include "../../../util/event.h"
+#include "../../../util/evlist.h"
+#include "../../../util/evsel.h"
+#include "../../../util/pmu.h"
+#include "../../../util/record.h"
+#include "../../../util/session.h"
+
+
+#define GTRACE_PMU_NAME "gtrace"
+#define GTRACE_AUXTRACE_PRIV_SIZE sizeof(u64)
+#define KiB(x) ((x) * 1024)
+#define MiB(x) ((x) * 1024 * 1024)
+
+struct rvtrace_recording {
+ struct auxtrace_record itr;
+ struct perf_pmu *rvtrace_pmu;
+ struct evlist *evlist;
+};
+
+static size_t rvtrace_info_priv_size(struct auxtrace_record *itr __maybe_unused,
+ struct evlist *evlist __maybe_unused)
+{
+ return GTRACE_AUXTRACE_PRIV_SIZE;
+}
+
+static int rvtrace_info_fill(struct auxtrace_record *itr,
+ struct perf_session *session __maybe_unused,
+ struct perf_record_auxtrace_info *auxtrace_info, size_t priv_size)
+{
+ struct rvtrace_recording *ptr = container_of(itr, struct rvtrace_recording, itr);
+ struct perf_pmu *rvtrace_pmu = ptr->rvtrace_pmu;
+
+ if (priv_size != GTRACE_AUXTRACE_PRIV_SIZE)
+ return -EINVAL;
+
+ auxtrace_info->type = PERF_AUXTRACE_GTRACE;
+ auxtrace_info->priv[0] = rvtrace_pmu->type;
+
+ return 0;
+}
+
+static int rvtrace_set_auxtrace_mmap_page(struct record_opts *opts)
+{
+ bool privileged = perf_event_paranoid_check(-1);
+
+ if (!opts->full_auxtrace)
+ return 0;
+
+ if (opts->full_auxtrace && !opts->auxtrace_mmap_pages) {
+ if (privileged) {
+ opts->auxtrace_mmap_pages = MiB(16) / page_size;
+ } else {
+ opts->auxtrace_mmap_pages = KiB(128) / page_size;
+ if (opts->mmap_pages == UINT_MAX)
+ opts->mmap_pages = KiB(256) / page_size;
+ }
+ }
+
+ /* Validate auxtrace_mmap_pages */
+ if (opts->auxtrace_mmap_pages) {
+ size_t sz = opts->auxtrace_mmap_pages * (size_t)page_size;
+ size_t min_sz = KiB(8);
+
+ if (sz < min_sz || !is_power_of_2(sz)) {
+ pr_err("Invalid mmap size : must be at least %zuKiB and a power of 2\n",
+ min_sz / 1024);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static int rvtrace_recording_options(struct auxtrace_record *itr, struct evlist *evlist,
+ struct record_opts *opts)
+{
+ struct rvtrace_recording *ptr = container_of(itr, struct rvtrace_recording, itr);
+ struct perf_pmu *rvtrace_pmu = ptr->rvtrace_pmu;
+ struct evsel *evsel, *rvtrace_evsel = NULL;
+ struct evsel *tracking_evsel;
+ int err;
+
+ ptr->evlist = evlist;
+ evlist__for_each_entry(evlist, evsel) {
+ if (evsel->core.attr.type == rvtrace_pmu->type) {
+ if (rvtrace_evsel) {
+ pr_err("There may be only one " GTRACE_PMU_NAME " event\n");
+ return -EINVAL;
+ }
+ evsel->core.attr.freq = 0;
+ evsel->core.attr.sample_period = 1;
+ evsel->needs_auxtrace_mmap = true;
+ rvtrace_evsel = evsel;
+ opts->full_auxtrace = true;
+ }
+ }
+
+ err = rvtrace_set_auxtrace_mmap_page(opts);
+ if (err)
+ return err;
+ /*
+ * To obtain the auxtrace buffer file descriptor, the auxtrace event
+ * must come first.
+ */
+ evlist__to_front(evlist, rvtrace_evsel);
+ evsel__set_sample_bit(rvtrace_evsel, TIME);
+
+ /* Add dummy event to keep tracking */
+ err = parse_event(evlist, "dummy:u");
+ if (err)
+ return err;
+
+ tracking_evsel = evlist__last(evlist);
+ evlist__set_tracking_event(evlist, tracking_evsel);
+
+ tracking_evsel->core.attr.freq = 0;
+ tracking_evsel->core.attr.sample_period = 1;
+ evsel__set_sample_bit(tracking_evsel, TIME);
+
+ return 0;
+}
+
+static u64 rvtrace_reference(struct auxtrace_record *itr __maybe_unused)
+{
+ return 0;
+}
+
+static void rvtrace_recording_free(struct auxtrace_record *itr)
+{
+ struct rvtrace_recording *ptr = container_of(itr, struct rvtrace_recording, itr);
+
+ free(ptr);
+}
+
+static struct auxtrace_record *rvtrace_recording_init(int *err, struct perf_pmu *rvtrace_pmu)
+{
+ struct rvtrace_recording *ptr;
+
+ if (!rvtrace_pmu) {
+ *err = -ENODEV;
+ return NULL;
+ }
+
+ ptr = zalloc(sizeof(*ptr));
+ if (!ptr) {
+ *err = -ENOMEM;
+ return NULL;
+ }
+
+ ptr->rvtrace_pmu = rvtrace_pmu;
+ ptr->itr.recording_options = rvtrace_recording_options;
+ ptr->itr.info_priv_size = rvtrace_info_priv_size;
+ ptr->itr.info_fill = rvtrace_info_fill;
+ ptr->itr.free = rvtrace_recording_free;
+ ptr->itr.reference = rvtrace_reference;
+ ptr->itr.read_finish = auxtrace_record__read_finish;
+ ptr->itr.alignment = 0;
+
+ *err = 0;
+ return &ptr->itr;
+}
+
+static struct perf_pmu *find_pmu_for_event(struct perf_pmu **pmus,
+ int pmu_nr, struct evsel *evsel)
+{
+ int i;
+
+ if (!pmus)
+ return NULL;
+
+ for (i = 0; i < pmu_nr; i++) {
+ if (evsel->core.attr.type == pmus[i]->type)
+ return pmus[i];
+ }
+
+ return NULL;
+}
+
+struct auxtrace_record *auxtrace_record__init(struct evlist *evlist, int *err)
+{
+ struct perf_pmu *rvtrace_pmu = NULL;
+ struct perf_pmu *found_pmu = NULL;
+ struct evsel *evsel;
+
+ if (!evlist)
+ return NULL;
+
+ rvtrace_pmu = perf_pmus__find(GTRACE_PMU_NAME);
+ evlist__for_each_entry(evlist, evsel) {
+ if (rvtrace_pmu && !found_pmu)
+ found_pmu = find_pmu_for_event(&rvtrace_pmu, 1, evsel);
+ }
+
+ if (found_pmu)
+ return rvtrace_recording_init(err, rvtrace_pmu);
+
+ *err = 0;
+ return NULL;
+}
diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index 4cd2caf54015..d7f27ca15b81 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -1427,6 +1427,7 @@ int perf_event__process_auxtrace_info(const struct perf_tool *tool __maybe_unuse
case PERF_AUXTRACE_VPA_DTL:
err = powerpc_vpadtl_process_auxtrace_info(event, session);
break;
+ case PERF_AUXTRACE_GTRACE:
case PERF_AUXTRACE_UNKNOWN:
default:
return -EINVAL;
diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
index 6947f3f284c0..42b1481ec894 100644
--- a/tools/perf/util/auxtrace.h
+++ b/tools/perf/util/auxtrace.h
@@ -46,6 +46,7 @@ enum auxtrace_type {
PERF_AUXTRACE_S390_CPUMSF,
PERF_AUXTRACE_HISI_PTT,
PERF_AUXTRACE_VPA_DTL,
+ PERF_AUXTRACE_GTRACE,
};
enum itrace_period_type {
diff --git a/tools/perf/util/gtrace.h b/tools/perf/util/gtrace.h
new file mode 100644
index 000000000000..4b87bdaf55c6
--- /dev/null
+++ b/tools/perf/util/gtrace.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ */
+
+#ifndef INCLUDE__UTIL_PERF_GTRACE_H__
+#define INCLUDE__UTIL_PERF_GTRACE_H__
+
+#define GTRACE_AUXTRACE_PRIV_SIZE sizeof(u64)
+
+#endif
+
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 12/14] perf tools: Add RISC-V trace PMU record capabilities
2026-08-10 15:22 ` [PATCH v5 12/14] perf tools: Add RISC-V trace PMU record capabilities Mayuresh Chitale
@ 2026-08-10 15:55 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:55 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: devicetree, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Missing explicit header inclusions for POSIX libc functions (`free()`) and constants (`UINT_MAX`), violating musl libc build compatibility guidelines.
- [Low] The newly added header `gtrace.h` is completely unused, its macro is re-defined locally, and it is structurally broken due to missing type dependencies.
- [Low] The function `parse_event()` is used without including its corresponding header file.
--
--- Patch [12]: [PATCH v5 12/14] perf tools: Add RISC-V trace PMU record capabilities ---
commit dc36885ec60bdf3420446c3aec420428e7cdc32a
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
perf tools: Add RISC-V trace PMU record capabilities
Introduce the required auxiliary API functions allowing the perf core
to interact with gtrace perf driver. On RISC-V systems the gtrace
PMU uses the RISC-V E-Trace components and optionally an ATB Bridge
component for connecting to ARM-side sinks, if it is present on the SoC.
> diff --git a/tools/perf/arch/riscv/util/auxtrace.c b/tools/perf/arch/riscv/util/auxtrace.c
> new file mode 100644
> index 0000000000000..39c31be9f8966
> --- /dev/null
> +++ b/tools/perf/arch/riscv/util/auxtrace.c
> @@ -0,0 +1,217 @@
[ ... ]
> +#include <internal/lib.h>
> +#include "../../../util/auxtrace.h"
> +#include "../../../util/cpumap.h"
> +#include "../../../util/debug.h"
> +#include "../../../util/event.h"
> +#include "../../../util/evlist.h"
> +#include "../../../util/evsel.h"
> +#include "../../../util/pmu.h"
> +#include "../../../util/record.h"
> +#include "../../../util/session.h"
> +
> +
> +#define GTRACE_PMU_NAME "gtrace"
> +#define GTRACE_AUXTRACE_PRIV_SIZE sizeof(u64)
[Severity: Low]
Is this macro definition duplicating the one provided in the newly added
tools/perf/util/gtrace.h? It looks like that header might have been intended
to be included here instead of redefining the macro locally.
> +#define KiB(x) ((x) * 1024)
> +#define MiB(x) ((x) * 1024 * 1024)
[ ... ]
> +static int rvtrace_set_auxtrace_mmap_page(struct record_opts *opts)
> +{
> + bool privileged = perf_event_paranoid_check(-1);
> +
> + if (!opts->full_auxtrace)
> + return 0;
> +
> + if (opts->full_auxtrace && !opts->auxtrace_mmap_pages) {
> + if (privileged) {
> + opts->auxtrace_mmap_pages = MiB(16) / page_size;
> + } else {
> + opts->auxtrace_mmap_pages = KiB(128) / page_size;
> + if (opts->mmap_pages == UINT_MAX)
[Severity: Low]
Does this code rely on implicit header inclusion for UINT_MAX? Including
limits.h directly might be required to ensure musl libc compatibility.
> + opts->mmap_pages = KiB(256) / page_size;
> + }
> + }
[ ... ]
> +static int rvtrace_recording_options(struct auxtrace_record *itr, struct evlist *evlist,
> + struct record_opts *opts)
> +{
[ ... ]
> + /* Add dummy event to keep tracking */
> + err = parse_event(evlist, "dummy:u");
[Severity: Low]
Could this trigger an implicit function declaration warning? It seems
util/parse-events.h is missing from the includes at the top of the file.
> + if (err)
> + return err;
[ ... ]
> +static void rvtrace_recording_free(struct auxtrace_record *itr)
> +{
> + struct rvtrace_recording *ptr = container_of(itr, struct rvtrace_recording, itr);
> +
> + free(ptr);
[Severity: Low]
Does this code rely on implicit header inclusion for free()? Explicitly
including stdlib.h is usually necessary for musl compatibility.
> +}
[ ... ]
> diff --git a/tools/perf/util/gtrace.h b/tools/perf/util/gtrace.h
> new file mode 100644
> index 0000000000000..4b87bdaf55c69
> --- /dev/null
> +++ b/tools/perf/util/gtrace.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2026 Qualcomm Technologies, Inc.
> + */
> +
> +#ifndef INCLUDE__UTIL_PERF_GTRACE_H__
> +#define INCLUDE__UTIL_PERF_GTRACE_H__
> +
> +#define GTRACE_AUXTRACE_PRIV_SIZE sizeof(u64)
[Severity: Low]
Is this header file completely unused, given that GTRACE_AUXTRACE_PRIV_SIZE
was redefined locally in tools/perf/arch/riscv/util/auxtrace.c?
Additionally, does this macro need linux/types.h to be included so that u64
is defined when this header is used?
> +
> +#endif
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=12
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 13/14] perf tools: Initial support for gtrace decoder
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (11 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 12/14] perf tools: Add RISC-V trace PMU record capabilities Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
2026-08-10 15:51 ` sashiko-bot
2026-08-10 15:22 ` [PATCH v5 14/14] MAINTAINERS: Add entry for RISC-V trace framework Mayuresh Chitale
13 siblings, 1 reply; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Mayuresh Chitale, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel, Anup Patel
Add bare bones support for gtrace decoder so that the data received
from the hardware by the gtrace perf driver can be written to the
perf record output file. Actual decoding of the trace payload is not yet
implemented.
Co-developed-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
---
tools/perf/util/Build | 1 +
tools/perf/util/auxtrace.c | 3 ++
tools/perf/util/gtrace-decoder.c | 93 ++++++++++++++++++++++++++++++++
tools/perf/util/gtrace.h | 1 +
4 files changed, 98 insertions(+)
create mode 100644 tools/perf/util/gtrace-decoder.c
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 5e2265018826..7bd752b5ae6a 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -148,6 +148,7 @@ perf-util-y += cs-etm.o
perf-util-y += cs-etm-decoder/
endif
perf-util-y += cs-etm-base.o
+perf-util-y += gtrace-decoder.o
perf-util-y += parse-branch-options.o
perf-util-y += parse-regs-options.o
diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index d7f27ca15b81..8eda626aede5 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -54,6 +54,7 @@
#include "arm-spe.h"
#include "hisi-ptt.h"
#include "s390-cpumsf.h"
+#include "gtrace.h"
#include "util/mmap.h"
#include "powerpc-vpadtl.h"
@@ -1428,6 +1429,8 @@ int perf_event__process_auxtrace_info(const struct perf_tool *tool __maybe_unuse
err = powerpc_vpadtl_process_auxtrace_info(event, session);
break;
case PERF_AUXTRACE_GTRACE:
+ err = gtrace__process_auxtrace_info(event, session);
+ break;
case PERF_AUXTRACE_UNKNOWN:
default:
return -EINVAL;
diff --git a/tools/perf/util/gtrace-decoder.c b/tools/perf/util/gtrace-decoder.c
new file mode 100644
index 000000000000..4a1571431edb
--- /dev/null
+++ b/tools/perf/util/gtrace-decoder.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic hardware trace (gtrace) decoder
+ * Copyright (c) 2026 Qualcomm Technologies, Inc.
+ */
+
+#include <errno.h>
+#include <inttypes.h>
+#include <internal/lib.h>
+#include "evlist.h"
+#include "session.h"
+#include "gtrace.h"
+
+struct gtrace_decoder {
+ struct auxtrace auxtrace;
+ u32 auxtrace_type;
+ struct perf_session *session;
+ struct machine *machine;
+ u32 pmu_type;
+};
+
+static int gtrace_process_event(struct perf_session *session __maybe_unused,
+ union perf_event *event __maybe_unused,
+ struct perf_sample *sample __maybe_unused,
+ const struct perf_tool *tool __maybe_unused)
+{
+ return 0;
+}
+
+static int gtrace_process_auxtrace_event(struct perf_session *session __maybe_unused,
+ union perf_event *event __maybe_unused,
+ const struct perf_tool *tool __maybe_unused)
+{
+ return 0;
+}
+
+static int gtrace_flush(struct perf_session *session __maybe_unused,
+ const struct perf_tool *tool __maybe_unused)
+{
+ return 0;
+}
+
+static void gtrace_free_events(struct perf_session *session __maybe_unused)
+{
+}
+
+static void gtrace_free(struct perf_session *session)
+{
+ struct gtrace_decoder *ptr = container_of(session->auxtrace, struct gtrace_decoder,
+ auxtrace);
+
+ session->auxtrace = NULL;
+ free(ptr);
+}
+
+static bool gtrace_evsel_is_auxtrace(struct perf_session *session,
+ struct evsel *evsel)
+{
+ struct gtrace_decoder *ptr = container_of(session->auxtrace,
+ struct gtrace_decoder, auxtrace);
+
+ return evsel->core.attr.type == ptr->pmu_type;
+}
+
+int gtrace__process_auxtrace_info(union perf_event *event,
+ struct perf_session *session)
+{
+ struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
+ struct gtrace_decoder *ptr;
+
+ if (auxtrace_info->header.size < GTRACE_AUXTRACE_PRIV_SIZE +
+ sizeof(struct perf_record_auxtrace_info))
+ return -EINVAL;
+
+ ptr = zalloc(sizeof(*ptr));
+ if (!ptr)
+ return -ENOMEM;
+
+ ptr->session = session;
+ ptr->machine = &session->machines.host;
+ ptr->auxtrace_type = auxtrace_info->type;
+ ptr->pmu_type = auxtrace_info->priv[0];
+
+ ptr->auxtrace.process_event = gtrace_process_event;
+ ptr->auxtrace.process_auxtrace_event = gtrace_process_auxtrace_event;
+ ptr->auxtrace.flush_events = gtrace_flush;
+ ptr->auxtrace.free_events = gtrace_free_events;
+ ptr->auxtrace.free = gtrace_free;
+ ptr->auxtrace.evsel_is_auxtrace = gtrace_evsel_is_auxtrace;
+ session->auxtrace = &ptr->auxtrace;
+
+ return 0;
+}
diff --git a/tools/perf/util/gtrace.h b/tools/perf/util/gtrace.h
index 4b87bdaf55c6..3531508b1dfb 100644
--- a/tools/perf/util/gtrace.h
+++ b/tools/perf/util/gtrace.h
@@ -8,5 +8,6 @@
#define GTRACE_AUXTRACE_PRIV_SIZE sizeof(u64)
+int gtrace__process_auxtrace_info(union perf_event *event, struct perf_session *session);
#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 13/14] perf tools: Initial support for gtrace decoder
2026-08-10 15:22 ` [PATCH v5 13/14] perf tools: Initial support for gtrace decoder Mayuresh Chitale
@ 2026-08-10 15:51 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-08-10 15:51 UTC (permalink / raw)
To: Mayuresh Chitale; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing explicit header inclusions for free() and zalloc(), leading to musl compatibility failures and pointer truncation.
- [Medium] Unconditionally overwriting `session->auxtrace` causes a memory leak on multiple `PERF_RECORD_AUXTRACE_INFO` events.
--
--- Patch [13]: [PATCH v5 13/14] perf tools: Initial support for gtrace decoder ---
commit 7c1bf67a1a25514ab10564a3e48db1965dbac1a8
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
perf tools: Initial support for gtrace decoder
Add bare bones support for gtrace decoder so that the data received
from the hardware by the gtrace perf driver can be written to the
perf record output file.
> diff --git a/tools/perf/util/gtrace-decoder.c b/tools/perf/util/gtrace-decoder.c
> new file mode 100644
> index 0000000000000..4a1571431edb3
> --- /dev/null
> +++ b/tools/perf/util/gtrace-decoder.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Generic hardware trace (gtrace) decoder
> + * Copyright (c) 2026 Qualcomm Technologies, Inc.
> + */
> +
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <internal/lib.h>
> +#include "evlist.h"
> +#include "session.h"
> +#include "gtrace.h"
[Severity: Medium]
Are we missing some explicit header inclusions here?
The code uses free() and zalloc() further down in the file, but does not
include <stdlib.h> or <linux/zalloc.h>.
Missing <stdlib.h> can cause compilation failures under musl.
Missing <linux/zalloc.h> can cause zalloc() to be implicitly declared as
returning an int. On 64-bit systems, this truncates the upper 32 bits of
the pointer, which can lead to a segmentation fault when dereferenced.
[ ... ]
> +int gtrace__process_auxtrace_info(union perf_event *event,
> + struct perf_session *session)
> +{
> + struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
> + struct gtrace_decoder *ptr;
> +
> + if (auxtrace_info->header.size < GTRACE_AUXTRACE_PRIV_SIZE +
> + sizeof(struct perf_record_auxtrace_info))
> + return -EINVAL;
> +
> + ptr = zalloc(sizeof(*ptr));
> + if (!ptr)
> + return -ENOMEM;
> +
> + ptr->session = session;
> + ptr->machine = &session->machines.host;
> + ptr->auxtrace_type = auxtrace_info->type;
> + ptr->pmu_type = auxtrace_info->priv[0];
> +
> + ptr->auxtrace.process_event = gtrace_process_event;
> + ptr->auxtrace.process_auxtrace_event = gtrace_process_auxtrace_event;
> + ptr->auxtrace.flush_events = gtrace_flush;
> + ptr->auxtrace.free_events = gtrace_free_events;
> + ptr->auxtrace.free = gtrace_free;
> + ptr->auxtrace.evsel_is_auxtrace = gtrace_evsel_is_auxtrace;
> + session->auxtrace = &ptr->auxtrace;
[Severity: Medium]
Could this leak the previously allocated decoder context if multiple
PERF_RECORD_AUXTRACE_INFO events are processed?
It looks like session->auxtrace is unconditionally overwritten here without
checking or freeing any structure that was already allocated.
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=13
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 14/14] MAINTAINERS: Add entry for RISC-V trace framework
2026-08-10 15:22 [PATCH v5 00/14] Linux RISC-V trace framework and drivers Mayuresh Chitale
` (12 preceding siblings ...)
2026-08-10 15:22 ` [PATCH v5 13/14] perf tools: Initial support for gtrace decoder Mayuresh Chitale
@ 2026-08-10 15:22 ` Mayuresh Chitale
13 siblings, 0 replies; 27+ messages in thread
From: Mayuresh Chitale @ 2026-08-10 15:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Palmer Dabbelt,
Paul Walmsley, Greg KH, Alexander Shishkin, Ian Rogers
Cc: Anup Patel, Alexandre Ghiti, Peter Zijlstra, Ingo Molnar,
Namhyung Kim, Mark Rutland, Jiri Olsa, Adrian Hunter,
Mayuresh Chitale, Anup Patel, Atish Patra, Andrew Jones,
Sunil V L, linux-riscv, devicetree, linux-kernel
From: Anup Patel <anup.patel@oss.qualcomm.com>
Add Mayuresh and myself as maintainers for the gtrace framework and
its RISC-V trace drivers.
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
MAINTAINERS | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 4b592cf49746..904392530963 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23359,6 +23359,17 @@ F: include/dt-bindings/power/thead,th1520-power.h
F: include/dt-bindings/reset/thead,th1520-reset.h
F: include/linux/firmware/thead/thead,th1520-aon.h
+GTRACE FRAMEWORK AND RISC-V TRACE DRIVERS
+M: Mayuresh Chitale <mchitale@gmail.com>
+M: Anup Patel <anup@brainfault.org>
+L: linux-riscv@lists.infradead.org
+S: Maintained
+F: Documentation/devicetree/bindings/riscv/riscv,trace-component.yaml
+F: drivers/hwtracing/gtrace/*
+F: include/linux/gtrace*
+F: tools/perf/arch/riscv/util/auxtrace.c
+F: tools/perf/util/gtrace*
+
RNBD BLOCK DRIVERS
M: Md. Haris Iqbal <haris.iqbal@ionos.com>
M: Jack Wang <jinpu.wang@ionos.com>
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread