[PATCH v1 0/5] coresight: Fix device registration and unregistration

[PATCH v1 0/5] coresight: Fix device registration and unregistration

[Leo Yan]

This series is to fix device registration and unregistration.

The first patch addresses the resource is not released properly for a
failure case during a device registration.

The second patch is to use mutex to protect unregistration flow.

The last three patches are for refactoring.  Patch 03 explicitly uses
the parent device handler.  Patch 04 separates the success and failure
flows for code readable and easier maintenance.  Patch 05 improves the
error handling by invoking specific functions for resource cleanup.


Leo Yan (5):
  coresight: Correct sink ID map allocation failure handling
  coresight: Protect unregistration with mutex
  coresight: Explicitly use the parent device handler
  coresight: Separate failure and success flows
  coresight: Refine error handling for device registration

 drivers/hwtracing/coresight/coresight-core.c | 67 +++++++++++---------
 1 file changed, 37 insertions(+), 30 deletions(-)

-- 
2.34.1

[PATCH v1 1/5] coresight: Correct sink ID map allocation failure handling

[Leo Yan]

When registering a CoreSight device, it first increase the reference
counter for the associated device and then allocates sink ID map.  The
problem happens when the sink ID map allocation fails - the flow misses
decreasing the device's reference counter.  As a result, the device can
never be properly cleaned up after the memory allocation failure.

To fix the issue, the allocation of the sink ID map is moved before
increasing the reference counter.  With this change, if sink ID map
allocation fails, the function can exit without holding a reference to
the device.  Afterwords, any subsequent failures will invoke
coresight_device_release() to release the device's resource, including
decrementing the reference counter.

Fixes: 5ad628a76176 ("coresight: Use per-sink trace ID maps for Perf sessions")
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 5632bcb8feb6..3e3823d9f991 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1318,12 +1318,6 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	csdev->dev.parent = desc->dev;
 	csdev->dev.release = coresight_device_release;
 	csdev->dev.bus = &coresight_bustype;
-	/*
-	 * Hold the reference to our parent device. This will be
-	 * dropped only in coresight_device_release().
-	 */
-	csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(desc->dev));
-	dev_set_name(&csdev->dev, "%s", desc->name);
 
 	if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
 	    csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
@@ -1335,6 +1329,14 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 			goto err_out;
 		}
 	}
+
+	/*
+	 * Hold the reference to our parent device. This will be
+	 * dropped only in coresight_device_release().
+	 */
+	csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(desc->dev));
+	dev_set_name(&csdev->dev, "%s", desc->name);
+
 	/*
 	 * Make sure the device registration and the connection fixup
 	 * are synchronised, so that we don't see uninitialised devices
-- 
2.34.1

[PATCH v1 2/5] coresight: Protect unregistration with mutex

[Leo Yan]

The device registration is protected by CoreSight mutex to ensure the
atomic operations when adding a device onto bus.  One the other hand,
the locking is absent when unregister a device.

Use mutex to ensure atomicity on device unregistration.  During
unregistration, unbinding the associated CTI device is not included in
the locking region, as CTI has its own locking mechanism.

Fixes: 8c1d3f79d9ca ("coresight: core: Fix coresight device probe failure issue")
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 3e3823d9f991..3eacdcf638df 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1400,14 +1400,17 @@ EXPORT_SYMBOL_GPL(coresight_register);
 
 void coresight_unregister(struct coresight_device *csdev)
 {
-	etm_perf_del_symlink_sink(csdev);
 	/* Remove references of that device in the topology */
 	if (cti_assoc_ops && cti_assoc_ops->remove)
 		cti_assoc_ops->remove(csdev);
+
+	mutex_lock(&coresight_mutex);
+	etm_perf_del_symlink_sink(csdev);
 	coresight_remove_conns(csdev);
 	coresight_clear_default_sink(csdev);
 	coresight_release_platform_data(csdev, csdev->dev.parent, csdev->pdata);
 	device_unregister(&csdev->dev);
+	mutex_unlock(&coresight_mutex);
 }
 EXPORT_SYMBOL_GPL(coresight_unregister);
 
-- 
2.34.1

[PATCH v1 3/5] coresight: Explicitly use the parent device handler

[Leo Yan]

A CoreSight device is present on the CoreSight bus, and its device node
in the DT binding is assigned as the parent device.  Comments are added
to document this relationship.

The code is refined to explicitly use the parent device handle, making
it more readable and clearly indicating which device handle is being
used.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 3eacdcf638df..4f51ce152ac7 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1313,9 +1313,13 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	csdev->access = desc->access;
 	csdev->orphan = true;
 
+	/*
+	 * 'csdev->dev' is a device present on the CoreSight bus. The device
+	 * node in the device tree is assigned as the parent device.
+	 */
+	csdev->dev.parent = desc->dev;
 	csdev->dev.type = &coresight_dev_type[desc->type];
 	csdev->dev.groups = desc->groups;
-	csdev->dev.parent = desc->dev;
 	csdev->dev.release = coresight_device_release;
 	csdev->dev.bus = &coresight_bustype;
 
@@ -1334,7 +1338,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	 * Hold the reference to our parent device. This will be
 	 * dropped only in coresight_device_release().
 	 */
-	csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(desc->dev));
+	csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(csdev->dev.parent));
 	dev_set_name(&csdev->dev, "%s", desc->name);
 
 	/*
@@ -1393,7 +1397,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 
 err_out:
 	/* Cleanup the connection information */
-	coresight_release_platform_data(NULL, desc->dev, desc->pdata);
+	coresight_release_platform_data(NULL, csdev->dev.parent, desc->pdata);
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(coresight_register);
-- 
2.34.1

[PATCH v1 4/5] coresight: Separate failure and success flows

[Leo Yan]

For a success registration, it releases mutex, then binds associated CTI
device, and returns a device pointer.

As a result, it separates flows between the success case and the failure
flow, any code after the tag 'out_unlock' is only used for failure
handling.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 4f51ce152ac7..4fc82206b326 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1377,17 +1377,21 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	registered = true;
 
 	ret = coresight_create_conns_sysfs_group(csdev);
-	if (!ret)
-		ret = coresight_fixup_orphan_conns(csdev);
+	if (ret)
+		goto out_unlock;
+
+	ret = coresight_fixup_orphan_conns(csdev);
+	if (ret)
+		goto out_unlock;
+
+	mutex_unlock(&coresight_mutex);
+
+	if (cti_assoc_ops && cti_assoc_ops->add)
+		cti_assoc_ops->add(csdev);
+	return csdev;
 
 out_unlock:
 	mutex_unlock(&coresight_mutex);
-	/* Success */
-	if (!ret) {
-		if (cti_assoc_ops && cti_assoc_ops->add)
-			cti_assoc_ops->add(csdev);
-		return csdev;
-	}
 
 	/* Unregister the device if needed */
 	if (registered) {
-- 
2.34.1

[PATCH v1 5/5] coresight: Refine error handling for device registration

[Leo Yan]

When error happens in a device registration, the coresight_unregister()
function is invoked for cleanup.  coresight_unregister() includes the
complete flow for unregisteration a CoreSight device, it causes
redundant operations for some errors.

This commit changes to invoke more specific functions for cleanup
resources for each error.  This can allow the cleanup flow in better
granularity.

As a result, the local "registered" variable is not used anymore, remove
it.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 26 ++++++++------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 4fc82206b326..1eb4f6f0fe40 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1297,7 +1297,6 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 {
 	int ret;
 	struct coresight_device *csdev;
-	bool registered = false;
 
 	csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
 	if (!csdev) {
@@ -1362,27 +1361,23 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	    csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
 		ret = etm_perf_add_symlink_sink(csdev);
 
-		if (ret) {
-			device_unregister(&csdev->dev);
+		if (ret)
 			/*
 			 * As with the above, all resources are free'd
 			 * explicitly via coresight_device_release() triggered
 			 * from put_device(), which is in turn called from
 			 * function device_unregister().
 			 */
-			goto out_unlock;
-		}
+			goto out_unregister_device;
 	}
-	/* Device is now registered */
-	registered = true;
 
 	ret = coresight_create_conns_sysfs_group(csdev);
 	if (ret)
-		goto out_unlock;
+		goto out_del_symlink_sink;
 
 	ret = coresight_fixup_orphan_conns(csdev);
 	if (ret)
-		goto out_unlock;
+		goto out_remove_conns;
 
 	mutex_unlock(&coresight_mutex);
 
@@ -1390,15 +1385,14 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 		cti_assoc_ops->add(csdev);
 	return csdev;
 
+out_remove_conns:
+	coresight_remove_conns_sysfs_group(csdev);
+out_del_symlink_sink:
+	etm_perf_del_symlink_sink(csdev);
+out_unregister_device:
+	device_unregister(&csdev->dev);
 out_unlock:
 	mutex_unlock(&coresight_mutex);
-
-	/* Unregister the device if needed */
-	if (registered) {
-		coresight_unregister(csdev);
-		return ERR_PTR(ret);
-	}
-
 err_out:
 	/* Cleanup the connection information */
 	coresight_release_platform_data(NULL, csdev->dev.parent, desc->pdata);
-- 
2.34.1

Re: [PATCH v1 3/5] coresight: Explicitly use the parent device handler

[Suzuki K Poulose]

On 12/05/2025 16:41, Leo Yan wrote:
> A CoreSight device is present on the CoreSight bus, and its device node
> in the DT binding is assigned as the parent device.  Comments are added
> to document this relationship.
> 
> The code is refined to explicitly use the parent device handle, making
> it more readable and clearly indicating which device handle is being
> used.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 3eacdcf638df..4f51ce152ac7 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1313,9 +1313,13 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>   	csdev->access = desc->access;
>   	csdev->orphan = true;
>   
> +	/*
> +	 * 'csdev->dev' is a device present on the CoreSight bus. The device
> +	 * node in the device tree is assigned as the parent device.
> +	 */
> +	csdev->dev.parent = desc->dev;
>   	csdev->dev.type = &coresight_dev_type[desc->type];
>   	csdev->dev.groups = desc->groups;
> -	csdev->dev.parent = desc->dev;
>   	csdev->dev.release = coresight_device_release;
>   	csdev->dev.bus = &coresight_bustype;
>   
> @@ -1334,7 +1338,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>   	 * Hold the reference to our parent device. This will be
>   	 * dropped only in coresight_device_release().
>   	 */
> -	csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(desc->dev));
> +	csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(csdev->dev.parent));
>   	dev_set_name(&csdev->dev, "%s", desc->name);
>   
>   	/*
> @@ -1393,7 +1397,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>   
>   err_out:
>   	/* Cleanup the connection information */
> -	coresight_release_platform_data(NULL, desc->dev, desc->pdata);
> +	coresight_release_platform_data(NULL, csdev->dev.parent, desc->pdata);

This may be problematic, as the csdev could be NULL ?

Suzuki

>   	return ERR_PTR(ret);
>   }
>   EXPORT_SYMBOL_GPL(coresight_register);

Re: [PATCH v1 3/5] coresight: Explicitly use the parent device handler

[Leo Yan]

On Thu, Sep 11, 2025 at 09:52:20AM +0100, Suzuki Kuruppassery Poulose wrote:

[...]

> > @@ -1393,7 +1397,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
> >   err_out:
> >   	/* Cleanup the connection information */
> > -	coresight_release_platform_data(NULL, desc->dev, desc->pdata);
> > +	coresight_release_platform_data(NULL, csdev->dev.parent, desc->pdata);
> 
> This may be problematic, as the csdev could be NULL ?

Indeed. I will drop this change in next version.

Thanks,
Leo

Re: [PATCH v1 4/5] coresight: Separate failure and success flows

[Suzuki K Poulose]

On 12/05/2025 16:41, Leo Yan wrote:
> For a success registration, it releases mutex, then binds associated CTI
> device, and returns a device pointer.
> 
> As a result, it separates flows between the success case and the failure
> flow, any code after the tag 'out_unlock' is only used for failure
> handling.
> 

This description is a bit ambiguous. Please could we simply say:

Subject: coresight: Cleanup coresight_register error handling

Separate the failure handling path from the successful case.
Use the out_unlock label only for the failure handling.

Rest looks fine to me.

Suzuki

> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 4f51ce152ac7..4fc82206b326 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1377,17 +1377,21 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>   	registered = true;
>   
>   	ret = coresight_create_conns_sysfs_group(csdev);
> -	if (!ret)
> -		ret = coresight_fixup_orphan_conns(csdev);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = coresight_fixup_orphan_conns(csdev);
> +	if (ret)
> +		goto out_unlock;
> +
> +	mutex_unlock(&coresight_mutex);
> +
> +	if (cti_assoc_ops && cti_assoc_ops->add)
> +		cti_assoc_ops->add(csdev);
> +	return csdev;
>   
>   out_unlock:
>   	mutex_unlock(&coresight_mutex);
> -	/* Success */
> -	if (!ret) {
> -		if (cti_assoc_ops && cti_assoc_ops->add)
> -			cti_assoc_ops->add(csdev);
> -		return csdev;
> -	}
>   
>   	/* Unregister the device if needed */
>   	if (registered) {

Re: [PATCH v1 5/5] coresight: Refine error handling for device registration

[Suzuki K Poulose]

On 12/05/2025 16:41, Leo Yan wrote:
> When error happens in a device registration, the coresight_unregister()
> function is invoked for cleanup.  coresight_unregister() includes the
> complete flow for unregisteration a CoreSight device, it causes
> redundant operations for some errors.
> 
> This commit changes to invoke more specific functions for cleanup
> resources for each error.  This can allow the cleanup flow in better
> granularity.
> 
> As a result, the local "registered" variable is not used anymore, remove
> it.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 26 ++++++++------------
>   1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 4fc82206b326..1eb4f6f0fe40 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1297,7 +1297,6 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>   {
>   	int ret;
>   	struct coresight_device *csdev;
> -	bool registered = false;
>   
>   	csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
>   	if (!csdev) {
> @@ -1362,27 +1361,23 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>   	    csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
>   		ret = etm_perf_add_symlink_sink(csdev);
>   
> -		if (ret) {
> -			device_unregister(&csdev->dev);
> +		if (ret)
>   			/*
>   			 * As with the above, all resources are free'd
>   			 * explicitly via coresight_device_release() triggered
>   			 * from put_device(), which is in turn called from
>   			 * function device_unregister().
>   			 */
> -			goto out_unlock;
> -		}
> +			goto out_unregister_device;
>   	}
> -	/* Device is now registered */
> -	registered = true;
>   
>   	ret = coresight_create_conns_sysfs_group(csdev);
>   	if (ret)
> -		goto out_unlock;
> +		goto out_del_symlink_sink;
>   
>   	ret = coresight_fixup_orphan_conns(csdev);
>   	if (ret)
> -		goto out_unlock;
> +		goto out_remove_conns;
>   
>   	mutex_unlock(&coresight_mutex);
>   
> @@ -1390,15 +1385,14 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>   		cti_assoc_ops->add(csdev);
>   	return csdev;
>   
> +out_remove_conns:
> +	coresight_remove_conns_sysfs_group(csdev);
> +out_del_symlink_sink:
> +	etm_perf_del_symlink_sink(csdev);
> +out_unregister_device:
> +	device_unregister(&csdev->dev);

Is there any reason why we can't use the coresigh_unregister() ? I see
the mutex_lock() needs to be released before calling it. But otherwise
all of the above could be done from there ?

Also, again, after device_unregister(), we are not allowed to touch
csdev. Another reason why the release_platform_data below is a problem.

Suzuki

		
>   out_unlock:
>   	mutex_unlock(&coresight_mutex);
> -
> -	/* Unregister the device if needed */
> -	if (registered) {
> -		coresight_unregister(csdev);
> -		return ERR_PTR(ret);
> -	}
> -
>   err_out:
>   	/* Cleanup the connection information */
>   	coresight_release_platform_data(NULL, csdev->dev.parent, desc->pdata);