usb: xhci: Handle USB transaction error on address command

PATCH Superseded Lu Baolu 3 participants 13 messages Jul 27, 2017 – Aug 11, 2017

Subsystems (2)

USB USB xHCI driver

lore

Share & tools

lore

Lu Baolu

author

2017-07-27 02:24 UTC #0 lore raw

v2 cover usb: xhci: Handle USB transaction error on address command

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:
- include 4 fixes in xhci_disable_slot which will be used to handle USB transaction error on address command.

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 |  2 +-
 drivers/usb/host/xhci.c     | 52 ++++++++++++++++++---------------------------
 drivers/usb/host/xhci.h     |  3 +--
 3 files changed, 23 insertions(+), 34 deletions(-)

Signature · 1 line

2.7.4

Lu Baolu

author

2017-07-27 02:24 UTC #1 lore raw

PATCH v2 1/5 usb: xhci: Disable slot even virt-dev is null

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 <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/usb/host/xhci.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/usb/host/xhci.c +0 -4

index b2ff1ff..e69073f 100644
@@ -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)

Signature · 1 line

2.7.4

Mathias Nyman

2017-08-09 07:55 UTC #2 lore raw

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

Quoted 9 lines · 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().

True, checking for xhci->devs[slot_id] doesn't work if xhci_alloc_dev() failed xhci_alloc_virt_device() and calls xhci_disable_slot,

but the virt dev check is needed for test mode, which will just try to disable all slots from 1 to HCS_MAX_SLOTS:

xhci_enter_test_mode(..)

       /* Disable all Device Slots */
	for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
                retval = xhci_disable_slot(xhci, NULL, i);

Quoted 26 lines

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 <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
  drivers/usb/host/xhci.c | 4 ----
  1 file changed, 4 deletions(-)

drivers/usb/host/xhci.c +0 -4

index b2ff1ff..e69073f 100644
@@ -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)

Lu Baolu

author

2017-08-10 00:35 UTC #3 lore raw

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

Hi,

Quoted 14 lines · Mathias Nyman

Quoted 9 lines · 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().
True, checking for xhci->devs[slot_id] doesn't work if xhci_alloc_dev() failed xhci_alloc_virt_device() and calls xhci_disable_slot,

Quoted 9 lines

but the virt dev check is needed for test mode, which will just try to disable all slots from 1 to HCS_MAX_SLOTS:
xhci_enter_test_mode(..)
       /* Disable all Device Slots */
    for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
                retval = xhci_disable_slot(xhci, NULL, i);

So, how about checking virt dev before this invoking?

@@ -612,10 +612,12 @@ 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++) {

- retval = xhci_disable_slot(xhci, NULL, i);
- if (retval)
- xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
- i, retval);
+ if (xhci->devs[i]) {
+ retval = xhci_disable_slot(xhci, NULL, i);
+ if (retval)
+ xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
+ i, retval);
+ }

        }
        spin_lock_irqsave(&xhci->lock, *flags);
        /* Put all ports to the Disable state by clear PP */

Best regards,
Lu Baolu

Quoted 33 lines

Quoted 27 lines

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 <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
  drivers/usb/host/xhci.c | 4 ----
  1 file changed, 4 deletions(-)

drivers/usb/host/xhci.c +0 -4

index b2ff1ff..e69073f 100644
@@ -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)

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mathias Nyman

2017-08-10 09:58 UTC #4 lore raw

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

Quoted 33 lines · Lu Baolu

Hi,
Quoted 14 lines · Mathias Nyman
Quoted 9 lines · 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().
True, checking for xhci->devs[slot_id] doesn't work if xhci_alloc_dev() failed xhci_alloc_virt_device() and calls xhci_disable_slot,
Quoted 9 lines
but the virt dev check is needed for test mode, which will just try to disable all slots from 1 to HCS_MAX_SLOTS:
  xhci_enter_test_mode(..)
        /* Disable all Device Slots */
     for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
                 retval = xhci_disable_slot(xhci, NULL, i);
So, how about checking virt dev before this invoking?
(quoted diff) +0 -0
@@ -612,10 +612,12 @@ 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++) {

Review static int xhci_enter_test_mode(struct xhci_hcd *xhci, · line 620

Quoted 10 lines

(quoted diff) +6 -4

-               retval = xhci_disable_slot(xhci, NULL, i);
-               if (retval)
-                       xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
-                                i, retval);
+               if (xhci->devs[i]) {
+                       retval = xhci_disable_slot(xhci, NULL, i);
+                       if (retval)
+                               xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
+                                        i, retval);
+               }

Yes, something like this will work

Thanks
Mathias

Lu Baolu

author

2017-08-11 01:18 UTC #5 lore raw

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

Hi,

Quoted 39 lines · Mathias Nyman

Quoted 38 lines · Lu Baolu
Hi,
Quoted 14 lines · Mathias Nyman
Quoted 9 lines · 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().
True, checking for xhci->devs[slot_id] doesn't work if xhci_alloc_dev() failed xhci_alloc_virt_device() and calls xhci_disable_slot,
Quoted 9 lines
but the virt dev check is needed for test mode, which will just try to disable all slots from 1 to HCS_MAX_SLOTS:
  xhci_enter_test_mode(..)
        /* Disable all Device Slots */
     for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
                 retval = xhci_disable_slot(xhci, NULL, i);
So, how about checking virt dev before this invoking?
(quoted diff) +1 -4
@@ -612,10 +612,12 @@ 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++) {
-               retval = xhci_disable_slot(xhci, NULL, i);
-               if (retval)
-                       xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
-                                i, retval);
+               if (xhci->devs[i]) {

Quoted 9 lines

Review static int xhci_enter_test_mode(struct xhci_hcd *xhci, · line 620
Quoted 5 lines
(quoted diff) +5 -0
+                       retval = xhci_disable_slot(xhci, NULL, i);
+                       if (retval)
+                               xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
+                                        i, retval);
+               }
Yes, something like this will work

Okay, I will submit a v3 to include this change.

Best regards,
Lu Baolu

Lu Baolu

author

2017-07-27 02:24 UTC #6 lore raw

PATCH v2 2/5 usb: xhci: Fix potential memory leak in xhci_disable_slot()

In reply to 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 <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 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(-)

drivers/usb/host/xhci-hub.c +1 -1

index 00721e8..c862d53 100644
@@ -612,7 +612,7 @@ 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++) {
-		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);

drivers/usb/host/xhci.c +9 ~~-21~~

index e69073f..cb2461a 100644
@@ -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);
 }
 
 /*

drivers/usb/host/xhci.h +1 -2

index e3e9352..6325d58 100644
@@ -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);

Signature · 1 line

2.7.4

Mathias Nyman

2017-08-09 10:06 UTC #7 lore raw

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

Quoted 112 lines · Lu Baolu

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 <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
  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(-)

drivers/usb/host/xhci-hub.c +1 -1

index 00721e8..c862d53 100644
@@ -612,7 +612,7 @@ 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++) {
-		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);

drivers/usb/host/xhci.c +8 ~~-16~~

index e69073f..cb2461a 100644
@@ -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? */

drivers/usb/host/xhci.c int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) · line 3700

Quoted 10 lines

drivers/usb/host/xhci.c +1 -5

  	/* 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);

avoiding allocating a new command after failing to allocate a virt dev was probably the reason why xhci_disable_slot() supported a command as a parameter in the first place.

But this is such a small corner case, and your patch fixes two more likely issues, and simplifies the code a lot so I think It's well worth it

-Mathias

Lu Baolu

author

2017-07-27 02:25 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/usb/host/xhci.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/usb/host/xhci.c +8 -6

index cb2461a..2df601e 100644
@@ -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;
 }
 
 /*

Signature · 1 line

2.7.4

Lu Baolu

author

2017-07-27 02:25 UTC #9 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/usb/host/xhci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/usb/host/xhci.c +1 -2

index 2df601e..d6b728d 100644
@@ -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,

Signature · 1 line

2.7.4

Lu Baolu

author

2017-07-27 02:25 UTC #10 lore raw

PATCH v2 5/5 usb: xhci: Handle USB transaction error on address command

In reply to Lu Baolu

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

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

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/usb/host/xhci.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/usb/host/xhci.c +5 -0

index d6b728d..95780f8 100644
@@ -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:

Signature · 1 line

2.7.4

Felipe Balbi

2017-07-27 07:56 UTC #11 lore raw

Hi,

Quoted 28 lines · 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 <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/usb/host/xhci.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/usb/host/xhci.c +0 -0
index d6b728d..95780f8 100644

drivers/usb/host/xhci.c static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, · line 3828

Quoted 7 lines

drivers/usb/host/xhci.c +4 -0

@@ -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);

aren't you leaking previously allocated virt_dev ?

Signature · 1 line

balbi

Lu Baolu

author

2017-07-27 08:56 UTC #12 lore raw

Hi,

Quoted 26 lines · Felipe Balbi

Hi,
Quoted 23 lines · 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 <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/usb/host/xhci.c | 5 +++++
 1 file changed, 5 insertions(+)

Quoted 14 lines

drivers/usb/host/xhci.c static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, · line 3828
Quoted 12 lines
drivers/usb/host/xhci.c +4 -0
index d6b728d..95780f8 100644
@@ -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);
aren't you leaking previously allocated virt_dev ?

When disable slot command completes, the command completion handler will release the previous allocated virt_dev.

Best regards,
Lu Baolu