On Thu, 2011-08-25 at 12:11 -0400, Alan Stern wrote: > On Thu, 25 Aug 2011, Andiry Xu wrote: > > > Current waiting time for warm(BH) reset in hub_port_warm_reset() is too > > short for xHC host to complete the warm reset and report a BH reset > > change. > > > > This patch extends the waiting time for warm reset and merges the function > > into hub_port_reset(), so it can handle both cold reset and warm reset. > > > > This fixes the issue that driver fails to clear BH reset change and port is > > "dead". > > This code looks a little odd. > > > @@ -2046,28 +2047,43 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1, > > if (ret < 0) > > return ret; > > > > - /* Device went away? */ > > - if (!(portstatus & USB_PORT_STAT_CONNECTION)) > > - return -ENOTCONN; > > - > > - /* bomb out completely if the connection bounced */ > > - if ((portchange & USB_PORT_STAT_C_CONNECTION)) > > - return -ENOTCONN; > > - > > - /* if we`ve finished resetting, then break out of the loop */ > > - if (!(portstatus & USB_PORT_STAT_RESET) && > > - (portstatus & USB_PORT_STAT_ENABLE)) { > > - if (hub_is_wusb(hub)) > > - udev->speed = USB_SPEED_WIRELESS; > > - else if (hub_is_superspeed(hub->hdev)) > > - udev->speed = USB_SPEED_SUPER; > > - else if (portstatus & USB_PORT_STAT_HIGH_SPEED) > > - udev->speed = USB_SPEED_HIGH; > > - else if (portstatus & USB_PORT_STAT_LOW_SPEED) > > - udev->speed = USB_SPEED_LOW; > > - else > > - udev->speed = USB_SPEED_FULL; > > - return 0; > > + if (!warm) { > > What's wrong with running all the code above for a warm reset? > The original warm reset code may reset a port with no connection and udev is NULL, so can not run the the code above. I may modify the code to warm reset the port on connection only, but udev is still lacked, and it's meaningless to re-set the udev speed as it's always superspeed. > > + /* Device went away? */ > > + if (!(portstatus & USB_PORT_STAT_CONNECTION)) > > + return -ENOTCONN; > > + > > + /* bomb out completely if the connection bounced */ > > + if ((portchange & USB_PORT_STAT_C_CONNECTION)) > > + return -ENOTCONN; > > + > > + /* if we`ve finished resetting, then break out of > > + * the loop > > + */ > > + if (!(portstatus & USB_PORT_STAT_RESET) && > > + (portstatus & USB_PORT_STAT_ENABLE)) { > > + if (hub_is_wusb(hub)) > > + udev->speed = USB_SPEED_WIRELESS; > > + else if (hub_is_superspeed(hub->hdev)) > > + udev->speed = USB_SPEED_SUPER; > > + else if (portstatus & USB_PORT_STAT_HIGH_SPEED) > > + udev->speed = USB_SPEED_HIGH; > > + else if (portstatus & USB_PORT_STAT_LOW_SPEED) > > + udev->speed = USB_SPEED_LOW; > > + else > > + udev->speed = USB_SPEED_FULL; > > + return 0; > > + } > > + } else { > > + if (portchange & USB_PORT_STAT_C_BH_RESET) { > > + clear_port_feature(hub->hdev, port1, > > + USB_PORT_FEAT_C_BH_PORT_RESET); > > + > > + if (portchange & USB_PORT_STAT_C_LINK_STATE) > > + clear_port_feature(hub->hdev, port1, > > + USB_PORT_FEAT_C_PORT_LINK_STATE); > > + > > Why do you need to clear these features specially for a warm reset? > Can't they be cleared the same way as USB_PORT_FEAT_C_RESET? > The clear PRC code is used for both code and warm reset, while WRC and PLC is for warm reset only, so I put the warm reset specific code here. I will put them along with PRC clear code, but hub_port_reset() is already too fat. > > /* Reset the port */ > > for (i = 0; i < PORT_RESET_TRIES; i++) { > > - status = set_port_feature(hub->hdev, > > - port1, USB_PORT_FEAT_RESET); > > - if (status) > > + if (!warm) > > + status = set_port_feature(hub->hdev, > > + port1, USB_PORT_FEAT_RESET); > > + else > > + status = set_port_feature(hub->hdev, > > + port1, USB_PORT_FEAT_BH_PORT_RESET); > > You could simplify this: > > status = set_port_feature(hub->hdev, port1, (warm ? > USB_PORT_FEAT_BH_PORT_RESET : > USB_PORT_FEAT_RESET)); > OK. > > @@ -2113,9 +2143,13 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > > /* return on disconnect or reset */ > > switch (status) { > > case 0: > > + if (warm) > > + goto clear_reset_change; > > + > > Why use a goto instead of doing: if (!warm) {...}? > Just because there are too many tabs and the code will look ugly to comply with the 80-line rule. I'll use a if block if you want. > > /* TRSTRCY = 10 ms; plus some extra */ > > msleep(10 + 40); > > update_devnum(udev, 0); > > + hcd = bus_to_hcd(udev->bus); > > Why was this line moved? > Because hcd in only used in this if block, and if I don't move the line here, it will show a compile warning like "variable hcd may be used uninitialized". Thanks, Andiry -- 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