[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Alessandro Rubini]

The entry point of an image can be 0 (like on PowerPC), so allow next_out
to be NULL in inflate()

Signed-off-by: Alessandro Rubini <rubini@unipv.it>
---

Stefan Roese:
> I'll try to uncompress to something != 0 tomorrow on PPC.

You are right. I naively thought my arm has RAM at 0 like PPC, but
entry point of image is not really 0.


 lib_generic/zlib.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib_generic/zlib.c b/lib_generic/zlib.c
index f415f6b..49fb145 100644
--- a/lib_generic/zlib.c
+++ b/lib_generic/zlib.c
@@ -1365,7 +1365,7 @@ int flush;
     static const unsigned short order[19] = /* permutation of code lengths */
         {16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15};
 
-    if (strm == Z_NULL || strm->state == Z_NULL || strm->next_out == Z_NULL ||
+    if (strm == Z_NULL || strm->state == Z_NULL ||
         (strm->next_in == Z_NULL && strm->avail_in != 0))
         return Z_STREAM_ERROR;
 
-- 
1.5.6.5

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[rhabarber1848]

Alessandro Rubini wrote:

> The entry point of an image can be 0 (like on PowerPC), so allow next_out
> to be NULL in inflate()

Hi,

this patch solves the inflate() error but now a watchdog reset is triggered
during decompression:

## Booting kernel from Legacy Image at 00200000 ...
   Image Name:   Linux-2.4.37.2-dbox2
   Image Type:   PowerPC Linux Kernel Image (gzip compressed)
   Data Size:    772042 Bytes = 753.9 kB
   Load Address: 00000000
   Entry Point:  00000000
   Verifying Checksum ... OK
   Uncompressing Kernel Image ...
                                  debug: DDF: Calibrating delay loop...
debug: DDF: 66.76 BogoMIPS
debug: WATCHDOG RESET
[...]

The neither the old nor the new code incorporate WATCHDOG_RESET() so the new
code seems to be slower than the old one which works. I have yet to dig
through the zlib code to insert some WATCHDOG_RESET() here and there like I
did with the LZMA code.

Cheers, rhabarber1848

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Wolfgang Denk]

Dear rhabarber1848,

In message <h4kq2v$eoj$1@ger.gmane.org> you wrote:
> 
> this patch solves the inflate() error but now a watchdog reset is triggered
> during decompression:
...
> The neither the old nor the new code incorporate WATCHDOG_RESET() so the new

You are wrong. The urrent code installs WATCHDOG_RESET as s.outcb()
callback; see function zunzip() in "lib_generic/gunzip.c".

> code seems to be slower than the old one which works. I have yet to dig
> through the zlib code to insert some WATCHDOG_RESET() here and there like I
> did with the LZMA code.

I think this is the wrong thing to do. Use the provided callback
instead.


I wonder why you do not create a patch between the old original
version and what wd have in U-Boot now (which should clearly show
what has been changed to adapt this code for U-Boot), and then apply
(if necessary, manually) that patch to the current version?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"You ain't experienced..." "Well, nor are you." "That's true. But the
point is ... the point is ... the point is we've been not experienced
for a lot longer than you. We've got  a  lot  of  experience  of  not
having any experience."           - Terry Pratchett, _Witches Abroad_

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[rhabarber1848]

Wolfgang Denk wrote:

> I wonder why you do not create a patch between the old original
> version and what wd have in U-Boot now (which should clearly show
> what has been changed to adapt this code for U-Boot), and then apply
> (if necessary, manually) that patch to the current version?

Hi Wolfgang,

I did exactly that, I took rc1, downloaded the zlib-1.2.3 patch from the
newsgroup[1] and added the patch by Alessandro[2]. The WATCHDOG_RESET code
in gunzip.c is not touched by either patches, but I still get a
WATCHDOG_RESET when the kernel image is decompressed with the new code.

Though is seems that

if (z->outcb != Z_NULL)
     (*z->outcb)(Z_NULL, 0);

is not used anymore in zlib.c so the WATCHDOG_RESET calls are silently
ignored. I will try to re-add them tomorrow.

Besides this, rc1 is working fine here.

Cheers, rhabarber1848

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/64727
[2] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/64969

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Wolfgang Denk]

Dear rhabarber1848,

In message <h4l1p8$d52$1@ger.gmane.org> you wrote:
>
> Though is seems that
> 
> if (z->outcb != Z_NULL)
>      (*z->outcb)(Z_NULL, 0);
> 
> is not used anymore in zlib.c so the WATCHDOG_RESET calls are silently
> ignored. I will try to re-add them tomorrow.

I see.

> Besides this, rc1 is working fine here.

Thanks for the confirmation.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Pardon me for breathing, which I never do anyway so I don't know why
I bother to say it, oh God, I'm so depressed. Here's another of those
self-satisfied doors. Life! Don't talk to me about life."
                                        - Marvin the Paranoid Android

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Stefan Roese]

On Monday 27 July 2009 18:40:31 Alessandro Rubini wrote:
> The entry point of an image can be 0 (like on PowerPC), so allow next_out
> to be NULL in inflate()
>
> Signed-off-by: Alessandro Rubini <rubini@unipv.it>
> ---
>
> Stefan Roese:
> > I'll try to uncompress to something != 0 tomorrow on PPC.
>
> You are right. I naively thought my arm has RAM at 0 like PPC, but
> entry point of image is not really 0.

Yes, this works:

Tested-by: Stefan Roese <sr@denx.de>

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Giuseppe CONDORELLI]

Very good!!!
So is it time to re-apply zlib patch to u-boot main tree, together
Alessandro's patch?

Wolfgang, what's your position?

Best Regards,
Giuseppe

-----Original Message-----
From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces at lists.denx.de] On
Behalf Of Stefan Roese
Sent: Tuesday, July 28, 2009 8:31 AM
To: Alessandro Rubini
Cc: u-boot at lists.denx.de; rhabarber1848 at web.de
Subject: Re: [U-Boot] [PATCH] zlib: allow 0 as destination pointer

On Monday 27 July 2009 18:40:31 Alessandro Rubini wrote:
> The entry point of an image can be 0 (like on PowerPC), so allow next_out
> to be NULL in inflate()
>
> Signed-off-by: Alessandro Rubini <rubini@unipv.it>
> ---
>
> Stefan Roese:
> > I'll try to uncompress to something != 0 tomorrow on PPC.
>
> You are right. I naively thought my arm has RAM at 0 like PPC, but
> entry point of image is not really 0.

Yes, this works:

Tested-by: Stefan Roese <sr@denx.de>

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Wolfgang Denk]

Dear Giuseppe CONDORELLI,

In message <001601ca1012$e0f72730$c08182a4@st.com> you wrote:
> Very good!!!
> So is it time to re-apply zlib patch to u-boot main tree, together
> Alessandro's patch?
> 
> Wolfgang, what's your position?

Actually I'm waiting for a new patch that integrates that fix.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Just about every computer on the market today runs Unix,  except  the
Mac (and nobody cares about it).                   - Bill Joy 6/21/85

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Giuseppe CONDORELLI]

Dear Wolfgang,

> Actually I'm waiting for a new patch that integrates that fix.
Maybe I've not understood.
Do you mean you are waiting a new zlib patch [v4] that includes Alessandro's
patch, to apply
only one patch containing all?

Best Regards,
Giuseppe
-----Original Message-----
From: Wolfgang Denk [mailto:wd at denx.de] 
Sent: Wednesday, July 29, 2009 8:24 AM
To: Giuseppe CONDORELLI
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH] zlib: allow 0 as destination pointer

Dear Giuseppe CONDORELLI,

In message <001601ca1012$e0f72730$c08182a4@st.com> you wrote:
> Very good!!!
> So is it time to re-apply zlib patch to u-boot main tree, together
> Alessandro's patch?
> 
> Wolfgang, what's your position?

Actually I'm waiting for a new patch that integrates that fix.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Just about every computer on the market today runs Unix,  except  the
Mac (and nobody cares about it).                   - Bill Joy 6/21/85

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[rhabarber1848]

Wolfgang Denk wrote:

> Dear Giuseppe CONDORELLI,
> 
> In message <001601ca1012$e0f72730$c08182a4@st.com> you wrote:
>> Very good!!!
>> So is it time to re-apply zlib patch to u-boot main tree, together
>> Alessandro's patch?
>> 
>> Wolfgang, what's your position?
> 
> Actually I'm waiting for a new patch that integrates that fix.

Hi,

please do not commit the zlib-1.2.3 patch in its current state, although
Alessandro's patch fixes on bug there is still the problem that the
WATCHDOG_RESET() calls from gunzip.c

#if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
        s.outcb = (cb_func)WATCHDOG_RESET;
#else
        s.outcb = Z_NULL;
#endif<>/* CONFIG_HW_WATCHDOG */

are silently ignored in the new zlib code. My C knowledge is insufficient to
solve this problem. In its current state it will break zlib decompression
on the machines I am taking care of.

Cheers, rhabarber1848

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Wolfgang Denk]

Dear Giuseppe CONDORELLI,

In message <001701ca1015$dbf7fae0$c08182a4@st.com> you wrote:
> 
> > Actually I'm waiting for a new patch that integrates that fix.
> Maybe I've not understood.
> Do you mean you are waiting a new zlib patch [v4] that includes Alessandro's
> patch, to apply
> only one patch containing all?

Yes. It makes no sense to add a known to be broken patch and fix it in
a second commit, as this leaves only a state in the tree where
attempts to bisect any issues run into problems. So please submit a
new, fixed patch, that really works.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Applying computer technology is simply finding the  right  wrench  to
pound in the correct screw.

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Wolfgang Denk]

Dear rhabarber1848,

In message <h4orm0$l1d$1@ger.gmane.org> you wrote:
> 
> > Actually I'm waiting for a new patch that integrates that fix.
> 
> please do not commit the zlib-1.2.3 patch in its current state, although
> Alessandro's patch fixes on bug there is still the problem that the
> WATCHDOG_RESET() calls from gunzip.c
> 
> #if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
>         s.outcb = (cb_func)WATCHDOG_RESET;
> #else
>         s.outcb = Z_NULL;
> #endif<>/* CONFIG_HW_WATCHDOG */
> 
> are silently ignored in the new zlib code. My C knowledge is insufficient to
> solve this problem. In its current state it will break zlib decompression
> on the machines I am taking care of.

Agreed - that should be working, too, before we pull that patch into
mainline.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Perfection is reached, not when there is no longer anything  to  add,
but when there is no longer anything to take away.
                                           - Antoine de Saint-Exupery

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Giuseppe CONDORELLI]

Hi rhabarber1848,

please could you test latest two zlib patches I have sent few minutes ago?

NOTE: the first one is waiting for approval due to its big size.

Thanks, I hope these patches will solve your issue.

G.


-----Original Message-----
From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces at lists.denx.de] On
Behalf Of Wolfgang Denk
Sent: Wednesday, July 29, 2009 9:12 AM
To: rhabarber1848 at web.de
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH] zlib: allow 0 as destination pointer

Dear rhabarber1848,

In message <h4orm0$l1d$1@ger.gmane.org> you wrote:
> 
> > Actually I'm waiting for a new patch that integrates that fix.
> 
> please do not commit the zlib-1.2.3 patch in its current state, although
> Alessandro's patch fixes on bug there is still the problem that the
> WATCHDOG_RESET() calls from gunzip.c
> 
> #if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
>         s.outcb = (cb_func)WATCHDOG_RESET;
> #else
>         s.outcb = Z_NULL;
> #endif<>/* CONFIG_HW_WATCHDOG */
> 
> are silently ignored in the new zlib code. My C knowledge is insufficient
to
> solve this problem. In its current state it will break zlib decompression
> on the machines I am taking care of.

Agreed - that should be working, too, before we pull that patch into
mainline.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Perfection is reached, not when there is no longer anything  to  add,
but when there is no longer anything to take away.
                                           - Antoine de Saint-Exupery
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Alessandro Rubini]

> Hi rhabarber1848,
> please could you test latest two zlib patches I have sent few minutes ago?

Unfortunately you add outcb() only in inflatestart and inflateend. IT should
also go in the main loop.

Being an out-callback with arguments, it should be called during copy to
output, with proper arguments (and in that form it would be a fix to
the official zlib).

I tried this one, which is a quick hack (no arguments passed, so not
suitable for upstream zlib in any case).  Moreover, one of those two
should be sufficient, but I don't know which one exactly. I'm lazy so I
won't trace program flow in detail.

I think rhabarber should test with this addition too and select which one
is the good one.

(formatted for git-am for quick testing, even thought he talk is irrelevant
as a commit message).

ps: please note that your addition of outcb() has white-space inconsistency with
zlib.c (which is not u-boot style while those lines are).

/alessandro

---
 lib_generic/zlib.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/lib_generic/zlib.c b/lib_generic/zlib.c
index 6a78dc9..66b18eb 100644
--- a/lib_generic/zlib.c
+++ b/lib_generic/zlib.c
@@ -1129,6 +1129,10 @@ unsigned out;
 
     state = (struct inflate_state FAR *)strm->state;
 
+    /* call watchdog_reset if needed (addition for U-Boot) */
+    if (strm->outcb != Z_NULL)
+        (*strm->outcb)(Z_NULL, 0);
+
     /* if it hasn't been done already, allocate space for the window */
     if (state->window == Z_NULL) {
         state->window = (unsigned char FAR *)
@@ -1741,6 +1745,8 @@ int flush;
             Tracev((stderr, "inflate:       codes ok\n"));
             state->mode = LEN;
         case LEN:
+            if (strm->outcb != Z_NULL) /* for watchdog (U-Boot) */
+                (*strm->outcb)(Z_NULL, 0);
             if (have >= 6 && left >= 258) {
                 RESTORE();
                 inflate_fast(strm, out);
-- 
1.6.0.2

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[rhabarber1848]

Hi,

Alessandro Rubini wrote:
> Giuseppe Condorelli wrote:
>> Hi rhabarber1848,
>> please could you test latest two zlib patches I have sent few minutes
>> ago?
> 
> Unfortunately you add outcb() only in inflatestart and inflateend. IT
> should also go in the main loop.

you are right. The two patches sent by Giuseppe do not solve the problem.

> I think rhabarber should test with this addition too and select which one
> is the good one.

Let's see:

> --- a/lib_generic/zlib.c
> +++ b/lib_generic/zlib.c
> @@ -1129,6 +1129,10 @@ unsigned out;
>  
>      state = (struct inflate_state FAR *)strm->state;
>  
> +    /* call watchdog_reset if needed (addition for U-Boot) */
> +    if (strm->outcb != Z_NULL)
> +        (*strm->outcb)(Z_NULL, 0);
> +
>      /* if it hasn't been done already, allocate space for the window */
>      if (state->window == Z_NULL) {
>          state->window = (unsigned char FAR *)

This patch does not fix the problem.

> @@ -1741,6 +1745,8 @@ int flush;
>              Tracev((stderr, "inflate:       codes ok\n"));
>              state->mode = LEN;
>          case LEN:
> +            if (strm->outcb != Z_NULL) /* for watchdog (U-Boot) */
> +                (*strm->outcb)(Z_NULL, 0);
>              if (have >= 6 && left >= 258) {
>                  RESTORE();
>                  inflate_fast(strm, out);

This patch alone seems to solve the problem, only with it U-Boot could
decompress a Linux image, proven with 10 successful boot attempts. All the
other patches regarding WATCHDOG_RESET() can be ignored.

Tested with U-Boot 2008.09-rc1, the original zlib123-patch from Giuseppe +
your "allow 0 as destination pointer"-patch and the last patch from the
mail quoted above.

Cheers, rhabarber1848

[U-Boot] [PATCH] zlib: allow 0 as destination pointer

[Giuseppe CONDORELLI]

Hi rhbarber1848,

I've merged patches into one only and resent it to list (v5).
Please verify once more the issue is solved, to close it definitively.

Cheers and thanks,
Giuseppe

-----Original Message-----
From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces at lists.denx.de] On
Behalf Of rhabarber1848
Sent: Wednesday, July 29, 2009 12:33 PM
To: u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH] zlib: allow 0 as destination pointer

Hi,

Alessandro Rubini wrote:
> Giuseppe Condorelli wrote:
>> Hi rhabarber1848,
>> please could you test latest two zlib patches I have sent few minutes
>> ago?
> 
> Unfortunately you add outcb() only in inflatestart and inflateend. IT
> should also go in the main loop.

you are right. The two patches sent by Giuseppe do not solve the problem.

> I think rhabarber should test with this addition too and select which one
> is the good one.

Let's see:

> --- a/lib_generic/zlib.c
> +++ b/lib_generic/zlib.c
> @@ -1129,6 +1129,10 @@ unsigned out;
>  
>      state = (struct inflate_state FAR *)strm->state;
>  
> +    /* call watchdog_reset if needed (addition for U-Boot) */
> +    if (strm->outcb != Z_NULL)
> +        (*strm->outcb)(Z_NULL, 0);
> +
>      /* if it hasn't been done already, allocate space for the window */
>      if (state->window == Z_NULL) {
>          state->window = (unsigned char FAR *)

This patch does not fix the problem.

> @@ -1741,6 +1745,8 @@ int flush;
>              Tracev((stderr, "inflate:       codes ok\n"));
>              state->mode = LEN;
>          case LEN:
> +            if (strm->outcb != Z_NULL) /* for watchdog (U-Boot) */
> +                (*strm->outcb)(Z_NULL, 0);
>              if (have >= 6 && left >= 258) {
>                  RESTORE();
>                  inflate_fast(strm, out);

This patch alone seems to solve the problem, only with it U-Boot could
decompress a Linux image, proven with 10 successful boot attempts. All the
other patches regarding WATCHDOG_RESET() can be ignored.

Tested with U-Boot 2008.09-rc1, the original zlib123-patch from Giuseppe +
your "allow 0 as destination pointer"-patch and the last patch from the
mail quoted above.

Cheers, rhabarber1848


_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot