On Fri, Apr 26, 2002 at 01:49:33AM +0200, Oliver Neukum wrote:
> Am Donnerstag, 25. April 2002 17:11 schrieb Greg KH:
> > 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" :)
>
> That means that you absolutely depend on that demon always working.
> There's no fault tolerance at all. This means, for example that
> any script failing due to oom or an io error opens a potential
> security risk.
>
> I really want to see a scheme that works securely. And I won't
> even mention the issue of races. You'll find a very long thread
> between me and Adam Richter about this in the archives.
I remember that thread :)
I'll ignore this for now, as I'm not writing a userspace daemon to do
this.
> Devfs goes the whole way.
> Let's look at the basic function of major:minor combinations.
> You use them to assign names and permissions to a device.
> If minors become dynamic minors no longer have any function,
> you are better of eliminating them.
I would love to eliminate minors, but can't due to the current system. If
the core of the kernel that handles major/minor numbers changes, then I
will change USB to match that. But for today, this is what is needed
for large number of devices.
> Your system as is has an obvious race. A new device will
> have old arbitrary permissions before the hotplug script can reset them.
>
> > 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.
>
> What is the loop supposed to do then ?
> It has some comments about looking internally for a minor.
That loop executes if CONFIG_USB_DYNAMIC_MINORS is NOT enabled. It's
the existing way of finding a minor number for a device. I'm not
changing that logic at all, just moving it within a if() call. This
way, if CONFIG_USB_DYNAMIC_MINORS is not enabled, the driver falls back
to the original way of handling its minors.
>
> > > > @@ -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? :)
>
> Some drivers keep an internal array to assign minors to device
> descriptors.
Agreed, and I'm not touching that logic. They still use that array to
assign a minor to a device descriptor, I just increased the size of the
array if CONFIG_USB_DYNAMIC_MINORS is enabled.
> > 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.
>
> Yes.
Added, thanks. See the patch I just sent to the list for the final
version with this lock.
> > > 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 :)
>
> You should have.
> You allow yourself to be bound to the concept of minors
> without regard for meaning.
> In fact your solution gets you the worst of both worlds.
> You no longer have easy, simple persistent storage of names
> and permissions in /dev. In fact you have now stale names
> and permissions on disk.
> Yet you have none of the features devfs offers.
Wait, I'm still playing nicely with devfs. I am not trying to replace
devfs at all. With this patch we now can have 256 printers. Using
devfs this will cause the nodes:
/dev/usb/lp0 .. /dev/usb/lp255
if we have that many devices.
Hm, I think I see what you are saying, if I have 1 printer and 1
scanner, the following nodes will be created:
/dev/usb/lp0
/dev/usb/scanner1
instead of:
/dev/usb/lp0
/dev/usb/scanner0
that's not so nice, but pretty simple to fix. I'll work on that
tomorrow.
Is this your objection?
Remember, devfs does not remove our need for minor numbers. It just
imposes a naming scheme on the kernel for the major/minor pairs that
happen to be present in the system at a moment in time. This patch is
still needed to extend our usage of the USB minor range.
thanks,
greg k-h
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
