Thanks for the reply, Alan.
The USB device is initially in link state 0, and only after the "cold"
reset in link state SS.inactive. After the following warm reset, it is
in link state 0 again.
> ... but the code is in the wrong place. The check needs to occur
> before hub_port_finish_reset, not after. It's a bug to call
> hub_port_finish_reset before the retry loop is over; that call should
> happen after the end of the loop.
Looking at the code, I'd say the hub_port_finish_reset() call is actually
in the _right_ place, because that's where we know that a reset was really
initiated (that step could have errored out) and the reset _try_ should be
finished before the next retry.
What is in the wrong place is coincidentally that code fragment at the
end of hub_port_finish_reset() which I had quoted last time:
> if (udev)
> usb_set_device_state(udev, *status
> ? USB_STATE_NOTATTACHED
> : USB_STATE_DEFAULT);
That part should be done after the retry loop, because the device state
should not be set based on the result of an "interim" reset result
(especially not when the setting may be irrevocable), but rather based
on the final reset result.
So I moved this code fragment from hub_port_finish_reset() to the end of
hub_port_reset() and made sure it only gets there when a reset attempt
was actually made. The following diff is based on kernel sources 3.19.8:
diff -c -r a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
*** a/drivers/usb/core/hub.c 2015-05-21 00:11:25.000000000 +0200
--- b/drivers/usb/core/hub.c 2015-05-21 00:19:38.471481393 +0200
***************
*** 2736,2745 ****
usb_clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_CONNECTION);
}
- if (udev)
- usb_set_device_state(udev, *status
- ? USB_STATE_NOTATTACHED
- : USB_STATE_DEFAULT);
break;
}
}
--- 2736,2741 ----
***************
*** 2770,2781 ****
status = hub_port_status(hub, port1,
&portstatus, &portchange);
if (status < 0)
! goto done;
if (hub_port_warm_reset_required(hub, port1, portstatus))
warm = true;
}
clear_bit(port1, hub->warm_reset_bits);
/* Reset the port */
for (i = 0; i < PORT_RESET_TRIES; i++) {
--- 2766,2778 ----
status = hub_port_status(hub, port1,
&portstatus, &portchange);
if (status < 0)
! return status;
if (hub_port_warm_reset_required(hub, port1, portstatus))
warm = true;
}
clear_bit(port1, hub->warm_reset_bits);
+ status = 0;
/* Reset the port */
for (i = 0; i < PORT_RESET_TRIES; i++) {
***************
*** 2836,2841 ****
--- 2833,2843 ----
dev_err(&port_dev->dev, "Cannot enable. Maybe the USB cable is bad?\n");
done:
+ if (udev)
+ usb_set_device_state(udev, status
+ ? USB_STATE_NOTATTACHED
+ : USB_STATE_DEFAULT);
+
if (!hub_is_superspeed(hub->hdev))
up_read(&ehci_cf_port_reset_rwsem);
That also alters the handling of some errors which before the patch caused
hub_port_finish_reset() never to be called and thus the USB device state
not to be altered, but I think it is actually more appropriate to set a
USB device to NOTATTACHED if the reset could not even be initiated, e.g.
because of ENODEV...
It will still leave the USB device state unchanged if it errors out at the
initial "double check" if a warm reset is needed, as the current code
does. It might be more appropriate to just skip the double check rather
than exiting, though...
What do you think, is that an appropriate fix?
Best Regards,
Robert Schlabbach
--
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
