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(-)
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
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);
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);
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);
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
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) {
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) {
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);
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);
-
Leo Yan -
Suzuki K Poulose