Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540408 > unrolled thread

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

Started byMathias Nyman <mathias.nyman@linux.intel.com>
First post2016-12-12 17:00 +0100
Last post2016-12-22 02:50 +0100
Articles 9 on this page of 29 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-12-12 17:00 +0100
    Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2016-12-13 04:30 +0100
      Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-12-19 11:40 +0100
        Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2016-12-19 12:40 +0100
          Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@intel.com> - 2016-12-19 13:20 +0100
            Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2016-12-20 04:30 +0100
            Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Lu Baolu <baolu.lu@linux.intel.com> - 2016-12-20 05:30 +0100
              Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2016-12-20 07:10 +0100
                Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Lu Baolu <baolu.lu@linux.intel.com> - 2016-12-20 07:50 +0100
                  Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2016-12-20 07:50 +0100
                    Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Lu Baolu <baolu.lu@linux.intel.com> - 2016-12-20 08:20 +0100
                      Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2016-12-20 08:40 +0100
                        Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-12-20 16:20 +0100
                          Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2016-12-21 03:30 +0100
                            Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-12-21 14:10 +0100
                              Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2016-12-27 04:10 +0100
                                Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2017-01-02 16:00 +0100
                                  Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang <baolin.wang@linaro.org> - 2017-01-03 07:30 +0100
                          Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Lu Baolu <baolu.lu@linux.intel.com> - 2016-12-21 07:20 +0100
                            Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-12-21 13:50 +0100
                              Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> - 2016-12-21 15:40 +0100
                                Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-12-21 16:10 +0100
                                  Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> - 2016-12-21 16:20 +0100
                                    Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Lu Baolu <baolu.lu@linux.intel.com> - 2016-12-22 02:50 +0100
                                      Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-12-23 14:00 +0100
                              Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Lu Baolu <baolu.lu@linux.intel.com> - 2016-12-22 02:50 +0100
                          Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Lu Baolu <baolu.lu@linux.intel.com> - 2016-12-21 08:00 +0100
                            Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-12-21 14:00 +0100
                              Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command Lu Baolu <baolu.lu@linux.intel.com> - 2016-12-22 02:50 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1545792

FromOGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Date2016-12-21 15:40 +0100
Message-ID<sQQdI-4Cy-23@gated-at.bofh.it>
In reply to#1545747
Mathias Nyman <mathias.nyman@linux.intel.com> writes:

>> Below is the latest code. I put my comments in line.
>>
>>   322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
>>   323 {
>>   324         u64 temp_64;
>>   325         int ret;
>>   326
>>   327         xhci_dbg(xhci, "Abort command ring\n");
>>   328
>>   329         reinit_completion(&xhci->cmd_ring_stop_completion);
>>   330
>>   331         temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>>   332         xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
>>   333                         &xhci->op_regs->cmd_ring);
>>
>> We should hold xhci->lock when we are modifying xhci registers
>> at runtime.
>>
>
> Makes sense, but we need to unlock it before sleeping or waiting for
> completion.  I need to look into that in more detail.
>
> But this was an issue already before these changes.

We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what
is for taking lock for register though, I guess it should be enough just
lock around of read=>write of ->cmd_ring if need lock.

[Rather ->cmd_ring user should check CMD_RING_STATE_ABORTED state.]

> But then again I really like OGAWA Hiroumi's solution that separates the
> command ring stopping from aborting commands and restarting the ring.
>
> The current way of always restarting the command ring as a response to
> a stop command ring command really limits its usage.
>
> So, with this in mind most reasonable would be to
> 1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
> 2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
> 3. remove unnecessary second abort try as a separate patch, send to usb-next
> 4. remove polling for the Command ring running (CRR), waiting for completion
>     is enough, if completion times out then we can check CRR. for usb-next

I think we should check both of CRR and even of stop completion. Because
CRR and stop completion was not same time (both can be first).

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[toc] | [prev] | [next] | [standalone]

#1545824

FromMathias Nyman <mathias.nyman@linux.intel.com>
Date2016-12-21 16:10 +0100
Message-ID<sQQGK-52H-45@gated-at.bofh.it>
In reply to#1545792
On 21.12.2016 16:10, OGAWA Hirofumi wrote:
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>
>>> Below is the latest code. I put my comments in line.
>>>
>>>    322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
>>>    323 {
>>>    324         u64 temp_64;
>>>    325         int ret;
>>>    326
>>>    327         xhci_dbg(xhci, "Abort command ring\n");
>>>    328
>>>    329         reinit_completion(&xhci->cmd_ring_stop_completion);
>>>    330
>>>    331         temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>>>    332         xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
>>>    333                         &xhci->op_regs->cmd_ring);
>>>
>>> We should hold xhci->lock when we are modifying xhci registers
>>> at runtime.
>>>
>>
>> Makes sense, but we need to unlock it before sleeping or waiting for
>> completion.  I need to look into that in more detail.
>>
>> But this was an issue already before these changes.
>
> We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what
> is for taking lock for register though, I guess it should be enough just
> lock around of read=>write of ->cmd_ring if need lock.

After your patch it should be enough to have the lock only while
reading and writing the cmd_ring register.

If we want a locking fix that applies more easily to older stable releases before
your change then the lock needs to cover set CMD_RING_STATE_ABORT, read cmd_reg,
write cmd_reg and busiloop checking CRR bit.  Otherwise the stop cmd ring interrupt
handler may restart the ring just before we start checing CRR. The stop cmd ring
interrupt will set the CMD_RING_STATE_ABORTED to CMD_RING_STATE_RUNNING so
ring will really restart in the interrupt handler.

>
> [Rather ->cmd_ring user should check CMD_RING_STATE_ABORTED state.]
>
>> But then again I really like OGAWA Hiroumi's solution that separates the
>> command ring stopping from aborting commands and restarting the ring.
>>
>> The current way of always restarting the command ring as a response to
>> a stop command ring command really limits its usage.
>>
>> So, with this in mind most reasonable would be to
>> 1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
>> 2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
>> 3. remove unnecessary second abort try as a separate patch, send to usb-next
>> 4. remove polling for the Command ring running (CRR), waiting for completion
>>      is enough, if completion times out then we can check CRR. for usb-next
>
> I think we should check both of CRR and even of stop completion. Because
> CRR and stop completion was not same time (both can be first).

We can keep both, maybe change the order and do the busylooping-checking after
waiting for completion, but thats a optimization for usb-next sometimes later.

Thanks

-Mathias

[toc] | [prev] | [next] | [standalone]

#1545828

FromOGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Date2016-12-21 16:20 +0100
Message-ID<sQQQp-57o-11@gated-at.bofh.it>
In reply to#1545824
Mathias Nyman <mathias.nyman@linux.intel.com> writes:

>> We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what
>> is for taking lock for register though, I guess it should be enough just
>> lock around of read=>write of ->cmd_ring if need lock.
>
> After your patch it should be enough to have the lock only while
> reading and writing the cmd_ring register.
>
> If we want a locking fix that applies more easily to older stable
> releases before your change then the lock needs to cover set
> CMD_RING_STATE_ABORT, read cmd_reg, write cmd_reg and busiloop
> checking CRR bit.  Otherwise the stop cmd ring interrupt handler may
> restart the ring just before we start checing CRR. The stop cmd ring
> interrupt will set the CMD_RING_STATE_ABORTED to
> CMD_RING_STATE_RUNNING so ring will really restart in the interrupt
> handler.

Just for record (no chance to make patch I myself for now, sorry), while
checking locking slightly, I noticed unrelated missing locking.

	xhci_cleanup_command_queue()

We are calling it without locking, but we need to lock for accessing list.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[toc] | [prev] | [next] | [standalone]

#1546123

FromLu Baolu <baolu.lu@linux.intel.com>
Date2016-12-22 02:50 +0100
Message-ID<sR0G5-2H9-15@gated-at.bofh.it>
In reply to#1545828
Hi,

On 12/21/2016 11:18 PM, OGAWA Hirofumi wrote:
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>
>>> We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what
>>> is for taking lock for register though, I guess it should be enough just
>>> lock around of read=>write of ->cmd_ring if need lock.
>> After your patch it should be enough to have the lock only while
>> reading and writing the cmd_ring register.
>>
>> If we want a locking fix that applies more easily to older stable
>> releases before your change then the lock needs to cover set
>> CMD_RING_STATE_ABORT, read cmd_reg, write cmd_reg and busiloop
>> checking CRR bit.  Otherwise the stop cmd ring interrupt handler may
>> restart the ring just before we start checing CRR. The stop cmd ring
>> interrupt will set the CMD_RING_STATE_ABORTED to
>> CMD_RING_STATE_RUNNING so ring will really restart in the interrupt
>> handler.
> Just for record (no chance to make patch I myself for now, sorry), while
> checking locking slightly, I noticed unrelated missing locking.
>
> 	xhci_cleanup_command_queue()
>
> We are calling it without locking, but we need to lock for accessing list.

Yeah. I can make the patch.

Best regards,
Lu Baolu

[toc] | [prev] | [next] | [standalone]

#1546857

FromMathias Nyman <mathias.nyman@linux.intel.com>
Date2016-12-23 14:00 +0100
Message-ID<sRxC1-70g-3@gated-at.bofh.it>
In reply to#1546123
On 22.12.2016 03:46, Lu Baolu wrote:
> Hi,
>
> On 12/21/2016 11:18 PM, OGAWA Hirofumi wrote:
>> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>>
>>>> We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what
>>>> is for taking lock for register though, I guess it should be enough just
>>>> lock around of read=>write of ->cmd_ring if need lock.
>>> After your patch it should be enough to have the lock only while
>>> reading and writing the cmd_ring register.
>>>
>>> If we want a locking fix that applies more easily to older stable
>>> releases before your change then the lock needs to cover set
>>> CMD_RING_STATE_ABORT, read cmd_reg, write cmd_reg and busiloop
>>> checking CRR bit.  Otherwise the stop cmd ring interrupt handler may
>>> restart the ring just before we start checing CRR. The stop cmd ring
>>> interrupt will set the CMD_RING_STATE_ABORTED to
>>> CMD_RING_STATE_RUNNING so ring will really restart in the interrupt
>>> handler.
>> Just for record (no chance to make patch I myself for now, sorry), while
>> checking locking slightly, I noticed unrelated missing locking.
>>
>> 	xhci_cleanup_command_queue()
>>
>> We are calling it without locking, but we need to lock for accessing list.
>
> Yeah. I can make the patch.
>

Force updated timeout_race_fixes branch.

I'll be out for a few days during xmas, continue testing after that

-Mathias

[toc] | [prev] | [next] | [standalone]

#1546121

FromLu Baolu <baolu.lu@linux.intel.com>
Date2016-12-22 02:50 +0100
Message-ID<sR0G5-2H9-13@gated-at.bofh.it>
In reply to#1545747
Hi,

On 12/21/2016 08:48 PM, Mathias Nyman wrote:
> On 21.12.2016 08:17, Lu Baolu wrote:
>> Hi Mathias,
>>
>> I have some comments for the implementation of xhci_abort_cmd_ring() below.
>>
>> On 12/20/2016 11:13 PM, Mathias Nyman wrote:
>>> On 20.12.2016 09:30, Baolin Wang wrote:
>>> ...
>>>
>>> Alright, I gathered all current work related to xhci races and timeouts
>>> and put them into a branch:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes
>>>
>>> Its based on 4.9
>>> It includes a few other patches just to avoid conflicts and  make my life easier
>>>
>>> Interesting patches are:
>>>
>>> ee4eb91 xhci: remove unnecessary check for pending timer
>>> 0cba67d xhci: detect stop endpoint race using pending timer instead of counter.
>>> 4f2535f xhci: Handle command completion and timeout race
>>> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort command
>>> 529a5a0 usb: xhci: fix possible wild pointer
>>> 4766555 xhci: Fix race related to abort operation
>>> de834a3 xhci: Use delayed_work instead of timer for command timeout
>>> 69973b8 Linux 4.9
>>>
>>> The fixes for command queue races will go to usb-linus and stable, the
>>> reworks for stop ep watchdog timer will go to usb-next.
>>>
>>> Still completely untested, (well it compiles)
>>>
>>> Felipe gave instructions how to modify dwc3 driver to timeout on address
>>> devicecommands to test these, I'll try to set that up.
>>>
>>> All additional testing is welcome, especially if you can trigger timeouts
>>> and races
>>>
>>> -Mathias
>>>
>>>
>>
>> Below is the latest code. I put my comments in line.
>>
>>   322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
>>   323 {
>>   324         u64 temp_64;
>>   325         int ret;
>>   326
>>   327         xhci_dbg(xhci, "Abort command ring\n");
>>   328
>>   329         reinit_completion(&xhci->cmd_ring_stop_completion);
>>   330
>>   331         temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>>   332         xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
>>   333                         &xhci->op_regs->cmd_ring);
>>
>> We should hold xhci->lock when we are modifying xhci registers
>> at runtime.
>>
>
> Makes sense, but we need to unlock it before sleeping or waiting for completion.
> I need to look into that in more detail.
>
> But this was an issue already before these changes.
>
>> The retry of setting CMD_RING_ABORT is not necessary according to
>> previous discussion. We have cleaned code for second try in
>> xhci_handle_command_timeout(). Need to clean up here as well.
>>
>
> Yes it can be cleaned up as well, but the two cases are a bit different.
> The cleaned up one was about command ring not starting again after it was stopped.
>
> This second try is a workaround for what we thought was the command ring failing
> to stop in the first place, but is most likely due to the race that OGAWA Hirofumi
> fixed.  It races if the stop command ring interrupt happens between writing the abort
> bit and polling for the ring stopped bit. The interrupt hander may start the command
> ring again, and we would believe we failed to stop it in the first place.
>
> This race could probably be fixed by just extending the lock (and preventing
> interrupts) to cover both writing the abort bit and polling for the command ring
> running bit, as you pointed out here previously.
>
> But then again I really like OGAWA Hiroumi's solution that separates the
> command ring stopping from aborting commands and restarting the ring.
>
> The current way of always restarting the command ring as a response to
> a stop command ring command really limits its usage.
>
> So, with this in mind most reasonable would be to
> 1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
> 2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
> 3. remove unnecessary second abort try as a separate patch, send to usb-next
> 4. remove polling for the Command ring running (CRR), waiting for completion
>    is enough, if completion times out then we can check CRR. for usb-next
>    I'll fix the typos these patches would introduce. Fixing old typos can be done as separate
> patches later.

This is exactly the same as what I am thinking of. I will submit the patches later.

Best regards,
Lu Baolu

[toc] | [prev] | [next] | [standalone]

#1545618

FromLu Baolu <baolu.lu@linux.intel.com>
Date2016-12-21 08:00 +0100
Message-ID<sQJ2x-8nk-5@gated-at.bofh.it>
In reply to#1545192
Hi Mathias,

I have some comments for the implementation of
xhci_handle_command_timeout() as well.

On 12/20/2016 11:13 PM, Mathias Nyman wrote:
> On 20.12.2016 09:30, Baolin Wang wrote:
> ...
>
> Alright, I gathered all current work related to xhci races and timeouts
> and put them into a branch:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes
>
> Its based on 4.9
> It includes a few other patches just to avoid conflicts and  make my life easier
>
> Interesting patches are:
>
> ee4eb91 xhci: remove unnecessary check for pending timer
> 0cba67d xhci: detect stop endpoint race using pending timer instead of counter.
> 4f2535f xhci: Handle command completion and timeout race
> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort command
> 529a5a0 usb: xhci: fix possible wild pointer
> 4766555 xhci: Fix race related to abort operation
> de834a3 xhci: Use delayed_work instead of timer for command timeout
> 69973b8 Linux 4.9
>
> The fixes for command queue races will go to usb-linus and stable, the
> reworks for stop ep watchdog timer will go to usb-next.
>
> Still completely untested, (well it compiles)
>
> Felipe gave instructions how to modify dwc3 driver to timeout on address
> devicecommands to test these, I'll try to set that up.
>
> All additional testing is welcome, especially if you can trigger timeouts
> and races
>
> -Mathias
>
>

I post the code below and add my comments in line.

1276 void xhci_handle_command_timeout(struct work_struct *work)
1277 {
1278         struct xhci_hcd *xhci;
1279         int ret;
1280         unsigned long flags;
1281         u64 hw_ring_state;
1282
1283         xhci = container_of(to_delayed_work(work), struct xhci_hcd, cmd_timer);
1284
1285         spin_lock_irqsave(&xhci->lock, flags);
1286
1287         /*
1288          * If timeout work is pending, or current_cmd is NULL, it means we
1289          * raced with command completion. Command is handled so just return.
1290          */
1291         if (!xhci->current_cmd || delayed_work_pending(&xhci->cmd_timer)) {
1292                 spin_unlock_irqrestore(&xhci->lock, flags);
1293                 return;
1294         }
1295         /* mark this command to be cancelled */
1296         xhci->current_cmd->status = COMP_CMD_ABORT;
1297
1298         /* Make sure command ring is running before aborting it */
1299         hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
1300         if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
1301             (hw_ring_state & CMD_RING_RUNNING))  {
1302                 /* Prevent new doorbell, and start command abort */
1303                 xhci->cmd_ring_state = CMD_RING_STATE_ABORTED;
1304                 spin_unlock_irqrestore(&xhci->lock, flags);
1305                 xhci_dbg(xhci, "Command timeout\n");
1306                 ret = xhci_abort_cmd_ring(xhci);
1307                 if (unlikely(ret == -ESHUTDOWN)) {
1308                         xhci_err(xhci, "Abort command ring failed\n");
1309                         xhci_cleanup_command_queue(xhci);
1310                         usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
1311                         xhci_dbg(xhci, "xHCI host controller is dead.\n");
1312                 }
1313                 return;
1314         }
1315
1316         /* host removed. Bail out */
1317         if (xhci->xhc_state & XHCI_STATE_REMOVING) {
1318                 spin_unlock_irqrestore(&xhci->lock, flags);
1319                 xhci_dbg(xhci, "host removed, ring start fail?\n");
1320                 xhci_cleanup_command_queue(xhci);
1321                 return;
1322         }

I think this part of code should be moved up to line 1295.

1323
1324         /* command timeout on stopped ring, ring can't be aborted */
1325         xhci_dbg(xhci, "Command timeout on stopped ring\n");
1326         xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
1327         spin_unlock_irqrestore(&xhci->lock, flags);

This part of code is tricky. I have no idea about in which case should this
code be executed? Anyway, we shouldn't call xhci_handle_stopped_cmd_ring()
here, right?

1328         return;
1329 }

Best regards,
Lu Baolu

[toc] | [prev] | [next] | [standalone]

#1545749

FromMathias Nyman <mathias.nyman@linux.intel.com>
Date2016-12-21 14:00 +0100
Message-ID<sQOEV-3zl-9@gated-at.bofh.it>
In reply to#1545618
On 21.12.2016 08:57, Lu Baolu wrote:
> Hi Mathias,
>
> I have some comments for the implementation of
> xhci_handle_command_timeout() as well.
>
> On 12/20/2016 11:13 PM, Mathias Nyman wrote:
>> On 20.12.2016 09:30, Baolin Wang wrote:
>> ...
>>
>> Alright, I gathered all current work related to xhci races and timeouts
>> and put them into a branch:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes
>>
>> Its based on 4.9
>> It includes a few other patches just to avoid conflicts and  make my life easier
>>
>> Interesting patches are:
>>
>> ee4eb91 xhci: remove unnecessary check for pending timer
>> 0cba67d xhci: detect stop endpoint race using pending timer instead of counter.
>> 4f2535f xhci: Handle command completion and timeout race
>> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort command
>> 529a5a0 usb: xhci: fix possible wild pointer
>> 4766555 xhci: Fix race related to abort operation
>> de834a3 xhci: Use delayed_work instead of timer for command timeout
>> 69973b8 Linux 4.9
>>
>> The fixes for command queue races will go to usb-linus and stable, the
>> reworks for stop ep watchdog timer will go to usb-next.
>>
>> Still completely untested, (well it compiles)
>>
>> Felipe gave instructions how to modify dwc3 driver to timeout on address
>> devicecommands to test these, I'll try to set that up.
>>
>> All additional testing is welcome, especially if you can trigger timeouts
>> and races
>>
>> -Mathias
>>
>>
>
> I post the code below and add my comments in line.
>
> 1276 void xhci_handle_command_timeout(struct work_struct *work)
> 1277 {
> 1278         struct xhci_hcd *xhci;
> 1279         int ret;
> 1280         unsigned long flags;
> 1281         u64 hw_ring_state;
> 1282
> 1283         xhci = container_of(to_delayed_work(work), struct xhci_hcd, cmd_timer);
> 1284
> 1285         spin_lock_irqsave(&xhci->lock, flags);
> 1286
> 1287         /*
> 1288          * If timeout work is pending, or current_cmd is NULL, it means we
> 1289          * raced with command completion. Command is handled so just return.
> 1290          */
> 1291         if (!xhci->current_cmd || delayed_work_pending(&xhci->cmd_timer)) {
> 1292                 spin_unlock_irqrestore(&xhci->lock, flags);
> 1293                 return;
> 1294         }
> 1295         /* mark this command to be cancelled */
> 1296         xhci->current_cmd->status = COMP_CMD_ABORT;
> 1297
> 1298         /* Make sure command ring is running before aborting it */
> 1299         hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
> 1300         if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
> 1301             (hw_ring_state & CMD_RING_RUNNING))  {
> 1302                 /* Prevent new doorbell, and start command abort */
> 1303                 xhci->cmd_ring_state = CMD_RING_STATE_ABORTED;
> 1304                 spin_unlock_irqrestore(&xhci->lock, flags);
> 1305                 xhci_dbg(xhci, "Command timeout\n");
> 1306                 ret = xhci_abort_cmd_ring(xhci);
> 1307                 if (unlikely(ret == -ESHUTDOWN)) {
> 1308                         xhci_err(xhci, "Abort command ring failed\n");
> 1309                         xhci_cleanup_command_queue(xhci);
> 1310                         usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
> 1311                         xhci_dbg(xhci, "xHCI host controller is dead.\n");
> 1312                 }
> 1313                 return;
> 1314         }
> 1315
> 1316         /* host removed. Bail out */
> 1317         if (xhci->xhc_state & XHCI_STATE_REMOVING) {
> 1318                 spin_unlock_irqrestore(&xhci->lock, flags);
> 1319                 xhci_dbg(xhci, "host removed, ring start fail?\n");
> 1320                 xhci_cleanup_command_queue(xhci);
> 1321                 return;
> 1322         }
>
> I think this part of code should be moved up to line 1295.

The XHCI_STATE_REMOVING and XHCI_STATE_DYING needs a rework,
I'm working on that.

Basically we want XHCI_STATE_REMOVING to mean that  all devices are going,
away and driver will be removed. Don't bother with re-calculating available
bandwidths after every device removal, but do use xhci hardware to disable
devices cleanly etc.

XHCI_STATE_DYING should mean hardware is not working/responding. Don't
bother writing any registers or queuing anything. Just return all
pending and cancelled URBs, notify core we died, and free all allocated memory.

>
> 1323
> 1324         /* command timeout on stopped ring, ring can't be aborted */
> 1325         xhci_dbg(xhci, "Command timeout on stopped ring\n");
> 1326         xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
> 1327         spin_unlock_irqrestore(&xhci->lock, flags);
>
> This part of code is tricky. I have no idea about in which case should this
> code be executed? Anyway, we shouldn't call xhci_handle_stopped_cmd_ring()
> here, right?
>

This isn't changed it these patches.

It will remove the aborted commands and restart the ring. It's useful if we
want to abort a command but command ring was not running. (if for some
unkown reason it was stopped, or forgot to restart.

-Mathias

[toc] | [prev] | [next] | [standalone]

#1546124

FromLu Baolu <baolu.lu@linux.intel.com>
Date2016-12-22 02:50 +0100
Message-ID<sR0G5-2H9-19@gated-at.bofh.it>
In reply to#1545749
Hi,

On 12/21/2016 08:57 PM, Mathias Nyman wrote:
> On 21.12.2016 08:57, Lu Baolu wrote:
>> Hi Mathias,
>>
>> I have some comments for the implementation of
>> xhci_handle_command_timeout() as well.
>>
>> On 12/20/2016 11:13 PM, Mathias Nyman wrote:
>>> On 20.12.2016 09:30, Baolin Wang wrote:
>>> ...
>>>
>>> Alright, I gathered all current work related to xhci races and timeouts
>>> and put them into a branch:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes
>>>
>>> Its based on 4.9
>>> It includes a few other patches just to avoid conflicts and  make my life easier
>>>
>>> Interesting patches are:
>>>
>>> ee4eb91 xhci: remove unnecessary check for pending timer
>>> 0cba67d xhci: detect stop endpoint race using pending timer instead of counter.
>>> 4f2535f xhci: Handle command completion and timeout race
>>> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort command
>>> 529a5a0 usb: xhci: fix possible wild pointer
>>> 4766555 xhci: Fix race related to abort operation
>>> de834a3 xhci: Use delayed_work instead of timer for command timeout
>>> 69973b8 Linux 4.9
>>>
>>> The fixes for command queue races will go to usb-linus and stable, the
>>> reworks for stop ep watchdog timer will go to usb-next.
>>>
>>> Still completely untested, (well it compiles)
>>>
>>> Felipe gave instructions how to modify dwc3 driver to timeout on address
>>> devicecommands to test these, I'll try to set that up.
>>>
>>> All additional testing is welcome, especially if you can trigger timeouts
>>> and races
>>>
>>> -Mathias
>>>
>>>
>>
>> I post the code below and add my comments in line.
>>
>> 1276 void xhci_handle_command_timeout(struct work_struct *work)
>> 1277 {
>> 1278         struct xhci_hcd *xhci;
>> 1279         int ret;
>> 1280         unsigned long flags;
>> 1281         u64 hw_ring_state;
>> 1282
>> 1283         xhci = container_of(to_delayed_work(work), struct xhci_hcd, cmd_timer);
>> 1284
>> 1285         spin_lock_irqsave(&xhci->lock, flags);
>> 1286
>> 1287         /*
>> 1288          * If timeout work is pending, or current_cmd is NULL, it means we
>> 1289          * raced with command completion. Command is handled so just return.
>> 1290          */
>> 1291         if (!xhci->current_cmd || delayed_work_pending(&xhci->cmd_timer)) {
>> 1292                 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1293                 return;
>> 1294         }
>> 1295         /* mark this command to be cancelled */
>> 1296         xhci->current_cmd->status = COMP_CMD_ABORT;
>> 1297
>> 1298         /* Make sure command ring is running before aborting it */
>> 1299         hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>> 1300         if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
>> 1301             (hw_ring_state & CMD_RING_RUNNING))  {
>> 1302                 /* Prevent new doorbell, and start command abort */
>> 1303                 xhci->cmd_ring_state = CMD_RING_STATE_ABORTED;
>> 1304                 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1305                 xhci_dbg(xhci, "Command timeout\n");
>> 1306                 ret = xhci_abort_cmd_ring(xhci);
>> 1307                 if (unlikely(ret == -ESHUTDOWN)) {
>> 1308                         xhci_err(xhci, "Abort command ring failed\n");
>> 1309                         xhci_cleanup_command_queue(xhci);
>> 1310                         usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
>> 1311                         xhci_dbg(xhci, "xHCI host controller is dead.\n");
>> 1312                 }
>> 1313                 return;
>> 1314         }
>> 1315
>> 1316         /* host removed. Bail out */
>> 1317         if (xhci->xhc_state & XHCI_STATE_REMOVING) {
>> 1318                 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1319                 xhci_dbg(xhci, "host removed, ring start fail?\n");
>> 1320                 xhci_cleanup_command_queue(xhci);
>> 1321                 return;
>> 1322         }
>>
>> I think this part of code should be moved up to line 1295.
>
> The XHCI_STATE_REMOVING and XHCI_STATE_DYING needs a rework,
> I'm working on that.
>
> Basically we want XHCI_STATE_REMOVING to mean that  all devices are going,
> away and driver will be removed. Don't bother with re-calculating available
> bandwidths after every device removal, but do use xhci hardware to disable
> devices cleanly etc.
>
> XHCI_STATE_DYING should mean hardware is not working/responding. Don't
> bother writing any registers or queuing anything. Just return all
> pending and cancelled URBs, notify core we died, and free all allocated memory.

Okay, thanks for the information.

>
>>
>> 1323
>> 1324         /* command timeout on stopped ring, ring can't be aborted */
>> 1325         xhci_dbg(xhci, "Command timeout on stopped ring\n");
>> 1326         xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
>> 1327         spin_unlock_irqrestore(&xhci->lock, flags);
>>
>> This part of code is tricky. I have no idea about in which case should this
>> code be executed? Anyway, we shouldn't call xhci_handle_stopped_cmd_ring()
>> here, right?
>>
>
> This isn't changed it these patches.
>
> It will remove the aborted commands and restart the ring. It's useful if we
> want to abort a command but command ring was not running. (if for some
> unkown reason it was stopped, or forgot to restart.

Make sense.

So how about put a warning (instead of a debug message which will normally
be ignored) here?

Best regards,
Lu Baolu

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web