| Message ID | 20190618125433.9739-3-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm4x: save/restore ETMv4 context across CPU low power states | expand |
On Tue, Jun 18, 2019 at 01:54:30PM +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> > --- > 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 da7cf74d612b..ae623415c431 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > @@ -187,6 +187,10 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) > dev_err(drvdata->dev, > "timeout while waiting for Idle Trace Status\n"); > > + /* As recommended by 4.3.7 of ARM IHI 0064D */ > + dsb(sy); > + isb(); > + > done: > CS_LOCK(drvdata->base); > > @@ -453,7 +457,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); As far as I can tell mb() is equal to dsb(sy). > isb(); > writel_relaxed(control, drvdata->base + TRCPRGCTLR); > > -- > 2.21.0 >
On 18/06/2019 23:34, Mathieu Poirier wrote: > On Tue, Jun 18, 2019 at 01:54:30PM +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> >> --- >> 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 da7cf74d612b..ae623415c431 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c >> @@ -187,6 +187,10 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) >> dev_err(drvdata->dev, >> "timeout while waiting for Idle Trace Status\n"); >> >> + /* As recommended by 4.3.7 of ARM IHI 0064D */ >> + dsb(sy); >> + isb(); >> + I think this should go to stable. Please Cc stable in the next version. or alternately you may add a "fixes" tag. >> done: >> CS_LOCK(drvdata->base); >> >> @@ -453,7 +457,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); > > As far as I can tell mb() is equal to dsb(sy). Yes, however, given that etm4x is for armv8 cores, having the explicit dsb() make sense to avoid someone looking up again to see what mb() translates to. I suggested the dsb() over mb(), so that it is evident to someone who is trying to correlate the TRM vs the code. Cheers Suzuki
On Wed, Jun 19, 2019 at 09:32:56AM +0100, Suzuki K Poulose wrote: > > > On 18/06/2019 23:34, Mathieu Poirier wrote: > > On Tue, Jun 18, 2019 at 01:54:30PM +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> > > > --- > > > 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 da7cf74d612b..ae623415c431 100644 > > > --- a/drivers/hwtracing/coresight/coresight-etm4x.c > > > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > > > @@ -187,6 +187,10 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) > > > dev_err(drvdata->dev, > > > "timeout while waiting for Idle Trace Status\n"); > > > + /* As recommended by 4.3.7 of ARM IHI 0064D */ > > > + dsb(sy); > > > + isb(); > > > + > > I think this should go to stable. Please Cc stable in the next version. or > alternately you may add a "fixes" tag. Sure I'll CC stable. > > > > done: > > > CS_LOCK(drvdata->base); > > > @@ -453,7 +457,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); > > > > As far as I can tell mb() is equal to dsb(sy). > > Yes, however, given that etm4x is for armv8 cores, having the explicit dsb() > make sense to avoid someone looking up again to see what mb() translates to. > I suggested the dsb() over mb(), so that it is evident to someone who is trying > to correlate the TRM vs the code. As the TRM explictly mentions dsb, I'd prefer to explictly do this in the code rather than rely on whatever mb will be in the future. Thanks, Andrew Murray > > > Cheers > Suzuki
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index da7cf74d612b..ae623415c431 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -187,6 +187,10 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) dev_err(drvdata->dev, "timeout while waiting for Idle Trace Status\n"); + /* As recommended by 4.3.7 of ARM IHI 0064D */ + dsb(sy); + isb(); + done: CS_LOCK(drvdata->base); @@ -453,7 +457,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); isb(); writel_relaxed(control, drvdata->base + TRCPRGCTLR);
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> --- drivers/hwtracing/coresight/coresight-etm4x.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)