Hi Andrew,
On Thu, Jun 27, 2019 at 09:35:22AM +0100, Andrew Murray wrote:
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.
Signed-off-by: Andrew Murray andrew.murray@arm.com CC: stable@vger.kernel.org
drivers/hwtracing/coresight/coresight-etm4x.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index c89190d464ab..68e8e3954cef 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -188,6 +188,10 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) dev_err(etm_dev, "timeout while waiting for Idle Trace Status\n");
- /* As recommended by 4.3.7 of ARM IHI 0064D */
- dsb(sy);
- isb();
I read the spec, it recommends to use dsb/isb after accessing trace unit, so here I think dsb(sy) is the most safe way.
arm64 defines barrier in arch/arm64/include/asm/barrier.h:
#define mb() dsb(sy)
so here I suggest to use barriers:
mb(); isb();
I wrongly assumed that mb() is for dmb operations, but actually it's defined for dsb operation. So we should use it and this is a common function between arm64 and arm.
done: CS_LOCK(drvdata->base); @@ -454,7 +458,8 @@ static void etm4_disable_hw(void *info) control &= ~0x1; /* make sure everything completes before disabling */
- mb();
- /* As recommended by 7.3.77 of ARM IHI 0064D */
- dsb(sy);
Here the old code should be right, mb() is the same thing with dsb(sy).
So we don't need to change at here?
Thanks, Leo Yan
isb(); writel_relaxed(control, drvdata->base + TRCPRGCTLR); -- 2.21.0
CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight