Re: [PATCH v4 2/2] coresight: Make sysFS functional on topologies with per core sink

[Mathieu Poirier <mathieu.poirier@linaro.org>]

Hi Linu,

On Fri, Sep 04, 2020 at 08:11:06AM +0530, Linu Cherian wrote:
> Coresight driver assumes sink is common across all the ETMs,
> and tries to build a path between ETM and the first enabled
> sink found using bus based search. This breaks sysFS usage
> on implementations that has multiple per core sinks in
> enabled state.
> 
> To fix this, coresight_get_enabled_sink API is updated to
> do a connection based search starting from the given source,
> instead of bus based search.
> With sink selection using sysfs depecrated for perf interface,
> provision for reset is removed as well in this API.
> 
> Signed-off-by: Linu Cherian <lcherian@marvell.com>
> ---
>  drivers/hwtracing/coresight/coresight-priv.h |  3 +-
>  drivers/hwtracing/coresight/coresight.c      | 58 +++++++++-----------
>  2 files changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index f2dc625ea585..5fe773c4d6cc 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -148,7 +148,8 @@ static inline void coresight_write_reg_pair(void __iomem *addr, u64 val,
>  void coresight_disable_path(struct list_head *path);
>  int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
>  struct coresight_device *coresight_get_sink(struct list_head *path);
> -struct coresight_device *coresight_get_enabled_sink(bool reset);
> +struct coresight_device *
> +coresight_get_enabled_sink(struct coresight_device *source);
>  struct coresight_device *coresight_get_sink_by_id(u32 id);
>  struct coresight_device *
>  coresight_find_default_sink(struct coresight_device *csdev);
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index e9c90f2de34a..562dc3d5f5b2 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -540,50 +540,46 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
>  	return csdev;
>  }
>  
> -static int coresight_enabled_sink(struct device *dev, const void *data)
> +static struct coresight_device *
> +coresight_find_enabled_sink(struct coresight_device *csdev)
>  {
> -	const bool *reset = data;
> -	struct coresight_device *csdev = to_coresight_device(dev);
> +	int i;
> +	struct coresight_device *sink;
>  
>  	if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
>  	     csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
> -	     csdev->activated) {
> -		/*
> -		 * Now that we have a handle on the sink for this session,
> -		 * disable the sysFS "enable_sink" flag so that possible
> -		 * concurrent perf session that wish to use another sink don't
> -		 * trip on it.  Doing so has no ramification for the current
> -		 * session.
> -		 */
> -		if (*reset)
> -			csdev->activated = false;
> +	    csdev->activated)

Indentation problem.

> +		return csdev;
>  
> -		return 1;
> +	/*
> +	 * Recursively explore each port found on this element.
> +	 */
> +	for (i = 0; i < csdev->pdata->nr_outport; i++) {
> +		struct coresight_device *child_dev;
> +
> +		child_dev = csdev->pdata->conns[i].child_dev;
> +		if (child_dev)
> +			sink = coresight_find_enabled_sink(child_dev);
> +		if (sink)
> +			return sink;
>  	}
>  
> -	return 0;
> +	return NULL;
>  }
>  
>  /**
> - * coresight_get_enabled_sink - returns the first enabled sink found on the bus
> - * @deactivate:	Whether the 'enable_sink' flag should be reset
> - *
> - * When operated from perf the deactivate parameter should be set to 'true'.
> - * That way the "enabled_sink" flag of the sink that was selected can be reset,
> - * allowing for other concurrent perf sessions to choose a different sink.
> + * coresight_get_enabled_sink - returns the first enabled sink using
> + * connection based search starting from the source reference
>   *
> - * When operated from sysFS users have full control and as such the deactivate
> - * parameter should be set to 'false', hence mandating users to explicitly
> - * clear the flag.
> + * @source: Coresight source device reference
>   */
> -struct coresight_device *coresight_get_enabled_sink(bool deactivate)
> +struct coresight_device *
> +coresight_get_enabled_sink(struct coresight_device *source)
>  {
> -	struct device *dev = NULL;
> -
> -	dev = bus_find_device(&coresight_bustype, NULL, &deactivate,
> -			      coresight_enabled_sink);
> +	if (!source)
> +		return NULL;
>  
> -	return dev ? to_coresight_device(dev) : NULL;
> +	return coresight_find_enabled_sink(source);
>  }
>  
>  static int coresight_sink_by_id(struct device *dev, const void *data)
> @@ -992,7 +988,7 @@ int coresight_enable(struct coresight_device *csdev)
>  	 * Search for a valid sink for this session but don't reset the
>  	 * "enable_sink" flag in sysFS.  Users get to do that explicitly.
>  	 */

With this patch the above comment is no longer relevant.  Since you were nice
enough to do the extra work I fixed both problem and applied your patches. 

Thanks,
Mathieu

> -	sink = coresight_get_enabled_sink(false);
> +	sink = coresight_get_enabled_sink(csdev);
>  	if (!sink) {
>  		ret = -EINVAL;
>  		goto out;
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel