On Thursday 29 July 2004 14:55, Alan Stern wrote: > On Thu, 29 Jul 2004, David Brownell wrote: > > > > Our work on the hub driver is well on its way to getting hopelessly > > > tangled. Should we try to coordinate the changes we've been sending in > > > independently? > > > > This looked OK to me with respect to RC2 and to what's in Greg's tree > > (except for some "endpoint_running" detritus in Greg's tree), so I'm > > not quite sure what you mean. > > I'm referring to patches as338 and as340b, which I sent to Greg a few > weeks ago but he hasn't applied yet.
Oh, those! Right now they're not in the trees because of problems they turned up (as I understand things). What I had to do: - Get USB_SUSPEND to work, with locktree(), tested; in use on x86 + ARM before those other patches; - Try merging against those patches; result didn't seem ever quite right and never got tested. (This is where most of those doc/comment updates came from: sorting some issues out.) - Go back to normal Linus tree _without_ those locking changes; tested, it still works. So I guess my coordination proposal is that instead of me trying to coordinate with your locking patches again, it should go the other way around this time. (And anyway, you had seen locktree a few weeks before you did your patches!) I'd like to see the USB_SUSPEND stuff go out in 2.6.8 rather than hang around in MM for very long. It's been around long enough, and that's going to be the only way to start shaking out integration issues with ACPI, PM core, USB drivers, layers that the USB drivers talk to, and even, as you noticed, other usbcore work (not just OTG). > I realize that these delays would be minimal, but I don't see any reason > for imposing them at all. And I don't understand what you mean by "that > kind of signaling". Making usb_reset_device() use locktree won't protect > any downstream devices; instead it will prevent someone from resetting the > device while an upstream hub is locked. Likewise for khubd. Right, the reason that upstream port got locked was because it needed to use non-data signaling downstream of that port, which would interfere with other such uses -- including a RESET. And certainly trying to reset a hub should protect against anyone thinking the downstream devices will just continue as usual! You're right that the khubd code could drop the hub lock in some cases (then re-aquire it before checking the next port) ... except this was just a "minimal changes" patch. All those performance tweaks can be addressed later on, as they're needed. Those kinds of signaling were in the description for locktree(); basically everything that puts non-data signaling through the port (a synch point essentially dictated by the hardware): >> * tree operations like suspend, resume, reset, and disconnect, which > > * apply to everything downstream of a given port. where "disconnect" includes "drop VBUS" as well usb_disconnect(). > They _do_ get disconnected later on. Or rather, the devices that have > already been through usb_new_device() get disconnected. And it's not all > that much later; it's as soon as khubd can wake up and process the > virtual-disconnect notification. I'd have to look again ... but assuming you're right, then that'd be a fine thing to put in comments: non-obvious. > > > Rather than fretting too much about unbinding drivers that don't > > > support suspend(), one could improve things a lot right now by > > > adding suspend/resume support to the HID driver. > > > > That won't address usb-storage, or any of the other drivers, though. > > (I tried to suspend my Swiss Army Knife USB-key, and it misbehaved > > rudely during suspend since something kept probing it through SCSI. > > Really? Exactly how did it misbehave? Numerous SCSI errors showing up in > your system log? See the attachment. READ CAPACITY failing every second or so. Presumably a usb-storage suspend that called SCSI suspend would behave right ... assuming there's a SCSI suspend to talk to! :) > > Though it came back on resume, unlike HID.) Meanwhile we **KNOW** > > every USB driver probe()/disconnect() cycle is basically working now, > > so that the USB parts of the problem have a simple solution. > > > > Thing is, I think we want to go from where we are to an environment > > where USB suspend behaves _without_ having to change every single > > last driver both in-tree and external. Implementing suspend() and > > resume() callbacks should be just to add value; drivers without them > > don't need to act broken. > > That's all true; however it's got to be easier to add suspend/resume to > the HID driver than to fix up the driver-model core. Quicker, anyway. > And adding suspend/resume support to usb-storage will also be very easy. I don't object to teaching drivers how to suspend()/resume() ... at least so long as someone else is doing the work! But I would object to leaving the PM core broken, long-term. Of course, sometimes teaching how to suspend may be tricky. Like Wake-on-LAN from network adapters. We may want to change the driver suspend() to provide a "don't enable remote wakeup" indicator of some kind. It may be simpler to fix the PM core (I actually have an old Pat Mochel patch that does much of that) than address all those types of issue at once! - Dave
kernel: usb 2-2: new full speed USB device using address 3 kernel: usb 2-2: new device strings: Mfr=1, Product=2, SerialNumber=3 kernel: usb 2-2: default language 0x0409 kernel: usb 2-2: Product: Victorinox kernel: usb 2-2: Manufacturer: SWISSBIT kernel: usb 2-2: SerialNumber: 3EBCBBD3171324B3 kernel: usb 2-2: hotplug kernel: usb 2-2: adding 2-2:1.0 (config #1, interface 0) kernel: usb 2-2:1.0: hotplug kernel: usb-storage 2-2:1.0: usb_probe_interface kernel: usb-storage 2-2:1.0: usb_probe_interface - got id kernel: scsi1 : SCSI emulation for USB Mass Storage devices usb.agent[19319]: need a device for this command kernel: Vendor: SWISSBIT Model: Victorinox Rev: 1.89 kernel: Type: Direct-Access ANSI SCSI revision: 02 kernel: sda: Unit Not Ready, sense: kernel: Current : sense = 70 6 kernel: ASC=28 ASCQ= 0 kernel: Raw sense data:0x70 0x00 0x06 0x00 0x00 0x00 0x00 0x0a 0x00 0x00 0x00 0x00 0x28 0x00 0x00 0x00 0x00 0x00 kernel: SCSI device sda: 126720 512-byte hdwr sectors (65 MB) kernel: sda: assuming Write Enabled kernel: sda: assuming drive cache: write through block.agent[19393]: try 1 while waiting for /block/sda's bus_id 1:0:0:0 kernel: sda: sda1 kernel: Attached scsi removable disk sda at scsi1, channel 0, id 0, lun 0 kernel: USB Mass Storage device found at 3 block.agent[19415]: new block device /block/sda/sda1 block.agent[19393]: waiting for /var/lock/block.agent.lock, process 19415 holds it block.agent[19415]: mount by-path/usb-storage-3EBCBBD3171324B3:0:0:0p1 block.agent[19415]: mount: fs type subfs not supported by kernel block.agent[19393]: new block device /block/sda "echo -n 2 > power/state" for the usb device This prints "resume is unsafe" rather than unbinding the driver, since that deadlocks inside pmcore kernel: usb-storage 2-2:1.0: resume is unsafe! kernel: usb 2-2: usb suspend kernel: ohci_hcd 0000:00:02.0: suspend root hub kernel: sda : READ CAPACITY failed. kernel: sda : status=0, message=00, host=7, driver=00 kernel: sda : sense not available. kernel: sda: assuming Write Enabled kernel: sda: assuming drive cache: write through kernel: sda : READ CAPACITY failed. kernel: sda : status=0, message=00, host=7, driver=00 kernel: sda : sense not available. kernel: sda: assuming Write Enabled kernel: sda: assuming drive cache: write through kernel: sda:<3>Buffer I/O error on device sda, logical block 0 kernel: Buffer I/O error on device sda, logical block 0 kernel: unable to read partition table kernel: sda : READ CAPACITY failed. kernel: sda : status=0, message=00, host=7, driver=00 kernel: sda : sense not available. kernel: sda: assuming Write Enabled kernel: sda: assuming drive cache: write through kernel: sda : READ CAPACITY failed. kernel: sda : status=0, message=00, host=7, driver=00 kernel: sda : sense not available. kernel: sda: assuming Write Enabled kernel: sda: assuming drive cache: write through kernel: sda:<3>Buffer I/O error on device sda, logical block 0 kernel: Buffer I/O error on device sda, logical block 0 kernel: unable to read partition table kernel: sda : READ CAPACITY failed. kernel: sda : status=0, message=00, host=7, driver=00 kernel: sda : sense not available. kernel: sda: assuming Write Enabled kernel: sda: assuming drive cache: write through ... repeats ...
