[PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Andrew Murray]

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();
+
 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);
 	isb();
 	writel_relaxed(control, drvdata->base + TRCPRGCTLR);
 
-- 
2.21.0

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Suzuki K Poulose]

Hi Andrew,

On 27/06/2019 09:35, 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.

Please could you also mention why we switched from mb() ?

> 
> 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 */

nit: It would be good to mention the "section name" to help the reader
find the same on a different version of the document. Also within the same
version, this is listed in the subsection:
"Synchronization when using the memory-mapped interface"

Please could you update the comment to reflect the same ?

> +	dsb(sy);
> +	isb();
> +
>   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 */

Nit: This refers to completely unrelated section. Shouldn't this be the same
as above ?

With the above fixed:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Andrew Murray]

On Thu, Jun 27, 2019 at 10:16:17AM +0100, Suzuki K Poulose wrote:
> Hi Andrew,
> 
> On 27/06/2019 09:35, 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.
> 
> Please could you also mention why we switched from mb() ?

No problem.

> 
> > 
> > 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 */
> 
> nit: It would be good to mention the "section name" to help the reader
> find the same on a different version of the document. Also within the same
> version, this is listed in the subsection:
> "Synchronization when using the memory-mapped interface"
> 
> Please could you update the comment to reflect the same ?
> 

Yes sure.


> > +	dsb(sy);
> > +	isb();
> > +
> >   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 */
> 
> Nit: This refers to completely unrelated section. Shouldn't this be the same
> as above ?

Actually 4.3.7 relates to using dsb/isb after programming the trace unit
registers and indicates this is to 'ensure that all updates are committed to
the trace unit before normal code execution resumes'.

Whereas 7.3.77 (hidden awawy in the SSTATUS description) relates to using
dsb/isb before disabling the trace unit to 'prevent any start or stop points
being specualtive at the point of disabling the trace unit'.

Both sections suggest the same course of action - however I felt that the
description in 7.3.77 better related to the context of etm4_disable_hw.

Perhaps if I also add the section name, readers are more likely to find this
text?


> 
> With the above fixed:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Thanks,

Andrew Murray

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Leo Yan]

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

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Andrew Murray]

On Fri, Jun 28, 2019 at 10:45:29AM +0800, Leo Yan wrote:
> 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?

Correct - on arm64 there is no difference between mb and dsb(sy) so no
functional change on this hunk.

In repsonse to Suzuki's feedback on this patch, I've updated the commit
message to describe why I've made this change, as follows:
     
"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."

Does that make sense?

Thanks,

Andrew Murray

> 
> 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

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Leo Yan]

Hi Andrew,

On Fri, Jun 28, 2019 at 09:35:24AM +0100, Andrew Murray wrote:

[...]

> > > @@ -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?
> 
> Correct - on arm64 there is no difference between mb and dsb(sy) so no
> functional change on this hunk.
> 
> In repsonse to Suzuki's feedback on this patch, I've updated the commit
> message to describe why I've made this change, as follows:
>      
> "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."
> 
> Does that make sense?

On reason for preferring to use mb() rather than dsb(sy) is for
compatibility cross different architectures (armv7, armv8, and
so on ...).  Seems to me mb() is a general API and transparent for
architecture's difference.

dsb(sy) is quite dependent on specific Arm architecture, e.g. some old
Arm architecures might don't support dsb(sy); and we are not sure later
it will change for new architectures.

Thanks,
Leo Yan

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Andrew Murray]

On Fri, Jun 28, 2019 at 04:51:54PM +0800, Leo Yan wrote:
> Hi Andrew,
> 
> On Fri, Jun 28, 2019 at 09:35:24AM +0100, Andrew Murray wrote:
> 
> [...]
> 
> > > > @@ -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?
> > 
> > Correct - on arm64 there is no difference between mb and dsb(sy) so no
> > functional change on this hunk.
> > 
> > In repsonse to Suzuki's feedback on this patch, I've updated the commit
> > message to describe why I've made this change, as follows:
> >      
> > "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."
> > 
> > Does that make sense?
> 
> On reason for preferring to use mb() rather than dsb(sy) is for
> compatibility cross different architectures (armv7, armv8, and
> so on ...).  Seems to me mb() is a general API and transparent for
> architecture's difference.
> 
> dsb(sy) is quite dependent on specific Arm architecture, e.g. some old
> Arm architecures might don't support dsb(sy); and we are not sure later
> it will change for new architectures.

Yes but please note that the KConfig for this driver depends on ARM64.

Thanks,

Andrew Murray

> 
> Thanks,
> Leo Yan

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Leo Yan]

Hi Andrew,

On Fri, Jun 28, 2019 at 10:00:14AM +0100, Andrew Murray wrote:
> On Fri, Jun 28, 2019 at 04:51:54PM +0800, Leo Yan wrote:
> > Hi Andrew,
> > 
> > On Fri, Jun 28, 2019 at 09:35:24AM +0100, Andrew Murray wrote:
> > 
> > [...]
> > 
> > > > > @@ -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?
> > > 
> > > Correct - on arm64 there is no difference between mb and dsb(sy) so no
> > > functional change on this hunk.
> > > 
> > > In repsonse to Suzuki's feedback on this patch, I've updated the commit
> > > message to describe why I've made this change, as follows:
> > >      
> > > "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."
> > > 
> > > Does that make sense?
> > 
> > On reason for preferring to use mb() rather than dsb(sy) is for
> > compatibility cross different architectures (armv7, armv8, and
> > so on ...).  Seems to me mb() is a general API and transparent for
> > architecture's difference.
> > 
> > dsb(sy) is quite dependent on specific Arm architecture, e.g. some old
> > Arm architecures might don't support dsb(sy); and we are not sure later
> > it will change for new architectures.
> 
> Yes but please note that the KConfig for this driver depends on ARM64.

Understood your point.

I am a bit suspect it's right thing to always set dependency on ARM64
for ETMv4 driver.  The reason is Armv8 CPU can also run with aarch32
mode in EL1.

If we let ETMv4 driver to support both aarch32 and aarch64, then we
will see dsb(sy) might break building for some old Arm arches.

Thanks,
Leo Yan

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Suzuki K Poulose]

Hi Leo,

On 28/06/2019 10:41, Leo Yan wrote:
> Hi Andrew,
> 
> On Fri, Jun 28, 2019 at 10:00:14AM +0100, Andrew Murray wrote:
>> On Fri, Jun 28, 2019 at 04:51:54PM +0800, Leo Yan wrote:
>>> Hi Andrew,
>>>
>>> On Fri, Jun 28, 2019 at 09:35:24AM +0100, Andrew Murray wrote:
>>>
>>> [...]
>>>
>>>>>> @@ -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?
>>>>
>>>> Correct - on arm64 there is no difference between mb and dsb(sy) so no
>>>> functional change on this hunk.
>>>>
>>>> In repsonse to Suzuki's feedback on this patch, I've updated the commit
>>>> message to describe why I've made this change, as follows:
>>>>       
>>>> "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."
>>>>
>>>> Does that make sense?
>>>
>>> On reason for preferring to use mb() rather than dsb(sy) is for
>>> compatibility cross different architectures (armv7, armv8, and
>>> so on ...).  Seems to me mb() is a general API and transparent for
>>> architecture's difference.
>>>
>>> dsb(sy) is quite dependent on specific Arm architecture, e.g. some old
>>> Arm architecures might don't support dsb(sy); and we are not sure later
>>> it will change for new architectures.
>>
>> Yes but please note that the KConfig for this driver depends on ARM64.
> 
> Understood your point.
> 
> I am a bit suspect it's right thing to always set dependency on ARM64
> for ETMv4 driver.  The reason is Armv8 CPU can also run with aarch32
> mode in EL1.
> 
> If we let ETMv4 driver to support both aarch32 and aarch64, then we
> will see dsb(sy) might break building for some old Arm arches.

If we add support for ETMv4 on aarch32, I would recommend adding a "dsb"
explicitly for aarch32 to make sure, it doesn't default to something else
that the mb() may cover up as. There is no point in creating another level
of indirection when the architecture is clear about it and the ETMv4 supporting
architectures must implement "dsb". Had this been in a generic code, I would
be happy to retain mb(). But this is specific to the ETMv4 driver and we know
that dsb must be there.

Cheers
Suzuki

Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Leo Yan]

Hi Suzuki,

On Mon, Jul 01, 2019 at 09:58:29AM +0100, Suzuki K Poulose wrote:
> Hi Leo,
> 
> On 28/06/2019 10:41, Leo Yan wrote:
> > Hi Andrew,
> > 
> > On Fri, Jun 28, 2019 at 10:00:14AM +0100, Andrew Murray wrote:
> > > On Fri, Jun 28, 2019 at 04:51:54PM +0800, Leo Yan wrote:
> > > > Hi Andrew,
> > > > 
> > > > On Fri, Jun 28, 2019 at 09:35:24AM +0100, Andrew Murray wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > > @@ -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?
> > > > > 
> > > > > Correct - on arm64 there is no difference between mb and dsb(sy) so no
> > > > > functional change on this hunk.
> > > > > 
> > > > > In repsonse to Suzuki's feedback on this patch, I've updated the commit
> > > > > message to describe why I've made this change, as follows:
> > > > > "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."
> > > > > 
> > > > > Does that make sense?
> > > > 
> > > > On reason for preferring to use mb() rather than dsb(sy) is for
> > > > compatibility cross different architectures (armv7, armv8, and
> > > > so on ...).  Seems to me mb() is a general API and transparent for
> > > > architecture's difference.
> > > > 
> > > > dsb(sy) is quite dependent on specific Arm architecture, e.g. some old
> > > > Arm architecures might don't support dsb(sy); and we are not sure later
> > > > it will change for new architectures.
> > > 
> > > Yes but please note that the KConfig for this driver depends on ARM64.
> > 
> > Understood your point.
> > 
> > I am a bit suspect it's right thing to always set dependency on ARM64
> > for ETMv4 driver.  The reason is Armv8 CPU can also run with aarch32
> > mode in EL1.
> > 
> > If we let ETMv4 driver to support both aarch32 and aarch64, then we
> > will see dsb(sy) might break building for some old Arm arches.
> 
> If we add support for ETMv4 on aarch32, I would recommend adding a "dsb"
> explicitly for aarch32 to make sure, it doesn't default to something else
> that the mb() may cover up as.

For aarch32, mb() should work well with below definition:

#ifdef CONFIG_ARM_HEAVY_MB
#define __arm_heavy_mb(x...) do { dsb(x); arm_heavy_mb(); } while (0)
#else
#define __arm_heavy_mb(x...) dsb(x)
#endif

#if defined(CONFIG_ARM_DMA_MEM_BUFFERABLE) || defined(CONFIG_SMP)
#define mb()		__arm_heavy_mb()
#else
#define mb()		barrier()
#endif

> There is no point in creating another level
> of indirection when the architecture is clear about it and the ETMv4 supporting
> architectures must implement "dsb". Had this been in a generic code, I would
> be happy to retain mb(). But this is specific to the ETMv4 driver and we know
> that dsb must be there.

Okay, I understand the purpose for more explict barrier in the code;
this would be fine for me.

Thanks,
Leo Yan