Re: [PATCH] arm: mach-snapdragon: pinctrl: Place pin_name in .data section

From: Tom Rini <trini@konsulko.com>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: u-boot@lists.denx.de,
	Jorge Ramirez-Ortiz <jorge.ramirez.ortiz@gmail.com>,
	Nicolas Dechesne <nicolas.dechesne@linaro.org>,
	Ramon Fried <rfried.dev@gmail.com>
Subject: Re: [PATCH] arm: mach-snapdragon: pinctrl: Place pin_name in .data section
Date: Wed, 14 Jul 2021 16:52:31 -0400	[thread overview]
Message-ID: <20210714205231.GW9516@bill-the-cat> (raw)
In-Reply-To: <20210705121847.48432-1-stephan@gerhold.net>

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

On Mon, Jul 05, 2021 at 02:18:47PM +0200, Stephan Gerhold wrote:

> According to arch/arm/lib/crt0_64.S, the BSS section is "UNAVAILABLE"
> and uninitialized before relocation. Also, it overlaps with the
> appended DTB before relocation, so writing data into a variable
> in the BSS section might corrupt the appended DTB.
> 
> Unfortunately, pinctrl-apq8016.c and pinctrl-apq8096.c do place the
> "pin_name" variable in the BSS section (since it's uninitialized).
> It's also used before relocation, when setting up the pinctrl for
> the serial driver.
> 
> On DB410c this causes "GPIO_5" to be written into some part of an
> appended DTB, e.g.:
> 
> 80111820: edfe0dd0 9f100000 38000000 c00e0000    ...........8....
> 80111830: 28000000 11000000 10000000 00000000    ...(............
> 80111840: 4f495047 8800355f 00000000 00000000    GPIO_5..........
> 80111850: 00000000 00000000 01000000 00000000    ................
> 80111860: 03000000 04000000 00000000 02000000    ................
> 80111870: 03000000 04000000 0f000000 02000000    ................
> 80111880: 03000000 2d000000 1b000000 6c617551    .......-....Qual
> 80111890: 6d6d6f63 63655420 6c6f6e68 6569676f    comm Technologie
> 
> Depending on the part of the DTB that is corrupted this might not
> cause any problems, but it can also result in strange reboots
> without any serial output.
> 
> Fortunately, in practice this does not cause issues on DB410c yet
> because board_fdt_blob_setup() in dragonboard410c.c currently
> overrides the appended DTB with the one passed by the previous
> bootloader (LK) (which does not get corrupted).
> 
> DB820c does not have board_fdt_blob_setup() so I would expect it to
> be affected by this problem. Perhaps everyone was just fortunate to
> not compile an U-Boot configuration where the pin_name corrupts an
> important part of the DTB.
> 
> Make sure "pin_name" is explicitly placed in the .data section
> instead of .bss to fix this.
> 
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]