Hi Richard, Hi List-Readers!
In linux-2.4.17/fs/devfs/util.c, I found the following code:
struct major_list
{
spinlock_t lock;
__u32 bits[8];
};
/* Block majors already assigned:
0-3, 7-9, 11-63, 65-99, 101-113, 120-127, 199, 201, 240-255
Total free: 122
*/
static struct major_list block_major_list =
{SPIN_LOCK_UNLOCKED,
{0xfffffb8f, /* Majors 0 to 31 */
0xffffffff, /* Majors 32 to 63 */
0xfffffffe, /* Majors 64 to 95 */
0xff03ffef, /* Majors 96 to 127 */
0x00000000, /* Majors 128 to 159 */
0x00000000, /* Majors 160 to 191 */
0x00000280, /* Majors 192 to 223 */
0xffff0000} /* Majors 224 to 255 */
};
Afterwards, the block_major_list.bits is processed using
find_first_zero_bit & set_bit out of asm/bitops.h.
Since bitops are only defined for the datatype long, this does
only work on 32-bit architectures (on 64 bit data gets
incorrectly alligned -not on 8byte boundary & the ordering of
the data is incorrect).
I attached a patch that should fix it for all architectures.
(See attached file: linux-2.4.17-devfs_fixup.diff)
with kind regards
Carsten Otte
On Mon, Feb 11, 2002 at 02:00:06PM +0100, Carsten Otte wrote:
> Afterwards, the block_major_list.bits is processed using
> find_first_zero_bit & set_bit out of asm/bitops.h.
Isn't it about time we made the bitops take an unsigned long pointer
argument, rather than a void pointer to catch errors like this?
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
> Isn't it about time we made the bitops take an unsigned long pointer
> argument, rather than a void pointer to catch errors like this?
Davem tried this a while ago (the changes made it into the vger tree)
but I think the number of casts required led to it being removed.
But I do agree, I have found long standing bugs before by changing them
to take an unsigned long.
Anton
From: Russell King <[email protected]>
Date: Mon, 11 Feb 2002 16:12:59 +0000
Isn't it about time we made the bitops take an unsigned long pointer
argument, rather than a void pointer to catch errors like this?
No, I tried to do that once, but the casts become stupid
and ugly.
David S. Miller <[email protected]> wrote:
>No, I tried to do that once, but the casts become stupid
>and ugly.
I agree, and with explicit casts the compiler still would'nt even print a
warning
when the bitops are incorrectly used.
w/kind regards
Carsten Otte
Carsten Otte writes:
> In linux-2.4.17/fs/devfs/util.c, I found the following code:
> struct major_list
> {
> spinlock_t lock;
> __u32 bits[8];
> };
>
> static struct major_list block_major_list =
> {SPIN_LOCK_UNLOCKED,
> {0xfffffb8f, /* Majors 0 to 31 */
[...]
> 0xffff0000} /* Majors 224 to 255 */
> };
>
> Afterwards, the block_major_list.bits is processed using
> find_first_zero_bit & set_bit out of asm/bitops.h.
> Since bitops are only defined for the datatype long, this does
> only work on 32-bit architectures (on 64 bit data gets
> incorrectly alligned -not on 8byte boundary & the ordering of
> the data is incorrect).
> I attached a patch that should fix it for all architectures.
Sorry, but I find your approach grotesque. Apart from basic warts such
as not declaring code+data as __init, the approach of populating the
bitfield from yet another list doesn't appeal to me. I'd much rather
see an approach which preserved the initialisation using bitmasks.
> (See attached file: linux-2.4.17-devfs_fixup.diff)
BTW: please don't send attachments. Send patches inline instead.
Regards,
Richard....
Permanent: [email protected]
Current: [email protected]
Hi Richard!
>BTW: please don't send attachments. Send patches inline instead.
Sorry for sending the patch as attachment, but Notes messes
up whitespace so the patch would'nt apply if I include it directly.
>Sorry, but I find your approach grotesque. Apart from basic warts such
>as not declaring code+data as __init, the approach of populating the
>bitfield from yet another list doesn't appeal to me. I'd much rather
>see an approach which preserved the initialisation using bitmasks.
I do not think this patch is very nice either & it does not work at
all (the initialisation of the array is only called in error case).
I find the overall thing for registering/deregistering devices &
allocating majors very inconsistent.
devfs_alloc_major and devfs_register_*dev do hold the information
about which majors are allocated in two different places without
knowing about each other (bdops field and this private bitfield).
A good solution would be if *dev_register would never return a
major being statically allocated when called with major 0. If this is the
case, I do not see what alloc_major and dealloc_major are useful for.
mit freundlichem Gru? / with kind regards
Carsten Otte
IBM Deutschland Entwicklung GmbH
Linux for eServer development - device driver team
Phone: +49/07031/16-4076
IBM internal phone: *120-4076
--
We are Linux.
Resistance indicates that you're missing the point!
Carsten Otte writes:
>
> Hi Richard!
>
> >BTW: please don't send attachments. Send patches inline instead.
> Sorry for sending the patch as attachment, but Notes messes
> up whitespace so the patch would'nt apply if I include it directly.
>
> >Sorry, but I find your approach grotesque. Apart from basic warts such
> >as not declaring code+data as __init, the approach of populating the
> >bitfield from yet another list doesn't appeal to me. I'd much rather
> >see an approach which preserved the initialisation using bitmasks.
>
> I do not think this patch is very nice either & it does not work at
> all (the initialisation of the array is only called in error case).
Now you've confused me. Your patch seems to unconditionally initialise
the bitfields at startup. I didn't see logic for restricting that
initialisation to an error path.
> I find the overall thing for registering/deregistering devices &
> allocating majors very inconsistent.
> devfs_alloc_major and devfs_register_*dev do hold the information
> about which majors are allocated in two different places without
> knowing about each other (bdops field and this private bitfield).
> A good solution would be if *dev_register would never return a major
> being statically allocated when called with major 0. If this is the
> case, I do not see what alloc_major and dealloc_major are useful
> for.
Except that devfs_register_???dev() (which are in fact minor
variations on the register_???dev() calls) *do not* avoid assigned
majors. That is why I wrote devfs_alloc_major() in the first place.
And while I do think that register_???dev() should in fact do just
what devfs_alloc_major() does, that's not a battle I care to fight. By
writing devfs_alloc_major(), this functionality is optional, and I can
avoid a whole pile of stupid flaming.
Hey, hey! It looks like vger is sending emails again!
Regards,
Richard....
Permanent: [email protected]
Current: [email protected]
Richard Gooch writes:
>Except that devfs_register_???dev() (which are in fact minor
>variations on the register_???dev() calls) *do not* avoid assigned
>majors. That is why I wrote devfs_alloc_major() in the first place.
To me, the disadvantage of this soloution is that if a device driver has
called devfs_register_blkdev with major 0 (automatic allocation) and
another device driver calls devfs_alloc_major, the same major number may
be assigned twice. Same situation is vice versa (major has been allocated,
devfs_register_blkdev with major 0 gives the same major to a different
driver).
>And while I do think that register_???dev() should in fact do just
>what devfs_alloc_major() does, that's not a battle I care to fight. By
>writing devfs_alloc_major(), this functionality is optional, and I can
>avoid a whole pile of stupid flaming.
I reworked my initial patch in order to
- avoid setting up the array on initialisation (since you dislike this)
- have a portable soloution that works on any arch
- avoid assigning staticaly allocated majors with any devfs_* functions
- avoid assigning the same major twice with devfs_* functions
Please do have a look at this patch (sorry, attached again) and let me
know what you think of including this solution.
w/kind regards
Carsten Otte
(See attached file: linux-2.4.17-devfs_fixup.diff)
Hi Richard,
the previous version of my patch did contain a bug,
caused by incorrect parameter order when calling
__devfs_unregister_major. The symptom was, that
major numbers registered with devfs_register_*dev
but not allocated with devfs_alloc_major were not
freed by calling devfs_unregister_*dev. This is
now fixed. Sorry, the patch is attached (due to Notes
messing up with whitespace).
(See attached file: linux-2.4.17-devfs_fixup.diff)
Richard, I would appreciate it if you could finally
look into this.
mit freundlichem Gru? / with kind regards
Carsten Otte
IBM Deutschland Entwicklung GmbH
Linux for eServer development - device driver team
Phone: +49/07031/16-4076
IBM internal phone: *120-4076
--
We are Linux.
Resistance indicates that you're missing the point!
Carsten Otte writes:
> the previous version of my patch did contain a bug,
> caused by incorrect parameter order when calling
> __devfs_unregister_major. The symptom was, that
> major numbers registered with devfs_register_*dev
> but not allocated with devfs_alloc_major were not
> freed by calling devfs_unregister_*dev. This is
> now fixed. Sorry, the patch is attached (due to Notes
> messing up with whitespace).
> (See attached file: linux-2.4.17-devfs_fixup.diff)
In future, please send patches in plain text, rather than
MIME-encoded.
> Richard, I would appreciate it if you could finally
> look into this.
I don't like your patch because:
- it replaces the bitfield with a character array. This in turn causes
more data bloat (8 times more), and also prevents the use of the ffz
functions
- you've mixed in a behavioural change to devfs_register_???dev() to
call devfs_alloc_major() when the provided major is 0. While this
might be good idea in the long run (but maybe not), it should be
separated from the actual fix.
Unfortunately I've been busy chasing other (possible, not sure yet
what the source of the problems are) bugs, so I haven't had time to
code up a solution yet.
Is there any reason why switching from __u32 to unsigned long won't
work? Of course, the initialising values would need to be 64 bits wide
on a 64 bit machine, but that could probably be taken care of with
some clever macros (ab)use:
#if 64 bit
#define INITIALISER64(a,b) (a)<<32|(b)
#else
#define INITIALISER64(a,b) (a),(b)
#endif
static struct major_list block_major_list =
{SPIN_LOCK_UNLOCKED,
{INITIALISER64(0xfffffb8f,0xffffffff), /* Majors 0 to 63 */
INITIALISER64(0xfffffffe,0xff03ffef), /* Majors 64 to 127 */
and so on. Untested, and I only spent a few seconds thinking about it,
but perhaps this will solve it.
Regards,
Richard....
Permanent: [email protected]
Current: [email protected]