Hi Nicolas,
On Tue, Oct 4, 2022 at 7:54 AM Nicolas Cavallari
<nicolas.cavallari@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> The USB interface between the host and the bluetooth adapter used for
> SCO packets uses an USB isochronous endpoint with a fragmentation scheme
> that does not tolerate errors. Except USB isochronous transfers do
> not provide a reliable stream with guaranteed delivery. (There is no
> retry on error, see USB spec v2.0 5.6 and 8.5.5.)
>
> To fragment a packet, the bluetooth HCI simply splits it in parts and
> transfer them as-is. The receiver is expected to reconstruct the packet
> by assuming the first fragment contains the header and parsing its size
> field. There is no error detection either.
>
> If a fragment is lost, the end result is that the kernel is no longer
> synchronized and will pass malformed data to the upper layers, since it
> has no way to tell if the first fragment is an actual first fragment or
> a continuation fragment. Resynchronization can only happen by luck and
> requires an unbounded amount of time.
>
> The typical symptom for a HSP/HFP bluetooth headset is that the
> microphone stops working and dmesg contains piles of rate-limited
> "Bluetooth: hci0: SCO packet for unknown connection handle XXXX"
> errors for an indeterminate amount of time, until the kernel accidentally
> resynchronize.
>
> A workaround is to ask the upper layer to prevalidate the first fragment
> header. This is not possible with user channels so this workaround is
> disabled in this case.
>
> This problem is the most severe when using an ath3k adapter on an i.MX 6
> board, where packet loss occur regularly, possibly because it is an USB1
> device connected on an USB2 hub and this is a special case requiring
> split transactions.
Interesting, but does this actually make it work if with the packet losses?
> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@xxxxxxxxxxxxxxxxxxxxxxx>
>
> ---
> v2: fix typos in commit description and expand it
>
> drivers/bluetooth/btusb.c | 7 +++++--
> include/net/bluetooth/hci_core.h | 20 ++++++++++++++++++++
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 15caa6469538..f6256af98233 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -980,9 +980,12 @@ static int btusb_recv_isoc(struct btusb_data *data, void *buffer, int count)
>
> if (skb->len == HCI_SCO_HDR_SIZE) {
> /* Complete SCO header */
> - hci_skb_expect(skb) = hci_sco_hdr(skb)->dlen;
> + struct hci_sco_hdr *hdr = hci_sco_hdr(skb);
>
> - if (skb_tailroom(skb) < hci_skb_expect(skb)) {
> + hci_skb_expect(skb) = hdr->dlen;
> +
> + if (skb_tailroom(skb) < hci_skb_expect(skb) ||
> + !hci_conn_prevalidate_sco_hdr(data->hdev, hdr)) {
> kfree_skb(skb);
> skb = NULL;
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e7862903187d..d589b54e89e6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1286,6 +1286,26 @@ static inline struct hci_conn *hci_lookup_le_connect(struct hci_dev *hdev)
> return NULL;
> }
>
> +static inline bool hci_conn_prevalidate_sco_hdr(struct hci_dev *hdev,
> + struct hci_sco_hdr *hdr)
> +{
> + __u16 handle;
> +
> + if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
> + // Can't validate, userspace controls everything.
> + return true;
> +
> + handle = hci_handle(__le16_to_cpu(hdr->handle));
> +
> + switch (hci_conn_lookup_type(hdev, handle)) {
> + case SCO_LINK:
> + case ESCO_LINK:
> + return true;
> + default:
> + return false;
> + }
> +}
Don't really like to have this in hci_core.h, it is sort of messy
already beside this is probably too specific to USB so I'd go with
something like btusb_validate_sco_handle and add a comment explaining
why this is necessary.
> int hci_disconnect(struct hci_conn *conn, __u8 reason);
> bool hci_setup_sync(struct hci_conn *conn, __u16 handle);
> void hci_sco_setup(struct hci_conn *conn, __u8 status);
> --
> 2.37.2
>
--
Luiz Augusto von Dentz
