linux-kernel - Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Nov 2022 12:49:20 -0800
From:   Luiz Augusto von Dentz <luiz.dentz@...il.com>
To:     Ismael Ferreras Morezuelas <swyterzone@...il.com>
Cc:     marcel@...tmann.org, johan.hedberg@...il.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, luiz.von.dentz@...el.com,
        quic_zijuhu@...cinc.com, hdegoede@...hat.com,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users
 disable the fake CSR force-suspend hack

Hi Ismael,

On Sat, Oct 29, 2022 at 1:25 PM Ismael Ferreras Morezuelas
<swyterzone@...il.com> wrote:
>
> A few users have reported that their cloned Chinese dongle doesn't
> work well with the hack Hans de Goede added, that tries this
> off-on mechanism as a way to unfreeze them.
>
> It's still more than worthwhile to have it, as in the vast majority
> of cases it either completely brings dongles to life or just resets
> them harmlessly as it already happens during normal USB operation.
>
> This is nothing new and the controllers are expected to behave
> correctly. But yeah, go figure. :)
>
> For that unhappy minority we can easily handle this edge case by letting
> users disable it via our «btusb.disable_fake_csr_forcesuspend_hack=1» kernel option.

Don't really like the idea of adding module parameter for device
specific problem.

> I believe this is the most generic way of doing it, given the constraints
> and by still having a good out-of-the-box experience.
>
> No clone left behind.
>
> Cc: stable@...r.kernel.org
> Cc: Hans de Goede <hdegoede@...hat.com>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@...il.com>
> ---
>  drivers/bluetooth/btusb.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 8f34bf195bae..d31d4f925463 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -34,6 +34,7 @@ static bool force_scofix;
>  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
>  static bool enable_poll_sync = IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC);
>  static bool reset = true;
> +static bool disable_fake_csr_forcesuspend_hack;
>
>  static struct usb_driver btusb_driver;
>
> @@ -2171,7 +2172,7 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>                 is_fake = true;
>
>         if (is_fake) {
> -               bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds and force-suspending once...");
> +               bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds...");
>
>                 /* Generally these clones have big discrepancies between
>                  * advertised features and what's actually supported.
> @@ -2215,21 +2216,24 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>                  * apply this initialization quirk to every controller that gets here,
>                  * it should be harmless. The alternative is to not work at all.
>                  */
> -               pm_runtime_allow(&data->udev->dev);
> +               if (!disable_fake_csr_forcesuspend_hack) {
> +                       bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; force-suspending once...");
> +                       pm_runtime_allow(&data->udev->dev);
>
> -               ret = pm_runtime_suspend(&data->udev->dev);
> -               if (ret >= 0)
> -                       msleep(200);
> -               else
> -                       bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
> +                       ret = pm_runtime_suspend(&data->udev->dev);
> +                       if (ret >= 0)
> +                               msleep(200);
> +                       else
> +                               bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");

Is this specific to Barrot 8041a02? Why don't we add a quirk then?

> -               pm_runtime_forbid(&data->udev->dev);
> +                       pm_runtime_forbid(&data->udev->dev);
>
> -               device_set_wakeup_capable(&data->udev->dev, false);
> +                       device_set_wakeup_capable(&data->udev->dev, false);
>
> -               /* Re-enable autosuspend if this was requested */
> -               if (enable_autosuspend)
> -                       usb_enable_autosuspend(data->udev);
> +                       /* Re-enable autosuspend if this was requested */
> +                       if (enable_autosuspend)
> +                               usb_enable_autosuspend(data->udev);
> +               }
>         }
>
>         kfree_skb(skb);
> @@ -4312,6 +4316,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
>  module_param(reset, bool, 0644);
>  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
>
> +module_param(disable_fake_csr_forcesuspend_hack, bool, 0644);
> +MODULE_PARM_DESC(disable_fake_csr_forcesuspend_hack, "Don't indiscriminately force-suspend Chinese-cloned CSR dongles trying to unfreeze them");
> +
>  MODULE_AUTHOR("Marcel Holtmann <marcel@...tmann.org>");
>  MODULE_DESCRIPTION("Generic Bluetooth USB driver ver " VERSION);
>  MODULE_VERSION(VERSION);
> --
> 2.38.1
>


-- 
Luiz Augusto von Dentz