The generic version of csum_ipv6_magic has the len argument declared as
__u16, while most arch dependent version declare it as __u32. After
looking at the call site of this function, I come up to a conclusion
that __u32 is a better match with the actual usage.
Hence, patch to change argument type for greater consistency.
Signed-off-by: Ken Chen <[email protected]>
---
asm-m32r and asm-parisc both have it declared as __u16. I think it is a
copy-n-paste error and needs to be fixed by arch maintainer, I hope.
--- ./include/net/ip6_checksum.h.orig 2006-11-08 18:49:50.000000000 -0800
+++ ./include/net/ip6_checksum.h 2006-11-08 18:50:04.000000000 -0800
@@ -36,7 +36,7 @@
static __inline__ unsigned short int csum_ipv6_magic(struct in6_addr *saddr,
struct in6_addr *daddr,
- __u16 len,
+ __u32 len,
unsigned short proto,
unsigned int csum)
{
From: "Chen, Kenneth W" <[email protected]>
Date: Wed, 8 Nov 2006 18:02:06 -0800
> The generic version of csum_ipv6_magic has the len argument declared as
> __u16, while most arch dependent version declare it as __u32. After
> looking at the call site of this function, I come up to a conclusion
> that __u32 is a better match with the actual usage.
>
> Hence, patch to change argument type for greater consistency.
>
> Signed-off-by: Ken Chen <[email protected]>
Architecture implementations such as the ones for m32r and parisc have
the same problem, so "for consistency" please fix them up as well.
Thanks a lot.
On Wed, Nov 08, 2006 at 11:00:59PM -0800, David Miller wrote:
> From: "Chen, Kenneth W" <[email protected]>
> Date: Wed, 8 Nov 2006 18:02:06 -0800
>
> > The generic version of csum_ipv6_magic has the len argument declared as
> > __u16, while most arch dependent version declare it as __u32. After
> > looking at the call site of this function, I come up to a conclusion
> > that __u32 is a better match with the actual usage.
> >
> > Hence, patch to change argument type for greater consistency.
> >
> > Signed-off-by: Ken Chen <[email protected]>
>
> Architecture implementations such as the ones for m32r and parisc have
> the same problem, so "for consistency" please fix them up as well.
>
> Thanks a lot.
Please, hold. One of the patches in my queue gets sanitized prototypes
for all that stuff and it'll conflict like crazy.
I haven't touch that argument yet; if there's an agreement as to what should
we switch to, I'll do that. So... does everyone agree that u32 is the way
to go?
From: Al Viro <[email protected]>
Date: Thu, 9 Nov 2006 07:22:16 +0000
> I haven't touch that argument yet; if there's an agreement as to what should
> we switch to, I'll do that. So... does everyone agree that u32 is the way
> to go?
I definitely do.
On Wed, Nov 08, 2006 at 11:55:48PM -0800, David Miller wrote:
> From: Al Viro <[email protected]>
> Date: Thu, 9 Nov 2006 07:22:16 +0000
>
> > I haven't touch that argument yet; if there's an agreement as to what should
> > we switch to, I'll do that. So... does everyone agree that u32 is the way
> > to go?
>
> I definitely do.
OK. While we are at it, let's really sanitize the prototypes for that zoo.
General notes:
* I've added two types for checksums (__sum16 == checksum, __wsum == wide
unfolded checksum). Those are fairly obvious.
* We have a bloody mess of what we do with pointers to data being checksumed -
char *, unsigned char *, __user and const used inconsistently. I'm converting
them all to pointers to (qualified) void, with const and __user where needed.
* struct in6_addr * in csum_ipv6_magic() should be pointer to const.
* IPv4 addresses should be passed as __be32.
* length in csum_ipv6_magic() should be u32.
After doing the above we have the following:
Consistent on all platforms:
__sum16 ip_fast_csum(const void *, unsigned int);
__sum16 ip_compute_csum(const void *, int);
__sum16 csum_fold(__wsum);
__wsum csum_partial_copy_from_user(const void __user *, void *, int, __wsum, int *);
__wsum csum_partial_copy_nocheck(const void *, void *, int, __wsum);
__wsum csum_and_copy_to_user(const void *, void *, int, __wsum, int *);
__sum16 csum_ipv6_magic(struct in6_addr *, struct in6_addr *, __u32, unsigned short, __wsum);
Platform-dependent:
__wsum csum_partial(const void *, T, __wsum);
On amd64 and uml/amd64 we have T = unsigned int, everywhere else it's int.
__wsum csum_tcpudp_nofold(__be32, __be32, T1, T2, __wsum);
On arm/arm26: T1 = unsigned short, T2 = unsigned int.
On sparc/sparc64: T1 = unsigned int, T2 = unsigned short.
Everywhere else: T1 = T2 = unsigned short.
__sum16 csum_tcpudp_magic(__be32, __be32, unsigned short, T, __wsum);
On arm/arm26 T is unsigned int, elsewhere it's unsigned short.
The first question is in the types we are using for length. OK,
csum_tcpudp_...() is a special case; there we want u16 and unless
there's a reason _not_ to do so on sparc, I'd rather convert it
to the same thing.
Another special case is ip_fast_csum() (length in 32bit words, never more
than 15, actually; existing code happily breaks for (never passed) large
values).
The rest is a mess and should clearly be the same type. Do we want it
to be signed? Several architectures do have checks for the bit 31 being
set and demonstrate bloody odd behaviour in such cases.
Incidentally, WTF is
define SK_CS_CALCULATE_CHECKSUM
#ifndef CONFIG_X86_64
#define SkCsCalculateChecksum(p,l) ((~ip_compute_csum(p, l)) & 0xffff)
#else
#define SkCsCalculateChecksum(p,l) ((~ip_fast_csum(p, l)) & 0xffff)
#endif
in ./drivers/net/sk98lin/h/skdrv1st.h? I don't see any callers of that
beast, anyway, but I'd really love to know who'd written it in the
first place; the meaning of the second argument is different in those
two...
The next question: csum_tcpudp_...() 'proto' argument. What do we want
there? u8? u16? u32? It always fits in u8 and many architectures
rely on it never exceeding 255. It's always constant, BTW...
csum_partial_copy_fromuser(): Can die, only 3 targets have its rudiment
and nothing in the tree uses it. ACK?
csum_partial_copy(). Rare alias for csum_partial_copy_nocheck(). Can die;
all instances simply should be renamed to csum_partial_copy_nocheck. ACK?
On Mon, Nov 13, 2006 at 08:52:23AM +0000, Al Viro wrote:
> After doing the above we have the following:
>
> Platform-dependent:
> __wsum csum_tcpudp_nofold(__be32, __be32, T1, T2, __wsum);
> On arm/arm26: T1 = unsigned short, T2 = unsigned int.
> __sum16 csum_tcpudp_magic(__be32, __be32, unsigned short, T, __wsum);
> On arm/arm26 T is unsigned int, elsewhere it's unsigned short.
Could both become unsigned short or unsigned int. Would prefer
unsigned int on ARM, since otherwise the compiler generate code to
truncate any variable "int"s to an unsigned short.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
On Mon, Nov 13, 2006 at 09:12:22AM +0000, Russell King wrote:
> On Mon, Nov 13, 2006 at 08:52:23AM +0000, Al Viro wrote:
> > After doing the above we have the following:
> >
> > Platform-dependent:
> > __wsum csum_tcpudp_nofold(__be32, __be32, T1, T2, __wsum);
> > On arm/arm26: T1 = unsigned short, T2 = unsigned int.
> > __sum16 csum_tcpudp_magic(__be32, __be32, unsigned short, T, __wsum);
> > On arm/arm26 T is unsigned int, elsewhere it's unsigned short.
>
> Could both become unsigned short or unsigned int. Would prefer
> unsigned int on ARM, since otherwise the compiler generate code to
> truncate any variable "int"s to an unsigned short.
Frankly, as for the generated code... I'd expect s/htons(len)/len * 256/
and the same for proto in there to have much more effect. And it does give
the same checksum...
Al Viro <[email protected]> writes:
>
> Incidentally, WTF is
> define SK_CS_CALCULATE_CHECKSUM
> #ifndef CONFIG_X86_64
> #define SkCsCalculateChecksum(p,l) ((~ip_compute_csum(p, l)) & 0xffff)
> #else
> #define SkCsCalculateChecksum(p,l) ((~ip_fast_csum(p, l)) & 0xffff)
> #endif
> in ./drivers/net/sk98lin/h/skdrv1st.h?
Looks totally bogus. The x86-64 ip_fast_csum is practically identical
to the i386 version. Someone must have been asleep while reviewing
that code.
But AFAIK sk98lin is scheduled for deletion anyways, to be replaced by
sky*. Probably for good reasons. Hopefully soon.
-Andi
From: Al Viro <[email protected]>
Date: Mon, 13 Nov 2006 08:52:23 +0000
> The first question is in the types we are using for length. OK,
> csum_tcpudp_...() is a special case; there we want u16 and unless
> there's a reason _not_ to do so on sparc, I'd rather convert it
> to the same thing.
That's fine.
> csum_partial_copy_fromuser(): Can die, only 3 targets have its rudiment
> and nothing in the tree uses it. ACK?
ACK.
> csum_partial_copy(). Rare alias for csum_partial_copy_nocheck(). Can die;
> all instances simply should be renamed to csum_partial_copy_nocheck. ACK?
ACK.