| Message ID | 20210218123728.17067-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: btusb: Revert "Fix the autosuspend enable and disable" | expand |
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
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
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
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 > > > >
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
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
Hi Hans, On Thu, Feb 18, 2021 at 2:08 PM Hans de Goede <hdegoede@redhat.com> 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 >
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@redhat.com> 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
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@redhat.com> 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 >
Hi, On 2/25/21 5:37 AM, Hui Wang wrote: > > 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@redhat.com> 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. There is no balance this is not a (reference)counted thing. It is a straight forward bool, which is either enabled or disabled. So who or what-ever sets its value last *wins* there is *no balancing* / *no counting* ! All drivers using this to opt-in to auto-suspend by default call usb_enable_autosuspend() from their probe function and leave it at that, without making any further calls on remove. What is necessary here is a straight-forward revert of your commit, as I submitted and nothing else. If you can reply with an Acked-by or Reviewed-by to my original revert/patch then that would be greatly appreciated. Regards, Hans p.s. Calling usb_disable_autosuspend(data->udev) on driver unbind is a bad idea because it would break like e.g. this: 1. userspace writes auto to /sys/.../power/control, as is done from udev rules as we have seen already, or could be done manually by the user. The device now autosuspends 2. btusb binds, the devices now autosuspends when not in use 3. btusb unbinds and disables autosuspend 4. The device is now constant on despite userspace having requested that it should be autosuspended. With just a straight forward revert this problem does not happen. > According to this idea, the fixing patch is like below: Again this is WRONG. Please STOP submitting patches for code where you clearly do not grasp the full implications of the changes which you are making. > 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 >> >
On 2/25/21 10:17 PM, Hans de Goede wrote: > Hi, > > On 2/25/21 5:37 AM, Hui Wang wrote: >> On 2/19/21 5:24 PM, Hans de Goede wrote: >>> Hi, >>> >>> On 2/19/21 12:41 AM, Luiz Augusto von Dentz wrote: >>> [...] > There is no balance this is not a (reference)counted thing. It is a straight forward bool, > which is either enabled or disabled. So who or what-ever sets its value last *wins* > there is *no balancing* / *no counting* ! > > All drivers using this to opt-in to auto-suspend by default call usb_enable_autosuspend() > from their probe function and leave it at that, without making any further calls on remove. > > What is necessary here is a straight-forward revert of your commit, as I submitted and > nothing else. > > If you can reply with an Acked-by or Reviewed-by to my original revert/patch then > that would be greatly appreciated. OK, got it. > > Regards, > > Hans > > > p.s. > > Calling usb_disable_autosuspend(data->udev) on driver unbind is a bad idea > because it would break like e.g. this: > > 1. userspace writes auto to /sys/.../power/control, as is done from > udev rules as we have seen already, or could be done manually > by the user. The device now autosuspends > > 2. btusb binds, the devices now autosuspends when not in use > > 3. btusb unbinds and disables autosuspend > > 4. The device is now constant on despite userspace having requested > that it should be autosuspended. > > With just a straight forward revert this problem does not happen. > > > What I thought is there are many many devices which are not controlled by userspace. > > > > >> According to this idea, the fixing patch is like below: > Again this is WRONG. Please STOP submitting patches for code where > you clearly do not grasp the full implications of the changes which > you are making. > OK, got it. Thanks, Hui. > > > >> 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 >>>
Acked-by: Hui Wang <hui.wang@canonical.com> On 2/18/21 8:37 PM, Hans de Goede wrote: > 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 <hui.wang@canonical.com> > Fixes: 7bd9fb058d77 ("Bluetooth: btusb: Fix the autosuspend enable and disable") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > 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
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
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 <hui.wang@canonical.com> Fixes: 7bd9fb058d77 ("Bluetooth: btusb: Fix the autosuspend enable and disable") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/bluetooth/btusb.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)