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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).