Re: [PATCH v5 4/5] coresight: sysfs: Allow select default sink on source enable.

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

Hi Mike,

I have applied patches 1 to 3 of this set.  Please see below for comments on
this patch.

On Tue, Jun 16, 2020 at 05:40:05PM +0100, Mike Leach wrote:
> When enabling a trace source using sysfs, allow the CoreSight system to
> auto-select a default sink if none has been enabled by the user.
> 
> Uses the sink select algorithm that uses the default select priorities
> set when sinks are registered with the system. At present this will
> prefer ETR over ETB / ETF.
> 
> Adds a new attribute 'last_sink' to source CoreSight devices. This is set
> when a source is enabled using sysfs, to the sink that the device will
> trace into. This applies for both user enabled and default enabled sinks.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++++--
>  include/linux/coresight.h               |  3 ++
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index e9c90f2de34a..db39e0b56994 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -934,6 +934,16 @@ static void coresight_clear_default_sink(struct coresight_device *csdev)
>  	}
>  }
>  
> +static void coresight_set_last_sink_name(struct coresight_device *source,
> +					 struct coresight_device *sink)
> +{
> +	/* remove current value and set new one if *sink not NULL */
> +	kfree(source->last_sink);
> +	source->last_sink = NULL;
> +	if (sink)
> +		source->last_sink = kstrdup(dev_name(&sink->dev), GFP_KERNEL);
> +}
> +
>  /** coresight_validate_source - make sure a source has the right credentials
>   *  @csdev:	the device structure for a source.
>   *  @function:	the function this was called from.
> @@ -994,8 +1004,15 @@ int coresight_enable(struct coresight_device *csdev)
>  	 */
>  	sink = coresight_get_enabled_sink(false);
>  	if (!sink) {
> -		ret = -EINVAL;
> -		goto out;
> +		/* look for a default sink if nothing enabled */
> +		sink = coresight_find_default_sink(csdev);
> +		if (!sink) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		/* mark the default as enabled */
> +		sink->activated = true;
> +		dev_info(&sink->dev, "Enabled default sink.");

I'm very ambivalent about extending the automatic sink selection to the sysfs
interface, mainly because of the new sysfs entry it requires.  I find it
clunky that users don't have to specify the sink to use but have to explicitly
disable it after the trace session.  We could automatically disable the sink
after a trace session but that would break with the current sysfs heuristic
where sinks have to be explicitly enabled and disabled.

Thanks,
Mathieu 

>  	}
>  
>  	path = coresight_build_path(csdev, sink);
> @@ -1033,6 +1050,9 @@ int coresight_enable(struct coresight_device *csdev)
>  		break;
>  	}
>  
> +	/* record name of sink used for this session */
> +	coresight_set_last_sink_name(csdev, sink);
> +
>  out:
>  	mutex_unlock(&coresight_mutex);
>  	return ret;
> @@ -1145,6 +1165,19 @@ static ssize_t enable_source_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(enable_source);
>  
> +static ssize_t last_sink_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct coresight_device *csdev = to_coresight_device(dev);
> +	ssize_t size = 0;
> +
> +	if (csdev->last_sink)
> +		size = scnprintf(buf, PAGE_SIZE, "%s\n", csdev->last_sink);
> +	return size;
> +}
> +static DEVICE_ATTR_RO(last_sink);
> +
> +
>  static struct attribute *coresight_sink_attrs[] = {
>  	&dev_attr_enable_sink.attr,
>  	NULL,
> @@ -1153,6 +1186,7 @@ ATTRIBUTE_GROUPS(coresight_sink);
>  
>  static struct attribute *coresight_source_attrs[] = {
>  	&dev_attr_enable_source.attr,
> +	&dev_attr_last_sink.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(coresight_source);
> @@ -1524,6 +1558,7 @@ void coresight_unregister(struct coresight_device *csdev)
>  	/* Remove references of that device in the topology */
>  	coresight_remove_conns(csdev);
>  	coresight_clear_default_sink(csdev);
> +	coresight_set_last_sink_name(csdev, NULL);
>  	coresight_release_platform_data(csdev, csdev->pdata);
>  	device_unregister(&csdev->dev);
>  }
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 58fffdecdbfd..fc320dd2cedc 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -184,6 +184,8 @@ struct coresight_sysfs_link {
>   *		from source to that sink.
>   * @ea:		Device attribute for sink representation under PMU directory.
>   * @def_sink:	cached reference to default sink found for this device.
> + * @last_sink:	Name of last sink used for this source to trace into. Set when
> + *		enabling a source using sysfs - only set on a source device.
>   * @ect_dev:	Associated cross trigger device. Not part of the trace data
>   *		path or connections.
>   * @nr_links:   number of sysfs links created to other components from this
> @@ -203,6 +205,7 @@ struct coresight_device {
>  	bool activated;	/* true only if a sink is part of a path */
>  	struct dev_ext_attribute *ea;
>  	struct coresight_device *def_sink;
> +	const char *last_sink;
>  	/* cross trigger handling */
>  	struct coresight_device *ect_dev;
>  	/* sysfs links between components */
> -- 
> 2.17.1
> 

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