LinuxLists.cc - [PATCH v2 0/3] coresight: Add preferred trace id support

2024-06-03 09:48:56

Subject: [PATCH v2 0/3] coresight: Add preferred trace id support

Some HW has static trace id which cannot be changed via
software programming. For this case, configure the trace id
in device tree with "trace-id = <xxx>", and
call coresight_trace_id_get_system_id with the trace id value
in device probe function. The id will be reserved for the HW
all the time if the device is probed.

Changes since V1:
1. Add argument to coresight_trace_id_get_system_id for preferred id
instead of adding new function coresight_trace_id_reserve_system_id.
2. Add constraint to trace-id in dt-binding file.

Mao Jinlong (3):
dt-bindings: arm: Add trace-id for coresight dummy source
coresight: Add support to get preferred id for system trace sources
coresight: dummy: Add reserve atid support for dummy source

.../sysfs-bus-coresight-devices-dummy-source | 15 +++++
.../arm/arm,coresight-dummy-source.yaml | 6 ++
drivers/hwtracing/coresight/coresight-dummy.c | 58 +++++++++++++++++--
.../hwtracing/coresight/coresight-platform.c | 26 +++++++++
drivers/hwtracing/coresight/coresight-stm.c | 2 +-
.../hwtracing/coresight/coresight-trace-id.c | 32 ++++++----
.../hwtracing/coresight/coresight-trace-id.h | 5 +-
include/linux/coresight.h | 1 +
8 files changed, 128 insertions(+), 17 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source

--
2.41.0

2024-06-03 09:48:57

by Mao Jinlong

[permalink] [raw]

Subject: [PATCH v2 3/3] coresight: dummy: Add reserve atid support for dummy source

Some dummy source has static trace id configured in HW and it cannot
be changed via software programming. Configure the trace id in device
tree and reserve the id when device probe.

Signed-off-by: Mao Jinlong <[email protected]>
---
.../sysfs-bus-coresight-devices-dummy-source | 15 +++++
drivers/hwtracing/coresight/coresight-dummy.c | 58 +++++++++++++++++--
2 files changed, 69 insertions(+), 4 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
new file mode 100644
index 000000000000..d93c198115c9
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
@@ -0,0 +1,15 @@
+What: /sys/bus/coresight/devices/dummy_source<N>/enable_source
+Date: June 2024
+KernelVersion: 6.9
+Contact: Mao Jinlong <[email protected]>
+Description: (RW) Enable/disable tracing of dummy source. A sink should be activated
+ before enabling the source. The path of coresight components linking
+ the source to the sink is configured and managed automatically by the
+ coresight framework.
+
+What: /sys/bus/coresight/devices/dummy_source<N>/traceid
+Date: June 2024
+KernelVersion: 6.9
+Contact: Mao Jinlong <[email protected]>
+Description: (R) Show the trace ID that will appear in the trace stream
+ coming from this trace entity.
diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
index ac70c0b491be..1f7133ac2c0b 100644
--- a/drivers/hwtracing/coresight/coresight-dummy.c
+++ b/drivers/hwtracing/coresight/coresight-dummy.c
@@ -11,10 +11,12 @@
#include <linux/pm_runtime.h>

#include "coresight-priv.h"
+#include "coresight-trace-id.h"

struct dummy_drvdata {
struct device *dev;
struct coresight_device *csdev;
+ u8 traceid;
};

DEFINE_CORESIGHT_DEVLIST(source_devs, "dummy_source");
@@ -67,6 +69,32 @@ static const struct coresight_ops dummy_sink_cs_ops = {
.sink_ops = &dummy_sink_ops,
};

+/* User can get the trace id of dummy source from this node. */
+static ssize_t traceid_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ unsigned long val;
+ struct dummy_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+ val = drvdata->traceid;
+ return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+static DEVICE_ATTR_RO(traceid);
+
+static struct attribute *coresight_dummy_attrs[] = {
+ &dev_attr_traceid.attr,
+ NULL,
+};
+
+static const struct attribute_group coresight_dummy_group = {
+ .attrs = coresight_dummy_attrs,
+};
+
+static const struct attribute_group *coresight_dummy_groups[] = {
+ &coresight_dummy_group,
+ NULL,
+};
+
static int dummy_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -74,6 +102,11 @@ static int dummy_probe(struct platform_device *pdev)
struct coresight_platform_data *pdata;
struct dummy_drvdata *drvdata;
struct coresight_desc desc = { 0 };
+ int ret, trace_id;
+
+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;

if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {

@@ -85,6 +118,24 @@ static int dummy_probe(struct platform_device *pdev)
desc.subtype.source_subtype =
CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
desc.ops = &dummy_source_cs_ops;
+ desc.groups = coresight_dummy_groups;
+
+ ret = coresight_get_source_traceid(dev, &trace_id);
+ if (!ret) {
+ ret = coresight_trace_id_get_system_id(trace_id);
+ if (ret < 0)
+ return ret;
+
+ drvdata->traceid = ret;
+ } else {
+ trace_id = coresight_trace_id_get_system_id(0);
+ if (trace_id < 0) {
+ ret = trace_id;
+ return ret;
+ }
+ drvdata->traceid = (u8)trace_id;
+ }
+
} else if (of_device_is_compatible(node, "arm,coresight-dummy-sink")) {
desc.name = coresight_alloc_device_name(&sink_devs, dev);
if (!desc.name)
@@ -103,10 +154,6 @@ static int dummy_probe(struct platform_device *pdev)
return PTR_ERR(pdata);
pdev->dev.platform_data = pdata;

- drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
- if (!drvdata)
- return -ENOMEM;
-
drvdata->dev = &pdev->dev;
platform_set_drvdata(pdev, drvdata);

@@ -126,7 +173,10 @@ static void dummy_remove(struct platform_device *pdev)
{
struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
struct device *dev = &pdev->dev;
+ struct device_node *node = dev->of_node;

+ if (of_device_is_compatible(node, "arm,coresight-dummy-source"))
+ coresight_trace_id_put_system_id(drvdata->traceid);
pm_runtime_disable(dev);
coresight_unregister(drvdata->csdev);
}
--
2.41.0

2024-06-03 09:49:17

by Mao Jinlong

[permalink] [raw]

Subject: [PATCH v2 2/3] coresight: Add support to get preferred id for system trace sources

Dynamic trace id was introduced in coresight subsystem, so trace id is
allocated dynamically. However, some hardware ATB source has static trace
id and it cannot be changed via software programming. For such source,
it can call coresight_get_source_traceid to get the fixed trace id from
device node and pass id to coresight_trace_id_get_system_id to reserve
the id.

Signed-off-by: Mao Jinlong <[email protected]>
---
.../hwtracing/coresight/coresight-platform.c | 26 +++++++++++++++
drivers/hwtracing/coresight/coresight-stm.c | 2 +-
.../hwtracing/coresight/coresight-trace-id.c | 32 ++++++++++++-------
.../hwtracing/coresight/coresight-trace-id.h | 5 ++-
include/linux/coresight.h | 1 +
5 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 9d550f5697fa..8dd3cbd676b8 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -183,6 +183,17 @@ static int of_coresight_get_cpu(struct device *dev)
return cpu;
}

+/*
+ * of_coresight_get_trace_id: Get the atid of a source device.
+ *
+ * Returns 0 on success.
+ */
+static int of_coresight_get_trace_id(struct device *dev, u32 *id)
+{
+
+ return of_property_read_u32(dev->of_node, "trace-id", id);
+}
+
/*
* of_coresight_parse_endpoint : Parse the given output endpoint @ep
* and fill the connection information in @pdata->out_conns
@@ -782,6 +793,12 @@ static inline int acpi_coresight_get_cpu(struct device *dev)
{
return -ENODEV;
}
+
+static int of_coresight_get_trace_id(struct device *dev, u32 *id)
+{
+ return -ENODEV;
+}
+
#endif

int coresight_get_cpu(struct device *dev)
@@ -794,6 +811,15 @@ int coresight_get_cpu(struct device *dev)
}
EXPORT_SYMBOL_GPL(coresight_get_cpu);

+int coresight_get_source_traceid(struct device *dev, u32 *id)
+{
+ if (!is_of_node(dev->fwnode))
+ return -EINVAL;
+
+ return of_coresight_get_trace_id(dev, id);
+}
+EXPORT_SYMBOL_GPL(coresight_get_source_traceid);
+
struct coresight_platform_data *
coresight_get_platform_data(struct device *dev)
{
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index e1c62820dfda..802f9e4ae570 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -901,7 +901,7 @@ static int __stm_probe(struct device *dev, struct resource *res)
goto stm_unregister;
}

- trace_id = coresight_trace_id_get_system_id();
+ trace_id = coresight_trace_id_get_system_id(0);
if (trace_id < 0) {
ret = trace_id;
goto cs_unregister;
diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
index af5b4ef59cea..5c25c75a2f08 100644
--- a/drivers/hwtracing/coresight/coresight-trace-id.c
+++ b/drivers/hwtracing/coresight/coresight-trace-id.c
@@ -75,20 +75,23 @@ static int coresight_trace_id_find_odd_id(struct coresight_trace_id_map *id_map)
* Allocate new ID and set in use
*
* if @preferred_id is a valid id then try to use that value if available.
+ * if @only_preferred is true, if @preferred_id is used, return error EINVAL.
* if @preferred_id is not valid and @prefer_odd_id is true, try for odd id.
*
* Otherwise allocate next available ID.
*/
static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map,
- int preferred_id, bool prefer_odd_id)
+ int preferred_id, bool prefer_odd_id, bool only_preferred)
{
int id = 0;

/* for backwards compatibility, cpu IDs may use preferred value */
- if (IS_VALID_CS_TRACE_ID(preferred_id) &&
- !test_bit(preferred_id, id_map->used_ids)) {
- id = preferred_id;
- goto trace_id_allocated;
+ if (IS_VALID_CS_TRACE_ID(preferred_id)) {
+ if (!test_bit(preferred_id, id_map->used_ids)) {
+ id = preferred_id;
+ goto trace_id_allocated;
+ } else if (WARN(only_preferred, "Trace ID %d is used.\n", preferred_id))
+ return -EINVAL;
} else if (prefer_odd_id) {
/* may use odd ids to avoid preferred legacy cpu IDs */
id = coresight_trace_id_find_odd_id(id_map);
@@ -175,7 +178,7 @@ static int coresight_trace_id_map_get_cpu_id(int cpu, struct coresight_trace_id_
*/
id = coresight_trace_id_alloc_new_id(id_map,
CORESIGHT_LEGACY_CPU_TRACE_ID(cpu),
- false);
+ false, false);
if (!IS_VALID_CS_TRACE_ID(id))
goto get_cpu_id_out_unlock;

@@ -222,14 +225,21 @@ static void coresight_trace_id_map_put_cpu_id(int cpu, struct coresight_trace_id
DUMP_ID_MAP(id_map);
}

-static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map)
+static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map,
+ int preferred_id, bool only_preferred)
{
unsigned long flags;
int id;

spin_lock_irqsave(&id_map_lock, flags);
- /* prefer odd IDs for system components to avoid legacy CPU IDS */
- id = coresight_trace_id_alloc_new_id(id_map, 0, true);
+
+ if (preferred_id > 0) {
+ /* use preferred id if it is available */
+ id = coresight_trace_id_alloc_new_id(id_map, preferred_id, false, true);
+ } else {
+ /* prefer odd IDs for system components to avoid legacy CPU IDS */
+ id = coresight_trace_id_alloc_new_id(id_map, 0, true, false);
+ }
spin_unlock_irqrestore(&id_map_lock, flags);

DUMP_ID(id);
@@ -269,9 +279,9 @@ int coresight_trace_id_read_cpu_id(int cpu)
}
EXPORT_SYMBOL_GPL(coresight_trace_id_read_cpu_id);

-int coresight_trace_id_get_system_id(void)
+int coresight_trace_id_get_system_id(int id)
{
- return coresight_trace_id_map_get_system_id(&id_map_default);
+ return coresight_trace_id_map_get_system_id(&id_map_default, id, true);
}
EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);

diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h
index 3797777d367e..9189a33c5857 100644
--- a/drivers/hwtracing/coresight/coresight-trace-id.h
+++ b/drivers/hwtracing/coresight/coresight-trace-id.h
@@ -118,9 +118,12 @@ int coresight_trace_id_read_cpu_id(int cpu);
*
* Used to allocate IDs for system trace sources such as STM.
*
+ * @id: Preferred id value. If id is 0, get a free id from id map. If id is greater
+ * than 0, get a preferred id.
+ *
* return: Trace ID or -EINVAL if allocation is impossible.
*/
-int coresight_trace_id_get_system_id(void);
+int coresight_trace_id_get_system_id(int id);

/**
* Release an allocated system trace ID.
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index f09ace92176e..0599303be326 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -643,6 +643,7 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);

extern int coresight_get_cpu(struct device *dev);
+extern int coresight_get_source_traceid(struct device *dev, u32 *id);

struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
struct coresight_connection *
--
2.41.0

2024-06-03 10:06:02

by James Clark

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] coresight: Add support to get preferred id for system trace sources

On 03/06/2024 10:43, Mao Jinlong wrote:
> Dynamic trace id was introduced in coresight subsystem, so trace id is
> allocated dynamically. However, some hardware ATB source has static trace
> id and it cannot be changed via software programming. For such source,
> it can call coresight_get_source_traceid to get the fixed trace id from
> device node and pass id to coresight_trace_id_get_system_id to reserve
> the id.
>
> Signed-off-by: Mao Jinlong <[email protected]>
> ---
> .../hwtracing/coresight/coresight-platform.c | 26 +++++++++++++++
> drivers/hwtracing/coresight/coresight-stm.c | 2 +-
> .../hwtracing/coresight/coresight-trace-id.c | 32 ++++++++++++-------
> .../hwtracing/coresight/coresight-trace-id.h | 5 ++-
> include/linux/coresight.h | 1 +
> 5 files changed, 53 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 9d550f5697fa..8dd3cbd676b8 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -183,6 +183,17 @@ static int of_coresight_get_cpu(struct device *dev)
> return cpu;
> }
>
> +/*
> + * of_coresight_get_trace_id: Get the atid of a source device.
> + *
> + * Returns 0 on success.
> + */
> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
> +{
> +
> + return of_property_read_u32(dev->of_node, "trace-id", id);
> +}
> +
> /*
> * of_coresight_parse_endpoint : Parse the given output endpoint @ep
> * and fill the connection information in @pdata->out_conns
> @@ -782,6 +793,12 @@ static inline int acpi_coresight_get_cpu(struct device *dev)
> {
> return -ENODEV;
> }
> +
> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
> +{
> + return -ENODEV;
> +}
> +
> #endif
>
> int coresight_get_cpu(struct device *dev)
> @@ -794,6 +811,15 @@ int coresight_get_cpu(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(coresight_get_cpu);
>
> +int coresight_get_source_traceid(struct device *dev, u32 *id)
> +{
> + if (!is_of_node(dev->fwnode))
> + return -EINVAL;
> +
> + return of_coresight_get_trace_id(dev, id);
> +}
> +EXPORT_SYMBOL_GPL(coresight_get_source_traceid);
> +
> struct coresight_platform_data *
> coresight_get_platform_data(struct device *dev)
> {
> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> index e1c62820dfda..802f9e4ae570 100644
> --- a/drivers/hwtracing/coresight/coresight-stm.c
> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> @@ -901,7 +901,7 @@ static int __stm_probe(struct device *dev, struct resource *res)
> goto stm_unregister;
> }
>
> - trace_id = coresight_trace_id_get_system_id();
> + trace_id = coresight_trace_id_get_system_id(0);

I would #define 0 to "TRACE_ID_ANY" or something like that so it's
obvious what it means.

> if (trace_id < 0) {
> ret = trace_id;
> goto cs_unregister;
> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
> index af5b4ef59cea..5c25c75a2f08 100644
> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
> @@ -75,20 +75,23 @@ static int coresight_trace_id_find_odd_id(struct coresight_trace_id_map *id_map)
> * Allocate new ID and set in use
> *
> * if @preferred_id is a valid id then try to use that value if available.
> + * if @only_preferred is true, if @preferred_id is used, return error EINVAL.

if @only_preferred is true @preferred_id must be free, otherwise return
error -EINVAL

> * if @preferred_id is not valid and @prefer_odd_id is true, try for odd id.
> *
> * Otherwise allocate next available ID.
> */
> static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map,
> - int preferred_id, bool prefer_odd_id)
> + int preferred_id, bool prefer_odd_id, bool only_preferred)
> {
> int id = 0;
>
> /* for backwards compatibility, cpu IDs may use preferred value */
> - if (IS_VALID_CS_TRACE_ID(preferred_id) &&
> - !test_bit(preferred_id, id_map->used_ids)) {
> - id = preferred_id;
> - goto trace_id_allocated;
> + if (IS_VALID_CS_TRACE_ID(preferred_id)) {
> + if (!test_bit(preferred_id, id_map->used_ids)) {
> + id = preferred_id;
> + goto trace_id_allocated;
> + } else if (WARN(only_preferred, "Trace ID %d is used.\n", preferred_id))
> + return -EINVAL;
> } else if (prefer_odd_id) {
> /* may use odd ids to avoid preferred legacy cpu IDs */
> id = coresight_trace_id_find_odd_id(id_map);
> @@ -175,7 +178,7 @@ static int coresight_trace_id_map_get_cpu_id(int cpu, struct coresight_trace_id_
> */
> id = coresight_trace_id_alloc_new_id(id_map,
> CORESIGHT_LEGACY_CPU_TRACE_ID(cpu),
> - false);
> + false, false);
> if (!IS_VALID_CS_TRACE_ID(id))
> goto get_cpu_id_out_unlock;
>
> @@ -222,14 +225,21 @@ static void coresight_trace_id_map_put_cpu_id(int cpu, struct coresight_trace_id
> DUMP_ID_MAP(id_map);
> }
>
> -static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map)
> +static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map,
> + int preferred_id, bool only_preferred)

Looks like "only_preferred" was left on here by mistake and it's not
used. For this function preferred_id != 0 means the same.

> {
> unsigned long flags;
> int id;
>
> spin_lock_irqsave(&id_map_lock, flags);
> - /* prefer odd IDs for system components to avoid legacy CPU IDS */
> - id = coresight_trace_id_alloc_new_id(id_map, 0, true);
> +
> + if (preferred_id > 0) {
> + /* use preferred id if it is available */
> + id = coresight_trace_id_alloc_new_id(id_map, preferred_id, false, true);
> + } else {
> + /* prefer odd IDs for system components to avoid legacy CPU IDS */
> + id = coresight_trace_id_alloc_new_id(id_map, 0, true, false);

prefer_odd_id is different in each of these calls which I think is a
mistake?

You could do one function call to avoid that:

only_preferred = preferred_id > 0
coresight_trace_id_alloc_new_id(id_map, preferred_id, true,
only_preferred);

> + }
> spin_unlock_irqrestore(&id_map_lock, flags);
>
> DUMP_ID(id);
> @@ -269,9 +279,9 @@ int coresight_trace_id_read_cpu_id(int cpu)
> }
> EXPORT_SYMBOL_GPL(coresight_trace_id_read_cpu_id);
>
> -int coresight_trace_id_get_system_id(void)
> +int coresight_trace_id_get_system_id(int id)
> {
> - return coresight_trace_id_map_get_system_id(&id_map_default);
> + return coresight_trace_id_map_get_system_id(&id_map_default, id, true);
> }
> EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);
>
> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h
> index 3797777d367e..9189a33c5857 100644
> --- a/drivers/hwtracing/coresight/coresight-trace-id.h
> +++ b/drivers/hwtracing/coresight/coresight-trace-id.h
> @@ -118,9 +118,12 @@ int coresight_trace_id_read_cpu_id(int cpu);
> *
> * Used to allocate IDs for system trace sources such as STM.
> *
> + * @id: Preferred id value. If id is 0, get a free id from id map. If id is greater

0 -> TRACE_ID_ANY

> + * than 0, get a preferred id.
> + *
> * return: Trace ID or -EINVAL if allocation is impossible.
> */
> -int coresight_trace_id_get_system_id(void);
> +int coresight_trace_id_get_system_id(int id);
>
> /**
> * Release an allocated system trace ID.
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index f09ace92176e..0599303be326 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -643,6 +643,7 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
> void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
>
> extern int coresight_get_cpu(struct device *dev);
> +extern int coresight_get_source_traceid(struct device *dev, u32 *id);
>
> struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
> struct coresight_connection *

2024-06-03 10:15:24

by James Clark

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] coresight: Add support to get preferred id for system trace sources

On 03/06/2024 11:04, James Clark wrote:
>
>
> On 03/06/2024 10:43, Mao Jinlong wrote:
>> Dynamic trace id was introduced in coresight subsystem, so trace id is
>> allocated dynamically. However, some hardware ATB source has static trace
>> id and it cannot be changed via software programming. For such source,
>> it can call coresight_get_source_traceid to get the fixed trace id from
>> device node and pass id to coresight_trace_id_get_system_id to reserve
>> the id.
>>
>> Signed-off-by: Mao Jinlong <[email protected]>
>> ---
>> .../hwtracing/coresight/coresight-platform.c | 26 +++++++++++++++
>> drivers/hwtracing/coresight/coresight-stm.c | 2 +-
>> .../hwtracing/coresight/coresight-trace-id.c | 32 ++++++++++++-------
>> .../hwtracing/coresight/coresight-trace-id.h | 5 ++-
>> include/linux/coresight.h | 1 +
>> 5 files changed, 53 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>> index 9d550f5697fa..8dd3cbd676b8 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -183,6 +183,17 @@ static int of_coresight_get_cpu(struct device *dev)
>> return cpu;
>> }
>>
>> +/*
>> + * of_coresight_get_trace_id: Get the atid of a source device.
>> + *
>> + * Returns 0 on success.
>> + */
>> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
>> +{
>> +
>> + return of_property_read_u32(dev->of_node, "trace-id", id);
>> +}
>> +
>> /*
>> * of_coresight_parse_endpoint : Parse the given output endpoint @ep
>> * and fill the connection information in @pdata->out_conns
>> @@ -782,6 +793,12 @@ static inline int acpi_coresight_get_cpu(struct device *dev)
>> {
>> return -ENODEV;
>> }
>> +
>> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> #endif
>>
>> int coresight_get_cpu(struct device *dev)
>> @@ -794,6 +811,15 @@ int coresight_get_cpu(struct device *dev)
>> }
>> EXPORT_SYMBOL_GPL(coresight_get_cpu);
>>
>> +int coresight_get_source_traceid(struct device *dev, u32 *id)
>> +{
>> + if (!is_of_node(dev->fwnode))
>> + return -EINVAL;
>> +
>> + return of_coresight_get_trace_id(dev, id);
>> +}
>> +EXPORT_SYMBOL_GPL(coresight_get_source_traceid);
>> +
>> struct coresight_platform_data *
>> coresight_get_platform_data(struct device *dev)
>> {
>> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
>> index e1c62820dfda..802f9e4ae570 100644
>> --- a/drivers/hwtracing/coresight/coresight-stm.c
>> +++ b/drivers/hwtracing/coresight/coresight-stm.c
>> @@ -901,7 +901,7 @@ static int __stm_probe(struct device *dev, struct resource *res)
>> goto stm_unregister;
>> }
>>
>> - trace_id = coresight_trace_id_get_system_id();
>> + trace_id = coresight_trace_id_get_system_id(0);
>
> I would #define 0 to "TRACE_ID_ANY" or something like that so it's
> obvious what it means.
>
>> if (trace_id < 0) {
>> ret = trace_id;
>> goto cs_unregister;
>> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
>> index af5b4ef59cea..5c25c75a2f08 100644
>> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
>> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
>> @@ -75,20 +75,23 @@ static int coresight_trace_id_find_odd_id(struct coresight_trace_id_map *id_map)
>> * Allocate new ID and set in use
>> *
>> * if @preferred_id is a valid id then try to use that value if available.
>> + * if @only_preferred is true, if @preferred_id is used, return error EINVAL.
>
> if @only_preferred is true @preferred_id must be free, otherwise return
> error -EINVAL
>
>> * if @preferred_id is not valid and @prefer_odd_id is true, try for odd id.
>> *
>> * Otherwise allocate next available ID.
>> */
>> static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map,
>> - int preferred_id, bool prefer_odd_id)
>> + int preferred_id, bool prefer_odd_id, bool only_preferred)
>> {
>> int id = 0;
>>
>> /* for backwards compatibility, cpu IDs may use preferred value */
>> - if (IS_VALID_CS_TRACE_ID(preferred_id) &&
>> - !test_bit(preferred_id, id_map->used_ids)) {
>> - id = preferred_id;
>> - goto trace_id_allocated;
>> + if (IS_VALID_CS_TRACE_ID(preferred_id)) {
>> + if (!test_bit(preferred_id, id_map->used_ids)) {
>> + id = preferred_id;
>> + goto trace_id_allocated;
>> + } else if (WARN(only_preferred, "Trace ID %d is used.\n", preferred_id))
>> + return -EINVAL;
>> } else if (prefer_odd_id) {
>> /* may use odd ids to avoid preferred legacy cpu IDs */
>> id = coresight_trace_id_find_odd_id(id_map);
>> @@ -175,7 +178,7 @@ static int coresight_trace_id_map_get_cpu_id(int cpu, struct coresight_trace_id_
>> */
>> id = coresight_trace_id_alloc_new_id(id_map,
>> CORESIGHT_LEGACY_CPU_TRACE_ID(cpu),
>> - false);
>> + false, false);
>> if (!IS_VALID_CS_TRACE_ID(id))
>> goto get_cpu_id_out_unlock;
>>
>> @@ -222,14 +225,21 @@ static void coresight_trace_id_map_put_cpu_id(int cpu, struct coresight_trace_id
>> DUMP_ID_MAP(id_map);
>> }
>>
>> -static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map)
>> +static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map,
>> + int preferred_id, bool only_preferred)
>
> Looks like "only_preferred" was left on here by mistake and it's not
> used. For this function preferred_id != 0 means the same.
>
>> {
>> unsigned long flags;
>> int id;
>>
>> spin_lock_irqsave(&id_map_lock, flags);
>> - /* prefer odd IDs for system components to avoid legacy CPU IDS */
>> - id = coresight_trace_id_alloc_new_id(id_map, 0, true);
>> +
>> + if (preferred_id > 0) {
>> + /* use preferred id if it is available */
>> + id = coresight_trace_id_alloc_new_id(id_map, preferred_id, false, true);
>> + } else {
>> + /* prefer odd IDs for system components to avoid legacy CPU IDS */
>> + id = coresight_trace_id_alloc_new_id(id_map, 0, true, false);
>
> prefer_odd_id is different in each of these calls which I think is a
> mistake?
>
> You could do one function call to avoid that:
>
> only_preferred = preferred_id > 0
> coresight_trace_id_alloc_new_id(id_map, preferred_id, true,
> only_preferred);
>
>

Actually, even better we can have an "int flags" argument rather than
having to work out what a load of booleans mean. And then use these:

TRACE_ID_WANT_ODD | TRACE_ID_WANT_PREFERRED

flags = TRACE_ID_WANT_ODD
flags |= preferred_id > 0 ? TRACE_ID_WANT_PREFERRED : 0;
coresight_trace_id_alloc_new_id(id_map, preferred_id, flags);

>> + }
>> spin_unlock_irqrestore(&id_map_lock, flags);
>>
>> DUMP_ID(id);
>> @@ -269,9 +279,9 @@ int coresight_trace_id_read_cpu_id(int cpu)
>> }
>> EXPORT_SYMBOL_GPL(coresight_trace_id_read_cpu_id);
>>
>> -int coresight_trace_id_get_system_id(void)
>> +int coresight_trace_id_get_system_id(int id)
>> {
>> - return coresight_trace_id_map_get_system_id(&id_map_default);
>> + return coresight_trace_id_map_get_system_id(&id_map_default, id, true);
>> }
>> EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h
>> index 3797777d367e..9189a33c5857 100644
>> --- a/drivers/hwtracing/coresight/coresight-trace-id.h
>> +++ b/drivers/hwtracing/coresight/coresight-trace-id.h
>> @@ -118,9 +118,12 @@ int coresight_trace_id_read_cpu_id(int cpu);
>> *
>> * Used to allocate IDs for system trace sources such as STM.
>> *
>> + * @id: Preferred id value. If id is 0, get a free id from id map. If id is greater
>
> 0 -> TRACE_ID_ANY
>
>> + * than 0, get a preferred id.
>> + *
>> * return: Trace ID or -EINVAL if allocation is impossible.
>> */
>> -int coresight_trace_id_get_system_id(void);
>> +int coresight_trace_id_get_system_id(int id);
>>
>> /**
>> * Release an allocated system trace ID.
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index f09ace92176e..0599303be326 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -643,6 +643,7 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
>> void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
>>
>> extern int coresight_get_cpu(struct device *dev);
>> +extern int coresight_get_source_traceid(struct device *dev, u32 *id);
>>
>> struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
>> struct coresight_connection *

2024-06-03 10:24:34

by James Clark

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] coresight: dummy: Add reserve atid support for dummy source

On 03/06/2024 10:43, Mao Jinlong wrote:
> Some dummy source has static trace id configured in HW and it cannot
> be changed via software programming. Configure the trace id in device
> tree and reserve the id when device probe.
>
> Signed-off-by: Mao Jinlong <[email protected]>
> ---
> .../sysfs-bus-coresight-devices-dummy-source | 15 +++++
> drivers/hwtracing/coresight/coresight-dummy.c | 58 +++++++++++++++++--
> 2 files changed, 69 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
> new file mode 100644
> index 000000000000..d93c198115c9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
> @@ -0,0 +1,15 @@
> +What: /sys/bus/coresight/devices/dummy_source<N>/enable_source
> +Date: June 2024
> +KernelVersion: 6.9
> +Contact: Mao Jinlong <[email protected]>
> +Description: (RW) Enable/disable tracing of dummy source. A sink should be activated
> + before enabling the source. The path of coresight components linking
> + the source to the sink is configured and managed automatically by the
> + coresight framework.
> +
> +What: /sys/bus/coresight/devices/dummy_source<N>/traceid
> +Date: June 2024
> +KernelVersion: 6.9
> +Contact: Mao Jinlong <[email protected]>
> +Description: (R) Show the trace ID that will appear in the trace stream
> + coming from this trace entity.
> diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
> index ac70c0b491be..1f7133ac2c0b 100644
> --- a/drivers/hwtracing/coresight/coresight-dummy.c
> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> @@ -11,10 +11,12 @@
> #include <linux/pm_runtime.h>
>
> #include "coresight-priv.h"
> +#include "coresight-trace-id.h"
>
> struct dummy_drvdata {
> struct device *dev;
> struct coresight_device *csdev;
> + u8 traceid;
> };
>
> DEFINE_CORESIGHT_DEVLIST(source_devs, "dummy_source");
> @@ -67,6 +69,32 @@ static const struct coresight_ops dummy_sink_cs_ops = {
> .sink_ops = &dummy_sink_ops,
> };
>
> +/* User can get the trace id of dummy source from this node. */
> +static ssize_t traceid_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + unsigned long val;
> + struct dummy_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> + val = drvdata->traceid;
> + return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
> +}
> +static DEVICE_ATTR_RO(traceid);
> +
> +static struct attribute *coresight_dummy_attrs[] = {
> + &dev_attr_traceid.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group coresight_dummy_group = {
> + .attrs = coresight_dummy_attrs,
> +};
> +
> +static const struct attribute_group *coresight_dummy_groups[] = {
> + &coresight_dummy_group,
> + NULL,
> +};
> +
> static int dummy_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -74,6 +102,11 @@ static int dummy_probe(struct platform_device *pdev)
> struct coresight_platform_data *pdata;
> struct dummy_drvdata *drvdata;
> struct coresight_desc desc = { 0 };
> + int ret, trace_id;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
>
> if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
>
> @@ -85,6 +118,24 @@ static int dummy_probe(struct platform_device *pdev)
> desc.subtype.source_subtype =
> CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
> desc.ops = &dummy_source_cs_ops;
> + desc.groups = coresight_dummy_groups;
> +
> + ret = coresight_get_source_traceid(dev, &trace_id);
> + if (!ret) {
> + ret = coresight_trace_id_get_system_id(trace_id);
> + if (ret < 0)
> + return ret;
> +
> + drvdata->traceid = ret;
> + } else {
> + trace_id = coresight_trace_id_get_system_id(0);
> + if (trace_id < 0) {
> + ret = trace_id;
> + return ret;
> + }
> + drvdata->traceid = (u8)trace_id;
> + }
> +

It's a bit weird to use ret in one version and trace_id in the other.
And one casts to (u8) but the other doesn't. I'm sure it all compiles to
the same in the end but it would be a bit more readable if it was more
consistent.

> } else if (of_device_is_compatible(node, "arm,coresight-dummy-sink")) {
> desc.name = coresight_alloc_device_name(&sink_devs, dev);
> if (!desc.name)
> @@ -103,10 +154,6 @@ static int dummy_probe(struct platform_device *pdev)
> return PTR_ERR(pdata);
> pdev->dev.platform_data = pdata;
>
> - drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> - if (!drvdata)
> - return -ENOMEM;
> -
> drvdata->dev = &pdev->dev;
> platform_set_drvdata(pdev, drvdata);
>
> @@ -126,7 +173,10 @@ static void dummy_remove(struct platform_device *pdev)
> {
> struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
> struct device *dev = &pdev->dev;
> + struct device_node *node = dev->of_node;
>
> + if (of_device_is_compatible(node, "arm,coresight-dummy-source"))
> + coresight_trace_id_put_system_id(drvdata->traceid);
> pm_runtime_disable(dev);
> coresight_unregister(drvdata->csdev);
> }

2024-06-03 15:18:29

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] coresight: Add support to get preferred id for system trace sources

2024-06-04 02:04:23

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] coresight: Add support to get preferred id for system trace sources

Hi Mao,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on atorgue-stm32/stm32-next linus/master v6.10-rc2 next-20240603]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Mao-Jinlong/dt-bindings-arm-Add-trace-id-for-coresight-dummy-source/20240603-175023
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240603094354.2348-3-quic_jinlmao%40quicinc.com
patch subject: [PATCH v2 2/3] coresight: Add support to get preferred id for system trace sources
config: arm64-randconfig-001-20240604 (https://download.01.org/0day-ci/archive/20240604/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d7d2d4f53fc79b4b58e8d8d08151b577c3699d4a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240604/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from drivers/hwtracing/coresight/coresight-tpda.c:6:
In file included from include/linux/amba/bus.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
500 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
501 | item];
| ~~~~
include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
507 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
508 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
519 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
520 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
528 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
529 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/hwtracing/coresight/coresight-tpda.c:254:42: error: too few arguments to function call, single argument 'id' was not specified
254 | atid = coresight_trace_id_get_system_id();
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
drivers/hwtracing/coresight/coresight-trace-id.h:126:5: note: 'coresight_trace_id_get_system_id' declared here
126 | int coresight_trace_id_get_system_id(int id);
| ^ ~~~~~~
5 warnings and 1 error generated.

vim +/id +254 drivers/hwtracing/coresight/coresight-tpda.c

5b7916625c017e Mao Jinlong 2023-01-17 243
5b7916625c017e Mao Jinlong 2023-01-17 244 static int tpda_init_default_data(struct tpda_drvdata *drvdata)
5b7916625c017e Mao Jinlong 2023-01-17 245 {
5b7916625c017e Mao Jinlong 2023-01-17 246 int atid;
5b7916625c017e Mao Jinlong 2023-01-17 247 /*
5b7916625c017e Mao Jinlong 2023-01-17 248 * TPDA must has a unique atid. This atid can uniquely
5b7916625c017e Mao Jinlong 2023-01-17 249 * identify the TPDM trace source connected to the TPDA.
5b7916625c017e Mao Jinlong 2023-01-17 250 * The TPDMs which are connected to same TPDA share the
5b7916625c017e Mao Jinlong 2023-01-17 251 * same trace-id. When TPDA does packetization, different
5b7916625c017e Mao Jinlong 2023-01-17 252 * port will have unique channel number for decoding.
5b7916625c017e Mao Jinlong 2023-01-17 253 */
5b7916625c017e Mao Jinlong 2023-01-17 @254 atid = coresight_trace_id_get_system_id();
5b7916625c017e Mao Jinlong 2023-01-17 255 if (atid < 0)
5b7916625c017e Mao Jinlong 2023-01-17 256 return atid;
5b7916625c017e Mao Jinlong 2023-01-17 257
5b7916625c017e Mao Jinlong 2023-01-17 258 drvdata->atid = atid;
5b7916625c017e Mao Jinlong 2023-01-17 259 return 0;
5b7916625c017e Mao Jinlong 2023-01-17 260 }
5b7916625c017e Mao Jinlong 2023-01-17 261

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki