On Tue, Nov 12, 2013 at 3:10 PM, Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> wrote: > 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? The same, but forcing userspace to explicitly override the connection_type to "hardwired" before allowing power off. We're already in the situation where multiple settings (remote wakeup enabled, peer port power state) will keep a port powered. My intent is adding connection_type to that list and making it configurable as another port power_off gate. It takes the implementation in the direction of making it safer to aggressively clear the pm_qos_no_power_off flag on usb ports... not that we would ever go so far as to make pm_qos_no_power_off=0 the default. > >> - 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. > I'm shocked, shocked I tell you. ;-) >> 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. > Hmm, yes, if we need to handle hub-internal tier mismatch I agree we're pretty much out of luck without a firmware table. -- 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
