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@xxxxxxx> > CC: stable@xxxxxxxxxxxxxxx > --- > 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@xxxxxxxxxxxxxxxx > https://lists.linaro.org/mailman/listinfo/coresight
