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
On Wed, 12 Mar 2014, Poulain, Loic wrote: 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? I will submit my patch, and then you can submit yours to go on top of it. Alan Stern 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. PS: When you submit your patch, the email message should not contain this warning. -- 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
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 loic.poul...@intel.com This patch looks okay now. Have you tested it with btusb? What was the result? Alan Stern -- 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 loic.poul...@intel.com 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
RE: [PATCHv2] rebind: Add rebind mechanism for runtime-resume
On Tue, 11 Mar 2014, Poulain, Loic wrote:
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).
You're right about this problem; it is a weakness in the USB core. The
patch below should fix it, but I haven't done any testing. Please try
it out and see if it helps with your problem.
Alan Stern
Index: usb-3.14/drivers/usb/core/driver.c
===
--- usb-3.14.orig/drivers/usb/core/driver.c
+++ usb-3.14/drivers/usb/core/driver.c
@@ -980,8 +980,7 @@ EXPORT_SYMBOL_GPL(usb_deregister);
* it doesn't support pre_reset/post_reset/reset_resume or
* because it doesn't support suspend/resume.
*
- * The caller must hold @intf's device's lock, but not its pm_mutex
- * and not @intf-dev.sem.
+ * The caller must hold @intf's device's lock, but not @intf's lock.
*/
void usb_forced_unbind_intf(struct usb_interface *intf)
{
@@ -994,16 +993,37 @@ void usb_forced_unbind_intf(struct usb_i
intf-needs_binding = 1;
}
+/*
+ * Unbind drivers for @udev's marked interfaces. These interfaces have
+ * the needs_binding flag set, for example by usb_resume_interface().
+ *
+ * The caller must hold @udev's device lock.
+ */
+static void unbind_marked_interfaces(struct usb_device *udev)
+{
+ struct usb_host_config *config;
+ int i;
+ struct usb_interface*intf;
+
+ config = udev-actconfig;
+ if (config) {
+ for (i = 0; i config-desc.bNumInterfaces; ++i) {
+ intf = config-interface[i];
+ if (intf-dev.driver intf-needs_binding)
+ usb_forced_unbind_intf(intf);
+ }
+ }
+}
+
/* Delayed forced unbinding of a USB interface driver and scan
* for rebinding.
*
- * The caller must hold @intf's device's lock, but not its pm_mutex
- * and not @intf-dev.sem.
+ * The caller must hold @intf's device's lock, but not @intf's lock.
*
* Note: Rebinds will be skipped if a system sleep transition is in
* progress and the PM complete callback hasn't occurred yet.
*/
-void usb_rebind_intf(struct usb_interface *intf)
+static void usb_rebind_intf(struct usb_interface *intf)
{
int rc;
@@ -1020,68 +1040,66 @@ void usb_rebind_intf(struct usb_interfac
}
}
-#ifdef CONFIG_PM
-
-/* Unbind drivers for @udev's interfaces that don't support suspend/resume
- * There is no check for reset_resume here because it can be determined
- * only during resume whether reset_resume is needed.
+/*
+ * Rebind drivers to @udev's marked interfaces. These interfaces have
+ * the needs_binding flag set.
*
* The caller must hold @udev's device lock.
*/
-static void unbind_no_pm_drivers_interfaces(struct usb_device *udev)
+static void rebind_marked_interfaces(struct usb_device *udev)
{
struct usb_host_config *config;
int i;
struct usb_interface*intf;
- struct usb_driver *drv;
config = udev-actconfig;
if (config) {
for (i = 0; i config-desc.bNumInterfaces; ++i) {
intf = config-interface[i];
-
- if (intf-dev.driver) {
- drv = to_usb_driver(intf-dev.driver);
- if (!drv-suspend || !drv-resume)
-
