patch "usb: host: xhci: add mutex for non-thread-safe data" added to usb-linus
Subsystems (3)
This is a note to let you know that I've just added the patch titled
usb: host: xhci: add mutex for non-thread-safe data
to my usb git tree which can be found at
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
in the usb-linus branch.
The patch will show up in the next release of the linux-next tree (usually sometime within the next 24 hours during the week.)
The patch will hopefully also be merged in Linus's tree for the next -rc kernel release.
If you have any questions about this process, please let me know.
Quoted 1 line
From
a00918d0521df1c7a2ec9143142a3ea998c8526dMon Sep 17 00:00:00 2001
From: Chris Bainbridge <[email protected]>
Date: Tue, 19 May 2015 16:30:51 +0300
Subject: usb: host: xhci: add mutex for non-thread-safe data
Regression in commit 638139eb95d2 ("usb: hub: allow to process more usb hub events in parallel")
The regression resulted in intermittent failure to initialise a 10-port hub (with three internal VL812 4-port hub controllers) on boot, with a failure rate of around 8%, due to multiple race conditions when accessing addr_dev and slot_id in struct xhci_hcd.
This regression also exposed a problem with xhci_setup_device, which "should be protected by the usb_address0_mutex" but no longer is due to
commit 6fecd4f2a58c ("USB: separate usb_address0 mutexes for each bus")
With separate buses (and locks) it is no longer the case that a single lock will protect xhci_setup_device from accesses by two parallel threads processing events on the two buses.
Fix this by adding a mutex to protect addr_dev and slot_id in struct xhci_hcd, and by making the assignment of slot_id atomic.
Fixes multiple boot errors:
[ 0.583008] xhci_hcd 0000:00:14.0: Bad Slot ID 2 [ 0.583009] xhci_hcd 0000:00:14.0: Could not allocate xHCI USB device data structures [ 0.583012] usb usb1-port3: couldn't allocate usb_device
And:
[ 0.637409] xhci_hcd 0000:00:14.0: Error while assigning device slot ID [ 0.637417] xhci_hcd 0000:00:14.0: Max number of devices this xHCI host supports is 32. [ 0.637421] usb usb1-port1: couldn't allocate usb_device
And:
[ 0.753372] xhci_hcd 0000:00:14.0: ERROR: unexpected setup context command completion code 0x0. [ 0.753373] usb 1-3: hub failed to enable device, error -22 [ 0.753400] xhci_hcd 0000:00:14.0: Error while assigning device slot ID [ 0.753402] xhci_hcd 0000:00:14.0: Max number of devices this xHCI host supports is 32. [ 0.753403] usb usb1-port3: couldn't allocate usb_device
And:
[ 11.018386] usb 1-3: device descriptor read/all, error -110
And:
[ 5.753838] xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
Tested with 200 reboots, resulting in no USB hub init related errors.
--- drivers/usb/host/xhci.c | 49 ++++++++++++++++++++++++++++++------------------- drivers/usb/host/xhci.h | 2 ++ 2 files changed, 32 insertions(+), 19 deletions(-)
drivers/usb/host/xhci.c +30 -19
@@ -3682,18 +3682,21 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) {struct xhci_hcd *xhci =hcd_to_xhci (hcd);unsigned long flags; -int ret; +int ret, slot_id;struct xhci_command *command; command =xhci_alloc_command (xhci, false, false,GFP_KERNEL );if (!command)return 0 ; +/* xhci->slot_id and xhci->addr_dev are not thread-safe */ +mutex_lock (&xhci->mutex);spin_lock_irqsave (&xhci->lock, flags); command->completion = &xhci->addr_dev; ret =xhci_queue_slot_control (xhci, command,TRB_ENABLE_SLOT ,0 );if (ret) {spin_unlock_irqrestore (&xhci->lock, flags); +mutex_unlock (&xhci->mutex);xhci_dbg (xhci,"FIXME: allocate a command ring segment\n" );kfree (command);return 0 ;
@@ -3702,8 +3705,10 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)spin_unlock_irqrestore (&xhci->lock, flags);wait_for_completion (command->completion); + slot_id = xhci->slot_id; +mutex_unlock (&xhci->mutex); -if (!xhci->slot_id || command->status !=COMP_SUCCESS ) { +if (!slot_id || command->status !=COMP_SUCCESS ) {xhci_err (xhci,"Error while assigning device slot ID\n" ); xhci_err(xhci, "Max number of devices this xHCI host supports is %u.\n", HCS_MAX_SLOTS(
@@ -3728,11 +3733,11 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)* xhci_discover_or_reset_device(), which may be called as part of * mass storage driver error handling. */ -if (!xhci_alloc_virt_device (xhci, xhci->slot_id, udev,GFP_NOIO )) { +if (!xhci_alloc_virt_device (xhci, slot_id, udev,GFP_NOIO )) {xhci_warn (xhci,"Could not allocate xHCI USB device data structures\n" ); goto disable_slot; } - udev->slot_id = xhci->slot_id; + udev->slot_id = slot_id;#ifndef CONFIG_USB_DEFAULT_PERSIST /*
@@ -3778,12 +3783,15 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,struct xhci_slot_ctx *slot_ctx;struct xhci_input_control_ctx *ctrl_ctx;u64 temp_64; -struct xhci_command *command; +struct xhci_command *command =NULL ; + +mutex_lock (&xhci->mutex);if (!udev->slot_id) {xhci_dbg_trace (xhci, trace_xhci_dbg_address,"Bad Slot ID %d" , udev->slot_id); -return -EINVAL ; + ret = -EINVAL ; + goto out; } virt_dev = xhci->devs[udev->slot_id];
@@ -3796,7 +3804,8 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,*/ xhci_warn (xhci,"Virt dev invalid for slot_id 0x%x!\n" , udev->slot_id); -return -EINVAL ; + ret = -EINVAL ; + goto out; }if (setup ==SETUP_CONTEXT_ONLY ) {
@@ -3804,13 +3813,15 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,if (GET_SLOT_STATE (le32_to_cpu (slot_ctx->dev_state)) ==SLOT_STATE_DEFAULT ) {xhci_dbg (xhci,"Slot already in default state\n" ); -return 0 ; + goto out; } } command =xhci_alloc_command (xhci, false, false,GFP_KERNEL ); -if (!command) -return -ENOMEM ; +if (!command) { + ret = -ENOMEM ; + goto out; + } command->in_ctx = virt_dev->in_ctx; command->completion = &xhci->addr_dev;
@@ -3820,8 +3831,8 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,if (!ctrl_ctx) {xhci_warn (xhci,"%s: Could not get input context, bad type.\n" , __func__); -kfree (command); -return -EINVAL ; + ret = -EINVAL ; + goto out; }/* * If this is the first Set Address since device plug-in or
@@ -3848,8 +3859,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,spin_unlock_irqrestore (&xhci->lock, flags);xhci_dbg_trace (xhci, trace_xhci_dbg_address,"FIXME: allocate a command ring segment" ); -kfree (command); -return ret; + goto out; }xhci_ring_cmd_db (xhci);spin_unlock_irqrestore (&xhci->lock, flags);
@@ -3896,10 +3906,8 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, ret = -EINVAL ;break ; } -if (ret) { -kfree (command); -return ret; - } +if (ret) + goto out; temp_64 =xhci_read_64 (xhci, &xhci->op_regs->dcbaa_ptr);xhci_dbg_trace (xhci, trace_xhci_dbg_address,"Op regs DCBAA ptr = %#016llx" , temp_64);
@@ -3932,8 +3940,10 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,xhci_dbg_trace (xhci, trace_xhci_dbg_address,"Internal device address = %d" ,le32_to_cpu (slot_ctx->dev_state) &DEV_ADDR_MASK ); +out: +mutex_unlock (&xhci->mutex);kfree (command); -return 0 ; +return ret; }int xhci_address_device (struct usb_hcd *hcd,struct usb_device *udev)
@@ -4855,6 +4865,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)return 0 ; } +mutex_init (&xhci->mutex); xhci->cap_regs = hcd->regs; xhci->op_regs = hcd->regs +HC_LENGTH (readl (&xhci->cap_regs->hc_capbase));
drivers/usb/host/xhci.h +2 -0
@@ -1497,6 +1497,8 @@ struct xhci_hcd {struct list_head lpm_failed_devs;/* slot enabling and address device helpers */ +/* these are not thread safe so use mutex */ +struct mutex mutex;struct completion addr_dev;int slot_id;/* For USB 3.0 LPM enable/disable. */
Signature · 1 line
2.4.1