1. Patchew
  2. linux
  3. View series

[PATCH] coresight: Fix data argument to coresight_enable_helpers

Carl Worth posted 1 patch 4 months, 3 weeks ago 
drivers/hwtracing/coresight/coresight-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] coresight: Fix data argument to coresight_enable_helpers
Posted by Carl Worth 4 months, 3 weeks ago 
In the commit being fixed, coresight_enable_path() was changed to call
coresight_enable_helpers() with a final argument of 'path' rather than
'sink_data'. This was invalid, resulting in derefencing a pointer to
a 'struct coresight_path' as if it were a 'struct perf_output_handle'.

The compiler could not flag the error since there are several layers
of function calls treating the pointer as void*.

Fix to correctly pass the sink_data pointer as the final argument to
coresight_enable_helpers(), exactly as it was before the buggy commit.

Bug can be reproduced with:

$ perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null

Showing an oops as follows:

[   88.696535] Unable to handle kernel paging request at virtual address 000f6e84934ed19e
...
[   88.911293] Call trace:
[   88.913728]  tmc_etr_get_buffer+0x30/0x80 [coresight_tmc] (P)
[   88.919463]  catu_enable_hw+0xbc/0x3d0 [coresight_catu]
[   88.924677]  catu_enable+0x70/0xe0 [coresight_catu]
[   88.929542]  coresight_enable_path+0xb0/0x258 [coresight]

Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path")
Signed-off-by: Carl Worth <carl@os.amperecomputing.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index fa758cc21827..b1077d73c988 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -510,7 +510,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 		type = csdev->type;
 
 		/* Enable all helpers adjacent to the path first */
-		ret = coresight_enable_helpers(csdev, mode, path);
+		ret = coresight_enable_helpers(csdev, mode, sink_data);
 		if (ret)
 			goto err_disable_path;
 		/*
-- 
2.39.5
Re: [PATCH] coresight: Fix data argument to coresight_enable_helpers
Posted by Jie Gan 4 months, 3 weeks ago 

On 9/17/2025 6:44 AM, Carl Worth wrote:
> In the commit being fixed, coresight_enable_path() was changed to call
> coresight_enable_helpers() with a final argument of 'path' rather than
> 'sink_data'. This was invalid, resulting in derefencing a pointer to
> a 'struct coresight_path' as if it were a 'struct perf_output_handle'.
> 
> The compiler could not flag the error since there are several layers
> of function calls treating the pointer as void*.
> 
> Fix to correctly pass the sink_data pointer as the final argument to
> coresight_enable_helpers(), exactly as it was before the buggy commit.
> 
> Bug can be reproduced with:
> 
> $ perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null
> 
> Showing an oops as follows:
> 
> [   88.696535] Unable to handle kernel paging request at virtual address 000f6e84934ed19e
> ...
> [   88.911293] Call trace:
> [   88.913728]  tmc_etr_get_buffer+0x30/0x80 [coresight_tmc] (P)
> [   88.919463]  catu_enable_hw+0xbc/0x3d0 [coresight_catu]
> [   88.924677]  catu_enable+0x70/0xe0 [coresight_catu]
> [   88.929542]  coresight_enable_path+0xb0/0x258 [coresight]
> 
> Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path")
> Signed-off-by: Carl Worth <carl@os.amperecomputing.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fa758cc21827..b1077d73c988 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -510,7 +510,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>   		type = csdev->type;
>   
>   		/* Enable all helpers adjacent to the path first */
> -		ret = coresight_enable_helpers(csdev, mode, path);
> +		ret = coresight_enable_helpers(csdev, mode, sink_data);

I dont think we can change back to sink_data since we introduced 
coresight_path to wrap 'data' which is needed by the path.

I suggest you to add the struct perf_output_handle to the 
coresight_path, then retrieving it with data->perf_handle in 
tmc_etr_get_buffer.

before:
struct perf_output_handle *handle = data;

after:
struct coresight_path *path = data;
struct perf_output_handle *handle = path->perf_handle;

We can assign the perf_output_handle to the coresight_path after we 
constructed the coresight_path in perf mode.


Thanks,
Jie

>   		if (ret)
>   			goto err_disable_path;
>   		/*
Re: [PATCH] coresight: Fix data argument to coresight_enable_helpers
Posted by Carl Worth 4 months, 3 weeks ago 
Jie Gan <jie.gan@oss.qualcomm.com> writes:
> I dont think we can change back to sink_data since we introduced 
> coresight_path to wrap 'data' which is needed by the path.
>
> I suggest you to add the struct perf_output_handle to the 
> coresight_path, then retrieving it with data->perf_handle in 
> tmc_etr_get_buffer.
...
> We can assign the perf_output_handle to the coresight_path after we 
> constructed the coresight_path in perf mode.

Thanks. That much makes sense to me, and I'll put together a patch along
those lines.

But, further: with core coresight code assembling into the path all the
data that is necessary, is there any reason to be using void* in these
enable/disable functions?

Could we also change these functions to accept a coresight_path* and
actually get some compiler help at finding mistakes like the one we're
fixing here?

-Carl
Re: [PATCH] coresight: Fix data argument to coresight_enable_helpers
Posted by Jie Gan 4 months, 3 weeks ago 

On 9/19/2025 6:18 AM, Carl Worth wrote:
> Jie Gan <jie.gan@oss.qualcomm.com> writes:
>> I dont think we can change back to sink_data since we introduced
>> coresight_path to wrap 'data' which is needed by the path.
>>
>> I suggest you to add the struct perf_output_handle to the
>> coresight_path, then retrieving it with data->perf_handle in
>> tmc_etr_get_buffer.
> ...
>> We can assign the perf_output_handle to the coresight_path after we
>> constructed the coresight_path in perf mode.
> 
> Thanks. That much makes sense to me, and I'll put together a patch along
> those lines.
> 
> But, further: with core coresight code assembling into the path all the
> data that is necessary, is there any reason to be using void* in these
> enable/disable functions?

In my opinion, yes, we can change void * to coresight_path * for 
helper's enable/disable functions since we have everything in path so 
the cast is not necessary now.

> 
> Could we also change these functions to accept a coresight_path* and
> actually get some compiler help at finding mistakes like the one we're
> fixing here?

That's the only benefit in my mind so far.

Thanks,
Jie

> 
> -Carl
Re: [PATCH] coresight: Fix data argument to coresight_enable_helpers
Posted by Suzuki K Poulose 4 months, 3 weeks ago 
On 19/09/2025 02:20, Jie Gan wrote:
> 
> 
> On 9/19/2025 6:18 AM, Carl Worth wrote:
>> Jie Gan <jie.gan@oss.qualcomm.com> writes:
>>> I dont think we can change back to sink_data since we introduced
>>> coresight_path to wrap 'data' which is needed by the path.
>>>
>>> I suggest you to add the struct perf_output_handle to the
>>> coresight_path, then retrieving it with data->perf_handle in
>>> tmc_etr_get_buffer.
>> ...
>>> We can assign the perf_output_handle to the coresight_path after we
>>> constructed the coresight_path in perf mode.
>>
>> Thanks. That much makes sense to me, and I'll put together a patch along
>> those lines.
>>
>> But, further: with core coresight code assembling into the path all the
>> data that is necessary, is there any reason to be using void* in these
>> enable/disable functions?
> 
> In my opinion, yes, we can change void * to coresight_path * for 
> helper's enable/disable functions since we have everything in path so 
> the cast is not necessary now.
> 
>>
>> Could we also change these functions to accept a coresight_path* and
>> actually get some compiler help at finding mistakes like the one we're
>> fixing here?
> 


Yes, please. I was going to suggest that. May be we could do that as
a separate patch after fixing the problem here first (so that it
can be back ported).

This was initially a perf_handle only used for the perf mode, and
it didn't make sens to have a "perf" argument to "enable" which
could be used for both sysfs and perf. Now that the path
is a generic data structure, it makes sense to move everything
to accept the path.

Suzuki


> That's the only benefit in my mind so far.
> 
> Thanks,
> Jie
> 
>>
>> -Carl
>
Re: [PATCH] coresight: Fix data argument to coresight_enable_helpers
Posted by Carl Worth 4 months, 2 weeks ago 
Suzuki K Poulose <suzuki.poulose@arm.com> writes:
> Yes, please. I was going to suggest that. May be we could do that as
> a separate patch after fixing the problem here first (so that it
> can be back ported).

Hi, Suzuki,

I saw this suggestion after I had put together my v2 version of the
patch, where I split the path and handle into separate arguments and
also got rid of void* in a single patch. With that approach I think it
only makes sense to do them together.

But if we instead were to make everything work with a single path
argument, then I agree it would have made sense to change the type in a
separate patch.

> This was initially a perf_handle only used for the perf mode, and
> it didn't make sens to have a "perf" argument to "enable" which
> could be used for both sysfs and perf. Now that the path
> is a generic data structure, it makes sense to move everything
> to accept the path.

I'm fairly new to the entire coresight subsystem, so I might be getting
this wrong. It looks to me like the perf handle is really part of the
event so wouldn't logically make sense as part of the path? Am I
understanding that correctly?

-Carl
[PATCH v2] coresight: Fix data arguments to coresight enable/disable helpers
Posted by Carl Worth 4 months, 2 weeks ago 
In the commit being fixed, coresight_enable_path() was changed to call
coresight_enable_helpers() by passing a struct coresight_path*
as the final void* 'path' argument, rather passing 'sink_data'
as was being passed previously. This set the groundwork for the
subsequent addition of the ctcu_enable function which interprets
void* data as a struct coresight_path*, but it also broke the
existing catu_enable function which interprets void* data
as a struct perf_output_handle*.

The compiler could not flag the error since there are several layers
of function calls treating the pointer as void*.

Fix both users simultaneously by adding an additional argument to the
enable helper interface. So now both the struct coresight_path and the
struct perf_output_handle pointers are passed. Also, eliminate all
usages of the void* from these code paths and use explicit types
instead to make all of this code more robust.

Note: The disable function is also changed from void* to
struct coresight_path* but no implementations of this function
need the struct perf_output_handle* so it is not added to the interface.

The existing bug can be reproduced with:

# perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null

Showing an oops as follows:

Unable to handle kernel paging request at virtual address 000f6e84934ed19e

Call trace:
 tmc_etr_get_buffer+0x30/0x80 [coresight_tmc] (P)
 catu_enable_hw+0xbc/0x3d0 [coresight_catu]
 catu_enable+0x70/0xe0 [coresight_catu]
 coresight_enable_path+0xb0/0x258 [coresight]

Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path")
Signed-off-by: Carl Worth <carl@os.amperecomputing.com>
---

Jie, I looked into your suggestion of adding the struct
perf_output_handle* to the struct coresight_path, but I couldn't
justify adding event-related data to the path structure, which has
nothing to do with it.

So, instead I took the approach in this patch to plumb both arguments
through the enable path, (and changed away from void* as you agreed
to).

Note: I've tested that this fixes the bug for catu_enable. This patch
also obviously touches ctcu_enable, which I believe is correct, but I
have not tested since I don't have ready access to the necessary
hardware. I will appreciate other testing.

-Carl

 drivers/hwtracing/coresight/coresight-catu.c  | 12 ++++++----
 drivers/hwtracing/coresight/coresight-core.c  | 23 +++++++++++--------
 .../hwtracing/coresight/coresight-ctcu-core.c | 11 ++++-----
 .../hwtracing/coresight/coresight-cti-core.c  |  7 ++++--
 .../hwtracing/coresight/coresight-cti-sysfs.c |  2 +-
 drivers/hwtracing/coresight/coresight-cti.h   |  6 +++--
 drivers/hwtracing/coresight/coresight-priv.h  |  2 +-
 .../hwtracing/coresight/coresight-tmc-etr.c   |  4 ++--
 drivers/hwtracing/coresight/coresight-tmc.h   |  3 ++-
 include/linux/coresight.h                     |  6 +++--
 10 files changed, 45 insertions(+), 31 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index 5058432233da..724c25d0afa4 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -397,7 +397,7 @@ static int catu_wait_for_ready(struct catu_drvdata *drvdata)
 }
 
 static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
-			  void *data)
+			  struct perf_output_handle *handle)
 {
 	int rc;
 	u32 control, mode;
@@ -425,7 +425,7 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
 	etrdev = coresight_find_input_type(
 		csdev->pdata, CORESIGHT_DEV_TYPE_SINK, etr_subtype);
 	if (etrdev) {
-		etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data);
+		etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, handle);
 		if (IS_ERR(etr_buf))
 			return PTR_ERR(etr_buf);
 	}
@@ -455,7 +455,8 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
 }
 
 static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
-		       void *data)
+		       struct coresight_path *path,
+		       struct perf_output_handle *handle)
 {
 	int rc = 0;
 	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
@@ -463,7 +464,7 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
 	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
 	if (csdev->refcnt == 0) {
 		CS_UNLOCK(catu_drvdata->base);
-		rc = catu_enable_hw(catu_drvdata, mode, data);
+		rc = catu_enable_hw(catu_drvdata, mode, handle);
 		CS_LOCK(catu_drvdata->base);
 	}
 	if (!rc)
@@ -488,7 +489,8 @@ static int catu_disable_hw(struct catu_drvdata *drvdata)
 	return rc;
 }
 
-static int catu_disable(struct coresight_device *csdev, void *__unused)
+static int catu_disable(struct coresight_device *csdev,
+			struct coresight_path *__unused)
 {
 	int rc = 0;
 	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index fa758cc21827..cc449d5196a4 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -353,14 +353,17 @@ static bool coresight_is_helper(struct coresight_device *csdev)
 }
 
 static int coresight_enable_helper(struct coresight_device *csdev,
-				   enum cs_mode mode, void *data)
+				   enum cs_mode mode,
+				   struct coresight_path *path,
+				   struct perf_output_handle *handle)
 {
-	return helper_ops(csdev)->enable(csdev, mode, data);
+	return helper_ops(csdev)->enable(csdev, mode, path, handle);
 }
 
-static void coresight_disable_helper(struct coresight_device *csdev, void *data)
+static void coresight_disable_helper(struct coresight_device *csdev,
+				     struct coresight_path *path)
 {
-	helper_ops(csdev)->disable(csdev, data);
+	helper_ops(csdev)->disable(csdev, path);
 }
 
 static void coresight_disable_helpers(struct coresight_device *csdev, void *data)
@@ -477,7 +480,9 @@ void coresight_disable_path(struct coresight_path *path)
 EXPORT_SYMBOL_GPL(coresight_disable_path);
 
 static int coresight_enable_helpers(struct coresight_device *csdev,
-				    enum cs_mode mode, void *data)
+				    enum cs_mode mode,
+				    struct coresight_path *path,
+				    struct perf_output_handle *handle)
 {
 	int i, ret = 0;
 	struct coresight_device *helper;
@@ -487,7 +492,7 @@ static int coresight_enable_helpers(struct coresight_device *csdev,
 		if (!helper || !coresight_is_helper(helper))
 			continue;
 
-		ret = coresight_enable_helper(helper, mode, data);
+		ret = coresight_enable_helper(helper, mode, path, handle);
 		if (ret)
 			return ret;
 	}
@@ -496,7 +501,7 @@ static int coresight_enable_helpers(struct coresight_device *csdev,
 }
 
 int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
-			  void *sink_data)
+			  struct perf_output_handle *handle)
 {
 	int ret = 0;
 	u32 type;
@@ -510,7 +515,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 		type = csdev->type;
 
 		/* Enable all helpers adjacent to the path first */
-		ret = coresight_enable_helpers(csdev, mode, path);
+		ret = coresight_enable_helpers(csdev, mode, path, handle);
 		if (ret)
 			goto err_disable_path;
 		/*
@@ -526,7 +531,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 
 		switch (type) {
 		case CORESIGHT_DEV_TYPE_SINK:
-			ret = coresight_enable_sink(csdev, mode, sink_data);
+			ret = coresight_enable_sink(csdev, mode, handle);
 			/*
 			 * Sink is the first component turned on. If we
 			 * failed to enable the sink, there are no components
diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
index c6bafc96db96..9f6d71c59d4e 100644
--- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
+++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
@@ -156,17 +156,16 @@ static int ctcu_set_etr_traceid(struct coresight_device *csdev, struct coresight
 	return __ctcu_set_etr_traceid(csdev, traceid, port_num, enable);
 }
 
-static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode, void *data)
+static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode,
+		       struct coresight_path *path,
+		       struct perf_output_handle *handle)
 {
-	struct coresight_path *path = (struct coresight_path *)data;
-
 	return ctcu_set_etr_traceid(csdev, path, true);
 }
 
-static int ctcu_disable(struct coresight_device *csdev, void *data)
+static int ctcu_disable(struct coresight_device *csdev,
+			struct coresight_path *path)
 {
-	struct coresight_path *path = (struct coresight_path *)data;
-
 	return ctcu_set_etr_traceid(csdev, path, false);
 }
 
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
index 8fb30dd73fd2..f92e3be4607c 100644
--- a/drivers/hwtracing/coresight/coresight-cti-core.c
+++ b/drivers/hwtracing/coresight/coresight-cti-core.c
@@ -799,14 +799,17 @@ static void cti_pm_release(struct cti_drvdata *drvdata)
 }
 
 /** cti ect operations **/
-int cti_enable(struct coresight_device *csdev, enum cs_mode mode, void *data)
+int cti_enable(struct coresight_device *csdev, enum cs_mode mode,
+	       struct coresight_path *path,
+	       struct perf_output_handle *handle)
 {
 	struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
 
 	return cti_enable_hw(drvdata);
 }
 
-int cti_disable(struct coresight_device *csdev, void *data)
+int cti_disable(struct coresight_device *csdev,
+		struct coresight_path *path)
 {
 	struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
 
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
index 572b80ee96fb..2bb6929eeb19 100644
--- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
@@ -112,7 +112,7 @@ static ssize_t enable_store(struct device *dev,
 		ret = pm_runtime_resume_and_get(dev->parent);
 		if (ret)
 			return ret;
-		ret = cti_enable(drvdata->csdev, CS_MODE_SYSFS, NULL);
+		ret = cti_enable(drvdata->csdev, CS_MODE_SYSFS, NULL, NULL);
 		if (ret)
 			pm_runtime_put(dev->parent);
 	} else {
diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
index 8362a47c939c..73954369654c 100644
--- a/drivers/hwtracing/coresight/coresight-cti.h
+++ b/drivers/hwtracing/coresight/coresight-cti.h
@@ -216,8 +216,10 @@ int cti_add_connection_entry(struct device *dev, struct cti_drvdata *drvdata,
 			     const char *assoc_dev_name);
 struct cti_trig_con *cti_allocate_trig_con(struct device *dev, int in_sigs,
 					   int out_sigs);
-int cti_enable(struct coresight_device *csdev, enum cs_mode mode, void *data);
-int cti_disable(struct coresight_device *csdev, void *data);
+int cti_enable(struct coresight_device *csdev, enum cs_mode mode,
+	       struct coresight_path *path,
+	       struct perf_output_handle *handle);
+int cti_disable(struct coresight_device *csdev, struct coresight_path *path);
 void cti_write_all_hw_regs(struct cti_drvdata *drvdata);
 void cti_write_intack(struct device *dev, u32 ackval);
 void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value);
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 33e22b1ba043..65c4984d98c0 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -136,7 +136,7 @@ static inline void CS_UNLOCK(void __iomem *addr)
 
 void coresight_disable_path(struct coresight_path *path);
 int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
-			  void *sink_data);
+			  struct perf_output_handle *handle);
 struct coresight_device *coresight_get_sink(struct coresight_path *path);
 struct coresight_device *coresight_get_sink_by_id(u32 id);
 struct coresight_device *
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index b07fcdb3fe1a..2ed7fa2366ce 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1325,9 +1325,9 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 }
 
 struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
-				   enum cs_mode mode, void *data)
+				   enum cs_mode mode,
+				   struct perf_output_handle *handle)
 {
-	struct perf_output_handle *handle = data;
 	struct etr_perf_buffer *etr_perf;
 
 	switch (mode) {
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 6541a27a018e..b6e2b00d393a 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -440,7 +440,8 @@ struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);
 void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);
 void tmc_etr_remove_catu_ops(void);
 struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
-				   enum cs_mode mode, void *data);
+				   enum cs_mode mode,
+				   struct perf_output_handle *handle);
 extern const struct attribute_group coresight_etr_group;
 
 #endif
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 4ac65c68bbf4..450cccd46f38 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -422,8 +422,10 @@ struct coresight_ops_source {
  */
 struct coresight_ops_helper {
 	int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
-		      void *data);
-	int (*disable)(struct coresight_device *csdev, void *data);
+		      struct coresight_path *path,
+		      struct perf_output_handle *data);
+	int (*disable)(struct coresight_device *csdev,
+		       struct coresight_path *path);
 };
 
 

base-commit: 46a51f4f5edade43ba66b3c151f0e25ec8b69cb6
-- 
2.39.5
Re: [PATCH v2] coresight: Fix data arguments to coresight enable/disable helpers
Posted by Jie Gan 4 months, 2 weeks ago 

On 9/20/2025 1:49 AM, Carl Worth wrote:
> In the commit being fixed, coresight_enable_path() was changed to call
> coresight_enable_helpers() by passing a struct coresight_path*
> as the final void* 'path' argument, rather passing 'sink_data'
> as was being passed previously. This set the groundwork for the
> subsequent addition of the ctcu_enable function which interprets
> void* data as a struct coresight_path*, but it also broke the
> existing catu_enable function which interprets void* data
> as a struct perf_output_handle*.
> 
> The compiler could not flag the error since there are several layers
> of function calls treating the pointer as void*.
> 
> Fix both users simultaneously by adding an additional argument to the
> enable helper interface. So now both the struct coresight_path and the
> struct perf_output_handle pointers are passed. Also, eliminate all
> usages of the void* from these code paths and use explicit types
> instead to make all of this code more robust.
> 
> Note: The disable function is also changed from void* to
> struct coresight_path* but no implementations of this function
> need the struct perf_output_handle* so it is not added to the interface.
> 
> The existing bug can be reproduced with:
> 
> # perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null
> 
> Showing an oops as follows:
> 
> Unable to handle kernel paging request at virtual address 000f6e84934ed19e
> 
> Call trace:
>   tmc_etr_get_buffer+0x30/0x80 [coresight_tmc] (P)
>   catu_enable_hw+0xbc/0x3d0 [coresight_catu]
>   catu_enable+0x70/0xe0 [coresight_catu]
>   coresight_enable_path+0xb0/0x258 [coresight]
> 
> Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path")
> Signed-off-by: Carl Worth <carl@os.amperecomputing.com>
> ---
> 
> Jie, I looked into your suggestion of adding the struct
> perf_output_handle* to the struct coresight_path, but I couldn't
> justify adding event-related data to the path structure, which has
> nothing to do with it.

The AUX event only available in perf mode. Each subsystem can access it 
from the path if needed. Since the Coresight system operates by 
constructing a path — and this path is passed to each device as data - 
it’s acceptable to include all relevant parameters in the 
coresight_path, provided the devices along the path require them.

Do you want me to create a patch as example?

Thanks,
Jie

> 
> So, instead I took the approach in this patch to plumb both arguments
> through the enable path, (and changed away from void* as you agreed
> to).
> 
> Note: I've tested that this fixes the bug for catu_enable. This patch
> also obviously touches ctcu_enable, which I believe is correct, but I
> have not tested since I don't have ready access to the necessary
> hardware. I will appreciate other testing.
> 
> -Carl
> 
>   drivers/hwtracing/coresight/coresight-catu.c  | 12 ++++++----
>   drivers/hwtracing/coresight/coresight-core.c  | 23 +++++++++++--------
>   .../hwtracing/coresight/coresight-ctcu-core.c | 11 ++++-----
>   .../hwtracing/coresight/coresight-cti-core.c  |  7 ++++--
>   .../hwtracing/coresight/coresight-cti-sysfs.c |  2 +-
>   drivers/hwtracing/coresight/coresight-cti.h   |  6 +++--
>   drivers/hwtracing/coresight/coresight-priv.h  |  2 +-
>   .../hwtracing/coresight/coresight-tmc-etr.c   |  4 ++--
>   drivers/hwtracing/coresight/coresight-tmc.h   |  3 ++-
>   include/linux/coresight.h                     |  6 +++--
>   10 files changed, 45 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index 5058432233da..724c25d0afa4 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -397,7 +397,7 @@ static int catu_wait_for_ready(struct catu_drvdata *drvdata)
>   }
>   
>   static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
> -			  void *data)
> +			  struct perf_output_handle *handle)
>   {
>   	int rc;
>   	u32 control, mode;
> @@ -425,7 +425,7 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
>   	etrdev = coresight_find_input_type(
>   		csdev->pdata, CORESIGHT_DEV_TYPE_SINK, etr_subtype);
>   	if (etrdev) {
> -		etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data);
> +		etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, handle);
>   		if (IS_ERR(etr_buf))
>   			return PTR_ERR(etr_buf);
>   	}
> @@ -455,7 +455,8 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
>   }
>   
>   static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
> -		       void *data)
> +		       struct coresight_path *path,
> +		       struct perf_output_handle *handle)
>   {
>   	int rc = 0;
>   	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
> @@ -463,7 +464,7 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
>   	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
>   	if (csdev->refcnt == 0) {
>   		CS_UNLOCK(catu_drvdata->base);
> -		rc = catu_enable_hw(catu_drvdata, mode, data);
> +		rc = catu_enable_hw(catu_drvdata, mode, handle);
>   		CS_LOCK(catu_drvdata->base);
>   	}
>   	if (!rc)
> @@ -488,7 +489,8 @@ static int catu_disable_hw(struct catu_drvdata *drvdata)
>   	return rc;
>   }
>   
> -static int catu_disable(struct coresight_device *csdev, void *__unused)
> +static int catu_disable(struct coresight_device *csdev,
> +			struct coresight_path *__unused)
>   {
>   	int rc = 0;
>   	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fa758cc21827..cc449d5196a4 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -353,14 +353,17 @@ static bool coresight_is_helper(struct coresight_device *csdev)
>   }
>   
>   static int coresight_enable_helper(struct coresight_device *csdev,
> -				   enum cs_mode mode, void *data)
> +				   enum cs_mode mode,
> +				   struct coresight_path *path,
> +				   struct perf_output_handle *handle)
>   {
> -	return helper_ops(csdev)->enable(csdev, mode, data);
> +	return helper_ops(csdev)->enable(csdev, mode, path, handle);
>   }
>   
> -static void coresight_disable_helper(struct coresight_device *csdev, void *data)
> +static void coresight_disable_helper(struct coresight_device *csdev,
> +				     struct coresight_path *path)
>   {
> -	helper_ops(csdev)->disable(csdev, data);
> +	helper_ops(csdev)->disable(csdev, path);
>   }
>   
>   static void coresight_disable_helpers(struct coresight_device *csdev, void *data)
> @@ -477,7 +480,9 @@ void coresight_disable_path(struct coresight_path *path)
>   EXPORT_SYMBOL_GPL(coresight_disable_path);
>   
>   static int coresight_enable_helpers(struct coresight_device *csdev,
> -				    enum cs_mode mode, void *data)
> +				    enum cs_mode mode,
> +				    struct coresight_path *path,
> +				    struct perf_output_handle *handle)
>   {
>   	int i, ret = 0;
>   	struct coresight_device *helper;
> @@ -487,7 +492,7 @@ static int coresight_enable_helpers(struct coresight_device *csdev,
>   		if (!helper || !coresight_is_helper(helper))
>   			continue;
>   
> -		ret = coresight_enable_helper(helper, mode, data);
> +		ret = coresight_enable_helper(helper, mode, path, handle);
>   		if (ret)
>   			return ret;
>   	}
> @@ -496,7 +501,7 @@ static int coresight_enable_helpers(struct coresight_device *csdev,
>   }
>   
>   int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> -			  void *sink_data)
> +			  struct perf_output_handle *handle)
>   {
>   	int ret = 0;
>   	u32 type;
> @@ -510,7 +515,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>   		type = csdev->type;
>   
>   		/* Enable all helpers adjacent to the path first */
> -		ret = coresight_enable_helpers(csdev, mode, path);
> +		ret = coresight_enable_helpers(csdev, mode, path, handle);
>   		if (ret)
>   			goto err_disable_path;
>   		/*
> @@ -526,7 +531,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>   
>   		switch (type) {
>   		case CORESIGHT_DEV_TYPE_SINK:
> -			ret = coresight_enable_sink(csdev, mode, sink_data);
> +			ret = coresight_enable_sink(csdev, mode, handle);
>   			/*
>   			 * Sink is the first component turned on. If we
>   			 * failed to enable the sink, there are no components
> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> index c6bafc96db96..9f6d71c59d4e 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> @@ -156,17 +156,16 @@ static int ctcu_set_etr_traceid(struct coresight_device *csdev, struct coresight
>   	return __ctcu_set_etr_traceid(csdev, traceid, port_num, enable);
>   }
>   
> -static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode, void *data)
> +static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode,
> +		       struct coresight_path *path,
> +		       struct perf_output_handle *handle)
>   {
> -	struct coresight_path *path = (struct coresight_path *)data;
> -
>   	return ctcu_set_etr_traceid(csdev, path, true);
>   }
>   
> -static int ctcu_disable(struct coresight_device *csdev, void *data)
> +static int ctcu_disable(struct coresight_device *csdev,
> +			struct coresight_path *path)
>   {
> -	struct coresight_path *path = (struct coresight_path *)data;
> -
>   	return ctcu_set_etr_traceid(csdev, path, false);
>   }
>   
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index 8fb30dd73fd2..f92e3be4607c 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -799,14 +799,17 @@ static void cti_pm_release(struct cti_drvdata *drvdata)
>   }
>   
>   /** cti ect operations **/
> -int cti_enable(struct coresight_device *csdev, enum cs_mode mode, void *data)
> +int cti_enable(struct coresight_device *csdev, enum cs_mode mode,
> +	       struct coresight_path *path,
> +	       struct perf_output_handle *handle)
>   {
>   	struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
>   
>   	return cti_enable_hw(drvdata);
>   }
>   
> -int cti_disable(struct coresight_device *csdev, void *data)
> +int cti_disable(struct coresight_device *csdev,
> +		struct coresight_path *path)
>   {
>   	struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
>   
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index 572b80ee96fb..2bb6929eeb19 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -112,7 +112,7 @@ static ssize_t enable_store(struct device *dev,
>   		ret = pm_runtime_resume_and_get(dev->parent);
>   		if (ret)
>   			return ret;
> -		ret = cti_enable(drvdata->csdev, CS_MODE_SYSFS, NULL);
> +		ret = cti_enable(drvdata->csdev, CS_MODE_SYSFS, NULL, NULL);
>   		if (ret)
>   			pm_runtime_put(dev->parent);
>   	} else {
> diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
> index 8362a47c939c..73954369654c 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.h
> +++ b/drivers/hwtracing/coresight/coresight-cti.h
> @@ -216,8 +216,10 @@ int cti_add_connection_entry(struct device *dev, struct cti_drvdata *drvdata,
>   			     const char *assoc_dev_name);
>   struct cti_trig_con *cti_allocate_trig_con(struct device *dev, int in_sigs,
>   					   int out_sigs);
> -int cti_enable(struct coresight_device *csdev, enum cs_mode mode, void *data);
> -int cti_disable(struct coresight_device *csdev, void *data);
> +int cti_enable(struct coresight_device *csdev, enum cs_mode mode,
> +	       struct coresight_path *path,
> +	       struct perf_output_handle *handle);
> +int cti_disable(struct coresight_device *csdev, struct coresight_path *path);
>   void cti_write_all_hw_regs(struct cti_drvdata *drvdata);
>   void cti_write_intack(struct device *dev, u32 ackval);
>   void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value);
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 33e22b1ba043..65c4984d98c0 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -136,7 +136,7 @@ static inline void CS_UNLOCK(void __iomem *addr)
>   
>   void coresight_disable_path(struct coresight_path *path);
>   int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> -			  void *sink_data);
> +			  struct perf_output_handle *handle);
>   struct coresight_device *coresight_get_sink(struct coresight_path *path);
>   struct coresight_device *coresight_get_sink_by_id(u32 id);
>   struct coresight_device *
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index b07fcdb3fe1a..2ed7fa2366ce 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1325,9 +1325,9 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>   }
>   
>   struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
> -				   enum cs_mode mode, void *data)
> +				   enum cs_mode mode,
> +				   struct perf_output_handle *handle)
>   {
> -	struct perf_output_handle *handle = data;
>   	struct etr_perf_buffer *etr_perf;
>   
>   	switch (mode) {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 6541a27a018e..b6e2b00d393a 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -440,7 +440,8 @@ struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);
>   void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);
>   void tmc_etr_remove_catu_ops(void);
>   struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
> -				   enum cs_mode mode, void *data);
> +				   enum cs_mode mode,
> +				   struct perf_output_handle *handle);
>   extern const struct attribute_group coresight_etr_group;
>   
>   #endif
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 4ac65c68bbf4..450cccd46f38 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -422,8 +422,10 @@ struct coresight_ops_source {
>    */
>   struct coresight_ops_helper {
>   	int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> -		      void *data);
> -	int (*disable)(struct coresight_device *csdev, void *data);
> +		      struct coresight_path *path,
> +		      struct perf_output_handle *data);
> +	int (*disable)(struct coresight_device *csdev,
> +		       struct coresight_path *path);
>   };
>   
>   
> 
> base-commit: 46a51f4f5edade43ba66b3c151f0e25ec8b69cb6
Re: [PATCH] coresight: Fix data argument to coresight_enable_helpers
Posted by Leo Yan 4 months, 3 weeks ago 
On Wed, Sep 17, 2025 at 09:35:55AM +0800, Jie Gan wrote:

[...]

> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index fa758cc21827..b1077d73c988 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -510,7 +510,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >   		type = csdev->type;
> >   		/* Enable all helpers adjacent to the path first */
> > -		ret = coresight_enable_helpers(csdev, mode, path);
> > +		ret = coresight_enable_helpers(csdev, mode, sink_data);
> 
> I dont think we can change back to sink_data since we introduced
> coresight_path to wrap 'data' which is needed by the path.

This change can fix catu issue but will cause regression for ctcu
driver.

> I suggest you to add the struct perf_output_handle to the coresight_path,
> then retrieving it with data->perf_handle in tmc_etr_get_buffer.
> 
> before:
> struct perf_output_handle *handle = data;
> 
> after:
> struct coresight_path *path = data;
> struct perf_output_handle *handle = path->perf_handle;
> 
> We can assign the perf_output_handle to the coresight_path after we
> constructed the coresight_path in perf mode.

The suggestion looks good to me.

Thanks,
Leo

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.