Hi Alan, thanks for taking a look.
On Thu, Oct 24, 2013 at 10:25 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 24 Oct 2013, Dan Williams wrote:
>
>> Summary:
>> Address the following issues for port power control:
>> 1/ Port power policy needs 'connector' awareness.
>> 2/ Reliable port power control requires coordination with khubd.
>> 3/ Userspace needs full control, but also help coordinating port
>> power policy.
>>
>> Even with these changes port power control still defaults to disabled
>> (ports are always on). This is 3.14 material.
>
> I find these descriptions rather unclear...
No problem. I have been steeping in this rework for a bit, so it's
good to step back and clarify the approach.
>
>> 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.
> Which disconnects count as "unintended"?
A device connecting to the USB2 port when power is lost on the USB3 port.
> 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.
>> The attached device, provided
>> it is not a hub may, switch from the USB3 connection to USB2.
>
> Are you still talking about unintended disconnects? If you are, how
> can there be an attached device if it has just disconnected? Or are
> you referring to a device attached to the port's peer (whatever that
> is)?
Yes, the naming is confusing. I'm defining a 'connector' as a
physical plug with one or more port members. Sarah and I went back
and forth on a good name for these items. At least the XHCI spec
calls the relationship 'connectors' in Appendix D.
> 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?
>> Introduce a 'connector' object to track ports from different hcds
>> (for example the two hcds provided by XHCI).
>
> Exactly what ports will these "connectors" track? The USB-2 and USB-3
> logical ports that share the same physical port on a USB-3 hub?
Hmm, only the ones defined by ACPI... That's a fairly large hole not
covered in these patches. Of course 'connectors' need to be created
for downstream USB-3 hub ports.
>> 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.
>> Port-connector membership data comes from
>> platorm firmware.
>
> What about port-connector membership data for ports on external hubs?
> They aren't described by platform firmware.
>
Yup, exactly the facepalm realization I made a comment or two back.
That's incremental to this set, will add.
>> 2/ Reliable port power control requires coordination with khubd.
>>
>> 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? We have the busy_bits check but what does
that synchronize against?
We also used to have this usb_port_runtime_resume:
static int usb_port_runtime_resume(struct device *dev)
{...
usb_autopm_get_interface(intf);
...}
Which arranges for hub_activate() to run on a known powered down port.
We would subsequently notice the connection gone and disconnect the
device. I could reliably reproduce this after just a few power
toggles... fixed now. khubd now ignores powered down ports and we no
longer have confusing code like this in hub_power_on():
static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
{...
for (port1 = 1; port1 <= hub->hdev->maxchild; port1++)
usb_hub_set_port_power(hub->hdev, hub, port1,
hub->ports[port1 - 1]->power_is_on);
...}
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(-)
>> 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.
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.
>> 3/ Userspace needs full control, but also help coordinating port
>> power policy.
>>
>> Augment the existing controls portX/power/pm_qos_no_power_off and
>> portX/power/control with connector awareness (i.e. propagate control
>> settings across ports in a connector), but also add policies based
>> on the connector type ('hotplug' ports never power down as compared
>> to 'hardwired').
>
> Isn't this already implemented, at least in part? I thought we
> treated hotpluggable ports differently from hardwired ports.
Not yet no. It was discussed, but never implemented.
Pre-these-patches the implementation updated the 'removable' bit based
on whether firmware said the port was hotplug capable, but that's it.
Again, patch 5 [3] has more details.
>> Even with these changes port power control still defaults to on.
>>
>> When enabled (pm_qos_no_power_off=0) it tries to do the right thing
>> for connectors and hotplug capable ports. At all times userspace
>> can override what acpi has defined (pm_qos_no_notify_flags to stop
>> propagating settings to 'peers', connect_type to override whether
>> the port is hotplug capable, or usbcore.noacpi to turn it all off).
>
> It might help to put somewhere a complete description of all the
> mechanisms involved in port power control, and how they interact,
> together with a description of which items (and attribute files) can be
> changed by userspace.
Yes, I'll move some of the patch descriptions into a documentation file:
Quick summary is:
portX/power/pm_qos_no_power_off: regardless of connect_type never power down
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), 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.
portX/power/pm_qos_no_notify_flags: when set don't try to keep
connector peer power policies synchronized. See patch 4 [4]
portX/<child-usb-device>/power/control: when set to auto the port
runtime state is implicitly controlled by the child driver
[1]: http://marc.info/?l=linux-usb&m=138260014707015&w=2
[2]: http://marc.info/?l=linux-usb&m=138260021107055&w=2
[3]: http://marc.info/?l=linux-usb&m=138260015707025&w=2
[4]: http://marc.info/?l=linux-usb&m=138260015107021&w=2
--
Dan
--
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
