| Message ID | 20240628101624.3470355-1-hildawu@realtek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: btrtl: fix duplicate SCO packet | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | warning | CheckSparse WARNING drivers/bluetooth/btusb.c:1326:61: warning: restricted __le16 degrades to integerdrivers/bluetooth/btusb.c:1327:70: warning: incorrect type in argument 1 (different base types)drivers/bluetooth/btusb.c:1327:70: expected unsigned short [usertype] mpsdrivers/bluetooth/btusb.c:1327:70: got restricted __le16 [usertype] wMaxPacketSize |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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=866520 ---Test result--- Test Summary: CheckPatch PASS 0.93 seconds GitLint PASS 0.23 seconds SubjectPrefix PASS 0.08 seconds BuildKernel PASS 29.76 seconds CheckAllWarning PASS 32.09 seconds CheckSparse WARNING 37.14 seconds CheckSmatch PASS 106.62 seconds BuildKernel32 PASS 28.64 seconds TestRunnerSetup PASS 523.80 seconds TestRunner_l2cap-tester PASS 23.82 seconds TestRunner_iso-tester PASS 32.67 seconds TestRunner_bnep-tester PASS 4.77 seconds TestRunner_mgmt-tester PASS 113.49 seconds TestRunner_rfcomm-tester PASS 7.39 seconds TestRunner_sco-tester PASS 14.96 seconds TestRunner_ioctl-tester PASS 7.75 seconds TestRunner_mesh-tester PASS 5.90 seconds TestRunner_smp-tester PASS 6.90 seconds TestRunner_userchan-tester PASS 4.94 seconds IncrementalBuild PASS 27.69 seconds Details ############################## Test: CheckSparse - WARNING Desc: Run sparse tool with linux kernel Output: drivers/bluetooth/btusb.c:1326:61: warning: restricted __le16 degrades to integerdrivers/bluetooth/btusb.c:1327:70: warning: incorrect type in argument 1 (different base types)drivers/bluetooth/btusb.c:1327:70: expected unsigned short [usertype] mpsdrivers/bluetooth/btusb.c:1327:70: got restricted __le16 [usertype] wMaxPacketSize --- Regards, Linux Bluetooth
Hi Hilda, On Fri, Jun 28, 2024 at 6:16 AM Hilda Wu <hildawu@realtek.com> wrote: > > In some platform found unknown connection handle case when HFP. The > unknown connection handle may affect SCO audio sound quality. > After investigation, it was found the value of the unknown connection > handle is actually a duplicated data. > > The duplicate data affected the unknown connection handle in some > Realtek chip. This issue only occurs in (e)SCO, does not affect ACLs. > > This commit is to filter out the duplicate packet for avoiding influence > SCO audio. > > Below btmon trace gives a better idea of what we're filtering. > The following excerpts are part of SCO packets in the HCI log: > > > SCO Data RX: Handle 11 flags 0x00 dlen 72 #23327 [hci0] 132.343418 > 8c a3 55 4f 8a d5 56 e9 35 56 37 8d 55 87 53 55 ..UO..V.5V7.U.SU > 59 66 d5 57 1d b5 54 00 01 08 ad 00 00 e0 10 00 Yf.W..T......... > 00 00 85 c6 d5 60 e9 b5 52 94 6d 54 e4 9b 55 b1 .....`..R.mT..U. > b6 d5 62 91 b5 57 84 6d 56 e4 5b 55 75 c6 d5 51 ..b..W.mV.[Uu..Q > 2d b5 53 9a 6d 54 a5 1b -.S.mT.. > < SCO Data TX: Handle 11 flags 0x00 dlen 72 #23328 [hci0] 132.343600 > 01 c8 ad 00 00 aa db ba aa a9 72 b4 d9 5d af 14 ..........r..].. > 53 0c 75 b0 a6 f3 8a 51 b3 54 17 b1 a6 d5 62 c5 S.u....Q.T....b. > d5 6b 35 29 8d c5 1c 56 4c 24 96 9b 8d b5 d7 1a .k5)...VL$...... > b2 8d bc da 3b 8c 46 ae 1d 4d a4 04 01 f8 ad 00 ....;.F..M...... > 00 3d ec bb a9 98 8b 28 .=.....( > > SCO Data RX: Handle 11 flags 0x00 dlen 72 #23329 [hci0] 132.353419 > 55 55 c6 d5 62 29 b5 57 b2 6d 54 00 01 38 ad 00 UU..b).W.mT..8.. > 00 e0 10 00 00 00 0b 00 d5 62 55 c6 57 b2 29 b5 .........bU.W.). > 00 01 6d 54 00 00 38 ad 00 00 e0 10 00 00 00 92 ..mT..8......... > 36 d5 5a ed b5 58 6c 6d 55 b3 1b 55 6b 26 d5 52 6.Z..XlmU..Uk&.R > d1 b5 54 23 6d 56 82 db ..T#mV.. > < SCO Data TX: Handle 11 flags 0x00 dlen 72 #23330 [hci0] 132.353581 > 6d 5b be db 89 34 66 e9 fa 99 a6 6e e5 6d 9f 1a m[...4f....n.m.. > 1c 57 d2 66 92 63 98 99 a9 3b 8a 6c 3e 5b 5a 34 .W.f.c...;.l>[Z4 > a4 96 e2 21 21 8c f8 88 0f 3d e0 52 48 85 18 00 ...!!....=.RH... > 01 08 ad 00 00 0c eb ba a9 a8 28 ca 9a d0 3c 33 ..........(...<3 > 45 4a f9 90 fb ca 4b 39 EJ....K9 > > SCO Data RX: Handle 2901 flags 0x0a dlen 54 #23331 [hci0] 132.373416 > d5 48 a9 b5 56 aa 6d 56 d2 db 55 75 36 d5 56 2d .H..V.mV..Uu6.V- > b5 57 5b 6d 54 00 0b 00 48 01 c8 ad 00 00 e0 10 .W[mT...H....... > 00 00 00 5e c6 d5 56 e1 b5 56 43 6d 55 ca db 55 ...^..V..VCmU..U > 7d c6 d5 5b 31 b5 > > This is HCI SCO data RX packets. > The packet 23327 was a normal HCI SCO data RX packet. > The packet 23329 was the abnormal HCI SCO data RX packet. > The packet 23331 was the invalid connection handle case but the packet is > affected by the packet 23329 abnormal HCI SCO Data RX packet. > > So this patch expects to filter the packet 23329 SCO data RX packet > case. The packet 23329's connection handle (0x0B 00/11) and length > (0x48/72) is normal. > This btmon trace is SCO packets in USB alternate setting 3, payload > length is 72 bytes that consist of three SCO data packets. > The anomaly is due to the intermediate composed data.The duplicate > data in the intermediate composition data, but it affects packet > combination. Cause the system parses the next packet of the connection > handle mistake that shows unknown connection handle messages. > > This commit can estimate and find out its abnormal rule to filter the > duplicate packet out for avoiding influence. > Check fragments and filter out the abnormal packet, and then it will > not affect the system parsing of the connection handle subsequent. > > Signed-off-by: Alex Lu <alex_lu@realsil.com.cn> > Signed-off-by: Hilda Wu <hildawu@realtek.com> > --- > drivers/bluetooth/btrtl.c | 49 +++++++++++++++++++++++++++++++++++++++ > drivers/bluetooth/btrtl.h | 7 ++++++ > drivers/bluetooth/btusb.c | 8 +++++++ > 3 files changed, 64 insertions(+) > > diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > index f2f37143c454..f286654a8fae 100644 > --- a/drivers/bluetooth/btrtl.c > +++ b/drivers/bluetooth/btrtl.c > @@ -1300,6 +1300,11 @@ 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_8822C || > + btrtl_dev->project_id == CHIP_ID_8852A || > + btrtl_dev->project_id == CHIP_ID_8852B) > + btrealtek_set_flag(hdev, REALTEK_SCO_CLEAN_DUPLICATE_DATA); > + > hci_set_aosp_capable(hdev); > break; > default: > @@ -1479,6 +1484,50 @@ int btrtl_get_uart_settings(struct hci_dev *hdev, > } > EXPORT_SYMBOL_GPL(btrtl_get_uart_settings); > > +int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb) > +{ > + u8 *prev; > + u8 tmp[8]; > + u32 *a; > + u32 *b; > + u16 i; > + u8 *next; > + u8 *start = skb->data; > + > + for (i = 0; i < 2; i++) { > + prev = start + i * mps; > + next = prev + mps; > + > + if (!memcmp(prev + 4, next + 2, 8)) > + continue; > + > + /* Check the current fragment with the previous one. > + * If the current fragment is redundant but it is a little bit > + * different from the previous, drop it. > + * For example, > + * 04 00 48 55 4E CB 55 52 80 95 55 07 XX XX ... > + * 04 00 55 52 4E CB 55 07 80 95 XX XX XX XX ... > + */ > + memcpy(tmp, prev + 4, 8); > + a = (u32 *)(tmp); > + b = (u32 *)(tmp + 4); > + *a = swahw32(*a); > + *b = swahw32(*b); > + > + if (next[0] == prev[0] && next[1] == prev[1] && > + !memcmp(next + 2, tmp, 8)) { > + if (i == 0) > + memcpy(start + mps, start + 2 * mps, mps); > + skb_trim(skb, 2 * mps); > + hci_skb_expect(skb) = mps; > + return -EILSEQ; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(btrtl_validate_isoc_data); NAK, the driver has no business deep inspecting the packets like that, specially since you are not doing any length checks. > 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 a2d9d34f9fb0..3ddb691dff94 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_SCO_CLEAN_DUPLICATE_DATA, > > __REALTEK_NUM_FLAGS, > }; > @@ -148,6 +149,7 @@ int btrtl_get_uart_settings(struct hci_dev *hdev, > unsigned int *controller_baudrate, > u32 *device_baudrate, bool *flow_control); > void btrtl_set_driver_name(struct hci_dev *hdev, const char *driver_name); > +int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb); > > #else > > @@ -195,4 +197,9 @@ static inline void btrtl_set_driver_name(struct hci_dev *hdev, const char *drive > { > } > > +static inline int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb) > +{ > + return -EILSEQ; > +} > + > #endif > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 2d7d47f9d007..2b66211eb02c 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -1317,6 +1317,14 @@ static int btusb_recv_isoc(struct btusb_data *data, void *buffer, int count) > } > > if (!hci_skb_expect(skb)) { > + if (btrealtek_test_flag(data->hdev, REALTEK_SCO_CLEAN_DUPLICATE_DATA) && > + data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP && > + test_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags) && > + data->isoc_altsetting == 3 && > + skb->len == 3 * data->isoc_rx_ep->wMaxPacketSize && > + btrtl_validate_isoc_data(data->isoc_rx_ep->wMaxPacketSize, skb)) > + continue; Can't you take a simpler approach and not use the ALT3 setting to begin with? So instead of handling this via REALTEK_SCO_CLEAN_DUPLICATE_DATA just don't set BTUSB_USE_ALT3_FOR_WBS, if that means WBS cannot be used then so be it since NBS is still better than a broken WBS. > /* Complete frame */ > hci_recv_frame(data->hdev, skb); > skb = NULL; > -- > 2.34.1 >
diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c index f2f37143c454..f286654a8fae 100644 --- a/drivers/bluetooth/btrtl.c +++ b/drivers/bluetooth/btrtl.c @@ -1300,6 +1300,11 @@ 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_8822C || + btrtl_dev->project_id == CHIP_ID_8852A || + btrtl_dev->project_id == CHIP_ID_8852B) + btrealtek_set_flag(hdev, REALTEK_SCO_CLEAN_DUPLICATE_DATA); + hci_set_aosp_capable(hdev); break; default: @@ -1479,6 +1484,50 @@ int btrtl_get_uart_settings(struct hci_dev *hdev, } EXPORT_SYMBOL_GPL(btrtl_get_uart_settings); +int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb) +{ + u8 *prev; + u8 tmp[8]; + u32 *a; + u32 *b; + u16 i; + u8 *next; + u8 *start = skb->data; + + for (i = 0; i < 2; i++) { + prev = start + i * mps; + next = prev + mps; + + if (!memcmp(prev + 4, next + 2, 8)) + continue; + + /* Check the current fragment with the previous one. + * If the current fragment is redundant but it is a little bit + * different from the previous, drop it. + * For example, + * 04 00 48 55 4E CB 55 52 80 95 55 07 XX XX ... + * 04 00 55 52 4E CB 55 07 80 95 XX XX XX XX ... + */ + memcpy(tmp, prev + 4, 8); + a = (u32 *)(tmp); + b = (u32 *)(tmp + 4); + *a = swahw32(*a); + *b = swahw32(*b); + + if (next[0] == prev[0] && next[1] == prev[1] && + !memcmp(next + 2, tmp, 8)) { + if (i == 0) + memcpy(start + mps, start + 2 * mps, mps); + skb_trim(skb, 2 * mps); + hci_skb_expect(skb) = mps; + return -EILSEQ; + } + } + + return 0; +} +EXPORT_SYMBOL_GPL(btrtl_validate_isoc_data); + 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 a2d9d34f9fb0..3ddb691dff94 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_SCO_CLEAN_DUPLICATE_DATA, __REALTEK_NUM_FLAGS, }; @@ -148,6 +149,7 @@ int btrtl_get_uart_settings(struct hci_dev *hdev, unsigned int *controller_baudrate, u32 *device_baudrate, bool *flow_control); void btrtl_set_driver_name(struct hci_dev *hdev, const char *driver_name); +int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb); #else @@ -195,4 +197,9 @@ static inline void btrtl_set_driver_name(struct hci_dev *hdev, const char *drive { } +static inline int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb) +{ + return -EILSEQ; +} + #endif diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 2d7d47f9d007..2b66211eb02c 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -1317,6 +1317,14 @@ static int btusb_recv_isoc(struct btusb_data *data, void *buffer, int count) } if (!hci_skb_expect(skb)) { + if (btrealtek_test_flag(data->hdev, REALTEK_SCO_CLEAN_DUPLICATE_DATA) && + data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP && + test_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags) && + data->isoc_altsetting == 3 && + skb->len == 3 * data->isoc_rx_ep->wMaxPacketSize && + btrtl_validate_isoc_data(data->isoc_rx_ep->wMaxPacketSize, skb)) + continue; + /* Complete frame */ hci_recv_frame(data->hdev, skb); skb = NULL;