[PATCH v3 0/5] usb: xhci: Handle USB transaction error on address command

[PATCH v3 0/5] usb: xhci: Handle USB transaction error on address command

[Lu Baolu]

Xhci driver handles USB transaction errors on transfer events,
but transaction errors are possible on address device command
completion events as well.

The xHCI specification (section 4.6.5) says: A USB Transaction
Error Completion Code for an Address Device Command may be due
to a Stall response from a device. Software should issue a Disable
Slot Command for the Device Slot then an Enable Slot Command to
recover from this error.

The related discussion threads can be found through below links.

http://marc.info/?l=linux-usb&m=149362010728921&w=2
http://marc.info/?l=linux-usb&m=149252752825755&w=2

This patch set includes some fixes in xhci_disable_slot() as well
which will be used to handle USB transaction error on address
command.

---
Change log:

v1->v2:
 - Add 4 fixes in xhci_disable_slot which will be used
   to handle USB transaction error on address command.

v2->v3:
 - Add checking virt dev for test mode in PATCH 1/5.

Lu Baolu (5):
  usb: xhci: Disable slot even virt-dev is null
  usb: xhci: Fix potential memory leak in xhci_disable_slot()
  usb: xhci: Fix memory leak when xhci_disable_slot() returns error
  usb: xhci: Return error when host is dead in xhci_disable_slot()
  usb: xhci: Handle USB transaction error on address command

 drivers/usb/host/xhci-hub.c |  5 ++++-
 drivers/usb/host/xhci.c     | 52 ++++++++++++++++++---------------------------
 drivers/usb/host/xhci.h     |  3 +--
 3 files changed, 26 insertions(+), 34 deletions(-)

-- 
2.7.4

[PATCH v3 1/5] usb: xhci: Disable slot even virt-dev is null

[Lu Baolu]

xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it checks
the corespoding virt-dev pointer and returns directly (w/o issuing
disable slot command) if it's null.

This is unnecessary and will cause problems in case where virt-dev
allocation fails and xhci_disable_slot() is called to roll back the
hardware state. Refer to the implementation of xhci_alloc_dev().

This patch removes lines to check virt-dev in xhci_disable_slot().

Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang <guoqing.zhang@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c | 3 +++
 drivers/usb/host/xhci.c     | 4 ----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 00721e8..6fcb98d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -612,6 +612,9 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
 	xhci_dbg(xhci, "Disable all slots\n");
 	spin_unlock_irqrestore(&xhci->lock, *flags);
 	for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
+		if (!xhci->devs[i])
+			continue;
+
 		retval = xhci_disable_slot(xhci, NULL, i);
 		if (retval)
 			xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b2ff1ff..e69073f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3567,11 +3567,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
 	unsigned long flags;
 	u32 state;
 	int ret = 0;
-	struct xhci_virt_device *virt_dev;
 
-	virt_dev = xhci->devs[slot_id];
-	if (!virt_dev)
-		return -EINVAL;
 	if (!command)
 		command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
 	if (!command)
-- 
2.7.4

[PATCH v3 2/5] usb: xhci: Fix potential memory leak in xhci_disable_slot()

[Lu Baolu]

xhci_disable_slot() allows the invoker to pass a command pointer
as paramenter. Otherwise, it will allocate one. This will cause
memory leak when a command structure was allocated inside of this
function while queuing command trb fails. Another problem comes up
when the invoker passed a command pointer, but xhci_disable_slot()
frees it when it detects a dead host.

This patch fixes these two problems by removing the command parameter
from xhci_disable_slot().

Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang <guoqing.zhang@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c |  2 +-
 drivers/usb/host/xhci.c     | 30 +++++++++---------------------
 drivers/usb/host/xhci.h     |  3 +--
 3 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 6fcb98d..daaf155 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -615,7 +615,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
 		if (!xhci->devs[i])
 			continue;
 
-		retval = xhci_disable_slot(xhci, NULL, i);
+		retval = xhci_disable_slot(xhci, i);
 		if (retval)
 			xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
 				 i, retval);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e69073f..cb2461a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3519,11 +3519,6 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
 	struct xhci_virt_device *virt_dev;
 	struct xhci_slot_ctx *slot_ctx;
 	int i, ret;
-	struct xhci_command *command;
-
-	command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
-	if (!command)
-		return;
 
 #ifndef CONFIG_USB_DEFAULT_PERSIST
 	/*
@@ -3539,10 +3534,8 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
 	/* If the host is halted due to driver unload, we still need to free the
 	 * device.
 	 */
-	if (ret <= 0 && ret != -ENODEV) {
-		kfree(command);
+	if (ret <= 0 && ret != -ENODEV)
 		return;
-	}
 
 	virt_dev = xhci->devs[udev->slot_id];
 	slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
@@ -3554,22 +3547,21 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
 		del_timer_sync(&virt_dev->eps[i].stop_cmd_timer);
 	}
 
-	xhci_disable_slot(xhci, command, udev->slot_id);
+	xhci_disable_slot(xhci, udev->slot_id);
 	/*
 	 * Event command completion handler will free any data structures
 	 * associated with the slot.  XXX Can free sleep?
 	 */
 }
 
-int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
-			u32 slot_id)
+int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
 {
+	struct xhci_command *command;
 	unsigned long flags;
 	u32 state;
 	int ret = 0;
 
-	if (!command)
-		command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+	command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
 	if (!command)
 		return -ENOMEM;
 
@@ -3588,7 +3580,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
 				slot_id);
 	if (ret) {
 		spin_unlock_irqrestore(&xhci->lock, flags);
-		xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
+		kfree(command);
 		return ret;
 	}
 	xhci_ring_cmd_db(xhci);
@@ -3663,6 +3655,8 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
 		return 0;
 	}
 
+	xhci_free_command(xhci, command);
+
 	if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) {
 		spin_lock_irqsave(&xhci->lock, flags);
 		ret = xhci_reserve_host_control_ep_resources(xhci);
@@ -3698,18 +3692,12 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
 		pm_runtime_get_noresume(hcd->self.controller);
 #endif
 
-
-	xhci_free_command(xhci, command);
 	/* Is this a LS or FS device under a HS hub? */
 	/* Hub or peripherial? */
 	return 1;
 
 disable_slot:
-	/* Disable slot, if we can do it without mem alloc */
-	kfree(command->completion);
-	command->completion = NULL;
-	command->status = 0;
-	return xhci_disable_slot(xhci, command, udev->slot_id);
+	return xhci_disable_slot(xhci, udev->slot_id);
 }
 
 /*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e3e9352..6325d58 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2003,8 +2003,7 @@ int xhci_run(struct usb_hcd *hcd);
 int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks);
 void xhci_init_driver(struct hc_driver *drv,
 		      const struct xhci_driver_overrides *over);
-int xhci_disable_slot(struct xhci_hcd *xhci,
-			struct xhci_command *command, u32 slot_id);
+int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
 
 int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
 int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
-- 
2.7.4

[PATCH v3 3/5] usb: xhci: Fix memory leak when xhci_disable_slot() returns error

[Lu Baolu]

If xhci_disable_slot() returns success, a disable slot command
trb was queued in the command ring. The command completion
handler will free the virtual device data structure associated
with the slot. On the other hand, when xhci_disable_slot()
returns error, the invokers should take the responsibilities to
free the slot related data structure. Otherwise, memory leakage
happens.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index cb2461a..2df601e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3547,11 +3547,9 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
 		del_timer_sync(&virt_dev->eps[i].stop_cmd_timer);
 	}
 
-	xhci_disable_slot(xhci, udev->slot_id);
-	/*
-	 * Event command completion handler will free any data structures
-	 * associated with the slot.  XXX Can free sleep?
-	 */
+	ret = xhci_disable_slot(xhci, udev->slot_id);
+	if (ret)
+		xhci_free_virt_device(xhci, udev->slot_id);
 }
 
 int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
@@ -3697,7 +3695,11 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
 	return 1;
 
 disable_slot:
-	return xhci_disable_slot(xhci, udev->slot_id);
+	ret = xhci_disable_slot(xhci, udev->slot_id);
+	if (ret)
+		xhci_free_virt_device(xhci, udev->slot_id);
+
+	return 0;
 }
 
 /*
-- 
2.7.4

[PATCH v3 4/5] usb: xhci: Return error when host is dead in xhci_disable_slot()

[Lu Baolu]

xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it returns
success when it sees a dead host. This is not the right way to go.
It should return error and let the invoker know that disable slot
command was failed due to a dead host.

Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang <guoqing.zhang@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2df601e..d6b728d 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3568,10 +3568,9 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
 	state = readl(&xhci->op_regs->status);
 	if (state == 0xffffffff || (xhci->xhc_state & XHCI_STATE_DYING) ||
 			(xhci->xhc_state & XHCI_STATE_HALTED)) {
-		xhci_free_virt_device(xhci, slot_id);
 		spin_unlock_irqrestore(&xhci->lock, flags);
 		kfree(command);
-		return ret;
+		return -ENODEV;
 	}
 
 	ret = xhci_queue_slot_control(xhci, command, TRB_DISABLE_SLOT,
-- 
2.7.4

[PATCH v3 5/5] usb: xhci: Handle USB transaction error on address command

[Lu Baolu]

Xhci driver handles USB transaction errors on transfer events,
but transaction errors are possible on address device command
completion events as well.

The xHCI specification (section 4.6.5) says: A USB Transaction
Error Completion Code for an Address Device Command may be due
to a Stall response from a device. Software should issue a Disable
Slot Command for the Device Slot then an Enable Slot Command to
recover from this error.

This patch handles USB transaction errors on address command
completion events. The related discussion threads can be found
through below links.

http://marc.info/?l=linux-usb&m=149362010728921&w=2
http://marc.info/?l=linux-usb&m=149252752825755&w=2

Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d6b728d..95780f8 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
 		break;
 	case COMP_USB_TRANSACTION_ERROR:
 		dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
+
+		ret = xhci_disable_slot(xhci, udev->slot_id);
+		if (!ret)
+			xhci_alloc_dev(hcd, udev);
+
 		ret = -EPROTO;
 		break;
 	case COMP_INCOMPATIBLE_DEVICE_ERROR:
-- 
2.7.4

Re: [PATCH v3 5/5] usb: xhci: Handle USB transaction error on address command

[Mathias Nyman]

On 11.08.2017 05:41, Lu Baolu wrote:
> Xhci driver handles USB transaction errors on transfer events,
> but transaction errors are possible on address device command
> completion events as well.
>
> The xHCI specification (section 4.6.5) says: A USB Transaction
> Error Completion Code for an Address Device Command may be due
> to a Stall response from a device. Software should issue a Disable
> Slot Command for the Device Slot then an Enable Slot Command to
> recover from this error.
>
> This patch handles USB transaction errors on address command
> completion events. The related discussion threads can be found
> through below links.
>
> http://marc.info/?l=linux-usb&m=149362010728921&w=2
> http://marc.info/?l=linux-usb&m=149252752825755&w=2
>
> Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/usb/host/xhci.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index d6b728d..95780f8 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>   		break;
>   	case COMP_USB_TRANSACTION_ERROR:
>   		dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
> +
> +		ret = xhci_disable_slot(xhci, udev->slot_id);
> +		if (!ret)
> +			xhci_alloc_dev(hcd, udev);

Might be a xhci->mutex locking issue here,
both xhci_setup_device() and xhci_alloc_dev() take xhci->mutex

-Mathias
  

Re: [PATCH v3 5/5] usb: xhci: Handle USB transaction error on address command

[Lu Baolu]

Hi,

On 08/15/2017 07:30 PM, Mathias Nyman wrote:
> On 11.08.2017 05:41, Lu Baolu wrote:
>> Xhci driver handles USB transaction errors on transfer events,
>> but transaction errors are possible on address device command
>> completion events as well.
>>
>> The xHCI specification (section 4.6.5) says: A USB Transaction
>> Error Completion Code for an Address Device Command may be due
>> to a Stall response from a device. Software should issue a Disable
>> Slot Command for the Device Slot then an Enable Slot Command to
>> recover from this error.
>>
>> This patch handles USB transaction errors on address command
>> completion events. The related discussion threads can be found
>> through below links.
>>
>> http://marc.info/?l=linux-usb&m=149362010728921&w=2
>> http://marc.info/?l=linux-usb&m=149252752825755&w=2
>>
>> Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/usb/host/xhci.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index d6b728d..95780f8 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>>           break;
>>       case COMP_USB_TRANSACTION_ERROR:
>>           dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
>> +
>> +        ret = xhci_disable_slot(xhci, udev->slot_id);
>> +        if (!ret)
>> +            xhci_alloc_dev(hcd, udev);
>
> Might be a xhci->mutex locking issue here,
> both xhci_setup_device() and xhci_alloc_dev() take xhci->mutex
>

I will apply xhci->mutex in this patch for code consistency, but I think
we can drop xhci->mutex (in a separated patch) anyway.

xhci->mutex was introduced to protect two race sources of xhci->slot_id
and xhci->addr_dev by below commit:

commit a00918d0521df1c7a2ec9143142a3ea998c8526d
Author: Chris Bainbridge <chris.bainbridge@gmail.com>
Date:   Tue May 19 16:30:51 2015 +0300

    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.

[--cut---]

We have already removed these two race sources after that by below
two commits:

c2d3d49 usb: xhci: move slot_id from xhci_hcd to xhci_command structure
87e44f2 usb: xhci: remove the use of xhci->addr_dev

So we don't need xhci->mutex any more. I will try to do this in a separated
patch with more tests. For now, I will add xhci->mutex for code consistency.

Best regards,
Lu Baolu

Re: [PATCH v3 5/5] usb: xhci: Handle USB transaction error on address command

[Mathias Nyman]

On 16.08.2017 05:15, Lu Baolu wrote:
> Hi,
>
> On 08/15/2017 07:30 PM, Mathias Nyman wrote:
>> On 11.08.2017 05:41, Lu Baolu wrote:
>>> Xhci driver handles USB transaction errors on transfer events,
>>> but transaction errors are possible on address device command
>>> completion events as well.
>>>
>>> The xHCI specification (section 4.6.5) says: A USB Transaction
>>> Error Completion Code for an Address Device Command may be due
>>> to a Stall response from a device. Software should issue a Disable
>>> Slot Command for the Device Slot then an Enable Slot Command to
>>> recover from this error.
>>>
>>> This patch handles USB transaction errors on address command
>>> completion events. The related discussion threads can be found
>>> through below links.
>>>
>>> http://marc.info/?l=linux-usb&m=149362010728921&w=2
>>> http://marc.info/?l=linux-usb&m=149252752825755&w=2
>>>
>>> Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>    drivers/usb/host/xhci.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index d6b728d..95780f8 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>>>            break;
>>>        case COMP_USB_TRANSACTION_ERROR:
>>>            dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
>>> +
>>> +        ret = xhci_disable_slot(xhci, udev->slot_id);
>>> +        if (!ret)
>>> +            xhci_alloc_dev(hcd, udev);
>>
>> Might be a xhci->mutex locking issue here,
>> both xhci_setup_device() and xhci_alloc_dev() take xhci->mutex
>>
>
> I will apply xhci->mutex in this patch for code consistency, but I think
> we can drop xhci->mutex (in a separated patch) anyway.
>
> xhci->mutex was introduced to protect two race sources of xhci->slot_id
> and xhci->addr_dev by below commit:
>
> commit a00918d0521df1c7a2ec9143142a3ea998c8526d
>      usb: host: xhci: add mutex for non-thread-safe data
>
>
> c2d3d49 usb: xhci: move slot_id from xhci_hcd to xhci_command structure
> 87e44f2 usb: xhci: remove the use of xhci->addr_dev
>
> So we don't need xhci->mutex any more. I will try to do this in a separated
> patch with more tests. For now, I will add xhci->mutex for code consistency.

Fixing xhci->mutex sounds like a good idea, and you are right, it's no longer
needed for slot_id or addr_dev.
But it's use was extended to protect xhci_stop() and xhci_setup_device()
from racing at fast xhci host hotplug/removes in this patch.

commit 85ac90f8953a58f6a057b727bc9db97721e3fb8e
Author: Roger Quadros <rogerq@ti.com>
Date:   Mon Sep 21 17:46:12 2015 +0300

     usb: xhci: lock mutex on xhci_stop
  
     Else it races with xhci_setup_device

But I think it's safe to remove xhci->mutex  from xhci_alloc_dev()

There's no huurry with the patch 5/5 so would be nice to get that mutex
cleaned up before.

Thanks
Mathias
   

Re: [PATCH v3 5/5] usb: xhci: Handle USB transaction error on address command

[Lu Baolu]

Hi,

On 08/18/2017 09:31 PM, Mathias Nyman wrote:
> On 16.08.2017 05:15, Lu Baolu wrote:
>> Hi,
>>
>> On 08/15/2017 07:30 PM, Mathias Nyman wrote:
>>> On 11.08.2017 05:41, Lu Baolu wrote:
>>>> Xhci driver handles USB transaction errors on transfer events,
>>>> but transaction errors are possible on address device command
>>>> completion events as well.
>>>>
>>>> The xHCI specification (section 4.6.5) says: A USB Transaction
>>>> Error Completion Code for an Address Device Command may be due
>>>> to a Stall response from a device. Software should issue a Disable
>>>> Slot Command for the Device Slot then an Enable Slot Command to
>>>> recover from this error.
>>>>
>>>> This patch handles USB transaction errors on address command
>>>> completion events. The related discussion threads can be found
>>>> through below links.
>>>>
>>>> http://marc.info/?l=linux-usb&m=149362010728921&w=2
>>>> http://marc.info/?l=linux-usb&m=149252752825755&w=2
>>>>
>>>> Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/usb/host/xhci.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>>> index d6b728d..95780f8 100644
>>>> --- a/drivers/usb/host/xhci.c
>>>> +++ b/drivers/usb/host/xhci.c
>>>> @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>>>>            break;
>>>>        case COMP_USB_TRANSACTION_ERROR:
>>>>            dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
>>>> +
>>>> +        ret = xhci_disable_slot(xhci, udev->slot_id);
>>>> +        if (!ret)
>>>> +            xhci_alloc_dev(hcd, udev);
>>>
>>> Might be a xhci->mutex locking issue here,
>>> both xhci_setup_device() and xhci_alloc_dev() take xhci->mutex
>>>
>>
>> I will apply xhci->mutex in this patch for code consistency, but I think
>> we can drop xhci->mutex (in a separated patch) anyway.
>>
>> xhci->mutex was introduced to protect two race sources of xhci->slot_id
>> and xhci->addr_dev by below commit:
>>
>> commit a00918d0521df1c7a2ec9143142a3ea998c8526d
>>      usb: host: xhci: add mutex for non-thread-safe data
>>
>>
>> c2d3d49 usb: xhci: move slot_id from xhci_hcd to xhci_command structure
>> 87e44f2 usb: xhci: remove the use of xhci->addr_dev
>>
>> So we don't need xhci->mutex any more. I will try to do this in a separated
>> patch with more tests. For now, I will add xhci->mutex for code consistency.
>
> Fixing xhci->mutex sounds like a good idea, and you are right, it's no longer
> needed for slot_id or addr_dev.
> But it's use was extended to protect xhci_stop() and xhci_setup_device()
> from racing at fast xhci host hotplug/removes in this patch.
>
> commit 85ac90f8953a58f6a057b727bc9db97721e3fb8e
> Author: Roger Quadros <rogerq@ti.com>
> Date:   Mon Sep 21 17:46:12 2015 +0300
>
>     usb: xhci: lock mutex on xhci_stop
>  
>     Else it races with xhci_setup_device
>
> But I think it's safe to remove xhci->mutex  from xhci_alloc_dev()
>
> There's no huurry with the patch 5/5 so would be nice to get that mutex
> cleaned up before.

Okay.

Best regards,
Lu Baolu