| Message ID | 20200410185449.152564-1-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | e7882f65f02eca5a7d35871ba0355462bbf7362e |
| Headers | show |
| Series | tiny-printf: Support %i | expand |
Hi Marek, On Fri, 10 Apr 2020 at 12:54, Marek Vasut <marex at denx.de> wrote: > > The most basic printf("%i", value) formating string was missing, > add it for the sake of convenience. > > Signed-off-by: Marek Vasut <marex at denx.de> > Cc: Simon Glass <sjg at chromium.org> > Cc: Stefan Roese <sr at denx.de> > --- > lib/tiny-printf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Can you add to the test in print_ut.c? Regards, Simon
On 4/10/20 10:47 PM, Simon Glass wrote: > Hi Marek, > > On Fri, 10 Apr 2020 at 12:54, Marek Vasut <marex at denx.de> wrote: >> >> The most basic printf("%i", value) formating string was missing, >> add it for the sake of convenience. >> >> Signed-off-by: Marek Vasut <marex at denx.de> >> Cc: Simon Glass <sjg at chromium.org> >> Cc: Stefan Roese <sr at denx.de> >> --- >> lib/tiny-printf.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > Can you add to the test in print_ut.c? Sure, is that a hard-requirement for such a minor patch ? Is there an example for the other %u / %d variants ?
On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > The most basic printf("%i", value) formating string was missing, > add it for the sake of convenience. > > Signed-off-by: Marek Vasut <marex at denx.de> > Cc: Simon Glass <sjg at chromium.org> > Cc: Stefan Roese <sr at denx.de> > --- > lib/tiny-printf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > index 1138c7012a..8fc7e48d99 100644 > --- a/lib/tiny-printf.c > +++ b/lib/tiny-printf.c > @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > goto abort; > case 'u': > case 'd': > + case 'i': > div = 1000000000; > if (islong) { > num = va_arg(va, unsigned long); > @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > num = va_arg(va, unsigned int); > } > > - if (ch == 'd') { > + if (ch != 'u') { > if (islong && (long)num < 0) { > num = -(long)num; > out(info, '-'); How much does the size change and where do we see this as a problem? Thanks!
Hi Marek, On Fri, 10 Apr 2020 at 14:52, Marek Vasut <marex at denx.de> wrote: > > On 4/10/20 10:47 PM, Simon Glass wrote: > > Hi Marek, > > > > On Fri, 10 Apr 2020 at 12:54, Marek Vasut <marex at denx.de> wrote: > >> > >> The most basic printf("%i", value) formating string was missing, > >> add it for the sake of convenience. > >> > >> Signed-off-by: Marek Vasut <marex at denx.de> > >> Cc: Simon Glass <sjg at chromium.org> > >> Cc: Stefan Roese <sr at denx.de> > >> --- > >> lib/tiny-printf.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > > > > Can you add to the test in print_ut.c? > > Sure, is that a hard-requirement for such a minor patch ? > Is there an example for the other %u / %d variants ? I think we should promote adding tests. Just add a few lines and then others can contribute more. The current tests use sprintf(). But there is a ut_check_console_line() that should help. It's really easy to use. See here for an example: https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/blob/corp6-working/test/dm/acpi.c#L262 Regards, Simon
On 4/14/20 1:42 AM, Simon Glass wrote: > Hi Marek, > > On Fri, 10 Apr 2020 at 14:52, Marek Vasut <marex at denx.de> wrote: >> >> On 4/10/20 10:47 PM, Simon Glass wrote: >>> Hi Marek, >>> >>> On Fri, 10 Apr 2020 at 12:54, Marek Vasut <marex at denx.de> wrote: >>>> >>>> The most basic printf("%i", value) formating string was missing, >>>> add it for the sake of convenience. >>>> >>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>> Cc: Simon Glass <sjg at chromium.org> >>>> Cc: Stefan Roese <sr at denx.de> >>>> --- >>>> lib/tiny-printf.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> Can you add to the test in print_ut.c? >> >> Sure, is that a hard-requirement for such a minor patch ? >> Is there an example for the other %u / %d variants ? > > I think we should promote adding tests. > > Just add a few lines and then others can contribute more. > > The current tests use sprintf(). But there is a > ut_check_console_line() that should help. It's really easy to use. > > See here for an example: > > https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/blob/corp6-working/test/dm/acpi.c#L262 These look like ACPI tests, not printf() tests ? Also, please answer my question -- there are currently no tests for the other variants, so you expect me to implement those as well, correct ?
On 4/14/20 1:27 AM, Tom Rini wrote: > On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > >> The most basic printf("%i", value) formating string was missing, >> add it for the sake of convenience. >> >> Signed-off-by: Marek Vasut <marex at denx.de> >> Cc: Simon Glass <sjg at chromium.org> >> Cc: Stefan Roese <sr at denx.de> >> --- >> lib/tiny-printf.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >> index 1138c7012a..8fc7e48d99 100644 >> --- a/lib/tiny-printf.c >> +++ b/lib/tiny-printf.c >> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >> goto abort; >> case 'u': >> case 'd': >> + case 'i': >> div = 1000000000; >> if (islong) { >> num = va_arg(va, unsigned long); >> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >> num = va_arg(va, unsigned int); >> } >> >> - if (ch == 'd') { >> + if (ch != 'u') { >> if (islong && (long)num < 0) { >> num = -(long)num; >> out(info, '-'); > > How much does the size change and where do we see this as a problem? Any code which uses %i in SPL just misbehaves, e.g. printf("%s[%i] value=%x", __func__, __LINE__, val); prints function name and then incorrect value, because %i is ignored. This is also documented in the commit message. U-Boot grows in size massively due to all the DM/DT bloat which is being forced upon everyone, but there the uncontrolled growth is apparently OK even if it brings no obvious improvement, rather the opposite. And yet here, size increase suddenly matters? Sorry, that's not right. The code grows by 6 bytes.
Hi Marek, On Mon, 13 Apr 2020 at 19:18, Marek Vasut <marex at denx.de> wrote: > > On 4/14/20 1:27 AM, Tom Rini wrote: > > On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > > > >> The most basic printf("%i", value) formating string was missing, > >> add it for the sake of convenience. > >> > >> Signed-off-by: Marek Vasut <marex at denx.de> > >> Cc: Simon Glass <sjg at chromium.org> > >> Cc: Stefan Roese <sr at denx.de> > >> --- > >> lib/tiny-printf.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > >> index 1138c7012a..8fc7e48d99 100644 > >> --- a/lib/tiny-printf.c > >> +++ b/lib/tiny-printf.c > >> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >> goto abort; > >> case 'u': > >> case 'd': > >> + case 'i': > >> div = 1000000000; > >> if (islong) { > >> num = va_arg(va, unsigned long); > >> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >> num = va_arg(va, unsigned int); > >> } > >> > >> - if (ch == 'd') { > >> + if (ch != 'u') { > >> if (islong && (long)num < 0) { > >> num = -(long)num; > >> out(info, '-'); > > > > How much does the size change and where do we see this as a problem? > > Any code which uses %i in SPL just misbehaves, e.g. > printf("%s[%i] value=%x", __func__, __LINE__, val); > prints function name and then incorrect value, because %i is ignored. > This is also documented in the commit message. > > U-Boot grows in size massively due to all the DM/DT bloat which is being > forced upon everyone, but there the uncontrolled growth is apparently OK > even if it brings no obvious improvement, rather the opposite. And yet > here, size increase suddenly matters? Sorry, that's not right. > > The code grows by 6 bytes. This is not an issue of code size. It is simply that we have a lot of untested code, and we all need to club together to fix that. Re DM/DT, if you have thoughts on how to improve it, please let me know. I am very concerned about it also. If some of the most senior maintainers in U-Boot are so opposed to adding tests with new code, how do we expect others to make the effort? Marek, you were one of the most vocal advocates of a longer release cycle because you were seeing lots of breakage that maintainers didn't have time to find. How do I square that with the avoidance of adding tests? Regards, Simon
On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: > On 4/14/20 1:27 AM, Tom Rini wrote: > > On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > > > >> The most basic printf("%i", value) formating string was missing, > >> add it for the sake of convenience. > >> > >> Signed-off-by: Marek Vasut <marex at denx.de> > >> Cc: Simon Glass <sjg at chromium.org> > >> Cc: Stefan Roese <sr at denx.de> > >> --- > >> lib/tiny-printf.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > >> index 1138c7012a..8fc7e48d99 100644 > >> --- a/lib/tiny-printf.c > >> +++ b/lib/tiny-printf.c > >> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >> goto abort; > >> case 'u': > >> case 'd': > >> + case 'i': > >> div = 1000000000; > >> if (islong) { > >> num = va_arg(va, unsigned long); > >> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >> num = va_arg(va, unsigned int); > >> } > >> > >> - if (ch == 'd') { > >> + if (ch != 'u') { > >> if (islong && (long)num < 0) { > >> num = -(long)num; > >> out(info, '-'); > > > > How much does the size change and where do we see this as a problem? > > Any code which uses %i in SPL just misbehaves, e.g. > printf("%s[%i] value=%x", __func__, __LINE__, val); > prints function name and then incorrect value, because %i is ignored. > This is also documented in the commit message. > > U-Boot grows in size massively due to all the DM/DT bloat which is being > forced upon everyone, but there the uncontrolled growth is apparently OK > even if it brings no obvious improvement, rather the opposite. And yet > here, size increase suddenly matters? Sorry, that's not right. > > The code grows by 6 bytes. Yes, it matters for _tiny-printf_ as that's where we have little to no room for growth. So it's just debug prints you were doing that ran in to a problem? Thanks!
On 4/14/20 5:03 AM, Tom Rini wrote: > On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: >> On 4/14/20 1:27 AM, Tom Rini wrote: >>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: >>> >>>> The most basic printf("%i", value) formating string was missing, >>>> add it for the sake of convenience. >>>> >>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>> Cc: Simon Glass <sjg at chromium.org> >>>> Cc: Stefan Roese <sr at denx.de> >>>> --- >>>> lib/tiny-printf.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >>>> index 1138c7012a..8fc7e48d99 100644 >>>> --- a/lib/tiny-printf.c >>>> +++ b/lib/tiny-printf.c >>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>> goto abort; >>>> case 'u': >>>> case 'd': >>>> + case 'i': >>>> div = 1000000000; >>>> if (islong) { >>>> num = va_arg(va, unsigned long); >>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>> num = va_arg(va, unsigned int); >>>> } >>>> >>>> - if (ch == 'd') { >>>> + if (ch != 'u') { >>>> if (islong && (long)num < 0) { >>>> num = -(long)num; >>>> out(info, '-'); >>> >>> How much does the size change and where do we see this as a problem? >> >> Any code which uses %i in SPL just misbehaves, e.g. >> printf("%s[%i] value=%x", __func__, __LINE__, val); >> prints function name and then incorrect value, because %i is ignored. >> This is also documented in the commit message. >> >> U-Boot grows in size massively due to all the DM/DT bloat which is being >> forced upon everyone, but there the uncontrolled growth is apparently OK >> even if it brings no obvious improvement, rather the opposite. And yet >> here, size increase suddenly matters? Sorry, that's not right. >> >> The code grows by 6 bytes. > > Yes, it matters for _tiny-printf_ as that's where we have little to no > room for growth. How many systems that use tiny-printf in SPL are also forced to use DM in SPL ? > So it's just debug prints you were doing that ran in > to a problem? Thanks! git grep %i indicates ~400 sites where %i is used, so no, not just debug prints. All of those are broken. And no, I'm not inclined to patch all the code to use %d instead of %i just because handling %i is a problem.
On 4/14/20 4:18 AM, Simon Glass wrote: > Hi Marek, > > On Mon, 13 Apr 2020 at 19:18, Marek Vasut <marex at denx.de> wrote: >> >> On 4/14/20 1:27 AM, Tom Rini wrote: >>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: >>> >>>> The most basic printf("%i", value) formating string was missing, >>>> add it for the sake of convenience. >>>> >>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>> Cc: Simon Glass <sjg at chromium.org> >>>> Cc: Stefan Roese <sr at denx.de> >>>> --- >>>> lib/tiny-printf.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >>>> index 1138c7012a..8fc7e48d99 100644 >>>> --- a/lib/tiny-printf.c >>>> +++ b/lib/tiny-printf.c >>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>> goto abort; >>>> case 'u': >>>> case 'd': >>>> + case 'i': >>>> div = 1000000000; >>>> if (islong) { >>>> num = va_arg(va, unsigned long); >>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>> num = va_arg(va, unsigned int); >>>> } >>>> >>>> - if (ch == 'd') { >>>> + if (ch != 'u') { >>>> if (islong && (long)num < 0) { >>>> num = -(long)num; >>>> out(info, '-'); >>> >>> How much does the size change and where do we see this as a problem? >> >> Any code which uses %i in SPL just misbehaves, e.g. >> printf("%s[%i] value=%x", __func__, __LINE__, val); >> prints function name and then incorrect value, because %i is ignored. >> This is also documented in the commit message. >> >> U-Boot grows in size massively due to all the DM/DT bloat which is being >> forced upon everyone, but there the uncontrolled growth is apparently OK >> even if it brings no obvious improvement, rather the opposite. And yet >> here, size increase suddenly matters? Sorry, that's not right. >> >> The code grows by 6 bytes. > > This is not an issue of code size. It is simply that we have a lot of > untested code, and we all need to club together to fix that. It seems to me that Toms counter-argument is code size. > Re DM/DT, if you have thoughts on how to improve it, please let me > know. I am very concerned about it also. I think I mentioned it before, what about bypassing uclasses which have only one driver instance in them . Then the whole code for tracking instances can be optimized away for that particular uclass, which should save quite a bit of space. > If some of the most senior maintainers in U-Boot are so opposed to > adding tests with new code, how do we expect others to make the > effort? Marek, you were one of the most vocal advocates of a longer > release cycle because you were seeing lots of breakage that > maintainers didn't have time to find. How do I square that with the > avoidance of adding tests? You are wrong, I am not opposed to adding tests. I am opposed to blocking trivial patches which take minutes to implement by requesting adding an entire class of tests, which would likely take days to implement properly. That sheer difference in magnitude does not sound right to me.
On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: > On 4/14/20 5:03 AM, Tom Rini wrote: > > On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: > >> On 4/14/20 1:27 AM, Tom Rini wrote: > >>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > >>> > >>>> The most basic printf("%i", value) formating string was missing, > >>>> add it for the sake of convenience. > >>>> > >>>> Signed-off-by: Marek Vasut <marex at denx.de> > >>>> Cc: Simon Glass <sjg at chromium.org> > >>>> Cc: Stefan Roese <sr at denx.de> > >>>> --- > >>>> lib/tiny-printf.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > >>>> index 1138c7012a..8fc7e48d99 100644 > >>>> --- a/lib/tiny-printf.c > >>>> +++ b/lib/tiny-printf.c > >>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>> goto abort; > >>>> case 'u': > >>>> case 'd': > >>>> + case 'i': > >>>> div = 1000000000; > >>>> if (islong) { > >>>> num = va_arg(va, unsigned long); > >>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>> num = va_arg(va, unsigned int); > >>>> } > >>>> > >>>> - if (ch == 'd') { > >>>> + if (ch != 'u') { > >>>> if (islong && (long)num < 0) { > >>>> num = -(long)num; > >>>> out(info, '-'); > >>> > >>> How much does the size change and where do we see this as a problem? > >> > >> Any code which uses %i in SPL just misbehaves, e.g. > >> printf("%s[%i] value=%x", __func__, __LINE__, val); > >> prints function name and then incorrect value, because %i is ignored. > >> This is also documented in the commit message. > >> > >> U-Boot grows in size massively due to all the DM/DT bloat which is being > >> forced upon everyone, but there the uncontrolled growth is apparently OK > >> even if it brings no obvious improvement, rather the opposite. And yet > >> here, size increase suddenly matters? Sorry, that's not right. > >> > >> The code grows by 6 bytes. > > > > Yes, it matters for _tiny-printf_ as that's where we have little to no > > room for growth. > > How many systems that use tiny-printf in SPL are also forced to use DM > in SPL ? I don't know how many times I've said no one is forced to switch to DM in SPL. > > So it's just debug prints you were doing that ran in > > to a problem? Thanks! > > git grep %i indicates ~400 sites where %i is used, so no, not just debug > prints. All of those are broken. And no, I'm not inclined to patch all > the code to use %d instead of %i just because handling %i is a problem. Not all 400 of them but the ones that are expected to be used in SPL and with TINY_PRINTF need to, yes. Go look at the git log of tiny-printf.c, we've changed things around to avoid growth when at all possible. Because yes, I don't want to grow a few hundred boards by 6 bytes when we have a reasonable alternative. There's 300 hits, of which a dozen are non-debug and likely to ever be in SPL code. And no, this isn't the first time I've raised such an issue, it's just the first time you've been hit by this, sorry.
On 4/14/20 3:26 PM, Tom Rini wrote: > On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: >> On 4/14/20 5:03 AM, Tom Rini wrote: >>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: >>>> On 4/14/20 1:27 AM, Tom Rini wrote: >>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: >>>>> >>>>>> The most basic printf("%i", value) formating string was missing, >>>>>> add it for the sake of convenience. >>>>>> >>>>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>>>> Cc: Simon Glass <sjg at chromium.org> >>>>>> Cc: Stefan Roese <sr at denx.de> >>>>>> --- >>>>>> lib/tiny-printf.c | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >>>>>> index 1138c7012a..8fc7e48d99 100644 >>>>>> --- a/lib/tiny-printf.c >>>>>> +++ b/lib/tiny-printf.c >>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>> goto abort; >>>>>> case 'u': >>>>>> case 'd': >>>>>> + case 'i': >>>>>> div = 1000000000; >>>>>> if (islong) { >>>>>> num = va_arg(va, unsigned long); >>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>> num = va_arg(va, unsigned int); >>>>>> } >>>>>> >>>>>> - if (ch == 'd') { >>>>>> + if (ch != 'u') { >>>>>> if (islong && (long)num < 0) { >>>>>> num = -(long)num; >>>>>> out(info, '-'); >>>>> >>>>> How much does the size change and where do we see this as a problem? >>>> >>>> Any code which uses %i in SPL just misbehaves, e.g. >>>> printf("%s[%i] value=%x", __func__, __LINE__, val); >>>> prints function name and then incorrect value, because %i is ignored. >>>> This is also documented in the commit message. >>>> >>>> U-Boot grows in size massively due to all the DM/DT bloat which is being >>>> forced upon everyone, but there the uncontrolled growth is apparently OK >>>> even if it brings no obvious improvement, rather the opposite. And yet >>>> here, size increase suddenly matters? Sorry, that's not right. >>>> >>>> The code grows by 6 bytes. >>> >>> Yes, it matters for _tiny-printf_ as that's where we have little to no >>> room for growth. >> >> How many systems that use tiny-printf in SPL are also forced to use DM >> in SPL ? > > I don't know how many times I've said no one is forced to switch to DM > in SPL. This is beside the point, there are boards which use SPL and DM, because the non-DM drivers are steadily going away. So the growth in SPL size is there, either directly or as a side-effect. >>> So it's just debug prints you were doing that ran in >>> to a problem? Thanks! >> >> git grep %i indicates ~400 sites where %i is used, so no, not just debug >> prints. All of those are broken. And no, I'm not inclined to patch all >> the code to use %d instead of %i just because handling %i is a problem. > > Not all 400 of them but the ones that are expected to be used in SPL and > with TINY_PRINTF need to, yes. Go look at the git log of tiny-printf.c, > we've changed things around to avoid growth when at all possible. I appreciate that. However, I would also appreciate if printf() behaved in a sane manner, and missing %i support is really weird. > Because yes, I don't want to grow a few hundred boards by 6 bytes when > we have a reasonable alternative. There's 300 hits, of which a dozen > are non-debug and likely to ever be in SPL code. Why don't we instead replace %d with %i altogether then ? The %d seems to be seldom used outside of U-Boot, where it is only used because of this tiny-printf limitation, while %i is used quite often. > And no, this isn't the > first time I've raised such an issue, it's just the first time you've > been hit by this, sorry. Does this therefore set a precedent that we are allowed to block any and all patches which grow SPL size, no matter how useful they might be ?
On Tue, Apr 14, 2020 at 03:40:14PM +0200, Marek Vasut wrote: > On 4/14/20 3:26 PM, Tom Rini wrote: > > On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: > >> On 4/14/20 5:03 AM, Tom Rini wrote: > >>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: > >>>> On 4/14/20 1:27 AM, Tom Rini wrote: > >>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > >>>>> > >>>>>> The most basic printf("%i", value) formating string was missing, > >>>>>> add it for the sake of convenience. > >>>>>> > >>>>>> Signed-off-by: Marek Vasut <marex at denx.de> > >>>>>> Cc: Simon Glass <sjg at chromium.org> > >>>>>> Cc: Stefan Roese <sr at denx.de> > >>>>>> --- > >>>>>> lib/tiny-printf.c | 3 ++- > >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > >>>>>> index 1138c7012a..8fc7e48d99 100644 > >>>>>> --- a/lib/tiny-printf.c > >>>>>> +++ b/lib/tiny-printf.c > >>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>>>> goto abort; > >>>>>> case 'u': > >>>>>> case 'd': > >>>>>> + case 'i': > >>>>>> div = 1000000000; > >>>>>> if (islong) { > >>>>>> num = va_arg(va, unsigned long); > >>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>>>> num = va_arg(va, unsigned int); > >>>>>> } > >>>>>> > >>>>>> - if (ch == 'd') { > >>>>>> + if (ch != 'u') { > >>>>>> if (islong && (long)num < 0) { > >>>>>> num = -(long)num; > >>>>>> out(info, '-'); > >>>>> > >>>>> How much does the size change and where do we see this as a problem? > >>>> > >>>> Any code which uses %i in SPL just misbehaves, e.g. > >>>> printf("%s[%i] value=%x", __func__, __LINE__, val); > >>>> prints function name and then incorrect value, because %i is ignored. > >>>> This is also documented in the commit message. > >>>> > >>>> U-Boot grows in size massively due to all the DM/DT bloat which is being > >>>> forced upon everyone, but there the uncontrolled growth is apparently OK > >>>> even if it brings no obvious improvement, rather the opposite. And yet > >>>> here, size increase suddenly matters? Sorry, that's not right. > >>>> > >>>> The code grows by 6 bytes. > >>> > >>> Yes, it matters for _tiny-printf_ as that's where we have little to no > >>> room for growth. > >> > >> How many systems that use tiny-printf in SPL are also forced to use DM > >> in SPL ? > > > > I don't know how many times I've said no one is forced to switch to DM > > in SPL. > > This is beside the point, there are boards which use SPL and DM, because > the non-DM drivers are steadily going away. So the growth in SPL size is > there, either directly or as a side-effect. > > >>> So it's just debug prints you were doing that ran in > >>> to a problem? Thanks! > >> > >> git grep %i indicates ~400 sites where %i is used, so no, not just debug > >> prints. All of those are broken. And no, I'm not inclined to patch all > >> the code to use %d instead of %i just because handling %i is a problem. > > > > Not all 400 of them but the ones that are expected to be used in SPL and > > with TINY_PRINTF need to, yes. Go look at the git log of tiny-printf.c, > > we've changed things around to avoid growth when at all possible. > > I appreciate that. However, I would also appreciate if printf() behaved > in a sane manner, and missing %i support is really weird. > > > Because yes, I don't want to grow a few hundred boards by 6 bytes when > > we have a reasonable alternative. There's 300 hits, of which a dozen > > are non-debug and likely to ever be in SPL code. > > Why don't we instead replace %d with %i altogether then ? The %d seems > to be seldom used outside of U-Boot, where it is only used because of > this tiny-printf limitation, while %i is used quite often. > > > And no, this isn't the > > first time I've raised such an issue, it's just the first time you've > > been hit by this, sorry. > > Does this therefore set a precedent that we are allowed to block any and > all patches which grow SPL size, no matter how useful they might be ? This is following the precedent that was set for tiny printf a while ago with some other "it would be nice if..." format that could instead be handled differently, again for the case of tiny printf. It is not supposed to cover everything, or most things. It is supposed to let SPL/TPL still have printf in otherwise very tight situations. And as a reminder, I throw every PR through a before/after size check and flag growth that's global and not fixing a bug that can't be fixed some other way. Change your prints to %d and fix the problem without a size change.
Hi Marek, On Tue, 14 Apr 2020 at 07:41, Marek Vasut <marex at denx.de> wrote: > > On 4/14/20 3:26 PM, Tom Rini wrote: > > On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: > >> On 4/14/20 5:03 AM, Tom Rini wrote: > >>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: > >>>> On 4/14/20 1:27 AM, Tom Rini wrote: > >>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > >>>>> > >>>>>> The most basic printf("%i", value) formating string was missing, > >>>>>> add it for the sake of convenience. > >>>>>> > >>>>>> Signed-off-by: Marek Vasut <marex at denx.de> > >>>>>> Cc: Simon Glass <sjg at chromium.org> > >>>>>> Cc: Stefan Roese <sr at denx.de> > >>>>>> --- > >>>>>> lib/tiny-printf.c | 3 ++- > >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > >>>>>> index 1138c7012a..8fc7e48d99 100644 > >>>>>> --- a/lib/tiny-printf.c > >>>>>> +++ b/lib/tiny-printf.c > >>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>>>> goto abort; > >>>>>> case 'u': > >>>>>> case 'd': > >>>>>> + case 'i': > >>>>>> div = 1000000000; > >>>>>> if (islong) { > >>>>>> num = va_arg(va, unsigned long); > >>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>>>> num = va_arg(va, unsigned int); > >>>>>> } > >>>>>> > >>>>>> - if (ch == 'd') { > >>>>>> + if (ch != 'u') { > >>>>>> if (islong && (long)num < 0) { > >>>>>> num = -(long)num; > >>>>>> out(info, '-'); > >>>>> > >>>>> How much does the size change and where do we see this as a problem? > >>>> > >>>> Any code which uses %i in SPL just misbehaves, e.g. > >>>> printf("%s[%i] value=%x", __func__, __LINE__, val); > >>>> prints function name and then incorrect value, because %i is ignored. > >>>> This is also documented in the commit message. > >>>> > >>>> U-Boot grows in size massively due to all the DM/DT bloat which is being > >>>> forced upon everyone, but there the uncontrolled growth is apparently OK > >>>> even if it brings no obvious improvement, rather the opposite. And yet > >>>> here, size increase suddenly matters? Sorry, that's not right. > >>>> > >>>> The code grows by 6 bytes. > >>> > >>> Yes, it matters for _tiny-printf_ as that's where we have little to no > >>> room for growth. > >> > >> How many systems that use tiny-printf in SPL are also forced to use DM > >> in SPL ? > > > > I don't know how many times I've said no one is forced to switch to DM > > in SPL. > > This is beside the point, there are boards which use SPL and DM, because > the non-DM drivers are steadily going away. So the growth in SPL size is > there, either directly or as a side-effect. I think this is a good point. For serial we have a debug UART. I am think it would be useful to have a 'simple bypass' for more subsystems. For example, for I2C we could have a simple option to access a single I2C driver directly, bypassing driver model. Of course this is painful to do before we have completed I2C migration. [..] Regards, Simon
On 4/14/20 4:11 PM, Tom Rini wrote: > On Tue, Apr 14, 2020 at 03:40:14PM +0200, Marek Vasut wrote: >> On 4/14/20 3:26 PM, Tom Rini wrote: >>> On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: >>>> On 4/14/20 5:03 AM, Tom Rini wrote: >>>>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: >>>>>> On 4/14/20 1:27 AM, Tom Rini wrote: >>>>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: >>>>>>> >>>>>>>> The most basic printf("%i", value) formating string was missing, >>>>>>>> add it for the sake of convenience. >>>>>>>> >>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>>>>>> Cc: Simon Glass <sjg at chromium.org> >>>>>>>> Cc: Stefan Roese <sr at denx.de> >>>>>>>> --- >>>>>>>> lib/tiny-printf.c | 3 ++- >>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >>>>>>>> index 1138c7012a..8fc7e48d99 100644 >>>>>>>> --- a/lib/tiny-printf.c >>>>>>>> +++ b/lib/tiny-printf.c >>>>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>>>> goto abort; >>>>>>>> case 'u': >>>>>>>> case 'd': >>>>>>>> + case 'i': >>>>>>>> div = 1000000000; >>>>>>>> if (islong) { >>>>>>>> num = va_arg(va, unsigned long); >>>>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>>>> num = va_arg(va, unsigned int); >>>>>>>> } >>>>>>>> >>>>>>>> - if (ch == 'd') { >>>>>>>> + if (ch != 'u') { >>>>>>>> if (islong && (long)num < 0) { >>>>>>>> num = -(long)num; >>>>>>>> out(info, '-'); >>>>>>> >>>>>>> How much does the size change and where do we see this as a problem? >>>>>> >>>>>> Any code which uses %i in SPL just misbehaves, e.g. >>>>>> printf("%s[%i] value=%x", __func__, __LINE__, val); >>>>>> prints function name and then incorrect value, because %i is ignored. >>>>>> This is also documented in the commit message. >>>>>> >>>>>> U-Boot grows in size massively due to all the DM/DT bloat which is being >>>>>> forced upon everyone, but there the uncontrolled growth is apparently OK >>>>>> even if it brings no obvious improvement, rather the opposite. And yet >>>>>> here, size increase suddenly matters? Sorry, that's not right. >>>>>> >>>>>> The code grows by 6 bytes. >>>>> >>>>> Yes, it matters for _tiny-printf_ as that's where we have little to no >>>>> room for growth. >>>> >>>> How many systems that use tiny-printf in SPL are also forced to use DM >>>> in SPL ? >>> >>> I don't know how many times I've said no one is forced to switch to DM >>> in SPL. >> >> This is beside the point, there are boards which use SPL and DM, because >> the non-DM drivers are steadily going away. So the growth in SPL size is >> there, either directly or as a side-effect. >> >>>>> So it's just debug prints you were doing that ran in >>>>> to a problem? Thanks! >>>> >>>> git grep %i indicates ~400 sites where %i is used, so no, not just debug >>>> prints. All of those are broken. And no, I'm not inclined to patch all >>>> the code to use %d instead of %i just because handling %i is a problem. >>> >>> Not all 400 of them but the ones that are expected to be used in SPL and >>> with TINY_PRINTF need to, yes. Go look at the git log of tiny-printf.c, >>> we've changed things around to avoid growth when at all possible. >> >> I appreciate that. However, I would also appreciate if printf() behaved >> in a sane manner, and missing %i support is really weird. >> >>> Because yes, I don't want to grow a few hundred boards by 6 bytes when >>> we have a reasonable alternative. There's 300 hits, of which a dozen >>> are non-debug and likely to ever be in SPL code. >> >> Why don't we instead replace %d with %i altogether then ? The %d seems >> to be seldom used outside of U-Boot, where it is only used because of >> this tiny-printf limitation, while %i is used quite often. >> >>> And no, this isn't the >>> first time I've raised such an issue, it's just the first time you've >>> been hit by this, sorry. >> >> Does this therefore set a precedent that we are allowed to block any and >> all patches which grow SPL size, no matter how useful they might be ? > > This is following the precedent that was set for tiny printf a while ago > with some other "it would be nice if..." format that could instead be > handled differently, again for the case of tiny printf. It is not > supposed to cover everything, or most things. It is supposed to let > SPL/TPL still have printf in otherwise very tight situations. Except the way it is right now, a lot of output is broken in SPL in an inobvious way, which makes working with and/or debugging SPL difficult and special, compared to U-Boot and other software. > And as a reminder, I throw every PR through a before/after size check > and flag growth that's global and not fixing a bug that can't be fixed > some other way. Change your prints to %d and fix the problem without a > size change. Sorry, no, "change your formatting strings to cater for our broken printf() implementation" really is not the solution here.
On 4/14/20 4:41 PM, Simon Glass wrote: > Hi Marek, > > On Tue, 14 Apr 2020 at 07:41, Marek Vasut <marex at denx.de> wrote: >> >> On 4/14/20 3:26 PM, Tom Rini wrote: >>> On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: >>>> On 4/14/20 5:03 AM, Tom Rini wrote: >>>>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: >>>>>> On 4/14/20 1:27 AM, Tom Rini wrote: >>>>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: >>>>>>> >>>>>>>> The most basic printf("%i", value) formating string was missing, >>>>>>>> add it for the sake of convenience. >>>>>>>> >>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>>>>>> Cc: Simon Glass <sjg at chromium.org> >>>>>>>> Cc: Stefan Roese <sr at denx.de> >>>>>>>> --- >>>>>>>> lib/tiny-printf.c | 3 ++- >>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >>>>>>>> index 1138c7012a..8fc7e48d99 100644 >>>>>>>> --- a/lib/tiny-printf.c >>>>>>>> +++ b/lib/tiny-printf.c >>>>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>>>> goto abort; >>>>>>>> case 'u': >>>>>>>> case 'd': >>>>>>>> + case 'i': >>>>>>>> div = 1000000000; >>>>>>>> if (islong) { >>>>>>>> num = va_arg(va, unsigned long); >>>>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>>>> num = va_arg(va, unsigned int); >>>>>>>> } >>>>>>>> >>>>>>>> - if (ch == 'd') { >>>>>>>> + if (ch != 'u') { >>>>>>>> if (islong && (long)num < 0) { >>>>>>>> num = -(long)num; >>>>>>>> out(info, '-'); >>>>>>> >>>>>>> How much does the size change and where do we see this as a problem? >>>>>> >>>>>> Any code which uses %i in SPL just misbehaves, e.g. >>>>>> printf("%s[%i] value=%x", __func__, __LINE__, val); >>>>>> prints function name and then incorrect value, because %i is ignored. >>>>>> This is also documented in the commit message. >>>>>> >>>>>> U-Boot grows in size massively due to all the DM/DT bloat which is being >>>>>> forced upon everyone, but there the uncontrolled growth is apparently OK >>>>>> even if it brings no obvious improvement, rather the opposite. And yet >>>>>> here, size increase suddenly matters? Sorry, that's not right. >>>>>> >>>>>> The code grows by 6 bytes. >>>>> >>>>> Yes, it matters for _tiny-printf_ as that's where we have little to no >>>>> room for growth. >>>> >>>> How many systems that use tiny-printf in SPL are also forced to use DM >>>> in SPL ? >>> >>> I don't know how many times I've said no one is forced to switch to DM >>> in SPL. >> >> This is beside the point, there are boards which use SPL and DM, because >> the non-DM drivers are steadily going away. So the growth in SPL size is >> there, either directly or as a side-effect. > > I think this is a good point. For serial we have a debug UART. I am > think it would be useful to have a 'simple bypass' for more > subsystems. For example, for I2C we could have a simple option to > access a single I2C driver directly, bypassing driver model. Of course > this is painful to do before we have completed I2C migration. Right. Although it could be prototyped e.g. on the UART subssystem, which is a good candidate.
On Tue, Apr 14, 2020 at 05:22:19PM +0200, Marek Vasut wrote: > On 4/14/20 4:11 PM, Tom Rini wrote: > > On Tue, Apr 14, 2020 at 03:40:14PM +0200, Marek Vasut wrote: > >> On 4/14/20 3:26 PM, Tom Rini wrote: > >>> On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: > >>>> On 4/14/20 5:03 AM, Tom Rini wrote: > >>>>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: > >>>>>> On 4/14/20 1:27 AM, Tom Rini wrote: > >>>>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > >>>>>>> > >>>>>>>> The most basic printf("%i", value) formating string was missing, > >>>>>>>> add it for the sake of convenience. > >>>>>>>> > >>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de> > >>>>>>>> Cc: Simon Glass <sjg at chromium.org> > >>>>>>>> Cc: Stefan Roese <sr at denx.de> > >>>>>>>> --- > >>>>>>>> lib/tiny-printf.c | 3 ++- > >>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>>>> > >>>>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > >>>>>>>> index 1138c7012a..8fc7e48d99 100644 > >>>>>>>> --- a/lib/tiny-printf.c > >>>>>>>> +++ b/lib/tiny-printf.c > >>>>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>>>>>> goto abort; > >>>>>>>> case 'u': > >>>>>>>> case 'd': > >>>>>>>> + case 'i': > >>>>>>>> div = 1000000000; > >>>>>>>> if (islong) { > >>>>>>>> num = va_arg(va, unsigned long); > >>>>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>>>>>> num = va_arg(va, unsigned int); > >>>>>>>> } > >>>>>>>> > >>>>>>>> - if (ch == 'd') { > >>>>>>>> + if (ch != 'u') { > >>>>>>>> if (islong && (long)num < 0) { > >>>>>>>> num = -(long)num; > >>>>>>>> out(info, '-'); > >>>>>>> > >>>>>>> How much does the size change and where do we see this as a problem? > >>>>>> > >>>>>> Any code which uses %i in SPL just misbehaves, e.g. > >>>>>> printf("%s[%i] value=%x", __func__, __LINE__, val); > >>>>>> prints function name and then incorrect value, because %i is ignored. > >>>>>> This is also documented in the commit message. > >>>>>> > >>>>>> U-Boot grows in size massively due to all the DM/DT bloat which is being > >>>>>> forced upon everyone, but there the uncontrolled growth is apparently OK > >>>>>> even if it brings no obvious improvement, rather the opposite. And yet > >>>>>> here, size increase suddenly matters? Sorry, that's not right. > >>>>>> > >>>>>> The code grows by 6 bytes. > >>>>> > >>>>> Yes, it matters for _tiny-printf_ as that's where we have little to no > >>>>> room for growth. > >>>> > >>>> How many systems that use tiny-printf in SPL are also forced to use DM > >>>> in SPL ? > >>> > >>> I don't know how many times I've said no one is forced to switch to DM > >>> in SPL. > >> > >> This is beside the point, there are boards which use SPL and DM, because > >> the non-DM drivers are steadily going away. So the growth in SPL size is > >> there, either directly or as a side-effect. > >> > >>>>> So it's just debug prints you were doing that ran in > >>>>> to a problem? Thanks! > >>>> > >>>> git grep %i indicates ~400 sites where %i is used, so no, not just debug > >>>> prints. All of those are broken. And no, I'm not inclined to patch all > >>>> the code to use %d instead of %i just because handling %i is a problem. > >>> > >>> Not all 400 of them but the ones that are expected to be used in SPL and > >>> with TINY_PRINTF need to, yes. Go look at the git log of tiny-printf.c, > >>> we've changed things around to avoid growth when at all possible. > >> > >> I appreciate that. However, I would also appreciate if printf() behaved > >> in a sane manner, and missing %i support is really weird. > >> > >>> Because yes, I don't want to grow a few hundred boards by 6 bytes when > >>> we have a reasonable alternative. There's 300 hits, of which a dozen > >>> are non-debug and likely to ever be in SPL code. > >> > >> Why don't we instead replace %d with %i altogether then ? The %d seems > >> to be seldom used outside of U-Boot, where it is only used because of > >> this tiny-printf limitation, while %i is used quite often. > >> > >>> And no, this isn't the > >>> first time I've raised such an issue, it's just the first time you've > >>> been hit by this, sorry. > >> > >> Does this therefore set a precedent that we are allowed to block any and > >> all patches which grow SPL size, no matter how useful they might be ? > > > > This is following the precedent that was set for tiny printf a while ago > > with some other "it would be nice if..." format that could instead be > > handled differently, again for the case of tiny printf. It is not > > supposed to cover everything, or most things. It is supposed to let > > SPL/TPL still have printf in otherwise very tight situations. > > Except the way it is right now, a lot of output is broken in SPL in an > inobvious way, which makes working with and/or debugging SPL difficult > and special, compared to U-Boot and other software. No, it's not "a lot", it's whatever you ran in to. We don't have a lot of "%i" usage to start with and even less that's not a debug print and also in SPL code. > > And as a reminder, I throw every PR through a before/after size check > > and flag growth that's global and not fixing a bug that can't be fixed > > some other way. Change your prints to %d and fix the problem without a > > size change. > > Sorry, no, "change your formatting strings to cater for our broken > printf() implementation" really is not the solution here. Except it is, for the specifically limited printf limitation. Adapt the print for the limitation and move on. It's not even "rework the code slightly so the print is correct", it's switch to %d.
Hi Marek, On Tue, 14 Apr 2020 at 10:32, Marek Vasut <marex at denx.de> wrote: > > On 4/14/20 4:41 PM, Simon Glass wrote: > > Hi Marek, > > > > On Tue, 14 Apr 2020 at 07:41, Marek Vasut <marex at denx.de> wrote: > >> > >> On 4/14/20 3:26 PM, Tom Rini wrote: > >>> On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: > >>>> On 4/14/20 5:03 AM, Tom Rini wrote: > >>>>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: > >>>>>> On 4/14/20 1:27 AM, Tom Rini wrote: > >>>>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > >>>>>>> > >>>>>>>> The most basic printf("%i", value) formating string was missing, > >>>>>>>> add it for the sake of convenience. > >>>>>>>> > >>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de> > >>>>>>>> Cc: Simon Glass <sjg at chromium.org> > >>>>>>>> Cc: Stefan Roese <sr at denx.de> > >>>>>>>> --- > >>>>>>>> lib/tiny-printf.c | 3 ++- > >>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>>>> > >>>>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > >>>>>>>> index 1138c7012a..8fc7e48d99 100644 > >>>>>>>> --- a/lib/tiny-printf.c > >>>>>>>> +++ b/lib/tiny-printf.c > >>>>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>>>>>> goto abort; > >>>>>>>> case 'u': > >>>>>>>> case 'd': > >>>>>>>> + case 'i': > >>>>>>>> div = 1000000000; > >>>>>>>> if (islong) { > >>>>>>>> num = va_arg(va, unsigned long); > >>>>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) > >>>>>>>> num = va_arg(va, unsigned int); > >>>>>>>> } > >>>>>>>> > >>>>>>>> - if (ch == 'd') { > >>>>>>>> + if (ch != 'u') { > >>>>>>>> if (islong && (long)num < 0) { > >>>>>>>> num = -(long)num; > >>>>>>>> out(info, '-'); > >>>>>>> > >>>>>>> How much does the size change and where do we see this as a problem? > >>>>>> > >>>>>> Any code which uses %i in SPL just misbehaves, e.g. > >>>>>> printf("%s[%i] value=%x", __func__, __LINE__, val); > >>>>>> prints function name and then incorrect value, because %i is ignored. > >>>>>> This is also documented in the commit message. > >>>>>> > >>>>>> U-Boot grows in size massively due to all the DM/DT bloat which is being > >>>>>> forced upon everyone, but there the uncontrolled growth is apparently OK > >>>>>> even if it brings no obvious improvement, rather the opposite. And yet > >>>>>> here, size increase suddenly matters? Sorry, that's not right. > >>>>>> > >>>>>> The code grows by 6 bytes. > >>>>> > >>>>> Yes, it matters for _tiny-printf_ as that's where we have little to no > >>>>> room for growth. > >>>> > >>>> How many systems that use tiny-printf in SPL are also forced to use DM > >>>> in SPL ? > >>> > >>> I don't know how many times I've said no one is forced to switch to DM > >>> in SPL. > >> > >> This is beside the point, there are boards which use SPL and DM, because > >> the non-DM drivers are steadily going away. So the growth in SPL size is > >> there, either directly or as a side-effect. > > > > I think this is a good point. For serial we have a debug UART. I am > > think it would be useful to have a 'simple bypass' for more > > subsystems. For example, for I2C we could have a simple option to > > access a single I2C driver directly, bypassing driver model. Of course > > this is painful to do before we have completed I2C migration. > > Right. Although it could be prototyped e.g. on the UART subssystem, > which is a good candidate. Yes it is on my mind, once i get my lab working properly. Will have a think about it... Regards, Simon
On 4/15/20 1:01 AM, Tom Rini wrote: > On Tue, Apr 14, 2020 at 05:22:19PM +0200, Marek Vasut wrote: >> On 4/14/20 4:11 PM, Tom Rini wrote: >>> On Tue, Apr 14, 2020 at 03:40:14PM +0200, Marek Vasut wrote: >>>> On 4/14/20 3:26 PM, Tom Rini wrote: >>>>> On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: >>>>>> On 4/14/20 5:03 AM, Tom Rini wrote: >>>>>>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: >>>>>>>> On 4/14/20 1:27 AM, Tom Rini wrote: >>>>>>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: >>>>>>>>> >>>>>>>>>> The most basic printf("%i", value) formating string was missing, >>>>>>>>>> add it for the sake of convenience. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>>>>>>>> Cc: Simon Glass <sjg at chromium.org> >>>>>>>>>> Cc: Stefan Roese <sr at denx.de> >>>>>>>>>> --- >>>>>>>>>> lib/tiny-printf.c | 3 ++- >>>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >>>>>>>>>> index 1138c7012a..8fc7e48d99 100644 >>>>>>>>>> --- a/lib/tiny-printf.c >>>>>>>>>> +++ b/lib/tiny-printf.c >>>>>>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>>>>>> goto abort; >>>>>>>>>> case 'u': >>>>>>>>>> case 'd': >>>>>>>>>> + case 'i': >>>>>>>>>> div = 1000000000; >>>>>>>>>> if (islong) { >>>>>>>>>> num = va_arg(va, unsigned long); >>>>>>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>>>>>> num = va_arg(va, unsigned int); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - if (ch == 'd') { >>>>>>>>>> + if (ch != 'u') { >>>>>>>>>> if (islong && (long)num < 0) { >>>>>>>>>> num = -(long)num; >>>>>>>>>> out(info, '-'); >>>>>>>>> >>>>>>>>> How much does the size change and where do we see this as a problem? >>>>>>>> >>>>>>>> Any code which uses %i in SPL just misbehaves, e.g. >>>>>>>> printf("%s[%i] value=%x", __func__, __LINE__, val); >>>>>>>> prints function name and then incorrect value, because %i is ignored. >>>>>>>> This is also documented in the commit message. >>>>>>>> >>>>>>>> U-Boot grows in size massively due to all the DM/DT bloat which is being >>>>>>>> forced upon everyone, but there the uncontrolled growth is apparently OK >>>>>>>> even if it brings no obvious improvement, rather the opposite. And yet >>>>>>>> here, size increase suddenly matters? Sorry, that's not right. >>>>>>>> >>>>>>>> The code grows by 6 bytes. >>>>>>> >>>>>>> Yes, it matters for _tiny-printf_ as that's where we have little to no >>>>>>> room for growth. >>>>>> >>>>>> How many systems that use tiny-printf in SPL are also forced to use DM >>>>>> in SPL ? >>>>> >>>>> I don't know how many times I've said no one is forced to switch to DM >>>>> in SPL. >>>> >>>> This is beside the point, there are boards which use SPL and DM, because >>>> the non-DM drivers are steadily going away. So the growth in SPL size is >>>> there, either directly or as a side-effect. >>>> >>>>>>> So it's just debug prints you were doing that ran in >>>>>>> to a problem? Thanks! >>>>>> >>>>>> git grep %i indicates ~400 sites where %i is used, so no, not just debug >>>>>> prints. All of those are broken. And no, I'm not inclined to patch all >>>>>> the code to use %d instead of %i just because handling %i is a problem. >>>>> >>>>> Not all 400 of them but the ones that are expected to be used in SPL and >>>>> with TINY_PRINTF need to, yes. Go look at the git log of tiny-printf.c, >>>>> we've changed things around to avoid growth when at all possible. >>>> >>>> I appreciate that. However, I would also appreciate if printf() behaved >>>> in a sane manner, and missing %i support is really weird. >>>> >>>>> Because yes, I don't want to grow a few hundred boards by 6 bytes when >>>>> we have a reasonable alternative. There's 300 hits, of which a dozen >>>>> are non-debug and likely to ever be in SPL code. >>>> >>>> Why don't we instead replace %d with %i altogether then ? The %d seems >>>> to be seldom used outside of U-Boot, where it is only used because of >>>> this tiny-printf limitation, while %i is used quite often. >>>> >>>>> And no, this isn't the >>>>> first time I've raised such an issue, it's just the first time you've >>>>> been hit by this, sorry. >>>> >>>> Does this therefore set a precedent that we are allowed to block any and >>>> all patches which grow SPL size, no matter how useful they might be ? >>> >>> This is following the precedent that was set for tiny printf a while ago >>> with some other "it would be nice if..." format that could instead be >>> handled differently, again for the case of tiny printf. It is not >>> supposed to cover everything, or most things. It is supposed to let >>> SPL/TPL still have printf in otherwise very tight situations. >> >> Except the way it is right now, a lot of output is broken in SPL in an >> inobvious way, which makes working with and/or debugging SPL difficult >> and special, compared to U-Boot and other software. > > No, it's not "a lot", it's whatever you ran in to. We don't have a lot > of "%i" usage to start with and even less that's not a debug print and > also in SPL code. And it prevents any sort of sane code reuse, unless you want to spam the code with #ifdef TINY_PRINTF. >>> And as a reminder, I throw every PR through a before/after size check >>> and flag growth that's global and not fixing a bug that can't be fixed >>> some other way. Change your prints to %d and fix the problem without a >>> size change. >> >> Sorry, no, "change your formatting strings to cater for our broken >> printf() implementation" really is not the solution here. > > Except it is, for the specifically limited printf limitation. Adapt the > print for the limitation and move on. It's not even "rework the code > slightly so the print is correct", it's switch to %d. See above.
On 4/15/20 2:26 AM, Simon Glass wrote: > Hi Marek, Hi, > On Tue, 14 Apr 2020 at 10:32, Marek Vasut <marex at denx.de> wrote: >> >> On 4/14/20 4:41 PM, Simon Glass wrote: >>> Hi Marek, >>> >>> On Tue, 14 Apr 2020 at 07:41, Marek Vasut <marex at denx.de> wrote: >>>> >>>> On 4/14/20 3:26 PM, Tom Rini wrote: >>>>> On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote: >>>>>> On 4/14/20 5:03 AM, Tom Rini wrote: >>>>>>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote: >>>>>>>> On 4/14/20 1:27 AM, Tom Rini wrote: >>>>>>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: >>>>>>>>> >>>>>>>>>> The most basic printf("%i", value) formating string was missing, >>>>>>>>>> add it for the sake of convenience. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>>>>>>>> Cc: Simon Glass <sjg at chromium.org> >>>>>>>>>> Cc: Stefan Roese <sr at denx.de> >>>>>>>>>> --- >>>>>>>>>> lib/tiny-printf.c | 3 ++- >>>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >>>>>>>>>> index 1138c7012a..8fc7e48d99 100644 >>>>>>>>>> --- a/lib/tiny-printf.c >>>>>>>>>> +++ b/lib/tiny-printf.c >>>>>>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>>>>>> goto abort; >>>>>>>>>> case 'u': >>>>>>>>>> case 'd': >>>>>>>>>> + case 'i': >>>>>>>>>> div = 1000000000; >>>>>>>>>> if (islong) { >>>>>>>>>> num = va_arg(va, unsigned long); >>>>>>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) >>>>>>>>>> num = va_arg(va, unsigned int); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - if (ch == 'd') { >>>>>>>>>> + if (ch != 'u') { >>>>>>>>>> if (islong && (long)num < 0) { >>>>>>>>>> num = -(long)num; >>>>>>>>>> out(info, '-'); >>>>>>>>> >>>>>>>>> How much does the size change and where do we see this as a problem? >>>>>>>> >>>>>>>> Any code which uses %i in SPL just misbehaves, e.g. >>>>>>>> printf("%s[%i] value=%x", __func__, __LINE__, val); >>>>>>>> prints function name and then incorrect value, because %i is ignored. >>>>>>>> This is also documented in the commit message. >>>>>>>> >>>>>>>> U-Boot grows in size massively due to all the DM/DT bloat which is being >>>>>>>> forced upon everyone, but there the uncontrolled growth is apparently OK >>>>>>>> even if it brings no obvious improvement, rather the opposite. And yet >>>>>>>> here, size increase suddenly matters? Sorry, that's not right. >>>>>>>> >>>>>>>> The code grows by 6 bytes. >>>>>>> >>>>>>> Yes, it matters for _tiny-printf_ as that's where we have little to no >>>>>>> room for growth. >>>>>> >>>>>> How many systems that use tiny-printf in SPL are also forced to use DM >>>>>> in SPL ? >>>>> >>>>> I don't know how many times I've said no one is forced to switch to DM >>>>> in SPL. >>>> >>>> This is beside the point, there are boards which use SPL and DM, because >>>> the non-DM drivers are steadily going away. So the growth in SPL size is >>>> there, either directly or as a side-effect. >>> >>> I think this is a good point. For serial we have a debug UART. I am >>> think it would be useful to have a 'simple bypass' for more >>> subsystems. For example, for I2C we could have a simple option to >>> access a single I2C driver directly, bypassing driver model. Of course >>> this is painful to do before we have completed I2C migration. >> >> Right. Although it could be prototyped e.g. on the UART subssystem, >> which is a good candidate. > > Yes it is on my mind, once i get my lab working properly. Will have a > think about it... Thanks. This is long overdue to be implemented.
On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote: > The most basic printf("%i", value) formating string was missing, > add it for the sake of convenience. > > Signed-off-by: Marek Vasut <marex at denx.de> > Cc: Simon Glass <sjg at chromium.org> > Cc: Stefan Roese <sr at denx.de> Applied to u-boot/master, thanks!
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 1138c7012a..8fc7e48d99 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) goto abort; case 'u': case 'd': + case 'i': div = 1000000000; if (islong) { num = va_arg(va, unsigned long); @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) num = va_arg(va, unsigned int); } - if (ch == 'd') { + if (ch != 'u') { if (islong && (long)num < 0) { num = -(long)num; out(info, '-');
The most basic printf("%i", value) formating string was missing, add it for the sake of convenience. Signed-off-by: Marek Vasut <marex at denx.de> Cc: Simon Glass <sjg at chromium.org> Cc: Stefan Roese <sr at denx.de> --- lib/tiny-printf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)