On Tue, 13 Jun 2006, David Brownell wrote: > > 4. Make usb_generic into a genuine device driver. This involves > > adding a mechanism to tell apart USB interface drivers from USB > > device drivers -- of which usb_generic is currently the only one. > > Arguably this is how "hub.c" should work. (Plus as you mentioned local > proxies for remote or virtualzed usb gadgets.)
I don't understand what you mean about hub.c. > But this seems somewhat incomplete, and it bothers me that the driver > for a usb device is using usb_interface entities in its usbcore chitchat. > > Maybe a "usb_device_driver" could be a better virtualization hook too; > its responsibilities will certainly be very different from those of the > current usb_(interface_)_driver structure. Agreed, it should have been done that way. Can't think why I didn't do it originally, but it's easy enough to fix up. However I'm not so sure about going through and changing all the existing occurrences of "usb_driver" to "usb_interface_driver"! > > 8. General cleanup of the suspend/resume code. Duplicated code, > > unnecessary checks, that sort of thing. > > Always a nice thing to have. This patch was pretty large though, > maybe it should become a few smaller patches? Just to highlight > the parts that aren't actually duplicated, etc. And help sort > out the intermediate issues that will surely crop up. :) The patch could be split up. > Glad to see usage of dev.power.power_state start to vanish. When I realized that it was hopeless to use that field for anything very meaningful, I did my best to eliminate all uses of it. It remains in only one place: where the hub_suspend() routine checks to see that all the children devices are already suspended. Checking the device state won't work right when CONFIG_USB_SUSPEND isn't set, and even if it is set, the test would be wrong when the children were merely frozen. > > 9. Tie together suspend/resume operations on a device and on its > > interfaces. > > Another way to put this is that you've reverted some of the changes > from patch seven-of-nine. Any chance of not actually making those > changes in the first place, and thereby simplifying both patches? > Or maybe just re-ordering things so this goes first. Although you might think of this patch in that way, it's not really a reversion. Firstly, the recursion now occurs in a different place (in usb_suspend_both rather than in usb_port_suspend). Secondly, it works by directly calling usb_resume_interface instead of calling the bus's resume method. Thirdly, it affects both resume and suspend. > Along the lines of what we said earlier, I think the "right" approach > would be to have a single sysfs file like usb3/3-1/usb_state rather than > have all of usb3/3-1/power/state, usb3/3-1/3-1:1.0/power/state, and so > forth. That could be defined as "suspended" or "active" (or whatever), > writable from userspace, etc. So when power/state files vanish, a good > solution would be in place (for USB). Comments? :) That's not a bad idea, although right now it would be redundant. Fodder for a later patch... Alan Stern _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
