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
