coresight: etm4x: Use explicit barriers on enable/disable

Commit Message

Mathieu Poirier Oct. 1, 2019, 5:14 p.m. UTC

From: Andrew Murray <[e-mail]>

commit 1004ce4c255fc3eb3ad9145ddd53547d1b7ce327 upstream

Synchronization is recommended before disabling the trace registers
to prevent any start or stop points being speculative at the point
of disabling the unit (section 7.3.77 of ARM IHI 0064D).

Synchronization is also recommended after programming the trace
registers to ensure all updates are committed prior to normal code
resuming (section 4.3.7 of ARM IHI 0064D).

Let's ensure these syncronization points are present in the code
and clearly commented.

Note that we could rely on the barriers in CS_LOCK and
coresight_disclaim_device_unlocked or the context switch to user
space - however coresight may be of use in the kernel.

On armv8 the mb macro is defined as dsb(sy) - Given that the etm4x is
only used on armv8 let's directly use dsb(sy) instead of mb(). This
removes some ambiguity and makes it easier to correlate the code with
the TRM.

Signed-off-by: Andrew Murray <[e-mail]>
Reviewed-by: Suzuki K Poulose <[e-mail]>
[Fixed capital letter for "use" in title]
Signed-off-by: Mathieu Poirier <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]
Signed-off-by: Greg Kroah-Hartman <[e-mail]>
Cc: [e-mail] # 4.9+
Signed-off-by: Mathieu Poirier <[e-mail]>
---
 drivers/hwtracing/coresight/coresight-etm4x.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Greg KH Oct. 9, 2019, 9:35 a.m. UTC | #1

On Tue, Oct 01, 2019 at 11:14:11AM -0600, Mathieu Poirier wrote:
> From: Andrew Murray <[e-mail]>
> 
> commit 1004ce4c255fc3eb3ad9145ddd53547d1b7ce327 upstream
> 
> Synchronization is recommended before disabling the trace registers
> to prevent any start or stop points being speculative at the point
> of disabling the unit (section 7.3.77 of ARM IHI 0064D).
> 
> Synchronization is also recommended after programming the trace
> registers to ensure all updates are committed prior to normal code
> resuming (section 4.3.7 of ARM IHI 0064D).
> 
> Let's ensure these syncronization points are present in the code
> and clearly commented.
> 
> Note that we could rely on the barriers in CS_LOCK and
> coresight_disclaim_device_unlocked or the context switch to user
> space - however coresight may be of use in the kernel.
> 
> On armv8 the mb macro is defined as dsb(sy) - Given that the etm4x is
> only used on armv8 let's directly use dsb(sy) instead of mb(). This
> removes some ambiguity and makes it easier to correlate the code with
> the TRM.
> 
> Signed-off-by: Andrew Murray <[e-mail]>
> Reviewed-by: Suzuki K Poulose <[e-mail]>
> [Fixed capital letter for "use" in title]
> Signed-off-by: Mathieu Poirier <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]
> Signed-off-by: Greg Kroah-Hartman <[e-mail]>
> Cc: [e-mail] # 4.9+
> Signed-off-by: Mathieu Poirier <[e-mail]>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

Now queued up, thanks.

greg k-h

Patch

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index da27f8edba50..44d6c29e2644 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -181,6 +181,12 @@  static void etm4_enable_hw(void *info)
 	if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 0))
 		dev_err(drvdata->dev,
 			"timeout while waiting for Idle Trace Status\n");
+	/*
+	 * As recommended by section 4.3.7 ("Synchronization when using the
+	 * memory-mapped interface") of ARM IHI 0064D
+	 */
+	dsb(sy);
+	isb();
 
 	CS_LOCK(drvdata->base);
 
@@ -323,8 +329,12 @@  static void etm4_disable_hw(void *info)
 	/* EN, bit[0] Trace unit enable bit */
 	control &= ~0x1;
 
-	/* make sure everything completes before disabling */
-	mb();
+	/*
+	 * Make sure everything completes before disabling, as recommended
+	 * by section 7.3.77 ("TRCVICTLR, ViewInst Main Control Register,
+	 * SSTATUS") of ARM IHI 0064D
+	 */
+	dsb(sy);
 	isb();
 	writel_relaxed(control, drvdata->base + TRCPRGCTLR);