From owner-svn-src-head@FreeBSD.ORG Sat Jun 20 00:23:04 2015 Return-Path: Delivered-To: svn-src-head@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 F3FE52F7 for ; Sat, 20 Jun 2015 00:23:03 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: from mail-wg0-f41.google.com (mail-wg0-f41.google.com [74.125.82.41]) (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 88240146 for ; Sat, 20 Jun 2015 00:23:03 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: by wgbhy7 with SMTP id hy7so100975913wgb.2 for ; Fri, 19 Jun 2015 17:22:56 -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=EcNuQiXKy+aQT4F8OGbD6n9unlTVb8Xn5+ecNvFVDssIuB2q6ll+/HIriWQ5zWnSJv 0PbgyoD/0cXz8s2K4MIeaADCQgy4KCuQSKh+N6I1aYxch4zAhR6DUw1r2mzeZx1L03oK KLT52ELIdnYsSmmhsmLOXLwIlCvfqpfaHEamXTYmPsWr37HDe741xyv6J0obSwvOy9Qm 8rcuAJcNanj3Gc5ucaKY+ZH7PnYM0qrwfzspIkiFuvbMR+fyCPgxDdcBRC+0o2jVcu3i 71x80aTP7YnFqHzLGr1rEGCgqpr+6Pwd3Cvx+YmPF4onxI+kkVpBo3qJjspciAOReAvI 5+Rg== X-Gm-Message-State: ALoCoQlAI2j6hMp2j/cC/BMv+N/Pw/DiNwjQ3C8oVLW+58uf72mVd8y1CvZtAdNYXWuZeyNMIJj6 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-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Jun 2015 00:23:04 -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 > >