On Thu, 24 Oct 2013, Dan Williams wrote:
> >> Details:
> >> 1/ Port power policy needs 'connector' awareness.
> >>
> >> It is a recipe for unintended device disconnects to turn off a
> >> port while leaving its peer active.
> >
> > "It is a recipe" -- what is "it"? Do you mean that under the current
> > implementation, an unintended device disconnect turns off a port while
> > leaving the port's peer turned on? Or do you mean that this series of
> > patches creates a mechanism for doing this?
>
> The current implementation is unaware of cases where it is turning off
> a USB3 port that has a related USB2 port. These patches create
> mechanisms to turn off both ports in concert.
Why is that necessary?
Consider an external hub. The USB-3 spec says that the hub is allowed
to remove power from a port only if both the SuperSpeed and USB-2
port-power features are set to 0. Assuming xhci-hcd is made to do the
same for root-hub ports, what's wrong with the kernel treating related
ports independently?
> > What is a port's "peer"?
>
> See patch 2 [1]. For example ACPI enumerates when a USB2 port is
> peered (colocated in the same physical plug) with a USB3 for the XHCI
> controller.
Sarah has mentioned that some systems may do weird things like hook the
USB-3 wires in a port directly to a controller while routing the USB-2
wires through an integrated hub (a "tier mismatch"). Can the ACPI data
represent this sort of thing?
Even more relevant: How is the power for such ports controlled? By the
controller or by the integrated hub?
> > Note that most USB-3 devices are not allowed to connect to the both
> > USB-3 and USB-2 buses at the same time. Hubs are exceptions; they are
> > required to do so (and consequently each USB-3 hub appears to the
> > system as two separate and unrelated hubs; one that is USB-3 and one
> > that is USB-2).
>
> I special case hubs in the code as devices that are allowed to be
> connected to both ports in a connector simultaneously. There will be
> no peer power management in that case. Are there other devices that
> are expected to dual connect?
According to the USB-3 spec, it is not allowed.
> >> A 'connector' is
> >> distinct from 'companion' ports which share the same data lines
> >> across multiple hcds.
> >
> > 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.
> >> The existing implementation makes attempts to mitigate the damage of
> >> khubd running in the middle of a port power control event, but
> >> makes no guarantees.
> >
> > Sure it does. What damage are you talking about?
>
> 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.
> 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).
> We also used to have this usb_port_runtime_resume:
>
> static int usb_port_runtime_resume(struct device *dev)
> {...
> usb_autopm_get_interface(intf);
> ...}
What do you mean, "used to"? That code is there right now!
> 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.
> We would subsequently notice the connection gone and disconnect the
> device. I could reliably reproduce this after just a few power
> toggles... fixed now.
Fixed what? Calling usb_disconnect when the connection is gone is the
right thing to do.
> Now the policy is explicit that khubd only touches ports that have a
> power policy set to 'on', or are otherwise runtime active. No more
> pm_suspend_ignore_children() for the hub device. This makes hub.c a
> bit less complicated and the stats seem to agree:
>
> drivers/usb/core/hub.c | 252 +++++++++++++-----------------------------------
> drivers/usb/core/hub.h | 29 +++---
> 2 files changed, 85 insertions(+), 196 deletions(-)
Okay, so you are changing what it means for a port to be in runtime
suspend. In the current code, it means the port is powered down. If I
understand all this, your intention is to leave a port in runtime
suspend essentially whenever the attached device is suspended (or there
is no attached device). Then you have to decide whether or not to
power-down the port while suspending it.
> >> We need to support both suspending hubs with
> >> live ports (to receive wakeup events) and fully powering down the
> >> port when userspace policy allows.
> >
> > Don't we support this already?
> >
>
> The code tries to, yes. The usb_port_runtime_resume() issue above is
> one bug I hit.
What's the bug?
> When patch 6 [2] moved the port into its more natural place in the
> device model hierarchy the code needed to support two 'suspended'
> states for the port. Suspended with power on (to support remote
> wakeup), and suspended with power off. Patch 5 [3] gives more detail.
I couldn't understand the tables in the patch 5 description. There are
columns labelled "power", "state", and "count", but they don't say
whether this means power before or after the event in the left column,
or what is being counted.
> Quick summary is:
> portX/power/pm_qos_no_power_off: regardless of connect_type never power down
Okay. We do this already.
> portX/connect_type: when 'hotplug', prevent port power off (unless
> peer port is connected to a non-hub device, then power down until the
> peer port disconnects),
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.
> 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?
> 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.
> portX/<child-usb-device>/power/control: when set to auto the port
> runtime state is implicitly controlled by the child driver
What does this mean? Are you saying that if
portX/<child-usb-device>/power/control is set to "on" then the port's
runtime state _isn't_ implicitly controlled by the child driver?
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
