Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757396AbcLTHTM (ORCPT ); Tue, 20 Dec 2016 02:19:12 -0500 Received: from mga06.intel.com ([134.134.136.31]:16237 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751970AbcLTHSq (ORCPT ); Tue, 20 Dec 2016 02:18:46 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,377,1477983600"; d="scan'208";a="1101879665" Subject: Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command To: Baolin Wang 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> Cc: Mathias Nyman , Mathias Nyman , Greg KH , USB , LKML , Mark Brown , "Lu, Baolu" From: Lu Baolu Message-ID: <5858DB52.8060707@linux.intel.com> Date: Tue, 20 Dec 2016 15:18:42 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10485 Lines: 233 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. /* 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 >>> > >