Grant Grundler | 2 Jun 23:35

[PATCH] usb/input/hid-core.c extract() brain damage

Hi folks,,
extract() and implement() are bit field manipulation routines.
They mangle "n" bits starting at "offset" index into the bit field.
Since this is USB, the byte array is little endian.
Big endian machines (e.g. parisc, mips) should only need to
byte swap the value.

extract() and implement() have brain damaged attempts to handle
32-bit wide "fields".
The problem is the index math in the original code didn't clear all
the relevant bits.  (offset >> 5) only compensated for 32-bit index.
We need (offset >> 6) if we want to use 64-bit loads.

But it was also wrong in that it tried to use quasi-aligned loads.
Ie "report" was only incremented in multiples of 4 bytes and then
the offset was masked off for values greater than 4 bytes.
The right way is to pretend "report" points at a byte array.
And offset is then only minor adjustment for < 8 bits of offset.
"n" (field width) can then be as big as 24 (assuming 32-bit loads)
since "offset" will never be bigger than 7.

If someone needs either function to handle more than 24-bits,
please document why - point at a specification or specific USB
hid device - in comments in the code.

extract/implement() are also an eyesore to read.
Please banish whoever wrote it to read CodingStyle 3 times in a row
to a classroom full of 1st graders armed with rubberbands.
Or just flame them. Whatever. Globbing all the code together
on two lines does NOT make it faster and is Just Wrong.
(Continue reading)

John David Anglin | 4 Jun 00:41
Picon

Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage

> extract() and implement() have brain damaged attempts to handle
> 32-bit wide "fields".

It seems that this problem has been around for some time.  Does
it fix the usb keyboard and mouse problems in 2.6.12?

Dave
--

-- 
J. David Anglin                                  dave.anglin <at> nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

-------------------------------------------------------
This SF.Net email is sponsored by: NEC IT Guy Games.  How far can you shotput
a projector? How fast can you ride your desk chair down the office luge track?
If you want to score the big prize, get to know the little guy.  
Play to win an NEC 61" plasma display: http://www.necitguy.com/?r=20
_______________________________________________
linux-usb-devel <at> lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Grant Grundler | 4 Jun 03:16

Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage

On Fri, Jun 03, 2005 at 06:41:55PM -0400, John David Anglin wrote:
> > extract() and implement() have brain damaged attempts to handle
> > 32-bit wide "fields".
> 
> It seems that this problem has been around for some time.

I didn't check to see how long this code has been around.

> Does it fix the usb keyboard and mouse problems in 2.6.12?

USB keyboard and mouse work with 2.6.12-rc5 on parisc.

grant

-------------------------------------------------------
This SF.Net email is sponsored by: NEC IT Guy Games.  How far can you shotput
a projector? How fast can you ride your desk chair down the office luge track?
If you want to score the big prize, get to know the little guy.  
Play to win an NEC 61" plasma display: http://www.necitguy.com/?r=20
_______________________________________________
linux-usb-devel <at> lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

John David Anglin | 4 Jun 03:25
Picon

Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage

> > It seems that this problem has been around for some time.
> 
> I didn't check to see how long this code has been around.

It's in 2.6.10-pa3.  Since the USB mouse and keyboard work in it,
some other change must have been involved in the 2.6.12 breakage.

As an aside, 2.6.10-pa3 doesn't seem to have the stability problems
that I saw 2.6.11-pa4.  It pretty much seems as stable as 2.6.8.1-pa11
on my c3750.  There was hang in the java testsuite (PR218) in one of
two gcc builds and check.  Otherwise, the test results were identical.

> > Does it fix the usb keyboard and mouse problems in 2.6.12?
> 
> USB keyboard and mouse work with 2.6.12-rc5 on parisc.

Great!

Dave
--

-- 
J. David Anglin                                  dave.anglin <at> nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

-------------------------------------------------------
This SF.Net email is sponsored by: NEC IT Guy Games.  How far can you shotput
a projector? How fast can you ride your desk chair down the office luge track?
If you want to score the big prize, get to know the little guy.  
Play to win an NEC 61" plasma display: http://www.necitguy.com/?r=20
_______________________________________________
linux-usb-devel <at> lists.sourceforge.net
(Continue reading)

Grant Grundler | 4 Jun 09:25

Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage

On Fri, Jun 03, 2005 at 09:25:18PM -0400, John David Anglin wrote:
> > > It seems that this problem has been around for some time.
> > 
> > I didn't check to see how long this code has been around.
> 
> It's in 2.6.10-pa3.  Since the USB mouse and keyboard work in it,
> some other change must have been involved in the 2.6.12 breakage.

Yes, Kyle tracked it down to parisc switching from a parisc asm
definition of get_unaligned() to using the generic ones.
Ie we moved from avoiding kernel traps to exercising them.
But 64-bit kernel worked fine with USB.
And on 32-bit kernels le64() loads degenerate into two 32-bit
loads. Ie the trap handler is still only dealing with 32-bit
misaligned accesses like it would with this patch. So I don't
think the kernel trap support is the problem.

Regardless, the two functions are badly written and could
be alot clearer.

> As an aside, 2.6.10-pa3 doesn't seem to have the stability problems
> that I saw 2.6.11-pa4.  It pretty much seems as stable as 2.6.8.1-pa11
> on my c3750.  There was hang in the java testsuite (PR218) in one of
> two gcc builds and check.  Otherwise, the test results were identical.

Ok. Can you want to try the older version of include/asm-parisc/unaligned.h?

> > USB keyboard and mouse work with 2.6.12-rc5 on parisc.
> 
> Great!
(Continue reading)

Joel Soete | 4 Jun 12:31
Picon

Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage


Grant Grundler wrote:
> On Fri, Jun 03, 2005 at 09:25:18PM -0400, John David Anglin wrote:
> 
>>>>It seems that this problem has been around for some time.
>>>
>>>I didn't check to see how long this code has been around.
>>
>>It's in 2.6.10-pa3.  Since the USB mouse and keyboard work in it,
>>some other change must have been involved in the 2.6.12 breakage.
> 
> 
> Yes, Kyle tracked it down to parisc switching from a parisc asm
> definition of get_unaligned() to using the generic ones.
> Ie we moved from avoiding kernel traps to exercising them.
> But 64-bit kernel worked fine with USB.
> And on 32-bit kernels le64() loads degenerate into two 32-bit
> loads. Ie the trap handler is still only dealing with 32-bit
> misaligned accesses like it would with this patch. So I don't
> think the kernel trap support is the problem.
> 
> Regardless, the two functions are badly written and could
> be alot clearer.
> 
Thanks for feedback ;-)

That said, I also noticed a big difference of behaviour when I select or not the CONFIG_PDC_STABLE option or not:
	o when selected my stress test make panicing my b2k in 5 min showing a cash_grow() pb (as reported before)
	o when not selected the system hang as decribe by jda.

(Continue reading)

Joel Soete | 4 Jun 15:59
Picon

Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage

[...]
>>
>> Ok. Can you want to try the older version of 
>> include/asm-parisc/unaligned.h?
>>
> Interesting, I will too
> 
Sorry it didn't help for me (on a b180 with kernel 2.6.12-rc5-pa2 and 2.6.8/include/asm-parisc/unaligned.h)
still panicing as usual:
  [<101511b0>] cache_grow+0xd8/0x1a8
  [<10151428>] cache_alloc_refill+0x1a8/0x264
  [<10151744>] kmem_cache_alloc+0x48/0x4c
  [<101b590c>] ext3_alloc_inode+0x18/0x40
  [<10186ae8>] alloc_inode+0x28/0x1a0
  [<1018772c>] new_inode+0x10/0x8c
  [<101aa718>] ext3_new_inode+0xc4/0x73c
  [<101b36b8>] ext3_create+0x8c/0x12c
  [<1017b484>] vfs_create+0xe0/0x140
  [<1017bd54>] open_namei+0x680/0x734
  [<101694bc>] filp_open+0x3c/0x70
  [<101699ac>] sys_open+0x70/0xb8
  [<1010d12c>] syscall_exit+0x0/0x14

Kernel Fault: Code=15 regs=15154600 (Addr=27c944cc)

      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
PSW: 00000000000001001110011100001111 Not tainted
r00-03  00000000 10513e58 101511b0 105004f8
r04-07  17794000 100df6e0 00000010 00000050
r08-11  00000001 15f172bc 17b050dc 00000000
(Continue reading)

Alan Stern | 4 Jun 17:06
Picon
Favicon

Re: Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage

On Sat, 4 Jun 2005, Grant Grundler wrote:

> Yes, Kyle tracked it down to parisc switching from a parisc asm
> definition of get_unaligned() to using the generic ones.
> Ie we moved from avoiding kernel traps to exercising them.
> But 64-bit kernel worked fine with USB.
> And on 32-bit kernels le64() loads degenerate into two 32-bit
> loads. Ie the trap handler is still only dealing with 32-bit
> misaligned accesses like it would with this patch. So I don't
> think the kernel trap support is the problem.
> 
> Regardless, the two functions are badly written and could
> be alot clearer.

This is a naive question from someone who doesn't know much about the 
various implementations of get_unaligned() and put_unaligned().

In spite of the total overall number of changes required, wouldn't it be 
much simpler to have a suite of routines (inlines or macros) like:

	get_16, put_16, get_le16, put_le16, get_be16, put_be16
	get_32, put_32, get_le32, put_le32, get_be32, put_be32
	get_64, put_64, get_le64, put_le64, get_be64, put_be64

	in short, {get|put}_{|le|be}{16|32|64}

for native, little-endian, and big-endian unaligned accesses?  The generic 
definitions are extremely simple and architecture-specific headers could 
easily provide optimized versions.

(Continue reading)

Grant Grundler | 4 Jun 19:43

Re: Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage

On Sat, Jun 04, 2005 at 11:06:52AM -0400, Alan Stern wrote:
> In spite of the total overall number of changes required, wouldn't it be 
> much simpler to have a suite of routines (inlines or macros) like:
> 
> 	get_16, put_16, get_le16, put_le16, get_be16, put_be16
> 	get_32, put_32, get_le32, put_le32, get_be32, put_be32
> 	get_64, put_64, get_le64, put_le64, get_be64, put_be64
> 
> 	in short, {get|put}_{|le|be}{16|32|64}

Sorry, no. The architectures that trap on misaligned accesses have to
handle that in the kernel. And even though the implementations
get simpler, a plethoria of interfaces just clutters up the general
device driver API. For that reason alone I wouldn't want it.

The endian conversion (swap) macros are PITA already.
I'll argue the swap API should be simplified to four macros:
	cpu_to_le(x), cpu_to_be(x), le_to_cpu(x), be_to_cpu(x)

and force the caller to cast to the right size. switch(sizeof(x)) would
then select the right arch specific variant. I'll figure out how to
pitch this to linux-arch...and then see how far it gets batted back. :^)

> for native, little-endian, and big-endian unaligned accesses?  The generic 
> definitions are extremely simple and architecture-specific headers could 
> easily provide optimized versions.
> 
> I realize this doesn't fit in very well with the current state of the API, 
> but IMHO it would be a big improvement.

(Continue reading)

Alan Stern | 5 Jun 00:59
Picon
Favicon

Re: Re: [parisc-linux] [PATCH] usb/input/hid-core.c extract() brain damage

On Sat, 4 Jun 2005, Grant Grundler wrote:

> On Sat, Jun 04, 2005 at 11:06:52AM -0400, Alan Stern wrote:
> > In spite of the total overall number of changes required, wouldn't it be 
> > much simpler to have a suite of routines (inlines or macros) like:
> > 
> > 	get_16, put_16, get_le16, put_le16, get_be16, put_be16
> > 	get_32, put_32, get_le32, put_le32, get_be32, put_be32
> > 	get_64, put_64, get_le64, put_le64, get_be64, put_be64
> > 
> > 	in short, {get|put}_{|le|be}{16|32|64}
> 
> Sorry, no. The architectures that trap on misaligned accesses have to
> handle that in the kernel. And even though the implementations
> get simpler, a plethoria of interfaces just clutters up the general
> device driver API. For that reason alone I wouldn't want it.
> 
> The endian conversion (swap) macros are PITA already.
> I'll argue the swap API should be simplified to four macros:
> 	cpu_to_le(x), cpu_to_be(x), le_to_cpu(x), be_to_cpu(x)
> 
> and force the caller to cast to the right size. switch(sizeof(x)) would
> then select the right arch specific variant. I'll figure out how to
> pitch this to linux-arch...and then see how far it gets batted back. :^)

I don't really follow your argument.  For example, consider the swap API.  
There's nothing to stop you from defining, in your own code, those four 
macros.  Have them perform the appropriate conversion based on the type of 
x.  You don't have to use the full existing API if you don't want to.

(Continue reading)


Gmane