Good morning Alan, I just finished testing your latest patch on Ubuntu 14.04.4 LTS (4.2.x kernel) and Ubuntu 16.04 LTS (4.4.x kernel) on x86/x64, and it resolves the issues I've encountered. May we please have this patch submitted for review? Thank you and the linux-usb developers for your time and assistance with this issue. ============================================================ Matthew Giassa, MASc, BASc, EIT Security and Embedded Systems Specialist linkedin: https://ca.linkedin.com/in/giassa e-mail: matthew@xxxxxxxxxx website: www.giassa.net > -------- Original Message -------- > Subject: Alan's latest patch > From: Matthew Giassa <matthew@xxxxxxxxxx> > Date: Sat, April 23, 2016 7:33 am > To: Matthew Giassa <matthew@xxxxxxxxxx> > > > https://marc.info/?l=linux-usb&m=146134592012790&w=2 > > > > [prev in list] [next in list] [prev in thread] [next in thread] > > List: linux-usb > Subject: RE: [PATCH 1/1] usb: lpm: add boot flag to disable lpm > From: Alan Stern <stern () rowland ! harvard ! edu> > Date: 2016-04-22 17:25:15 > Message-ID: Pine.LNX.4.44L0.1604221321000.2270-100000 () iolanthe ! > rowland ! org > [Download message RAW] > > Please remember to use Reply-To-All so that your messages get sent to > the mailing list as well as to me. > > On Thu, 21 Apr 2016, Matthew Giassa wrote: > > > Hi Alan, > > > > I've tested your latest patch, and here is a subset of the output in > > `dmesg': > > ... > > [ 44.975704] enable LPM > > [ 44.975707] CPU: 4 PID: 2491 Comm: FlyCap2 Tainted: G E > > 4.5.0-astern-logging+ #11 > > [ 44.975708] Hardware name: ASUS All Series/Z97-PRO(Wi-Fi ac), BIOS > > 2012 09/30/2014 > > [ 44.975708] 0000000000000000 ffff880217c9bd50 ffffffff813c2ccc > > ffff8802362c5000 > > [ 44.975710] ffff88023308a000 ffff880217c9bd78 ffffffff815ceadf > > ffff88023308a000 > > [ 44.975711] ffff8802362c5000 ffffffff81ce5860 ffff880217c9bd98 > > ffffffff815ceb1c > > [ 44.975713] Call Trace: > > [ 44.975715] [<ffffffff813c2ccc>] dump_stack+0x63/0x87 > > [ 44.975717] [<ffffffff815ceadf>] usb_enable_lpm+0x10f/0x120 > > [ 44.975718] [<ffffffff815ceb1c>] usb_unlocked_enable_lpm+0x2c/0x40 > > [ 44.975719] [<ffffffff815dd10e>] > > usb_driver_claim_interface+0xbe/0x110 > > [ 44.975720] [<ffffffff815e386b>] claimintf+0x5b/0x80 > > [ 44.975721] [<ffffffff815e6960>] usbdev_do_ioctl+0xd10/0x1180 > > [ 44.975723] [<ffffffff815e6dfe>] usbdev_ioctl+0xe/0x20 > > [ 44.975724] [<ffffffff812174a6>] do_vfs_ioctl+0x96/0x590 > > [ 44.975725] [<ffffffff810fb491>] ? SyS_futex+0x71/0x150 > > [ 44.975727] [<ffffffff81217a19>] SyS_ioctl+0x79/0x90 > > [ 44.975729] [<ffffffff817e2036>] entry_SYSCALL_64_fastpath+0x16/0x75 > ... > > Now I see the problem. The patch changed usb_probe_interface, but I > forgot to change usb_driver_claim_interface also. Try this version of > the patch instead. It also has the extra debugging output, but now it > shouldn't ever get triggered. > > Alan Stern > > > > Index: usb-4.x/include/linux/usb.h > =================================================================== > --- usb-4.x.orig/include/linux/usb.h > +++ usb-4.x/include/linux/usb.h > @@ -1069,7 +1069,7 @@ struct usbdrv_wrap { > * for interfaces bound to this driver. > * @soft_unbind: if set to 1, the USB core will not kill URBs and disable > * endpoints before calling the driver's disconnect method. > - * @disable_hub_initiated_lpm: if set to 0, the USB core will not allow > hubs > + * @disable_hub_initiated_lpm: if set to 1, the USB core will not allow > hubs > * to initiate lower power link state transitions when an idle timeout > * occurs. Device-initiated USB 3.0 link PM will still be allowed. > * > Index: usb-4.x/drivers/usb/core/driver.c > =================================================================== > --- usb-4.x.orig/drivers/usb/core/driver.c > +++ usb-4.x/drivers/usb/core/driver.c > @@ -284,7 +284,7 @@ static int usb_probe_interface(struct de > struct usb_device *udev = interface_to_usbdev(intf); > const struct usb_device_id *id; > int error = -ENODEV; > - int lpm_disable_error; > + int lpm_disable_error = -ENODEV; > > dev_dbg(dev, "%s\n", __func__); > > @@ -336,12 +336,14 @@ static int usb_probe_interface(struct de > * setting during probe, that should also be fine. usb_set_interface() > * will attempt to disable LPM, and fail if it can't disable it. > */ > - lpm_disable_error = usb_unlocked_disable_lpm(udev); > - if (lpm_disable_error && driver->disable_hub_initiated_lpm) { > - dev_err(&intf->dev, "%s Failed to disable LPM for driver %s\n.", > - __func__, driver->name); > - error = lpm_disable_error; > - goto err; > + if (driver->disable_hub_initiated_lpm) { > + lpm_disable_error = usb_unlocked_disable_lpm(udev); > + if (lpm_disable_error) { > + dev_err(&intf->dev, "%s Failed to disable LPM for driver %s\n.", > + __func__, driver->name); > + error = lpm_disable_error; > + goto err; > + } > } > > /* Carry out a deferred switch to altsetting 0 */ > @@ -391,7 +393,8 @@ static int usb_unbind_interface(struct d > struct usb_interface *intf = to_usb_interface(dev); > struct usb_host_endpoint *ep, **eps = NULL; > struct usb_device *udev; > - int i, j, error, r, lpm_disable_error; > + int i, j, error, r; > + int lpm_disable_error = -ENODEV; > > intf->condition = USB_INTERFACE_UNBINDING; > > @@ -399,12 +402,13 @@ static int usb_unbind_interface(struct d > udev = interface_to_usbdev(intf); > error = usb_autoresume_device(udev); > > - /* Hub-initiated LPM policy may change, so attempt to disable LPM until > + /* If hub-initiated LPM policy may change, attempt to disable LPM until > * the driver is unbound. If LPM isn't disabled, that's fine because it > * wouldn't be enabled unless all the bound interfaces supported > * hub-initiated LPM. > */ > - lpm_disable_error = usb_unlocked_disable_lpm(udev); > + if (driver->disable_hub_initiated_lpm) > + lpm_disable_error = usb_unlocked_disable_lpm(udev); > > /* > * Terminate all URBs for this interface unless the driver > @@ -505,7 +509,7 @@ int usb_driver_claim_interface(struct us > struct device *dev; > struct usb_device *udev; > int retval = 0; > - int lpm_disable_error; > + int lpm_disable_error = -ENODEV; > > if (!iface) > return -ENODEV; > @@ -526,12 +530,14 @@ int usb_driver_claim_interface(struct us > > iface->condition = USB_INTERFACE_BOUND; > > - /* Disable LPM until this driver is bound. */ > - lpm_disable_error = usb_unlocked_disable_lpm(udev); > - if (lpm_disable_error && driver->disable_hub_initiated_lpm) { > - dev_err(&iface->dev, "%s Failed to disable LPM for driver %s\n.", > - __func__, driver->name); > - return -ENOMEM; > + /* See the comment about disabling LPM in usb_probe_interface(). */ > + if (driver->disable_hub_initiated_lpm) { > + lpm_disable_error = usb_unlocked_disable_lpm(udev); > + if (lpm_disable_error) { > + dev_err(&iface->dev, "%s Failed to disable LPM for driver %s\n.", > + __func__, driver->name); > + return -ENOMEM; > + } > } > > /* Claimed interfaces are initially inactive (suspended) and > Index: usb-4.x/drivers/usb/core/devio.c > =================================================================== > --- usb-4.x.orig/drivers/usb/core/devio.c > +++ usb-4.x/drivers/usb/core/devio.c > @@ -738,6 +738,8 @@ struct usb_driver usbfs_driver = { > .resume = driver_resume, > }; > > +int alantest; > + > static int claimintf(struct usb_dev_state *ps, unsigned int ifnum) > { > struct usb_device *dev = ps->dev; > @@ -757,8 +759,11 @@ static int claimintf(struct usb_dev_stat > intf = usb_ifnum_to_if(dev, ifnum); > if (!intf) > err = -ENOENT; > - else > + else { > + alantest = 1; > err = usb_driver_claim_interface(&usbfs_driver, intf, ps); > + alantest = 0; > + } > if (err == 0) > set_bit(ifnum, &ps->ifclaimed); > return err; > @@ -778,7 +783,9 @@ static int releaseintf(struct usb_dev_st > if (!intf) > err = -ENOENT; > else if (test_and_clear_bit(ifnum, &ps->ifclaimed)) { > + alantest = 1; > usb_driver_release_interface(&usbfs_driver, intf); > + alantest = 0; > err = 0; > } > return err; > Index: usb-4.x/drivers/usb/core/hub.c > =================================================================== > --- usb-4.x.orig/drivers/usb/core/hub.c > +++ usb-4.x/drivers/usb/core/hub.c > @@ -33,6 +33,8 @@ > #include "hub.h" > #include "otg_whitelist.h" > > +extern int alantest; > + > #define USB_VENDOR_GENESYS_LOGIC 0x05e3 > #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND 0x01 > > @@ -4054,6 +4056,11 @@ int usb_disable_lpm(struct usb_device *u > if ((udev->u1_params.timeout == 0 && udev->u2_params.timeout == 0)) > return 0; > > + if (alantest) { > + pr_info("disable LPM\n"); > + dump_stack(); > + } > + > /* If LPM is enabled, attempt to disable it. */ > if (usb_disable_link_state(hcd, udev, USB3_LPM_U1)) > goto enable_lpm; > @@ -4121,6 +4128,11 @@ void usb_enable_lpm(struct usb_device *u > if (!hub) > return; > > + if (alantest) { > + pr_info("enable LPM\n"); > + dump_stack(); > + } > + > port_dev = hub->ports[udev->portnum - 1]; > > if (port_dev->usb3_lpm_u1_permit) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > [prev in list] [next in list] [prev in thread] [next in thread] > > > Configure | About | News | Add a list | Sponsored by KoreLogic > > > > -- > ============================================================ > Matthew Giassa, MASc, BASc, EIT > Security and Embedded Systems Specialist > linkedin: https://ca.linkedin.com/in/giassa > e-mail: matthew@xxxxxxxxxx > website: www.giassa.net -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
