On Fri, Oct 25, 2013 at 8:23 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> 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?
I think we still need to coordinate 'logically off' even though
'physically off' is handled automatically. For xhci see Appendix D in
the XHCI spec. We might route a USB2 port through a different hub
topology/path than the peer USB3 port, however the platform should
still be managing 'physically off' for us.
>> > 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?
Yes. ACPI identifies connectors by a 'group id' and a 'group token'
(position within the group). When that data is identical for two
ports we join them as a connector. The code also verifies that the
connect_type is identical between the ports and downgrade the
connect_type to 'unknown' if the port data does not agree.
> Even more relevant: How is the power for such ports controlled? By the
> controller or by the integrated hub?
By the platform. We clear USB_PORT_FEAT_POWER in the parent hub and
call usb_acpi_set_power_state() to notify the platform.
>> > 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.
...but at any given point of time a controller has exclusive ownership
of that port, right? 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.
>> >> 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.
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.
>> 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)
>
>> 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!
>
Right, these patches remove that bug.
>> 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.
>> 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.
Not when we explicitly powered off and want the connection to persist
when power is restored.
>> 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.
Yes.
>> >> 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?
Disconnect instead of de-bouncing and persisting the connection when
power is restored.
>> 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.
>
After the event the runtime_status is 'state' and the runtime
usage_count 'count'. A count of 'child' means the child is keeping
the port active via device-model parent-child relationship.
>> 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.
You mean item 1 in my list :-). Although, it's not clear what a
'connector' is until later in the patch series.
>> 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.
>> 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.
>> 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?
No, just reiterating that the port does not suspend unless the child
is allowed to suspend. It's implicit in all cases now that the child
device is actually a child in the device model.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
