Re: [PATCH v4 2/2] coresight: core: Disable helpers for devices that fail to enable

[Leo Yan <leo.yan@arm.com>]

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;
>  		case CORESIGHT_DEV_TYPE_SOURCE:
>  			/* sources are enabled from either sysFS or Perf */
> @@ -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;
>  		default:
> -			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
>