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

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);