[U-Boot] [PATCH v2] SPL: tiny-printf: avoid any BSS usage

[U-Boot] [PATCH v2] SPL: tiny-printf: avoid any BSS usage

[Andre Przywara]

As printf calls may be executed quite early, we should avoid using any
BSS stored variables, since some boards put BSS in DRAM, which may not
have been initialised yet.
Explicitly mark those "static global" variables as belonging to the
.data section, to keep tiny-printf clear of any BSS usage.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Changes v1 .. v2:
- remove NO_BSS macro, use __attribute__ ... directly instead

 lib/tiny-printf.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 451f4f7..b334f05 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -13,11 +13,16 @@
 #include <stdarg.h>
 #include <serial.h>
 
-static char *bf;
-static char zs;
+/*
+ * This code in here may execute before the DRAM is initialised, so
+ * we should make sure that it doesn't touch BSS, which some boards
+ * put in DRAM.
+ */
+static char *bf __attribute__ ((section(".data")));
+static char zs __attribute__ ((section(".data")));
 
 /* Current position in sprintf() output string */
-static char *outstr;
+static char *outstr __attribute__ ((section(".data")));
 
 static void out(char c)
 {
-- 
2.8.2

[U-Boot] [U-Boot,v2] SPL: tiny-printf: avoid any BSS usage

[Tom Rini]

On Fri, Jul 08, 2016 at 03:18:35PM +0100, Andre Przywara wrote:

> As printf calls may be executed quite early, we should avoid using any
> BSS stored variables, since some boards put BSS in DRAM, which may not
> have been initialised yet.
> Explicitly mark those "static global" variables as belonging to the
> .data section, to keep tiny-printf clear of any BSS usage.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160708/6eaa6975/attachment.sig>

[U-Boot] [U-Boot,v2] SPL: tiny-printf: avoid any BSS usage

[Marek Vasut]

On 07/08/2016 06:50 PM, Tom Rini wrote:
> On Fri, Jul 08, 2016 at 03:18:35PM +0100, Andre Przywara wrote:
>
>> As printf calls may be executed quite early, we should avoid using any
>> BSS stored variables, since some boards put BSS in DRAM, which may not
>> have been initialised yet.
>> Explicitly mark those "static global" variables as belonging to the
>> .data section, to keep tiny-printf clear of any BSS usage.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>
> Applied to u-boot/master, thanks!
>
Mind you, __section(".data") would be enough, no need to spell the whole 
attribute.

-- 
Best regards,
Marek Vasut

[U-Boot] [U-Boot,v2] SPL: tiny-printf: avoid any BSS usage

[Tom Rini]

On Fri, Jul 08, 2016 at 11:48:12PM +0200, Marek Vasut wrote:
> On 07/08/2016 06:50 PM, Tom Rini wrote:
> >On Fri, Jul 08, 2016 at 03:18:35PM +0100, Andre Przywara wrote:
> >
> >>As printf calls may be executed quite early, we should avoid using any
> >>BSS stored variables, since some boards put BSS in DRAM, which may not
> >>have been initialised yet.
> >>Explicitly mark those "static global" variables as belonging to the
> >>.data section, to keep tiny-printf clear of any BSS usage.
> >>
> >>Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >
> >Applied to u-boot/master, thanks!
> >
> Mind you, __section(".data") would be enough, no need to spell the
> whole attribute.

True, but that's a helper that's not really utilized today.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160708/1550a9a8/attachment.sig>

[U-Boot] [U-Boot,v2] SPL: tiny-printf: avoid any BSS usage

[Marek Vasut]

On 07/09/2016 02:28 AM, Tom Rini wrote:
> On Fri, Jul 08, 2016 at 11:48:12PM +0200, Marek Vasut wrote:
>> On 07/08/2016 06:50 PM, Tom Rini wrote:
>>> On Fri, Jul 08, 2016 at 03:18:35PM +0100, Andre Przywara wrote:
>>>
>>>> As printf calls may be executed quite early, we should avoid using any
>>>> BSS stored variables, since some boards put BSS in DRAM, which may not
>>>> have been initialised yet.
>>>> Explicitly mark those "static global" variables as belonging to the
>>>> .data section, to keep tiny-printf clear of any BSS usage.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> Applied to u-boot/master, thanks!
>>>
>> Mind you, __section(".data") would be enough, no need to spell the
>> whole attribute.
>
> True, but that's a helper that's not really utilized today.
>
That's something that should likely be changed, as it allows to 
seamlessly deal with compiler quirks (if some were to ever pop up in 
this area).

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] SPL: tiny-printf: avoid any BSS usage

[Simon Glass]

On 8 July 2016 at 08:18, Andre Przywara <andre.przywara@arm.com> wrote:
> As printf calls may be executed quite early, we should avoid using any
> BSS stored variables, since some boards put BSS in DRAM, which may not
> have been initialised yet.
> Explicitly mark those "static global" variables as belonging to the
> .data section, to keep tiny-printf clear of any BSS usage.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Simon Glass <sjg@chromium.org>

Another approach would be to put these vars in a struct, declare it as
a local variable and pass it around. But this works OK too.

> ---
> Changes v1 .. v2:
> - remove NO_BSS macro, use __attribute__ ... directly instead
>
>  lib/tiny-printf.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> index 451f4f7..b334f05 100644
> --- a/lib/tiny-printf.c
> +++ b/lib/tiny-printf.c
> @@ -13,11 +13,16 @@
>  #include <stdarg.h>
>  #include <serial.h>
>
> -static char *bf;
> -static char zs;
> +/*
> + * This code in here may execute before the DRAM is initialised, so
> + * we should make sure that it doesn't touch BSS, which some boards
> + * put in DRAM.
> + */
> +static char *bf __attribute__ ((section(".data")));
> +static char zs __attribute__ ((section(".data")));
>
>  /* Current position in sprintf() output string */
> -static char *outstr;
> +static char *outstr __attribute__ ((section(".data")));
>
>  static void out(char c)
>  {
> --
> 2.8.2
>

[U-Boot] [U-Boot,v2] SPL: tiny-printf: avoid any BSS usage

[Tom Rini]

On Sat, Jul 09, 2016 at 10:22:37AM +0200, Marek Vasut wrote:
> On 07/09/2016 02:28 AM, Tom Rini wrote:
> >On Fri, Jul 08, 2016 at 11:48:12PM +0200, Marek Vasut wrote:
> >>On 07/08/2016 06:50 PM, Tom Rini wrote:
> >>>On Fri, Jul 08, 2016 at 03:18:35PM +0100, Andre Przywara wrote:
> >>>
> >>>>As printf calls may be executed quite early, we should avoid using any
> >>>>BSS stored variables, since some boards put BSS in DRAM, which may not
> >>>>have been initialised yet.
> >>>>Explicitly mark those "static global" variables as belonging to the
> >>>>.data section, to keep tiny-printf clear of any BSS usage.
> >>>>
> >>>>Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >>>
> >>>Applied to u-boot/master, thanks!
> >>>
> >>Mind you, __section(".data") would be enough, no need to spell the
> >>whole attribute.
> >
> >True, but that's a helper that's not really utilized today.
> >
> That's something that should likely be changed, as it allows to
> seamlessly deal with compiler quirks (if some were to ever pop up in
> this area).

Patches welcome for the next release ;)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160709/08357d10/attachment.sig>

[U-Boot] [PATCH v2] SPL: tiny-printf: avoid any BSS usage

[Marek Vasut]

On 07/09/2016 04:38 PM, Simon Glass wrote:
> On 8 July 2016 at 08:18, Andre Przywara <andre.przywara@arm.com> wrote:
>> As printf calls may be executed quite early, we should avoid using any
>> BSS stored variables, since some boards put BSS in DRAM, which may not
>> have been initialised yet.
>> Explicitly mark those "static global" variables as belonging to the
>> .data section, to keep tiny-printf clear of any BSS usage.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Another approach would be to put these vars in a struct, declare it as
> a local variable and pass it around. But this works OK too.

This might in fact be even better.

>> ---
>> Changes v1 .. v2:
>> - remove NO_BSS macro, use __attribute__ ... directly instead
>>
>>  lib/tiny-printf.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
>> index 451f4f7..b334f05 100644
>> --- a/lib/tiny-printf.c
>> +++ b/lib/tiny-printf.c
>> @@ -13,11 +13,16 @@
>>  #include <stdarg.h>
>>  #include <serial.h>
>>
>> -static char *bf;
>> -static char zs;
>> +/*
>> + * This code in here may execute before the DRAM is initialised, so
>> + * we should make sure that it doesn't touch BSS, which some boards
>> + * put in DRAM.
>> + */
>> +static char *bf __attribute__ ((section(".data")));
>> +static char zs __attribute__ ((section(".data")));
>>
>>  /* Current position in sprintf() output string */
>> -static char *outstr;
>> +static char *outstr __attribute__ ((section(".data")));
>>
>>  static void out(char c)
>>  {
>> --
>> 2.8.2
>>


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] SPL: tiny-printf: avoid any BSS usage

[André Przywara]

On 09/07/16 15:38, Simon Glass wrote:
> On 8 July 2016 at 08:18, Andre Przywara <andre.przywara@arm.com> wrote:
>> As printf calls may be executed quite early, we should avoid using any
>> BSS stored variables, since some boards put BSS in DRAM, which may not
>> have been initialised yet.
>> Explicitly mark those "static global" variables as belonging to the
>> .data section, to keep tiny-printf clear of any BSS usage.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Another approach would be to put these vars in a struct, declare it as
> a local variable and pass it around. But this works OK too.

Yeah, I was thinking about this too. Actually fixing the issue that we
need global variables in the first place.
But a quick look revealed that this is not trivial, so I reverted to
this simpler approach for the quick fix. Also I am not sure this will
eventually work against this "tiny" idea, we will see.

To be honest I think the real proper fix(TM) would be to provide a
separate BSS section for any code that runs _before_ DRAM init. This
should be a few bytes only and could easily live in SRAM.
If the loading part of the SPL requires a bigger BSS, that's fine as it
could still live in DRAM.
I might bake a patch when I get bored ...

Cheers,
Andre.


> 
>> ---
>> Changes v1 .. v2:
>> - remove NO_BSS macro, use __attribute__ ... directly instead
>>
>>  lib/tiny-printf.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
>> index 451f4f7..b334f05 100644
>> --- a/lib/tiny-printf.c
>> +++ b/lib/tiny-printf.c
>> @@ -13,11 +13,16 @@
>>  #include <stdarg.h>
>>  #include <serial.h>
>>
>> -static char *bf;
>> -static char zs;
>> +/*
>> + * This code in here may execute before the DRAM is initialised, so
>> + * we should make sure that it doesn't touch BSS, which some boards
>> + * put in DRAM.
>> + */
>> +static char *bf __attribute__ ((section(".data")));
>> +static char zs __attribute__ ((section(".data")));
>>
>>  /* Current position in sprintf() output string */
>> -static char *outstr;
>> +static char *outstr __attribute__ ((section(".data")));
>>
>>  static void out(char c)
>>  {
>> --
>> 2.8.2
>>
> 

[U-Boot] [PATCH v2] SPL: tiny-printf: avoid any BSS usage

[Simon Glass]

Hi,

On 9 July 2016 at 11:27, Andr? Przywara <andre.przywara@arm.com> wrote:
> On 09/07/16 15:38, Simon Glass wrote:
>> On 8 July 2016 at 08:18, Andre Przywara <andre.przywara@arm.com> wrote:
>>> As printf calls may be executed quite early, we should avoid using any
>>> BSS stored variables, since some boards put BSS in DRAM, which may not
>>> have been initialised yet.
>>> Explicitly mark those "static global" variables as belonging to the
>>> .data section, to keep tiny-printf clear of any BSS usage.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Another approach would be to put these vars in a struct, declare it as
>> a local variable and pass it around. But this works OK too.
>
> Yeah, I was thinking about this too. Actually fixing the issue that we
> need global variables in the first place.
> But a quick look revealed that this is not trivial, so I reverted to
> this simpler approach for the quick fix. Also I am not sure this will
> eventually work against this "tiny" idea, we will see.
>
> To be honest I think the real proper fix(TM) would be to provide a
> separate BSS section for any code that runs _before_ DRAM init. This
> should be a few bytes only and could easily live in SRAM.
> If the loading part of the SPL requires a bigger BSS, that's fine as it
> could still live in DRAM.
> I might bake a patch when I get bored ...

For reference I sent a patch to remove use of the data section:

http://patchwork.ozlabs.org/patch/656024/

>
> Cheers,
> Andre.

Regards,
Simon