[PATCH] lzo: fix possible typo in decompresor

[PATCH] lzo: fix possible typo in decompresor

[Harvey Harrison]

Shift of a le value seems strange, probably meant to shift the cpu-order
variable as in the prvious section of the switch statement.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 lib/lzo/lzo1x_decompress.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/lzo/lzo1x_decompress.c b/lib/lzo/lzo1x_decompress.c
index 9dc7056..77f0f9b 100644
--- a/lib/lzo/lzo1x_decompress.c
+++ b/lib/lzo/lzo1x_decompress.c
@@ -158,7 +158,7 @@ match:
 					t += 7 + *ip++;
 				}
 				m_pos -= le16_to_cpu(get_unaligned(
-					(const unsigned short *)ip) >> 2);
+					(const unsigned short *)ip)) >> 2;
 				ip += 2;
 				if (m_pos == op)
 					goto eof_found;
-- 
1.5.5.144.g3e42

Re: [PATCH] lzo: fix possible typo in decompresor

[Linus Torvalds]

On Thu, 10 Apr 2008, Harvey Harrison wrote:
>
> Shift of a le value seems strange, probably meant to shift the cpu-order
> variable as in the prvious section of the switch statement.

Hmm. This patch looks ObviouslyCorrect(tm), but it worries me that 
apparently the old broken code has been around since last July, and afaik 
it can never have worked on big-endian machines.

So did nobody ever use it, or why hasn't this ever triggered? How did you 
find this? A sparse warning?

		Linus

Re: [PATCH] lzo: fix possible typo in decompresor

[Harvey Harrison]

On Thu, 2008-04-10 at 13:49 -0700, Linus Torvalds wrote:
> 
> On Thu, 10 Apr 2008, Harvey Harrison wrote:
> >
> > Shift of a le value seems strange, probably meant to shift the cpu-order
> > variable as in the prvious section of the switch statement.
> 
> Hmm. This patch looks ObviouslyCorrect(tm), but it worries me that 
> apparently the old broken code has been around since last July, and afaik 
> it can never have worked on big-endian machines.
> 
> So did nobody ever use it, or why hasn't this ever triggered? How did you 
> find this? A sparse warning?
> 
> 		Linus

When converting in tree users to my proposed unaligned byteswapping api.
See the 8 patch series posted today to linux-arch.

Harvey

Re: [PATCH] lzo: fix possible typo in decompresor

[Richard Purdie]

On Thu, 2008-04-10 at 13:49 -0700, Linus Torvalds wrote:
> On Thu, 10 Apr 2008, Harvey Harrison wrote:
> >
> > Shift of a le value seems strange, probably meant to shift the cpu-order
> > variable as in the prvious section of the switch statement.
> 
> Hmm. This patch looks ObviouslyCorrect(tm), but it worries me that 
> apparently the old broken code has been around since last July, and afaik 
> it can never have worked on big-endian machines.
> 
> So did nobody ever use it, or why hasn't this ever triggered? How did you 
> find this? A sparse warning?

The heaviest users of the lzo code I know of are little-endian ARM
devices through jffs2. When the code was merged there was a lot of
discussion about the best way to handle the endian issues and unaligned
accesses and whilst I seem to remember someone posting big-endian test
results it could have been before some of the later changes were made.

So yes its possible its not been run on BE until now or that isn't a
common code path. I've checked this against the external LZO library its
based on and the patch is correct

Acked-by: Richard Purdie <rpurdie@rpsys.net>

-- 
Richard

Re: [PATCH] lzo: fix possible typo in decompresor

[Harvey Harrison]

On Thu, 2008-04-10 at 23:18 +0100, Richard Purdie wrote:
> On Thu, 2008-04-10 at 13:49 -0700, Linus Torvalds wrote:
> > On Thu, 10 Apr 2008, Harvey Harrison wrote:
> > >
> > > Shift of a le value seems strange, probably meant to shift the cpu-order
> > > variable as in the prvious section of the switch statement.
> > 
> > Hmm. This patch looks ObviouslyCorrect(tm), but it worries me that 
> > apparently the old broken code has been around since last July, and afaik 
> > it can never have worked on big-endian machines.
> > 
> > So did nobody ever use it, or why hasn't this ever triggered? How did you 
> > find this? A sparse warning?
> 
> The heaviest users of the lzo code I know of are little-endian ARM
> devices through jffs2. When the code was merged there was a lot of
> discussion about the best way to handle the endian issues and unaligned
> accesses and whilst I seem to remember someone posting big-endian test
> results it could have been before some of the later changes were made.
> 
> So yes its possible its not been run on BE until now or that isn't a
> common code path. I've checked this against the external LZO library its
> based on and the patch is correct
> 
> Acked-by: Richard Purdie <rpurdie@rpsys.net>
> 

This is one of the reasons I thought about for adding the new api, the
bracketing is just too easy to get wrong when you throw
get/put_unaligned into the mix.

Harvey