On Tue, Jan 20, 2004 at 02:50:48PM -0500, Alan Stern wrote:
> 1. Keep the children[] array? Getting rid of it wouldn't be as
> bad as you seem to think, but I don't object to keeping it.
>
> 2. Leave it in struct usb_device? At first I thought it would
> make more sense to move it into the hub private data structure, but now I
> don't think so. Doing that would require readers to keep a pointer to
> private data that could disappear at any time. Keeping it where it is
> seems simplest, even if not the most space-efficient.
I think it's simpler to just leave it alone, but if you come up with a
good patch for 2.7, I'll be glad to look at it.
> 3. Mark usb_devices as soon as we know they are unplugged? David
> is strongly in favor of this and I don't see any real problems with it
> either. It does raise the problem of protecting usbdev->state because
> it means state can change to NOTATTACHED at any time, even while
> usbdev->serialize is held by another task.
Ok, I like this idea, but we need to be careful about it. I got it
wrong last time I did it, so I'm hesitant :)
> 4. How to protect the global bus topology? So far this issue has
> been ignored, leading to incorrect code as I'll discuss below. For now
> just note that whatever mechanism protects the topology would also
> naturally protect ->state, since state = NOTATTACHED indicates a topology
> change.
The rwsem for the usb bus should protect this, right?
> > We would have to special case a lot of code in order to get this to work
> > properly (hint, how to detect a hub? everywhere that we walk the lists
> > we would have to export that logic, no fun... And other devices do have
> > children in the sysfs tree, scsi, tty, etc.)
>
> It's not all that bad. For instance, here's how to detect a hub: A struct
> usb_device is a hub iff it has 1 configuration with 1 interface and that
> interface is bound to the hub driver. Here's how to distinguish the
> usb_devices from the other children present in the sysfs tree: A struct
> device is embedded in a struct usb_device iff it is bound to the
> usb_generic_driver. Agreed, adding code to export these notions would
> make things a little messy.
>
> Furthermore, the USB device tree is not searched or walked in very many
> places. Of course there are the spots involved in adding/removing devices
> and doing device resets. The only others are usb_device_dump() in
> devices.c and match_device() in usb.c -- and they are incorrect. For
> instance, consider this code in match_device():
>
> for (child = 0; child < dev->maxchild; ++child) {
> if (dev->children[child]) {
> ret_dev = match_device(dev->children[child],
> vendor_id, product_id);
>
> What's to prevent dev from disappearing while this loop executes? What's
> to prevent dev->children[child] from disappearing in the instant between
> the "if" and the recursive call to match_device()?
>
> Clearly some sort of "topology lock" is needed, and a spinlock seems best
> suited to the job.
Heh, what about the code in the devices usbfs file? That's so scary
that it even works at all...
Sure, fixups in this area would be appreciated, for 2.7.
> > Again, every bit of code that walks the usb device lists would have to
> > have access to those data structures in order to have a chance of
> > walking the lists in a semi-coherient manner. Look around the USB code
> > for everywhere we do this. Then look in the security/ directory for
> > another user :)
>
> The code in security/root_plug.c only uses the data structures indirectly,
> by way of match_device().
Ah, good, I remember the first version of that code, which was a lot
worse...
> > The main problem about a NOTATTACHED type state, is that we can never
> > test properly that the state is not NOTATTACHED, but only that it is
> > NOTATTACHED.
>
> I really don't understand that comment. Certainly we know at all times
> what value is stored in usbdev->state. Do you mean that we can't find out
> immediately when a device is physically unplugged? That's true, but I'm
> more concerned about maintaining the internal consistency of the data
> structures. When something is unplugged, we will update the data
> structures to reflect the new reality shortly afterwards.
We can't rely on doing something because we think the device is
attached. All we can know for sure is that something is not attached.
In other words code that tries to do:
if (usb_dev->state == ATTACHED)
do_foo();
is wrong and racy.
But code that does:
if (usb_dev->state == NOTATTACHED)
goto reject_urb;
It's touchy code, just be aware of it.
> > > As David said earlier, it's _not_ legal to access a device after returning
> > > from disconnect() because another driver might already be bound to the
> > > interface. Again, that's the rmmod case -- for disconnect obviously it
> > > doesn't matter.
> >
> > But after rmmod finished, the driver isn't present to be touching the
> > device. Well, it had better not, otherwise we have worse problems :)
>
> How about drivers that are unbound through usbfs? They will remain in
> memory.
Yeah, forgot about that. You are correct. Bleah...
> Yes. One of the changes I want to make rather soon is to insure that URBs
> are unlinked and endpoints disabled whenever a driver is unbound from an
> interface, regardless of the pathway involved.
That seems sane.
> Likewise, if the device hasn't been unplugged then reinstall
> altsetting 0 after calling the driver's disconnect().
This might mess some devices up, but is worth trying for 2.7
thanks,
greg k-h
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
