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

From: Leo Yan <leo.yan@linaro.org>
To: Andrew Murray <andrew.murray@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	coresight@lists.linaro.org, stable@vger.kernel.org,
	Sudeep Holla <sudeep.holla@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Mike Leach <mike.leach@linaro.org>
Subject: Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable
Date: Fri, 28 Jun 2019 10:45:29 +0800	[thread overview]
Message-ID: <20190628024529.GC20296@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <20190627083525.37463-3-andrew.murray@arm.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel