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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
