Hi, On 20 December 2016 at 12:29, Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote: > Hi Mathias, > > On 12/19/2016 08:13 PM, Mathias Nyman wrote: >> On 19.12.2016 13:34, Baolin Wang wrote: >>> Hi Mathias, >>> >>> On 19 December 2016 at 18:33, Mathias Nyman >>> <mathias.nyman@xxxxxxxxxxxxxxx> wrote: >>>> On 13.12.2016 05:21, Baolin Wang wrote: >>>>> >>>>> Hi Mathias, >>>>> >>>>> On 12 December 2016 at 23:52, Mathias Nyman >>>>> <mathias.nyman@xxxxxxxxxxxxxxx> wrote: >>>>>> >>>>>> On 05.12.2016 09:51, Baolin Wang wrote: >>>>>>> >>>>>>> >>>>>>> If a command event is found on the event ring during an interrupt, >>>>>>> we need to stop the command timer with del_timer(). Since del_timer() >>>>>>> can fail if the timer is running and waiting on the xHCI lock, then >>>>>>> it maybe get the wrong timeout command in xhci_handle_command_timeout() >>>>>>> if host fetched a new command and updated the xhci->current_cmd in >>>>>>> handle_cmd_completion(). For this situation, we need a way to signal >>>>>>> to the command timer that everything is fine and it should exit. >>>>>> >>>>>> >>>>>> >>>>>> Ah, right, this could actually happen. >>>>>> >>>>>>> >>>>>>> >>>>>>> We should introduce a counter (xhci->current_cmd_pending) for the number >>>>>>> of pending commands. If we need to cancel the command timer and >>>>>>> del_timer() >>>>>>> succeeds, we decrement the number of pending commands. If del_timer() >>>>>>> fails, >>>>>>> we leave the number of pending commands alone. >>>>>>> >>>>>>> For handling timeout command, in xhci_handle_command_timeout() we will >>>>>>> check >>>>>>> the counter after decrementing it, if the counter >>>>>>> (xhci->current_cmd_pending) >>>>>>> is 0, which means xhci->current_cmd is the right timeout command. If the >>>>>>> counter (xhci->current_cmd_pending) is greater than 0, which means >>>>>>> current >>>>>>> timeout command has been handled by host and host has fetched new >>>>>>> command >>>>>>> as >>>>>>> xhci->current_cmd, then just return and wait for new current command. >>>>>> >>>>>> >>>>>> >>>>>> A counter like this could work. >>>>>> >>>>>> Writing the abort bit can generate either ABORT+STOP, or just STOP >>>>>> event, this seems to cover both. >>>>>> >>>>>> quick check, case 1: timeout and cmd completion at the same time. >>>>>> >>>>>> cpu1 cpu2 >>>>>> >>>>>> queue_command(first), p++ (=1) >>>>>> queue_command(more), >>>>>> --completion irq fires-- -- timer times out at same time-- >>>>>> handle_cmd_completion() handle_cmd_timeout(),) >>>>>> lock(xhci_lock ) spin_on(xhci_lock) >>>>>> del_timer() fail, p (=1, nochange) >>>>>> cur_cmd = list_next(), p++ (=2) >>>>>> unlock(xhci_lock) >>>>>> lock(xhci_lock) >>>>>> p-- (=1) >>>>>> if (p > 0), exit >>>>>> OK works >>>>>> >>>>>> case 2: normal timeout case with ABORT+STOP, no race. >>>>>> >>>>>> cpu1 cpu2 >>>>>> >>>>>> queue_command(first), p++ (=1) >>>>>> queue_command(more), >>>>>> handle_cmd_timeout() >>>>>> p-- (P=0), don't exit >>>>>> mod_timer(), p++ (P=1) >>>>>> write_abort_bit() >>>>>> handle_cmd_comletion(ABORT) >>>>>> del_timer(), ok, p-- (p = 0) >>>>>> handle_cmd_completion(STOP) >>>>>> del_timer(), fail, (P=0) >>>>>> handle_stopped_cmd_ring() >>>>>> cur_cmd = list_next(), p++ (=1) >>>>>> mod_timer() >>>>>> >>>>>> OK, works, and same for just STOP case, with the only difference that >>>>>> during handle_cmd_completion(STOP) p is decremented (p--) >>>>> >>>>> >>>>> Yes, that's the cases what I want to handle, thanks for your explicit >>>>> explanation. >>>>> >>>> >>>> Gave this some more thought over the weekend, and this implementation >>>> doesn't solve the case when the last command times out and races with the >>>> completion handler: >>>> >>>> cpu1 cpu2 >>>> >>>> queue_command(first), p++ (=1) >>>> --completion irq fires-- -- timer times out at same time-- >>>> handle_cmd_completion() handle_cmd_timeout(),) >>>> lock(xhci_lock ) spin_on(xhci_lock) >>>> del_timer() fail, p (=1, nochange) >>>> no more commands, P (=1, nochange) >>>> unlock(xhci_lock) >>>> lock(xhci_lock) >>>> p-- (=0) >>>> p == 0, continue, even if we should >>>> not. >>>> For this we still need to rely on >>>> checking cur_cmd == NULL in the timeout function. >>>> (Baolus patch sets it to NULL if there are no more commands pending) >>> >>> As I pointed out in patch 1 of this patchset, this patchset is based >>> on Lu Baolu's new fix patch: >>> usb: xhci: fix possible wild pointer >>> https://www.spinics.net/lists/linux-usb/msg150219.html >>> >>> After applying Baolu's patch, after decrement the counter, we will >>> check the xhci->cur_command if is NULL. So in this situation: >>> cpu1 cpu2 >>> >>> queue_command(first), p++ (=1) >>> --completion irq fires-- -- timer times out at same time-- >>> handle_cmd_completion() handle_cmd_timeout(),) >>> lock(xhci_lock ) spin_on(xhci_lock) >>> del_timer() fail, p (=1, nochange) >>> no more commands, P (=1, nochange) >>> unlock(xhci_lock) >>> lock(xhci_lock) >>> p-- (=0) >>> no current command, return >>> if (!xhci->current_cmd) { >>> unlock(xhci_lock); >>> return; >>> } >>> >>> It can work. >> >> Yes, >> >> What I wanted to say is that as it relies on Baolus patch for that one case >> it seems that patch 2/2 can be replaced by a single line change: >> >> if (!xhci->current_cmd || timer_pending(&xhci->cmd_timer)) >> >> Right? >> >> -Mathias >> > > It seems that the watch dog algorithm for command queue becomes > more and more complicated and hard for maintain. I am also seeing > another case where a command may lose the chance to be tracked by > the watch dog timer. > > Say, > > queue_command(the only command in queue) > - completion irq fires-- - timer times out at same time-- - another command enqueue-- > - lock(xhci_lock ) - spin_on(xhci_lock) - spin_on(xhci_lock) > - del_timer() fail > - free the command and > set current_cmd to NULL > - unlock(xhci_lock) > - lock(xhci_lock) > - queue_command()(timer will > not rescheduled since the timer > is pending) In this case, since the command timer was fired and you did not re-add the command timer, why here timer is pending? Maybe I missed something? Thanks. > - lock(xhci_lock) > - no current command > - return > > As the result, the later command isn't under track of the watch dog. > If hardware fails to response to this command, kernel will hang in > the thread which is waiting for the completion of the command. > > I can write a patch to fix this and cc stable kernel as well. For long > term, in order to make it simple and easy to maintain, how about > allocating a watch dog timer for each command? It could be part > of the command structure and be managed just like the life cycle > of a command structure. > > I can write a patch for review and discussion, if you think this > change is possible. > > Best regards, > Lu Baolu -- Baolin.wang Best Regards -- 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
