From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1763005AbcLTHbD (ORCPT ); Tue, 20 Dec 2016 02:31:03 -0500 Received: from mail-oi0-f50.google.com ([209.85.218.50]:35647 "EHLO mail-oi0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762411AbcLTHbA (ORCPT ); Tue, 20 Dec 2016 02:31:00 -0500 MIME-Version: 1.0 In-Reply-To: <5858DB52.8060707@linux.intel.com> References: <0c39bfed1cf6f7b747e702aa841f82c9d2140f27.1480922249.git.baolin.wang@linaro.org> <584EC7B9.6040100@linux.intel.com> <5857B794.2070100@linux.intel.com> <5857CEE4.6040007@intel.com> <5858B3AE.90705@linux.intel.com> <5858D231.7040407@linux.intel.com> <5858DB52.8060707@linux.intel.com> From: Baolin Wang Date: Tue, 20 Dec 2016 15:30:59 +0800 Message-ID: Subject: Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command To: Lu Baolu Cc: Mathias Nyman , Mathias Nyman , Greg KH , USB , LKML , Mark Brown , "Lu, Baolu" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 20 December 2016 at 15:18, Lu Baolu wrote: > Hi, > > On 12/20/2016 02:46 PM, Baolin Wang wrote: >> On 20 December 2016 at 14:39, Lu Baolu wrote: >>> Hi, >>> >>> On 12/20/2016 02:06 PM, Baolin Wang wrote: >>>> Hi, >>>> >>>> On 20 December 2016 at 12:29, Lu Baolu 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 >>>>>>> wrote: >>>>>>>> On 13.12.2016 05:21, Baolin Wang wrote: >>>>>>>>> Hi Mathias, >>>>>>>>> >>>>>>>>> On 12 December 2016 at 23:52, Mathias Nyman >>>>>>>>> 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. >>> In queue_command(), >>> >>> /* if there are no other commands queued we start the timeout timer */ >>> if (list_is_singular(&xhci->cmd_list) && >>> !timer_pending(&xhci->cmd_timer)) { >>> xhci->current_cmd = cmd; >>> mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT); >>> } >>> >>> timer_pending() will return true if the timer is fired, but the function is still >>> running on another CPU. Do I understand it right? >> >From my understanding, if the timer was fired, no matter the timeout >> function is running or finished, timer_pending() will return false. >> Please correct me if I made mistakes. Thanks. > > Just looked into kernel/time/timer.c. You are right. > > The pending is cleared in expire_timers() before call the timer function. > > Base on this fact, we don't need to check timer_pending() at all in below code. Indeed, I think so too. > > /* if there are no other commands queued we start the timeout timer */ > if (xhci->cmd_list.next == &cmd->cmd_list && > !timer_pending(&xhci->cmd_timer)) { > xhci->current_cmd = cmd; > mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT); > } > > > Best regards, > Lu Baolu > >> >>> Best regards, >>> Lu Baolu >>> >>>>> - 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