From: Lu Baolu <baolu.lu@linux.intel.com>
To: Mathias Nyman <mathias.nyman@linux.intel.com>,
Baolin Wang <baolin.wang@linaro.org>
Cc: Mathias Nyman <mathias.nyman@intel.com>,
Greg KH <gregkh@linuxfoundation.org>,
USB <linux-usb@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Mark Brown <broonie@kernel.org>, "Lu, Baolu" <baolu.lu@intel.com>
Subject: Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Date: Wed, 21 Dec 2016 14:17:11 +0800 [thread overview]
Message-ID: <585A1E67.4020902@linux.intel.com> (raw)
In-Reply-To: <58594A9A.10507@linux.intel.com>
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.
334
335 /* Section 4.6.1.2 of xHCI 1.0 spec says software should
336 * time the completion od all xHCI commands, including
s/od/of/g
337 * the Command Abort operation. If software doesn't see
338 * CRR negated in a timely manner (e.g. longer than 5
339 * seconds), then it should assume that the there are
340 * larger problems with the xHC and assert HCRST.
341 */
342 ret = xhci_handshake(&xhci->op_regs->cmd_ring,
343 CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
344 if (ret < 0) {
345 /* we are about to kill xhci, give it one more chance */
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.
346 xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
347 &xhci->op_regs->cmd_ring);
348 udelay(1000);
349 ret = xhci_handshake(&xhci->op_regs->cmd_ring,
350 CMD_RING_RUNNING, 0, 3 * 1000 * 1000);
351 if (ret < 0) {
352 xhci_err(xhci, "Stopped the command ring failed, "
353 "maybe the host is dead\n");
354 xhci->xhc_state |= XHCI_STATE_DYING;
355 xhci_halt(xhci);
356 return -ESHUTDOWN;
357 }
358 }
359 /*
360 * Writing the CMD_RING_ABORT bit should cause a cmd completion event,
361 * however on some host hw the CMD_RING_RUNNING bit is correctly cleared
362 * but the completion event in never sent. Wait 2 secs (arbitrary
s/in never sent/is never sent/g
363 * number) to handle those cases after negation of CMD_RING_RUNNING.
364 */
This should be implemented with a quirk bit. It's not common for all host controllers.
365 if (!wait_for_completion_timeout(&xhci->cmd_ring_stop_completion,
366 2 * HZ)) {
367 xhci_dbg(xhci, "No stop event for abort, ring start fail?\n");
368 xhci_cleanup_command_queue(xhci);
369 } else {
370 unsigned long flags;
371
372 spin_lock_irqsave(&xhci->lock, flags);
373 xhci_handle_stopped_cmd_ring(xhci, xhci_next_queued_cmd(xhci));
374 spin_unlock_irqrestore(&xhci->lock, flags);
375 }
376 return 0;
377 }
Best regards,
Lu Baolu
next prev parent reply other threads:[~2016-12-21 6:17 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-05 7:51 [PATCH 1/2] usb: host: xhci: Fix possible wild pointer when handling abort command Baolin Wang
2016-12-05 7:51 ` [PATCH 2/2] usb: host: xhci: Handle the right timeout command Baolin Wang
2016-12-12 15:52 ` Mathias Nyman
2016-12-13 3:21 ` Baolin Wang
2016-12-19 10:33 ` Mathias Nyman
2016-12-19 11:34 ` Baolin Wang
2016-12-19 12:13 ` Mathias Nyman
2016-12-20 3:23 ` Baolin Wang
2016-12-20 4:29 ` Lu Baolu
2016-12-20 6:06 ` Baolin Wang
2016-12-20 6:39 ` Lu Baolu
2016-12-20 6:46 ` Baolin Wang
2016-12-20 7:18 ` Lu Baolu
2016-12-20 7:30 ` Baolin Wang
2016-12-20 15:13 ` Mathias Nyman
2016-12-21 2:22 ` Baolin Wang
2016-12-21 13:00 ` Mathias Nyman
2016-12-27 3:07 ` Baolin Wang
2017-01-02 14:57 ` Mathias Nyman
2017-01-03 6:20 ` Baolin Wang
2016-12-21 6:17 ` Lu Baolu [this message]
2016-12-21 12:48 ` Mathias Nyman
2016-12-21 14:10 ` OGAWA Hirofumi
2016-12-21 15:04 ` Mathias Nyman
2016-12-21 15:18 ` OGAWA Hirofumi
2016-12-22 1:46 ` Lu Baolu
2016-12-23 12:54 ` Mathias Nyman
2016-12-22 1:43 ` Lu Baolu
2016-12-21 6:57 ` Lu Baolu
2016-12-21 12:57 ` Mathias Nyman
2016-12-22 1:39 ` Lu Baolu
2016-12-05 14:08 ` [PATCH 1/2] usb: host: xhci: Fix possible wild pointer when handling abort command Mathias Nyman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=585A1E67.4020902@linux.intel.com \
--to=baolu.lu@linux.intel.com \
--cc=baolin.wang@linaro.org \
--cc=baolu.lu@intel.com \
--cc=broonie@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@intel.com \
--cc=mathias.nyman@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.