Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command

From: Lu Baolu
Date: Tue Dec 20 2016 - 02:19:15 EST

Next message: Christoph Hellwig: "Re: [PATCH v3 2/5] lib: Add Sed-opal library"
Previous message: Andrew Jeffery: "[PATCH v4 5/5] mfd: dt: Add bindings for the Aspeed SoC Display Controller (GFX)"
In reply to: Baolin Wang: "Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command"
Next in thread: Baolin Wang: "Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 12/20/2016 02:46 PM, Baolin Wang wrote:
> On 20 December 2016 at 14:39, Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
>> Hi,
>>
>> On 12/20/2016 02:06 PM, Baolin Wang wrote:
>>> 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.
>> 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
>>>
>
>

Next message: Christoph Hellwig: "Re: [PATCH v3 2/5] lib: Add Sed-opal library"
Previous message: Andrew Jeffery: "[PATCH v4 5/5] mfd: dt: Add bindings for the Aspeed SoC Display Controller (GFX)"
In reply to: Baolin Wang: "Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command"
Next in thread: Baolin Wang: "Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]