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

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

[Stephan Gerhold]

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

 arch/arm/mach-snapdragon/pinctrl-apq8016.c | 3 +--
 arch/arm/mach-snapdragon/pinctrl-apq8096.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
index 1042b564c3..70c0be0bca 100644
--- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c
+++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
@@ -10,7 +10,7 @@
 #include <common.h>
 
 #define MAX_PIN_NAME_LEN 32
-static char pin_name[MAX_PIN_NAME_LEN];
+static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
 static const char * const msm_pinctrl_pins[] = {
 	"SDC1_CLK",
 	"SDC1_CMD",
@@ -59,4 +59,3 @@ struct msm_pinctrl_data apq8016_data = {
 	.get_function_mux = apq8016_get_function_mux,
 	.get_pin_name = apq8016_get_pin_name,
 };
-
diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8096.c b/arch/arm/mach-snapdragon/pinctrl-apq8096.c
index 20a71c319b..45462f01c2 100644
--- a/arch/arm/mach-snapdragon/pinctrl-apq8096.c
+++ b/arch/arm/mach-snapdragon/pinctrl-apq8096.c
@@ -10,7 +10,7 @@
 #include <common.h>
 
 #define MAX_PIN_NAME_LEN 32
-static char pin_name[MAX_PIN_NAME_LEN];
+static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
 static const char * const msm_pinctrl_pins[] = {
 	"SDC1_CLK",
 	"SDC1_CMD",
-- 
2.32.0

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

[Ramon Fried]

On Mon, Jul 5, 2021 at 3:19 PM Stephan Gerhold <stephan@gerhold.net> 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>
> ---
>
>  arch/arm/mach-snapdragon/pinctrl-apq8016.c | 3 +--
>  arch/arm/mach-snapdragon/pinctrl-apq8096.c | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> index 1042b564c3..70c0be0bca 100644
> --- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> +++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> @@ -10,7 +10,7 @@
>  #include <common.h>
>
>  #define MAX_PIN_NAME_LEN 32
> -static char pin_name[MAX_PIN_NAME_LEN];
> +static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
>  static const char * const msm_pinctrl_pins[] = {
>         "SDC1_CLK",
>         "SDC1_CMD",
> @@ -59,4 +59,3 @@ struct msm_pinctrl_data apq8016_data = {
>         .get_function_mux = apq8016_get_function_mux,
>         .get_pin_name = apq8016_get_pin_name,
>  };
> -
> diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8096.c b/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> index 20a71c319b..45462f01c2 100644
> --- a/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> +++ b/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> @@ -10,7 +10,7 @@
>  #include <common.h>
>
>  #define MAX_PIN_NAME_LEN 32
> -static char pin_name[MAX_PIN_NAME_LEN];
> +static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
>  static const char * const msm_pinctrl_pins[] = {
>         "SDC1_CLK",
>         "SDC1_CMD",
> --
> 2.32.0
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

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

[Tom Rini]

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

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

[Jorge Ramirez-Ortiz, Gmail]

On 06/07/21, Ramon Fried wrote:
> On Mon, Jul 5, 2021 at 3:19 PM Stephan Gerhold <stephan@gerhold.net> 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.

I'll try to confirm during the week.

After Ramon's pinctrl changes db820c did indeed break - was able to
test a few months back- but we didnt have a board to test back then
(and nobody complained)

will come back to you on this but from what you are saying, this is
probably the fix we needed for db820c


> >
> > 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>
> > ---
> >
> >  arch/arm/mach-snapdragon/pinctrl-apq8016.c | 3 +--
> >  arch/arm/mach-snapdragon/pinctrl-apq8096.c | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > index 1042b564c3..70c0be0bca 100644
> > --- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > +++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > @@ -10,7 +10,7 @@
> >  #include <common.h>
> >
> >  #define MAX_PIN_NAME_LEN 32
> > -static char pin_name[MAX_PIN_NAME_LEN];
> > +static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
> >  static const char * const msm_pinctrl_pins[] = {
> >         "SDC1_CLK",
> >         "SDC1_CMD",
> > @@ -59,4 +59,3 @@ struct msm_pinctrl_data apq8016_data = {
> >         .get_function_mux = apq8016_get_function_mux,
> >         .get_pin_name = apq8016_get_pin_name,
> >  };
> > -
> > diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8096.c b/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> > index 20a71c319b..45462f01c2 100644
> > --- a/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> > +++ b/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> > @@ -10,7 +10,7 @@
> >  #include <common.h>
> >
> >  #define MAX_PIN_NAME_LEN 32
> > -static char pin_name[MAX_PIN_NAME_LEN];
> > +static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
> >  static const char * const msm_pinctrl_pins[] = {
> >         "SDC1_CLK",
> >         "SDC1_CMD",
> > --
> > 2.32.0
> >
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

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

[Jorge Ramirez-Ortiz, Gmail]

On 15/07/21, Jorge Ramirez-Ortiz, Gmail wrote:
> On 06/07/21, Ramon Fried wrote:
> > On Mon, Jul 5, 2021 at 3:19 PM Stephan Gerhold <stephan@gerhold.net> 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.
> 
> I'll try to confirm during the week.
> 
> After Ramon's pinctrl changes db820c did indeed break - was able to
> test a few months back- but we didnt have a board to test back then
> (and nobody complained)
> 
> will come back to you on this but from what you are saying, this is
> probably the fix we needed for db820c


yep, all good. just tested it. thanks for the fix!


U-Boot 2021.07-00467-gc11f5abce8 (Jul 15 2021 - 09:20:22 +0200)                                        
Qualcomm-DragonBoard 820C                                                                              
                                                                                                       
DRAM:  3 GiB                                                                                           
PSCI:  v1.0                                                                                            
MMC:   sdhci@74a4900: 0                                                                                
Loading Environment from EXT4... MMC: no card present                                                  
In:    serial@75b0000                                                                                  
Out:   serial@75b0000                                                                                  
Err:   serial@75b0000                                                                                  
Net:   Net Initialization Skipped                                                                      
No ethernet found.                                                                                     
Hit any key to stop autoboot:  0                                                                       
MMC: no card present                                                                                   
dragonboard820c =>
dragonboard820c => bdinfo
boot_params = 0x0000000000000000
DRAM bank   = 0x0000000000000000
-> start    = 0x0000000080000000
-> size     = 0x0000000060000000
DRAM bank   = 0x0000000000000001
-> start    = 0x0000000100000000
-> size     = 0x000000005ea4ffff
flashstart  = 0x0000000000000000
flashsize   = 0x0000000000000000
flashoffset = 0x0000000000000000
baudrate    = 115200 bps
relocaddr   = 0x000000013ff70000
reloc off   = 0x00000000bfef0000
Build       = 64-bit
current eth = unknown
ethaddr     = (not set)
IP addr     = <NULL>
fdt_blob    = 0x000000013f7673c0
new_fdt     = 0x000000013f7673c0
fdt_size    = 0x0000000000000a20
lmb_dump_all:
 memory.cnt  = 0x2
 memory[0]      [0x80000000-0xdfffffff], 0x60000000 bytes flags: 0
 memory[1]      [0x100000000-0x15ea4fffe], 0x5ea4ffff bytes flags: 0
 reserved.cnt  = 0x1
 reserved[0]    [0x86300000-0x864fffff], 0x00200000 bytes flags: 4
arch_number = 0x0000000000000000
TLB addr    = 0x000000013fff0000
irq_sp      = 0x000000013f7673b0
sp start    = 0x000000013f7673b0
Early malloc usage: 5d8 / 2000


> 
> 
> > >
> > > 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>
> > > ---
> > >
> > >  arch/arm/mach-snapdragon/pinctrl-apq8016.c | 3 +--
> > >  arch/arm/mach-snapdragon/pinctrl-apq8096.c | 2 +-
> > >  2 files changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > > index 1042b564c3..70c0be0bca 100644
> > > --- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > > +++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > > @@ -10,7 +10,7 @@
> > >  #include <common.h>
> > >
> > >  #define MAX_PIN_NAME_LEN 32
> > > -static char pin_name[MAX_PIN_NAME_LEN];
> > > +static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
> > >  static const char * const msm_pinctrl_pins[] = {
> > >         "SDC1_CLK",
> > >         "SDC1_CMD",
> > > @@ -59,4 +59,3 @@ struct msm_pinctrl_data apq8016_data = {
> > >         .get_function_mux = apq8016_get_function_mux,
> > >         .get_pin_name = apq8016_get_pin_name,
> > >  };
> > > -
> > > diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8096.c b/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> > > index 20a71c319b..45462f01c2 100644
> > > --- a/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> > > +++ b/arch/arm/mach-snapdragon/pinctrl-apq8096.c
> > > @@ -10,7 +10,7 @@
> > >  #include <common.h>
> > >
> > >  #define MAX_PIN_NAME_LEN 32
> > > -static char pin_name[MAX_PIN_NAME_LEN];
> > > +static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
> > >  static const char * const msm_pinctrl_pins[] = {
> > >         "SDC1_CLK",
> > >         "SDC1_CMD",
> > > --
> > > 2.32.0
> > >
> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com>