This is a tricky topic... There are several interrelated issues to
consider.
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.
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.
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.
On Mon, 19 Jan 2004, Greg KH wrote:
> > It's not a question of saving memory, it's the principle of not
> > duplicating information and thereby avoiding the risk of having
> > inconsistent records. I don't think it will complicate other parts of the
> > USB code very much. Yes, it will be a little harder to go from a port
> > number to the corresponding usb_device (it will require a search through
> > the list of children). But that lookup only occurs while processing a
> > connect or disconnect event, not very often. And also it will simplify
> > the device_reset code, going from usb_device to parent port number --
> > the search that's in there now will become unnecessary.
>
> The scsi people tried to do this, and ran away screaming in terror (I
> heard them accross the building, it wasn't pretty...)
>
> 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.
> 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().
> > This lock should not see much contention. It will be held only very
> > briefly in any path where a state gets changed. Something like this:
> >
> > spin_lock_irq(&global_state_lock);
> > if (usbdev->state != USB_STATE_NOTATTACHED)
> > usbdev->state = USB_STATE_CONFIGURED;
> > else
> > notattached = 1;
> > spin_unlock_irq(&global_state_lock);
> > if (notattached) ...
> >
> > Even with multiple probes running in parallel this won't cause any
> > significant delays.
> >
> > The reason for adding the lock was David's firm belief that we should
> > report a device disconnection (by setting the state to
> > USB_STATE_NOTATTACHED) as soon as it is detected, rather than having to
> > wait for a driver's disconnect() routine to run or to wait for
> > usbdev->serialize. If the state is not protected by serialize then it
> > needs some other sort of protection, and a global spinlock is the simplest
> > way to do it.
>
> Why does the lock have to be taken to set the state in this case?
> There is not more than one thread that is setting the state, right?
> Just multiple users.
No, there can be two threads setting the state. usb_set_configuration()
could be executing code like the part quoted above concurrently with
usb_mark_unplugged() -- a new routine that will change the state to
NOTATTACHED as soon as we know the device is unplugged.
If we don't implement David's idea then yes, only one thread would set the
state at a time. But we would still need the topology lock, for the
reasons given above. So why not have it protect state as well?
> 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.
> > Another possibility suggested by David would be to add an extra
> > "unplugged" flag to struct usb_device. That flag could be set as soon as
> > we know the device is unplugged, with no protection at all since it would
> > never get reset.
>
> That's fine with me too. But watch out, I did that with the driver
> model during the 2.5 development cycle, and spent a lot of time trying
> to clean up the mess it caused...
I've decided it's simpler not to do this, just add the topology spinlock.
> > 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.
> > > We already do this today.
> >
> > Ah -- we do it when the device is unplugged. So far as I can see, we
> > don't do it when usb_deregister() is called.
>
> That should be changed then.
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. Likewise, if the device
hasn't been unplugged then reinstall altsetting 0 after calling the
driver's disconnect().
Alan Stern
-------------------------------------------------------
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
