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
