RE: [PATCH] bluetooth: add quirk using packet size 60

[Hilda Wu <hildawu@realtek.com>]

Hi Luiz,
Thanks for your feedback.
I just send the V2. 
Please see the reply below. Thank you

-----Original Message-----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> 
Sent: Friday, November 1, 2024 4:47 AM
To: Hilda Wu <hildawu@realtek.com>
Cc: marcel@holtmann.org; linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org; Max Chou <max.chou@realtek.com>; alex_lu@realsil.com.cn; KidmanLee <kidman@realtek.com>
Subject: Re: [PATCH] bluetooth: add quirk using packet size 60


External mail.



Hi Hilda,

On Wed, Oct 30, 2024 at 6:08 AM Hilda Wu <hildawu@realtek.com> wrote:
>
> The RTL8852BE-VT supports USB alternate setting 6.
> However, its descriptor does not report this capability to the host.
> Therefore, a quirk is needed to bypass the RTL8852BE-VT's descriptor 
> and allow it to use USB ALT 6 directly.

It is getting very hackish trying to fixup the descriptor in software, isn't it possible to update the USB descriptors via firmware update?
Not to mention you didn't include any logs of how this was tested, I suppose this is for HFP/SCO wideband (using msbc), is that just plain broken right now?

Since there is currently no way for update the USB descriptors even if it is firmware. That’s why we do it through the driver.
This is for HFP/SCO wideband (using msbc), the broken state is other USB ALT will be used and there will be no sound.

> Signed-off-by: Hilda Wu <hildawu@realtek.com>
> ---
>  drivers/bluetooth/btrtl.c |  3 ++
>  drivers/bluetooth/btrtl.h |  1 +
>  drivers/bluetooth/btusb.c | 89 
> ++++++++++++++++++++++++++++++---------
>  3 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c 
> index 0bcb44cf7b31..b75f0b36a09b 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -1312,6 +1312,9 @@ void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
>                     btrtl_dev->project_id == CHIP_ID_8852C)
>                         set_bit(HCI_QUIRK_USE_MSFT_EXT_ADDRESS_FILTER, 
> &hdev->quirks);
>
> +               if (btrtl_dev->project_id == CHIP_ID_8852BT)
> +                       btrealtek_set_flag(hdev, REALTEK_ALT6_FORCE);
> +
>                 hci_set_aosp_capable(hdev);
>                 break;
>         default:
> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h 
> index a2d9d34f9fb0..ffec2fca88ec 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -105,6 +105,7 @@ struct rtl_vendor_cmd {
>
>  enum {
>         REALTEK_ALT6_CONTINUOUS_TX_CHIP,
> +       REALTEK_ALT6_FORCE,
>
>         __REALTEK_NUM_FLAGS,
>  };
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c 
> index 514d593923ad..eca0b173232e 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -804,6 +804,7 @@ struct qca_dump_info {  #define 
> BTUSB_USE_ALT3_FOR_WBS 15
>  #define BTUSB_ALT6_CONTINUOUS_TX       16
>  #define BTUSB_HW_SSR_ACTIVE    17
> +#define BTUSB_ALT_CHANGED      18
>
>  struct btusb_data {
>         struct hci_dev       *hdev;
> @@ -2130,16 +2131,61 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
>         }
>  }
>
> +static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> +                                                       int alt) {
> +       struct usb_interface *intf = data->isoc;
> +       int i;
> +
> +       BT_DBG("Looking for Alt no :%d", alt);
> +
> +       if (!intf)
> +               return NULL;
> +
> +       for (i = 0; i < intf->num_altsetting; i++) {
> +               if (intf->altsetting[i].desc.bAlternateSetting == alt)
> +                       return &intf->altsetting[i];
> +       }
> +
> +       return NULL;
> +}
> +
>  static inline int __set_isoc_interface(struct hci_dev *hdev, int 
> altsetting)  {
>         struct btusb_data *data = hci_get_drvdata(hdev);
>         struct usb_interface *intf = data->isoc;
>         struct usb_endpoint_descriptor *ep_desc;
> +       struct usb_host_interface *alt;
>         int i, err;
>
>         if (!data->isoc)
>                 return -ENODEV;
>
> +       /* For some Realtek chips, they actually have the altsetting 6, but its
> +        * altsetting descriptor is not exposed. We can activate altsetting 6 by
> +        * replacing the altsetting 5.
> +        */
> +       if (altsetting == 6 && !btusb_find_altsetting(data, 6) &&
> +           btrealtek_test_flag(hdev, REALTEK_ALT6_FORCE)) {
> +               alt = NULL;
> +               for (i = 0; i < intf->num_altsetting; i++) {
> +                       if (intf->altsetting[i].desc.bAlternateSetting == 5) {
> +                               alt = &intf->altsetting[i];
> +                               break;
> +                       }
> +               }

I believe you can replace the code above with btusb_find_altsetting so please use that instead of duplicating its logic.

Adjusted in V2.

> +               if (alt) {
> +                       for (i = 0; i < alt->desc.bNumEndpoints; i++) {
> +                               ep_desc = &alt->endpoint[i].desc;
> +                               if (usb_endpoint_is_isoc_out(ep_desc) ||
> +                                   usb_endpoint_is_isoc_in(ep_desc))
> +                                       ep_desc->wMaxPacketSize = 63;
> +                       }
> +                       alt->desc.bAlternateSetting = 6;
> +                       set_bit(BTUSB_ALT_CHANGED, &data->flags);
> +               }
> +       }
> +
>         err = usb_set_interface(data->udev, data->isoc_ifnum, altsetting);
>         if (err < 0) {
>                 bt_dev_err(hdev, "setting interface failed (%d)", 
> -err); @@ -2151,6 +2197,27 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
>         data->isoc_tx_ep = NULL;
>         data->isoc_rx_ep = NULL;
>
> +       /* Recover alt 5 desc if alt 0 is set. */
> +       if (!altsetting && test_bit(BTUSB_ALT_CHANGED, &data->flags)) {
> +               alt = NULL;
> +               for (i = 0; i < intf->num_altsetting; i++) {
> +                       if (intf->altsetting[i].desc.bAlternateSetting == 6) {
> +                               alt = &intf->altsetting[i];
> +                               break;
> +                       }
> +               }

Ditto.

Adjusted in V2.

> +               if (alt) {
> +                       for (i = 0; i < alt->desc.bNumEndpoints; i++) {
> +                               ep_desc = &alt->endpoint[i].desc;
> +                               if (usb_endpoint_is_isoc_out(ep_desc) ||
> +                                   usb_endpoint_is_isoc_in(ep_desc))
> +                                       ep_desc->wMaxPacketSize = 49;
> +                       }
> +                       alt->desc.bAlternateSetting = 5;
> +                       clear_bit(BTUSB_ALT_CHANGED, &data->flags);
> +               }
> +       }
> +
>         for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
>                 ep_desc = &intf->cur_altsetting->endpoint[i].desc;
>
> @@ -2213,25 +2280,6 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
>         return 0;
>  }
>
> -static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> -                                                       int alt)
> -{
> -       struct usb_interface *intf = data->isoc;
> -       int i;
> -
> -       BT_DBG("Looking for Alt no :%d", alt);
> -
> -       if (!intf)
> -               return NULL;
> -
> -       for (i = 0; i < intf->num_altsetting; i++) {
> -               if (intf->altsetting[i].desc.bAlternateSetting == alt)
> -                       return &intf->altsetting[i];
> -       }
> -
> -       return NULL;
> -}
> -
>  static void btusb_work(struct work_struct *work)  {
>         struct btusb_data *data = container_of(work, struct 
> btusb_data, work); @@ -2269,7 +2317,8 @@ static void btusb_work(struct work_struct *work)
>                          * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
>                          * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
>                          */
> -                       if (btusb_find_altsetting(data, 6))
> +                       if (btusb_find_altsetting(data, 6) ||
> +                           btrealtek_test_flag(hdev, 
> + REALTEK_ALT6_FORCE))
>                                 new_alts = 6;
>                         else if (btusb_find_altsetting(data, 3) &&
>                                  hdev->sco_mtu >= 72 &&
> --
> 2.34.1
>


--
Luiz Augusto von Dentz