* [PATCH 0/2] coresight: Allow sharing of STM @ 2017-03-09 15:27 Suzuki K Poulose 2017-03-09 15:27 ` [PATCH 1/2] coresight: Disable the path only when the source is disabled Suzuki K Poulose 2017-03-09 15:27 ` [PATCH 2/2] coresight: Fix reference count for software sources Suzuki K Poulose 0 siblings, 2 replies; 13+ messages in thread From: Suzuki K Poulose @ 2017-03-09 15:27 UTC (permalink / raw) To: mathieu.poirier Cc: linux-arm-kernel, Chunyan Zhang, linux-kernel, Suzuki K Poulose This series fixes the coresight generic layer to handle the reference counting for the STM source properly, to allow multiple applications to share the STM. Without this series, the STM is disabled when the first user closes its connection, causing trace data losses. Suzuki K Poulose (2): coresight: Disable the path only when the source is disabled coresight: Fix reference count for software sources drivers/hwtracing/coresight/coresight.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] coresight: Disable the path only when the source is disabled 2017-03-09 15:27 [PATCH 0/2] coresight: Allow sharing of STM Suzuki K Poulose @ 2017-03-09 15:27 ` Suzuki K Poulose 2017-03-14 17:37 ` [1/2] " Mathieu Poirier 2017-03-09 15:27 ` [PATCH 2/2] coresight: Fix reference count for software sources Suzuki K Poulose 1 sibling, 1 reply; 13+ messages in thread From: Suzuki K Poulose @ 2017-03-09 15:27 UTC (permalink / raw) To: mathieu.poirier Cc: linux-arm-kernel, Chunyan Zhang, linux-kernel, Suzuki K Poulose With a coresight tracing session, the components along the path from the source to sink are disabled after the source is disabled. However, if the source was not actually disabled due to active users, we should not disable the components in the path. Cc: Mathieu Poirier <mathieu.poirier@linaro.org> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- drivers/hwtracing/coresight/coresight.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 0c37356..34cd1ed 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -253,7 +253,8 @@ static int coresight_enable_source(struct coresight_device *csdev, u32 mode) return 0; } -static void coresight_disable_source(struct coresight_device *csdev) +/* coresight_disable_source: Returns true if the device has been disabled */ +static bool coresight_disable_source(struct coresight_device *csdev) { if (atomic_dec_return(csdev->refcnt) == 0) { if (source_ops(csdev)->disable) { @@ -261,6 +262,7 @@ static void coresight_disable_source(struct coresight_device *csdev) csdev->enable = false; } } + return !csdev->enable; } void coresight_disable_path(struct list_head *path) @@ -629,7 +631,7 @@ void coresight_disable(struct coresight_device *csdev) if (ret) goto out; - if (!csdev->enable) + if (!csdev->enable || !coresight_disable_source(csdev)) goto out; switch (csdev->subtype.source_subtype) { @@ -647,7 +649,6 @@ void coresight_disable(struct coresight_device *csdev) break; } - coresight_disable_source(csdev); coresight_disable_path(path); coresight_release_path(path); -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [1/2] coresight: Disable the path only when the source is disabled 2017-03-09 15:27 ` [PATCH 1/2] coresight: Disable the path only when the source is disabled Suzuki K Poulose @ 2017-03-14 17:37 ` Mathieu Poirier 2017-03-14 17:39 ` Suzuki K Poulose 0 siblings, 1 reply; 13+ messages in thread From: Mathieu Poirier @ 2017-03-14 17:37 UTC (permalink / raw) To: Suzuki K. Poulose; +Cc: Chunyan Zhang, linux-kernel, linux-arm-kernel Hi Suzuki, On 14 March 2017 at 11:31, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> > > With a coresight tracing session, the components along the path > from the source to sink are disabled after the source is disabled. > However, if the source was not actually disabled due to active > users, we should not disable the components in the path. > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/hwtracing/coresight/coresight.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c > index 0c37356..34cd1ed 100644 > --- a/drivers/hwtracing/coresight/coresight.c > +++ b/drivers/hwtracing/coresight/coresight.c > @@ -253,7 +253,8 @@ static int coresight_enable_source(struct coresight_device *csdev, u32 mode) > return 0; > } > > -static void coresight_disable_source(struct coresight_device *csdev) > +/* coresight_disable_source: Returns true if the device has been disabled */ To be coherent with the rest of the file please use: /* * coresight_disable_source: Returns true if the device has been disabled */ I would have done the modification myself but 2/2 needs tending as well. Thanks, Mathieu > +static bool coresight_disable_source(struct coresight_device *csdev) > { > if (atomic_dec_return(csdev->refcnt) == 0) { > if (source_ops(csdev)->disable) { > @@ -261,6 +262,7 @@ static void coresight_disable_source(struct coresight_device *csdev) > csdev->enable = false; > } > } > + return !csdev->enable; > } > > void coresight_disable_path(struct list_head *path) > @@ -629,7 +631,7 @@ void coresight_disable(struct coresight_device *csdev) > if (ret) > goto out; > > - if (!csdev->enable) > + if (!csdev->enable || !coresight_disable_source(csdev)) > goto out; > > switch (csdev->subtype.source_subtype) { > @@ -647,7 +649,6 @@ void coresight_disable(struct coresight_device *csdev) > break; > } > > - coresight_disable_source(csdev); > coresight_disable_path(path); > coresight_release_path(path); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [1/2] coresight: Disable the path only when the source is disabled 2017-03-14 17:37 ` [1/2] " Mathieu Poirier @ 2017-03-14 17:39 ` Suzuki K Poulose 0 siblings, 0 replies; 13+ messages in thread From: Suzuki K Poulose @ 2017-03-14 17:39 UTC (permalink / raw) To: Mathieu Poirier; +Cc: Chunyan Zhang, linux-kernel, linux-arm-kernel On 14/03/17 17:37, Mathieu Poirier wrote: > Hi Suzuki, > > On 14 March 2017 at 11:31, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: >> From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> >> >> With a coresight tracing session, the components along the path >> from the source to sink are disabled after the source is disabled. >> However, if the source was not actually disabled due to active >> users, we should not disable the components in the path. >> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> drivers/hwtracing/coresight/coresight.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c >> index 0c37356..34cd1ed 100644 >> --- a/drivers/hwtracing/coresight/coresight.c >> +++ b/drivers/hwtracing/coresight/coresight.c >> @@ -253,7 +253,8 @@ static int coresight_enable_source(struct coresight_device *csdev, u32 mode) >> return 0; >> } >> >> -static void coresight_disable_source(struct coresight_device *csdev) >> +/* coresight_disable_source: Returns true if the device has been disabled */ > > To be coherent with the rest of the file please use: > > /* > * coresight_disable_source: Returns true if the device has been disabled > */ > > I would have done the modification myself but 2/2 needs tending as well. > Ah, sorry about that, will resend it. Thanks for spotting. Suzuki ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] coresight: Fix reference count for software sources 2017-03-09 15:27 [PATCH 0/2] coresight: Allow sharing of STM Suzuki K Poulose 2017-03-09 15:27 ` [PATCH 1/2] coresight: Disable the path only when the source is disabled Suzuki K Poulose @ 2017-03-09 15:27 ` Suzuki K Poulose 2017-03-14 17:40 ` [2/2] " Mathieu Poirier 1 sibling, 1 reply; 13+ messages in thread From: Suzuki K Poulose @ 2017-03-09 15:27 UTC (permalink / raw) To: mathieu.poirier Cc: linux-arm-kernel, Chunyan Zhang, linux-kernel, Suzuki K Poulose For software sources (i.e STM), there could be multiple agents generating the trace data, unlike the ETMs. So we need to properly do the accounting for the active number of users to disable the device when the last user goes away. Right now, the reference counting is broken for sources as we skip the actions when we detect that the source is enabled. This patch fixes the problem by adding the refcounting for software sources, even when they are enabled. Cc: Mathieu Poirier <mathieu.poirier@linaro.org> Reported-by: Robert Walker <robert.walker@arm.com> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 34cd1ed..2da9e39 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev) int cpu, ret = 0; struct coresight_device *sink; struct list_head *path; + enum coresight_dev_subtype_source subtype = csdev->subtype.source_subtype; mutex_lock(&coresight_mutex); @@ -559,8 +560,16 @@ int coresight_enable(struct coresight_device *csdev) if (ret) goto out; - if (csdev->enable) + if (csdev->enable) { + /* + * There could be multiple applications driving the software + * source. So keep the refcount for each such user when the + * source is already enabled. + */ + if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) + atomic_inc(csdev->refcnt); goto out; + } /* * Search for a valid sink for this session but don't reset the @@ -587,7 +596,7 @@ int coresight_enable(struct coresight_device *csdev) if (ret) goto err_source; - switch (csdev->subtype.source_subtype) { + switch (subtype) { case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: /* * When working from sysFS it is important to keep track -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [2/2] coresight: Fix reference count for software sources 2017-03-09 15:27 ` [PATCH 2/2] coresight: Fix reference count for software sources Suzuki K Poulose @ 2017-03-14 17:40 ` Mathieu Poirier 2017-03-14 17:55 ` Suzuki K Poulose 2017-03-14 18:06 ` Suzuki K Poulose 0 siblings, 2 replies; 13+ messages in thread From: Mathieu Poirier @ 2017-03-14 17:40 UTC (permalink / raw) To: Suzuki K. Poulose; +Cc: linux-arm-kernel, Chunyan Zhang, linux-kernel On 14 March 2017 at 11:32, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> > > For software sources (i.e STM), there could be multiple agents > generating the trace data, unlike the ETMs. So we need to > properly do the accounting for the active number of users > to disable the device when the last user goes away. Right > now, the reference counting is broken for sources as we skip > the actions when we detect that the source is enabled. > > This patch fixes the problem by adding the refcounting for > software sources, even when they are enabled. > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Reported-by: Robert Walker <robert.walker@arm.com> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c > index 34cd1ed..2da9e39 100644 > --- a/drivers/hwtracing/coresight/coresight.c > +++ b/drivers/hwtracing/coresight/coresight.c > @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev) > int cpu, ret = 0; > struct coresight_device *sink; > struct list_head *path; > + enum coresight_dev_subtype_source subtype = csdev->subtype.source_subtype; Checkpatch.pl complains about a line over 80 characters. > > mutex_lock(&coresight_mutex); > > @@ -559,8 +560,16 @@ int coresight_enable(struct coresight_device *csdev) > if (ret) > goto out; > > - if (csdev->enable) > + if (csdev->enable) { > + /* > + * There could be multiple applications driving the software > + * source. So keep the refcount for each such user when the > + * source is already enabled. > + */ > + if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) > + atomic_inc(csdev->refcnt); > goto out; > + } > > /* > * Search for a valid sink for this session but don't reset the > @@ -587,7 +596,7 @@ int coresight_enable(struct coresight_device *csdev) > if (ret) > goto err_source; > > - switch (csdev->subtype.source_subtype) { > + switch (subtype) { > case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: > /* > * When working from sysFS it is important to keep track ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [2/2] coresight: Fix reference count for software sources 2017-03-14 17:40 ` [2/2] " Mathieu Poirier @ 2017-03-14 17:55 ` Suzuki K Poulose 2017-03-14 19:32 ` Mathieu Poirier 2017-03-14 18:06 ` Suzuki K Poulose 1 sibling, 1 reply; 13+ messages in thread From: Suzuki K Poulose @ 2017-03-14 17:55 UTC (permalink / raw) To: Mathieu Poirier; +Cc: linux-arm-kernel, Chunyan Zhang, linux-kernel On 14/03/17 17:40, Mathieu Poirier wrote: > On 14 March 2017 at 11:32, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: >> From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> >> >> For software sources (i.e STM), there could be multiple agents >> generating the trace data, unlike the ETMs. So we need to >> properly do the accounting for the active number of users >> to disable the device when the last user goes away. Right >> now, the reference counting is broken for sources as we skip >> the actions when we detect that the source is enabled. >> >> This patch fixes the problem by adding the refcounting for >> software sources, even when they are enabled. >> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> Reported-by: Robert Walker <robert.walker@arm.com> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c >> index 34cd1ed..2da9e39 100644 >> --- a/drivers/hwtracing/coresight/coresight.c >> +++ b/drivers/hwtracing/coresight/coresight.c >> @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev) >> int cpu, ret = 0; >> struct coresight_device *sink; >> struct list_head *path; >> + enum coresight_dev_subtype_source subtype = csdev->subtype.source_subtype; > > Checkpatch.pl complains about a line over 80 characters. Hmm, I think the name of the enum is unusually long. I could clean it up a little bit. Also, it does look silly to split a simple variable assignment though. What do you prefer ? Suzuki ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [2/2] coresight: Fix reference count for software sources 2017-03-14 17:55 ` Suzuki K Poulose @ 2017-03-14 19:32 ` Mathieu Poirier 0 siblings, 0 replies; 13+ messages in thread From: Mathieu Poirier @ 2017-03-14 19:32 UTC (permalink / raw) To: Suzuki K Poulose; +Cc: linux-arm-kernel, Chunyan Zhang, linux-kernel On 14 March 2017 at 11:55, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > On 14/03/17 17:40, Mathieu Poirier wrote: >> >> On 14 March 2017 at 11:32, Mathieu Poirier <mathieu.poirier@linaro.org> >> wrote: >>> >>> From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> >>> >>> For software sources (i.e STM), there could be multiple agents >>> generating the trace data, unlike the ETMs. So we need to >>> properly do the accounting for the active number of users >>> to disable the device when the last user goes away. Right >>> now, the reference counting is broken for sources as we skip >>> the actions when we detect that the source is enabled. >>> >>> This patch fixes the problem by adding the refcounting for >>> software sources, even when they are enabled. >>> >>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >>> Reported-by: Robert Walker <robert.walker@arm.com> >>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>> --- >>> drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight.c >>> b/drivers/hwtracing/coresight/coresight.c >>> index 34cd1ed..2da9e39 100644 >>> --- a/drivers/hwtracing/coresight/coresight.c >>> +++ b/drivers/hwtracing/coresight/coresight.c >>> @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev) >>> int cpu, ret = 0; >>> struct coresight_device *sink; >>> struct list_head *path; >>> + enum coresight_dev_subtype_source subtype = >>> csdev->subtype.source_subtype; >> >> >> Checkpatch.pl complains about a line over 80 characters. > > > > Hmm, I think the name of the enum is unusually long. I could clean it up a > little bit. > Also, it does look silly to split a simple variable assignment though. > > What do you prefer ? enum coresight_dev_subtype_source subtype; ... ... subtype = csdev->subtype.source_subtype; That is probably the easiest. > > Suzuki ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [2/2] coresight: Fix reference count for software sources 2017-03-14 17:40 ` [2/2] " Mathieu Poirier 2017-03-14 17:55 ` Suzuki K Poulose @ 2017-03-14 18:06 ` Suzuki K Poulose 2017-03-15 3:51 ` Chunyan Zhang 1 sibling, 1 reply; 13+ messages in thread From: Suzuki K Poulose @ 2017-03-14 18:06 UTC (permalink / raw) To: Mathieu Poirier; +Cc: linux-arm-kernel, Chunyan Zhang, linux-kernel On 14/03/17 17:40, Mathieu Poirier wrote: > On 14 March 2017 at 11:32, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: >> From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> >> >> For software sources (i.e STM), there could be multiple agents >> generating the trace data, unlike the ETMs. So we need to >> properly do the accounting for the active number of users >> to disable the device when the last user goes away. Right >> now, the reference counting is broken for sources as we skip >> the actions when we detect that the source is enabled. >> >> This patch fixes the problem by adding the refcounting for >> software sources, even when they are enabled. >> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> Reported-by: Robert Walker <robert.walker@arm.com> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c >> index 34cd1ed..2da9e39 100644 >> --- a/drivers/hwtracing/coresight/coresight.c >> +++ b/drivers/hwtracing/coresight/coresight.c >> @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev) >> int cpu, ret = 0; >> struct coresight_device *sink; >> struct list_head *path; >> + enum coresight_dev_subtype_source subtype = csdev->subtype.source_subtype; > > Checkpatch.pl complains about a line over 80 characters. > >> >> mutex_lock(&coresight_mutex); >> >> @@ -559,8 +560,16 @@ int coresight_enable(struct coresight_device *csdev) >> if (ret) >> goto out; >> >> - if (csdev->enable) >> + if (csdev->enable) { >> + /* >> + * There could be multiple applications driving the software >> + * source. So keep the refcount for each such user when the >> + * source is already enabled. >> + */ >> + if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) >> + atomic_inc(csdev->refcnt); >> goto out; >> + } >> Btw, should we allow the user to turn on the STM from sysfs (echo 1 > $STM/enable_source) ? All STM users should set their policy via ioctls and that in turn turns the device on. So it doesn't make sense for enable_source to really enable the hardware unless someone really opens it. Chunyan, Mathieu, thoughts ? Suzuki ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [2/2] coresight: Fix reference count for software sources 2017-03-14 18:06 ` Suzuki K Poulose @ 2017-03-15 3:51 ` Chunyan Zhang 2017-03-15 16:00 ` Suzuki K Poulose 0 siblings, 1 reply; 13+ messages in thread From: Chunyan Zhang @ 2017-03-15 3:51 UTC (permalink / raw) To: Suzuki K Poulose; +Cc: Mathieu Poirier, linux-arm-kernel, linux-kernel Hi Suzuki, On 15 March 2017 at 02:06, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > On 14/03/17 17:40, Mathieu Poirier wrote: >> >> On 14 March 2017 at 11:32, Mathieu Poirier <mathieu.poirier@linaro.org> >> wrote: >>> >>> From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> >>> >>> For software sources (i.e STM), there could be multiple agents >>> generating the trace data, unlike the ETMs. So we need to >>> properly do the accounting for the active number of users >>> to disable the device when the last user goes away. Right >>> now, the reference counting is broken for sources as we skip >>> the actions when we detect that the source is enabled. >>> >>> This patch fixes the problem by adding the refcounting for >>> software sources, even when they are enabled. >>> >>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >>> Reported-by: Robert Walker <robert.walker@arm.com> >>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>> --- >>> drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight.c >>> b/drivers/hwtracing/coresight/coresight.c >>> index 34cd1ed..2da9e39 100644 >>> --- a/drivers/hwtracing/coresight/coresight.c >>> +++ b/drivers/hwtracing/coresight/coresight.c >>> @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev) >>> int cpu, ret = 0; >>> struct coresight_device *sink; >>> struct list_head *path; >>> + enum coresight_dev_subtype_source subtype = >>> csdev->subtype.source_subtype; >> >> >> Checkpatch.pl complains about a line over 80 characters. >> >>> >>> mutex_lock(&coresight_mutex); >>> >>> @@ -559,8 +560,16 @@ int coresight_enable(struct coresight_device *csdev) >>> if (ret) >>> goto out; >>> >>> - if (csdev->enable) >>> + if (csdev->enable) { >>> + /* >>> + * There could be multiple applications driving the >>> software >>> + * source. So keep the refcount for each such user when >>> the >>> + * source is already enabled. >>> + */ >>> + if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) >>> + atomic_inc(csdev->refcnt); >>> goto out; >>> + } >>> > > Btw, should we allow the user to turn on the STM from sysfs (echo 1 > > $STM/enable_source) ? If enabling STM can not be allowed via sysfs, how should we allow users to turn on STM when they want to mmap STM to user space, and write STM device from user space directly? For example this kind of use case [1]. > All STM users should set their policy via ioctls and that in turn turns the > device on. Yes users can set policy via ioctls to request resource of STM (i.e. which STM channel(s) will be written), but they still need to use sysfs to enable STM. > So it doesn't make sense for enable_source to really enable > the hardware unless someone really opens it. Right, there're two ways to enable STM currently, e.g. 1) echo <addr>.stm > /sys/class/stm_source/stm_ftrace/stm_source_link 2) echo 1 > $STM/enable_source That would probably make people confused, I would appreciate any better solution. Thanks, Chunyan [1] https://github.com/lyrazhang/user-space-tests/tree/mmap-wrapper/mmap-wrapper > > Chunyan, Mathieu, > > thoughts ? > > Suzuki > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [2/2] coresight: Fix reference count for software sources 2017-03-15 3:51 ` Chunyan Zhang @ 2017-03-15 16:00 ` Suzuki K Poulose 2017-03-16 12:13 ` Chunyan Zhang 0 siblings, 1 reply; 13+ messages in thread From: Suzuki K Poulose @ 2017-03-15 16:00 UTC (permalink / raw) To: Chunyan Zhang; +Cc: Mathieu Poirier, linux-arm-kernel, linux-kernel On 15/03/17 03:51, Chunyan Zhang wrote: > Hi Suzuki, > > On 15 March 2017 at 02:06, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: >> On 14/03/17 17:40, Mathieu Poirier wrote: >>> >>> On 14 March 2017 at 11:32, Mathieu Poirier <mathieu.poirier@linaro.org> >>> wrote: >>>> >>>> From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> >>>> >>>> For software sources (i.e STM), there could be multiple agents >>>> generating the trace data, unlike the ETMs. So we need to >>>> properly do the accounting for the active number of users >>>> to disable the device when the last user goes away. Right >>>> now, the reference counting is broken for sources as we skip >>>> the actions when we detect that the source is enabled. >>>> >>>> This patch fixes the problem by adding the refcounting for >>>> software sources, even when they are enabled. >>>> >>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >>>> Reported-by: Robert Walker <robert.walker@arm.com> >>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>>> --- >>>> drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/hwtracing/coresight/coresight.c >>>> b/drivers/hwtracing/coresight/coresight.c >>>> index 34cd1ed..2da9e39 100644 >>>> --- a/drivers/hwtracing/coresight/coresight.c >>>> +++ b/drivers/hwtracing/coresight/coresight.c >>>> @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev) >>>> int cpu, ret = 0; >>>> struct coresight_device *sink; >>>> struct list_head *path; >>>> + enum coresight_dev_subtype_source subtype = >>>> csdev->subtype.source_subtype; >>> >>> >>> Checkpatch.pl complains about a line over 80 characters. >>> >>>> >>>> mutex_lock(&coresight_mutex); >>>> >>>> @@ -559,8 +560,16 @@ int coresight_enable(struct coresight_device *csdev) >>>> if (ret) >>>> goto out; >>>> >>>> - if (csdev->enable) >>>> + if (csdev->enable) { >>>> + /* >>>> + * There could be multiple applications driving the >>>> software >>>> + * source. So keep the refcount for each such user when >>>> the >>>> + * source is already enabled. >>>> + */ >>>> + if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) >>>> + atomic_inc(csdev->refcnt); >>>> goto out; >>>> + } >>>> >> >> Btw, should we allow the user to turn on the STM from sysfs (echo 1 > >> $STM/enable_source) ? > > If enabling STM can not be allowed via sysfs, how should we allow > users to turn on STM when they want to mmap STM to user space, and > write STM device from user space directly? For example this kind of > use case [1]. The ioct(, STP_POLICY_ID_SET) indirectly turns on the STM hardware via : stm_char_policy_set_ioctl()->stm.link (stm_generic_link)-> coresight_enable(). > >> All STM users should set their policy via ioctls and that in turn turns the >> device on. > > Yes users can set policy via ioctls to request resource of STM (i.e. > which STM channel(s) will be written), but they still need to use > sysfs to enable STM. As mentioned above, it is not necessary. > >> So it doesn't make sense for enable_source to really enable >> the hardware unless someone really opens it. > > Right, there're two ways to enable STM currently, e.g. > 1) echo <addr>.stm > /sys/class/stm_source/stm_ftrace/stm_source_link I am not familiar with the stm_source class. From a quick glance, it looks like, writing to stm_source_link triggers : stm_source_link_store()->stm_source_link_add()->(stm->data->link()). which is fine for connecting a source (ftrace,console or heartbeat) to STM. > 2) echo 1 > $STM/enable_source This is a bit awkward for an application who wants to mmap and stream data, and is quite unnecessary from my explanation above. > > That would probably make people confused, I would appreciate any > better solution. Please let me know if you have any outstanding concerns. Cheers Suzuki ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [2/2] coresight: Fix reference count for software sources 2017-03-15 16:00 ` Suzuki K Poulose @ 2017-03-16 12:13 ` Chunyan Zhang 2017-03-17 10:36 ` Suzuki K Poulose 0 siblings, 1 reply; 13+ messages in thread From: Chunyan Zhang @ 2017-03-16 12:13 UTC (permalink / raw) To: Suzuki K Poulose, Mathieu Poirier; +Cc: linux-arm-kernel, linux-kernel On 16 March 2017 at 00:00, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > On 15/03/17 03:51, Chunyan Zhang wrote: >> >> Hi Suzuki, >> >> On 15 March 2017 at 02:06, Suzuki K Poulose <Suzuki.Poulose@arm.com> >> wrote: >>> >>> On 14/03/17 17:40, Mathieu Poirier wrote: >>>> >>>> >>>> On 14 March 2017 at 11:32, Mathieu Poirier <mathieu.poirier@linaro.org> >>>> wrote: >>>>> >>>>> >>>>> From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> >>>>> >>>>> For software sources (i.e STM), there could be multiple agents >>>>> generating the trace data, unlike the ETMs. So we need to >>>>> properly do the accounting for the active number of users >>>>> to disable the device when the last user goes away. Right >>>>> now, the reference counting is broken for sources as we skip >>>>> the actions when we detect that the source is enabled. >>>>> >>>>> This patch fixes the problem by adding the refcounting for >>>>> software sources, even when they are enabled. >>>>> >>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >>>>> Reported-by: Robert Walker <robert.walker@arm.com> >>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>>>> --- >>>>> drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- >>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/hwtracing/coresight/coresight.c >>>>> b/drivers/hwtracing/coresight/coresight.c >>>>> index 34cd1ed..2da9e39 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight.c >>>>> +++ b/drivers/hwtracing/coresight/coresight.c >>>>> @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device >>>>> *csdev) >>>>> int cpu, ret = 0; >>>>> struct coresight_device *sink; >>>>> struct list_head *path; >>>>> + enum coresight_dev_subtype_source subtype = >>>>> csdev->subtype.source_subtype; >>>> >>>> >>>> >>>> Checkpatch.pl complains about a line over 80 characters. >>>> >>>>> >>>>> mutex_lock(&coresight_mutex); >>>>> >>>>> @@ -559,8 +560,16 @@ int coresight_enable(struct coresight_device >>>>> *csdev) >>>>> if (ret) >>>>> goto out; >>>>> >>>>> - if (csdev->enable) >>>>> + if (csdev->enable) { >>>>> + /* >>>>> + * There could be multiple applications driving the >>>>> software >>>>> + * source. So keep the refcount for each such user when >>>>> the >>>>> + * source is already enabled. >>>>> + */ >>>>> + if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) >>>>> + atomic_inc(csdev->refcnt); >>>>> goto out; >>>>> + } >>>>> >>> >>> Btw, should we allow the user to turn on the STM from sysfs (echo 1 > >>> $STM/enable_source) ? >> >> >> If enabling STM can not be allowed via sysfs, how should we allow >> users to turn on STM when they want to mmap STM to user space, and >> write STM device from user space directly? For example this kind of >> use case [1]. > > > The ioct(, STP_POLICY_ID_SET) indirectly turns on the STM hardware via : > stm_char_policy_set_ioctl()->stm.link (stm_generic_link)-> > coresight_enable(). > Ah, that's right. Especially after your patch [1] merged, it is indeed not necessary. Thanks for pointing this. > >> >>> All STM users should set their policy via ioctls and that in turn turns >>> the >>> device on. >> >> >> Yes users can set policy via ioctls to request resource of STM (i.e. >> which STM channel(s) will be written), but they still need to use >> sysfs to enable STM. > > > As mentioned above, it is not necessary. > >> >>> So it doesn't make sense for enable_source to really enable >>> the hardware unless someone really opens it. >> >> >> Right, there're two ways to enable STM currently, e.g. >> 1) echo <addr>.stm > /sys/class/stm_source/stm_ftrace/stm_source_link > > > I am not familiar with the stm_source class. From a quick glance, it looks > like, > writing to stm_source_link triggers : > stm_source_link_store()->stm_source_link_add()->(stm->data->link()). > > which is fine for connecting a source (ftrace,console or heartbeat) to STM. > >> 2) echo 1 > $STM/enable_source > > > This is a bit awkward for an application who wants to mmap and stream data, > and is quite unnecessary from my explanation above. > >> >> That would probably make people confused, I would appreciate any >> better solution. > > > Please let me know if you have any outstanding concerns. I haven't thought out other use case which need this sysfs interface for CoreSight STM device, I'm ok with hiding it, but I'm not sure if it's good to remove this ABI from CoreSight STM but leave it to other CoreSight source components. Mathieu, Any thought? Thanks, Chunyan [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/479893.html > > Cheers > Suzuki > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [2/2] coresight: Fix reference count for software sources 2017-03-16 12:13 ` Chunyan Zhang @ 2017-03-17 10:36 ` Suzuki K Poulose 0 siblings, 0 replies; 13+ messages in thread From: Suzuki K Poulose @ 2017-03-17 10:36 UTC (permalink / raw) To: Chunyan Zhang, Mathieu Poirier; +Cc: linux-arm-kernel, linux-kernel On 16/03/17 12:13, Chunyan Zhang wrote: > On 16 March 2017 at 00:00, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: >> On 15/03/17 03:51, Chunyan Zhang wrote: >>>> Btw, should we allow the user to turn on the STM from sysfs (echo 1 > >>>> $STM/enable_source) ? >>> >>> >>> If enabling STM can not be allowed via sysfs, how should we allow >>> users to turn on STM when they want to mmap STM to user space, and >>> write STM device from user space directly? For example this kind of >>> use case [1]. >> >> >> The ioct(, STP_POLICY_ID_SET) indirectly turns on the STM hardware via : >> stm_char_policy_set_ioctl()->stm.link (stm_generic_link)-> >> coresight_enable(). >> > > Ah, that's right. Especially after your patch [1] merged, it is > indeed not necessary. Thanks for pointing this. > >> >>> >>>> All STM users should set their policy via ioctls and that in turn turns >>>> the >>>> device on. >>> >>> >>> Yes users can set policy via ioctls to request resource of STM (i.e. >>> which STM channel(s) will be written), but they still need to use >>> sysfs to enable STM. >> >> >> As mentioned above, it is not necessary. >> >>> >>>> So it doesn't make sense for enable_source to really enable >>>> the hardware unless someone really opens it. >>> >>> >>> Right, there're two ways to enable STM currently, e.g. >>> 1) echo <addr>.stm > /sys/class/stm_source/stm_ftrace/stm_source_link >> >> >> I am not familiar with the stm_source class. From a quick glance, it looks >> like, >> writing to stm_source_link triggers : >> stm_source_link_store()->stm_source_link_add()->(stm->data->link()). >> >> which is fine for connecting a source (ftrace,console or heartbeat) to STM. >> >>> 2) echo 1 > $STM/enable_source >> >> >> This is a bit awkward for an application who wants to mmap and stream data, >> and is quite unnecessary from my explanation above. >> >>> >>> That would probably make people confused, I would appreciate any >>> better solution. >> >> >> Please let me know if you have any outstanding concerns. > > I haven't thought out other use case which need this sysfs interface > for CoreSight STM device, I'm ok with hiding it, but I'm not sure if > it's good to remove this ABI from CoreSight STM but leave it to other > CoreSight source components. I was not planning to remove the ABI, but make it a no-op. I had a chat with some of our experts in the area and they have one use case which needs the STM turned on. If the user wants to run two applications back to back (without overlap in execution), there is no way to make sure that the STM stays on after the first application exits. So I think that is a valid use case. So, we cannot make it The reason I brought up this discussion is that we don't know how many times the STM is enabled from the sysfs vs. stm_generic_link(). This could possibly cause issues with sysfs disabling the STM (by unbalanced enable/disable). As the user doesn't necessarily know how many times he/she enabled the device. May be we should keep track of the sysfs refcount separately and pass it down to the csdev->refcnt only once. I will cook up a patch. Suzuki > > Thanks, > Chunyan > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/479893.html > >> >> Cheers >> Suzuki >> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-03-17 10:45 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-09 15:27 [PATCH 0/2] coresight: Allow sharing of STM Suzuki K Poulose 2017-03-09 15:27 ` [PATCH 1/2] coresight: Disable the path only when the source is disabled Suzuki K Poulose 2017-03-14 17:37 ` [1/2] " Mathieu Poirier 2017-03-14 17:39 ` Suzuki K Poulose 2017-03-09 15:27 ` [PATCH 2/2] coresight: Fix reference count for software sources Suzuki K Poulose 2017-03-14 17:40 ` [2/2] " Mathieu Poirier 2017-03-14 17:55 ` Suzuki K Poulose 2017-03-14 19:32 ` Mathieu Poirier 2017-03-14 18:06 ` Suzuki K Poulose 2017-03-15 3:51 ` Chunyan Zhang 2017-03-15 16:00 ` Suzuki K Poulose 2017-03-16 12:13 ` Chunyan Zhang 2017-03-17 10:36 ` Suzuki K Poulose
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).