confused by char dev registration in a gpio driver

confused by char dev registration in a gpio driver

[Robert P. J. Day]

  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@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


-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

confused by char dev registration in a gpio driver

[Greg KH]

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

confused by char dev registration in a gpio driver

[Jim Cromie]

On Tue, May 3, 2011 at 4:06 PM, Greg KH <greg@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

confused by char dev registration in a gpio driver

[Greg KH]

On Tue, May 10, 2011 at 11:26:59AM -0600, Jim Cromie wrote:
> 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 ?

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).

> - 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 ?

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.

Let's get the api fixed up first, that's the most important thing as
it's what people use all the time.

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.

thanks,

greg k-h

confused by char dev registration in a gpio driver

[Jim Cromie]

On Tue, May 10, 2011 at 12:40 PM, Greg KH <greg@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
>

confused by char dev registration in a gpio driver

[Greg KH]

On Fri, May 13, 2011 at 11:05:08AM -0600, Jim Cromie wrote:
> On Tue, May 10, 2011 at 12:40 PM, Greg KH <greg@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

confused by char dev registration in a gpio driver

[Jim Cromie]

hi Robert,

On Tue, May 3, 2011 at 3:21 PM, Robert P. J. Day <rpjday@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