On Fri, 25 Oct 2013, Dan Williams wrote: > On Fri, Oct 25, 2013 at 2:11 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > All right, I'm starting to get the overall picture. > > > > Thanks for the patience I really appreciate it. You're welcome. > > This patch set makes a large number of significant changes to important > > and subtle aspects of the USB stack. It would be a lot easier to > > discuss in pieces; I can't possibly review the whole series in a > > reasonable time. > > > > And I wish the basic approach had been discussed beforehand... Some of > > the ideas are quite nice, but there are a lot of details that need > > close attention. > > I wanted to at least have a working implementation that tackled the > peer disconnect case, I did not expect 15 patches to fall out of that. > However, after talking through this I think it can be abbreviated. Yes. It looks like we're talking about several major changes: Redefining what it means for a port to be in runtime suspend. Rearranging the device model tree. Implementing connectors. Changing the port-power-off policy. The patch set also changes khubd into a workqueue, but now it looks like we don't need to do that (although I'm not ruling it out as a future clean-up). Does this leave anything out? > In the existing code at usb_port_runtime_resume() time we know the > port is off. By calling usb_autopm_get_interface(intf) before turning > on the power we are simply causing an unnecessary check of the port > status registers. We do have the power_is_on check to block > hub_activate() from setting the change_bits, but that was not enough > to prevent khubd from triggering a disconnect. In fact, the reason for calling usb_autopm_get_interface was to prevent the hub from being suspended while we change the port's power state. Something like this may still be needed. Have you considered what would happen if the user changed the port's pm_qos setting while the port was suspended? Ideally, such changes should immediately affect the port's power state. For example, if the port was suspended with power off when the user sets the no_power_off flag, we should immediately turn port power back on. I don't know if the PM QoS design permits this sort of thing, though. > > You should also explain at the start of the description what you mean > > by related (or peer) ports. Then the idea of a connector is clear -- > > although I got the impression that the implementation may be a little > > over-engineered. > > Hence the RFC, yes, not opposed to backing up and simplifying this. Since a port never needs to be connected to more than one other port, you could implement connectors simply by storing a "peer" pointer in the usb_port structure. > > Is this intended to be a way for the user to force a USB-3 device to > > connect at high speed rather than SuperSpeed? If it is, it seems like > > a rather roundabout way of doing so. > > I agree. I think a lot of this would become more straightforward if > we could make the connector a device. Then all the connector relevant > policies (power control, forced connect speeds, hotplug policy) could > be specified centrally, but there is no way to make it a parent of a > usb_port. It would end up being a child device of the host controller > which does not seem right either. My feeling is that we shouldn't worry about most of those things. The only critical item is power policy, in particular, whether to power-down a hardwired port. The question about powering down a hotpluggable port can be left for the future. How much energy do you save by powering down a port when no device is attached? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
