From owner-svn-src-all@FreeBSD.ORG Fri Jun 19 23:56:49 2015 Return-Path: Delivered-To: svn-src-all@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id DE2877E4 for ; Fri, 19 Jun 2015 23:56:49 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: from mail-wi0-f181.google.com (mail-wi0-f181.google.com [209.85.212.181]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6D852A58 for ; Fri, 19 Jun 2015 23:56:48 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: by wiga1 with SMTP id a1so31791193wig.0 for ; Fri, 19 Jun 2015 16:56:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=6wNrYAU4Q7jTfbSxEgMHZDf3xSL1mT91+FjTS7M6zPE=; b=h6VfKvouV1WXozwCvwp32vYFw1ypRKH/zPNNdei8bVDi5e3N7SgrPN0/wriX2loGYQ tsVz22wWK/TkbdVMFUYURpFAQFl4sFxdYw80aKHnftc/dwy5B4cKAmW5MlpG7a9yuhUe QGcvnMgOpJDgmNN9dwCGVXoZQDtz/TlK17Jt9yMo3gSV6svnq6J50+pg2SKnbT5H0oHJ SnRTYCtpp+VfRHtjw9LYYCqE6dMcWe2gO6qhsvjK9Qi6Si3L+LkJmYRrcjlTL/KvUGz6 2DOeNZYRH3RuCho5r60211v2fcsCqIUNNw0JT92bh4OgjU3qVVX1FxjCk09FII8q1srL wy4Q== X-Gm-Message-State: ALoCoQmOZSt9DU7GcyhDRhEaz8TfEbZYsTeL3tyYyiA1/HLdkyINyCyGnx3HkujhLjmo/6KW9tVl MIME-Version: 1.0 X-Received: by 10.180.72.176 with SMTP id e16mr11329339wiv.12.1434758207467; Fri, 19 Jun 2015 16:56:47 -0700 (PDT) Sender: sobomax@sippysoft.com Received: by 10.27.205.66 with HTTP; Fri, 19 Jun 2015 16:56:47 -0700 (PDT) In-Reply-To: <1434755385.1415.114.camel@freebsd.org> References: <201506192224.t5JMOxpC097306@svn.freebsd.org> <1434755385.1415.114.camel@freebsd.org> Date: Fri, 19 Jun 2015 16:56:47 -0700 X-Google-Sender-Auth: nNqOVZRIjAgJSlcjasnpgpq55fE Message-ID: Subject: Re: svn commit: r284614 - head/sys/boot/uboot/lib From: Maxim Sobolev To: Ian Lepore Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Jun 2015 23:56:50 -0000 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 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 > >