On Tue, 27 Apr 2004, Greg KH wrote:

> So, what's next in this patch series?  :)

Funny you should ask...

While writing those patches I noted a problem, that the USB device tree
can change while a process reading /proc/bus/usb/devices is traversing it,
leading to an oops when a pointer to a no-longer-existing child device is
dereferenced.  The ensuing discussion led to the conclusion that the
devices' ->serialize locks should be acquired, top-down, while going
through the tree.

That means changing the code that populates the devices file and changing
the code that adds and removes USB device structures.  This patch takes
care of the first part.  I'm delaying the second part because that section
of usbcore is still under change -- David Brownell's revisions have not
yet been fully integrated.

A similar change should be made to usb_find_device() and match_device() in
usb.c.  You may want to add that yourself.

Here is a driver for the usb servo controllers from Phidgets
<http://www.phidgets.com/>, using sysfs.

Note that the devices claim to be hid devices, so I've added them to the
hid_blacklist (HID_QUIRK_IGNORE). A servo controller isn't really an hid
device (or is it?).

This is a revised version of an earlier patch; I feel a lot better about
this one.  Basically it does the same thing as before: allocate
interfaces dynamically to avoid the problems with reusing them.

The difference is that this patch adds a struct kref to the array of
usb_interface_cache's, so the array can persist if needed after the
device has been disconnected.  Each interface takes a reference to it
(along with the configuration itself), so as long as the interfaces
remain pinned in memory the altsettings will also remain.

Here is a slight revision of patch as246b.  This one allocates all the new
interfaces before changing any other state; otherwise it's the same.

This is a trivial patch to remove some duplicate includes.  sched.h and
errno.h are already included in this file about a dozen lines or so above
this point.

This patch adds some clear-halt calls if a GetMaxLUN fails.  Apparently,
some devices (like certain early-rev Zip100s) stall their bulk pipes if
they receive a GetMaxLUN.

This is patch as248b from Alan Stern, modified by myself:  This adds a flag
which allows us to supress the "unneeded unusual_devs.h entry" message.
This is useful for times when idiotic device manufacturers break the rules
and release two different devices with the same VID, PID, and revision
number.

Patch as239b from Alan Stern:  This patch improves the interaction between
a SCSI reset, an internally generated reset, and an abort.  This improves
our error-recovery in cases where the device is hung (or almost hung) while
we're trying to auto-reset.

The updates needed for proper altsetting handling among the USB serial
drivers turned out to be a lot easier than I expected, thanks to the
organization of the drivers.  Only a handful of changes were needed.

Be assertive.

there were some missing audits in usb_init()

The attached patch for the 3M Touch Systems Capacitive controller.

Quick list of changes:

        *    Changed reset from standard USB dev reset to vendor reset
        *    Changed data sent to host from compensated to raw coordinates
        *    Eliminated vendor/product module params
        *    Performed multiple successfull tests with an EXII-5010UC

The changes are primarily due to comments from Vojtech Pavlik, as well
as making the newer EXII-50XXUC controllers work.

Thanks to 3M Touch Systems for sending me some new controllers to test with!

An updated HOWTO is also available at:


http://groomlakelabs.com/grandamp/code/microtouch/Linux-Input-USB-Touchscreen-HowTo.txt

The only driver under usb/net that needed any altsetting changes was
usbnet.  I'm not looking forward to going through all the source files
under usb/serial. :-(

This is the altsetting update for the drivers under usb/misc.  As you can,
not much was needed at all.

This patch implements the new altsetting regime for the drivers under
usb/media.  Not much needed to be changed.  I'm unable to test any of the
changes, but at least they compile all right (except that I didn't even
try to compile the pwc driver since it's marked BROKEN).

The stv680 and w9968cf drivers still include an assumption that they are
bound to interface number 0.  Since that the drivers are fairly tightly
linked to a specific kind of device I didn't try to change those
assumptions, but maybe they should be changed.

This patch makes a few minor improvements to the appearance of the UHCI
driver.  Please apply.

Following a suggestion of David Brownell's I have decided to remove
support for the URB_NO_INTERRUPT flag in the UHCI driver.  The overall
effect of the flag is to reduce the number of interrupts, thereby
improving throughput somewhat while increasing the duration of the
remaining IRQ handlers quite a lot (i.e., increasing interrupt variance).
So I think we're better off without it.  Mind you, this is all in the
absence of any firm measurements.

A common case where this will come up is during usb-storage bulk
transfers.  Such transfers are generally divided into scatter-gather
components each corresponding to a single URB and transferring one memory
page (4 KB).  While generating an interrupt for each one is a little
faster than ideal -- about every 3 ms -- it's better than waiting until 64
KB has been transferred and there are 1024 individual TDs to clean up
during the IRQ.

The check of interface->dev.driver requires a lock to be taken
to protect against driver binding changes.  But in fact I think it
is better just to drop the test.  The result is that the caller is
required to claim an interface before changing the altsetting,
which is consistent with the other routines that operate on
interfaces.

 devio.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

Hi Oliver,

> I expect it to rarely matter, but it might matter now and then. It's
> just a question of hygiene. If you are using a temporary buffer I'd
> like to see it used to full advantage. So either drop the lock or do
> a direct copy. I'd prefer the first option your patch implemented.

I agree.  Greg, please consider applying the updated patch:



Protect against driver binding changes while reading the driver name.

The remaining three patches contain miscellaneous fixes to usbfs.
This one fixes up the disconnect callback to only shoot down urbs
on the disconnected interface, and not on all interfaces.  It also adds
a sanity check (this check is pointless because the interface could
never have been claimed in the first place if it failed, but I feel better
having it there).

 devio.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

The semaphore is now taken in the callers.

 devio.c |    2 --
 1 files changed, 2 deletions(-)

The semaphore is now taken in the caller.

 devio.c |    2 --
 1 files changed, 2 deletions(-)

The semaphore is now taken in the caller.

 devio.c |    2 --
 1 files changed, 2 deletions(-)

 devio.c |    3 ---
 1 files changed, 3 deletions(-)

 devio.c		|   43 +++++++++++++++++++++++--------------------
 usbdevice_fs.h	|    1 -
 2 files changed, 23 insertions(+), 21 deletions(-)

Hi Greg, this is the first of a series of patches that replace the
per-file semaphore ps->devsem with the per-device semaphore
ps->dev->serialize.  The role of devsem was to protect against
device disconnection.  This can be done equally well using
ps->dev->serialize.  On the other hand, ps->dev->serialize
protects against configuration and other changes, and has
already been introduced into usbfs in several places.  Using
just one semaphore simplifies the code and removes some
remaining race conditions.  It should also fix the oopses some
people have been seeing.  In this first patch, a reference is
taken to the usb device as long as the usbfs file is open.  That
way we can use ps->dev->serialize for as long as ps exists.

 devio.c |   27 ++++++++++++++++-----------
 inode.c |    3 ---
 2 files changed, 16 insertions(+), 14 deletions(-)

This goes on top of the other enumeration patch I just sent,
to handle some dubious and/or broken hub configurations better.


Make khubd handle some cases better:

 - Track power budget for bus-powered hubs.  This version only warns
   when the budgets are exceeded.  Eventually, the budgets should help
   prevent such errors.

 - Rejects illegal USB setup:  two consecutive bus powered hubs
   would exceed the voltage drop budget, causing much flakiness.

 - For hosts with high speed hubs, warn when devices are hooked up
   to full speed hubs if they'd be faster on a high speed one.

 - For hubs that don't do power switching, don't try to use it

 - For hubs that aren't self-powered, don't report local power status

This is an update to some patches from the December/January
timeframe, which will help sort out some of the mess for
drivers that need to use the reset logic.  It's one of the
last significant patches in my gadget-2.6 tree that haven't
yet been merged into the main kernel tree.


More refactoring of the enumeration code paths:

 * The first half of usb_new_device() becomes the second half of a new
   hub_port_init() routine (resets, sets address, gets descriptor)

 * The middle chunk of hub_port_connect_change() becomes the first half
   of that new hub_port_init() routine.

 * Khubd uses that new routine in hub_port_connect_change().

 * Now usb_new_device() cleans up better after faults, and has
   a more useful locking policy (caller owns dev->serialize).

 * Has related minor cleanups including commenting some of
   the curious request sequences coming from khubd.

Refactoring means a lot of the current usb_reset_device() logic won't
need to stay an imperfect clone of the enumeration code ... soon, it
can just call hub_port_init().

Even without touching usb_reset_device(), this eliminates a deadlock.
Previously, address0_sem was used both during probe and during reset,
so probe routines can't implement DFU firmware download (involves a
reset; DFU also uncovers other problems) or safely recover from probe
faults by resetting (usb-storage can try that).  Now that lock is no
longer held during probe(); so those deadlocks are gone.  (And some
drivers, like at76c503, can start to remove ugly workarounds.)

Handle some SMP-visible unlink states better.

I'm not sure what this piece of code is doing in the UHCI driver.  It
looks like someone envisioned queuing several URBs for the same endpoint
simultaneously.  Anyway, the driver can't do that and this code can never
run.

This patch implements the endpoint_disable method for the UHCI driver, as
you requested a while back.  It guarantees that during unbinding events
(disconnect, configuration change, rmmod) the UHCI driver will have
finished using every URB for the interface being unbound.  It doesn't
quite guarantee that the completion handlers will have finished running,
but it would take a pretty unlikely race to violate that assumption.  (I
think it's the same with the OHCI and EHCI drivers.)

Despite the patch numbering this one applies _after_ as249, which is a
more important bugfix.

On Tue, 20 Apr 2004, Damian Ivereigh wrote:

> Here is the output of dmesg when plugging in an IBM USB MemKey
>
> usb-storage: This device (0a16,8888,0100 S 06 P 50) has unneeded SubClass and Protocol entries in unusual_devs.h
>    Please send a copy of this message to <linux-usb-devel@lists.sourceforge.net>

Thank you for sending this in.  Greg and Pete, here's the patch.

It looks safe to conclude that the unusual_devs.h entries for the Minolta
DiMAGE 7x cameras aren't needed.  (Michael has tested the 7Hi and it's
definitely unnecessary.)  The two other DiMAGE entries probably aren't
needed either, but we don't have any evidence of that so I'm leaving them.

The patch lets HCDs report the root hub remote wakeup feature to usbcore
through config descriptors, and lets usbcore say whether or not remote
wakeup (of host from sleep, by devices) should be enabled.

Both OHCI and UHCI HCDs have some remote wakeup support already; I'm not
too sure how well it works.  Given (separate) patches, their root hubs
can start to act more like other hubs in this area too.  That'll make
it easier to start using USB suspend mode.

In 2.6, ISO transfers on USB require a value for urb->interval ... which
usbfs didn't provide (until this patch), or let user mode drivers specify.

This patch initializes the urb->interval from the endpoint's descriptor,
so ISO transfers should now work from userspace.  It also fixes a related
problem for interrupt transfers.