headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
Grant Grundler | 5 Jun 09:58

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

On Sat, Jun 04, 2005 at 06:59:53PM -0400, Alan Stern wrote:
> The point I was driving at is that we currently have separate APIs for 
> byte swapping and for unaligned access, and it would make a lot of sense 
> to combine them into a single API.  Knowing that the bytes have to be 
> swapped _and_ that the value isn't aligned correctly should allow us to 
> use more efficient code (in some cases) than a simple unaligned access 
> followed by a byte swap.

Ah ok. I missed that, sorry. *Some* architectures might be able
to provide more efficient implementations if they were combined.
But I don't think parisc is one of those. If I'm lucky, someone
will prove me wrong. :^)
Since many of the arches now use asm-generic/unaligned.h, I
can't say which other ones would benefit.

Just avoiding the trap is already a *huge* improvement. I'm alot
less worried about shaving a few cycles of here and there unless
it's really a critical code path (e.g. TLB miss handler or DMA
mapping support).

> I also don't understand your point about architectures that trap unaligned 
> accesses.

Architectures that trap unaligned accesses, *must* handle them.
If they don't, then networking won't work either.
I believe avoiding the trap is a good thing.

>   Let's put aside for one moment the question of whether it's 
> better to manually load the pieces of an unaligned value versus incurring 
> the overhead of a trap and a kernel fixup -- the arch-specific code is
(Continue reading)

Permalink | Reply |
headers
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)

Permalink | Reply |
headers
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)

Permalink | Reply |

Gmane