| Message ID | 20200616164006.15309-5-mike.leach@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update CoreSight infrastructure to select a default sink. | expand |
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 >
Hi Mathieu, On Mon, 29 Jun 2020 at 18:47, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > 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. That's interesting - this was added to overcome Suzuki's objection that it wasn't possible to determine which sink was in use! However, I think it is important to allow this as once we see systems with many cores + many sinks, determining the correct sink to enable becomes much more difficult. You said yourself, albeit in relation to perf, that for 1:1 systems, sink selection should be implicit. This is something I completely agree with, and hence the automatic selection algorithm that was chosen to ensure that this is the case. Is there any reason not to make the same assertion for sysfs? Further, this allows sysfs users to write board agnostic tests (similar to the one Leo wrote for perf) - effectively all we need to do to test the coresight function on a board is iterate through the cpus / etms without worrying about the sink in use, then name of which can be read from the etm and then data read out. As an aside - last_sink also shows which sink was used should you happen to explicitly enable two sinks in the etm path (e.g. etf & etr). > 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. Sure - but is it not just as clunky to have to figure out which sink attaches to your etm in the first place? (yes there are topolgy links now but this is not the most straighforward thing to use) Ultimately, if you are only using sysfs, you never actually need to disable the sink to read back data if you don't want to. I am not sure there are many people who use both syfs and perf in the same session to collect trace - and these are the ones who would need to be careful about disabling the sink. From a debug tools perspective, anything we can do to make things easier for users - especially at so little extra cost, can only be a good thing. > 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. > Or possibly only auto-disable if the sink was selected by default in the first place. Another short cut would be to allow writing 0 to the last_sink attribute to disable the named sink. Regards Mike > 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 > > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Hi Mike, Mathieu, On 07/01/2020 05:40 PM, Mike Leach wrote: > Hi Mathieu, > > On Mon, 29 Jun 2020 at 18:47, Mathieu Poirier > <mathieu.poirier@linaro.org> wrote: >> >> 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. > > That's interesting - this was added to overcome Suzuki's objection > that it wasn't possible to determine which sink was in use! I personally don't prefer the auto selection for sysfs mode. And that was one of the arguments to support it. > > However, I think it is important to allow this as once we see systems > with many cores + many sinks, determining the correct sink to enable > becomes much more difficult. > > You said yourself, albeit in relation to perf, that for 1:1 systems, > sink selection should be implicit. This is something I completely > agree with, and hence the automatic selection algorithm that was > chosen to ensure that this is the case. > Is there any reason not to make the same assertion for sysfs? > > Further, this allows sysfs users to write board agnostic tests > (similar to the one Leo wrote for perf) - effectively all we need to > do to test the coresight function on a board is iterate through the > cpus / etms without worrying about the sink in use, then name of which > can be read from the etm and then data read out. The tests could use the "connections" exposed via the sysfs to figure out the appropriate sink for a given source. > > As an aside - last_sink also shows which sink was used should you > happen to explicitly enable two sinks in the etm path (e.g. etf & > etr). > >> 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. > > Sure - but is it not just as clunky to have to figure out which sink > attaches to your etm in the first place? (yes there are topolgy links > now but this is not the most straighforward thing to use) > Ultimately, if you are only using sysfs, you never actually need to > disable the sink to read back data if you don't want to. I am not sure > there are many people who use both syfs and perf in the same session > to collect trace - and these are the ones who would need to be careful > about disabling the sink. The problem lies exactly there. Just like we don't know how many actual sysfs mode users are there, who consume the trace data and use it in a production environment compared to a bring up situation (verifying that the board topology is detected fine and the components are working fine), there could be users of the perf on these systems. Debugging such cases where someone forgot to disable the trace can be a painful process. Like I have said from the beginning, this is not worth the benefit that we get from this code (i.e, figuring out which sink is closer to a source in sysfs mode, when there is an existing infrastructure, i.e, "connections" already available for this). Cheers Suzuki
Hi Suzuki, On Wed, 1 Jul 2020 at 23:19, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > Hi Mike, Mathieu, > > On 07/01/2020 05:40 PM, Mike Leach wrote: > > Hi Mathieu, > > > > On Mon, 29 Jun 2020 at 18:47, Mathieu Poirier > > <mathieu.poirier@linaro.org> wrote: > >> > >> 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. > > > > That's interesting - this was added to overcome Suzuki's objection > > that it wasn't possible to determine which sink was in use! > > I personally don't prefer the auto selection for sysfs mode. And that > was one of the arguments to support it. > > > > > However, I think it is important to allow this as once we see systems > > with many cores + many sinks, determining the correct sink to enable > > becomes much more difficult. > > > > You said yourself, albeit in relation to perf, that for 1:1 systems, > > sink selection should be implicit. This is something I completely > > agree with, and hence the automatic selection algorithm that was > > chosen to ensure that this is the case. > > Is there any reason not to make the same assertion for sysfs? > > > > Further, this allows sysfs users to write board agnostic tests > > (similar to the one Leo wrote for perf) - effectively all we need to > > do to test the coresight function on a board is iterate through the > > cpus / etms without worrying about the sink in use, then name of which > > can be read from the etm and then data read out. > > The tests could use the "connections" exposed via the sysfs to figure > out the appropriate sink for a given source. > > > > > As an aside - last_sink also shows which sink was used should you > > happen to explicitly enable two sinks in the etm path (e.g. etf & > > etr). > > > >> 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. > > > > Sure - but is it not just as clunky to have to figure out which sink > > attaches to your etm in the first place? (yes there are topolgy links > > now but this is not the most straighforward thing to use) > > Ultimately, if you are only using sysfs, you never actually need to > > disable the sink to read back data if you don't want to. I am not sure > > there are many people who use both syfs and perf in the same session > > to collect trace - and these are the ones who would need to be careful > > about disabling the sink. > > The problem lies exactly there. Just like we don't know how many actual > sysfs mode users are there, who consume the trace data and use it in a > production environment compared to a bring up situation (verifying > that the board topology is detected fine and the components are working > fine), there could be users of the perf on these systems. > This is an issue irrespective of how the trace sink is turned on, be it automatically or explicitly. Given that it is possible to read the sink data without disabling the sink - the chances are it could happen either way. > Debugging such cases where someone forgot to disable the trace can be > a painful process. Like I have said from the beginning, this is not > worth the benefit that we get from this code (i.e, figuring out which > sink is closer to a source in sysfs mode, when there is an existing > infrastructure, i.e, "connections" already available for this). > Actually all connections can tell you is the number sinks available to the etm on the path - not which would be selected by the current priority algorithm - unless the user is willing to dig into the driver source code and figure out the priority mechanism. Regards Mike > Cheers > Suzuki
On 07/02/2020 01:21 AM, Mike Leach wrote: > Hi Suzuki, > > On Wed, 1 Jul 2020 at 23:19, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: >> >> Hi Mike, Mathieu, >> >> On 07/01/2020 05:40 PM, Mike Leach wrote: >>> Hi Mathieu, >>> >>> On Mon, 29 Jun 2020 at 18:47, Mathieu Poirier >>> <mathieu.poirier@linaro.org> wrote: >>>> >>>> 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. >>> >>> That's interesting - this was added to overcome Suzuki's objection >>> that it wasn't possible to determine which sink was in use! >> >> I personally don't prefer the auto selection for sysfs mode. And that >> was one of the arguments to support it. >> >>> >>> However, I think it is important to allow this as once we see systems >>> with many cores + many sinks, determining the correct sink to enable >>> becomes much more difficult. >>> >>> You said yourself, albeit in relation to perf, that for 1:1 systems, >>> sink selection should be implicit. This is something I completely >>> agree with, and hence the automatic selection algorithm that was >>> chosen to ensure that this is the case. >>> Is there any reason not to make the same assertion for sysfs? >>> >>> Further, this allows sysfs users to write board agnostic tests >>> (similar to the one Leo wrote for perf) - effectively all we need to >>> do to test the coresight function on a board is iterate through the >>> cpus / etms without worrying about the sink in use, then name of which >>> can be read from the etm and then data read out. >> >> The tests could use the "connections" exposed via the sysfs to figure >> out the appropriate sink for a given source. >> >>> >>> As an aside - last_sink also shows which sink was used should you >>> happen to explicitly enable two sinks in the etm path (e.g. etf & >>> etr). >>> >>>> 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. >>> >>> Sure - but is it not just as clunky to have to figure out which sink >>> attaches to your etm in the first place? (yes there are topolgy links >>> now but this is not the most straighforward thing to use) >>> Ultimately, if you are only using sysfs, you never actually need to >>> disable the sink to read back data if you don't want to. I am not sure >>> there are many people who use both syfs and perf in the same session >>> to collect trace - and these are the ones who would need to be careful >>> about disabling the sink. >> >> The problem lies exactly there. Just like we don't know how many actual >> sysfs mode users are there, who consume the trace data and use it in a >> production environment compared to a bring up situation (verifying >> that the board topology is detected fine and the components are working >> fine), there could be users of the perf on these systems. >> > > This is an issue irrespective of how the trace sink is turned on, be > it automatically or explicitly. > Given that it is possible to read the sink data without disabling the > sink - the chances are it could happen either way. > >> Debugging such cases where someone forgot to disable the trace can be >> a painful process. Like I have said from the beginning, this is not >> worth the benefit that we get from this code (i.e, figuring out which >> sink is closer to a source in sysfs mode, when there is an existing >> infrastructure, i.e, "connections" already available for this). >> > > Actually all connections can tell you is the number sinks available to > the etm on the path - not which would be selected by the current > priority algorithm - unless the user is willing to dig into the driver > source code and figure out the priority mechanism. Exactly. My point is, don't do this for sysfs mode. The user can figure this out for sysfs mode, if he/she wanted to (unlike the perf mode, where the event could be placed on any CPU). We don't have to add this fragile change just because the user don't want to do this himself, when he must know what was used for collecting the trace back (again, something the perf mode doesn't have to worry about and is justifying the use there). In other words, the change for sysfs mode is not justified enough. Cheers Suzuki > > Regards > > Mike > >> Cheers >> Suzuki > > >
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."); } 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 */
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(-)