On Wed, Mar 26, 2014 at 1:10 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 19 Mar 2014, Dan Williams wrote:
>
>> If a port is powered-off, or in the process of being powered-off, prevent
>> khubd from operating on it. Otherwise, the following sequence of events
>> leading to an unintended disconnect may occur:
>>
>> Events:
>> (0) <set pm_qos_no_poweroff to '0' for port1>
>> (1) hub 2-2:1.0: hub_resume
>> (2) hub 2-2:1.0: port 1: status 0301 change 0000
>> (3) hub 2-2:1.0: state 7 ports 4 chg 0002 evt 0000
>> (4) hub 2-2:1.0: port 1, power off status 0000, change 0000, 12 Mb/s
>> (5) usb 2-2.1: USB disconnect, device number 5
>>
>> Description:
>> (1) hub is resumed before sending a ClearPortFeature request
>> (2) hub_activate() notices the port is connected and sets
>> hub->change_bits for the port
>> (3) hub_events() starts, but at the same time the port suspends
>> (4) hub_connect_change() sees the disabled port and triggers disconnect
>>
>> Most of the code thrash here is just indenting the portions of
>> port_event() that require the port to be runtime pm active.
>>
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>> drivers/usb/core/hub.c | 91 +++++++++++++++++++++++++++---------------------
>> 1 files changed, 51 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index e0c593ea7a0e..10f420626204 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -4761,51 +4761,56 @@ static void port_event(struct usb_hub *hub, int port1)
>> USB_PORT_FEAT_C_PORT_CONFIG_ERROR);
>> }
>>
>> - if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange))
>> - connect_change = 1;
>> -
>> - /*
>> - * Warm reset a USB3 protocol port if it's in
>> - * SS.Inactive state.
>> - */
>> - if (hub_port_warm_reset_required(hub, portstatus)) {
>> - int status;
>> + /* take port actions that require the port to be powered on */
>> + if (pm_runtime_active(&port_dev->dev)) {
>
> How about avoiding all the churn by doing this?
>
> + /* The following actions aren't needed if the port is powered off */
> + if (!pm_runtime_active(&port_dev->dev))
> + return;
> +
Done.
>
>> @@ -4892,11 +4897,17 @@ static void hub_events(void)
>>
>> /* deal with port status changes */
>> for (i = 1; i <= hdev->maxchild; i++) {
>> + struct usb_port *port_dev = hub->ports[i - 1];
>> +
>> if (!test_bit(i, hub->busy_bits)
>> && (test_and_clear_bit(i, hub->event_bits)
>> || test_bit(i, hub->change_bits)
>> - || test_bit(i, hub->wakeup_bits)))
>> + || test_bit(i, hub->wakeup_bits))) {
>
> Please add a comment here, explaining that this is to prevent any
> runtime suspends from powering-off the port while we're handling the
> events.
>
Added:
/*
* The get_noresume and barrier ensures that if
* the port was in the process of resuming we
* flush that work and keep the port active for
* the duration of the port_event(). However,
* if the port is runtime pm suspended
* (powered-off), we leave it in that state, run
* an abbreviated port_event(), and move on.
*/
--
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
