| Message ID | 20240726095828.2707111-1-quic_chejiang@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] Bluetooth: hci_qca: Drop unused event during BT on | expand |
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=874041 ---Test result--- Test Summary: CheckPatch PASS 0.49 seconds GitLint PASS 0.21 seconds SubjectPrefix PASS 0.06 seconds BuildKernel PASS 30.47 seconds CheckAllWarning PASS 33.56 seconds CheckSparse PASS 41.00 seconds CheckSmatch PASS 103.76 seconds BuildKernel32 PASS 29.81 seconds TestRunnerSetup PASS 539.76 seconds TestRunner_l2cap-tester PASS 23.00 seconds TestRunner_iso-tester FAIL 39.30 seconds TestRunner_bnep-tester PASS 4.85 seconds TestRunner_mgmt-tester FAIL 113.26 seconds TestRunner_rfcomm-tester PASS 7.45 seconds TestRunner_sco-tester PASS 13.08 seconds TestRunner_ioctl-tester PASS 8.00 seconds TestRunner_mesh-tester PASS 5.96 seconds TestRunner_smp-tester PASS 6.90 seconds TestRunner_userchan-tester PASS 5.00 seconds IncrementalBuild PASS 27.51 seconds Details ############################## Test: TestRunner_iso-tester - FAIL Desc: Run iso-tester with test-runner Output: Total: 122, Passed: 117 (95.9%), Failed: 1, Not Run: 4 Failed Test Cases ISO Connect Suspend - Success Failed 6.191 seconds ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2 Failed Test Cases LL Privacy - Remove Device 4 (Disable Adv) Timed out 1.841 seconds --- Regards, Linux Bluetooth
Dear receivers, Is there any comment for the changes? Thanks! On 7/26/2024 5:58 PM, Cheng Jiang wrote: > For the WCN6750/WCN6855/WCN7850, the vendor command for a baudrate > change is not sent as synchronous HCI command, controller sends the > corresponding vendor event with the new baudrate. It needs to be > dropped, otherwise it may be misinterpreted as response to a later > command. > > Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com> > --- > drivers/bluetooth/hci_qca.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index ca6466676902..f497d601e035 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -1206,7 +1206,15 @@ static int qca_recv_event(struct hci_dev *hdev, struct sk_buff *skb) > * vendor command). > */ > > - if (hdr->evt == HCI_EV_VENDOR) > + /* For the WCN6750/WCN6855/WCN7850, like the WCN3990, the > + * vendor command for a baudrate change command isn't sent as > + * synchronous HCI command, the controller sends the corresponding > + * command complete event with the new baudrate. The event is > + * received and properly decoded after changing the baudrate of > + * the host port. It needs to be dropped. > + */ > + > + if (hdr->evt == HCI_EV_VENDOR || hdr->evt == HCI_EV_CMD_COMPLETE) > complete(&qca->drop_ev_comp); > > kfree_skb(skb); > @@ -1497,6 +1505,9 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) > > switch (soc_type) { > case QCA_WCN3990: > + case QCA_WCN6750: > + case QCA_WCN6855: > + case QCA_WCN7850: > reinit_completion(&qca->drop_ev_comp); > set_bit(QCA_DROP_VENDOR_EVENT, &qca->flags); > break; > @@ -1531,6 +1542,9 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) > > switch (soc_type) { > case QCA_WCN3990: > + case QCA_WCN6750: > + case QCA_WCN6855: > + case QCA_WCN7850: > /* Wait for the controller to send the vendor event > * for the baudrate change command. > */
Dear Cheng, Thank you for your patch. Am 21.08.24 um 06:16 schrieb Cheng Jiang: > Is there any comment for the changes? Thanks! > > On 7/26/2024 5:58 PM, Cheng Jiang wrote: >> For the WCN6750/WCN6855/WCN7850, the vendor command for a baudrate >> change is not sent as synchronous HCI command, controller sends the >> corresponding vendor event with the new baudrate. It needs to be >> dropped, otherwise it may be misinterpreted as response to a later >> command. Is that documented in some datasheet? How can this behavior be tested, and your change be verified? How did you test it? >> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com> >> --- >> drivers/bluetooth/hci_qca.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index ca6466676902..f497d601e035 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -1206,7 +1206,15 @@ static int qca_recv_event(struct hci_dev *hdev, struct sk_buff *skb) >> * vendor command). >> */ >> >> - if (hdr->evt == HCI_EV_VENDOR) >> + /* For the WCN6750/WCN6855/WCN7850, like the WCN3990, the Does “like the” mean “and” in this situation? WCN3990 is not mentioned in the commit message. >> + * vendor command for a baudrate change command isn't sent as >> + * synchronous HCI command, the controller sends the corresponding >> + * command complete event with the new baudrate. The event is >> + * received and properly decoded after changing the baudrate of >> + * the host port. It needs to be dropped. >> + */ >> + It’d remove the blank line. >> + if (hdr->evt == HCI_EV_VENDOR || hdr->evt == HCI_EV_CMD_COMPLETE) >> complete(&qca->drop_ev_comp); Excuse my ignorance. Is `HCI_EV_CMD_COMPLETE` only sent in case of a baudrate change? >> kfree_skb(skb); >> @@ -1497,6 +1505,9 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) >> >> switch (soc_type) { >> case QCA_WCN3990: >> + case QCA_WCN6750: >> + case QCA_WCN6855: >> + case QCA_WCN7850: So setting the speed wasn’t working before? >> reinit_completion(&qca->drop_ev_comp); >> set_bit(QCA_DROP_VENDOR_EVENT, &qca->flags); >> break; >> @@ -1531,6 +1542,9 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) >> >> switch (soc_type) { >> case QCA_WCN3990: >> + case QCA_WCN6750: >> + case QCA_WCN6855: >> + case QCA_WCN7850: >> /* Wait for the controller to send the vendor event >> * for the baudrate change command. >> */ Kind regards, Paul
Hi Paul, Thank you for the comment. On 8/21/2024 12:49 PM, Paul Menzel wrote: > Dear Cheng, > > > Thank you for your patch. > > Am 21.08.24 um 06:16 schrieb Cheng Jiang: > >> Is there any comment for the changes? Thanks! >> >> On 7/26/2024 5:58 PM, Cheng Jiang wrote: >>> For the WCN6750/WCN6855/WCN7850, the vendor command for a baudrate >>> change is not sent as synchronous HCI command, controller sends the >>> corresponding vendor event with the new baudrate. It needs to be >>> dropped, otherwise it may be misinterpreted as response to a later >>> command. > > Is that documented in some datasheet? It's defined in 80-WL520-14_REV_F_Bluetooth_HCI_Vendor_Specific_Commands_Application_Note.pdf. > > How can this behavior be tested, and your change be verified? How did you test it?
Dear Cheng, Thank you for the detailed response and explanation. It’d be great if you integrated some of the answers into the commit message and resent. Thank you again. Kind regards, Paul
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index ca6466676902..f497d601e035 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -1206,7 +1206,15 @@ static int qca_recv_event(struct hci_dev *hdev, struct sk_buff *skb) * vendor command). */ - if (hdr->evt == HCI_EV_VENDOR) + /* For the WCN6750/WCN6855/WCN7850, like the WCN3990, the + * vendor command for a baudrate change command isn't sent as + * synchronous HCI command, the controller sends the corresponding + * command complete event with the new baudrate. The event is + * received and properly decoded after changing the baudrate of + * the host port. It needs to be dropped. + */ + + if (hdr->evt == HCI_EV_VENDOR || hdr->evt == HCI_EV_CMD_COMPLETE) complete(&qca->drop_ev_comp); kfree_skb(skb); @@ -1497,6 +1505,9 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) switch (soc_type) { case QCA_WCN3990: + case QCA_WCN6750: + case QCA_WCN6855: + case QCA_WCN7850: reinit_completion(&qca->drop_ev_comp); set_bit(QCA_DROP_VENDOR_EVENT, &qca->flags); break; @@ -1531,6 +1542,9 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) switch (soc_type) { case QCA_WCN3990: + case QCA_WCN6750: + case QCA_WCN6855: + case QCA_WCN7850: /* Wait for the controller to send the vendor event * for the baudrate change command. */
For the WCN6750/WCN6855/WCN7850, the vendor command for a baudrate change is not sent as synchronous HCI command, controller sends the corresponding vendor event with the new baudrate. It needs to be dropped, otherwise it may be misinterpreted as response to a later command. Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com> --- drivers/bluetooth/hci_qca.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)