linux-kernel - Re: [PATCH 5/8] power: reset: at91-reset: use at91_ramc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141022090810.167c470f@free-electrons.com>
Date:	Wed, 22 Oct 2014 09:08:10 +0200
From:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
To:	Alexandre Belloni <alexandre.belloni@...e-electrons.com>
Cc:	Nicolas Ferre <nicolas.ferre@...el.com>,
	Sebastian Reichel <sre@...nel.org>, linux-pm@...r.kernel.org,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	linux-kernel@...r.kernel.org,
	David Woodhouse <dwmw2@...radead.org>,
	linux-arm-kernel@...ts.infradead.org,
	Maxime Ripard <maxime.ripard@...e-electrons.com>
Subject: Re: [PATCH 5/8] power: reset: at91-reset: use at91_ramc_shutdown

Dear Alexandre Belloni,

On Tue, 21 Oct 2014 23:55:37 +0200, Alexandre Belloni wrote:

>  /*
>  * unless the SDRAM is cleanly shutdown before we hit the
>  * reset register it can be left driving the data bus and
>  * killing the chance of a subsequent boot from NAND
>  */
> -static void at91sam9260_restart(enum reboot_mode mode, const char *cmd)
> +static void at91_restart(enum reboot_mode mode, const char *cmd)
>  {
> -	asm volatile(
> -		/* Align to cache lines */
> -		".balign 32\n\t"
> -
> -		/* Disable SDRAM accesses */
> -		"str	%2, [%0, #" __stringify(AT91_SDRAMC_TR) "]\n\t"
> -
> -		/* Power down SDRAM */
> -		"str	%3, [%0, #" __stringify(AT91_SDRAMC_LPR) "]\n\t"
> +	if (at91_ramc_shutdown)
> +		at91_ramc_shutdown();
>  
> +	asm volatile(
>  		/* Reset CPU */
> -		"str	%4, [%1, #" __stringify(AT91_RSTC_CR) "]\n\t"
> +		"str	%1, [%0, #" __stringify(AT91_RSTC_CR) "]\n\t"
>  
>  		"b	.\n\t"

Are you sure this is working properly? There was a reason to have the
SDRAM controller shutdown right before resetting the CPU: the code was
ensuring that all those assembly instructions fitted in one cache line,
so that even if the SDRAM controller gets shutdown, the rest of the
code can properly execute until resetting the CPU. Now, the SDRAM
controller shutdown code and the code resetting the CPU are in
completely separate places, which break this assumption.

And also, you forgot to Cc: Maxime Ripard who did the initial work on
this at91-reset controller.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/