[U-Boot] [PATCH] common/image.c: Make boot_get_ramdisk() perform a check for Android images

[Tom Rini]

On Fri, Aug 28, 2015 at 10:35:50AM -0500, Rob Herring wrote:
> On Thu, Aug 27, 2015 at 4:47 PM, Tom Rini <trini at konsulko.com> wrote:
> > On Thu, Aug 27, 2015 at 04:04:30PM -0500, Rob Herring wrote:
> >> On Thu, Aug 27, 2015 at 2:42 PM, Tom Rini <trini at konsulko.com> wrote:
> >> > In 2dd4632 the check for where a ramdisk is found on an Android image
> >> > was got moved into the "normal" loop here, causing people to have to
> >> > pass the kernel address in the ramdisk address location in order to have
> >> > Android boot still.  This changed previous behavior so perform a check
> >> > early in the function to see if we have an Android image and if so use
> >> > that as where to look for the ramdisk (which is what the rest of the
> >> > code here expects).
> >> >
> >> > Cc: Rob Herring <robh at kernel.org>
> >> > Reported-by: Paul Kocialkowski <contact at paulk.fr>
> >> > Signed-off-by: Tom Rini <trini at konsulko.com>
> >> > ---
> >> >  common/image.c |    9 +++++++++
> >> >  1 file changed, 9 insertions(+)
> >> >
> >> > diff --git a/common/image.c b/common/image.c
> >> > index ca721c5..e938bea 100644
> >> > --- a/common/image.c
> >> > +++ b/common/image.c
> >> > @@ -907,6 +907,15 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
> >> >         if (argc >= 2)
> >> >                 select = argv[1];
> >>
> >> Perhaps this check should come second so you could override the
> >> ramdisk with "bootm <bootimg> <ramdisk>". Then again, maybe people
> >> should have to pick between a bootimg or separate components.
> >
> > Yeah, no, I'm not convinced there's a good case for "Android image
> > kernel+ramdisk 1" kernel + "Android image+ramdisk 2" ramdisk where you
> > wouldn't be cobbling that particular combination together outside of
> > U-Boot anyhow.  Is there really?  And we still allow for disabling the
> > ramdisk.  Or of course just loading separate components and booting
> > Android that way.
> 
> I was thinking a separate raw ramdisk. But yes, I agree it is probably
> better if we don't support all random combinations.

OK, separate raw ramdisk is a use case I can get behind, so check
android image @ argv[0], then if argv[1] exists poke at that.

> >> > +#ifdef CONFIG_ANDROID_BOOT_IMAGE
> >> > +       /*
> >> > +        * Look for an Android boot image.
> >> > +        */
> >> > +       buf = map_sysmem(images->os.start, 0);
> >> > +       if (genimg_get_format(buf) == IMAGE_FORMAT_ANDROID)
> >> > +               select = argv[0];
> >> > +#endif
> >>
> >> Tracing code paths in these functions is bad enough. I would do
> >> something like this to simplify the code path:
> >>
> >> buf = map_sysmem(images->os.start, 0);
> >> if (genimg_get_format(buf) == IMAGE_FORMAT_ANDROID) {
> >>   ret = android_image_get_ramdisk((void *)images->os.start, rd_start, &rd_len);
> >>   *rd_end = *rd_start + rd_len;
> >>   return ret;
> >> }
> >>
> >> And then remove the case statement. We loose dataflash copy and some
> >> debug prints.
> >
> > But then we're also saying Android images are a special case that needs
> > to be treated differently than everyone else here.
> 
> The code seems to indicate it is given that most of it doesn't apply...
> 
> Having multiple decision points based on the image type makes it hard
> to follow the flow. It would be much better if the code structure was:
> 
> common setup/parsing
> switch (image type)
>   - handle each image type
> common clean-up
> 
> Maybe this function is the wrong level to do this restructuring. The
> bootm related code needs some love, but I'm afraid to touch it with
> any major change.

Yeah, Simon gave things a re-org a while back and it took forever to
shake out some of the corner cases.  There's room for futher
improvement still.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150828/161f155d/attachment.sig>