Re: [PATCH 2/2 v2] chrdev: allocate dynamic chardevs in all unused holes

[Linus Torvalds]

On Mon, Feb 22, 2016 at 4:20 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> This is a duct-tape-and-chewing-gum solution to the problem
> with the major numbers running out when allocating major
> numbers dynamically.

Ok, much less hacky, but now the initialization is fairly unreadable,
even if the comment kind of saves it:

> +/*
> + * A bitmap for the 255 lower device numbers. A "1" means the
> + * major number is taken, a "0" means it is available. We first
> + * need to define all the assigned devices as taken so that
> + * dynamic device allocation will not go in and steal them.
> + */

But then the numbers are really hard to grok, and verify that they
actually match the comments:

> +static u32 majors_map[] = {
> +       /* 8 and 26 are free */
> +       BITS(0, 7) | BITS(9, 25) | BITS(27, 31),

That first one looks "obviously correct", but then:

> +       /* 40 and 60-63 are free */
> +       BITS(0, 7) | BITS(9, 27),
> +       /* 93 and 94 are free */
> +       BITS(0, 28) | BIT(31),

Yeah, it's not exactly obvious that 93/94 modulo 32 is 29/30 etc.

Now, we could fix it two ways:

 - make a BITS32() macro that just masks things by 32 bits, so that
you could write this as

      /* 8 and 26 are free */
      BITS32(0, 7) | BITS32(9, 25) | BITS32(27, 31),
      /* 40 and 60-63 are free */
      BITS32(32, 39) | BITS32(41, 59),
      /* 93 and 94 are free */
      BITS32(64, 92) | BIT32(95),
      ...

which would at least take away *some* of the costs.

Or, quite frankly, just waste memory, and make the array be an array
of buytes, and make it a whole lot more readable. That *could* have
advantages in that it would allow you to differentiate "used
dynamically" from "statically allocated" in the array too, in that you
now have more values than 0/1 to play with. You could also make it
show both the block and char assignments, for example, and use the
same map for both (even if perhaps only used for character devices
initially). Then the initializer could just look like

      #define CHR 1
      #define BLK 2
      #define DYNCHR 0x10  // set when allocated for dynamic char device
     #define DYNBLK 0x20 // set when allocated for dynamic block device

      static unsigned char major_map[] = {
            [0] = CHR|BLK,   // unnamed block devices, no character device
            [1] = CHR|BLK,   // memory devices, ramdisk
            ...

and that would make the source code much bigger, but it would perhaps
be worth it for the legibility and documentation reasons.

I dunno. Maybe I'm worrying about something that isn't worth worrying about.

                    Linus