2021-02-18 14:02:28

by Hans de Goede

[permalink] [raw]
Subject: [PATCH 5.12 regression fix 0/1] Bluetooth: btusb: Revert "Fix the autosuspend enable and disable"

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



2021-02-18 14:02:29

by Hans de Goede

[permalink] [raw]
Subject: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the autosuspend enable and disable

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.

This reverts commit 7bd9fb058d77213130e4b3e594115c028b708e7e.

Cc: Hui Wang <[email protected]>
Fixes: 7bd9fb058d77 ("Bluetooth: btusb: Fix the autosuspend enable and disable")
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/bluetooth/btusb.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 54a4f86f32e2..03b83aa91277 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -4627,8 +4627,8 @@ static int btusb_probe(struct usb_interface *intf,
data->diag = NULL;
}

- if (!enable_autosuspend)
- usb_disable_autosuspend(data->udev);
+ if (enable_autosuspend)
+ usb_enable_autosuspend(data->udev);

err = hci_register_dev(hdev);
if (err < 0)
@@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interface *intf)
gpiod_put(data->reset_gpio);

hci_free_dev(hdev);
-
- if (!enable_autosuspend)
- usb_enable_autosuspend(data->udev);
}

#ifdef CONFIG_PM
--
2.30.1

2021-02-18 14:06:44

by Hui Wang

[permalink] [raw]
Subject: Re: [PATCH 5.12 regression fix 0/1] Bluetooth: btusb: Revert "Fix the autosuspend enable and disable"


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
>
>

2021-02-18 17:30:39

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the autosuspend enable and disable

Hi,

On 2/18/21 3:36 PM, Hui Wang wrote:
>
> On 2/18/21 8:37 PM, Hans de Goede wrote:
>> drivers/usb/core/hub.c: usb_new_device() contains the following:
> [...]
>>         err = hci_register_dev(hdev);
>>       if (err < 0)
>> @@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interface *intf)
>>           gpiod_put(data->reset_gpio);
>>         hci_free_dev(hdev);
>> -
>> -    if (!enable_autosuspend)
>> -        usb_enable_autosuspend(data->udev);
> Hi Hans,
>
> And Do we need to call usb_disable_autosuspend() in the disconnect()? like below:
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 32161dd40ed6..ef831492363c 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4673,6 +4673,9 @@ static void btusb_disconnect(struct usb_interface *intf)
>
>         hci_unregister_dev(hdev);
>
> +       if (enable_autosuspend)
> +               usb_disable_autosuspend(data->udev);
> +
>         if (intf == data->intf) {
>                 if (data->isoc)
> usb_driver_release_interface(&btusb_driver, data->isoc);
>
>
> Before the btusb_probe() is called, the usb device is autosuspend disabled, suppose users set the btusb.enable_autosuspend=1, the driver btusb will enable the autosuspend on this device. If users remove this driver, the disconnect() will be called, the usb device will keep autosuspend enabled. Next time if users reload this driver by 'sudo modprobe  btusb enalbe_autosuspend=0',  they will find the device is autosuspend enabled instead of disabled.

The problem with calling usb_disable_autosuspend() is that the auto-suspend setting is a bool,
rather then a counter, so if a udev-rule or the user manually through e.g. :

echo auto > /sys/bus/usb/devices/1-10/power/control

Has enabled autosuspend then we would be disabling it, which is undesirable.

Most USB drivers which have some way of enabling autosuspend by-default
(IOW which call usb_enable_autosuspend()) simply enable it at the end
of a successful probe and leave it as is on remove.

Also keep in mind that remove normally runs on unplug of the device, in
which case it does not matter as the device is going away.

If a user wants to disable autosuspend after loading the btusb module,
the correct way to do this is by simply running e.g. :

echo on > /sys/bus/usb/devices/1-10/power/control

Rather then rmmod-ing and insmod-ing the module with a different module-param value.

Regards,

Hans




2021-02-18 17:30:42

by Hui Wang

[permalink] [raw]
Subject: Re: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the autosuspend enable and disable


On 2/18/21 8:37 PM, Hans de Goede wrote:
> drivers/usb/core/hub.c: usb_new_device() contains the following:
[...]
>
> err = hci_register_dev(hdev);
> if (err < 0)
> @@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interface *intf)
> gpiod_put(data->reset_gpio);
>
> hci_free_dev(hdev);
> -
> - if (!enable_autosuspend)
> - usb_enable_autosuspend(data->udev);
Hi Hans,

And Do we need to call usb_disable_autosuspend() in the disconnect()?
like below:

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 32161dd40ed6..ef831492363c 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -4673,6 +4673,9 @@ static void btusb_disconnect(struct usb_interface
*intf)

        hci_unregister_dev(hdev);

+       if (enable_autosuspend)
+               usb_disable_autosuspend(data->udev);
+
        if (intf == data->intf) {
                if (data->isoc)
usb_driver_release_interface(&btusb_driver, data->isoc);


Before the btusb_probe() is called, the usb device is autosuspend
disabled, suppose users set the btusb.enable_autosuspend=1, the driver
btusb will enable the autosuspend on this device. If users remove this
driver, the disconnect() will be called, the usb device will keep
autosuspend enabled. Next time if users reload this driver by 'sudo
modprobe  btusb enalbe_autosuspend=0',  they will find the device is
autosuspend enabled instead of disabled.

Thanks.


> }
>
> #ifdef CONFIG_PM

2021-02-18 17:31:14

by bluez.test.bot

[permalink] [raw]
Subject: RE: Bluetooth: btusb: Revert "Fix the autosuspend enable and disable"

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=435009

---Test result---

##############################
Test: CheckPatch - FAIL
Bluetooth: btusb: Revert Fix the autosuspend enable and disable
WARNING: Unknown commit id '7bd9fb058d77', maybe rebased or not pulled?
#25:
Fixes: 7bd9fb058d77 ("Bluetooth: btusb: Fix the autosuspend enable and disable")

total: 0 errors, 1 warnings, 19 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: btusb: Revert Fix the autosuspend enable and" has style problems, please review.

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckGitLint - PASS


##############################
Test: CheckBuildK - PASS


##############################
Test: CheckTestRunner: Setup - PASS


##############################
Test: CheckTestRunner: l2cap-tester - PASS
Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6

##############################
Test: CheckTestRunner: bnep-tester - PASS
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: mgmt-tester - PASS
Total: 416, Passed: 400 (96.2%), Failed: 0, Not Run: 16

##############################
Test: CheckTestRunner: rfcomm-tester - PASS
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: sco-tester - PASS
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: smp-tester - PASS
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: userchan-tester - PASS
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


Attachments:
l2cap-tester.log (42.33 kB)
bnep-tester.log (3.45 kB)
mgmt-tester.log (533.43 kB)
rfcomm-tester.log (11.38 kB)
sco-tester.log (9.66 kB)
smp-tester.log (11.52 kB)
userchan-tester.log (5.30 kB)
Download all attachments

2021-02-18 17:32:36

by Hui Wang

[permalink] [raw]
Subject: Re: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the autosuspend enable and disable


On 2/18/21 11:02 PM, Hans de Goede wrote:
> Hi,
>
> On 2/18/21 3:36 PM, Hui Wang wrote:
>> On 2/18/21 8:37 PM, Hans de Goede wrote:
>>> drivers/usb/core/hub.c: usb_new_device() contains the following:
>> [...]
>>>         err = hci_register_dev(hdev);
>>>       if (err < 0)
>>> @@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interface *intf)
>>>           gpiod_put(data->reset_gpio);
>>>         hci_free_dev(hdev);
>>> -
>>> -    if (!enable_autosuspend)
>>> -        usb_enable_autosuspend(data->udev);
>> Hi Hans,
>>
>> And Do we need to call usb_disable_autosuspend() in the disconnect()? like below:
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 32161dd40ed6..ef831492363c 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -4673,6 +4673,9 @@ static void btusb_disconnect(struct usb_interface *intf)
>>
>>         hci_unregister_dev(hdev);
>>
>> +       if (enable_autosuspend)
>> +               usb_disable_autosuspend(data->udev);
>> +
>>         if (intf == data->intf) {
>>                 if (data->isoc)
>> usb_driver_release_interface(&btusb_driver, data->isoc);
>>
>>
>> Before the btusb_probe() is called, the usb device is autosuspend disabled, suppose users set the btusb.enable_autosuspend=1, the driver btusb will enable the autosuspend on this device. If users remove this driver, the disconnect() will be called, the usb device will keep autosuspend enabled. Next time if users reload this driver by 'sudo modprobe  btusb enalbe_autosuspend=0',  they will find the device is autosuspend enabled instead of disabled.
> The problem with calling usb_disable_autosuspend() is that the auto-suspend setting is a bool,
> rather then a counter, so if a udev-rule or the user manually through e.g. :
>
> echo auto > /sys/bus/usb/devices/1-10/power/control
>
> Has enabled autosuspend then we would be disabling it, which is undesirable.
>
> Most USB drivers which have some way of enabling autosuspend by-default
> (IOW which call usb_enable_autosuspend()) simply enable it at the end
> of a successful probe and leave it as is on remove.
>
> Also keep in mind that remove normally runs on unplug of the device, in
> which case it does not matter as the device is going away.
>
> If a user wants to disable autosuspend after loading the btusb module,
> the correct way to do this is by simply running e.g. :
>
> echo on > /sys/bus/usb/devices/1-10/power/control
>
> Rather then rmmod-ing and insmod-ing the module with a different module-param value.

Got it, thanks for your explanation.

Thanks,

Hui.

>
> Regards,
>
> Hans
>
>
>
>

2021-02-18 20:08:40

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the autosuspend enable and disable

Hi Hans,

>>> drivers/usb/core/hub.c: usb_new_device() contains the following:
>> [...]
>>> err = hci_register_dev(hdev);
>>> if (err < 0)
>>> @@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interface *intf)
>>> gpiod_put(data->reset_gpio);
>>> hci_free_dev(hdev);
>>> -
>>> - if (!enable_autosuspend)
>>> - usb_enable_autosuspend(data->udev);
>> Hi Hans,
>>
>> And Do we need to call usb_disable_autosuspend() in the disconnect()? like below:
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 32161dd40ed6..ef831492363c 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -4673,6 +4673,9 @@ static void btusb_disconnect(struct usb_interface *intf)
>>
>> hci_unregister_dev(hdev);
>>
>> + if (enable_autosuspend)
>> + usb_disable_autosuspend(data->udev);
>> +
>> if (intf == data->intf) {
>> if (data->isoc)
>> usb_driver_release_interface(&btusb_driver, data->isoc);
>>
>>
>> Before the btusb_probe() is called, the usb device is autosuspend disabled, suppose users set the btusb.enable_autosuspend=1, the driver btusb will enable the autosuspend on this device. If users remove this driver, the disconnect() will be called, the usb device will keep autosuspend enabled. Next time if users reload this driver by 'sudo modprobe btusb enalbe_autosuspend=0', they will find the device is autosuspend enabled instead of disabled.
>
> The problem with calling usb_disable_autosuspend() is that the auto-suspend setting is a bool,
> rather then a counter, so if a udev-rule or the user manually through e.g. :
>
> echo auto > /sys/bus/usb/devices/1-10/power/control
>
> Has enabled autosuspend then we would be disabling it, which is undesirable.
>
> Most USB drivers which have some way of enabling autosuspend by-default
> (IOW which call usb_enable_autosuspend()) simply enable it at the end
> of a successful probe and leave it as is on remove.
>
> Also keep in mind that remove normally runs on unplug of the device, in
> which case it does not matter as the device is going away.
>
> If a user wants to disable autosuspend after loading the btusb module,
> the correct way to do this is by simply running e.g. :
>
> echo on > /sys/bus/usb/devices/1-10/power/control
>
> Rather then rmmod-ing and insmod-ing the module with a different module-param value.

then lets remove the module parameter from btusb.ko.

Regards

Marcel

2021-02-18 22:08:45

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the autosuspend enable and disable

Hi Marcel,

On 2/18/21 9:01 PM, Marcel Holtmann wrote:
> Hi Hans,
>
>>>> drivers/usb/core/hub.c: usb_new_device() contains the following:
>>> [...]
>>>> err = hci_register_dev(hdev);
>>>> if (err < 0)
>>>> @@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interface *intf)
>>>> gpiod_put(data->reset_gpio);
>>>> hci_free_dev(hdev);
>>>> -
>>>> - if (!enable_autosuspend)
>>>> - usb_enable_autosuspend(data->udev);
>>> Hi Hans,
>>>
>>> And Do we need to call usb_disable_autosuspend() in the disconnect()? like below:
>>>
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 32161dd40ed6..ef831492363c 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -4673,6 +4673,9 @@ static void btusb_disconnect(struct usb_interface *intf)
>>>
>>> hci_unregister_dev(hdev);
>>>
>>> + if (enable_autosuspend)
>>> + usb_disable_autosuspend(data->udev);
>>> +
>>> if (intf == data->intf) {
>>> if (data->isoc)
>>> usb_driver_release_interface(&btusb_driver, data->isoc);
>>>
>>>
>>> Before the btusb_probe() is called, the usb device is autosuspend disabled, suppose users set the btusb.enable_autosuspend=1, the driver btusb will enable the autosuspend on this device. If users remove this driver, the disconnect() will be called, the usb device will keep autosuspend enabled. Next time if users reload this driver by 'sudo modprobe btusb enalbe_autosuspend=0', they will find the device is autosuspend enabled instead of disabled.
>>
>> The problem with calling usb_disable_autosuspend() is that the auto-suspend setting is a bool,
>> rather then a counter, so if a udev-rule or the user manually through e.g. :
>>
>> echo auto > /sys/bus/usb/devices/1-10/power/control
>>
>> Has enabled autosuspend then we would be disabling it, which is undesirable.
>>
>> Most USB drivers which have some way of enabling autosuspend by-default
>> (IOW which call usb_enable_autosuspend()) simply enable it at the end
>> of a successful probe and leave it as is on remove.
>>
>> Also keep in mind that remove normally runs on unplug of the device, in
>> which case it does not matter as the device is going away.
>>
>> If a user wants to disable autosuspend after loading the btusb module,
>> the correct way to do this is by simply running e.g. :
>>
>> echo on > /sys/bus/usb/devices/1-10/power/control
>>
>> Rather then rmmod-ing and insmod-ing the module with a different module-param value.
>
> then lets remove the module parameter from btusb.ko.

The module parameter is useful to make sure runtime-suspend never gets
enabled starting from boot onwards, either through the kernel cmdline
or through modprobe.conf settings.

Also the module parameter is used to implement CONFIG_BT_HCIBTUSB_AUTOSUSPEND
Kconfig option which sets the default value for the module param;
and most distros enable that option since it having autosuspend enabled
is the right thing to do in almost all cases.

Regards,

Hans

2021-02-18 23:44:11

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the autosuspend enable and disable

Hi Hans,

On Thu, Feb 18, 2021 at 2:08 PM Hans de Goede <[email protected]> wrote:
>
> Hi Marcel,
>
> On 2/18/21 9:01 PM, Marcel Holtmann wrote:
> > Hi Hans,
> >
> >>>> drivers/usb/core/hub.c: usb_new_device() contains the following:
> >>> [...]
> >>>> err = hci_register_dev(hdev);
> >>>> if (err < 0)
> >>>> @@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interface *intf)
> >>>> gpiod_put(data->reset_gpio);
> >>>> hci_free_dev(hdev);
> >>>> -
> >>>> - if (!enable_autosuspend)
> >>>> - usb_enable_autosuspend(data->udev);
> >>> Hi Hans,
> >>>
> >>> And Do we need to call usb_disable_autosuspend() in the disconnect()? like below:
> >>>
> >>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> >>> index 32161dd40ed6..ef831492363c 100644
> >>> --- a/drivers/bluetooth/btusb.c
> >>> +++ b/drivers/bluetooth/btusb.c
> >>> @@ -4673,6 +4673,9 @@ static void btusb_disconnect(struct usb_interface *intf)
> >>>
> >>> hci_unregister_dev(hdev);
> >>>
> >>> + if (enable_autosuspend)
> >>> + usb_disable_autosuspend(data->udev);
> >>> +
> >>> if (intf == data->intf) {
> >>> if (data->isoc)
> >>> usb_driver_release_interface(&btusb_driver, data->isoc);
> >>>
> >>>
> >>> Before the btusb_probe() is called, the usb device is autosuspend disabled, suppose users set the btusb.enable_autosuspend=1, the driver btusb will enable the autosuspend on this device. If users remove this driver, the disconnect() will be called, the usb device will keep autosuspend enabled. Next time if users reload this driver by 'sudo modprobe btusb enalbe_autosuspend=0', they will find the device is autosuspend enabled instead of disabled.
> >>
> >> The problem with calling usb_disable_autosuspend() is that the auto-suspend setting is a bool,
> >> rather then a counter, so if a udev-rule or the user manually through e.g. :
> >>
> >> echo auto > /sys/bus/usb/devices/1-10/power/control
> >>
> >> Has enabled autosuspend then we would be disabling it, which is undesirable.
> >>
> >> Most USB drivers which have some way of enabling autosuspend by-default
> >> (IOW which call usb_enable_autosuspend()) simply enable it at the end
> >> of a successful probe and leave it as is on remove.
> >>
> >> Also keep in mind that remove normally runs on unplug of the device, in
> >> which case it does not matter as the device is going away.
> >>
> >> If a user wants to disable autosuspend after loading the btusb module,
> >> the correct way to do this is by simply running e.g. :
> >>
> >> echo on > /sys/bus/usb/devices/1-10/power/control
> >>
> >> Rather then rmmod-ing and insmod-ing the module with a different module-param value.
> >
> > then lets remove the module parameter from btusb.ko.
>
> The module parameter is useful to make sure runtime-suspend never gets
> enabled starting from boot onwards, either through the kernel cmdline
> or through modprobe.conf settings.
>
> Also the module parameter is used to implement CONFIG_BT_HCIBTUSB_AUTOSUSPEND
> Kconfig option which sets the default value for the module param;
> and most distros enable that option since it having autosuspend enabled
> is the right thing to do in almost all cases.

Actually in case we are connected we should probably disable
autosuspend as some BT controllers don't seem to be able to transmit
any data back to the host if the connection stays idle long enough to
trigger auto suspend.

> Regards,
>
> Hans
>


--
Luiz Augusto von Dentz

2021-02-19 09:28:49

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the autosuspend enable and disable

Hi,

On 2/19/21 12:41 AM, Luiz Augusto von Dentz wrote:
> Hi Hans,
>
> On Thu, Feb 18, 2021 at 2:08 PM Hans de Goede <[email protected]> wrote:
>>
>> Hi Marcel,
>>
>> On 2/18/21 9:01 PM, Marcel Holtmann wrote:
>>> Hi Hans,
>>>
>>>>>> drivers/usb/core/hub.c: usb_new_device() contains the following:
>>>>> [...]
>>>>>> err = hci_register_dev(hdev);
>>>>>> if (err < 0)
>>>>>> @@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interface *intf)
>>>>>> gpiod_put(data->reset_gpio);
>>>>>> hci_free_dev(hdev);
>>>>>> -
>>>>>> - if (!enable_autosuspend)
>>>>>> - usb_enable_autosuspend(data->udev);
>>>>> Hi Hans,
>>>>>
>>>>> And Do we need to call usb_disable_autosuspend() in the disconnect()? like below:
>>>>>
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index 32161dd40ed6..ef831492363c 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -4673,6 +4673,9 @@ static void btusb_disconnect(struct usb_interface *intf)
>>>>>
>>>>> hci_unregister_dev(hdev);
>>>>>
>>>>> + if (enable_autosuspend)
>>>>> + usb_disable_autosuspend(data->udev);
>>>>> +
>>>>> if (intf == data->intf) {
>>>>> if (data->isoc)
>>>>> usb_driver_release_interface(&btusb_driver, data->isoc);
>>>>>
>>>>>
>>>>> Before the btusb_probe() is called, the usb device is autosuspend disabled, suppose users set the btusb.enable_autosuspend=1, the driver btusb will enable the autosuspend on this device. If users remove this driver, the disconnect() will be called, the usb device will keep autosuspend enabled. Next time if users reload this driver by 'sudo modprobe btusb enalbe_autosuspend=0', they will find the device is autosuspend enabled instead of disabled.
>>>>
>>>> The problem with calling usb_disable_autosuspend() is that the auto-suspend setting is a bool,
>>>> rather then a counter, so if a udev-rule or the user manually through e.g. :
>>>>
>>>> echo auto > /sys/bus/usb/devices/1-10/power/control
>>>>
>>>> Has enabled autosuspend then we would be disabling it, which is undesirable.
>>>>
>>>> Most USB drivers which have some way of enabling autosuspend by-default
>>>> (IOW which call usb_enable_autosuspend()) simply enable it at the end
>>>> of a successful probe and leave it as is on remove.
>>>>
>>>> Also keep in mind that remove normally runs on unplug of the device, in
>>>> which case it does not matter as the device is going away.
>>>>
>>>> If a user wants to disable autosuspend after loading the btusb module,
>>>> the correct way to do this is by simply running e.g. :
>>>>
>>>> echo on > /sys/bus/usb/devices/1-10/power/control
>>>>
>>>> Rather then rmmod-ing and insmod-ing the module with a different module-param value.
>>>
>>> then lets remove the module parameter from btusb.ko.
>>
>> The module parameter is useful to make sure runtime-suspend never gets
>> enabled starting from boot onwards, either through the kernel cmdline
>> or through modprobe.conf settings.
>>
>> Also the module parameter is used to implement CONFIG_BT_HCIBTUSB_AUTOSUSPEND
>> Kconfig option which sets the default value for the module param;
>> and most distros enable that option since it having autosuspend enabled
>> is the right thing to do in almost all cases.
>
> Actually in case we are connected we should probably disable
> autosuspend as some BT controllers don't seem to be able to transmit
> any data back to the host if the connection stays idle long enough to
> trigger auto suspend.

Do those controller accept connection requests from previously paired
devices, without them having to be woken from userspace first?

In my experience if controllers have this issue then these controller
falsely advertise USB remote wake-up support / has broken USB remote
wake-up support and devices will also not automatically (re)connect
without first going to the bluetooth control-panel in the desktop
which wakes-up the controller from the PC side.

The fix for these controllers would be to explicitly disable
remote-wakeup on these controllers by making a call like this:

device_set_wakeup_capable(&data->udev->dev, false);

But only on controllers where we know the remote-wakeup is broken.

This will still allow the device to be runtime/auto-suspended when
bluetooth is disabled by the user, while disabling it when bluetooth
is enabled. This works this way because of the following btusb.c code:

static int btusb_open(struct hci_dev *hdev)
{
...
data->intf->needs_remote_wakeup = 1;
...
}

static int btusb_close(struct hci_dev *hdev)
{
...
data->intf->needs_remote_wakeup = 0;
...
}

Regards,

Hans