Re: [PATCH v11 14/27] coresight: Move source helper disabling to coresight_disable_path()

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

On 01/05/2026 17:47, Leo Yan wrote:
> Move source helper disabling to coresight_disable_path() to pair it with
> coresight_enable_path().
> 
> In coresight_enable_path(), if enabling a node fails, iterate to the
> next node (which is the last successfully enabled node) and pass it to
> coresight_disable_path() for rollback.  If the failed node is the last
> node on the path, no device is actually enabled, so bail out directly.
> 
> As a result, coresight_disable_source() only controls the source,
> leaving the associated helpers to be managed as part of path.  Update
> the comment to reflect this change.
> 
> Tested-by: Jie Gan <jie.gan@oss.qualcomm.com>
> Reviewed-by: Yeoreum Yun <yeoreum.yun@arm.com>
> Reviewed-by: James Clark <james.clark@linaro.org>
> Tested-by: James Clark <james.clark@linaro.org>
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 38 +++++++++++++---------------
>   1 file changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 0c11e7fff1bd5cf504b17241595d83a1d11bcf40..6ab80ae7c389e9031d4ab065d83bb676fd12160b 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -447,19 +447,12 @@ static void coresight_disable_helpers(struct coresight_device *csdev,
>   }
>   
>   /*
> - * Helper function to call source_ops(csdev)->disable and also disable the
> - * helpers.
> - *
> - * There is an imbalance between coresight_enable_path() and
> - * coresight_disable_path(). Enabling also enables the source's helpers as part
> - * of the path, but disabling always skips the first item in the path (which is
> - * the source), so sources and their helpers don't get disabled as part of that
> - * function and we need the extra step here.
> + * coresight_disable_source() only disables the source, but do nothing for
> + * the associated helpers, which are controlled as part of the path.
>    */
>   void coresight_disable_source(struct coresight_device *csdev, void *data)
>   {
>   	source_ops(csdev)->disable(csdev, data);
> -	coresight_disable_helpers(csdev, NULL);
>   }
>   EXPORT_SYMBOL_GPL(coresight_disable_source);
>   
> @@ -486,9 +479,9 @@ int coresight_resume_source(struct coresight_device *csdev)
>   EXPORT_SYMBOL_GPL(coresight_resume_source);
>   
>   /*
> - * coresight_disable_path_from : Disable components in the given path beyond
> - * @nd in the list. If @nd is NULL, all the components, except the SOURCE are
> - * disabled.
> + * coresight_disable_path_from : Disable components in the given path starting
> + * from @nd in the list. If @nd is NULL, all the components, except the SOURCE
> + * are disabled.
>    */
>   static void coresight_disable_path_from(struct coresight_path *path,
>   					struct coresight_node *nd)
> @@ -499,7 +492,7 @@ static void coresight_disable_path_from(struct coresight_path *path,
>   	if (!nd)
>   		nd = list_first_entry(&path->path_list, struct coresight_node, link);
>   
> -	list_for_each_entry_continue(nd, &path->path_list, link) {
> +	list_for_each_entry_from(nd, &path->path_list, link) {
>   		csdev = nd->csdev;
>   		type = csdev->type;
>   
> @@ -519,12 +512,6 @@ static void coresight_disable_path_from(struct coresight_path *path,
>   			coresight_disable_sink(csdev);
>   			break;
>   		case CORESIGHT_DEV_TYPE_SOURCE:
> -			/*
> -			 * We skip the first node in the path assuming that it
> -			 * is the source. So we don't expect a source device in
> -			 * the middle of a path.
> -			 */
> -			WARN_ON(1);
>   			break;
>   		case CORESIGHT_DEV_TYPE_LINK:
>   			parent = list_prev_entry(nd, link)->csdev;


--8>--

> @@ -580,12 +567,16 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode)
>   {
>   	int ret = 0;
>   	u32 type;
> -	struct coresight_node *nd;
> +	struct coresight_node *nd, *last;
>   	struct coresight_device *csdev, *parent, *child;
>   	struct coresight_device *source;
>   
>   	source = coresight_get_source(path);
> -	list_for_each_entry_reverse(nd, &path->path_list, link) {
> +
> +	last = list_last_entry(&path->path_list, struct coresight_node, link);
> +
> +	nd = last;
> +	list_for_each_entry_from_reverse(nd, &path->path_list, link) {
>   		csdev = nd->csdev;
>   		type = csdev->type;
>   
> @@ -639,6 +630,11 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode)
>   err_disable_helpers:
>   	coresight_disable_helpers(csdev, path);
>   err_disable_path:
> +	/* No device is actually enabled */
> +	if (nd == last)
> +		goto out;
  +

This hunk seems to be different from the one described in the commit and
should be a different patch if at all we need it ?
Moreover, this looks unnecessary. We already do the check to see if we
didn't enable any device. See CORESIGHT_DEVICE_TYPE_SINK case.

Suzuki


> +	nd = list_next_entry(nd, link);
>   	coresight_disable_path_from(path, nd);
>   	goto out;
>   }
>