On Thu, 21 May 2015, Robert Schlabbach wrote:
> 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.
On closer inspection, the situation looks rather mixed up.
hub_port_finish_reset() does two types of things:
Things that need to happen after each reset in a retry
sequence (like clearing USB_PORT_FEAT_C_RESET);
Things that need to happen after the entire retry sequence
is over (like the TRSTRCY delay and calling
usb_set_device_state).
These shouldn't all be in the same subroutine. Maybe the first class
of actions should be moved into hub_port_wait_reset. Or maybe the
second class should be moved into hub_port_reset.
> 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:
Kernel hackers always use the unified diff format. We are so used it
that we can't even read the old format. :-)
> 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?
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.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
