Date: Fri, 19 Jun 2015 16:56:47 -0700 From: Maxim Sobolev <sobomax@FreeBSD.org> To: Ian Lepore <ian@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r284614 - head/sys/boot/uboot/lib Message-ID: <CAH7qZfuj4ZqEVVWtROZXTsrhy7C2DFw06ijJxUwbLbGEDtRPbQ@mail.gmail.com> In-Reply-To: <1434755385.1415.114.camel@freebsd.org> References: <201506192224.t5JMOxpC097306@svn.freebsd.org> <1434755385.1415.114.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Ian, that's cool and dandy, but I still suggest we put some sanity checking and have certain workarounds in the loader (whenever it does not add ambiguity or blows up a code too much of course), so that new folks in town trying to port to new platforms like myself won't spend hours and hours hunting known issues and bugs. And hitting those infinite loops is very frustrating with no errors or anything. On top of that, in some cases you may be stuck with vendor-provided u-boot with no way to patch and re-compile. BTW, there is another stupid bug existing in the u-boot loader, which basically sets fdtaddr in decimal not in hex. On my particular board this makes ubldr to blow up with CPU exception, unfortunately no workaround is possible since there is no 0x for hex values and majority of cases when this variable is set is in hex. -Maxim On Fri, Jun 19, 2015 at 4:09 PM, Ian Lepore <ian@freebsd.org> wrote: > On Fri, 2015-06-19 at 22:24 +0000, Maxim Sobolev wrote: > > Author: sobomax > > Date: Fri Jun 19 22:24:58 2015 > > New Revision: 284614 > > URL: https://svnweb.freebsd.org/changeset/base/284614 > > > > Log: > > Provide bug4bug workaround for certain dumbiness of the u-boot's > API_env_enum > > function, which is expected to set returned env to NULL upon reaching > the end > > of the environment list but fails to do so in certain cases. The > respective > > u-boot code looks like the following (HEAD at the time of this commit): > > > > --- api.c --- > > 496 static int API_env_enum(va_list ap) > > ... > > 510 *next = last; > > 511 > > 512 for (i = 0; env_get_char(i) != '\0'; i = n + 1) { > > 513 for (n = i; env_get_char(n) != '\0'; ++n) > { > > 514 if (n >= CONFIG_ENV_SIZE) { > > 515 /* XXX shouldn't we set > *next = NULL?? */ > > 516 return 0; > > 517 } > > 518 } > > ------------- > > > > The net result is that any unfortunate user of the loader's > ub_env_enum() > > function hitting this condition would be trapped in the infinite loop, > as > > the main use pattern of ub_env_enum() is basically the following: > > > > while ((env = ub_env_enum(env)) != NULL) { DO STUFF } > > > > Which would stuck forever with the last element. > > > > Modified: > > head/sys/boot/uboot/lib/glue.c > > > > Modified: head/sys/boot/uboot/lib/glue.c > > > ============================================================================== > > --- head/sys/boot/uboot/lib/glue.c Fri Jun 19 21:55:12 2015 > (r284613) > > +++ head/sys/boot/uboot/lib/glue.c Fri Jun 19 22:24:58 2015 > (r284614) > > @@ -513,7 +513,7 @@ ub_env_enum(const char *last) > > if (!syscall(API_ENV_ENUM, NULL, (uint32_t)last, (uint32_t)&env)) > > return (NULL); > > > > - if (env == NULL) > > + if (env == NULL || last == env) > > /* no more env. variables to enumerate */ > > return (NULL); > > > > > > This is only a problem with an unpatched u-boot, which has a completely > bogus and un-useful implementation of API_env_enum(). That's why every > one of our u-boot ports has the same patch to put in an implementation > that actually works. > > Your change works around the worst part of the bug (the infinite loop) > but leaves the major problem of the implementation only returning values > initially loaded from the saved environment, not anything set by the > scripts that loaded ubldr(). > > -- Ian > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAH7qZfuj4ZqEVVWtROZXTsrhy7C2DFw06ijJxUwbLbGEDtRPbQ>