linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Bluetooth: Add btrealtek data struct and improve SCO sound quality of RTK chips
@ 2022-09-29 11:44 hildawu
  2022-09-29 11:44 ` [PATCH v3 1/3] Bluetooth: btrtl: Add btrealtek data struct hildawu
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: hildawu @ 2022-09-29 11:44 UTC (permalink / raw)
  To: marcel
  Cc: johan.hedberg, luiz.dentz, linux-bluetooth, linux-kernel,
	apusaka, yinghsu, max.chou, alex_lu, kidman

From: Hilda Wu <hildawu@realtek.com>

Add btrealtek data struct and use definition of vendor flags to manage the
specific chip.

The Bluetooth: btusb: Workaround for spotty SCO quality patch is for
filter out duplicate packet for avoiding spotty SCO quality.
- The comparison of btrtl_usb_recv_isoc here is for invalid handle,
the invalid handle shouldn't appear.
So we try to find out the rule and filter out this.

The Bluetooth: btsub: Ignore zero length of USB packets on ALT 6 patch is
for some Realtek chips need to transmit mSBC data continuously without the
zero length of USB packets.

---
Changes in v3:
 - Use the vendor function to replace btus_recv_isoc.
 - For ignore_usb_alt6_packet_flow, manage the common flag by the vendor private flag.

Changes in v2:
 - Set the proper priv_size to hci_alloc_dev_priv().
 - Separate commits for functions.
---
Thank you for your review and suggestions.

Hilda Wu (3):
  Bluetooth: btrtl: Add btrealtek data struct
  Bluetooth: btusb: Workaround for spotty SCO quality
  Bluetooth:btsub:Ignore zero length of USB packets on ALT 6 for
    specific chip

 drivers/bluetooth/btrtl.c |  34 ++++++++++++
 drivers/bluetooth/btrtl.h |  29 +++++++++++
 drivers/bluetooth/btusb.c | 105 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 165 insertions(+), 3 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/3] Bluetooth: btrtl: Add btrealtek data struct
  2022-09-29 11:44 [PATCH v3 0/3] Bluetooth: Add btrealtek data struct and improve SCO sound quality of RTK chips hildawu
@ 2022-09-29 11:44 ` hildawu
  2022-09-29 12:11   ` Bluetooth: Add btrealtek data struct and improve SCO sound quality of RTK chips bluez.test.bot
  2022-09-29 11:44 ` [PATCH v3 2/3] Bluetooth: btusb: Workaround for spotty SCO quality hildawu
  2022-09-29 11:44 ` [PATCH v3 3/3] Bluetooth:btsub:Ignore zero length of USB packets on ALT 6 for specific chip hildawu
  2 siblings, 1 reply; 6+ messages in thread
From: hildawu @ 2022-09-29 11:44 UTC (permalink / raw)
  To: marcel
  Cc: johan.hedberg, luiz.dentz, linux-bluetooth, linux-kernel,
	apusaka, yinghsu, max.chou, alex_lu, kidman

From: Hilda Wu <hildawu@realtek.com>

This patch adds a data structure for btrealtek object, and the
definition of vendor behavior flags. It also adds macros to set/test/get
the flags.

Signed-off-by: Hilda Wu <hildawu@realtek.com>
---
Changes in v3:
 - Update private flag.

Changes in v2:
 - Set the proper priv_size to hci_alloc_dev_priv().
 - Separate commits for functions.
---
---
 drivers/bluetooth/btrtl.h | 21 +++++++++++++++++++++
 drivers/bluetooth/btusb.c |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
index 2c441bda390a..ebf0101c959b 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -47,6 +47,27 @@ struct rtl_vendor_config {
 	struct rtl_vendor_config_entry entry[];
 } __packed;
 
+enum {
+	REALTEK_ALT6_CONTINUOUS_TX_CHIP,
+
+	__REALTEK_NUM_FLAGS,
+};
+
+struct btrealtek_data {
+	DECLARE_BITMAP(flags, __REALTEK_NUM_FLAGS);
+};
+
+#define btrealtek_set_flag(hdev, nr)					\
+	do {								\
+		struct btrealtek_data *realtek = hci_get_priv((hdev));	\
+		set_bit((nr), realtek->flags);				\
+	} while (0)
+
+#define btrealtek_get_flag(hdev)					\
+	(((struct btrealtek_data *)hci_get_priv(hdev))->flags)
+
+#define btrealtek_test_flag(hdev, nr)	test_bit((nr), btrealtek_get_flag(hdev))
+
 #if IS_ENABLED(CONFIG_BT_RTL)
 
 struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index c3daba17de7f..4c3aed89ff05 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3749,6 +3749,9 @@ static int btusb_probe(struct usb_interface *intf,
 		/* Override the rx handlers */
 		data->recv_event = btusb_recv_event_intel;
 		data->recv_bulk = btusb_recv_bulk_intel;
+	} else if (id->driver_info & BTUSB_REALTEK) {
+		/* Allocate extra space for Realtek device */
+		priv_size += sizeof(struct btrealtek_data);
 	}
 
 	data->recv_acl = hci_recv_frame;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/3] Bluetooth: btusb: Workaround for spotty SCO quality
  2022-09-29 11:44 [PATCH v3 0/3] Bluetooth: Add btrealtek data struct and improve SCO sound quality of RTK chips hildawu
  2022-09-29 11:44 ` [PATCH v3 1/3] Bluetooth: btrtl: Add btrealtek data struct hildawu
@ 2022-09-29 11:44 ` hildawu
  2022-09-29 19:35   ` Luiz Augusto von Dentz
  2022-09-29 11:44 ` [PATCH v3 3/3] Bluetooth:btsub:Ignore zero length of USB packets on ALT 6 for specific chip hildawu
  2 siblings, 1 reply; 6+ messages in thread
From: hildawu @ 2022-09-29 11:44 UTC (permalink / raw)
  To: marcel
  Cc: johan.hedberg, luiz.dentz, linux-bluetooth, linux-kernel,
	apusaka, yinghsu, max.chou, alex_lu, kidman

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: Alex Lu <alex_lu@realsil.com.cn>
Signed-off-by: Hilda Wu <hildawu@realtek.com>
---
Changes in v3:
 - Use the vendor function to replace btus_recv_isoc
 - Additional info: The comparison of btrtl_usb_recv_isoc here is for invalid handle,
   the invalid handle shouldn't appear. So we try to find out the rule and filter out this.

Changes in v2:
 - Seperate commits for functions
---
---
 drivers/bluetooth/btrtl.c | 27 ++++++++++++++
 drivers/bluetooth/btrtl.h |  8 ++++
 drivers/bluetooth/btusb.c | 77 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index fb52313a1d45..272f90621a10 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -937,6 +937,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);
+
+		/* 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 ebf0101c959b..c9783e8fe0aa 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -83,6 +83,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
 
@@ -126,4 +128,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..7583059e3b2c 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -683,6 +683,7 @@ struct btusb_data {
 	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
 	int (*recv_acl)(struct hci_dev *hdev, struct sk_buff *skb);
 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
+	int (*recv_isoc)(struct btusb_data *data, void *buffer, int count);
 
 	int (*setup_on_usb)(struct hci_dev *hdev);
 
@@ -1239,7 +1240,7 @@ static void btusb_isoc_complete(struct urb *urb)
 
 			hdev->stat.byte_rx += length;
 
-			if (btusb_recv_isoc(data, urb->transfer_buffer + offset,
+			if (data->recv_isoc(data, urb->transfer_buffer + offset,
 					    length) < 0) {
 				bt_dev_err(hdev, "corrupted SCO packet");
 				hdev->stat.err_rx++;
@@ -2309,6 +2310,77 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
 	return -EILSEQ;
 }
 
+static int btusb_recv_isoc_realtek(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;
+
+	while (count) {
+		int len;
+
+		if (!skb) {
+			skb = bt_skb_alloc(HCI_MAX_SCO_SIZE, GFP_ATOMIC);
+			if (!skb) {
+				err = -ENOMEM;
+				break;
+			}
+
+			hci_skb_pkt_type(skb) = HCI_SCODATA_PKT;
+			hci_skb_expect(skb) = HCI_SCO_HDR_SIZE;
+		}
+
+		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 (!btusb_find_altsetting(data, 6) &&
+			test_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags)) {
+			err = btrtl_usb_recv_isoc(skb->len, skb->data, buffer,
+							len, wMaxPacketSize);
+			if (err)
+				break;
+		}
+
+		skb_put_data(skb, buffer, len);
+
+		count -= len;
+		buffer += len;
+		hci_skb_expect(skb) -= len;
+
+		if (skb->len == HCI_SCO_HDR_SIZE) {
+			/* Complete SCO header */
+			hci_skb_expect(skb) = hci_sco_hdr(skb)->dlen;
+
+			if (skb_tailroom(skb) < hci_skb_expect(skb)) {
+				kfree_skb(skb);
+				skb = NULL;
+
+				err = -EILSEQ;
+				break;
+			}
+		}
+
+		if (!hci_skb_expect(skb)) {
+			/* Complete frame */
+			hci_recv_frame(data->hdev, skb);
+			skb = NULL;
+		}
+	}
+
+	data->sco_skb = skb;
+	spin_unlock_irqrestore(&data->rxlock, flags);
+
+	return err;
+}
+
 /* UHW CR mapping */
 #define MTK_BT_MISC		0x70002510
 #define MTK_BT_SUBSYS_RST	0x70002610
@@ -3741,6 +3813,7 @@ static int btusb_probe(struct usb_interface *intf,
 
 	data->recv_event = hci_recv_frame;
 	data->recv_bulk = btusb_recv_bulk;
+	data->recv_isoc = btusb_recv_isoc;
 
 	if (id->driver_info & BTUSB_INTEL_COMBINED) {
 		/* Allocate extra space for Intel device */
@@ -3914,6 +3987,8 @@ static int btusb_probe(struct usb_interface *intf,
 		hdev->shutdown = btrtl_shutdown_realtek;
 		hdev->cmd_timeout = btusb_rtl_cmd_timeout;
 
+		data->recv_isoc = btusb_recv_isoc_realtek;
+
 		/* Realtek devices need to set remote wakeup on auto-suspend */
 		set_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags);
 		set_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 3/3] Bluetooth:btsub:Ignore zero length of USB packets on ALT 6 for specific chip
  2022-09-29 11:44 [PATCH v3 0/3] Bluetooth: Add btrealtek data struct and improve SCO sound quality of RTK chips hildawu
  2022-09-29 11:44 ` [PATCH v3 1/3] Bluetooth: btrtl: Add btrealtek data struct hildawu
  2022-09-29 11:44 ` [PATCH v3 2/3] Bluetooth: btusb: Workaround for spotty SCO quality hildawu
@ 2022-09-29 11:44 ` hildawu
  2 siblings, 0 replies; 6+ messages in thread
From: hildawu @ 2022-09-29 11:44 UTC (permalink / raw)
  To: marcel
  Cc: johan.hedberg, luiz.dentz, linux-bluetooth, linux-kernel,
	apusaka, yinghsu, max.chou, alex_lu, kidman

From: Hilda Wu <hildawu@realtek.com>

For USB ALT 6 settings some Realtek chips need to transmit mSBC data
continuously without the zero length of USB packets.
In this commit, create BTUSB_ALT6_CONTINUOUS_TX to manage the behavior.
Therefore, create REALTEK_ALT6_CONTINUOUS_TX_CHIP to manage the specific
chip model for the behavior.

Signed-off-by: Max Chou <max.chou@realtek.com>
Signed-off-by: Hilda Wu <hildawu@realtek.com>
---
Changes in V3
 - For ignore_usb_alt6_packet_flow, manage the common flag by the vendor private flag.
---
---
 drivers/bluetooth/btrtl.c |  7 +++++++
 drivers/bluetooth/btusb.c | 25 +++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 272f90621a10..7159427c530e 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -781,6 +781,13 @@ 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);
+
+		/* RTL8852C needs to transmit mSBC data continuously without
+		 * the zero length of USB packets for the ALT 6 supported chips
+		 */
+		if (btrtl_dev->project_id == CHIP_ID_8852C)
+			btrealtek_set_flag(hdev, REALTEK_ALT6_CONTINUOUS_TX_CHIP);
+
 		hci_set_aosp_capable(hdev);
 		break;
 	default:
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7583059e3b2c..076cfee1ad57 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -626,6 +626,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
 #define BTUSB_TX_WAIT_VND_EVT	13
 #define BTUSB_WAKEUP_AUTOSUSPEND	14
 #define BTUSB_USE_ALT3_FOR_WBS	15
+#define BTUSB_ALT6_CONTINUOUS_TX	16
 
 struct btusb_data {
 	struct hci_dev       *hdev;
@@ -1271,11 +1272,17 @@ static void btusb_isoc_complete(struct urb *urb)
 static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
 					       int mtu, struct btusb_data *data)
 {
-	int i, offset = 0;
+	int i = 0, offset = 0;
 	unsigned int interval;
 
 	BT_DBG("len %d mtu %d", len, mtu);
 
+	/* For mSBC ALT 6 settings some chips need to transmit the data
+	 * continuously without the zero length of USB packets.
+	 */
+	if (test_bit(BTUSB_ALT6_CONTINUOUS_TX, &data->flags))
+		goto ignore_usb_alt6_packet_flow;
+
 	/* For mSBC ALT 6 setting the host will send the packet at continuous
 	 * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting
 	 * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets.
@@ -1295,6 +1302,7 @@ static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
 		urb->iso_frame_desc[i].length = offset;
 	}
 
+ignore_usb_alt6_packet_flow:
 	if (len && i < BTUSB_MAX_ISOC_FRAMES) {
 		urb->iso_frame_desc[i].offset = offset;
 		urb->iso_frame_desc[i].length = len;
@@ -2381,6 +2389,19 @@ static int btusb_recv_isoc_realtek(struct btusb_data *data, void *buffer,
 	return err;
 }
 
+static int btusb_setup_realtek(struct hci_dev *hdev)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	int ret;
+
+	ret = btrtl_setup_realtek(hdev);
+
+	if (btrealtek_test_flag(data->hdev, REALTEK_ALT6_CONTINUOUS_TX_CHIP))
+		set_bit(BTUSB_ALT6_CONTINUOUS_TX, &data->flags);
+
+	return ret;
+}
+
 /* UHW CR mapping */
 #define MTK_BT_MISC		0x70002510
 #define MTK_BT_SUBSYS_RST	0x70002610
@@ -3983,7 +4004,7 @@ static int btusb_probe(struct usb_interface *intf,
 
 	if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) &&
 	    (id->driver_info & BTUSB_REALTEK)) {
-		hdev->setup = btrtl_setup_realtek;
+		hdev->setup = btusb_setup_realtek;
 		hdev->shutdown = btrtl_shutdown_realtek;
 		hdev->cmd_timeout = btusb_rtl_cmd_timeout;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: Bluetooth: Add btrealtek data struct and improve SCO sound quality of RTK chips
  2022-09-29 11:44 ` [PATCH v3 1/3] Bluetooth: btrtl: Add btrealtek data struct hildawu
@ 2022-09-29 12:11   ` bluez.test.bot
  0 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2022-09-29 12:11 UTC (permalink / raw)
  To: linux-bluetooth, hildawu

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=681879

---Test result---

Test Summary:
CheckPatch                    PASS      4.94 seconds
GitLint                       FAIL      2.88 seconds
SubjectPrefix                 FAIL      1.93 seconds
BuildKernel                   PASS      44.62 seconds
BuildKernel32                 PASS      40.86 seconds
Incremental Build with patchesPASS      75.88 seconds
TestRunner: Setup             PASS      694.33 seconds
TestRunner: l2cap-tester      PASS      21.50 seconds
TestRunner: iso-tester        PASS      22.09 seconds
TestRunner: bnep-tester       PASS      8.42 seconds
TestRunner: mgmt-tester       PASS      137.38 seconds
TestRunner: rfcomm-tester     PASS      13.29 seconds
TestRunner: sco-tester        PASS      12.30 seconds
TestRunner: ioctl-tester      PASS      14.72 seconds
TestRunner: smp-tester        PASS      12.54 seconds
TestRunner: userchan-tester   PASS      9.03 seconds

Details
##############################
Test: GitLint - FAIL - 2.88 seconds
Run gitlint with rule in .gitlint
[v3,2/3] Bluetooth: btusb: Workaround for spotty SCO quality
18: B1 Line exceeds max length (85>80): " - Additional info: The comparison of btrtl_usb_recv_isoc here is for invalid handle,"
19: B1 Line exceeds max length (91>80): "   the invalid handle shouldn't appear. So we try to find out the rule and filter out this."

[v3,3/3] Bluetooth:btsub:Ignore zero length of USB packets on ALT 6 for specific chip
1: T1 Title exceeds max length (85>80): "[v3,3/3] Bluetooth:btsub:Ignore zero length of USB packets on ALT 6 for specific chip"
15: B1 Line exceeds max length (86>80): " - For ignore_usb_alt6_packet_flow, manage the common flag by the vendor private flag."


##############################
Test: SubjectPrefix - FAIL - 1.93 seconds
Check subject contains "Bluetooth" prefix
"Bluetooth: " is not specified in the subject



---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/3] Bluetooth: btusb: Workaround for spotty SCO quality
  2022-09-29 11:44 ` [PATCH v3 2/3] Bluetooth: btusb: Workaround for spotty SCO quality hildawu
@ 2022-09-29 19:35   ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2022-09-29 19:35 UTC (permalink / raw)
  To: hildawu
  Cc: marcel, johan.hedberg, linux-bluetooth, linux-kernel, apusaka,
	yinghsu, max.chou, alex_lu, kidman

Hi Hilda,

On Thu, Sep 29, 2022 at 4:44 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.

Please add the btmon trace to make it more clear what we are dealing with.

> 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: Alex Lu <alex_lu@realsil.com.cn>
> Signed-off-by: Hilda Wu <hildawu@realtek.com>
> ---
> Changes in v3:
>  - Use the vendor function to replace btus_recv_isoc
>  - Additional info: The comparison of btrtl_usb_recv_isoc here is for invalid handle,
>    the invalid handle shouldn't appear. So we try to find out the rule and filter out this.
>
> Changes in v2:
>  - Seperate commits for functions
> ---
> ---
>  drivers/bluetooth/btrtl.c | 27 ++++++++++++++
>  drivers/bluetooth/btrtl.h |  8 ++++
>  drivers/bluetooth/btusb.c | 77 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index fb52313a1d45..272f90621a10 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -937,6 +937,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);
> +
> +               /* 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;
> +               }
> +       }

I'm really not getting what you trying to do above, if you want to
check if the handle is valid there use lookup function like we do for
example here:

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/drivers/bluetooth/btusb.c#n937

It is sort of a layer violation though and probably wouldn't work with
the likes of the user channel because the stack may not be handling
the connections, so the alternative would be to store the handle
somewhere when we start processing the fragments and then make sure
every fragment matches the handle.

> +       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 ebf0101c959b..c9783e8fe0aa 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -83,6 +83,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
>
> @@ -126,4 +128,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..7583059e3b2c 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -683,6 +683,7 @@ struct btusb_data {
>         int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
>         int (*recv_acl)(struct hci_dev *hdev, struct sk_buff *skb);
>         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> +       int (*recv_isoc)(struct btusb_data *data, void *buffer, int count);
>
>         int (*setup_on_usb)(struct hci_dev *hdev);
>
> @@ -1239,7 +1240,7 @@ static void btusb_isoc_complete(struct urb *urb)
>
>                         hdev->stat.byte_rx += length;
>
> -                       if (btusb_recv_isoc(data, urb->transfer_buffer + offset,
> +                       if (data->recv_isoc(data, urb->transfer_buffer + offset,
>                                             length) < 0) {
>                                 bt_dev_err(hdev, "corrupted SCO packet");
>                                 hdev->stat.err_rx++;
> @@ -2309,6 +2310,77 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
>         return -EILSEQ;
>  }
>
> +static int btusb_recv_isoc_realtek(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;
> +
> +       while (count) {
> +               int len;
> +
> +               if (!skb) {
> +                       skb = bt_skb_alloc(HCI_MAX_SCO_SIZE, GFP_ATOMIC);
> +                       if (!skb) {
> +                               err = -ENOMEM;
> +                               break;
> +                       }
> +
> +                       hci_skb_pkt_type(skb) = HCI_SCODATA_PKT;
> +                       hci_skb_expect(skb) = HCI_SCO_HDR_SIZE;
> +               }
> +
> +               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 (!btusb_find_altsetting(data, 6) &&
> +                       test_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags)) {
> +                       err = btrtl_usb_recv_isoc(skb->len, skb->data, buffer,
> +                                                       len, wMaxPacketSize);
> +                       if (err)
> +                               break;
> +               }
> +
> +               skb_put_data(skb, buffer, len);
> +
> +               count -= len;
> +               buffer += len;
> +               hci_skb_expect(skb) -= len;
> +
> +               if (skb->len == HCI_SCO_HDR_SIZE) {
> +                       /* Complete SCO header */
> +                       hci_skb_expect(skb) = hci_sco_hdr(skb)->dlen;
> +
> +                       if (skb_tailroom(skb) < hci_skb_expect(skb)) {
> +                               kfree_skb(skb);
> +                               skb = NULL;
> +
> +                               err = -EILSEQ;
> +                               break;
> +                       }
> +               }
> +
> +               if (!hci_skb_expect(skb)) {
> +                       /* Complete frame */
> +                       hci_recv_frame(data->hdev, skb);
> +                       skb = NULL;
> +               }
> +       }
> +
> +       data->sco_skb = skb;
> +       spin_unlock_irqrestore(&data->rxlock, flags);
> +
> +       return err;
> +}
> +
>  /* UHW CR mapping */
>  #define MTK_BT_MISC            0x70002510
>  #define MTK_BT_SUBSYS_RST      0x70002610
> @@ -3741,6 +3813,7 @@ static int btusb_probe(struct usb_interface *intf,
>
>         data->recv_event = hci_recv_frame;
>         data->recv_bulk = btusb_recv_bulk;
> +       data->recv_isoc = btusb_recv_isoc;
>
>         if (id->driver_info & BTUSB_INTEL_COMBINED) {
>                 /* Allocate extra space for Intel device */
> @@ -3914,6 +3987,8 @@ static int btusb_probe(struct usb_interface *intf,
>                 hdev->shutdown = btrtl_shutdown_realtek;
>                 hdev->cmd_timeout = btusb_rtl_cmd_timeout;
>
> +               data->recv_isoc = btusb_recv_isoc_realtek;
> +
>                 /* Realtek devices need to set remote wakeup on auto-suspend */
>                 set_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags);
>                 set_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags);
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-09-29 19:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-29 11:44 [PATCH v3 0/3] Bluetooth: Add btrealtek data struct and improve SCO sound quality of RTK chips hildawu
2022-09-29 11:44 ` [PATCH v3 1/3] Bluetooth: btrtl: Add btrealtek data struct hildawu
2022-09-29 12:11   ` Bluetooth: Add btrealtek data struct and improve SCO sound quality of RTK chips bluez.test.bot
2022-09-29 11:44 ` [PATCH v3 2/3] Bluetooth: btusb: Workaround for spotty SCO quality hildawu
2022-09-29 19:35   ` Luiz Augusto von Dentz
2022-09-29 11:44 ` [PATCH v3 3/3] Bluetooth:btsub:Ignore zero length of USB packets on ALT 6 for specific chip hildawu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).