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

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

[Tom Rini]

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@kernel.org>
Reported-by: Paul Kocialkowski <contact@paulk.fr>
Signed-off-by: Tom Rini <trini@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];
 
+#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
+
 	/*
 	 * Look for a '-' which indicates to ignore the
 	 * ramdisk argument
-- 
1.7.9.5

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

[Rob Herring]

On Thu, Aug 27, 2015 at 2:42 PM, Tom Rini <trini@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@kernel.org>
> Reported-by: Paul Kocialkowski <contact@paulk.fr>
> Signed-off-by: Tom Rini <trini@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.

>
> +#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.

Rob

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

[Tom Rini]

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@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@kernel.org>
> > Reported-by: Paul Kocialkowski <contact@paulk.fr>
> > Signed-off-by: Tom Rini <trini@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.

> > +#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.

-- 
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/20150827/958bf440/attachment.sig>

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

[Rob Herring]

On Thu, Aug 27, 2015 at 4:47 PM, Tom Rini <trini@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@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@kernel.org>
>> > Reported-by: Paul Kocialkowski <contact@paulk.fr>
>> > Signed-off-by: Tom Rini <trini@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.

>> > +#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.

Rob

[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@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@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@kernel.org>
> >> > Reported-by: Paul Kocialkowski <contact@paulk.fr>
> >> > Signed-off-by: Tom Rini <trini@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>

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

[Paul Kocialkowski]

Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
> 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).

That patch does fix my problem (the ramdisk is now correctly passed to
the kernel). I suggest that you merge it ASAP.

Thanks!

> Cc: Rob Herring <robh@kernel.org>
> Reported-by: Paul Kocialkowski <contact@paulk.fr>
> Signed-off-by: Tom Rini <trini@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];
>  
> +#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
> +
>  	/*
>  	 * Look for a '-' which indicates to ignore the
>  	 * ramdisk argument

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150901/56473b7c/attachment.sig>

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

[Rob Herring]

On Tue, Sep 1, 2015 at 8:50 AM, Paul Kocialkowski <contact@paulk.fr> wrote:
> Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
>> 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).
>
> That patch does fix my problem (the ramdisk is now correctly passed to
> the kernel). I suggest that you merge it ASAP.

Doesn't look like this was ever merged or respun. I had some comments,
but have no issue if they addressed separately as part of some
refactoring.

Rob

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

[Paul Kocialkowski]

Hi,

Le lundi 05 octobre 2015 ? 14:23 -0500, Rob Herring a ?crit :
> On Tue, Sep 1, 2015 at 8:50 AM, Paul Kocialkowski <contact@paulk.fr>
> wrote:
> > Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
> > > 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).
> > 
> > That patch does fix my problem (the ramdisk is now correctly passed
> > to
> > the kernel). I suggest that you merge it ASAP.
> 
> Doesn't look like this was ever merged or respun. I had some
> comments,
> but have no issue if they addressed separately as part of some
> refactoring.

Well, I would like to see this getting merged soon, because Android
booting on sniper (LG Optimus Black) is currently broken.

Tom, would you consider picking it up for the current rc round?
I realize I had forgotten about this patch and should have suggested it
earlier on in the cycle.

Either way, I should test the current rc on my device and report back
if there is anything else going wrong.

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis
on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151007/d99b0234/attachment.sig>

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

[Paul Kocialkowski]

Hi,

Le mercredi 07 octobre 2015 ? 20:07 +0200, Paul Kocialkowski a ?crit :
> Le lundi 05 octobre 2015 ? 14:23 -0500, Rob Herring a ?crit :
> > On Tue, Sep 1, 2015 at 8:50 AM, Paul Kocialkowski <contact@paulk.fr>
> > wrote:
> > > Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
> > > > 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).
> > > 
> > > That patch does fix my problem (the ramdisk is now correctly passed
> > > to
> > > the kernel). I suggest that you merge it ASAP.
> > 
> > Doesn't look like this was ever merged or respun. I had some
> > comments,
> > but have no issue if they addressed separately as part of some
> > refactoring.
> 
> Well, I would like to see this getting merged soon, because Android
> booting on sniper (LG Optimus Black) is currently broken.
> 
> Tom, would you consider picking it up for the current rc round?
> I realize I had forgotten about this patch and should have suggested it
> earlier on in the cycle.
> 
> Either way, I should test the current rc on my device and report back
> if there is anything else going wrong.

This is about the only problem I have on the LG Optimus Black P970
(codename sniper) with the current git master, so merging this patch
makes the device usable.

-- 
Paul Kocialkowski 

* Site web : http://www.paulk.fr/
* Blog : http://blog.paulk.fr/
* Dev blog : http://code.paulk.fr/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151010/929ad834/attachment.sig>

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

[Tom Rini]

On Mon, Oct 05, 2015 at 02:23:40PM -0500, Rob Herring wrote:
> On Tue, Sep 1, 2015 at 8:50 AM, Paul Kocialkowski <contact@paulk.fr> wrote:
> > Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
> >> 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).
> >
> > That patch does fix my problem (the ramdisk is now correctly passed to
> > the kernel). I suggest that you merge it ASAP.
> 
> Doesn't look like this was ever merged or respun. I had some comments,
> but have no issue if they addressed separately as part of some
> refactoring.

OK.  With respect to the code flow, that's on the "someday, probably"
list.  I've shuffled things around so that you can still provide a
separate ramdisk to override the image as that was the use-case I hadn't
considered before.

-- 
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/20151011/45cd43e2/attachment.sig>

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

[Tom Rini]

On Thu, Aug 27, 2015 at 03:42:41PM -0400, Tom Rini 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@kernel.org>
> Reported-by: Paul Kocialkowski <contact@paulk.fr>
> Signed-off-by: Tom Rini <trini@konsulko.com>

Moved around slightly so that we can override the ramdisk still and
then:

Applied to u-boot/master, thanks!

-- 
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/20151012/09779d54/attachment.sig>