On 2/19/21 5:24 PM, Hans de Goede wrote:
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 <hdegoede@xxxxxxxxxx> wrote:
Hi Marcel,
On 2/18/21 9:01 PM, Marcel Holtmann wrote:
Hi Hans,
Hi Marcel and Hans,
Looks like this reverting patch is not applied yet, If it is already
applied, please ignore the below content.
My patch really introduces a regression, that makes the autosuspend
can't be enabled by btusb.c anymore.
When the usb core layer calls the usb_driver->probe(), the autosuspend
is disabled by pm_runtime_forbid(), if users set
btusb.enable_autosuspend=1 or enable the CONFIG_BT_HCIBTUSB_AUTOSUSPEND,
the probe() should enable the autosuspend by calling
usb_enable_autosuspend() which will call pm_runtime_allow().
For keeping balance, when executing disconnect(), if probe() enabled the
autosuspend, disconect() should disable it by calling
usb_disable_autosuspend() which will call pm_runtime_forbid(). This
could guarantee the kernel parameter enable_autosuspend works as
expected when users repeatedly run "rmmod btusb;modporbe btusb
enable_autosuspend=1/0".
The users could also enable or disable autosuspend by echoing "auto" or
"on" to /sys/.../power/control, btusb doesn't know users change the
autosuspend from userspace, so btusb only keeps the autosuspend balanced
in itself, and userspace should keep balance from userspace, btusb has
no capability to detect and control the userspace operation.
According to this idea, the fixing patch is like below:
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 52683fd22e05..a0811e00a5a7 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -4849,8 +4849,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)
@@ -4888,6 +4888,10 @@ static void btusb_disconnect(struct usb_interface
*intf)
hci_unregister_dev(hdev);
+ /* if the autosuspend is enabled in _probe, we disable it here
for keeping balance */
+ if (enable_autosuspend)
+ usb_disable_autosuspend(data->udev);
+
if (intf == data->intf) {
if (data->isoc)
usb_driver_release_interface(&btusb_driver, data->isoc);
@@ -4910,9 +4914,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);
}
Regards,
Hui.
}
static int btusb_close(struct hci_dev *hdev)
{
...
data->intf->needs_remote_wakeup = 0;
...
}
Regards,
Hans
