On Sat, Oct 26, 2013 at 7:40 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Fri, 25 Oct 2013, Dan Williams wrote: >> > 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? No, but that last one can be subdivided into: - implement hotplug vs hardwired policy - consider/handle connector peers in port power policy >> 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. > With the device model change and no longer telling the hub interface device to pm_suspend_ignore_children() the pm subsystem will manage this wake up for us. > 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. > Yes, every flag change triggers a wake up of the port and, as a pre-requisite, its parent hub. >> > 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. Yes, no need to design for the non-existent 3 connection types in a port case now. Another simplification is that a "struct usb_domain" is currently known to never span a controller boundary. So, rather than adding ports to a usb_connector / usb_domain container hierarchy I think we can simply scan the child devices under the xhci device and look for is_usb_port() instances. In the meantime I am still thinking of the approach to reliably link USB3 hub ports to their peers. The spec mandates that they be labelled the same for this purpose (USB3 spec section 10.3.3). However, to determine peer relationships for downstream hubs I think we will need walk the hierarchy back to the root connector and compare paths. Tier mismatch makes it so we can not look at the path back to the root port. Unless there is some unique way to identify a hub? Also need to think about what to do in the case a hub implementation does not label its ports per the spec > >> > 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. All the questions about peering hub ports goes away if the policy on those is forced to hotplug only. I think the patch that allows the policy (connect_type) to be updated from userspace should also take care to error out on hub ports. Follow-on patches can tackle the external hub case. > How much energy do you save by powering down a port when no device is > attached? Yes, that is on my mind as well. I am picking up these fixes from my colleague Tianyu Lan (cc'd), and still need to get a power meter on my development system. -- 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
