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

["Jorge Ramirez-Ortiz, Gmail" <jorge.ramirez.ortiz@gmail.com>]

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>