On Fri, Mar 08, 2013 at 09:15:07PM -0500, Alan Stern wrote: > On Fri, 8 Mar 2013, Sarah Sharp wrote: > > > On Fri, Mar 08, 2013 at 11:14:38AM -0500, Alan Stern wrote: > > > On Thu, 7 Mar 2013, Sarah Sharp wrote: > > > You might want to go farther than this. I haven't read the driver code > > > enough to be sure how you have this organized, but it seems logical > > > that you shouldn't set a command's expiration time until that command > > > begins executing. > > > > > > Suppose the ring contains commands A and B and you set B's expiration > > > time while A is still running. Then A could continue running until B's > > > time is almost up. When B does start, the watchdog timer will expire > > > before B has a chance to do anything. > > > > Yes, it's true. With this patch, the watchdog timer will re-queue > > itself, of course. > > No; it will see that B is the currently executing command and either > cancel B or conclude that the host is dead. Just like the behavior > without this patch, except that the window of opportunity is smaller. Well, yes, I'm not sure if it's possible to completely eliminate that small window of opportunity. Even if only the currently executing command were timed, the host could start processing it at just the moment that the timer fired. We just give the host exactly the same amount of slack if we only time the currently executing command. > > I do agree that it's inefficient to have multiple > > timers running when we could only have one timer running. > > > > > By not setting a command's expiration time until that command begins > > > running, you remove the need for the check added by this patch. It > > > will not be possible for the command's watchdog timer to expire before > > > the command becomes the one currently executing. > > > > In general, all the commands wait (interruptibly) on a completion, with > > a 5 second timeout. However, the Stop Endpoint command doesn't have a > > completion, since it's part of the asynchronous call to cancel an URB. > > The Stop Endpoint command is the only one with a watchdog timer. > > > > I started out with an idea to yours, of having a global command list. > > Commands would be queued to the FIFO, submitted to the command ring, and > > then their submitters would call wait_for_completion_interruptible > > (without a timeout), or simply return, in the case of the Stop Endpoint > > command. The watchdog timer routine would be gutted to be a global > > command queue manager that timed the currently executing command, and > > would cancel it if necessary. > > > > In the end, I decided it would be better for the stable kernels to keep > > to the simpler (if less efficient) solution. But I would like to do the > > "right thing" later on. > > Would it be simpler to keep the existing implementation, but start the > timer for a Stop Endpoint command in the completion handler for the > previous command? (But then what happens if there are two Stop > Endpoint commands in a row?) It depends on if the canceled URB is on the same endpoint, or different endpoints. Each endpoint has a separate timer. If a Stop Endpoint is in flight for the same endpoint, the driver doesn't start the timer again, it just increments the number of in-flight Stop Endpoint commands for that endpoint. Two Stop Endpoint commands could be queued for different endpoints. If I were to add the code to only start the timer for the Stop Endpoint command in the completion handler for the previous command, I would probably just move to only timing the currently executing command. I'll look into it and see how much code is. Sarah Sharp -- 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
