Re: [PATCH] ARM: mvebu: drop pointless check for coherency_base

[Gregory CLEMENT <gregory.clement@bootlin.com>]

Hi Ard,

> The MMU off code path in ll_get_coherency_base() attempts to decide
> whether the coherency fabric is mapped by testing the value of
> coherency_base, which carries its virtual address if its mapped, and
> 0x0 otherwise.
>
> However, what the code actually does is take the virtual address of
> the coherency_base symbol, and compare it with 0x0, which are never
> equal, and so the branch is never taken. In fact, with the MMU off,
> dereferencing the VA of coherency_base is not possible to begin with,
> nor can its value be relied upon with the MMU off since it is not
> cleaned to the Dcache as is done with coherency_phys_base in
> armada_370_coherency_init().
>
> Instead, the value of coherency_phys_base is returned, which results
> in the correct behavior since it will be 0x0 as well if the coherency
> fabric is not mapped, and it is accessible with the MMU off. So just
> drop the comparison and the branch.

Thanks for this patch. I needed to refresh my memories about this
subject and I agree with your explanation. I applied it on mvebu/fixes.

Gregory

>
> Fixes: 30cdef97107370a7 ("ARM: mvebu: make the coherency_ll.S functions work with no coherency fabric")
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> Found by accident. Build tested only.
>
>  arch/arm/mach-mvebu/coherency_ll.S | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
> index 2d962fe48821..a3a64bf97250 100644
> --- a/arch/arm/mach-mvebu/coherency_ll.S
> +++ b/arch/arm/mach-mvebu/coherency_ll.S
> @@ -35,13 +35,8 @@ ENTRY(ll_get_coherency_base)
>  
>  	/*
>  	 * MMU is disabled, use the physical address of the coherency
> -	 * base address. However, if the coherency fabric isn't mapped
> -	 * (i.e its virtual address is zero), it means coherency is
> -	 * not enabled, so we return 0.
> +	 * base address, (or 0x0 if the coherency fabric is not mapped)
>  	 */
> -	ldr	r1, =coherency_base
> -	cmp	r1, #0
> -	beq	2f
>  	adr	r1, 3f
>  	ldr	r3, [r1]
>  	ldr	r1, [r1, r3]
> -- 
> 2.17.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

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