All right, I'm starting to get the overall picture.
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.
On Fri, 25 Oct 2013, Dan Williams wrote:
> >> > Why treat "companion" ports differently?
> >> >
> >>
> >> Companion-ports share 1 physical connection among controllers,
> >> 'connectors' co-locate multiple physical connections in one plug.
> >
> > Yes, I realize that. It doesn't answer the question, though. In both
> > cases we have two usb_port structures representing a single physical
> > plug. This suggests they should be treated the same way.
>
> ...but at any given point of time a controller has exclusive ownership
> of that port, right?
More to the point, at any given time an individual usb_port has
exclusive control of the physical power. Whereas with peered ports,
the physical power is controlled jointly through both usb_ports.
On the other hand, I suspect that powering down a companion port
wouldn't work right without help. Removing bus power would probably
cause the port switch to think the connection had been dropped, so
the port would be switched back to the EHCI controller. In any case,
it's most likely moot. I don't know of any EHCI/companion combinations
that actually can turn off Vbus power.
> We don't need to send a ClearPortFeature through
> each controller, but I wonder if we would need to undo
> usb_acpi_set_power_state() before handing off the port? As it stands
> only xhci will turn off ports via the platform hooks.
I don't think we need to worry about ACPI for companion ports.
> >> Unless I missed it I did not see what prevents the port from being
> >> powered down mid khubd run?
> >
> > It looks like hub_port_connect_change() needs to do a
> > pm_runtime_get_sync on the port... That's an easy fix, though.
>
> I ended up fixing it the other way round. Set the port as suspended
> and flush khubd. Otherwise we're waking up ports unnecessarily right?
> Now that I think about it we can just pm_runtime_get_noresume() each
> port and then check the state.
It's worth first spending a little time thinking about what could cause
khubd to start doing something to a port while it is powered off (or in
the process of being powered off). I can't come up with many
possibilities.
> >> We have the busy_bits check but what does
> >> that synchronize against?
> >
> > It prevents khubd from trying to manage a port when another thread is
> > doing something to it (a reset, for example).
>
> ...yes, but I did not see anything that prevented.
>
> core0 core1
> test_bit(busy_bit)
> set_bit(busy_bit)
There's nothing that directly prevents it, because in practice it
doesn't happen. For example, suppose core1 is doing a port reset.
core0 will have no reason to test busy_bit until it gets a
port-status-change message, and the port status doesn't change until
the reset is complete. (Admittedly, some memory barriers might be
needed in theory -- but a port reset takes long enough that it doesn't
matter.)
I suppose you could imagine a bizarre scenario, like a device getting
unplugged at the same time that it is being reset. In that case it
doesn't really matter what happens, because in the end the device is
gone.
> >> Which arranges for hub_activate() to run on a known powered down port.
> >
> > I'm not sure what you mean by "on a ... port". hub_activate doesn't
> > run on any ports; it affects the entire hub.
>
> I'm referring to when hub_activate hits this powered down port in its port loop.
I still don't understand this point, but for now let's pass over it.
(Note, by the way, that hub_activate does _not_ turn on power to a port
if port->power_is_on isn't set.)
> > All right, this policy explains why you need to keep track of related
> > ports. This should have been mentioned at the very start of your 00/15
> > description -- it appears to be the main reason for doing all this
> > work.
>
> You mean item 1 in my list :-). Although, it's not clear what a
> 'connector' is until later in the patch series.
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.
> >> when 'hardwired' honor pm_qos_no_power_off and
> >> child device runtime state. Hmm, also should block power-down when
> >> the child is configured for remote-wakeup, will add.
> >
> > This is what we do now, right?
>
> No, I don't think we do in the current (pre-patch) code. If the child
> device is configured for remote wakeup and goes into suspend I do not
> see anything that stops its port from powering down, outside of the
> pm_qos_no_power_off setting.
Look near the end of usb_port_suspend():
if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled) {
pm_runtime_put_sync(&port_dev->dev);
port_dev->did_runtime_put = true;
}
> >> portX/power/pm_qos_no_notify_flags: when set don't try to keep
> >> connector peer power policies synchronized. See patch 4 [4]
> >
> > I didn't understand the description of that patch either, but it seems
> > to contain some confusion about the power provided to a port and its
> > peer. The power is either on to both or off to both, because there is
> > only one Vbus pin in the physical plug.
>
> Ok, I was not distinguishing logically off vs physically off. We
> coordinate logical off to prevent unintentionally indicating that a
> downgraded connection is desired. But if userspace *does* want to
> force a certain connection the thought is that it can set this flag
> and then set pm_qos_no_power_off independently for each port. But the
> mechanism in patch 4 is admittedly quite ugly and I'm open to
> suggestions.
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.
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
