From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753444AbcD0Ooy (ORCPT ); Wed, 27 Apr 2016 10:44:54 -0400 Received: from mga14.intel.com ([192.55.52.115]:43158 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752918AbcD0Oow (ORCPT ); Wed, 27 Apr 2016 10:44:52 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,542,1455004800"; d="scan'208";a="941362344" Message-ID: <5720D1F8.6030809@linux.intel.com> Date: Wed, 27 Apr 2016 17:51:36 +0300 From: Mathias Nyman User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Chris Bainbridge , gregkh@linuxfoundation.org CC: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] usb: core: hub: hub_port_init lock controller instead of bus References: <1461588518-8290-1-git-send-email-chris.bainbridge@gmail.com> In-Reply-To: <1461588518-8290-1-git-send-email-chris.bainbridge@gmail.com> 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 25.04.2016 15:48, Chris Bainbridge wrote: > The XHCI controller presents two USB buses to the system - one for USB2 > and one for USB3. The hub init code (hub_port_init) is reentrant but > only locks one bus per thread, leading to a race condition failure when > two threads attempt to simultaneously initialise a USB2 and USB3 device: > > [ 8.034843] xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command > [ 13.183701] usb 3-3: device descriptor read/all, error -110 > > On a test system this failure occurred on 6% of all boots. > > The call traces at the point of failure are: > > Call Trace: > [] schedule+0x37/0x90 > [] usb_kill_urb+0x8d/0xd0 > [] ? wake_up_atomic_t+0x30/0x30 > [] usb_start_wait_urb+0xbe/0x150 > [] usb_control_msg+0xbc/0xf0 > [] hub_port_init+0x51e/0xb70 > [] hub_event+0x817/0x1570 > [] process_one_work+0x1ff/0x620 > [] ? process_one_work+0x15f/0x620 > [] worker_thread+0x64/0x4b0 > [] ? rescuer_thread+0x390/0x390 > [] kthread+0x105/0x120 > [] ? kthread_create_on_node+0x200/0x200 > [] ret_from_fork+0x3f/0x70 > [] ? kthread_create_on_node+0x200/0x200 > > Call Trace: > [] xhci_setup_device+0x53d/0xa40 > [] xhci_address_device+0xe/0x10 > [] hub_port_init+0x1bf/0xb70 > [] ? trace_hardirqs_on+0xd/0x10 > [] hub_event+0x817/0x1570 > [] process_one_work+0x1ff/0x620 > [] ? process_one_work+0x15f/0x620 > [] worker_thread+0x64/0x4b0 > [] ? rescuer_thread+0x390/0x390 > [] kthread+0x105/0x120 > [] ? kthread_create_on_node+0x200/0x200 > [] ret_from_fork+0x3f/0x70 > [] ? kthread_create_on_node+0x200/0x200 > > Which results from the two call chains: > > hub_port_init > usb_get_device_descriptor > usb_get_descriptor > usb_control_msg > usb_internal_control_msg > usb_start_wait_urb > usb_submit_urb / wait_for_completion_timeout / usb_kill_urb > > hub_port_init > hub_set_address > xhci_address_device > xhci_setup_device > > Mathias Nyman explains the current behaviour violates the XHCI spec: > > hub_port_reset() will end up moving the corresponding xhci device slot > to default state. > > As hub_port_reset() is called several times in hub_port_init() it > sounds reasonable that we could end up with two threads having their > xhci device slots in default state at the same time, which according to > xhci 4.5.3 specs still is a big no no: > > "Note: Software shall not transition more than one Device Slot to the > Default State at a time" > > So both threads fail at their next task after this. > One fails to read the descriptor, and the other fails addressing the > device. > > Fix this in hub_port_init by locking the USB controller (instead of an > individual bus) to prevent simultaneous initialisation of both buses. > > Fixes: 638139eb95d2 ("usb: hub: allow to process more usb hub events in parallel") > Link: https://lkml.org/lkml/2016/2/8/312 > Link: https://lkml.org/lkml/2016/2/4/748 > Signed-off-by: Chris Bainbridge > --- Acked-by: Mathias Nyman