Re: [PATCH] coresight: STM: Balance enable/disable
On 19/01/17 11:40, Greg KH wrote: Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight STM component") Cc: Pratik Patel Cc: Greg Kroah-Hartman Cc: sta...@vger.kernel.org # 4.7+ Acked-by: Mathieu Poirier Reviewed-by: Chunyan Zhang Reported-by: Robert Walker Signed-off-by: Suzuki K Poulose --- Greg, Without this patch, the coresight STM IP can only be used for one tracing session per boot, seriously limiting its usability. When you resend a patch, please tell me what is different from the previous version you sent. I figured it out here, but please do this next time. Hi Greg, Sure, will keep that in mind. For this patch nothing has changed, except for the addition of Review/Ack. It was resent (with to: you) just to make sure it gets through the rc series. Thanks for queuing it. Suzuki
Re: [PATCH] coresight: STM: Balance enable/disable
On Mon, Jan 16, 2017 at 06:00:00PM +, Suzuki K Poulose wrote: > The stm is automatically enabled when an application sets the policy > via ->link() call back by using coresight_enable(), which keeps the > refcount of the current users of the STM. However, the unlink() callback > issues stm_disable() directly, which leaves the STM turned off, without > the coresight layer knowing about it. This prevents any further uses > of the STM hardware as the coresight layer still thinks the STM is > turned on and doesn't enable the hardware when required. Even manually > enabling the STM via sysfs can't really enable the hw. > > e.g, > > $ echo 1 > $CS_DEVS/$ETR/enable_sink > $ mkdir -p $CONFIG_FS/stp-policy/$source.0/stm_test/ > $ echo 32768 65535 > $CONFIG_FS/stp-policy/$source.0/stm_test/channels > $ echo 64 > $CS_DEVS/$source/traceid > $ ./stm_app > Sending 64000 byte blocks of pattern 0 at 0us intervals > Success to map channel(32768~32783) to 0xa95fa000 > Sending on channel 32768 > $ dd if=/dev/$ETR of=~/trace.bin.1 > 597+1 records in > 597+1 records out > 305920 bytes (306 kB) copied, 0.399952 s, 765 kB/s > $ ./stm_app > Sending 64000 byte blocks of pattern 0 at 0us intervals > Success to map channel(32768~32783) to 0x7e9e2000 > Sending on channel 32768 > $ dd if=/dev/$ETR of=~/trace.bin.2 > 0+0 records in > 0+0 records out > 0 bytes (0 B) copied, 0.0232083 s, 0.0 kB/s > > Note that we don't get any data from the ETR for the second session. > > Also dmesg shows : > > [ 77.520458] coresight-tmc 2080.etr: TMC-ETR enabled > [ 77.537097] coresight-replicator etr_replicator@2089: REPLICATOR > enabled > [ 77.558828] coresight-replicator main_replicator@208a: REPLICATOR > enabled > [ 77.581068] coresight-funnel 208c.main_funnel: FUNNEL inport 0 enabled > [ 77.602217] coresight-tmc 2084.etf: TMC-ETF enabled > [ 77.618422] coresight-stm 2086.stm: STM tracing enabled > [ 139.554252] coresight-stm 2086.stm: STM tracing disabled > # End of first tracing session > [ 146.351135] coresight-tmc 2080.etr: TMC read start > [ 146.514486] coresight-tmc 2080.etr: TMC read end > # Note that the STM is not turned on via > stm_generic_link()->coresight_enable() > # and hence none of the components are turned on. > [ 152.479080] coresight-tmc 2080.etr: TMC read start > [ 152.542632] coresight-tmc 2080.etr: TMC read end > > This patch fixes the problem by balancing the unlink operation by using > the coresight_disable(), keeping the coresight layer in sync with the > hardware state and thus allowing normal usage of the STM component. > > Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight STM > component") > Cc: Pratik Patel > Cc: Greg Kroah-Hartman > Cc: sta...@vger.kernel.org # 4.7+ > Acked-by: Mathieu Poirier > Reviewed-by: Chunyan Zhang > Reported-by: Robert Walker > Signed-off-by: Suzuki K Poulose > --- > > Greg, > > Without this patch, the coresight STM IP can only be used for one tracing > session per boot, seriously limiting its usability. When you resend a patch, please tell me what is different from the previous version you sent. I figured it out here, but please do this next time. thanks, greg k-h
Re: [PATCH] coresight: STM: Balance enable/disable
On 13 January 2017 at 10:11, Suzuki K Poulose wrote: > On 13/01/17 16:48, Mathieu Poirier wrote: >> >> On 10 January 2017 at 04:21, Suzuki K Poulose >> wrote: >>> >>> The stm is automatically enabled when an application sets the policy >>> via ->link() call back by using coresight_enable(), which keeps the >>> refcount of the current users of the STM. However, the unlink() callback >>> issues stm_disable() directly, which leaves the STM turned off, without >>> the coresight layer knowing about it. This prevents any further uses >>> of the STM hardware as the coresight layer still thinks the STM is >>> turned on and doesn't issue an stm_enable(). Even manually enabling >>> the STM via sysfs can't really enable the hw. >>> > ... >>> >>> >>> This patch balances the unlink operation by using the >>> coresight_disable(), >>> keeping the coresight layer in sync with the hardware state. >>> >>> Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight >>> STM component") >>> Cc: Pratik Patel >>> Cc: Mathieu Poirier >>> Cc: Chunyan Zhang >>> Cc: Greg Kroah-Hartman >>> Cc: sta...@vger.kernel.org # 4.7+ >>> Reported-by: Robert Walker >>> Signed-off-by: Suzuki K Poulose >>> --- >>> drivers/hwtracing/coresight/coresight-stm.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-stm.c >>> b/drivers/hwtracing/coresight/coresight-stm.c >>> index 3524452..57b7330 100644 >>> --- a/drivers/hwtracing/coresight/coresight-stm.c >>> +++ b/drivers/hwtracing/coresight/coresight-stm.c >>> @@ -356,7 +356,7 @@ static void stm_generic_unlink(struct stm_data >>> *stm_data, >>> if (!drvdata || !drvdata->csdev) >>> return; >>> >>> - stm_disable(drvdata->csdev, NULL); >>> + coresight_disable(drvdata->csdev); >>> } >>> >>> static phys_addr_t >> >> >> Applied - thanks, > > > Mathieu, Greg, > > I think this should go into 4.10 (either way, as fix in this cycle or via > stable after the release). I think > it would be easier if it goes in as fix during one of these rc cycle. > > Please let me know your thoughts. I'm good with squeezing this patch in the 4.10 cycle. From here I suppose the easiest for Greg is for you to send another patch with Chunyan's Reviewed-by and my ack. > > Suzuki > >> Mathieu >> >>> -- >>> 2.7.4 >>> >
Re: [PATCH] coresight: STM: Balance enable/disable
On 13/01/17 16:48, Mathieu Poirier wrote: On 10 January 2017 at 04:21, Suzuki K Poulose wrote: The stm is automatically enabled when an application sets the policy via ->link() call back by using coresight_enable(), which keeps the refcount of the current users of the STM. However, the unlink() callback issues stm_disable() directly, which leaves the STM turned off, without the coresight layer knowing about it. This prevents any further uses of the STM hardware as the coresight layer still thinks the STM is turned on and doesn't issue an stm_enable(). Even manually enabling the STM via sysfs can't really enable the hw. ... This patch balances the unlink operation by using the coresight_disable(), keeping the coresight layer in sync with the hardware state. Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight STM component") Cc: Pratik Patel Cc: Mathieu Poirier Cc: Chunyan Zhang Cc: Greg Kroah-Hartman Cc: sta...@vger.kernel.org # 4.7+ Reported-by: Robert Walker Signed-off-by: Suzuki K Poulose --- drivers/hwtracing/coresight/coresight-stm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index 3524452..57b7330 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -356,7 +356,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, if (!drvdata || !drvdata->csdev) return; - stm_disable(drvdata->csdev, NULL); + coresight_disable(drvdata->csdev); } static phys_addr_t Applied - thanks, Mathieu, Greg, I think this should go into 4.10 (either way, as fix in this cycle or via stable after the release). I think it would be easier if it goes in as fix during one of these rc cycle. Please let me know your thoughts. Suzuki Mathieu -- 2.7.4
Re: [PATCH] coresight: STM: Balance enable/disable
On 10 January 2017 at 04:21, Suzuki K Poulose wrote: > The stm is automatically enabled when an application sets the policy > via ->link() call back by using coresight_enable(), which keeps the > refcount of the current users of the STM. However, the unlink() callback > issues stm_disable() directly, which leaves the STM turned off, without > the coresight layer knowing about it. This prevents any further uses > of the STM hardware as the coresight layer still thinks the STM is > turned on and doesn't issue an stm_enable(). Even manually enabling > the STM via sysfs can't really enable the hw. > > e.g, > > $ echo 1 > $CS_DEVS/$ETR/enable_sink > $ mkdir -p $CONFIG_FS/stp-policy/$source.0/stm_test/ > $ echo 32768 65535 > $CONFIG_FS/stp-policy/$source.0/stm_test/channels > $ echo 64 > $CS_DEVS/$source/traceid > $ ./stm_app > Sending 64000 byte blocks of pattern 0 at 0us intervals > Success to map channel(32768~32783) to 0xa95fa000 > Sending on channel 32768 > $ dd if=/dev/$ETR of=~/trace.bin.1 > 597+1 records in > 597+1 records out > 305920 bytes (306 kB) copied, 0.399952 s, 765 kB/s > $ ./stm_app > Sending 64000 byte blocks of pattern 0 at 0us intervals > Success to map channel(32768~32783) to 0x7e9e2000 > Sending on channel 32768 > $ dd if=/dev/$ETR of=~/trace.bin.2 > 0+0 records in > 0+0 records out > 0 bytes (0 B) copied, 0.0232083 s, 0.0 kB/s > > Note that we don't get any data from the ETR for the second session. > > Also dmesg shows : > > [ 77.520458] coresight-tmc 2080.etr: TMC-ETR enabled > [ 77.537097] coresight-replicator etr_replicator@2089: REPLICATOR > enabled > [ 77.558828] coresight-replicator main_replicator@208a: REPLICATOR > enabled > [ 77.581068] coresight-funnel 208c.main_funnel: FUNNEL inport 0 enabled > [ 77.602217] coresight-tmc 2084.etf: TMC-ETF enabled > [ 77.618422] coresight-stm 2086.stm: STM tracing enabled > [ 139.554252] coresight-stm 2086.stm: STM tracing disabled > # End of first tracing session > [ 146.351135] coresight-tmc 2080.etr: TMC read start > [ 146.514486] coresight-tmc 2080.etr: TMC read end > # Note that the STM is not turned on via > stm_generic_link()->coresight_enable() > # and hence none of the components are turned on. > [ 152.479080] coresight-tmc 2080.etr: TMC read start > [ 152.542632] coresight-tmc 2080.etr: TMC read end > > This patch balances the unlink operation by using the coresight_disable(), > keeping the coresight layer in sync with the hardware state. > > Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight STM > component") > Cc: Pratik Patel > Cc: Mathieu Poirier > Cc: Chunyan Zhang > Cc: Greg Kroah-Hartman > Cc: sta...@vger.kernel.org # 4.7+ > Reported-by: Robert Walker > Signed-off-by: Suzuki K Poulose > --- > drivers/hwtracing/coresight/coresight-stm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-stm.c > b/drivers/hwtracing/coresight/coresight-stm.c > index 3524452..57b7330 100644 > --- a/drivers/hwtracing/coresight/coresight-stm.c > +++ b/drivers/hwtracing/coresight/coresight-stm.c > @@ -356,7 +356,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, > if (!drvdata || !drvdata->csdev) > return; > > - stm_disable(drvdata->csdev, NULL); > + coresight_disable(drvdata->csdev); > } > > static phys_addr_t Applied - thanks, Mathieu > -- > 2.7.4 >
Re: [PATCH] coresight: STM: Balance enable/disable
On 11 January 2017 at 21:59, Suzuki K Poulose wrote: > On 11/01/17 11:41, Chunyan Zhang wrote: >> >> On 11 January 2017 at 01:36, Mathieu Poirier >> wrote: >>> >>> On Tue, Jan 10, 2017 at 11:21:55AM +, Suzuki K Poulose wrote: The stm is automatically enabled when an application sets the policy via ->link() call back by using coresight_enable(), which keeps the refcount of the current users of the STM. However, the unlink() callback issues stm_disable() directly, which leaves the STM turned off, without the coresight layer knowing about it. This prevents any further uses of the STM hardware as the coresight layer still thinks the STM is turned on and doesn't issue an stm_enable(). Even manually enabling the STM via sysfs can't really enable the hw. e.g, $ echo 1 > $CS_DEVS/$ETR/enable_sink $ mkdir -p $CONFIG_FS/stp-policy/$source.0/stm_test/ $ echo 32768 65535 > $CONFIG_FS/stp-policy/$source.0/stm_test/channels $ echo 64 > $CS_DEVS/$source/traceid $ ./stm_app Sending 64000 byte blocks of pattern 0 at 0us intervals Success to map channel(32768~32783) to 0xa95fa000 Sending on channel 32768 $ dd if=/dev/$ETR of=~/trace.bin.1 597+1 records in 597+1 records out 305920 bytes (306 kB) copied, 0.399952 s, 765 kB/s $ ./stm_app Sending 64000 byte blocks of pattern 0 at 0us intervals Success to map channel(32768~32783) to 0x7e9e2000 Sending on channel 32768 $ dd if=/dev/$ETR of=~/trace.bin.2 0+0 records in 0+0 records out 0 bytes (0 B) copied, 0.0232083 s, 0.0 kB/s Note that we don't get any data from the ETR for the second session. Also dmesg shows : [ 77.520458] coresight-tmc 2080.etr: TMC-ETR enabled [ 77.537097] coresight-replicator etr_replicator@2089: REPLICATOR enabled [ 77.558828] coresight-replicator main_replicator@208a: REPLICATOR enabled [ 77.581068] coresight-funnel 208c.main_funnel: FUNNEL inport 0 enabled [ 77.602217] coresight-tmc 2084.etf: TMC-ETF enabled [ 77.618422] coresight-stm 2086.stm: STM tracing enabled [ 139.554252] coresight-stm 2086.stm: STM tracing disabled # End of first tracing session [ 146.351135] coresight-tmc 2080.etr: TMC read start [ 146.514486] coresight-tmc 2080.etr: TMC read end # Note that the STM is not turned on via stm_generic_link()->coresight_enable() # and hence none of the components are turned on. [ 152.479080] coresight-tmc 2080.etr: TMC read start [ 152.542632] coresight-tmc 2080.etr: TMC read end This patch balances the unlink operation by using the coresight_disable(), keeping the coresight layer in sync with the hardware state. Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight STM component") Cc: Pratik Patel Cc: Mathieu Poirier Cc: Chunyan Zhang Cc: Greg Kroah-Hartman Cc: sta...@vger.kernel.org # 4.7+ Reported-by: Robert Walker Signed-off-by: Suzuki K Poulose --- drivers/hwtracing/coresight/coresight-stm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index 3524452..57b7330 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -356,7 +356,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, if (!drvdata || !drvdata->csdev) return; - stm_disable(drvdata->csdev, NULL); + coresight_disable(drvdata->csdev); >>> >>> >>> This looks valid to me. >>> >>> Chunyan, any reason to use stm_disable() directly rather than calling it >>> as part >>> of the device OPS in coresight_disable()? >> >> >> I don't think there's some special reason for this. I simply hadn't >> noticed that these two operations didn't use two balanced functions. > > > Please can I have an Ack/Reviewed -by on it, so that we can push it > as a fix. Sure, I've had a run with this patch, it works well, so, Reviewed-by: Chunyan Zhang Thanks, Chunyan > > > Suzuki >
Re: [PATCH] coresight: STM: Balance enable/disable
On 11/01/17 11:41, Chunyan Zhang wrote: On 11 January 2017 at 01:36, Mathieu Poirier wrote: On Tue, Jan 10, 2017 at 11:21:55AM +, Suzuki K Poulose wrote: The stm is automatically enabled when an application sets the policy via ->link() call back by using coresight_enable(), which keeps the refcount of the current users of the STM. However, the unlink() callback issues stm_disable() directly, which leaves the STM turned off, without the coresight layer knowing about it. This prevents any further uses of the STM hardware as the coresight layer still thinks the STM is turned on and doesn't issue an stm_enable(). Even manually enabling the STM via sysfs can't really enable the hw. e.g, $ echo 1 > $CS_DEVS/$ETR/enable_sink $ mkdir -p $CONFIG_FS/stp-policy/$source.0/stm_test/ $ echo 32768 65535 > $CONFIG_FS/stp-policy/$source.0/stm_test/channels $ echo 64 > $CS_DEVS/$source/traceid $ ./stm_app Sending 64000 byte blocks of pattern 0 at 0us intervals Success to map channel(32768~32783) to 0xa95fa000 Sending on channel 32768 $ dd if=/dev/$ETR of=~/trace.bin.1 597+1 records in 597+1 records out 305920 bytes (306 kB) copied, 0.399952 s, 765 kB/s $ ./stm_app Sending 64000 byte blocks of pattern 0 at 0us intervals Success to map channel(32768~32783) to 0x7e9e2000 Sending on channel 32768 $ dd if=/dev/$ETR of=~/trace.bin.2 0+0 records in 0+0 records out 0 bytes (0 B) copied, 0.0232083 s, 0.0 kB/s Note that we don't get any data from the ETR for the second session. Also dmesg shows : [ 77.520458] coresight-tmc 2080.etr: TMC-ETR enabled [ 77.537097] coresight-replicator etr_replicator@2089: REPLICATOR enabled [ 77.558828] coresight-replicator main_replicator@208a: REPLICATOR enabled [ 77.581068] coresight-funnel 208c.main_funnel: FUNNEL inport 0 enabled [ 77.602217] coresight-tmc 2084.etf: TMC-ETF enabled [ 77.618422] coresight-stm 2086.stm: STM tracing enabled [ 139.554252] coresight-stm 2086.stm: STM tracing disabled # End of first tracing session [ 146.351135] coresight-tmc 2080.etr: TMC read start [ 146.514486] coresight-tmc 2080.etr: TMC read end # Note that the STM is not turned on via stm_generic_link()->coresight_enable() # and hence none of the components are turned on. [ 152.479080] coresight-tmc 2080.etr: TMC read start [ 152.542632] coresight-tmc 2080.etr: TMC read end This patch balances the unlink operation by using the coresight_disable(), keeping the coresight layer in sync with the hardware state. Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight STM component") Cc: Pratik Patel Cc: Mathieu Poirier Cc: Chunyan Zhang Cc: Greg Kroah-Hartman Cc: sta...@vger.kernel.org # 4.7+ Reported-by: Robert Walker Signed-off-by: Suzuki K Poulose --- drivers/hwtracing/coresight/coresight-stm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index 3524452..57b7330 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -356,7 +356,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, if (!drvdata || !drvdata->csdev) return; - stm_disable(drvdata->csdev, NULL); + coresight_disable(drvdata->csdev); This looks valid to me. Chunyan, any reason to use stm_disable() directly rather than calling it as part of the device OPS in coresight_disable()? I don't think there's some special reason for this. I simply hadn't noticed that these two operations didn't use two balanced functions. Please can I have an Ack/Reviewed -by on it, so that we can push it as a fix. Suzuki
Re: [PATCH] coresight: STM: Balance enable/disable
On 11 January 2017 at 01:36, Mathieu Poirier wrote: > On Tue, Jan 10, 2017 at 11:21:55AM +, Suzuki K Poulose wrote: >> The stm is automatically enabled when an application sets the policy >> via ->link() call back by using coresight_enable(), which keeps the >> refcount of the current users of the STM. However, the unlink() callback >> issues stm_disable() directly, which leaves the STM turned off, without >> the coresight layer knowing about it. This prevents any further uses >> of the STM hardware as the coresight layer still thinks the STM is >> turned on and doesn't issue an stm_enable(). Even manually enabling >> the STM via sysfs can't really enable the hw. >> >> e.g, >> >> $ echo 1 > $CS_DEVS/$ETR/enable_sink >> $ mkdir -p $CONFIG_FS/stp-policy/$source.0/stm_test/ >> $ echo 32768 65535 > $CONFIG_FS/stp-policy/$source.0/stm_test/channels >> $ echo 64 > $CS_DEVS/$source/traceid >> $ ./stm_app >> Sending 64000 byte blocks of pattern 0 at 0us intervals >> Success to map channel(32768~32783) to 0xa95fa000 >> Sending on channel 32768 >> $ dd if=/dev/$ETR of=~/trace.bin.1 >> 597+1 records in >> 597+1 records out >> 305920 bytes (306 kB) copied, 0.399952 s, 765 kB/s >> $ ./stm_app >> Sending 64000 byte blocks of pattern 0 at 0us intervals >> Success to map channel(32768~32783) to 0x7e9e2000 >> Sending on channel 32768 >> $ dd if=/dev/$ETR of=~/trace.bin.2 >> 0+0 records in >> 0+0 records out >> 0 bytes (0 B) copied, 0.0232083 s, 0.0 kB/s >> >> Note that we don't get any data from the ETR for the second session. >> >> Also dmesg shows : >> >> [ 77.520458] coresight-tmc 2080.etr: TMC-ETR enabled >> [ 77.537097] coresight-replicator etr_replicator@2089: REPLICATOR >> enabled >> [ 77.558828] coresight-replicator main_replicator@208a: REPLICATOR >> enabled >> [ 77.581068] coresight-funnel 208c.main_funnel: FUNNEL inport 0 enabled >> [ 77.602217] coresight-tmc 2084.etf: TMC-ETF enabled >> [ 77.618422] coresight-stm 2086.stm: STM tracing enabled >> [ 139.554252] coresight-stm 2086.stm: STM tracing disabled >> # End of first tracing session >> [ 146.351135] coresight-tmc 2080.etr: TMC read start >> [ 146.514486] coresight-tmc 2080.etr: TMC read end >> # Note that the STM is not turned on via >> stm_generic_link()->coresight_enable() >> # and hence none of the components are turned on. >> [ 152.479080] coresight-tmc 2080.etr: TMC read start >> [ 152.542632] coresight-tmc 2080.etr: TMC read end >> >> This patch balances the unlink operation by using the coresight_disable(), >> keeping the coresight layer in sync with the hardware state. >> >> Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight >> STM component") >> Cc: Pratik Patel >> Cc: Mathieu Poirier >> Cc: Chunyan Zhang >> Cc: Greg Kroah-Hartman >> Cc: sta...@vger.kernel.org # 4.7+ >> Reported-by: Robert Walker >> Signed-off-by: Suzuki K Poulose >> --- >> drivers/hwtracing/coresight/coresight-stm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-stm.c >> b/drivers/hwtracing/coresight/coresight-stm.c >> index 3524452..57b7330 100644 >> --- a/drivers/hwtracing/coresight/coresight-stm.c >> +++ b/drivers/hwtracing/coresight/coresight-stm.c >> @@ -356,7 +356,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, >> if (!drvdata || !drvdata->csdev) >> return; >> >> - stm_disable(drvdata->csdev, NULL); >> + coresight_disable(drvdata->csdev); > > This looks valid to me. > > Chunyan, any reason to use stm_disable() directly rather than calling it as > part > of the device OPS in coresight_disable()? I don't think there's some special reason for this. I simply hadn't noticed that these two operations didn't use two balanced functions. Thanks, Chunyan > > Thanks, > Mathieu > >> } >> >> static phys_addr_t >> -- >> 2.7.4 >>
Re: [PATCH] coresight: STM: Balance enable/disable
On Tue, Jan 10, 2017 at 11:21:55AM +, Suzuki K Poulose wrote: > The stm is automatically enabled when an application sets the policy > via ->link() call back by using coresight_enable(), which keeps the > refcount of the current users of the STM. However, the unlink() callback > issues stm_disable() directly, which leaves the STM turned off, without > the coresight layer knowing about it. This prevents any further uses > of the STM hardware as the coresight layer still thinks the STM is > turned on and doesn't issue an stm_enable(). Even manually enabling > the STM via sysfs can't really enable the hw. > > e.g, > > $ echo 1 > $CS_DEVS/$ETR/enable_sink > $ mkdir -p $CONFIG_FS/stp-policy/$source.0/stm_test/ > $ echo 32768 65535 > $CONFIG_FS/stp-policy/$source.0/stm_test/channels > $ echo 64 > $CS_DEVS/$source/traceid > $ ./stm_app > Sending 64000 byte blocks of pattern 0 at 0us intervals > Success to map channel(32768~32783) to 0xa95fa000 > Sending on channel 32768 > $ dd if=/dev/$ETR of=~/trace.bin.1 > 597+1 records in > 597+1 records out > 305920 bytes (306 kB) copied, 0.399952 s, 765 kB/s > $ ./stm_app > Sending 64000 byte blocks of pattern 0 at 0us intervals > Success to map channel(32768~32783) to 0x7e9e2000 > Sending on channel 32768 > $ dd if=/dev/$ETR of=~/trace.bin.2 > 0+0 records in > 0+0 records out > 0 bytes (0 B) copied, 0.0232083 s, 0.0 kB/s > > Note that we don't get any data from the ETR for the second session. > > Also dmesg shows : > > [ 77.520458] coresight-tmc 2080.etr: TMC-ETR enabled > [ 77.537097] coresight-replicator etr_replicator@2089: REPLICATOR > enabled > [ 77.558828] coresight-replicator main_replicator@208a: REPLICATOR > enabled > [ 77.581068] coresight-funnel 208c.main_funnel: FUNNEL inport 0 enabled > [ 77.602217] coresight-tmc 2084.etf: TMC-ETF enabled > [ 77.618422] coresight-stm 2086.stm: STM tracing enabled > [ 139.554252] coresight-stm 2086.stm: STM tracing disabled > # End of first tracing session > [ 146.351135] coresight-tmc 2080.etr: TMC read start > [ 146.514486] coresight-tmc 2080.etr: TMC read end > # Note that the STM is not turned on via > stm_generic_link()->coresight_enable() > # and hence none of the components are turned on. > [ 152.479080] coresight-tmc 2080.etr: TMC read start > [ 152.542632] coresight-tmc 2080.etr: TMC read end > > This patch balances the unlink operation by using the coresight_disable(), > keeping the coresight layer in sync with the hardware state. > > Fixes: commit 237483aa5cf43 ("coresight: stm: adding driver for CoreSight STM > component") > Cc: Pratik Patel > Cc: Mathieu Poirier > Cc: Chunyan Zhang > Cc: Greg Kroah-Hartman > Cc: sta...@vger.kernel.org # 4.7+ > Reported-by: Robert Walker > Signed-off-by: Suzuki K Poulose > --- > drivers/hwtracing/coresight/coresight-stm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-stm.c > b/drivers/hwtracing/coresight/coresight-stm.c > index 3524452..57b7330 100644 > --- a/drivers/hwtracing/coresight/coresight-stm.c > +++ b/drivers/hwtracing/coresight/coresight-stm.c > @@ -356,7 +356,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, > if (!drvdata || !drvdata->csdev) > return; > > - stm_disable(drvdata->csdev, NULL); > + coresight_disable(drvdata->csdev); This looks valid to me. Chunyan, any reason to use stm_disable() directly rather than calling it as part of the device OPS in coresight_disable()? Thanks, Mathieu > } > > static phys_addr_t > -- > 2.7.4 >