Received-SPF: pass (google.com: domain of linux-kernel-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: <20220913100244.23660-1-hildawu@realtek.com> <20220913100244.23660-3-hildawu@realtek.com>
In-Reply-To: <20220913100244.23660-3-hildawu@realtek.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 14 Sep 2022 14:37:20 -0700
Message-ID: <CABBYNZKCEs_2Jb1tPncekgxGtjkNkgew4hzdKktoQhvPkuR1Lw@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] Bluetooth: btusb: Workaround for spotty SCO quality
To:     hildawu@realtek.com
Cc:     marcel@holtmann.org, johan.hedberg@gmail.com,
        linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
        apusaka@chromium.org, yinghsu@chromium.org, max.chou@realtek.com,
        alex_lu@realsil.com.cn, kidman@realtek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Hilda,

On Tue, Sep 13, 2022 at 3:02 AM <hildawu@realtek.com> wrote:
>
> From: Hilda Wu <hildawu@realtek.com>
>
> When streaming HFP, once a few minutes a brief pause in audio can be
> heard on some platform with Realtek Bluetooth. When the issue occurs,
> the system will see the SCO packet for unknown connection handle messages.
>
> Note: This issue affects (e)SCO only, does not affect ACLs.
> Because the duplicate packet causing the problem only occurs in Realtek BT.
> This is to filter out duplicate packet for avoiding influence.
>
> Signed-off-by: Hilda Wu <hildawu@realtek.com>
> ---
> Changes in v2:
>  - Seperate commits for functions
> ---
> ---
>  drivers/bluetooth/btrtl.c | 28 ++++++++++++++++++++++++++++
>  drivers/bluetooth/btrtl.h |  8 ++++++++
>  drivers/bluetooth/btusb.c | 14 ++++++++++++++
>  3 files changed, 50 insertions(+)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index fb52313a1d45..15223b3ed94d 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -781,6 +781,7 @@ void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
>         case CHIP_ID_8852C:
>                 set_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks);
>                 set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
> +               btrealtek_set_flag(hdev, REALTEK_WBS_FILTER);
>                 hci_set_aosp_capable(hdev);
>                 break;
>         default:
> @@ -937,6 +938,33 @@ int btrtl_get_uart_settings(struct hci_dev *hdev,
>  }
>  EXPORT_SYMBOL_GPL(btrtl_get_uart_settings);
>
> +int btrtl_usb_recv_isoc(u16 pos, u8 *data, u8 *p, int len,
> +                       u16 wMaxPacketSize)
> +{
> +       u8 *prev;
> +
> +       if (pos >= HCI_SCO_HDR_SIZE && pos >= wMaxPacketSize &&
> +           len == wMaxPacketSize && !(pos % wMaxPacketSize) &&
> +           wMaxPacketSize >= 10 && p[0] == data[0] && p[1] == data[1]) {
> +               prev = data + (pos - wMaxPacketSize);

Is this attempting to access before the skb-->data in case
wMaxPacketSize is bigger than pos? Anyway I'm not really following the
reasoning you are comparing the data like that, depending on the codec
there could be frames that match exactly but doesn't necessarily means
they are duplicated.

> +
> +               /* Detect the sco data of usb isoc pkt duplication. */
> +               if (!memcmp(p + 2, prev + 2, 8))
> +                       return -EILSEQ;
> +
> +               if (wMaxPacketSize >= 12 &&
> +                   p[2] == prev[6] && p[3] == prev[7] &&
> +                   p[4] == prev[4] && p[5] == prev[5] &&
> +                   p[6] == prev[10] && p[7] == prev[11] &&
> +                   p[8] == prev[8] && p[9] == prev[9]) {
> +                       return -EILSEQ;
> +               }
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(btrtl_usb_recv_isoc);
> +
>  MODULE_AUTHOR("Daniel Drake <drake@endlessm.com>");
>  MODULE_DESCRIPTION("Bluetooth support for Realtek devices ver " VERSION);
>  MODULE_VERSION(VERSION);
> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> index e2c99684799a..79e93a8b229f 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -84,6 +84,8 @@ int btrtl_get_uart_settings(struct hci_dev *hdev,
>                             struct btrtl_device_info *btrtl_dev,
>                             unsigned int *controller_baudrate,
>                             u32 *device_baudrate, bool *flow_control);
> +int btrtl_usb_recv_isoc(u16 pos, u8 *data, u8 *buffer, int len,
> +                               u16 wMaxPacketSize);
>
>  #else
>
> @@ -127,4 +129,10 @@ static inline int btrtl_get_uart_settings(struct hci_dev *hdev,
>         return -ENOENT;
>  }
>
> +static inline int btrtl_usb_recv_isoc(u16 pos, u8 *data, u8 *buffer, int len,
> +                                 u16 wMaxPacketSize)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>  #endif
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 4c3aed89ff05..8e595e03655a 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -961,6 +961,7 @@ static int btusb_recv_isoc(struct btusb_data *data, void *buffer, int count)
>         struct sk_buff *skb;
>         unsigned long flags;
>         int err = 0;
> +       u16 wMaxPacketSize = le16_to_cpu(data->isoc_rx_ep->wMaxPacketSize);
>
>         spin_lock_irqsave(&data->rxlock, flags);
>         skb = data->sco_skb;
> @@ -980,6 +981,19 @@ static int btusb_recv_isoc(struct btusb_data *data, void *buffer, int count)
>                 }
>
>                 len = min_t(uint, hci_skb_expect(skb), count);
> +
> +               /* Gaps in audio could be heard while streaming WBS using USB
> +                * alt settings 3 on some platforms, since this is only used
> +                * with RTK chips so let vendor function detect it.
> +                */
> +               if (test_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags) &&
> +                       btrealtek_test_flag(data->hdev, REALTEK_WBS_FILTER)) {
> +                       err = btrtl_usb_recv_isoc(skb->len, skb->data, buffer,
> +                                                       len, wMaxPacketSize);
> +                       if (err)
> +                               break;
> +               }

If we really need to do this then we need a way for vendors to replace
btus_recv_isoc with the vendor function.

>                 skb_put_data(skb, buffer, len);
>
>                 count -= len;
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz