Re: confused by char dev registration in a gpio driver
On Tue, May 10, 2011 at 12:40 PM, Greg KH g...@kroah.com wrote:
On Tue, May 10, 2011 at 11:26:59AM -0600, Jim Cromie wrote:
lets start with a list of grumbles about current api ?
...
Are the insanities you alluded to of a different sort,
ie internal suboptimalities ?
No, those you have listed are all things I was talking about.
And yes, there are also some internal issues as well (see the kobject
mapping stuff).
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 ?
The kobject map stuff should handle most of this, but really, 800 bytes
isn't that much overhead for the speed you get, right? Changing the
code to use something else would take more than 800 bytes from what I
can see.
cool. Im pleased that I agree with you :-) regarding the 800 byte cost.
but is speed even an issue ?
registering char drivers isnt exactly a hot path.
of course, simplicity is good too..
Let's get the api fixed up first, that's the most important thing as
it's what people use all the time.
Ive gone and done this.
Im currently messing with coccinelle / spatch to try to automate
the API - user conversion.
will send when the spatch transforms are a bit more sane.
I'm going to have some long-distance flights soon, so I'll try to work
on this then, thanks for the great summary above, I appreciate it.
Hope Ive beaten you to it,
or hold off on actually doing the work
thanks,
greg k-h
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confused by char dev registration in a gpio driver
On Fri, May 13, 2011 at 11:05:08AM -0600, Jim Cromie wrote:
On Tue, May 10, 2011 at 12:40 PM, Greg KH g...@kroah.com wrote:
On Tue, May 10, 2011 at 11:26:59AM -0600, Jim Cromie wrote:
lets start with a list of grumbles about current api ?
...
Are the insanities you alluded to of a different sort,
ie internal suboptimalities ?
No, those you have listed are all things I was talking about.
And yes, there are also some internal issues as well (see the kobject
mapping stuff).
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 ?
The kobject map stuff should handle most of this, but really, 800 bytes
isn't that much overhead for the speed you get, right? Changing the
code to use something else would take more than 800 bytes from what I
can see.
cool. Im pleased that I agree with you :-) regarding the 800 byte cost.
but is speed even an issue ?
registering char drivers isnt exactly a hot path.
of course, simplicity is good too..
Speed is an issue on open() for some things, so yes, it can be
important (meaning, don't make it a linked list that you need to walk
all of them to find the last character device in the system if opening
just that one.)
Let's get the api fixed up first, that's the most important thing as
it's what people use all the time.
Ive gone and done this.
Im currently messing with coccinelle / spatch to try to automate
the API - user conversion.
will send when the spatch transforms are a bit more sane.
Ok, sounds good, looking forward to it.
I'm going to have some long-distance flights soon, so I'll try to work
on this then, thanks for the great summary above, I appreciate it.
Hope Ive beaten you to it,
or hold off on actually doing the work
Well, my trip isn't for another week or so (depending on some changing
plans), so you might beat me to it, which is fine with me :)
thanks,
greg k-h
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confused by char dev registration in a gpio driver
On Tue, May 3, 2011 at 4:06 PM, Greg KH g...@kroah.com 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@kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confused by char dev registration in a gpio driver
hi Robert,
On Tue, May 3, 2011 at 3:21 PM, Robert P. J. Day rpj...@crashcourse.ca wrote:
i'm sure i'm going to embarrass myself here but i was perusing the
char drivers for nice examples, and i ran across this excerpt in
pc8736x_gpio.c:
Thanks for taking a look, and questioning what looks wrong.
= begin =
if (major) {
devid = MKDEV(major, 0);
rc = register_chrdev_region(devid, PC8736X_GPIO_CT, DEVNAME);
} else {
rc = alloc_chrdev_region(devid, 0, PC8736X_GPIO_CT, DEVNAME);
major = MAJOR(devid);
}
so major is set here, either before the if, or as result of the else.
if (rc 0) {
dev_err(pdev-dev, register-chrdev failed: %d\n, rc);
goto undo_request_region;
}
so when we get here, rc==0, and major is set
FWIW, it looks like I poached the above code from LDD3,
which explains why it looks right up to here..
Id agree that theres something wrong/unclear below.
major = rc looks kinda rotten.
Given that the only purpose was to debug-print,
and the dynamic fact is wrong, I think I'll prep a patch to yank that crap.
Unless you beat me to it :-)
if (!major) {
major = rc;
dev_dbg(pdev-dev, got dynamic major %d\n, major);
}
= end =
i'm good with most of that -- if the (parameter) major is explicit,
then a dev_t of devid is created and register_chrdev_region() is
used.
on the other hand, if major is zero, then alloc_chrdev_region() is
used for *dynamic* allocation of the major number. in both cases, the
return code rc is saved and, if it's 0, we have an error. and
that's where the confusion comes in.
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?
rday
again, thanks.
~jimc
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confused by char dev registration in a gpio driver
On Tue, May 03, 2011 at 05:21:11PM -0400, Robert P. J. Day wrote:
i'm sure i'm going to embarrass myself here but i was perusing the
char drivers for nice examples, and i ran across this excerpt in
pc8736x_gpio.c:
= begin =
if (major) {
devid = MKDEV(major, 0);
rc = register_chrdev_region(devid, PC8736X_GPIO_CT, DEVNAME);
} else {
rc = alloc_chrdev_region(devid, 0, PC8736X_GPIO_CT, DEVNAME);
major = MAJOR(devid);
}
if (rc 0) {
dev_err(pdev-dev, register-chrdev failed: %d\n, rc);
goto undo_request_region;
}
if (!major) {
major = rc;
dev_dbg(pdev-dev, got dynamic major %d\n, major);
}
= end =
i'm good with most of that -- if the (parameter) major is explicit,
then a dev_t of devid is created and register_chrdev_region() is
used.
on the other hand, if major is zero, then alloc_chrdev_region() is
used for *dynamic* allocation of the major number. in both cases, the
return code rc is saved and, if it's 0, we have an error. and
that's where the confusion comes in.
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
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
