[PATCH] CBFS decompress error

[Myles Watson]

Move the ulzma prototype into cbfs.h
Check the return value when decompressing.

Signed-off-by: Myles Watson

It's not 100% fixed, since the caller (cbfs_load_stage)

if (cbfs_decompress(stage->compression,
    		     ((unsigned char *) stage) +
    		     sizeof(struct cbfs_stage),
    		     (void *) (u32) stage->load,
    		     stage->len))
    	return (void *) -1;

returns -1 and then cbfs_and_run_core jumps to it.

print_debug("Jumping to image.\r\n");
dst = cbfs_load_stage(filename);
print_debug("Jumping to image.\r\n");

Why do we jump to a -1 instead of hanging?

Thanks,
Myles

[Myles Watson]

On Thu, Oct 8, 2009 at 2:49 PM, Myles Watson  wrote:
> Move the ulzma prototype into cbfs.h
> Check the return value when decompressing.

With the patch.
Signed-off-by: Myles Watson
> It's not 100% fixed, since the caller (cbfs_load_stage)
>
> if (cbfs_decompress(stage->compression,
>                             ((unsigned char *) stage) +
>                             sizeof(struct cbfs_stage),
>                             (void *) (u32) stage->load,
>                             stage->len))
>                return (void *) -1;
>
> returns -1 and then cbfs_and_run_core jumps to it.
>
> print_debug("Jumping to image.\r\n");
> dst = cbfs_load_stage(filename);
> print_debug("Jumping to image.\r\n");
>
> Why do we jump to a -1 instead of hanging?
>
> Thanks,
> Myles

[Peter Stuge]

Myles Watson wrote:
> case CBFS_COMPRESS_LZMA:
> if (!ulzma(src, dst)) {
> 	printk_err("CBFS: ulzma returned 0!\n");
> 	return -1;
> }

Please say "ulzma() failed!" or maybe "LZMA decompression failed!" ?

Other than that,

Acked-by: Peter Stuge

[Myles Watson]

On Thu, Oct 8, 2009 at 2:57 PM, Peter Stuge  wrote:
> Myles Watson wrote:
> > case CBFS_COMPRESS_LZMA:
> > if (!ulzma(src, dst)) {
> > printk_err("CBFS: ulzma returned 0!\n");
> > return -1;
> > }
>
> Please say "ulzma() failed!" or maybe "LZMA decompression failed!" ?

Good point.  Fixed.

Thanks,
Myles

[Myles Watson]

> Please say "ulzma() failed!" or maybe "LZMA decompression failed!" ?
>
> Other than that,
>
> Acked-by: Peter Stuge

With your suggestion and Patrick's taken into account.

Rev 4752.

Thanks,
Myles

[Patrick Georgi]

Am Donnerstag, den 08.10.2009, 14:49 -0600 schrieb Myles Watson:
> Move the ulzma prototype into cbfs.h

Not sure if I like it - that way, ulzma() becomes part of the public
interface of cbfs.
Any issue this fixes?
> Check the return value when decompressing.

That one is nice. Thank you.
> Signed-off-by: Myles Watson
>
> It's not 100% fixed, since the caller (cbfs_load_stage)
>
> if (cbfs_decompress(stage->compression,
>    		     ((unsigned char *) stage) +
>    		     sizeof(struct cbfs_stage),
>    		     (void *) (u32) stage->load,
>    		     stage->len))
>    	return (void *) -1;
>
> returns -1 and then cbfs_and_run_core jumps to it.
>
> print_debug("Jumping to image.\r\n");
> dst = cbfs_load_stage(filename);
> print_debug("Jumping to image.\r\n");
>
> Why do we jump to a -1 instead of hanging?

-1 is a known free value (it points into ROM, at a highly unlikely
address) and as such used as "invalid image" marker.

It should be caught by the caller, which _might_ have some better idea
than hanging (eg. some backup image), so this is not an option inside
cbfs_load_stage.

Patrick

[Myles Watson]

On Thu, Oct 8, 2009 at 2:56 PM, Patrick Georgi  wrote:
> Am Donnerstag, den 08.10.2009, 14:49 -0600 schrieb Myles Watson:
> > Move the ulzma prototype into cbfs.h
>
> Not sure if I like it - that way, ulzma() becomes part of the public
> interface of cbfs.
> Any issue this fixes?

Just the ugliness of having the prototype in the code right before the
call.  I'd be fine with putting it somewhere else.  The reason I put
it where I did is because it is right after the CBFS_COMPRESS_LZMA
define.
> > Check the return value when decompressing.
>
> That one is nice. Thank you.
> > Signed-off-by: Myles Watson
> >
> > It's not 100% fixed, since the caller (cbfs_load_stage)
> >
> > if (cbfs_decompress(stage->compression,
> >                            ((unsigned char *) stage) +
> >                            sizeof(struct cbfs_stage),
> >                            (void *) (u32) stage->load,
> >                            stage->len))
> >               return (void *) -1;
> >
> > returns -1 and then cbfs_and_run_core jumps to it.
> >
> > print_debug("Jumping to image.\r\n");
> > dst = cbfs_load_stage(filename);
> > print_debug("Jumping to image.\r\n");
> >
> > Why do we jump to a -1 instead of hanging?
>
> -1 is a known free value (it points into ROM, at a highly unlikely
> address) and as such used as "invalid image" marker.

It causes my machine to reboot.  I was wondering if that was intentional.
> It should be caught by the caller, which _might_ have some better idea
> than hanging (eg. some backup image), so this is not an option inside
> cbfs_load_stage.

Unless you have normal and fallback?

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 08.10.2009 22:49, Myles Watson wrote:
> Check the return value when decompressing.
>
> It's not 100% fixed, since the caller (cbfs_load_stage)
> returns -1 and then cbfs_and_run_core jumps to it.
>
> print_debug("Jumping to image.\r\n");
> dst = cbfs_load_stage(filename);
> print_debug("Jumping to image.\r\n");
>
> Why do we jump to a -1 instead of hanging?

I sent a patch do die() instead, but Stefan wrote he had a more
extensive pending in that area, so my patch was not committed. See
http://patchwork.coreboot.org/patch/203/ for details.

Regards,
Carl-Daniel

[Myles Watson]

On Thu, Oct 8, 2009 at 3:30 PM, Carl-Daniel Hailfinger
 wrote:
> On 08.10.2009 22:49, Myles Watson wrote:
> > Check the return value when decompressing.
> >
> > It's not 100% fixed, since the caller (cbfs_load_stage)
> > returns -1 and then cbfs_and_run_core jumps to it.
> >
> > print_debug("Jumping to image.\r\n");
> > dst = cbfs_load_stage(filename);
> > print_debug("Jumping to image.\r\n");
> >
> > Why do we jump to a -1 instead of hanging?
>
> I sent a patch do die() instead,

I thought it seemed familiar.  I just hit it for the first time.
> but Stefan wrote he had a more
> extensive pending in that area, so my patch was not committed. See
> http://patchwork.coreboot.org/patch/203/ for details.

It looks like he was talking about the linker error, right?

Thanks,
Myles

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 08.10.2009 22:49, Myles Watson wrote:
> > Check the return value when decompressing.
> >
> > It's not 100% fixed, since the caller (cbfs_load_stage)
> > returns -1 and then cbfs_and_run_core jumps to it.
> >
> > print_debug("Jumping to image.\r\n");
> > dst = cbfs_load_stage(filename);
> > print_debug("Jumping to image.\r\n");
> >
> > Why do we jump to a -1 instead of hanging?
>
> I sent a patch do die() instead, but Stefan wrote he had a more
> extensive pending in that area, so my patch was not committed.

Go ahead, I think I wiped that tree by accident. Sorry for causing the
delay.

Stefan