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)
- usb_forced_unbind_intf(intf);
- }
+ if (intf->needs_binding)
+ usb_rebind_intf(intf);
}
}
}
-/* Unbind drivers for @udev's interfaces that failed to support reset-resume.
- * These interfaces have the needs_binding flag set by usb_resume_interface().
+/*
+ * Unbind all of @udev's marked interfaces and then rebind all of them.
+ * This ordering is necessary because some drivers claim several interfaces
+ * when they are first probed.
*
* The caller must hold @udev's device lock.
*/
-static void unbind_no_reset_resume_drivers_interfaces(struct usb_device *udev)
+void usb_unbind_and_rebind_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);
- }
- }
+ unbind_marked_interfaces(udev);
+ rebind_marked_interfaces(udev);
}
-static void do_rebind_interfaces(struct usb_device *udev)
+#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.
+ *
+ * The caller must hold @udev's device lock.
+ */
+static void unbind_no_pm_drivers_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->needs_binding)
- usb_rebind_intf(intf);
+
+ if (intf->dev.driver) {
+ drv = to_usb_driver(intf->dev.driver);
+ if (!drv->suspend || !drv->resume)
+ usb_forced_unbind_intf(intf);
+ }
}
}
}
@@ -1410,7 +1428,7 @@ int usb_resume_complete(struct device *d
* whose needs_binding flag is set
*/
if (udev->state != USB_STATE_NOTATTACHED)
- do_rebind_interfaces(udev);
+ rebind_marked_interfaces(udev);
return 0;
}
@@ -1432,7 +1450,7 @@ int usb_resume(struct device *dev, pm_me
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
- unbind_no_reset_resume_drivers_interfaces(udev);
+ unbind_marked_interfaces(udev);
}
/* Avoid PM error messages for devices disconnected while suspended
Index: usb-3.14/drivers/usb/core/hub.c
===================================================================
--- usb-3.14.orig/drivers/usb/core/hub.c
+++ usb-3.14/drivers/usb/core/hub.c
@@ -5345,10 +5345,11 @@ int usb_reset_device(struct usb_device *
else if (cintf->condition ==
USB_INTERFACE_BOUND)
rebind = 1;
+ if (rebind)
+ cintf->needs_binding = 1;
}
- if (ret == 0 && rebind)
- usb_rebind_intf(cintf);
}
+ usb_unbind_and_rebind_marked_interfaces(udev);
}
usb_autosuspend_device(udev);
Index: usb-3.14/drivers/usb/core/usb.h
===================================================================
--- usb-3.14.orig/drivers/usb/core/usb.h
+++ usb-3.14/drivers/usb/core/usb.h
@@ -56,7 +56,7 @@ extern int usb_match_one_id_intf(struct
extern int usb_match_device(struct usb_device *dev,
const struct usb_device_id *id);
extern void usb_forced_unbind_intf(struct usb_interface *intf);
-extern void usb_rebind_intf(struct usb_interface *intf);
+extern void usb_unbind_and_rebind_marked_interfaces(struct usb_device *udev);
extern int usb_hub_claim_port(struct usb_device *hdev, unsigned port,
struct dev_state *owner);
--
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
