RE: [PATCHv4] rebind: Add rebind mechanism for runtime-resume
Same content, It's just a rebase on the master + 1 indent update. Regards, Loic Poulain From: Alan Stern [st...@rowland.harvard.edu] Sent: Wednesday, April 02, 2014 2:42 PM To: Poulain, Loic Cc: linux-usb@vger.kernel.org; gre...@linuxfoundation.org Subject: Re: [PATCHv4] rebind: Add rebind mechanism for runtime-resume On Wed, 2 Apr 2014, Poulain, Loic wrote: > Despite the needs_binding flag, the interface rebind was never > done for the PM runtime resume. This patch fixes this issue > by triggering the rebind in usb runtime resume. > > The rebind procedure needs to be called with the device lock. > However, depending the call path (remote wakeup, local resume), > the device lock may or may not already be held in usb runtime > resume. So, use a work queue to take the lock unconditionally. > > Signed-off-by: Loic Poulain > --- How does this differ from the v3 patch? You should include a description of the changes here, below the --- tear line. Alan Stern - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: "Les Montalets"- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4] rebind: Add rebind mechanism for runtime-resume
Despite the needs_binding flag, the interface rebind was never
done for the PM runtime resume. This patch fixes this issue
by triggering the rebind in usb runtime resume.
The rebind procedure needs to be called with the device lock.
However, depending the call path (remote wakeup, local resume),
the device lock may or may not already be held in usb runtime
resume. So, use a work queue to take the lock unconditionally.
Signed-off-by: Loic Poulain
---
drivers/usb/core/driver.c | 12
drivers/usb/core/usb.c| 14 ++
include/linux/usb.h | 2 ++
3 files changed, 28 insertions(+)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 81e..b8d4f38 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1834,11 +1834,23 @@ int usb_runtime_resume(struct device *dev)
{
struct usb_device *udev = to_usb_device(dev);
int status;
+ struct usb_interface*intf;
+ int i;
/* Runtime resume for a USB device means resuming both the device
* and all its interfaces.
*/
status = usb_resume_both(udev, PMSG_AUTO_RESUME);
+
+/* Schedule a safe device locked rebind if necessary */
+ for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) {
+ intf = udev->actconfig->interface[i];
+ if (intf->needs_binding) {
+ schedule_work(&udev->rebind_ws);
+ break;
+ }
+ }
+
return status;
}
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 4d11449..9ec90a3 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -265,6 +265,7 @@ static void usb_release_dev(struct device *dev)
udev = to_usb_device(dev);
hcd = bus_to_hcd(udev->bus);
+ cancel_work_sync(&udev->rebind_ws);
usb_destroy_configuration(udev);
usb_release_bos_descriptor(udev);
usb_put_hcd(hcd);
@@ -386,6 +387,17 @@ static unsigned usb_bus_is_wusb(struct usb_bus *bus)
return hcd->wireless;
}
+/* Internal function to queue device's interfaces rebind */
+static void usb_queue_rebind_interfaces(struct work_struct *ws)
+{
+ int rc;
+ struct usb_device *udev =
+ container_of(ws, struct usb_device, rebind_ws);
+
+ usb_lock_device(udev);
+ usb_unbind_and_rebind_marked_interfaces(udev);
+ usb_unlock_device(udev);
+}
/**
* usb_alloc_dev - usb device constructor (usbcore-internal)
@@ -487,6 +499,8 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
dev->parent = parent;
INIT_LIST_HEAD(&dev->filelist);
+ INIT_WORK(&dev->rebind_ws, usb_queue_rebind_interfaces);
+
#ifdef CONFIG_PM
pm_runtime_set_autosuspend_delay(&dev->dev,
usb_autosuspend_delay * 1000);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 6b7ec37..dae32df 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -508,6 +508,7 @@ struct usb3_lpm_parameters {
* to keep track of the number of functions that require USB 3.0 Link Power
* Management to be disabled for this usb_device. This count should only
* be manipulated by those functions, with the bandwidth_mutex is held.
+ * @rebind_ws: used for scheduling interface rebind with device lock
*
* Notes:
* Usbcore drivers should not set usbdev->state directly. Instead use
@@ -587,6 +588,7 @@ struct usb_device {
struct usb3_lpm_parameters u1_params;
struct usb3_lpm_parameters u2_params;
unsigned lpm_disable_count;
+ struct work_struct rebind_ws;
};
#defineto_usb_device(d) container_of(d, struct usb_device, dev)
--
1.8.3.2
-
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: reset_resume() for btusb
My thought was to fix the usbcore rebind issue (with pm_runtime)
to let the core unbind and rebind the device's interfaces for drivers
with no reset_resume callback (not only btusb).
Implementing the btusb reset_resume seems risky,
a patch implementing this callback has been previously reverted due to
HID dual mode device regression. (cf https://lkml.org/lkml/2013/11/26/347)
Regards,
Loic Poulain
From: Oliver Neukum [oneu...@suse.de]
Sent: Wednesday, March 12, 2014 12:03 PM
To: Poulain, Loic
Cc: linux-usb@vger.kernel.org
Subject: reset_resume() for btusb
Hi,
I still think it makes little sense to support reset_resume()
in btusb, but if you really want to, you can try this patch.
HTH
Oliver
>From 3776765dbd08701c30f45c1849691a16c1077cc3 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 12 Mar 2014 12:01:13 +0100
Subject: [PATCH] btusb: implement reset_resume()
This implements reset_resume() to the extent that this is possible
for btusb. It can be done if the HCI is down. In the other cases
the host would be thrown out of the network.
Signed-off-by: Oliver Neukum
---
drivers/bluetooth/btusb.c | 20
1 file changed, 20 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index baeaaed..e56fa2a 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1670,6 +1670,25 @@ done:
return err;
}
+
+static int btusb_reset_resume(struct usb_interface *intf)
+{
+ struct btusb_data *data = usb_get_intfdata(intf);
+ struct hci_dev *hdev = data->hdev;
+
+ /*
+* the interface can be recovered only if the HCI
+* is not part of a network because the synchronization
+* is lost as the device is reset
+*/
+ if (test_bit(HCI_RUNNING, &hdev->flags))
+ return -EIO;
+
+ if (hdev->setup)
+ return (hdev->setup)(hdev);
+ else
+ return 0;
+}
#endif
static struct usb_driver btusb_driver = {
@@ -1679,6 +1698,7 @@ static struct usb_driver btusb_driver = {
#ifdef CONFIG_PM
.suspend= btusb_suspend,
.resume = btusb_resume,
+ .reset_resume = btusb_reset_resume,
#endif
.id_table = btusb_table,
.supports_autosuspend = 1,
--
1.8.4.5
-
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3] rebind: Add rebind mechanism for runtime-resume
Despite the needs_binding flag, the interface rebind was never
done for the PM runtime resume. This patch fixes this issue
by triggering the rebind in usb runtime resume.
The rebind procedure needs to be called with the device lock.
However, depending the call path (remote wakeup, local resume),
the device lock may or may not already be held in usb runtime
resume. So, use a work queue to take the lock unconditionally.
Signed-off-by: Loic Poulain
---
drivers/usb/core/driver.c | 12
drivers/usb/core/usb.c| 14 ++
include/linux/usb.h | 2 ++
3 files changed, 28 insertions(+)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index a9f636e..5d5df85 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1773,11 +1773,23 @@ int usb_runtime_resume(struct device *dev)
{
struct usb_device *udev = to_usb_device(dev);
int status;
+ struct usb_interface*intf;
+ int i;
/* Runtime resume for a USB device means resuming both the device
* and all its interfaces.
*/
status = usb_resume_both(udev, PMSG_AUTO_RESUME);
+
+ /* Schedule a safe device locked rebind if necessary */
+ for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) {
+ intf = udev->actconfig->interface[i];
+ if (intf->needs_binding) {
+ schedule_work(&udev->rebind_ws);
+ break;
+ }
+ }
+
return status;
}
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 4d11449..a807e51 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -265,6 +265,7 @@ static void usb_release_dev(struct device *dev)
udev = to_usb_device(dev);
hcd = bus_to_hcd(udev->bus);
+ cancel_work_sync(&udev->rebind_ws);
usb_destroy_configuration(udev);
usb_release_bos_descriptor(udev);
usb_put_hcd(hcd);
@@ -386,6 +387,17 @@ static unsigned usb_bus_is_wusb(struct usb_bus *bus)
return hcd->wireless;
}
+/* Internal function to queue device's interfaces rebind */
+static void usb_queue_rebind_interfaces(struct work_struct *ws)
+{
+ int rc;
+ struct usb_device *udev =
+ container_of(ws, struct usb_device, rebind_ws);
+
+ usb_lock_device(udev);
+ usb_unbind_and_rebind_marked_interfaces(udev);
+ usb_unlock_device(udev);
+}
/**
* usb_alloc_dev - usb device constructor (usbcore-internal)
@@ -487,6 +499,8 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
dev->parent = parent;
INIT_LIST_HEAD(&dev->filelist);
+ INIT_WORK(&dev->rebind_ws, usb_queue_rebind_interfaces);
+
#ifdef CONFIG_PM
pm_runtime_set_autosuspend_delay(&dev->dev,
usb_autosuspend_delay * 1000);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 512ab16..1ee363c 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -502,6 +502,7 @@ struct usb3_lpm_parameters {
* to keep track of the number of functions that require USB 3.0 Link Power
* Management to be disabled for this usb_device. This count should only
* be manipulated by those functions, with the bandwidth_mutex is held.
+ * @rebind_ws: used for scheduling interface rebind with device lock
*
* Notes:
* Usbcore drivers should not set usbdev->state directly. Instead use
@@ -581,6 +582,7 @@ struct usb_device {
struct usb3_lpm_parameters u1_params;
struct usb3_lpm_parameters u2_params;
unsigned lpm_disable_count;
+ struct work_struct rebind_ws;
};
#defineto_usb_device(d) container_of(d, struct usb_device, dev)
--
1.8.3.2
-
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] rebind: Add rebind mechanism for runtime-resume
Hello Alan, I applied your patch, then reworked mine in order to use usb_unbind_and_rebind_marked_interfaces. It works great with the combination of this two fixes. I even reproduced the btusb runtime-resume hardware issue which is now handled correctly, interfaces are unbind/rebind. I can provide a new patchset rebased on yours or maybe I should wait for your "definitive" patch? Thanks & Regards, Loic Poulain - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: "Les Montalets"- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] rebind: Add rebind mechanism for runtime-resume
To test the patch, I tried it with the btusb driver, and forced manually the resume issue with one interface (intf 0) every five resume call. With this patch, the interface is correctly unbind/rebind. Then, the upper layer (here android) is able to retrieve the device. However, btusb driver itself doesn't seem compatible with the rebind mechanism. Indeed, When a real resume fails, all the interfaces of a same device are marked for rebind. The btusb driver works with two interfaces, it claims the "intf 1" in the "intf 0" probe and releases the two interfaces at the same time in the disconnect callback. However the rebind mechanism unbind/bind the device's interfaces sequentially. (cf do_rebind_interfaces), for the btusb case, it means: 1. unbind intf 0 2. bind intf 0 3. unbind intf 1 4. bind inf 1 At point 2. btusb probes intf 0, and tries to claim the intf 1 which is not yet unbind, so the intf 0 probe fails (interface 1 busy). At point 4. the busb probes the intf 1, since it's not the "main interface", it fails and returns -ENODEV, waiting for the intf0 probe. I don't know if it is a btusb driver implementation issue or a usb core problem. Two fixes are possible. - Fix the btusb driver, make it compatible with any probe sequence, "intf 0" first or "intf 1" first. (However, may several drivers have the same behavior) - Fix the rebind mechanism, unbind all the interfaces first then probe them all. Regarding, the rebind patch itself, it's ok for me with the forced software failure test. But should be completed with an other patch or new patchset for the btusb driver (at least). Regards, Loic Poulain From: Alan Stern [st...@rowland.harvard.edu] Sent: Tuesday, March 11, 2014 3:14 PM To: Poulain, Loic Cc: linux-usb@vger.kernel.org; oneu...@suse.de; gre...@linuxfoundation.org Subject: Re: [PATCHv2] rebind: Add rebind mechanism for runtime-resume On Tue, 11 Mar 2014, Poulain, Loic wrote: > Despite the needs_rebind flag, the interface rebind was never > done for the PM runtime resume. This patch fixes this issue > by triggering the rebind in usb runtime resume. > > The rebind procedure needs to be called with the device lock. > However, depending the call path (remote wakeup, local resume), > the device lock may or may not already be held in usb runtime > resume. So, use a work queue to take the lock unconditionally. > Protect this work against any deadlock in the same way as > reset_device. > > Signed-off-by: Loic Poulain This patch looks okay now. Have you tested it with btusb? What was the result? Alan Stern - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: "Les Montalets"- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2] rebind: Add rebind mechanism for runtime-resume
Despite the needs_rebind flag, the interface rebind was never
done for the PM runtime resume. This patch fixes this issue
by triggering the rebind in usb runtime resume.
The rebind procedure needs to be called with the device lock.
However, depending the call path (remote wakeup, local resume),
the device lock may or may not already be held in usb runtime
resume. So, use a work queue to take the lock unconditionally.
Protect this work against any deadlock in the same way as
reset_device.
Signed-off-by: Loic Poulain
---
drivers/usb/core/driver.c | 30 ++
drivers/usb/core/message.c | 37 +
include/linux/usb.h| 7 +++
3 files changed, 70 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index ab90a01..0f5296d 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -290,6 +290,18 @@ static void usb_cancel_queued_reset(struct usb_interface
*iface)
cancel_work_sync(&iface->reset_ws);
}
+/*
+ * Cancel any pending scheduled rebind
+ *
+ * See comments in __usb_queue_rebind_device() regarding
+ * udev->rebind_running.
+ */
+static void usb_cancel_queued_rebind(struct usb_interface *iface)
+{
+ if (iface->rebind_running == 0)
+ cancel_work_sync(&iface->rebind_ws);
+}
+
/* called from driver core with dev locked */
static int usb_probe_interface(struct device *dev)
{
@@ -381,6 +393,7 @@ static int usb_probe_interface(struct device *dev)
intf->needs_remote_wakeup = 0;
intf->condition = USB_INTERFACE_UNBOUND;
usb_cancel_queued_reset(intf);
+ usb_cancel_queued_rebind(intf);
/* If the LPM disable succeeded, balance the ref counts. */
if (!lpm_disable_error)
@@ -449,6 +462,8 @@ static int usb_unbind_interface(struct device *dev)
intf->condition = USB_INTERFACE_UNBOUND;
intf->needs_remote_wakeup = 0;
+ usb_cancel_queued_rebind(intf);
+
/* Attempt to re-enable USB3 LPM, if the disable succeeded. */
if (!lpm_disable_error)
usb_unlocked_enable_lpm(udev);
@@ -1080,7 +1095,7 @@ static void
unbind_no_reset_resume_drivers_interfaces(struct usb_device *udev)
}
}
-static void do_rebind_interfaces(struct usb_device *udev)
+static void do_rebind_interfaces(struct usb_device *udev, int use_workqueue)
{
struct usb_host_config *config;
int i;
@@ -1090,8 +1105,12 @@ static void do_rebind_interfaces(struct usb_device *udev)
if (config) {
for (i = 0; i < config->desc.bNumInterfaces; ++i) {
intf = config->interface[i];
- if (intf->needs_binding)
- usb_rebind_intf(intf);
+ if (intf->needs_binding) {
+ if (use_workqueue)
+ schedule_work(&intf->rebind_ws);
+ else
+ usb_rebind_intf(intf);
+ }
}
}
}
@@ -1420,7 +1439,7 @@ int usb_resume_complete(struct device *dev)
* whose needs_binding flag is set
*/
if (udev->state != USB_STATE_NOTATTACHED)
- do_rebind_interfaces(udev);
+ do_rebind_interfaces(udev, 0);
return 0;
}
@@ -1800,6 +1819,9 @@ int usb_runtime_resume(struct device *dev)
* and all its interfaces.
*/
status = usb_resume_both(udev, PMSG_AUTO_RESUME);
+
+ do_rebind_interfaces(udev, 1);
+
return status;
}
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index f829a1a..3d95867 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1661,6 +1661,42 @@ static void __usb_queue_reset_device(struct work_struct
*ws)
}
}
+/*
+ * Internal function to queue an interface rebind
+ *
+ * This is initialized into the workstruct in 'struct
+ * usb_device->rebind_ws' that is launched by
+ * message.c:usb_set_configuration() when initializing each 'struct
+ * usb_interface'.
+ *
+ * It is safe to get the USB device without reference counts because
+ * the life cycle of @iface is bound to the life cycle of @udev. Then,
+ * this function will be ran only if @iface is alive (and before
+ * freeing it any scheduled instances of it will have been cancelled).
+ *
+ * We need to set a flag (usb_dev->rebind_running) because when we call
+ * the rebind, the interfaces will be unbound. The current interface
+ * cannot try to remove the queued work as it would cause a deadlock
+ * (you cannot remove your work from within your executing workqueue).
+ * This flag lets it know, so that usb_cancel_queued_rebind() doesn't
+ * try to do it.
+ */
+static void __usb_queue_rebind_intf(struct work_struct *ws)
+{
+ int rc;
+ struct usb_interface *iface =
+
[PATCH] rebind: Add rebind mechanism for runtime-resume
Despite the needs_rebind flag, the interface rebind was never
done for the PM runtime resume. This patch fixes this issue
by triggering the rebind in usb runtime resume.
The rebind procedure needs to be called with the device lock.
However, depending the call path (remote wakeup, local resume),
the device lock may or may not already be held in usb runtime
resume. So, use a work queue to take the lock unconditionally.
Protect this work against any deadlock in the same way as
reset_device.
Signed-off-by: lpoulain
---
drivers/usb/core/driver.c | 30 +
drivers/usb/core/message.c | 47 ++
include/linux/usb.h| 6 ++
3 files changed, 79 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 47aade2..5a6692a 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -259,6 +259,18 @@ static void usb_cancel_queued_reset(struct usb_interface
*iface)
cancel_work_sync(&iface->reset_ws);
}
+/*
+ * Cancel any pending scheduled rebind
+ *
+ * See comments in __usb_queue_rebind_device() regarding
+ * udev->rebind_running.
+ */
+static void usb_cancel_queued_rebind(struct usb_interface *iface)
+{
+ if (iface->rebind_running == 0)
+ cancel_work_sync(&iface->rebind_ws);
+}
+
/* called from driver core with dev locked */
static int usb_probe_interface(struct device *dev)
{
@@ -350,6 +362,7 @@ static int usb_probe_interface(struct device *dev)
intf->needs_remote_wakeup = 0;
intf->condition = USB_INTERFACE_UNBOUND;
usb_cancel_queued_reset(intf);
+ usb_cancel_queued_rebind(intf);
/* If the LPM disable succeeded, balance the ref counts. */
if (!lpm_disable_error)
@@ -418,6 +431,8 @@ static int usb_unbind_interface(struct device *dev)
intf->condition = USB_INTERFACE_UNBOUND;
intf->needs_remote_wakeup = 0;
+ usb_cancel_queued_rebind(intf);
+
/* Attempt to re-enable USB3 LPM, if the disable succeeded. */
if (!lpm_disable_error)
usb_unlocked_enable_lpm(udev);
@@ -1049,7 +1064,7 @@ static void
unbind_no_reset_resume_drivers_interfaces(struct usb_device *udev)
}
}
-static void do_rebind_interfaces(struct usb_device *udev)
+static void do_rebind_interfaces(struct usb_device *udev, int use_workqueue)
{
struct usb_host_config *config;
int i;
@@ -1059,8 +1074,12 @@ static void do_rebind_interfaces(struct usb_device *udev)
if (config) {
for (i = 0; i < config->desc.bNumInterfaces; ++i) {
intf = config->interface[i];
- if (intf->needs_binding)
- usb_rebind_intf(intf);
+ if (intf->needs_binding) {
+ if (use_workqueue)
+ schedule_work(&intf->rebind_ws);
+ else
+ usb_rebind_intf(intf);
+ }
}
}
}
@@ -1389,7 +1408,7 @@ int usb_resume_complete(struct device *dev)
* whose needs_binding flag is set
*/
if (udev->state != USB_STATE_NOTATTACHED)
- do_rebind_interfaces(udev);
+ do_rebind_interfaces(udev, 0);
return 0;
}
@@ -1769,6 +1788,9 @@ int usb_runtime_resume(struct device *dev)
* and all its interfaces.
*/
status = usb_resume_both(udev, PMSG_AUTO_RESUME);
+
+ do_rebind_interfaces(udev, 1);
+
return status;
}
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index bb31597..bc46fc6 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1662,6 +1662,52 @@ static void __usb_queue_reset_device(struct work_struct
*ws)
}
}
+/*
+ * Internal function to queue an interface rebind
+ *
+ * This is initialized into the workstruct in 'struct
+ * usb_device->rebind_ws' that is launched by
+ * message.c:usb_set_configuration() when initializing each 'struct
+ * usb_interface'.
+ *
+ * It is safe to get the USB device without reference counts because
+ * the life cycle of @iface is bound to the life cycle of @udev. Then,
+ * this function will be ran only if @iface is alive (and before
+ * freeing it any scheduled instances of it will have been cancelled).
+ *
+ * We need to set a flag (usb_dev->rebind_running) because when we call
+ * the rebind, the interfaces will be unbound. The current interface
+ * cannot try to remove the queued work as it would cause a deadlock
+ * (you cannot remove your work from within your executing workqueue).
+ * This flag lets it know, so that usb_cancel_queued_rebind() doesn't
+ * try to do it.
+ */
+static void __usb_queue_rebind_intf(struct work_struct *ws)
+{
+ unsigned long jiffies_expire = jiffies + HZ;
+ struct u
RE: BUG: USB Reset-Resume Mechanism does not work for runtime resume
I agree, but because it's possible to have many different hardware for the same class driver, you can't predict if all of them will be not broken. So in this case, each USB class driver should have the reset-resume callback. A broken device can be a device which doesn't resume correctly 1 time per thousand (due to timing reason or anything else). Today, this single resume failure is enough to stuck the device (suspended forever). because mobile systems are more and more PM agressive, I think we could see this problem occurring more often. Unbind/rebind is here as a fallback mechanism for drivers that don't know how to restore device after resume failure, and it works pretty well for system PM. For runtime PM, half of the work is done, interface is declared as needs_rebind but never rebind. So either the rebind work should be completed for PM runtime or an other solution found (logical disconnect, reset_resume mandatory...). But I think this dead end should be fixed. Regards, Loic Poulain From: Peter Chen [peter.c...@freescale.com] Sent: Thursday, March 06, 2014 2:10 AM To: Alan Stern Cc: Poulain, Loic; linux-usb@vger.kernel.org Subject: Re: BUG: USB Reset-Resume Mechanism does not work for runtime resume On Wed, Mar 05, 2014 at 11:26:06AM -0500, Alan Stern wrote: > On Wed, 5 Mar 2014, Poulain, Loic wrote: > > > + function 'do_rebind_interface' is in charge of unbind and rebind the > > interfaces with the 'need_binfind' flag set. > > + However this method is only called in 'usb_resume_complete'. > > + Problem is that 'usb_resume_complete' is a dev_pm_ops callback > > (core/usb.h) used by the System PM core which is never called in a runtime > > PM scenario. > > + So interfaces remain suspended forever (or until the next PM system > > suspend/resume), device is unusable. > > (unbinding/rebinding manually the interface via sysfs recovers the device) > > > > Could you please let me know if it's the expected behavior or a real issue. > > Should we not call 'do_rebind_interface' in the runtime resume sequence? > > I suspect this is an oversight. > > Still, implementing it would be difficult. In usb_runtime_resume, the > device lock may or may not already be held. Therefore we cannot simply > lock the device and call do_rebind_interfaces. > > Using a work queue for this seems like overkill (and it seems fragile). > I don't know what we should do. > If the autosuspend supported device can't respond resume correctly, eg, after resume, it can't respond GET_STATUS correctly in this case, the .reset_resume interface is a must for class driver, it just like a workaround for broken hardware. -- Best Regards, Peter Chen - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: "Les Montalets"- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
BUG: USB Reset-Resume Mechanism does not work for runtime resume
Hi, I think there is an issue with the reset-resume mechanism during USB runtime resume, leaving the interfaces suspended forever. - Context: Android platform, running a 3.13 + Google AOSP patches kernel. I'm facing a USB issue with the bsusb driver. btusb driver has the USB runtime suspend enabled + remote wakeup. However sometimes the USB runtime resume does not work as expected (we admit here that this can happen). [ 3066.408413] usb 2-3: usb auto-resume [ 3066.408433] hub 2-0:1.0: state 7 ports 9 chg evt 0008 [ 3066.445465] usb 2-3: finish resume [ 3071.447591] usb 2-3: bt_hc_worker timed out on ep0in len=0/2 [ 3071.447598] usb 2-3: retry with reset-resume Then the usbcore tries a reset-resume, but btusb driver does not implement this callback. [ 3071.619408] btusb 2-3:1.0: no reset_resume for driver btusb? [ 3071.619413] btusb 2-3:1.1: no reset_resume for driver btusb? However for driver which doesn't implement 'reset_resume' callback, usbcore plans to unbind/rebind it. Here is the problem, because this never happens. - What happens (in core/driver.c): 0. Need to wake-up the usb device for data transfer 1. 'usb_runtime_resume' is called and calls 'usb_resume_both' 2.' usb_resume_both' calls 'usb_resume_device' then calls 'usb_resume_interface' for each interface with the 'reset_resume' flag set (since resume failed). 3. 'usb_resume_interface' enters in the 'reset_resume' condition and sets the 'needs_binding' interface flag. 4. At the end of this runtime resume, the interfaces stay suspended FOREVER. - Why: + function 'do_rebind_interface' is in charge of unbind and rebind the interfaces with the 'need_binfind' flag set. + However this method is only called in 'usb_resume_complete'. + Problem is that 'usb_resume_complete' is a dev_pm_ops callback (core/usb.h) used by the System PM core which is never called in a runtime PM scenario. + So interfaces remain suspended forever (or until the next PM system suspend/resume), device is unusable. (unbinding/rebinding manually the interface via sysfs recovers the device) Could you please let me know if it's the expected behavior or a real issue. Should we not call 'do_rebind_interface' in the runtime resume sequence? Thanks & Regards, Loic Poulain - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: "Les Montalets"- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
