Development of Linux for the HP PA-RISC platform

Alan Stern | 4 Jun 17:06

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)

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)

headers

Alan Stern | 5 Jun 00:59

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)

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)