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);
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;
}
}
out:
return ret;
+err_disable_helpers:
+ coresight_disable_helpers(csdev, path);
err:
coresight_disable_path_from(path, nd);
goto out;
--
2.49.0.604.gff1f9ca942-googOn 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
>
On Wed, Apr 23, 2025 at 2:32 AM Leo Yan <leo.yan@arm.com> wrote:
>
> 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.
I'm fine with these changes. However, there's one spot below that's
controversial in the v2 patch.
>
> 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;
I made a similar change in this area in the v2 patch, which
contradicts the original comment here.
Mike had concerns about it. I explained my reasoning, but haven't
heard back. Could we get confirmation
that it's safe to remove the comment and proceed to err_disable_helpers here?
> 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
> >
© 2016 - 2025 Red Hat, Inc.