Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758813AbcLAGEH (ORCPT ); Thu, 1 Dec 2016 01:04:07 -0500 Received: from mail-yw0-f174.google.com ([209.85.161.174]:34315 "EHLO mail-yw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755502AbcLAGEG (ORCPT ); Thu, 1 Dec 2016 01:04:06 -0500 MIME-Version: 1.0 In-Reply-To: <583FB8F0.6070503@linux.intel.com> References: <613dafc211127a4589306e91e231e151feb5ce80.1480496291.git.baolin.wang@linaro.org> <583FB8F0.6070503@linux.intel.com> From: Baolin Wang Date: Thu, 1 Dec 2016 14:04:04 +0800 Message-ID: Subject: Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command timer to watch stop endpoint command To: Lu Baolu 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: 2696 Lines: 65 Hi Baolu, On 1 December 2016 at 13:45, Lu Baolu wrote: > Hi, > > On 11/30/2016 05:02 PM, 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. > > I am afraid you can't do this. "stop_cmds_pending" was added > to fix the problem described in the comments that you want to > remove. But I didn't find any fix of this problem in your patch. Now we can not pending another stop endpoint command for the same one endpoint, since will check 'EP_HALT_PENDING' flag in xhci_urb_dequeue() function to avoid this. But after some investigation, I think I missed the stop endpoint command in xhci_stop_device() which did not check the 'EP_HALT_PENDING' flag, maybe need to add 'EP_HALT_PENDING' flag checking in xhci_stop_device() function. DId I miss something else? Thanks. > > - * The timer may also fire if the host takes a very long time to respond to the > - * command, and the stop endpoint command completion handler cannot delete the > - * timer before the timer function is called. Another endpoint cancellation may > - * sneak in before the timer function can grab the lock, and that may queue > - * another stop endpoint command and add the timer back. So we cannot use a > - * simple flag to say whether there is a pending stop endpoint command for a > - * particular endpoint. > - * > - * Instead we use a combination of that flag and a counter for the number of > - * pending stop endpoint commands. If the timer is the tail end of the last > - * stop endpoint command, and the endpoint's command is still pending, we assume > - * the host is dying. > > Best regards, > Lu Baolu > >> >> We also need to clean up the command queue before trying to halt the >> xHCI host in xhci_stop_endpoint_command_timeout() function. >> >> Signed-off-by: Baolin Wang > -- Baolin.wang Best Regards