On Thu, 21 May 2015, Alan Stern wrote:
> I suspect you have not divided up all the actions in the correct way.
> Look at everything in hub_port_finish_reset and think carefully about
> where each one belongs.
I had taken a very "conservative" approach to only minimally alter the
behavior specifically for the fix I needed, since I understand too little
of the code to know what I might break if I made bolder changes.
Now following "bold" approach, I went all the way and dissolved
hub_port_finish_reset() completely, moving all of its actions over to
hub_port_reset(). Moving the clearing of the port change bits over to
hub_port_wait_reset() seemed impractical to me, since that has many
return statements in it, and the clearing must be done in every case, so
it's simpler to put this in hub_port_reset() after hub_port_wait_reset()
has returned.
While at it, I also changed the error handling for the initial "double
check" and removed an unneeded advance declaration of hub_port_reset().
The resulting unified (!:-)) diff patch versus 3.19.8 is:
diff -u -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-22 08:15:37.199743870 +0200
@@ -2616,9 +2616,6 @@
return USE_NEW_SCHEME(retry);
}
-static int hub_port_reset(struct usb_hub *hub, int port1,
- struct usb_device *udev, unsigned int delay, bool warm);
-
/* Is a USB 3.0 port in the Inactive or Compliance Mode state?
* Port worm reset is required to recover
*/
@@ -2706,44 +2703,6 @@
return 0;
}
-static void hub_port_finish_reset(struct usb_hub *hub, int port1,
- struct usb_device *udev, int *status)
-{
- switch (*status) {
- case 0:
- /* TRSTRCY = 10 ms; plus some extra */
- msleep(10 + 40);
- if (udev) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
-
- update_devnum(udev, 0);
- /* The xHC may think the device is already reset,
- * so ignore the status.
- */
- if (hcd->driver->reset_device)
- hcd->driver->reset_device(hcd, udev);
- }
- /* FALL THROUGH */
- case -ENOTCONN:
- case -ENODEV:
- usb_clear_port_feature(hub->hdev,
- port1, USB_PORT_FEAT_C_RESET);
- if (hub_is_superspeed(hub->hdev)) {
- usb_clear_port_feature(hub->hdev, port1,
- USB_PORT_FEAT_C_BH_PORT_RESET);
- usb_clear_port_feature(hub->hdev, port1,
- USB_PORT_FEAT_C_PORT_LINK_STATE);
- 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;
- }
-}
-
/* Handle port reset and port warm(BH) reset (for USB3 protocol ports) */
static int hub_port_reset(struct usb_hub *hub, int port1,
struct usb_device *udev, unsigned int delay, bool warm)
@@ -2767,17 +2726,15 @@
* If the caller hasn't explicitly requested a warm reset,
* double check and see if one is needed.
*/
- status = hub_port_status(hub, port1,
- &portstatus, &portchange);
- if (status < 0)
- goto done;
-
- if (hub_port_warm_reset_required(hub, port1, portstatus))
- warm = true;
+ if (hub_port_status(hub, port1, &portstatus, &portchange) == 0) {
+ if (hub_port_warm_reset_required(hub, port1, portstatus))
+ warm = true;
+ }
}
clear_bit(port1, hub->warm_reset_bits);
/* Reset the port */
+ status = 0;
for (i = 0; i < PORT_RESET_TRIES; i++) {
status = set_port_feature(hub->hdev, port1, (warm ?
USB_PORT_FEAT_BH_PORT_RESET :
@@ -2795,11 +2752,22 @@
dev_dbg(hub->intfdev,
"port_wait_reset: err = %d\n",
status);
+
+ /* clear wPortChange bits which may have been set */
+ usb_clear_port_feature(hub->hdev,
+ port1, USB_PORT_FEAT_C_RESET);
+ if (hub_is_superspeed(hub->hdev)) {
+ usb_clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_BH_PORT_RESET);
+ usb_clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_PORT_LINK_STATE);
+ usb_clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_CONNECTION);
+ }
}
/* Check for disconnect or reset */
if (status == 0 || status == -ENOTCONN || status == -ENODEV) {
- hub_port_finish_reset(hub, port1, udev, &status);
if (!hub_is_superspeed(hub->hdev))
goto done;
@@ -2836,6 +2804,26 @@
dev_err(&port_dev->dev, "Cannot enable. Maybe the USB cable is bad?\n");
done:
+ if (status == 0) {
+ /* TRSTRCY = 10 ms; plus some extra */
+ msleep(10 + 40);
+ if (udev) {
+ struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+
+ update_devnum(udev, 0);
+ /* The xHC may think the device is already reset,
+ * so ignore the status.
+ */
+ if (hcd->driver->reset_device)
+ hcd->driver->reset_device(hcd, udev);
+
+ usb_set_device_state(udev, USB_STATE_DEFAULT);
+ }
+ } else {
+ if (udev)
+ usb_set_device_state(udev, USB_STATE_NOTATTACHED);
+ }
+
if (!hub_is_superspeed(hub->hdev))
up_read(&ehci_cf_port_reset_rwsem);
What do you think of this? It also fixes my problem.
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
