| Message ID | 20180108094416.4789-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, On 08-01-18 10:44, Hans de Goede wrote: > Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") > removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, > instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. > > This was done because the DIY BTUSB_RESET_RESUME reset-resume handling > has several issues (see the original commit message). An added advantage > of moving over to the USB-core reset-resume handling is that it also > disables autosuspend for these devices, which is similarly broken on these. > > But there are 2 issues with this approach: > 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek > devices. > 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been > added to usb/core/quirks.c and if we fix the Realtek case the same way > we need to add an additional 14 entries. So in essence we need to > duplicate a large part of the usb_device_id table in btusb.c in > usb/core/quirks.c and manually keep them in sync. > > This commit instead restores setting a reset-resume quirk for QCA devices > in the btusb.c code, avoiding the duplicate usb_device_id table problem. > > This commit avoids the problems with the original DIY BTUSB_RESET_RESUME > code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the > usb_device. > > This commit also moves the BTUSB_REALTEK case over to directly setting the > USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused > BTUSB_RESET_RESUME code. > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 > Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") > Cc: stable@vger.kernel.org > Cc: Leif Liddy <leif.linux@gmail.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Cc: Brian Norris <briannorris@chromium.org> > Cc: Daniel Drake <drake@endlessm.com> > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note: > 1) Once this has been merged, the 2 commits adding QCA device entries to > drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next. > 2) I don't have any of the affected devices, please test I've asked the reporter of: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 To test a kernel with this patch added and he confirms that this fixes the autosuspend related issues he was seeing, so this is no longer untested. Regards, Hans > --- > drivers/bluetooth/btusb.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 4764100a5888..c4689f03220f 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -23,6 +23,7 @@ > > #include <linux/module.h> > #include <linux/usb.h> > +#include <linux/usb/quirks.h> > #include <linux/firmware.h> > #include <linux/of_device.h> > #include <linux/of_irq.h> > @@ -388,9 +389,8 @@ static const struct usb_device_id blacklist_table[] = { > #define BTUSB_FIRMWARE_LOADED 7 > #define BTUSB_FIRMWARE_FAILED 8 > #define BTUSB_BOOTING 9 > -#define BTUSB_RESET_RESUME 10 > -#define BTUSB_DIAG_RUNNING 11 > -#define BTUSB_OOB_WAKE_ENABLED 12 > +#define BTUSB_DIAG_RUNNING 10 > +#define BTUSB_OOB_WAKE_ENABLED 11 > > struct btusb_data { > struct hci_dev *hdev; > @@ -3118,6 +3118,12 @@ static int btusb_probe(struct usb_interface *intf, > if (id->driver_info & BTUSB_QCA_ROME) { > data->setup_on_usb = btusb_setup_qca; > hdev->set_bdaddr = btusb_set_bdaddr_ath3012; > + > + /* QCA Rome devices lose their updated firmware over suspend, > + * but the USB hub doesn't notice any status change. > + * explicitly request a device reset on resume. > + */ > + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; > } > > #ifdef CONFIG_BT_HCIBTUSB_RTL > @@ -3128,7 +3134,7 @@ static int btusb_probe(struct usb_interface *intf, > * but the USB hub doesn't notice any status change. > * Explicitly request a device reset on resume. > */ > - set_bit(BTUSB_RESET_RESUME, &data->flags); > + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; > } > #endif > > @@ -3297,14 +3303,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > enable_irq(data->oob_wake_irq); > } > > - /* Optionally request a device reset on resume, but only when > - * wakeups are disabled. If wakeups are enabled we assume the > - * device will stay powered up throughout suspend. > - */ > - if (test_bit(BTUSB_RESET_RESUME, &data->flags) && > - !device_may_wakeup(&data->udev->dev)) > - data->udev->reset_resume = 1; > - > return 0; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, > Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") > removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, > instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. > > This was done because the DIY BTUSB_RESET_RESUME reset-resume handling > has several issues (see the original commit message). An added advantage > of moving over to the USB-core reset-resume handling is that it also > disables autosuspend for these devices, which is similarly broken on these. > > But there are 2 issues with this approach: > 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek > devices. > 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been > added to usb/core/quirks.c and if we fix the Realtek case the same way > we need to add an additional 14 entries. So in essence we need to > duplicate a large part of the usb_device_id table in btusb.c in > usb/core/quirks.c and manually keep them in sync. > > This commit instead restores setting a reset-resume quirk for QCA devices > in the btusb.c code, avoiding the duplicate usb_device_id table problem. > > This commit avoids the problems with the original DIY BTUSB_RESET_RESUME > code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the > usb_device. > > This commit also moves the BTUSB_REALTEK case over to directly setting the > USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused > BTUSB_RESET_RESUME code. > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 > Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") > Cc: stable@vger.kernel.org > Cc: Leif Liddy <leif.linux@gmail.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Cc: Brian Norris <briannorris@chromium.org> > Cc: Daniel Drake <drake@endlessm.com> > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note: > 1) Once this has been merged, the 2 commits adding QCA device entries to > drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next. > 2) I don't have any of the affected devices, please test > --- > drivers/bluetooth/btusb.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 08-01-18 21:46, Marcel Holtmann wrote: > Hi Hans, > >> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >> >> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >> has several issues (see the original commit message). An added advantage >> of moving over to the USB-core reset-resume handling is that it also >> disables autosuspend for these devices, which is similarly broken on these. >> >> But there are 2 issues with this approach: >> 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek >> devices. >> 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been >> added to usb/core/quirks.c and if we fix the Realtek case the same way >> we need to add an additional 14 entries. So in essence we need to >> duplicate a large part of the usb_device_id table in btusb.c in >> usb/core/quirks.c and manually keep them in sync. >> >> This commit instead restores setting a reset-resume quirk for QCA devices >> in the btusb.c code, avoiding the duplicate usb_device_id table problem. >> >> This commit avoids the problems with the original DIY BTUSB_RESET_RESUME >> code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the >> usb_device. >> >> This commit also moves the BTUSB_REALTEK case over to directly setting the >> USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused >> BTUSB_RESET_RESUME code. >> >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 >> Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >> Cc: stable@vger.kernel.org >> Cc: Leif Liddy <leif.linux@gmail.com> >> Cc: Matthias Kaehlcke <mka@chromium.org> >> Cc: Brian Norris <briannorris@chromium.org> >> Cc: Daniel Drake <drake@endlessm.com> >> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Note: >> 1) Once this has been merged, the 2 commits adding QCA device entries to >> drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next. >> 2) I don't have any of the affected devices, please test >> --- >> drivers/bluetooth/btusb.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) > > patch has been applied to bluetooth-next tree. I see that you've also dropped the 2 commits adding QCA device entries to drivers/usb/core/quirks.c, great. Thank you. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote: > Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") > removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, > instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. > > This was done because the DIY BTUSB_RESET_RESUME reset-resume handling > has several issues (see the original commit message). An added advantage > of moving over to the USB-core reset-resume handling is that it also > disables autosuspend for these devices, which is similarly broken on these. Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't think so -- I'm using one now. Thus, this is a poor solution, which negatively affects my systems. However, I see that this patch was applied regardless... What justifications was found for this anyway? AIUI, this is a platform bug, and not entirely a chipset bug. Why should all users of this chipset be penalized? Brian > But there are 2 issues with this approach: > 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek > devices. > 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been > added to usb/core/quirks.c and if we fix the Realtek case the same way > we need to add an additional 14 entries. So in essence we need to > duplicate a large part of the usb_device_id table in btusb.c in > usb/core/quirks.c and manually keep them in sync. > > This commit instead restores setting a reset-resume quirk for QCA devices > in the btusb.c code, avoiding the duplicate usb_device_id table problem. > > This commit avoids the problems with the original DIY BTUSB_RESET_RESUME > code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the > usb_device. > > This commit also moves the BTUSB_REALTEK case over to directly setting the > USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused > BTUSB_RESET_RESUME code. > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 > Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") > Cc: stable@vger.kernel.org > Cc: Leif Liddy <leif.linux@gmail.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Cc: Brian Norris <briannorris@chromium.org> > Cc: Daniel Drake <drake@endlessm.com> > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note: > 1) Once this has been merged, the 2 commits adding QCA device entries to > drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next. > 2) I don't have any of the affected devices, please test > --- > drivers/bluetooth/btusb.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 4764100a5888..c4689f03220f 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -23,6 +23,7 @@ > > #include <linux/module.h> > #include <linux/usb.h> > +#include <linux/usb/quirks.h> > #include <linux/firmware.h> > #include <linux/of_device.h> > #include <linux/of_irq.h> > @@ -388,9 +389,8 @@ static const struct usb_device_id blacklist_table[] = { > #define BTUSB_FIRMWARE_LOADED 7 > #define BTUSB_FIRMWARE_FAILED 8 > #define BTUSB_BOOTING 9 > -#define BTUSB_RESET_RESUME 10 > -#define BTUSB_DIAG_RUNNING 11 > -#define BTUSB_OOB_WAKE_ENABLED 12 > +#define BTUSB_DIAG_RUNNING 10 > +#define BTUSB_OOB_WAKE_ENABLED 11 > > struct btusb_data { > struct hci_dev *hdev; > @@ -3118,6 +3118,12 @@ static int btusb_probe(struct usb_interface *intf, > if (id->driver_info & BTUSB_QCA_ROME) { > data->setup_on_usb = btusb_setup_qca; > hdev->set_bdaddr = btusb_set_bdaddr_ath3012; > + > + /* QCA Rome devices lose their updated firmware over suspend, > + * but the USB hub doesn't notice any status change. > + * explicitly request a device reset on resume. > + */ > + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; > } > > #ifdef CONFIG_BT_HCIBTUSB_RTL > @@ -3128,7 +3134,7 @@ static int btusb_probe(struct usb_interface *intf, > * but the USB hub doesn't notice any status change. > * Explicitly request a device reset on resume. > */ > - set_bit(BTUSB_RESET_RESUME, &data->flags); > + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; > } > #endif > > @@ -3297,14 +3303,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > enable_irq(data->oob_wake_irq); > } > > - /* Optionally request a device reset on resume, but only when > - * wakeups are disabled. If wakeups are enabled we assume the > - * device will stay powered up throughout suspend. > - */ > - if (test_bit(BTUSB_RESET_RESUME, &data->flags) && > - !device_may_wakeup(&data->udev->dev)) > - data->udev->reset_resume = 1; > - > return 0; > } > > -- > 2.14.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 13-02-18 03:24, Brian Norris wrote: > Hi, > > On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote: >> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >> >> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >> has several issues (see the original commit message). An added advantage >> of moving over to the USB-core reset-resume handling is that it also >> disables autosuspend for these devices, which is similarly broken on these. > > Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't > think so -- I'm using one now. And have you manually enabled USB autosuspend for it, or are you running something which might have done so, e.g. powertop --auto-tune ? Because if you did not do that then you're already not using autosuspend for your QCA devices and this patch will change nothing. > Thus, this is a poor solution, which > negatively affects my systems. However, I see that this patch was > applied regardless... Note that there already is a quirk to handle broken suspend/resume behavior on ALL QCA devices in older kernels. Also note that the patches series which this commit builds on top of was already setting USB_QUIRK_RESET_RESUME for some devices in usb/core/quirks.c. All my commit does is instead of duplicating all the QCA USB-ids in usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME to btusb.c so that we don't need to duplicate the USB-id tables. The result of the combination of these patches is that the custom DIY reset on resume handling btusb.c was doing is now replaced by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk. As a (desirable) side effect this also disables USB autosuspend for QCA devices since the USB-core does not allow USB autosuspend on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown this to be necessary on at least some QCA devices and given that these devices tend to loose there firmware on a suspend, it seems sensible to not allow autosuspend on them. > What justifications was found for this anyway? AIUI, this is a platform > bug, and not entirely a chipset bug. No this is believed to be a chipset issue, hence also the quirk in older kernels to always reset these devices after a normal suspend/resume. Regards, Hans > > Brian > >> But there are 2 issues with this approach: >> 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek >> devices. >> 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been >> added to usb/core/quirks.c and if we fix the Realtek case the same way >> we need to add an additional 14 entries. So in essence we need to >> duplicate a large part of the usb_device_id table in btusb.c in >> usb/core/quirks.c and manually keep them in sync. >> >> This commit instead restores setting a reset-resume quirk for QCA devices >> in the btusb.c code, avoiding the duplicate usb_device_id table problem. >> >> This commit avoids the problems with the original DIY BTUSB_RESET_RESUME >> code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the >> usb_device. >> >> This commit also moves the BTUSB_REALTEK case over to directly setting the >> USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused >> BTUSB_RESET_RESUME code. >> >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 >> Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >> Cc: stable@vger.kernel.org >> Cc: Leif Liddy <leif.linux@gmail.com> >> Cc: Matthias Kaehlcke <mka@chromium.org> >> Cc: Brian Norris <briannorris@chromium.org> >> Cc: Daniel Drake <drake@endlessm.com> >> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Note: >> 1) Once this has been merged, the 2 commits adding QCA device entries to >> drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next. >> 2) I don't have any of the affected devices, please test >> --- >> drivers/bluetooth/btusb.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c >> index 4764100a5888..c4689f03220f 100644 >> --- a/drivers/bluetooth/btusb.c >> +++ b/drivers/bluetooth/btusb.c >> @@ -23,6 +23,7 @@ >> >> #include <linux/module.h> >> #include <linux/usb.h> >> +#include <linux/usb/quirks.h> >> #include <linux/firmware.h> >> #include <linux/of_device.h> >> #include <linux/of_irq.h> >> @@ -388,9 +389,8 @@ static const struct usb_device_id blacklist_table[] = { >> #define BTUSB_FIRMWARE_LOADED 7 >> #define BTUSB_FIRMWARE_FAILED 8 >> #define BTUSB_BOOTING 9 >> -#define BTUSB_RESET_RESUME 10 >> -#define BTUSB_DIAG_RUNNING 11 >> -#define BTUSB_OOB_WAKE_ENABLED 12 >> +#define BTUSB_DIAG_RUNNING 10 >> +#define BTUSB_OOB_WAKE_ENABLED 11 >> >> struct btusb_data { >> struct hci_dev *hdev; >> @@ -3118,6 +3118,12 @@ static int btusb_probe(struct usb_interface *intf, >> if (id->driver_info & BTUSB_QCA_ROME) { >> data->setup_on_usb = btusb_setup_qca; >> hdev->set_bdaddr = btusb_set_bdaddr_ath3012; >> + >> + /* QCA Rome devices lose their updated firmware over suspend, >> + * but the USB hub doesn't notice any status change. >> + * explicitly request a device reset on resume. >> + */ >> + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; >> } >> >> #ifdef CONFIG_BT_HCIBTUSB_RTL >> @@ -3128,7 +3134,7 @@ static int btusb_probe(struct usb_interface *intf, >> * but the USB hub doesn't notice any status change. >> * Explicitly request a device reset on resume. >> */ >> - set_bit(BTUSB_RESET_RESUME, &data->flags); >> + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; >> } >> #endif >> >> @@ -3297,14 +3303,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) >> enable_irq(data->oob_wake_irq); >> } >> >> - /* Optionally request a device reset on resume, but only when >> - * wakeups are disabled. If wakeups are enabled we assume the >> - * device will stay powered up throughout suspend. >> - */ >> - if (test_bit(BTUSB_RESET_RESUME, &data->flags) && >> - !device_may_wakeup(&data->udev->dev)) >> - data->udev->reset_resume = 1; >> - >> return 0; >> } >> >> -- >> 2.14.3 >> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote: > On 13-02-18 03:24, Brian Norris wrote: > > On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote: > > > Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") > > > removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, > > > instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. > > > > > > This was done because the DIY BTUSB_RESET_RESUME reset-resume handling > > > has several issues (see the original commit message). An added advantage > > > of moving over to the USB-core reset-resume handling is that it also > > > disables autosuspend for these devices, which is similarly broken on these. > > > > Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't > > think so -- I'm using one now. > > And have you manually enabled USB autosuspend for it, or are you > running something which might have done so, e.g. powertop --auto-tune ? > > Because if you did not do that then you're already not using autosuspend > for your QCA devices and this patch will change nothing. I use a set of udev rules that manually whitelist devices for autosuspend. You can see it here: https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 You'll find at least one Rome chip in there. > > Thus, this is a poor solution, which > > negatively affects my systems. However, I see that this patch was > > applied regardless... > > Note that there already is a quirk to handle broken suspend/resume > behavior on ALL QCA devices in older kernels. Also note that the Yes, and the quirk was broken, and I made sure it got reverted when it broke my devices ;) > patches series which this commit builds on top of was already > setting USB_QUIRK_RESET_RESUME for some devices in > usb/core/quirks.c. > > All my commit does is instead of duplicating all the QCA USB-ids in > usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME > to btusb.c so that we don't need to duplicate the USB-id tables. I was slightly more OK with marking specific IDs as broken, because those IDs didn't happen to be ones that I knew were currently working. Now you're breaking my systems again. But this time, it's more subtle because bluetooth will still work, but we just suck more power leaving our USB port active all the time. > The result of the combination of these patches is that the custom > DIY reset on resume handling btusb.c was doing is now replaced > by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk. > > As a (desirable) side effect this also disables USB autosuspend > for QCA devices since the USB-core does not allow USB autosuspend > on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown > this to be necessary on at least some QCA devices and given that > these devices tend to loose there firmware on a suspend, it seems > sensible to not allow autosuspend on them. But you're not accurately targeting the "some". AFAICT, you're wasting power on my system. > > What justifications was found for this anyway? AIUI, this is a platform > > bug, and not entirely a chipset bug. > > No this is believed to be a chipset issue, hence also the quirk in > older kernels to always reset these devices after a normal suspend/resume. I have Qualcomm telling me this is a platform issue. I haven't noticed problems with autosuspend nor system suspend/resume on my platform. Do you have any more detail on this issue? Have you consulsted QCA folks? Unfortunately, Greg is already queueing this patch up to all the -stable trees, so I'm going to have to revert it yet again in Chromium kernels... Brian -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/15/2018 06:27 PM, Brian Norris wrote: > Hi Hans, > > On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote: >> On 13-02-18 03:24, Brian Norris wrote: >>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote: >>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >>>> >>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >>>> has several issues (see the original commit message). An added advantage >>>> of moving over to the USB-core reset-resume handling is that it also >>>> disables autosuspend for these devices, which is similarly broken on these. >>> >>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't >>> think so -- I'm using one now. >> >> And have you manually enabled USB autosuspend for it, or are you >> running something which might have done so, e.g. powertop --auto-tune ? >> >> Because if you did not do that then you're already not using autosuspend >> for your QCA devices and this patch will change nothing. > > I use a set of udev rules that manually whitelist devices for > autosuspend. You can see it here: > > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 > > You'll find at least one Rome chip in there. > >>> Thus, this is a poor solution, which >>> negatively affects my systems. However, I see that this patch was >>> applied regardless... >> >> Note that there already is a quirk to handle broken suspend/resume >> behavior on ALL QCA devices in older kernels. Also note that the > > Yes, and the quirk was broken, and I made sure it got reverted when it > broke my devices ;) > >> patches series which this commit builds on top of was already >> setting USB_QUIRK_RESET_RESUME for some devices in >> usb/core/quirks.c. >> >> All my commit does is instead of duplicating all the QCA USB-ids in >> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME >> to btusb.c so that we don't need to duplicate the USB-id tables. > > I was slightly more OK with marking specific IDs as broken, because > those IDs didn't happen to be ones that I knew were currently working. > Now you're breaking my systems again. But this time, it's more subtle > because bluetooth will still work, but we just suck more power leaving > our USB port active all the time. > >> The result of the combination of these patches is that the custom >> DIY reset on resume handling btusb.c was doing is now replaced >> by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk. >> >> As a (desirable) side effect this also disables USB autosuspend >> for QCA devices since the USB-core does not allow USB autosuspend >> on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown >> this to be necessary on at least some QCA devices and given that >> these devices tend to loose there firmware on a suspend, it seems >> sensible to not allow autosuspend on them. > > But you're not accurately targeting the "some". AFAICT, you're wasting > power on my system. > >>> What justifications was found for this anyway? AIUI, this is a platform >>> bug, and not entirely a chipset bug. >> >> No this is believed to be a chipset issue, hence also the quirk in >> older kernels to always reset these devices after a normal suspend/resume. > > I have Qualcomm telling me this is a platform issue. I haven't noticed > problems with autosuspend nor system suspend/resume on my platform. Do > you have any more detail on this issue? Have you consulsted QCA folks? > > Unfortunately, Greg is already queueing this patch up to all the -stable > trees, so I'm going to have to revert it yet again in Chromium > kernels... > Grumble Grumble Grumble. I'll try to remember to revert this patch when I merge v4.14.20 into chromeos-4.14. Please remind me if I forget for some reason. I don't see it queued in v4.4.y - is that still coming ? Hope I won't miss it. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Going a bit off-topic here, so changed the subject. I will reply on topic in another mail. On 16-02-18 03:27, Brian Norris wrote: > Hi Hans, > > On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote: >> On 13-02-18 03:24, Brian Norris wrote: >>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote: >>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >>>> >>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >>>> has several issues (see the original commit message). An added advantage >>>> of moving over to the USB-core reset-resume handling is that it also >>>> disables autosuspend for these devices, which is similarly broken on these. >>> >>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't >>> think so -- I'm using one now. >> >> And have you manually enabled USB autosuspend for it, or are you >> running something which might have done so, e.g. powertop --auto-tune ? >> >> Because if you did not do that then you're already not using autosuspend >> for your QCA devices and this patch will change nothing. > > I use a set of udev rules that manually whitelist devices for > autosuspend. You can see it here: > > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 > > You'll find at least one Rome chip in there. Oh, that is a very interesting link for the work I've been doing to improve Linux power-consumption in general: https://fedoraproject.org/wiki/Changes/ImprovedLaptopBatteryLife I was actually planning on at least doing such a list for WWAN modems, for btusb my approach has been to just enable it everywhere (except for QCA devices as I got bugreports for those). Note that I plan to eventually submit this whitelist to the udev rules which are part of systemd upstream, so if chromeos is using systemd too, this is something to be aware of for you. Question, is the white-listing of the root and rate-limiting hubs really necessary? I thought these have this enabled by default? Also any caveats here I should be aware of? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, >>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote: >>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >>>> >>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >>>> has several issues (see the original commit message). An added advantage >>>> of moving over to the USB-core reset-resume handling is that it also >>>> disables autosuspend for these devices, which is similarly broken on these. >>> >>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't >>> think so -- I'm using one now. >> >> And have you manually enabled USB autosuspend for it, or are you >> running something which might have done so, e.g. powertop --auto-tune ? >> >> Because if you did not do that then you're already not using autosuspend >> for your QCA devices and this patch will change nothing. > > I use a set of udev rules that manually whitelist devices for > autosuspend. You can see it here: > > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 > > You'll find at least one Rome chip in there. > >>> Thus, this is a poor solution, which >>> negatively affects my systems. However, I see that this patch was >>> applied regardless... >> >> Note that there already is a quirk to handle broken suspend/resume >> behavior on ALL QCA devices in older kernels. Also note that the > > Yes, and the quirk was broken, and I made sure it got reverted when it > broke my devices ;) > >> patches series which this commit builds on top of was already >> setting USB_QUIRK_RESET_RESUME for some devices in >> usb/core/quirks.c. >> >> All my commit does is instead of duplicating all the QCA USB-ids in >> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME >> to btusb.c so that we don't need to duplicate the USB-id tables. > > I was slightly more OK with marking specific IDs as broken, because > those IDs didn't happen to be ones that I knew were currently working. > Now you're breaking my systems again. But this time, it's more subtle > because bluetooth will still work, but we just suck more power leaving > our USB port active all the time. > >> The result of the combination of these patches is that the custom >> DIY reset on resume handling btusb.c was doing is now replaced >> by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk. >> >> As a (desirable) side effect this also disables USB autosuspend >> for QCA devices since the USB-core does not allow USB autosuspend >> on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown >> this to be necessary on at least some QCA devices and given that >> these devices tend to loose there firmware on a suspend, it seems >> sensible to not allow autosuspend on them. > > But you're not accurately targeting the "some". AFAICT, you're wasting > power on my system. > >>> What justifications was found for this anyway? AIUI, this is a platform >>> bug, and not entirely a chipset bug. >> >> No this is believed to be a chipset issue, hence also the quirk in >> older kernels to always reset these devices after a normal suspend/resume. > > I have Qualcomm telling me this is a platform issue. I haven't noticed > problems with autosuspend nor system suspend/resume on my platform. Do > you have any more detail on this issue? Have you consulsted QCA folks? > > Unfortunately, Greg is already queueing this patch up to all the -stable > trees, so I'm going to have to revert it yet again in Chromium > kernels... if this is a platform issue, then doing this based on VID:PID pairs is wrong no matter what. Then again, if we have really dedicated VID:PID combination for certain platforms, we could also just whitelist them in the driver. The btusb.c already contains an extra blacklist check. It is easy to add another USB ID table for a QCA whitelist. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 16-02-18 03:27, Brian Norris wrote: > Hi Hans, > > On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote: >> On 13-02-18 03:24, Brian Norris wrote: >>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote: >>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >>>> >>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >>>> has several issues (see the original commit message). An added advantage >>>> of moving over to the USB-core reset-resume handling is that it also >>>> disables autosuspend for these devices, which is similarly broken on these. >>> >>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't >>> think so -- I'm using one now. >> >> And have you manually enabled USB autosuspend for it, or are you >> running something which might have done so, e.g. powertop --auto-tune ? >> >> Because if you did not do that then you're already not using autosuspend >> for your QCA devices and this patch will change nothing. > > I use a set of udev rules that manually whitelist devices for > autosuspend. You can see it here: > > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 > > You'll find at least one Rome chip in there. > >>> Thus, this is a poor solution, which >>> negatively affects my systems. However, I see that this patch was >>> applied regardless... >> >> Note that there already is a quirk to handle broken suspend/resume >> behavior on ALL QCA devices in older kernels. Also note that the > > Yes, and the quirk was broken, and I made sure it got reverted when it > broke my devices ;) Note that the revert for this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15 Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens in btusb_open(), hence firmware loading gets interrupted." And says: "If we really want to reset the USB device, we need to do it before btusb_open(). Let's handle it in drivers/usb/core/quirks.c." It does not mention that the quirk is not necessary on some devices only that the implementation of it we had before had issues. And the original commit of the quirk: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56 Says: "There's been numerous reported instances where BTUSB_QCA_ROME bluetooth controllers stop functioning upon resume from suspend." So it may be platform specific but it is not just limited to 1 or 2 platforms I think. Note I'm not saying that I don't believe you that the quirk is not necessary on your devices. >> patches series which this commit builds on top of was already >> setting USB_QUIRK_RESET_RESUME for some devices in >> usb/core/quirks.c. >> >> All my commit does is instead of duplicating all the QCA USB-ids in >> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME >> to btusb.c so that we don't need to duplicate the USB-id tables. > > I was slightly more OK with marking specific IDs as broken, because > those IDs didn't happen to be ones that I knew were currently working. > Now you're breaking my systems again. But this time, it's more subtle > because bluetooth will still work, but we just suck more power leaving > our USB port active all the time. I see, sorry about that. Ok, so we are going to need to make the reset-on-resume quirking more fine grained. I can see 3 ways to do this: 1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the usb_device_id table inside btusb.c (I still believe duplicating most ids to usb/core/quirks.c is a bad idea). 2) Use dmi based whitelisting to opt out of reset-resume behavior on QCA btusb devices. 3) Use dmi based blacklisting which enables reset-resume behavior. In retrospect I guess 3 would have been best, but if we do that now it will cause regressions. I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the QCA usb-ids except for the ones where you know things work and which only seem to be used in working devices (based on you not having objections against the additions of the quirk for some ids to drivers/usb/core/quirks.c). And if then those usb-ids do turn out to have broken suspend on on some devices too I guess we need to move to 2. >> The result of the combination of these patches is that the custom >> DIY reset on resume handling btusb.c was doing is now replaced >> by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk. >> >> As a (desirable) side effect this also disables USB autosuspend >> for QCA devices since the USB-core does not allow USB autosuspend >> on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown >> this to be necessary on at least some QCA devices and given that >> these devices tend to loose there firmware on a suspend, it seems >> sensible to not allow autosuspend on them. > > But you're not accurately targeting the "some". AFAICT, you're wasting > power on my system. OK, so it turns out that this is a platform problem, which was not known to those involved until now. As mentioned before solution 3. from my above list of solutions would then be ideal, but I don't see how we can get there now without causing regressions where bluetooth stops working completely for people. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, >>>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >>>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >>>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >>>>> >>>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >>>>> has several issues (see the original commit message). An added advantage >>>>> of moving over to the USB-core reset-resume handling is that it also >>>>> disables autosuspend for these devices, which is similarly broken on these. >>>> >>>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't >>>> think so -- I'm using one now. >>> >>> And have you manually enabled USB autosuspend for it, or are you >>> running something which might have done so, e.g. powertop --auto-tune ? >>> >>> Because if you did not do that then you're already not using autosuspend >>> for your QCA devices and this patch will change nothing. >> I use a set of udev rules that manually whitelist devices for >> autosuspend. You can see it here: >> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 >> You'll find at least one Rome chip in there. > > >>>> Thus, this is a poor solution, which >>>> negatively affects my systems. However, I see that this patch was >>>> applied regardless... >>> >>> Note that there already is a quirk to handle broken suspend/resume >>> behavior on ALL QCA devices in older kernels. Also note that the >> Yes, and the quirk was broken, and I made sure it got reverted when it >> broke my devices ;) > > Note that the revert for this: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15 > > Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens > in btusb_open(), hence firmware loading gets interrupted." > > And says: > > "If we really want to reset the USB device, we need to do it before btusb_open(). Let's > handle it in drivers/usb/core/quirks.c." > > It does not mention that the quirk is not necessary on some devices only > that the implementation of it we had before had issues. > > And the original commit of the quirk: > > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56 > > Says: "There's been numerous reported instances where BTUSB_QCA_ROME > bluetooth controllers stop functioning upon resume from suspend." > > So it may be platform specific but it is not just limited to 1 or > 2 platforms I think. > > Note I'm not saying that I don't believe you that the quirk is not > necessary on your devices. > >>> patches series which this commit builds on top of was already >>> setting USB_QUIRK_RESET_RESUME for some devices in >>> usb/core/quirks.c. >>> >>> All my commit does is instead of duplicating all the QCA USB-ids in >>> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME >>> to btusb.c so that we don't need to duplicate the USB-id tables. >> I was slightly more OK with marking specific IDs as broken, because >> those IDs didn't happen to be ones that I knew were currently working. >> Now you're breaking my systems again. But this time, it's more subtle >> because bluetooth will still work, but we just suck more power leaving >> our USB port active all the time. > > I see, sorry about that. Ok, so we are going to need to make the > reset-on-resume quirking more fine grained. I can see 3 ways to do this: > > 1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the > usb_device_id table inside btusb.c (I still believe duplicating most > ids to usb/core/quirks.c is a bad idea). > > 2) Use dmi based whitelisting to opt out of reset-resume behavior on > QCA btusb devices. > > 3) Use dmi based blacklisting which enables reset-resume behavior. > > In retrospect I guess 3 would have been best, but if we do that now > it will cause regressions. > > I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the > QCA usb-ids except for the ones where you know things work and which > only seem to be used in working devices (based on you not having > objections against the additions of the quirk for some ids to > drivers/usb/core/quirks.c). > > And if then those usb-ids do turn out to have broken suspend on > on some devices too I guess we need to move to 2. actually if this is really platform related as Qualcomm is indicating, then we should just go with 3) and the two platforms that previously added quirks to usb/core/quirks.c and blacklist these. I am all for figuring out what is going on here. So lets blacklist these and see how this goes. Maybe there are only two bad platforms out there and we are making too much fuzz about this. Before we added quirks in the USB core these platforms were just plain broken as well. So not much different situation than before. We need to push the DMI blacklisting back into -stable as well and that means any impact of a 3rd broken platform briefly working and then be broken again is slim and also fixable via -stable. I addition we can add a module option to btusb.ko that allows us to force this flag to be set. That way testing this is easy on a vendor kernel. A bunch of the btusb quirks (and also core quirks) can be tested via module options or debugfs. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 16-02-18 12:45, Marcel Holtmann wrote: > Hi Hans, > >>>>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") >>>>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, >>>>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. >>>>>> >>>>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling >>>>>> has several issues (see the original commit message). An added advantage >>>>>> of moving over to the USB-core reset-resume handling is that it also >>>>>> disables autosuspend for these devices, which is similarly broken on these. >>>>> >>>>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't >>>>> think so -- I'm using one now. >>>> >>>> And have you manually enabled USB autosuspend for it, or are you >>>> running something which might have done so, e.g. powertop --auto-tune ? >>>> >>>> Because if you did not do that then you're already not using autosuspend >>>> for your QCA devices and this patch will change nothing. >>> I use a set of udev rules that manually whitelist devices for >>> autosuspend. You can see it here: >>> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 >>> You'll find at least one Rome chip in there. >>> >>>>> Thus, this is a poor solution, which >>>>> negatively affects my systems. However, I see that this patch was >>>>> applied regardless... >>>> >>>> Note that there already is a quirk to handle broken suspend/resume >>>> behavior on ALL QCA devices in older kernels. Also note that the >>> Yes, and the quirk was broken, and I made sure it got reverted when it >>> broke my devices ;) >> >> Note that the revert for this: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15 >> >> Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens >> in btusb_open(), hence firmware loading gets interrupted." >> >> And says: >> >> "If we really want to reset the USB device, we need to do it before btusb_open(). Let's >> handle it in drivers/usb/core/quirks.c." >> >> It does not mention that the quirk is not necessary on some devices only >> that the implementation of it we had before had issues. >> >> And the original commit of the quirk: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56 >> >> Says: "There's been numerous reported instances where BTUSB_QCA_ROME >> bluetooth controllers stop functioning upon resume from suspend." >> >> So it may be platform specific but it is not just limited to 1 or >> 2 platforms I think. >> >> Note I'm not saying that I don't believe you that the quirk is not >> necessary on your devices. >> >>>> patches series which this commit builds on top of was already >>>> setting USB_QUIRK_RESET_RESUME for some devices in >>>> usb/core/quirks.c. >>>> >>>> All my commit does is instead of duplicating all the QCA USB-ids in >>>> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME >>>> to btusb.c so that we don't need to duplicate the USB-id tables. >>> I was slightly more OK with marking specific IDs as broken, because >>> those IDs didn't happen to be ones that I knew were currently working. >>> Now you're breaking my systems again. But this time, it's more subtle >>> because bluetooth will still work, but we just suck more power leaving >>> our USB port active all the time. >> >> I see, sorry about that. Ok, so we are going to need to make the >> reset-on-resume quirking more fine grained. I can see 3 ways to do this: >> >> 1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the >> usb_device_id table inside btusb.c (I still believe duplicating most >> ids to usb/core/quirks.c is a bad idea). >> >> 2) Use dmi based whitelisting to opt out of reset-resume behavior on >> QCA btusb devices. >> >> 3) Use dmi based blacklisting which enables reset-resume behavior. >> >> In retrospect I guess 3 would have been best, but if we do that now >> it will cause regressions. >> >> I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the >> QCA usb-ids except for the ones where you know things work and which >> only seem to be used in working devices (based on you not having >> objections against the additions of the quirk for some ids to >> drivers/usb/core/quirks.c). >> >> And if then those usb-ids do turn out to have broken suspend on >> on some devices too I guess we need to move to 2. > > actually if this is really platform related as Qualcomm is indicating, then we should just go with 3) and the two platforms that previously added quirks to usb/core/quirks.c and blacklist these. I am all for figuring out what is going on here. So lets blacklist these and see how this goes. Maybe there are only two bad platforms out there and we are making too much fuzz about this. Before we added quirks in the USB core these platforms were just plain broken as well. So not much different situation than before. We need to push the DMI blacklisting back into -stable as well and that means any impact of a 3rd broken platform briefly working and then be broken again is slim and also fixable via -stable. Ok, I've asked the reporter of: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 Which is how I got involved in this to provide DMI data and I will whip up a patch for his machine and ask him to test and once that is done submit upstream. This is the only machine I'm aware of though and looking at the history of this I think there will be others. > I addition we can add a module option to btusb.ko that allows us to force this flag to be set. That way testing this is easy on a vendor kernel. A bunch of the btusb quirks (and also core quirks) can be tested via module options or debugfs. Yes a module option for this is a good idea. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+ Benson (and there are probably others that know better answers) Hi, On Fri, Feb 16, 2018 at 09:26:37AM +0100, Hans de Goede wrote: > Going a bit off-topic here, so changed the subject. > I will reply on topic in another mail. > > On 16-02-18 03:27, Brian Norris wrote: > > I use a set of udev rules that manually whitelist devices for > > autosuspend. You can see it here: > > > > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 > > > > You'll find at least one Rome chip in there. > > Oh, that is a very interesting link for the work I've been doing to > improve Linux power-consumption in general: > > https://fedoraproject.org/wiki/Changes/ImprovedLaptopBatteryLife > > I was actually planning on at least doing such a list for WWAN modems, > for btusb my approach has been to just enable it everywhere > (except for QCA devices as I got bugreports for those). > > Note that I plan to eventually submit this whitelist to the > udev rules which are part of systemd upstream, so if chromeos > is using systemd too, this is something to be aware of for you. Chrome OS does not currently use systemd, but thanks for the heads up. > Question, is the white-listing of the root and rate-limiting > hubs really necessary? I thought these have this enabled by default? This list is old and maintained by several of my team, originating from quite a ways back (i.e., much older kernels). It's quite possible that some of it is redundant today. > Also any caveats here I should be aware of? That it's only maintained for the express purpose of Chrome{device}s? There's no guarantee that there aren't platform issues with other systems, for instance :) I'm not really aware of any particular caveats otherwise. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Feb 16, 2018 at 01:10:20PM +0100, Hans de Goede wrote: > On 16-02-18 12:45, Marcel Holtmann wrote: > > Hi Hans, > > > > > > > > > Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") > > > > > > > removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, > > > > > > > instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. > > > > > > > > > > > > > > This was done because the DIY BTUSB_RESET_RESUME reset-resume handling > > > > > > > has several issues (see the original commit message). An added advantage > > > > > > > of moving over to the USB-core reset-resume handling is that it also > > > > > > > disables autosuspend for these devices, which is similarly broken on these. > > > > > > > > > > > > Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't > > > > > > think so -- I'm using one now. > > > > > > > > > > And have you manually enabled USB autosuspend for it, or are you > > > > > running something which might have done so, e.g. powertop --auto-tune ? > > > > > > > > > > Because if you did not do that then you're already not using autosuspend > > > > > for your QCA devices and this patch will change nothing. > > > > I use a set of udev rules that manually whitelist devices for > > > > autosuspend. You can see it here: > > > > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 > > > > You'll find at least one Rome chip in there. > > > > > > > > > > Thus, this is a poor solution, which > > > > > > negatively affects my systems. However, I see that this patch was > > > > > > applied regardless... > > > > > > > > > > Note that there already is a quirk to handle broken suspend/resume > > > > > behavior on ALL QCA devices in older kernels. Also note that the > > > > Yes, and the quirk was broken, and I made sure it got reverted when it > > > > broke my devices ;) > > > > > > Note that the revert for this: > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15 > > > > > > Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens > > > in btusb_open(), hence firmware loading gets interrupted." > > > > > > And says: > > > > > > "If we really want to reset the USB device, we need to do it before btusb_open(). Let's > > > handle it in drivers/usb/core/quirks.c." Just a note: I didn't write that part ;) I also didn't have evidence to say that *no* QCA chipset was broken. I just knew that on my platform with my QCA ROME chipset, there was no problem. > > > It does not mention that the quirk is not necessary on some devices only > > > that the implementation of it we had before had issues. > > > > > > And the original commit of the quirk: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56 > > > > > > Says: "There's been numerous reported instances where BTUSB_QCA_ROME > > > bluetooth controllers stop functioning upon resume from suspend." > > > > > > So it may be platform specific but it is not just limited to 1 or > > > 2 platforms I think. Understood for most of the above. But I'll stick a "citation needed" on the "numerous reported instances" claim. As I note below, I believe that is the crux of the problem: understanding what platforms actually reported this problem. All attempts to "fix" it have broken things in one way or another, which have just caused noise since then. > > > Note I'm not saying that I don't believe you that the quirk is not > > > necessary on your devices. > > > > > > > > patches series which this commit builds on top of was already > > > > > setting USB_QUIRK_RESET_RESUME for some devices in > > > > > usb/core/quirks.c. > > > > > > > > > > All my commit does is instead of duplicating all the QCA USB-ids in > > > > > usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME > > > > > to btusb.c so that we don't need to duplicate the USB-id tables. > > > > I was slightly more OK with marking specific IDs as broken, because > > > > those IDs didn't happen to be ones that I knew were currently working. > > > > Now you're breaking my systems again. But this time, it's more subtle > > > > because bluetooth will still work, but we just suck more power leaving > > > > our USB port active all the time. > > > > > > I see, sorry about that. Ok, so we are going to need to make the > > > reset-on-resume quirking more fine grained. I can see 3 ways to do this: > > > > > > 1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the > > > usb_device_id table inside btusb.c (I still believe duplicating most > > > ids to usb/core/quirks.c is a bad idea). > > > > > > 2) Use dmi based whitelisting to opt out of reset-resume behavior on > > > QCA btusb devices. > > > > > > 3) Use dmi based blacklisting which enables reset-resume behavior. > > > > > > In retrospect I guess 3 would have been best, but if we do that now > > > it will cause regressions. > > > > > > I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the > > > QCA usb-ids except for the ones where you know things work and which > > > only seem to be used in working devices (based on you not having > > > objections against the additions of the quirk for some ids to > > > drivers/usb/core/quirks.c). > > > > > > And if then those usb-ids do turn out to have broken suspend on > > > on some devices too I guess we need to move to 2. > > > > actually if this is really platform related as Qualcomm is indicating, then we should just go with 3) and the two platforms that previously added quirks to usb/core/quirks.c and blacklist these. I am all for figuring out what is going on here. So lets blacklist these and see how this goes. Maybe there are only two bad platforms out there and we are making too much fuzz about this. Before we added quirks in the USB core these platforms were just plain broken as well. So not much different situation than before. We need to push the DMI blacklisting back into -stable as well and that means any impact of a 3rd broken platform briefly working and then be broken again is slim and also fixable via -stable. > > Ok, I've asked the reporter of: > > https://bugzilla.redhat.com/show_bug.cgi?id=1514836 Are you even sure that this reporter is seeing the original symptom at all (BT loses power, and therefore firmware)? Their report shows them running 4.15, which had this commit: fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume which is admittedly completely broken. It breaks even perfectly working BT/USB devices, like mine. That's where I first complained, and we got this into 4.16-rc1: 7d06d5895c15 Revert "Bluetooth: btusb: fix QCA Rome suspend/resume" Isn't it possible your reporter has no further problem, and none if this is actually important to them? I'd just caution you to be careful before assuming you need to add blacklist info for their DMI... As I read it, you need to investigate who are the "numerous reported instances" that generated commit fd865802c66b in the first place. That's where this mess started, IIUC. But otherwise, yes, option 3 sounds OK. FWIW, my systems are ARM based and don't have DMI data, so option 2 wouldn't work. > Which is how I got involved in this to provide DMI data and > I will whip up a patch for his machine and ask him to test > and once that is done submit upstream. This is the only machine > I'm aware of though and looking at the history of this I think > there will be others. > > > I addition we can add a module option to btusb.ko that allows us to force this flag to be set. That way testing this is easy on a vendor kernel. A bunch of the btusb quirks (and also core quirks) can be tested via module options or debugfs. > > Yes a module option for this is a good idea. That too. Or, would be nice to be able to force it either way. So if someone erroneously sticks the flag in this driver (yet again), we can easily undo that. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 16 Feb 2018, at 8:10 PM, Hans de Goede <hdegoede@redhat.com> wrote: > > > Ok, I've asked the reporter of: > > https://bugzilla.redhat.com/show_bug.cgi?id=1514836 > > Which is how I got involved in this to provide DMI data and > I will whip up a patch for his machine and ask him to test > and once that is done submit upstream. This is the only machine > I'm aware of though and looking at the history of this I think > there will be others. I have one machine needs the quirk. I’ll also send a patch to match its DMI. Kai-Heng > >> I addition we can add a module option to btusb.ko that allows us to force this flag to be set. That way testing this is easy on a vendor kernel. A bunch of the btusb quirks (and also core quirks) can be tested via module options or debugfs. > > Yes a module option for this is a good idea. > > Regards, > > Hans > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
HI, On 16-02-18 18:59, Brian Norris wrote: > Hi, > > On Fri, Feb 16, 2018 at 01:10:20PM +0100, Hans de Goede wrote: >> On 16-02-18 12:45, Marcel Holtmann wrote: <snip> >>> actually if this is really platform related as Qualcomm is indicating, then we should just go with 3) and the two platforms that previously added quirks to usb/core/quirks.c and blacklist these. I am all for figuring out what is going on here. So lets blacklist these and see how this goes. Maybe there are only two bad platforms out there and we are making too much fuzz about this. Before we added quirks in the USB core these platforms were just plain broken as well. So not much different situation than before. We need to push the DMI blacklisting back into -stable as well and that means any impact of a 3rd broken platform briefly working and then be broken again is slim and also fixable via -stable. >> >> Ok, I've asked the reporter of: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1514836 > > Are you even sure that this reporter is seeing the original symptom at > all (BT loses power, and therefore firmware)? Their report shows them > running 4.15, which had this commit: > > fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume > > which is admittedly completely broken. It breaks even perfectly working > BT/USB devices, like mine. That's where I first complained, and we got > this into 4.16-rc1: > > 7d06d5895c15 Revert "Bluetooth: btusb: fix QCA Rome suspend/resume" > > Isn't it possible your reporter has no further problem, and none if this > is actually important to them? I'd just caution you to be careful before > assuming you need to add blacklist info for their DMI... Thanks, that is a good question. His problems only started when I enabled usb-autosuspend by default for btusb devices and he got things working by adding "btusb.enable_autosuspend=n" on the kernel commandline, so he was not hitting the firmware loading race introduced by fd865802c66b and runtime suspend/resume is really broken for him. > As I read it, you need to investigate who are the "numerous reported > instances" that generated commit fd865802c66b in the first place. That's > where this mess started, IIUC. > > But otherwise, yes, option 3 sounds OK. FWIW, my systems are ARM based > and don't have DMI data, so option 2 wouldn't work. Right I think we all agree that the new plan now is to go back to QCA behaving normally wrt (runtime) suspend/resume and then set the USB-core RESET_RESUME quirk (which does not have the firmware loading race) based on a DMI blacklist. I only have the one report for which I will write a patch implementing this new policy soonish. And Kai-Heng Feng has another report which might even be the machine. I certainly would not be surprised if it is another Lenovo machine. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 16-02-18 17:49, Brian Norris wrote: > + Benson (and there are probably others that know better answers) > > Hi, > > On Fri, Feb 16, 2018 at 09:26:37AM +0100, Hans de Goede wrote: >> Going a bit off-topic here, so changed the subject. >> I will reply on topic in another mail. >> >> On 16-02-18 03:27, Brian Norris wrote: >>> I use a set of udev rules that manually whitelist devices for >>> autosuspend. You can see it here: >>> >>> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83 >>> >>> You'll find at least one Rome chip in there. >> >> Oh, that is a very interesting link for the work I've been doing to >> improve Linux power-consumption in general: >> >> https://fedoraproject.org/wiki/Changes/ImprovedLaptopBatteryLife >> >> I was actually planning on at least doing such a list for WWAN modems, >> for btusb my approach has been to just enable it everywhere >> (except for QCA devices as I got bugreports for those). >> >> Note that I plan to eventually submit this whitelist to the >> udev rules which are part of systemd upstream, so if chromeos >> is using systemd too, this is something to be aware of for you. > > Chrome OS does not currently use systemd, but thanks for the heads up. > >> Question, is the white-listing of the root and rate-limiting >> hubs really necessary? I thought these have this enabled by default? > > This list is old and maintained by several of my team, originating from > quite a ways back (i.e., much older kernels). It's quite possible that > some of it is redundant today. Ok, I double checked and it seems that explicitly setting power/control to auto for any USB hub is not necessary as they all default to auto now. So FWIW you may want to consider removing this. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Sorry if I'm a little slow to follow up here. This hasn't been my top priority... On Mon, Feb 19, 2018 at 11:17:24AM +0100, Hans de Goede wrote: > On 16-02-18 18:59, Brian Norris wrote: > > On Fri, Feb 16, 2018 at 01:10:20PM +0100, Hans de Goede wrote: > > > Ok, I've asked the reporter of: > > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1514836 > > > > Are you even sure that this reporter is seeing the original symptom at > > all (BT loses power, and therefore firmware)? Their report shows them > > running 4.15, which had this commit: > > > > fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume > > > > which is admittedly completely broken. It breaks even perfectly working > > BT/USB devices, like mine. That's where I first complained, and we got > > this into 4.16-rc1: > > > > 7d06d5895c15 Revert "Bluetooth: btusb: fix QCA Rome suspend/resume" > > > > Isn't it possible your reporter has no further problem, and none if this > > is actually important to them? I'd just caution you to be careful before > > assuming you need to add blacklist info for their DMI... > > Thanks, that is a good question. His problems only started when I > enabled usb-autosuspend by default for btusb devices and he got things > working by adding "btusb.enable_autosuspend=n" on the kernel commandline, > so he was not hitting the firmware loading race introduced by > fd865802c66b and runtime suspend/resume is really broken for him. Hmm? I'm not sure I completely follow here when you say "he was not hitting the firmware loading race". If things were functioning fine with system suspend (but not with autosuspend), then he's not seeing the controller (quoting commit fd865802c66b) "losing power during suspend". So, that would suggest he could only be seeing the race (as I was), and that his machine does not deserve a RESET_RESUME quirk? Or maybe I'm really misunderstanding. > > As I read it, you need to investigate who are the "numerous reported > > instances" that generated commit fd865802c66b in the first place. That's > > where this mess started, IIUC. > > > But otherwise, yes, option 3 sounds OK. FWIW, my systems are ARM based > > and don't have DMI data, so option 2 wouldn't work. > > Right I think we all agree that the new plan now is to go back to > QCA behaving normally wrt (runtime) suspend/resume and then set the > USB-core RESET_RESUME quirk (which does not have the firmware > loading race) based on a DMI blacklist. > > I only have the one report for which I will write a patch implementing > this new policy soonish. And Kai-Heng Feng has another report which > might even be the machine. I certainly would not be surprised if it > is another Lenovo machine. It seems like you folks moved forward on that one. Thanks. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 23-02-18 04:12, Brian Norris wrote: > Hi Hans, > > Sorry if I'm a little slow to follow up here. This hasn't been my > top priority... > > On Mon, Feb 19, 2018 at 11:17:24AM +0100, Hans de Goede wrote: >> On 16-02-18 18:59, Brian Norris wrote: >>> On Fri, Feb 16, 2018 at 01:10:20PM +0100, Hans de Goede wrote: >>>> Ok, I've asked the reporter of: >>>> >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1514836 >>> >>> Are you even sure that this reporter is seeing the original symptom at >>> all (BT loses power, and therefore firmware)? Their report shows them >>> running 4.15, which had this commit: >>> >>> fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume >>> >>> which is admittedly completely broken. It breaks even perfectly working >>> BT/USB devices, like mine. That's where I first complained, and we got >>> this into 4.16-rc1: >>> >>> 7d06d5895c15 Revert "Bluetooth: btusb: fix QCA Rome suspend/resume" >>> >>> Isn't it possible your reporter has no further problem, and none if this >>> is actually important to them? I'd just caution you to be careful before >>> assuming you need to add blacklist info for their DMI... >> >> Thanks, that is a good question. His problems only started when I >> enabled usb-autosuspend by default for btusb devices and he got things >> working by adding "btusb.enable_autosuspend=n" on the kernel commandline, >> so he was not hitting the firmware loading race introduced by >> fd865802c66b and runtime suspend/resume is really broken for him. > > Hmm? I'm not sure I completely follow here when you say "he was not > hitting the firmware loading race". If things were functioning fine with > system suspend (but not with autosuspend), then he's not seeing the > controller (quoting commit fd865802c66b) "losing power during suspend". He was running a kernel with the original "fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume" commit, which fixes regular suspend for devices which are "losing power during suspend", but does nothing for runtime-suspend. He ran tests both with and without runtime-pm enabled with that same kernel and he needed to disable runtime-pm to get working bluetooth. > So, that would suggest he could only be seeing the race (as I was), and > that his machine does not deserve a RESET_RESUME quirk? I hope my above answer helps to clarify why I believe the quirk is necessary on his machine. Regards, Hans > > Or maybe I'm really misunderstanding. > >>> As I read it, you need to investigate who are the "numerous reported >>> instances" that generated commit fd865802c66b in the first place. That's >>> where this mess started, IIUC. > >>> But otherwise, yes, option 3 sounds OK. FWIW, my systems are ARM based >>> and don't have DMI data, so option 2 wouldn't work. >> >> Right I think we all agree that the new plan now is to go back to >> QCA behaving normally wrt (runtime) suspend/resume and then set the >> USB-core RESET_RESUME quirk (which does not have the firmware >> loading race) based on a DMI blacklist. >> >> I only have the one report for which I will write a patch implementing >> this new policy soonish. And Kai-Heng Feng has another report which >> might even be the machine. I certainly would not be surprised if it >> is another Lenovo machine. > > It seems like you folks moved forward on that one. Thanks. > > Brian > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> wrote: > On 23-02-18 04:12, Brian Norris wrote: >> Hmm? I'm not sure I completely follow here when you say "he was not >> hitting the firmware loading race". If things were functioning fine with >> system suspend (but not with autosuspend), then he's not seeing the >> controller (quoting commit fd865802c66b) "losing power during suspend". > > > He was running a kernel with the original "fd865802c66b Bluetooth: btusb: > fix QCA Rome suspend/resume" commit, which fixes regular suspend for > devices which are "losing power during suspend", but does nothing for > runtime-suspend. > > He ran tests both with and without runtime-pm enabled with that same kernel > and he needed to disable runtime-pm to get working bluetooth. Did he ever test with commit fd865802c66b reverted? My symptoms were exactly the same as you described. BT was broken as of v4.14 if I had runtime suspend enabled. Things were fine if I either (a) reverted the patch or (b) disabled runtime suspend. I obviously preferred (a), which is why I continued to complain :) Did your tester ever try (a)? If not, then I don't think you've really ensured that he really needed a "fixed" version; he may not have needed the patch at all. Or an alternative question: did that system work on an older Fedora release (and presumably an older kernel)? If so, then he probably also did not need that patch. >> So, that would suggest he could only be seeing the race (as I was), and >> that his machine does not deserve a RESET_RESUME quirk? > > > I hope my above answer helps to clarify why I believe the quirk is > necessary on his machine. I'm sorry, but no it doesn't. If anything, it suggests to me even more that it may not have been necessary. But hey, I'm not personally going to notice if random Lenovo laptops are wasting power. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 27-02-18 03:29, Brian Norris wrote: > On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> On 23-02-18 04:12, Brian Norris wrote: >>> Hmm? I'm not sure I completely follow here when you say "he was not >>> hitting the firmware loading race". If things were functioning fine with >>> system suspend (but not with autosuspend), then he's not seeing the >>> controller (quoting commit fd865802c66b) "losing power during suspend". >> >> >> He was running a kernel with the original "fd865802c66b Bluetooth: btusb: >> fix QCA Rome suspend/resume" commit, which fixes regular suspend for >> devices which are "losing power during suspend", but does nothing for >> runtime-suspend. >> >> He ran tests both with and without runtime-pm enabled with that same kernel >> and he needed to disable runtime-pm to get working bluetooth. > > Did he ever test with commit fd865802c66b reverted? > > My symptoms were exactly the same as you described. BT was broken as > of v4.14 if I had runtime suspend enabled. Things were fine if I > either (a) reverted the patch or (b) disabled runtime suspend. I > obviously preferred (a), which is why I continued to complain :) > > Did your tester ever try (a)? If not, then I don't think you've really > ensured that he really needed a "fixed" version; he may not have > needed the patch at all. > > Or an alternative question: did that system work on an older Fedora > release (and presumably an older kernel)? If so, then he probably also > did not need that patch. > >>> So, that would suggest he could only be seeing the race (as I was), and >>> that his machine does not deserve a RESET_RESUME quirk? >> >> >> I hope my above answer helps to clarify why I believe the quirk is >> necessary on his machine. > > I'm sorry, but no it doesn't. If anything, it suggests to me even more > that it may not have been necessary. Ok, I've started another test-kernel build for the reporter this time without any quirks at all and I've asked him to test. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 27-02-18 15:07, Hans de Goede wrote: > Hi, > > On 27-02-18 03:29, Brian Norris wrote: >> On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>> On 23-02-18 04:12, Brian Norris wrote: >>>> Hmm? I'm not sure I completely follow here when you say "he was not >>>> hitting the firmware loading race". If things were functioning fine with >>>> system suspend (but not with autosuspend), then he's not seeing the >>>> controller (quoting commit fd865802c66b) "losing power during suspend". >>> >>> >>> He was running a kernel with the original "fd865802c66b Bluetooth: btusb: >>> fix QCA Rome suspend/resume" commit, which fixes regular suspend for >>> devices which are "losing power during suspend", but does nothing for >>> runtime-suspend. >>> >>> He ran tests both with and without runtime-pm enabled with that same kernel >>> and he needed to disable runtime-pm to get working bluetooth. >> >> Did he ever test with commit fd865802c66b reverted? >> >> My symptoms were exactly the same as you described. BT was broken as >> of v4.14 if I had runtime suspend enabled. Things were fine if I >> either (a) reverted the patch or (b) disabled runtime suspend. I >> obviously preferred (a), which is why I continued to complain :) >> >> Did your tester ever try (a)? If not, then I don't think you've really >> ensured that he really needed a "fixed" version; he may not have >> needed the patch at all. >> >> Or an alternative question: did that system work on an older Fedora >> release (and presumably an older kernel)? If so, then he probably also >> did not need that patch. >> >>>> So, that would suggest he could only be seeing the race (as I was), and >>>> that his machine does not deserve a RESET_RESUME quirk? >>> >>> >>> I hope my above answer helps to clarify why I believe the quirk is >>> necessary on his machine. >> >> I'm sorry, but no it doesn't. If anything, it suggests to me even more >> that it may not have been necessary. > > Ok, I've started another test-kernel build for the reporter this time > without any quirks at all and I've asked him to test. You were right, the Yoga 920 works fine without any quirks, thank you for being persistent on getting this tested properly. I will submit a patch dropping the Yoga 920 from the btusb_needs_reset_resume_table. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry for being a bit slow to respond to this. I'm the original author of commit: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56 > Says: "There's been numerous reported instances where BTUSB_QCA_ROME > bluetooth controllers stop functioning upon resume from suspend." > So it may be platform specific but it is not just limited to 1 or >2 platforms I think. This issue might very well be limited to just a few platforms. Here's a link to the original bug report I submitted. https://bugzilla.kernel.org/show_bug.cgi?id=193571 There was a number of people who reported they were having similar issues with various QCA Rome bluetooth devices. The userspace fix that seemed to work for everyone was resetting the usb device. ie #!/bin/bash echo 0 > /sys/bus/usb/devices/1-4/authorized echo 1 > /sys/bus/usb/devices/1-4/authorized or #!/usr/bin/python from usb.core import find as finddev dev = finddev(idVendor=0x0cf3, idProduct=0xe300) dev.reset() It's difficult to ascertain what the underlying issue was for each person (and associated device) who commented. I can only speak authoritatively for my own device. It's a Samsung ATIV Book 9 12.2 (2015) laptop that contains an integrated bluetooth controller that's attached to the internal usb bus. # lsusb Bus 001 Device 005: ID 0cf3:e300 Atheros Communications, Inc. After suspending the laptop for an extended length of time (sometimes it would take an hour or two before issue occurred), and resuming --the bluetooth controller would not come back on-line. After reading through various forums, I suspected the issue I was experiencing was due to the bluetooth device losing it's firmware during suspend. The original patch I created did fix the issue. (I do realize that targeting all QCA Rome chipset's was the wrong decision). Hans de Goede's rewritten version (commit: 61f5acea8737) also worked for my device. Since, some people think that the underlying issue may been fixed elsewhere in the kernel. I'm going to remove Hans' commit and see if the issue persists (with kernel 4.15.6). I'll report back tomorrow with the results. -Leif On Wed, Feb 28, 2018 at 11:54 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 27-02-18 15:07, Hans de Goede wrote: >> >> Hi, >> >> On 27-02-18 03:29, Brian Norris wrote: >>> >>> On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> >>> wrote: >>>> >>>> On 23-02-18 04:12, Brian Norris wrote: >>>>> >>>>> Hmm? I'm not sure I completely follow here when you say "he was not >>>>> hitting the firmware loading race". If things were functioning fine >>>>> with >>>>> system suspend (but not with autosuspend), then he's not seeing the >>>>> controller (quoting commit fd865802c66b) "losing power during suspend". >>>> >>>> >>>> >>>> He was running a kernel with the original "fd865802c66b Bluetooth: >>>> btusb: >>>> fix QCA Rome suspend/resume" commit, which fixes regular suspend for >>>> devices which are "losing power during suspend", but does nothing for >>>> runtime-suspend. >>>> >>>> He ran tests both with and without runtime-pm enabled with that same >>>> kernel >>>> and he needed to disable runtime-pm to get working bluetooth. >>> >>> >>> Did he ever test with commit fd865802c66b reverted? >>> >>> My symptoms were exactly the same as you described. BT was broken as >>> of v4.14 if I had runtime suspend enabled. Things were fine if I >>> either (a) reverted the patch or (b) disabled runtime suspend. I >>> obviously preferred (a), which is why I continued to complain :) >>> >>> Did your tester ever try (a)? If not, then I don't think you've really >>> ensured that he really needed a "fixed" version; he may not have >>> needed the patch at all. >>> >>> Or an alternative question: did that system work on an older Fedora >>> release (and presumably an older kernel)? If so, then he probably also >>> did not need that patch. >>> >>>>> So, that would suggest he could only be seeing the race (as I was), and >>>>> that his machine does not deserve a RESET_RESUME quirk? >>>> >>>> >>>> >>>> I hope my above answer helps to clarify why I believe the quirk is >>>> necessary on his machine. >>> >>> >>> I'm sorry, but no it doesn't. If anything, it suggests to me even more >>> that it may not have been necessary. >> >> >> Ok, I've started another test-kernel build for the reporter this time >> without any quirks at all and I've asked him to test. > > > You were right, the Yoga 920 works fine without any quirks, thank you > for being persistent on getting this tested properly. > > I will submit a patch dropping the Yoga 920 from the > btusb_needs_reset_resume_table. > > Regards, > > Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hans, Everything is now working perfectly fine without commit 61f5acea8737. I suspended the laptop for hours at a time, the bluetooth controller comes up fine every time. Whatever the underlying issue was, it's been resolved elsewhere in the kernel. There's no longer any need to implement any sort of RESET_RESUME logic for this device (0cf3:e300). I appreciate all of the work that you and other developers do. Thanks for staying on top of this issue! -Leif On Fri, Mar 9, 2018 at 1:56 AM, Leif Liddy <leif.linux@gmail.com> wrote: > Sorry for being a bit slow to respond to this. > > I'm the original author of commit: > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56 > >> Says: "There's been numerous reported instances where BTUSB_QCA_ROME >> bluetooth controllers stop functioning upon resume from suspend." > >> So it may be platform specific but it is not just limited to 1 or >>2 platforms I think. > > This issue might very well be limited to just a few platforms. > Here's a link to the original bug report I submitted. > > https://bugzilla.kernel.org/show_bug.cgi?id=193571 > > There was a number of people who reported they were having similar > issues with various QCA Rome bluetooth devices. > The userspace fix that seemed to work for everyone was resetting the usb device. > > ie > > #!/bin/bash > echo 0 > /sys/bus/usb/devices/1-4/authorized > echo 1 > /sys/bus/usb/devices/1-4/authorized > > or > > #!/usr/bin/python > from usb.core import find as finddev > dev = finddev(idVendor=0x0cf3, idProduct=0xe300) > dev.reset() > > It's difficult to ascertain what the underlying issue was for each > person (and associated device) who commented. I can only speak > authoritatively for my own device. > It's a Samsung ATIV Book 9 12.2 (2015) laptop that contains an > integrated bluetooth controller that's attached to the internal usb > bus. > > # lsusb > Bus 001 Device 005: ID 0cf3:e300 Atheros Communications, Inc. > > After suspending the laptop for an extended length of time (sometimes > it would take an hour or two before issue occurred), and resuming > --the bluetooth controller would not come back on-line. After reading > through various forums, I suspected the issue I was experiencing was > due to the bluetooth device losing it's firmware during suspend. > The original patch I created did fix the issue. (I do realize that > targeting all QCA Rome chipset's was the wrong decision). Hans de > Goede's rewritten version (commit: 61f5acea8737) also worked for my > device. Since, some people think that the underlying issue may been > fixed elsewhere in the kernel. I'm going to remove Hans' commit and > see if the issue persists (with kernel 4.15.6). I'll report back > tomorrow with the results. > > > -Leif > > On Wed, Feb 28, 2018 at 11:54 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> >> On 27-02-18 15:07, Hans de Goede wrote: >>> >>> Hi, >>> >>> On 27-02-18 03:29, Brian Norris wrote: >>>> >>>> On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> >>>> wrote: >>>>> >>>>> On 23-02-18 04:12, Brian Norris wrote: >>>>>> >>>>>> Hmm? I'm not sure I completely follow here when you say "he was not >>>>>> hitting the firmware loading race". If things were functioning fine >>>>>> with >>>>>> system suspend (but not with autosuspend), then he's not seeing the >>>>>> controller (quoting commit fd865802c66b) "losing power during suspend". >>>>> >>>>> >>>>> >>>>> He was running a kernel with the original "fd865802c66b Bluetooth: >>>>> btusb: >>>>> fix QCA Rome suspend/resume" commit, which fixes regular suspend for >>>>> devices which are "losing power during suspend", but does nothing for >>>>> runtime-suspend. >>>>> >>>>> He ran tests both with and without runtime-pm enabled with that same >>>>> kernel >>>>> and he needed to disable runtime-pm to get working bluetooth. >>>> >>>> >>>> Did he ever test with commit fd865802c66b reverted? >>>> >>>> My symptoms were exactly the same as you described. BT was broken as >>>> of v4.14 if I had runtime suspend enabled. Things were fine if I >>>> either (a) reverted the patch or (b) disabled runtime suspend. I >>>> obviously preferred (a), which is why I continued to complain :) >>>> >>>> Did your tester ever try (a)? If not, then I don't think you've really >>>> ensured that he really needed a "fixed" version; he may not have >>>> needed the patch at all. >>>> >>>> Or an alternative question: did that system work on an older Fedora >>>> release (and presumably an older kernel)? If so, then he probably also >>>> did not need that patch. >>>> >>>>>> So, that would suggest he could only be seeing the race (as I was), and >>>>>> that his machine does not deserve a RESET_RESUME quirk? >>>>> >>>>> >>>>> >>>>> I hope my above answer helps to clarify why I believe the quirk is >>>>> necessary on his machine. >>>> >>>> >>>> I'm sorry, but no it doesn't. If anything, it suggests to me even more >>>> that it may not have been necessary. >>> >>> >>> Ok, I've started another test-kernel build for the reporter this time >>> without any quirks at all and I've asked him to test. >> >> >> You were right, the Yoga 920 works fine without any quirks, thank you >> for being persistent on getting this tested properly. >> >> I will submit a patch dropping the Yoga 920 from the >> btusb_needs_reset_resume_table. >> >> Regards, >> >> Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 4764100a5888..c4689f03220f 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -23,6 +23,7 @@ #include <linux/module.h> #include <linux/usb.h> +#include <linux/usb/quirks.h> #include <linux/firmware.h> #include <linux/of_device.h> #include <linux/of_irq.h> @@ -388,9 +389,8 @@ static const struct usb_device_id blacklist_table[] = { #define BTUSB_FIRMWARE_LOADED 7 #define BTUSB_FIRMWARE_FAILED 8 #define BTUSB_BOOTING 9 -#define BTUSB_RESET_RESUME 10 -#define BTUSB_DIAG_RUNNING 11 -#define BTUSB_OOB_WAKE_ENABLED 12 +#define BTUSB_DIAG_RUNNING 10 +#define BTUSB_OOB_WAKE_ENABLED 11 struct btusb_data { struct hci_dev *hdev; @@ -3118,6 +3118,12 @@ static int btusb_probe(struct usb_interface *intf, if (id->driver_info & BTUSB_QCA_ROME) { data->setup_on_usb = btusb_setup_qca; hdev->set_bdaddr = btusb_set_bdaddr_ath3012; + + /* QCA Rome devices lose their updated firmware over suspend, + * but the USB hub doesn't notice any status change. + * explicitly request a device reset on resume. + */ + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; } #ifdef CONFIG_BT_HCIBTUSB_RTL @@ -3128,7 +3134,7 @@ static int btusb_probe(struct usb_interface *intf, * but the USB hub doesn't notice any status change. * Explicitly request a device reset on resume. */ - set_bit(BTUSB_RESET_RESUME, &data->flags); + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; } #endif @@ -3297,14 +3303,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) enable_irq(data->oob_wake_irq); } - /* Optionally request a device reset on resume, but only when - * wakeups are disabled. If wakeups are enabled we assume the - * device will stay powered up throughout suspend. - */ - if (test_bit(BTUSB_RESET_RESUME, &data->flags) && - !device_may_wakeup(&data->udev->dev)) - data->udev->reset_resume = 1; - return 0; }
Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. This was done because the DIY BTUSB_RESET_RESUME reset-resume handling has several issues (see the original commit message). An added advantage of moving over to the USB-core reset-resume handling is that it also disables autosuspend for these devices, which is similarly broken on these. But there are 2 issues with this approach: 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek devices. 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been added to usb/core/quirks.c and if we fix the Realtek case the same way we need to add an additional 14 entries. So in essence we need to duplicate a large part of the usb_device_id table in btusb.c in usb/core/quirks.c and manually keep them in sync. This commit instead restores setting a reset-resume quirk for QCA devices in the btusb.c code, avoiding the duplicate usb_device_id table problem. This commit avoids the problems with the original DIY BTUSB_RESET_RESUME code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the usb_device. This commit also moves the BTUSB_REALTEK case over to directly setting the USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused BTUSB_RESET_RESUME code. BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") Cc: stable@vger.kernel.org Cc: Leif Liddy <leif.linux@gmail.com> Cc: Matthias Kaehlcke <mka@chromium.org> Cc: Brian Norris <briannorris@chromium.org> Cc: Daniel Drake <drake@endlessm.com> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Note: 1) Once this has been merged, the 2 commits adding QCA device entries to drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next. 2) I don't have any of the affected devices, please test --- drivers/bluetooth/btusb.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)