On Fri, 22 May 2015, Robert Schlabbach wrote:
> 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().
Okay.
> 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;
So you removed this goto. Fair enough; it's probably not useful.
> -
> - 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;
I don't think this line is necessary, because status gets overwritten
two lines below.
> 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);
> + }
Strictly speaking, these lines should be executed only if status is 0
or -ENOTCONN or -ENODEV. Which means this new material belongs inside
an "else {}" block for the preceding "if" statement.
> }
>
> /* 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.
This looks a lot better. Remember to run your changes through
scripts/checkpatch.pl, and follow the instructions in
Documentation/SubmittingPatches when you submit it.
Alan Stern
--
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
