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@xxxxxxxxxx>
> ---
> 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
>
