Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221029202454.25651-1-swyterzone@gmail.com> <20221029202454.25651-3-swyterzone@gmail.com>
In-Reply-To: <20221029202454.25651-3-swyterzone@gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 9 Nov 2022 12:49:20 -0800
Message-ID: <CABBYNZKnw+b+KE2=M=gGV+rR_KBJLvrxRrtEc8x12W6PY=LKMw@mail.gmail.com>
Subject: Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users
 disable the fake CSR force-suspend hack
To:     Ismael Ferreras Morezuelas <swyterzone@gmail.com>
Cc:     marcel@holtmann.org, johan.hedberg@gmail.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, luiz.von.dentz@intel.com,
        quic_zijuhu@quicinc.com, hdegoede@redhat.com,
        linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org,
        netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Ismael,

On Sat, Oct 29, 2022 at 1:25 PM Ismael Ferreras Morezuelas
<swyterzone@gmail.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 =C2=ABbtusb.disable_fake_csr_forcesuspend_hack=
=3D1=C2=BB 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@vger.kernel.org
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.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 =3D IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUS=
PEND);
>  static bool enable_poll_sync =3D IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC=
);
>  static bool reset =3D 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 =3D true;
>
>         if (is_fake) {
> -               bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; add=
ing workarounds and force-suspending once...");
> +               bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; add=
ing 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 th=
at 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 detec=
ted; force-suspending once...");
> +                       pm_runtime_allow(&data->udev->dev);
>
> -               ret =3D pm_runtime_suspend(&data->udev->dev);
> -               if (ret >=3D 0)
> -                       msleep(200);
> -               else
> -                       bt_dev_warn(hdev, "CSR: Couldn't suspend the devi=
ce for our Barrot 8041a02 receive-issue workaround");
> +                       ret =3D pm_runtime_suspend(&data->udev->dev);
> +                       if (ret >=3D 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 au=
tosuspend 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 indiscrimina=
tely force-suspend Chinese-cloned CSR dongles trying to unfreeze them");
> +
>  MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
>  MODULE_DESCRIPTION("Generic Bluetooth USB driver ver " VERSION);
>  MODULE_VERSION(VERSION);
> --
> 2.38.1
>


--=20
Luiz Augusto von Dentz