On Thu, 15 Jan 2004, Greg KH wrote:
> Thanks for writing this. I know I've been pretty much staying out of
> some of these discussions, mainly because I don't think we need most of
> these changes. In other words, these seem like pretty big changes that
> do not need to be done during a stable kernel series. Remember, bug
> fixes now. But if some of the bug fixes require semi-big changes,
> that's ok.
>
> We have some very good stability right now in the USB core, let's keep
> it that way :)
Well, I'm glad I sent out that message. Negative feedback like this is at
least as important as positive feedback...
> On to specific comments...
>
> > The halted[] member of struct usb_device is going to change
> > to refuse[]. I posted another message about this a couple of
> > weeks ago and nobody has responded, so I guess that's okay.
> > Not surprising, since this feature is pretty much unused.
>
> sorry I missed that message. Why change this?
Two reasons. First, the current usage is incorrect. It assumes that once
an endpoint is halted it will remain that way until the host clears the
halt. But that's not true for all devices (David has an example). In
general, we don't need to and should not bother to record when endpoints
are halted.
Second, changing the meaning gives a clearer way to prevent URB
submissions than the current hack of setting the maxpacket size to 0
(which, in addition to being kludgy, doesn't prevent 0-length packets
from being accepted). It will also give drivers a very easy way to stop
automatic resubmissions.
David has suggested that drivers not be allowed to unblock endpoints once
the core has blocked them. Although I wasn't planning on it, that feature
could be added (or removed, depending on how you look at it!). It would
make it harder for a driver to stop automatic resubmissions to an endpoint
that it wanted to use again in the future, however.
> > The children[] and maxchild members will go away. They are
> > meaningful only for hubs, and most of the information is
> > already present in the driver model's children lists.
>
> I think this will be a lot of effort for basically no benefit. Why do
> you want to do this? Save the memory of that tiny pointer array? If
> you do this, a whole lot of other parts of the usb code will get quite
> complex.
>
> In short, I do not recommend doing this.
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.
However, this is not a big deal. If you still feel we should keep the
children[] array I won't mind. But then it should be moved to the
private hub data structure instead of sitting in every usb_device.
> > The information missing in the driver model is the mapping from
> > port number to child device. That mapping will be turned around;
> > each struct usb_device will get a new member called portnum.
>
> Again, if the above change is not made, this is not needed, right?
Right.
> > The state member of struct usb_device will be explicitly
> > _not_ under the protection of the serialize semaphore. Instead
> > there will be a single global spinlock to protect all the
> > states. Events like disconnection will be cause state to
> > change as soon as they are detected, without having to wait for
> > a driver to release usbdev->serialize.
>
> So all device state changes will be under the same spinlock? That's not
> a good idea. I want to get rid of locks, not add a bigger one. What
> happens when I change the driver core to run multiple device probes in
> parallel (yeah, I know that will break a lot of existing stuff, but
> let's not add things that make it worse. And yes, that's 2.7 work...)
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.
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. This would be simpler than using a spinlock, even though
using the lock would be the conventional approach. Do you prefer this
scheme?
> > Now a more interesting item, not yet entirely certain:
> >
> > struct usb_device->serialize will become a read-write semaphore.
>
> Why? I don't see the benefit of this. We want to reduce the contention
> on this lock, not make it bigger. And read-write locks are generally a
> bad thing, and should be avoided as much as possible...
I don't see how making it a read-write semaphore would increase
contention. If anything it should _lower_ contention, since several
readers could hold it at once rather than being forced to wait for each
other.
And remember, this is an rw _semaphore_, not a spinlock. While rw
spinlocks can have problems, I understand that semaphores are generally
much better behaved, at least in some implementations.
However, nobody seems to be certain that we really need to change this
(except possibly Duncan Sands).
> > The serialize semaphore protects against major changes, things that would
> > involve binding/unbinding drivers or altering the device's physical state.
> > In particular, the following routines will require the caller to hold the
> > semaphore (or, in the new version, to have a write-lock):
> >
> > usb_driver_claim_interface,
> > usb_driver_release_interface,
> > usb_disconnect,
> > usb_new_device,
> > usb_disable_device,
> > usb_reset_configuration,
> > usb_set_configuration,
> > usb_reset_device
> >
> > Also, a driver is guaranteed that the semaphore will be held when its
> > probe() and disconnect() routines are called.
>
> That's already the case, right?
For probe() and disconnect(), yes. For the other functions listed above,
no, not all of them, not yet -- although we are moving in that direction
and I would like to see us get there quickly.
> > In general, a driver can acquire the serialize semaphore whenever it wants
> > to block one of the functions above temporarily.
>
> Ick.
>
> > A good example would be when calling usb_ifnum_to_if(); that function
> > scans the interfaces in the current configuration and the results
> > would be meaningless if the configuration unexpectedly changed.
>
> A very rare occurrence.
Actually, as David pointed out, a driver generally will not need to do
this. It is guaranteed that the configuration won't change from the time
probe() is called until disconnect() returns. The primary users will end
up being usbfs and the hub driver (should it ever become multi-threaded).
Anyway, why complain that drivers would need the semaphore only on very
rare occasions if you're so worried about reducing contention? :-)
> I'm all for fixing bugs in the existing usbfs. But I think that's
> basically beating a dead horse into a prettier lump of goo. We really
> need to start over and make usbfs2. It should look something like the
> gadgetfs interface.
What about backward compatibility?
> Because of all of this, please don't do this now. It's a big change,
> one that should be held off till 2.7 to try to do (if you really want to
> then.)
I'm happy to leave usbdev->serialize as a regular semaphore. We should
still require that it be held by callers of all the routines in the list
above, though.
> > Some longer-range changes:
> >
> > Add a new usb_device_state: USB_STATE_RESET. Although it
> > doesn't correspond exactly to any state given in the USB spec
> > it could be very useful. A device is in this state while it
> > or a hub upstream from it is undergoing a reset.
> >
> > The somewhat obscure reason for having this is that it allows us to
> > recover the devices attached to a hub when that hub is reset. Resetting a
> > hub disables all its ports; after the hub revives, each of its children
> > will have undergone the equivalent of a reset. Knowing that this has
> > happened, we will be able to retain the child device structures and their
> > driver bindings. They will resume activity rather than going through the
> > process of logical disconnect followed by logical connect -- which would
> > have the effect of replacing each device with a new incarnation, breaking
> > things like filesystem mounts.
>
> But as the device did just loose power, and then get it back again, it's
> the only sane thing to do. We have to drop the whole device and
> initialize it again. Think of downloading firmware to devices.
As I mentioned in a previous message, for mass storage devices we do not
want to go through the disconnect() - probe() sequence. For other devices
it may not matter so much, so long as they know about the reset and can
reinitialize the device.
On further though I believe that this can all be implemented without
adding the extra USB_STATE. The only loss would be that the core couldn't
block URBs submitted to devices undergoing a reset, since the device state
wouldn't indicate the reset was happening -- but if the drivers are
notified of the upcoming reset then there won't be any such URBs to worry
about.
> And also, when is a hub ever reset? :)
You've got me there. I've heard that bus-powered hubs tend to be a bit
unreliable.
> > This may turn out to be excessive design. I don't know that hubs need to
> > be reset at all often, and it might not be worth the trouble to do all
> > this. Furthermore, it would greatly complicate the set of possible state
> > transitions in the hub driver.
>
> Exactly.
Oliver (and perhaps David too) seem keen on this idea. They can explain
their own views...
> > Add new callbacks to struct usb_driver to notify drivers that
> > their device is about to be reset or has just finished a reset.
> > I don't like the name "reset" for a callback as it implies that
> > the driver should reset the device. Maybe better names would
> > be "notify_reset" and "reinit" (or "notify_reinit").
> >
> > As things stand now, we don't handle resets well. A device can have
> > multiple drivers bound to multiple interfaces; when any one of them
> > requests a reset it will affect all the others but they have no way to
> > know what's going on. Likewise, if a hub is reset it automatically has
> > the effect of resetting all the downstream devices, and the drivers need
> > to know about it.
>
> Again, what's wrong with the current way of dropping everything and
> starting over? I think it's the only sane way, and our existing
> interfaces handle it quite well.
Right now we don't do that. If a driver decides to reset a device, the
core doesn't notify any other drivers for that device at all. They don't
get a disconnect(), probe(), or anything. If these new callbacks are
added, drivers that don't implement them will see nothing different from
the way things work now.
> > Design patterns for USB drivers
(Oliver has already convinced me that most of what I wrote here isn't
feasible. The real problem, as he pointed out, is that a driver can be
unbound not only because of a disconnect but also because of an rmmod,
or maybe even by way of usbfs.)
> > There has been a requirement that after a driver's disconnect() routine
> > returns, the driver will not try to access the device at all -- all URBs
> > must be unlinked and the struct usb_device may be deallocated at any time.
>
> No, that is not a requirement at all. All of the usb-serial devices
> violate this "requirement" today. It is perfectly legal to try to
> access a usb device structure after disconnect() is called. That's what
> the reference count is there in the structure for.
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.
> > The core is now (or soon will be) sufficiently well protected that this
> > requirement is no longer needed. When disconnect() is called,
> > usbdev->state will already have been set to USB_STATE_NOTATTACHED so no
> > new URBs will be accepted, and all URBs in flight will have been unlinked.
>
> 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. Nor do we set the interfaces
owned by the exiting driver to altsetting 0; that should be fixed. The
structure of the driver model makes it very hard to see how this can be
done, unfortunately. It might turn out to be easier in the end to install
altsetting 0 immediately before binding drivers rather than immediately
after unbinding them.
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
