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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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