Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758519AbcLAEzC (ORCPT ); Wed, 30 Nov 2016 23:55:02 -0500 Received: from mail-yw0-f174.google.com ([209.85.161.174]:35266 "EHLO mail-yw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754104AbcLAEzA (ORCPT ); Wed, 30 Nov 2016 23:55:00 -0500 MIME-Version: 1.0 In-Reply-To: <583EDD87.8030307@linux.intel.com> References: <613dafc211127a4589306e91e231e151feb5ce80.1480496291.git.baolin.wang@linaro.org> <583EDD87.8030307@linux.intel.com> From: Baolin Wang Date: Thu, 1 Dec 2016 12:54:58 +0800 Message-ID: Subject: Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command timer to watch stop endpoint command To: Mathias Nyman Cc: mathias.nyman@intel.com, Greg KH , USB , LKML , Mark Brown Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2195 Lines: 50 On 30 November 2016 at 22:09, Mathias Nyman wrote: > On 30.11.2016 11:02, Baolin Wang wrote: >> >> If the hardware never responds to the stop endpoint command, the >> URBs will never be completed, and we might hang the USB subsystem. >> The original watchdog timer is used to watch if one stop endpoint >> command is timeout, if timeout, then the watchdog timer will set >> XHCI_STATE_DYING, try to halt the xHCI host, and give back all >> pending URBs. >> >> But now we already have one command timer to control command timeout, >> thus we can also use the command timer to watch the stop endpoint >> command, instead of one duplicate watchdog timer which need to be >> removed. >> >> Meanwhile we don't need the 'stop_cmds_pending' flag to identy if >> this is the last stop endpoint command of one endpoint. Since we >> can make sure we only set one stop endpoint command for one endpoint >> by 'EP_HALT_PENDING' flag in xhci_urb_dequeue() function. Thus remove >> this flag. >> >> We also need to clean up the command queue before trying to halt the >> xHCI host in xhci_stop_endpoint_command_timeout() function. > > > This isn't a bad idea. > > There are anyway some corner cases and details that need to be > checked, such as suspend (which will clear the command queue), module unload > and abrupt host removal (like pci hotplug removal of host controller) > we need to make sure we can trust the command timer to always return the > canceled URB Yes, you are right, we need to check these carefully. Suspend process, module unload and abrupt host removal, they all will issue usb_disconnect() firstly before clear the command queue, it will check URBs for every endpoint by usb_disconnect()--->usb_disable_device()--->usb_disable_endpoint(), which will make sure every URBs of endpoints will be cancelled by the stop endpoint command responding or the timeout function of stop endpoint command (xhci_stop_endpoint_command_timeout()) in usb_hcd_flush_endpoint(). From that point, we can make sure the command timer will be useful to handle stop endpoint command timeout. Please correct me if I said something wrong. Thanks. -- Baolin.wang Best Regards