Messages in this thread | | | Subject | Re: [PATCH] mISDN cleanup user interface | From | David Woodhouse <> | Date | Wed, 30 Jul 2008 10:13:31 +0100 |
| |
On Tue, 2008-07-29 at 23:02 +0200, Marcel Holtmann wrote: > Hi Karsten, > > > > Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master > > > > > > The channelmap should have the same size on 32 and 64 bit > > > systems. Thanks to David Woodhouse for spotting this. > > > > > > Signed-off-by: Karsten Keil <kkeil@suse.de> > > > > Er, isn't that still broken? Consider 32-bit userspace on 64-bit kernel, > > big-endian. > > I agree with David here. Please lets not do these horribly things again. > Almost everybody has done their fair share of brokenness with the compat > layers. Especially 32-bit userspace on 64-bit kernels. Using __u32 and > alike is a way better choice and less headache.
Well, he's already switched to a 32-bit type, which is fine in that respect -- at least the struct is the same _size_ on 32-bit and 64-bit targets now. (OK, so inventing new types like 'u_int' instead of just using the ones that the C language provides is a bit silly, but that's the Linux norm so I wasn't complaining about that.)
But the channelmap field is still broken because it's using set_bit() on int types, and they're defined to work on unsigned long. So in the fairly common case where a BRI driver sets bits 1 and 2 (sic) in the channel map, that will look like this on a 64-bit big-endian kernel:
00 00 00 00 00 00 00 06 00 00 00 00 00 00 00 00 (unsigned long #1) (unsigned long #2)
When 32-bit userspace receives that, it'll see this:
00 00 00 00 00 00 00 06 00 00 00 00 00 00 00 00 (word #1) (word #2) (word #3) (word #4)
.. in which it's bits 33 and 34 that are set. So the new version in the patch to which I replied _still_ needs a compat routine.
You might get away with it if you use set_le_bit() though.
-- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation
| |