From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934796AbcLUOgw (ORCPT ); Wed, 21 Dec 2016 09:36:52 -0500 Received: from mail.parknet.co.jp ([210.171.160.6]:43685 "EHLO mail.parknet.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751506AbcLUOgu (ORCPT ); Wed, 21 Dec 2016 09:36:50 -0500 X-Greylist: delayed 1580 seconds by postgrey-1.27 at vger.kernel.org; Wed, 21 Dec 2016 09:36:49 EST From: OGAWA Hirofumi To: Mathias Nyman Cc: Lu Baolu , Baolin Wang , Mathias Nyman , Greg KH , USB , LKML , Mark Brown , "Lu\, Baolu" , Alan Stern Subject: Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command 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> <5858DB52.8060707@linux.intel.com> <58594A9A.10507@linux.intel.com> <585A1E67.4020902@linux.intel.com> <585A7A0A.6090401@linux.intel.com> Date: Wed, 21 Dec 2016 23:10:22 +0900 In-Reply-To: <585A7A0A.6090401@linux.intel.com> (Mathias Nyman's message of "Wed, 21 Dec 2016 14:48:10 +0200") Message-ID: <87tw9xv0dt.fsf@mail.parknet.co.jp> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Mathias Nyman 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