From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933389AbcLUPDw (ORCPT ); Wed, 21 Dec 2016 10:03:52 -0500 Received: from mga03.intel.com ([134.134.136.65]:9520 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756409AbcLUPDu (ORCPT ); Wed, 21 Dec 2016 10:03:50 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,384,1477983600"; d="scan'208";a="1085081240" Subject: Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command To: OGAWA Hirofumi 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> <87tw9xv0dt.fsf@mail.parknet.co.jp> Cc: Lu Baolu , Baolin Wang , Mathias Nyman , Greg KH , USB , LKML , Mark Brown , "Lu, Baolu" , Alan Stern From: Mathias Nyman Message-ID: <585A9A0F.4000603@linux.intel.com> Date: Wed, 21 Dec 2016 17:04:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <87tw9xv0dt.fsf@mail.parknet.co.jp> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 21.12.2016 16:10, OGAWA Hirofumi wrote: > 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. 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