[5.12,regression,fix,0/1] Bluetooth: btusb: Revert "Fix the autosuspend enable and disable"

Message

Hans de Goede Feb. 18, 2021, 12:37 p.m. UTC

Hi All,

From the commit msg:

"""
drivers/usb/core/hub.c: usb_new_device() contains the following:

        /* By default, forbid autosuspend for all devices.  It will be
         * allowed for hubs during binding.
         */
        usb_disable_autosuspend(udev);

So for anything which is not a hub, such as btusb devices, autosuspend is
disabled by default and we MUST call usb_enable_autosuspend(udev) to
enable it.

This means that the "Fix the autosuspend enable and disable" commit,
which drops the usb_enable_autosuspend() call when the enable_autosuspend
module option is true, is completely wrong, revert it.
"""

Hui, I guess that what you were seeing is caused by:
/lib/udev/hwdb.d/60-autosuspend-chromiumos.hwdb

Which enables autosuspend on a bunch of USB devices based on VID:PID,
overruling the kernel defaults. This is done to get better power-consumption
with devices where it is known that it is safe to do this.
 
I guess that that the device you were testing this with is on that list.
So the proper fix would be to edit that file and remove your VID:PID from it.

Hui, also next time please try to Cc the original author of the code you
are modifying. A simple "git blame drivers/bluetooth/btusb.c" would have
found you commit eff2d68ca738 ("Bluetooth: btusb: Add a Kconfig option to
enable USB autosuspend by default") and then you could have added me to
the Cc and I could have nacked the patch before it got merged.

I happen to spot this this time since I was looking into some other
btusb issue. But if I had not spotted this, this would have caused
a significant power-consumption regression on many laptops.

Btusb might not look like a big consumer, but if it does not autosuspend
it often is the only USB device not autosuspending, keeping the XHCI
controller awake, which in turn is keeping a whole power-plane awake on
what once used to be the southbridge. At least on Skylake era hw this
could lead to an extra idle powerconsumption of 1W. So a small change
can cause a big impact.

Regards,

Hans

Comments

Hui Wang Feb. 18, 2021, 1:32 p.m. UTC | #1

On 2/18/21 8:37 PM, Hans de Goede wrote:
> Hi All,
>
> >From the commit msg:
>
> """
> drivers/usb/core/hub.c: usb_new_device() contains the following:
>
>          /* By default, forbid autosuspend for all devices.  It will be
>           * allowed for hubs during binding.
>           */
>          usb_disable_autosuspend(udev);
>
> So for anything which is not a hub, such as btusb devices, autosuspend is
> disabled by default and we MUST call usb_enable_autosuspend(udev) to
> enable it.
>
> This means that the "Fix the autosuspend enable and disable" commit,
> which drops the usb_enable_autosuspend() call when the enable_autosuspend
> module option is true, is completely wrong, revert it.
> """
>
> Hui, I guess that what you were seeing is caused by:
> /lib/udev/hwdb.d/60-autosuspend-chromiumos.hwdb

Hi Hans,

You are right, the VID:PID of the BT adapter on my machine is in that 
file, the autosuspend is enabled by udev instead of kernel. I tested on 
another machine whose BT adapter's ID is not in that file, the 
autosuspend is controlled by btusb.enable_autosuspend=0.

Your reverting patch is correct.

> Which enables autosuspend on a bunch of USB devices based on VID:PID,
> overruling the kernel defaults. This is done to get better power-consumption
> with devices where it is known that it is safe to do this.
>   
> I guess that that the device you were testing this with is on that list.
> So the proper fix would be to edit that file and remove your VID:PID from it.
>
> Hui, also next time please try to Cc the original author of the code you
> are modifying. A simple "git blame drivers/bluetooth/btusb.c" would have
> found you commit eff2d68ca738 ("Bluetooth: btusb: Add a Kconfig option to
> enable USB autosuspend by default") and then you could have added me to
> the Cc and I could have nacked the patch before it got merged.
OK, got it. will be careful and will Cc to the original author next time.
> I happen to spot this this time since I was looking into some other
> btusb issue. But if I had not spotted this, this would have caused
> a significant power-consumption regression on many laptops.
>
> Btusb might not look like a big consumer, but if it does not autosuspend
> it often is the only USB device not autosuspending, keeping the XHCI
> controller awake, which in turn is keeping a whole power-plane awake on
> what once used to be the southbridge. At least on Skylake era hw this
> could lead to an extra idle powerconsumption of 1W. So a small change
> can cause a big impact.
>
> Regards,
>
> Hans
>     
>