On Fri, Oct 25, 2013 at 2:11 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> All right, I'm starting to get the overall picture.
>
Thanks for the patience I really appreciate it.
> 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.
> 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.
>
I think the missing piece was simply that khubd did not take a
pm_runtime reference on the port. With that in place (and having the
port as the device-model parent of the usb-device) I think we are
good.
>> >> 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.)
>
The case I was concerned about was khubd activated to service a status
change on port0 while port1 is being powered down. That's closed
simply with a pm_runtime reference. I'll add that and delete the
workqueue conversion.
> 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.)
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.
>> > 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.
Hence the RFC, yes, not opposed to backing up and simplifying this.
>> >> 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;
> }
Irk, yes. I need to add that to the new usb_port_runtime_poweroff().
>> >> 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.
I agree. I think a lot of this would become more straightforward if
we could make the connector a device. Then all the connector relevant
policies (power control, forced connect speeds, hotplug policy) could
be specified centrally, but there is no way to make it a parent of a
usb_port. It would end up being a child device of the host controller
which does not seem right either.
--
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
