On Tue, Apr 15, 2025 at 11:46:49AM -0700, Yabin Cui wrote:
When enabling a SINK or LINK type coresight device fails, the associated helpers should be disabled.
Signed-off-by: Yabin Cui yabinc@google.com Suggested-by: Suzuki K Poulose suzuki.poulose@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 fb43ef6a3b1f..d9fcea69d221 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -486,8 +486,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, * that need disabling. Disabling the path here * would mean we could disrupt an existing session. */
if (ret)
if (ret) {coresight_disable_helpers(csdev, path);
I think we can do better for code consolidation - we can use a central place for error handling. I will give details below.
goto out;
} break;@@ -497,15 +499,17 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, child = list_next_entry(nd, link)->csdev; ret = coresight_enable_link(csdev, parent, child, source); if (ret)
goto err;
goto err_disable_helpers; break;
goto err;
goto err_disable_helpers;
I know this is not a problem introduced by your patch - for an error case, it returns 0. This will hide unexpected issues. I would like to suggest to return -EINVAL for unknown types.
}} out: return ret; +err_disable_helpers:
- coresight_disable_helpers(csdev, path);
err: coresight_disable_path_from(path, nd); goto out;
I am just wandering if we can handle errors in a unified way and without using goto. I would change the code as below.
The point is to use a general flow for error handling, include a sink error. For sink error, we still invoke coresight_disable_path_from() for an empty operation.
Also, I think we need an additional patch for error handling in coresight_enable_helpers(). If any errors are detected while enabling a helper, we should disable the helpers that have already been enabled.
Please let me know if you have any questions.
Thanks, Leo
---8<---
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index fb43ef6a3b1f..cf2a3708a05e 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, /* Enable all helpers adjacent to the path first */ ret = coresight_enable_helpers(csdev, mode, path); if (ret) - goto err; + goto err_disable_path; /* * ETF devices are tricky... They can be a link or a sink, * depending on how they are configured. If an ETF has been @@ -487,7 +487,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, * would mean we could disrupt an existing session. */ if (ret) - goto out; + goto err_disable_helpers; break; case CORESIGHT_DEV_TYPE_SOURCE: /* sources are enabled from either sysFS or Perf */ @@ -497,18 +497,21 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, child = list_next_entry(nd, link)->csdev; ret = coresight_enable_link(csdev, parent, child, source); if (ret) - goto err; + goto err_disable_helpers; break; default: - goto err; + ret = -EINVAL; + goto err_disable_helpers; } }
-out: - return ret; -err: + return 0; + +err_disable_helpers: + coresight_disable_helpers(csdev, path); +err_disable_path: coresight_disable_path_from(path, nd); - goto out; + return ret; }
-- 2.49.0.604.gff1f9ca942-goog