On Mon, Oct 28, 2013 at 06:49:37PM -0700, Dan Williams wrote: > 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 I think the original agreement when the USB 2.0 port power off work went in was that hotplug vs hardwired shouldn't make a difference in the kernel. It would be up to userspace to read the port connection type, and decide whether to enable port power off. What in-kernel policy were you thinking of? > - 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). The USB 3.0 specification does provide a way to uniquely identify a USB 3.0 hub and its paired USB 2.0 hub. That's in the Container ID BOS descriptor described in section 9.6.2.3. Each USB hub manufactured by a vendor is supposed to provide a unique ID in order for the OS to pair the USB 2.0 and USB 3.0 hubs. However, just like the Device descriptor serial number, very few USB 3.0 hubs actually set the Container ID. They have the BOS descriptor (since they need it to pass compliance testing), but they set it to all zeros or 1234567890... It's basically useless. > 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 I don't think we should try to coordinate pairing USB 2.0 and USB 3.0 ports on external hubs that aren't described by ACPI. I think there's far too many corner cases, especially with the introduction of a tier mismatch, either at the roothub or within another USB 3.0 hub. I have a USB 3.0 hub (not with me right now), which has either three or four USB 3.0 ports exposed, and seven USB 2.0 ports. The OS sees it as one USB 3.0 hub and two USB 2.0 hubs chained together. My guess is that the manufacturer wanted to say they had a "seven port USB 3.0 hub", but only four-port USB 3.0 hub chips were available, and they didn't have room in the hardware design or didn't want to pay for the second USB 3.0 hub chip. There's no real way to tell how port power is routed across the ports without doing some physical layout tests. I could see how they could meet the requirement in section 10.3.3 by pairing the USB 3.0 ports with either USB 2.0 hub. If they wanted to attach to the parent hub, they could physically expose three USB 3.0 ports, leave one port unexposed and not connected to any physical wires, and say the USB 3.0 hub has four ports. That ensures compatibility with section 10.3.3 while still allowing the USB 2.0 port to be used by the second-tier hub without exposing a USB 3.0-only port. However, that's technically violating the spirit of section, 10.3.3, and it's probably easier to pair the USB 3.0 ports with the child USB 2.0 hub. That would also allow them to expose all four USB 3.0 ports. If the USB 3.0 ports are paired with the second-tier hub, I think Alan's algorithm for finding paired external hubs fails. In this case, the peer for the USB 3.0 hub isn't the USB 2.0 hub that's the same depth down in the external topology, but the next hub downstream. Sarah Sharp -- 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
