Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210218123728.17067-1-hdegoede@redhat.com> <20210218123728.17067-2-hdegoede@redhat.com>
 <90d8996d-a376-2e9c-37ce-ce50b8660fd1@canonical.com> <4510935f-a30b-445d-a048-683619f2855b@redhat.com>
 <6CE927B6-449A-472C-9196-AF98895AA5E1@holtmann.org> <2bf12891-eeae-e55f-ab46-7434dffbad76@redhat.com>
In-Reply-To: <2bf12891-eeae-e55f-ab46-7434dffbad76@redhat.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 18 Feb 2021 15:41:12 -0800
Message-ID: <CABBYNZ+euV5rwx=LeLKOGSL0AwnPfE7paEHhNhMiUpXGYixgBw@mail.gmail.com>
Subject: Re: [PATCH 5.12 regression fix] Bluetooth: btusb: Revert Fix the
 autosuspend enable and disable
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Hui Wang <hui.wang@canonical.com>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 =3D hci_register_dev(hdev);
> >>>>       if (err < 0)
> >>>> @@ -4688,9 +4688,6 @@ static void btusb_disconnect(struct usb_interf=
ace *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_interfa=
ce *intf)
> >>>
> >>>         hci_unregister_dev(hdev);
> >>>
> >>> +       if (enable_autosuspend)
> >>> +               usb_disable_autosuspend(data->udev);
> >>> +
> >>>         if (intf =3D=3D data->intf) {
> >>>                 if (data->isoc)
> >>> usb_driver_release_interface(&btusb_driver, data->isoc);
> >>>
> >>>
> >>> Before the btusb_probe() is called, the usb device is autosuspend dis=
abled, suppose users set the btusb.enable_autosuspend=3D1, the driver btusb=
 will enable the autosuspend on this device. If users remove this driver, t=
he disconnect() will be called, the usb device will keep autosuspend enable=
d. Next time if users reload this driver by 'sudo modprobe  btusb enalbe_au=
tosuspend=3D0',  they will find the device is autosuspend enabled instead o=
f disabled.
> >>
> >> The problem with calling usb_disable_autosuspend() is that the auto-su=
spend 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 undesi=
rable.
> >>
> >> Most USB drivers which have some way of enabling autosuspend by-defaul=
t
> >> (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, i=
n
> >> 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 modul=
e-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_AUTOSUS=
PEND
> 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
>


--=20
Luiz Augusto von Dentz