Date: Mon, 17 Mar 2014 10:21:28 -0400 From: Patrick Kelsey <kelsey@ieee.org> To: Ian Lepore <ian@freebsd.org> Cc: freebsd-arm <freebsd-arm@freebsd.org> Subject: Re: Booting FreeBSD from eMMC on BeagleBone Black Message-ID: <CAD44qMU0iDE=sJTyLfh2AD2aPRyM5sCgeq8U3MVgaCSpVGxsvw@mail.gmail.com> In-Reply-To: <1395064661.1149.565.camel@revolution.hippie.lan> References: <CAD44qMXrKbZqXT_Z1UL2LXuVON4Gb49m-GygW_6Y14Zz-egTFw@mail.gmail.com> <E1283934-0065-4979-92AA-99D1056BFBD5@FreeBSD.org> <CAD44qMVxoM=MdvDfosJgxQycYYNmxgMii3_0z91PGwHKXmszMg@mail.gmail.com> <1395064661.1149.565.camel@revolution.hippie.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Mar 17, 2014 at 9:57 AM, Ian Lepore <ian@freebsd.org> wrote:
> On Sun, 2014-03-16 at 22:16 -0400, Patrick Kelsey wrote:
> > On Sun, Mar 16, 2014 at 9:33 PM, Rui Paulo <rpaulo@freebsd.org> wrote:
> >
> > > On 16 Mar 2014, at 14:59, Patrick Kelsey <kelsey@ieee.org> wrote:
> > >
> > > > - Improved disk probing support that will now by default find and
> use the
> > > > first suitable partition among the available storage devices.
> > >
> > > I think this introduced a bug where, if you have a non-responsive boot
> > > device, ubldr will stop and won't try network booting:
> > >
> > > ## Starting application at 0x01000054 ...
> > > Consoles: U-Boot console
> > > Compatible API signature found @1d800a8
> > > Number of U-Boot devices: 2
> > >
> > > FreeBSD/armv6 U-Boot loader, Revision 1.2
> > > (rpaulo@zedfs.local, Fri Mar 14 22:35:47 PDT 2014)
> > > DRAM: 256MB
> > > Unknown device type '' <------------ this is new
> > > Found U-Boot device: disk
> > > Probing all storage devices...
> > > Checking unit=0 slice=0 partition=-1...disk0: read failed, error=1
> > >
> > > Checking unit=1 slice=0 partition=-1...
> > > Checking unit=2 slice=0 partition=-1...
> > > Checking unit=3 slice=0 partition=-1...
> > > Checking unit=4 slice=0 partition=-1...
> > > Checking unit=5 slice=0 partition=-1...
> > >
> > > can't load 'kernel'
> > >
> > > Type '?' for a list of commands, 'help' for more detailed help.
> > > loader>
> > >
> > > It stops here and doesn't try net0 booting.
> > >
> > >
> > I think the problem is that some of the conditionals in
> > sys/boot/uboot/common/main.c:main() are broken. I believe I sowed the
> seed
> > for this in the original patch I sent to Ian, which appears to have had
> an
> > out-of-order set of tests in the disk conditional, which in hindsight
> > turned out to work due to a friendly coincidence (namely disk appearing
> > before net in the devsw). That bad-pattern conditional seems to have
> > gotten munged a bit further and propagated in some of the refactoring Ian
> > did when integrating my patch.
> >
> > I believe sys/boot/uboot/common/main.c, starting around line 442, should
> > look like this:
> >
> > if (strcmp(devsw[i]->dv_name, "disk") == 0 &&
> > (load_type == -1 || (load_type & DEV_TYP_STOR))) {
> > if (probe_disks(i, load_type, load_unit,
> load_slice,
> > load_partition) == 0)
> > break;
> > }
> >
> > if (strcmp(devsw[i]->dv_name, "net") == 0 &&
> > (load_type == -1 || (load_type & DEV_TYP_NET)))
> > break;
> >
> >
> > Can you give that a try?
> >
> > -Patrick
>
> This was my bad. I think I was so enmeshed in whether the strange
> original strncmp() calls were really just a silly form of strcmp() (they
> were) that I didn't even notice I screwed up the overall logic.
>
> Rather than putting the strcmp() first as shown above, I just adjusted
> the parens in the network if() to be what I should have done originally.
>
>
I don't believe that approach will fix it. It's not just the parens in the
"net" conditional that are the issue - I had the order of the tests in the
"disk" conditional backwards to begin with. To get the desired behavior
(proper fallback to net), the test of devsw[]->dv_name needs to be first so
that a load_type of -1 cannot short circuit it in a loop iteration that is
for another type. Otherwise, when load_type is -1, coming out of an
unsuccessful probe_disks() will result in an exit from the loop via the
"net" conditional, and you will wind up out of the loop, but without
currdev set up properly for the net device (it will still be set up for
"disk", and with a possibly non-zero unit number). Putting the test of
devsw[]->dv_name first ensures that the loop won't be exited on behalf of a
given type without currdev being set up for that type.
-Patrick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAD44qMU0iDE=sJTyLfh2AD2aPRyM5sCgeq8U3MVgaCSpVGxsvw>