On Thu, Apr 25, 2002 at 10:14:59AM +0200, Oliver Neukum wrote:
>
> > Right now there isn't a "simple" way for userspace to realize what minor
> > is assigned to each device, but I will work on that tomorrow, as it's
> > late (hint, /sbin/hotplug will be involved :)
>
> This is no good. You come to a system of extreme brittleness this way.
> There's no way to resynchronise. You need a way to query all
> assigned minors at any time from any application from the kernel.
Why is that a requirement? I've heard this argument before quite
frequently from the people I work with (old Dynix/PTX engineers which
had such a system.) If you have a working userspace deamon that keeps
track of such things properly, you do not need to burden the kernel with
such a database. devfs doesn't have this "feature" :)
But I'll leave the userspace interaction comments for later, as other
people are working on solutions that USB will tie into. That work is
not yet finished, so I'll hold off for now.
> > This patch is against 2.5.10.
> >
> > Comments?
>
> Oh yes.
>
> > diff -Nru a/drivers/usb/image/scanner.c b/drivers/usb/image/scanner.c
> > --- a/drivers/usb/image/scanner.c Thu Apr 25 00:01:15 2002
> > +++ b/drivers/usb/image/scanner.c Thu Apr 25 00:01:15 2002
> > @@ -953,9 +953,11 @@
> >
> > down(&scn_mutex);
> >
> > - for (scn_minor = 0; scn_minor < SCN_MAX_MNR; scn_minor++) {
> > - if (!p_scn_table[scn_minor])
> > - break;
> > + if (usb_register_dev(&scanner_driver, 1, &scn_minor)) {
> > + for (scn_minor = 0; scn_minor < SCN_MAX_MNR; scn_minor++) {
> > + if (!p_scn_table[scn_minor])
> > + break;
> > + }
> > }
>
> What is this supposed to do ?
> If you have freeable minors, free them.
> Hoarding minors is sociopathic behaviour.
Huh? We are asking for a minor when we really have a device plugged in.
That's all, no hoarding. So in the probe() function, we call
usb_register_dev() to get a minor, and in the disconnect, we call
usb_deregister_dev() to free it up.
> > @@ -642,8 +649,11 @@
> > minor = dev->minor;
> >
> > /* remove our devfs node */
> > - devfs_unregister(dev->devfs);
> > + devfs_unregister (dev->devfs);
> >
> > + /* give back our dynamic minor */
> > + usb_deregister_dev (&skel_driver, 1, minor);
> > +
> > /* if the device is not opened, then we clean up right now */
> > if (!dev->open_count) {
> > up (&dev->sem);
>
> You are redefining the meaning of a minor while somebody
> can still have the file open. Is this safe ?
It's ok for devfs, why not for us? :)
It's ok, as all "giving up a minor" does, is clear the minor in the usb
core. It can then give the minor to someone else if asked, which will
be fine (someone else can call open() which will be with a new file
descriptor, which will go to the new device, not the old one. The "old"
minor can still be used with the previously open file descriptor.)
> Furthermore, you have no locking at all for the minors' array.
> There's a race.
Do you mean for the usb-skeleton driver? There's a lock right above
this chunk of code that locks the minor table within the driver.
Or do you mean the minor table in the usb core. Hm, yeah you're right,
that probably needs a lock. I'll go add that, thanks.
> Last, but not least, this looks like an attempt to do a one off
> solution to a common problem. In other words, your are frantic to
> avoid devfs.
No, this is orthogonal to devfs. devfs still works just fine, and
doesn't reuse minor numbers. Even using devfs you still have the
"limited to 16 minors" problem that this solution solves. All devfs
does for the USB major is to dynamically create the /dev nodes, which is
much different from this. Hey look, I don't even touch the devfs calls
in the usb drivers :)
thanks for the comments.
greg k-h
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
