On Tue, May 3, 2011 at 4:06 PM, Greg KH <greg@xxxxxxxxx> wrote:
> On Tue, May 03, 2011 at 05:21:11PM -0400, Robert P. J. Day wrote:
>>
>> i always thought both of those routines returned a simple zero to
>> indicate success. but look at those last few lines -- that return
>> code is assigned to "major", at which point it's *that* value that's
>> printed. wouldn't that just be zero all the time?
>>
>> and wouldn't it also print that this was a "dynamic" major even if
>> the user specified an explicit major number at load time? this second
>> point is more nitpicky, but what about that first point? wouldn't a
>> successful registration always print an allocated major number of
>> zero?
>
> I think you are right in that this code is wrong.
>
> The chardev stuff is a mess, I keep meaning for years to clean it up.
> Any proposals on a sane interface for this stuff is greatly appreciated.
>
> thanks,
>
> greg k-h
lets start with a list of grumbles about current api ?
1 - register_chrdev_region() VS alloc_chrdev_region()
ISTM that these could be combined into 1 function:
dev_t devid = MKNOD(major, minor);
register_chrdev_numbers(&devid, count, name);
ie make &devid an in/out parameter, where in-value
distinguishes which alloc/register behavior is desired.
this looks like a minor variation on alloc_chrdev_region()
one downside is need to include kdev_t.h
2 - EXPORTs in drivers/char/char_dev.c
original version (~2.6.12-rc2) had no __double-under exports,
now have:
EXPORT_SYMBOL(__register_chrdev);
EXPORT_SYMBOL(__unregister_chrdev);
__register_chrdev() looks to be a higher level function;
it 1st calls __register_chrdev_region()
then does cdev, kobject stuff too.
Or, why does it have __ in the name ?
__ typically means not for external use, but its exported.
is it because it does the cdev_add "too early"
ie before the caller is really ready to handle service requests ?
theres no caveat mentioned in the /** comments
Are the insanities you alluded to of a different sort,
ie internal suboptimalities ?
- struct inefficiencies ?
struct cdev *cdev; /* will die */
why die ? not needed, or available in another struct ?
- in char_dev.c, there is
static struct char_device_struct {
...} *chrdevs[CHRDEV_MAJOR_HASH_SIZE];
ie 255 elements.
This is == to legacy MAJOR number space,
and is scanned 255..0 to find unused MAJOR number.
It will need new const if MAJOR range increases,
but doesnt waste much memory currently
My 32bit laptop uses 48 devices, so ~200 major slots are
empty/wasted (800 bytes) and ISTM like over-engineering
to fix this preemptively if >255 is unneeded.
OTOH, a hash or rb-tree could use only whats needed.
is there a 'library' hash implementation in the kernel ?
thanks
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@xxxxxxxxxxxxxxxxx
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
