* [PATCH v2] Bluetooth: hci_core: Enable buffer flow control for SCO/eSCO
@ 2025-02-20 21:57 Luiz Augusto von Dentz
2025-02-20 21:59 ` Luiz Augusto von Dentz
2025-02-20 22:15 ` [v2] " bluez.test.bot
0 siblings, 2 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2025-02-20 21:57 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This enables buffer flow control for SCO/eSCO
(see: Bluetooth Core 6.0 spec: 6.22. Synchronous Flow Control Enable),
recently this has caused the following problem and is actually a nice
addition for the likes of Socket TX complete:
< HCI Command: Read Buffer Size (0x04|0x0005) plen 0
> HCI Event: Command Complete (0x0e) plen 11
Read Buffer Size (0x04|0x0005) ncmd 1
Status: Success (0x00)
ACL MTU: 1021 ACL max packet: 5
SCO MTU: 240 SCO max packet: 8
...
< SCO Data TX: Handle 257 flags 0x00 dlen 120
< SCO Data TX: Handle 257 flags 0x00 dlen 120
< SCO Data TX: Handle 257 flags 0x00 dlen 120
< SCO Data TX: Handle 257 flags 0x00 dlen 120
< SCO Data TX: Handle 257 flags 0x00 dlen 120
< SCO Data TX: Handle 257 flags 0x00 dlen 120
< SCO Data TX: Handle 257 flags 0x00 dlen 120
< SCO Data TX: Handle 257 flags 0x00 dlen 120
< SCO Data TX: Handle 257 flags 0x00 dlen 120
> HCI Event: Hardware Error (0x10) plen 1
Code: 0x0a
Fixes: 7fedd3bb6b77 ("Bluetooth: Prioritize SCO traffic")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/hci.h | 6 ++++++
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 15 +++++++++++++++
net/bluetooth/hci_sync.c | 23 +++++++++++++++++++++++
4 files changed, 45 insertions(+)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 0d51970d809f..a6a375cf97f9 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -432,6 +432,7 @@ enum {
HCI_WIDEBAND_SPEECH_ENABLED,
HCI_EVENT_FILTER_CONFIGURED,
HCI_PA_SYNC,
+ HCI_SCO_FLOWCTL,
HCI_DUT_MODE,
HCI_VENDOR_DIAG,
@@ -1528,6 +1529,11 @@ struct hci_rp_read_tx_power {
__s8 tx_power;
} __packed;
+#define HCI_OP_WRITE_SYNC_FLOWCTL 0x0c2f
+struct hci_cp_write_sync_flowctl {
+ __u8 enable;
+} __packed;
+
#define HCI_OP_READ_PAGE_SCAN_TYPE 0x0c46
struct hci_rp_read_page_scan_type {
__u8 status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f756fac95488..6abe1115fa92 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1857,6 +1857,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
#define lmp_hold_capable(dev) ((dev)->features[0][0] & LMP_HOLD)
#define lmp_sniff_capable(dev) ((dev)->features[0][0] & LMP_SNIFF)
#define lmp_park_capable(dev) ((dev)->features[0][1] & LMP_PARK)
+#define lmp_sco_capable(dev) ((dev)->features[0][1] & LMP_SCO)
#define lmp_inq_rssi_capable(dev) ((dev)->features[0][3] & LMP_RSSI_INQ)
#define lmp_esco_capable(dev) ((dev)->features[0][3] & LMP_ESCO)
#define lmp_bredr_capable(dev) (!((dev)->features[0][4] & LMP_NO_BREDR))
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e7ec12437c8b..a0a14fb8a496 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3564,11 +3564,19 @@ static void hci_sched_sco(struct hci_dev *hdev)
BT_DBG("skb %p len %d", skb, skb->len);
hci_send_frame(hdev, skb);
+
+ hdev->sco_cnt--;
conn->sent++;
if (conn->sent == ~0)
conn->sent = 0;
}
}
+
+ /* Restore sco_cnt if flow control has not been enabled as
+ * HCI_EV_NUM_COMP_PKTS won't be generated.
+ */
+ if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
+ hdev->sco_cnt = hdev->sco_pkts;
}
static void hci_sched_esco(struct hci_dev *hdev)
@@ -3588,11 +3596,18 @@ static void hci_sched_esco(struct hci_dev *hdev)
BT_DBG("skb %p len %d", skb, skb->len);
hci_send_frame(hdev, skb);
+ hdev->sco_cnt--;
conn->sent++;
if (conn->sent == ~0)
conn->sent = 0;
}
}
+
+ /* Restore sco_cnt if flow control has not been enabled as
+ * HCI_EV_NUM_COMP_PKTS won't be generated.
+ */
+ if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
+ hdev->sco_cnt = hdev->sco_pkts;
}
static void hci_sched_acl_pkt(struct hci_dev *hdev)
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index dd770ef5ec36..42ee9c5e11ad 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -3766,6 +3766,27 @@ static int hci_write_ca_timeout_sync(struct hci_dev *hdev)
sizeof(param), ¶m, HCI_CMD_TIMEOUT);
}
+/* Enable SCO flow control if supported */
+static int hci_write_sync_flowctl_sync(struct hci_dev *hdev)
+{
+ struct hci_cp_write_sync_flowctl cp;
+ int err;
+
+ /* Check if the controller supports SCO and HCI_OP_WRITE_SYNC_FLOWCTL */
+ if (!lmp_sco_capable(hdev) || !(hdev->commands[10] & BIT(4)))
+ return 0;
+
+ memset(&cp, 0, sizeof(cp));
+ cp.enable = 0x01;
+
+ err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SYNC_FLOWCTL,
+ sizeof(cp), &cp, HCI_CMD_TIMEOUT);
+ if (!err)
+ hci_dev_set_flag(hdev, HCI_SCO_FLOWCTL);
+
+ return err;
+}
+
/* BR Controller init stage 2 command sequence */
static const struct hci_init_stage br_init2[] = {
/* HCI_OP_READ_BUFFER_SIZE */
@@ -3784,6 +3805,8 @@ static const struct hci_init_stage br_init2[] = {
HCI_INIT(hci_clear_event_filter_sync),
/* HCI_OP_WRITE_CA_TIMEOUT */
HCI_INIT(hci_write_ca_timeout_sync),
+ /* HCI_OP_WRITE_SYNC_FLOWCTL */
+ HCI_INIT(hci_write_sync_flowctl_sync),
{}
};
--
2.48.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Bluetooth: hci_core: Enable buffer flow control for SCO/eSCO
2025-02-20 21:57 [PATCH v2] Bluetooth: hci_core: Enable buffer flow control for SCO/eSCO Luiz Augusto von Dentz
@ 2025-02-20 21:59 ` Luiz Augusto von Dentz
2025-02-21 18:51 ` Pauli Virtanen
2025-02-20 22:15 ` [v2] " bluez.test.bot
1 sibling, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2025-02-20 21:59 UTC (permalink / raw)
To: linux-bluetooth, Pauli Virtanen
Hi Pauli,
On Thu, Feb 20, 2025 at 4:57 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This enables buffer flow control for SCO/eSCO
> (see: Bluetooth Core 6.0 spec: 6.22. Synchronous Flow Control Enable),
> recently this has caused the following problem and is actually a nice
> addition for the likes of Socket TX complete:
>
> < HCI Command: Read Buffer Size (0x04|0x0005) plen 0
> > HCI Event: Command Complete (0x0e) plen 11
> Read Buffer Size (0x04|0x0005) ncmd 1
> Status: Success (0x00)
> ACL MTU: 1021 ACL max packet: 5
> SCO MTU: 240 SCO max packet: 8
> ...
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > HCI Event: Hardware Error (0x10) plen 1
> Code: 0x0a
>
> Fixes: 7fedd3bb6b77 ("Bluetooth: Prioritize SCO traffic")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci.h | 6 ++++++
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 15 +++++++++++++++
> net/bluetooth/hci_sync.c | 23 +++++++++++++++++++++++
> 4 files changed, 45 insertions(+)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 0d51970d809f..a6a375cf97f9 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -432,6 +432,7 @@ enum {
> HCI_WIDEBAND_SPEECH_ENABLED,
> HCI_EVENT_FILTER_CONFIGURED,
> HCI_PA_SYNC,
> + HCI_SCO_FLOWCTL,
>
> HCI_DUT_MODE,
> HCI_VENDOR_DIAG,
> @@ -1528,6 +1529,11 @@ struct hci_rp_read_tx_power {
> __s8 tx_power;
> } __packed;
>
> +#define HCI_OP_WRITE_SYNC_FLOWCTL 0x0c2f
> +struct hci_cp_write_sync_flowctl {
> + __u8 enable;
> +} __packed;
> +
> #define HCI_OP_READ_PAGE_SCAN_TYPE 0x0c46
> struct hci_rp_read_page_scan_type {
> __u8 status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index f756fac95488..6abe1115fa92 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1857,6 +1857,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> #define lmp_hold_capable(dev) ((dev)->features[0][0] & LMP_HOLD)
> #define lmp_sniff_capable(dev) ((dev)->features[0][0] & LMP_SNIFF)
> #define lmp_park_capable(dev) ((dev)->features[0][1] & LMP_PARK)
> +#define lmp_sco_capable(dev) ((dev)->features[0][1] & LMP_SCO)
> #define lmp_inq_rssi_capable(dev) ((dev)->features[0][3] & LMP_RSSI_INQ)
> #define lmp_esco_capable(dev) ((dev)->features[0][3] & LMP_ESCO)
> #define lmp_bredr_capable(dev) (!((dev)->features[0][4] & LMP_NO_BREDR))
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index e7ec12437c8b..a0a14fb8a496 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3564,11 +3564,19 @@ static void hci_sched_sco(struct hci_dev *hdev)
> BT_DBG("skb %p len %d", skb, skb->len);
> hci_send_frame(hdev, skb);
>
> +
> + hdev->sco_cnt--;
> conn->sent++;
> if (conn->sent == ~0)
> conn->sent = 0;
> }
> }
> +
> + /* Restore sco_cnt if flow control has not been enabled as
> + * HCI_EV_NUM_COMP_PKTS won't be generated.
> + */
> + if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
> + hdev->sco_cnt = hdev->sco_pkts;
> }
>
> static void hci_sched_esco(struct hci_dev *hdev)
> @@ -3588,11 +3596,18 @@ static void hci_sched_esco(struct hci_dev *hdev)
> BT_DBG("skb %p len %d", skb, skb->len);
> hci_send_frame(hdev, skb);
>
> + hdev->sco_cnt--;
> conn->sent++;
> if (conn->sent == ~0)
> conn->sent = 0;
> }
> }
> +
> + /* Restore sco_cnt if flow control has not been enabled as
> + * HCI_EV_NUM_COMP_PKTS won't be generated.
> + */
> + if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
> + hdev->sco_cnt = hdev->sco_pkts;
> }
>
> static void hci_sched_acl_pkt(struct hci_dev *hdev)
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index dd770ef5ec36..42ee9c5e11ad 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3766,6 +3766,27 @@ static int hci_write_ca_timeout_sync(struct hci_dev *hdev)
> sizeof(param), ¶m, HCI_CMD_TIMEOUT);
> }
>
> +/* Enable SCO flow control if supported */
> +static int hci_write_sync_flowctl_sync(struct hci_dev *hdev)
> +{
> + struct hci_cp_write_sync_flowctl cp;
> + int err;
> +
> + /* Check if the controller supports SCO and HCI_OP_WRITE_SYNC_FLOWCTL */
> + if (!lmp_sco_capable(hdev) || !(hdev->commands[10] & BIT(4)))
> + return 0;
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.enable = 0x01;
> +
> + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SYNC_FLOWCTL,
> + sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> + if (!err)
> + hci_dev_set_flag(hdev, HCI_SCO_FLOWCTL);
> +
> + return err;
> +}
> +
> /* BR Controller init stage 2 command sequence */
> static const struct hci_init_stage br_init2[] = {
> /* HCI_OP_READ_BUFFER_SIZE */
> @@ -3784,6 +3805,8 @@ static const struct hci_init_stage br_init2[] = {
> HCI_INIT(hci_clear_event_filter_sync),
> /* HCI_OP_WRITE_CA_TIMEOUT */
> HCI_INIT(hci_write_ca_timeout_sync),
> + /* HCI_OP_WRITE_SYNC_FLOWCTL */
> + HCI_INIT(hci_write_sync_flowctl_sync),
> {}
> };
>
> --
> 2.48.1
>
Let me know if this addresses your concerns, I figure this was why we
could not track packet completion of SCO/eSCO so I guess it is a plus
if we can finally enable it across all sockets.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [v2] Bluetooth: hci_core: Enable buffer flow control for SCO/eSCO
2025-02-20 21:57 [PATCH v2] Bluetooth: hci_core: Enable buffer flow control for SCO/eSCO Luiz Augusto von Dentz
2025-02-20 21:59 ` Luiz Augusto von Dentz
@ 2025-02-20 22:15 ` bluez.test.bot
1 sibling, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2025-02-20 22:15 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 551 bytes --]
This is an automated email and please do not reply to this email.
Dear Submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.
----- Output -----
error: patch failed: net/bluetooth/hci_core.c:3564
error: net/bluetooth/hci_core.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Please resolve the issue and submit the patches again.
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Bluetooth: hci_core: Enable buffer flow control for SCO/eSCO
2025-02-20 21:59 ` Luiz Augusto von Dentz
@ 2025-02-21 18:51 ` Pauli Virtanen
2025-02-21 20:28 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 5+ messages in thread
From: Pauli Virtanen @ 2025-02-21 18:51 UTC (permalink / raw)
To: Luiz Augusto von Dentz, linux-bluetooth
Hi,
to, 2025-02-20 kello 16:59 -0500, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>
> On Thu, Feb 20, 2025 at 4:57 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This enables buffer flow control for SCO/eSCO
> > (see: Bluetooth Core 6.0 spec: 6.22. Synchronous Flow Control Enable),
> > recently this has caused the following problem and is actually a nice
> > addition for the likes of Socket TX complete:
I tested this on Intel AX210 (current firmware) -- it doesn't seem to
work:
< HCI Command: Write Sync Flow Control Enable (0x03|0x002f) plen 1 #43 [hci0] 7.611473
Flow control: Enabled (0x01)
> HCI Event: Command Complete (0x0e) plen 4 #44 [hci0] 7.612831
Write Sync Flow Control Enable (0x03|0x002f) ncmd 1
Status: Success (0x00)
...
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #494 [hci0] 21.120883
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #495 [hci0] 21.120883
wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #496 [hci0] 21.131190
wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #497 [hci0] 21.131272
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #498 [hci0] 21.132282
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #499 [hci0] 21.132287
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #500 [hci0] 21.132288
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #501 [hci0] 21.132288
wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #502 [hci0] 21.137072
wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #503 [hci0] 21.137144
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #504 [hci0] 21.140898
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #505 [hci0] 21.140903
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #506 [hci0] 21.140903
wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #507 [hci0] 21.142969
wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #508 [hci0] 21.143108
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #509 [hci0] 21.150848
> SCO Data RX: Handle 257 flags 0x00 dlen 48 #510 [hci0] 21.150854
No Number of Completed Packets are generated for SCO Data TX, so it
gets stuck here and no sound output.
Same situation with Realtek 8761BU.
Maybe that hardware does not seem to be implementing this as required
in spec, is the reason why flow control for SCO was not enabled?
> >
> > < HCI Command: Read Buffer Size (0x04|0x0005) plen 0
> > > HCI Event: Command Complete (0x0e) plen 11
> > Read Buffer Size (0x04|0x0005) ncmd 1
> > Status: Success (0x00)
> > ACL MTU: 1021 ACL max packet: 5
> > SCO MTU: 240 SCO max packet: 8
> > ...
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > HCI Event: Hardware Error (0x10) plen 1
> > Code: 0x0a
> >
> > Fixes: 7fedd3bb6b77 ("Bluetooth: Prioritize SCO traffic")
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/hci.h | 6 ++++++
> > include/net/bluetooth/hci_core.h | 1 +
> > net/bluetooth/hci_core.c | 15 +++++++++++++++
> > net/bluetooth/hci_sync.c | 23 +++++++++++++++++++++++
> > 4 files changed, 45 insertions(+)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 0d51970d809f..a6a375cf97f9 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -432,6 +432,7 @@ enum {
> > HCI_WIDEBAND_SPEECH_ENABLED,
> > HCI_EVENT_FILTER_CONFIGURED,
> > HCI_PA_SYNC,
> > + HCI_SCO_FLOWCTL,
> >
> > HCI_DUT_MODE,
> > HCI_VENDOR_DIAG,
> > @@ -1528,6 +1529,11 @@ struct hci_rp_read_tx_power {
> > __s8 tx_power;
> > } __packed;
> >
> > +#define HCI_OP_WRITE_SYNC_FLOWCTL 0x0c2f
> > +struct hci_cp_write_sync_flowctl {
> > + __u8 enable;
> > +} __packed;
> > +
> > #define HCI_OP_READ_PAGE_SCAN_TYPE 0x0c46
> > struct hci_rp_read_page_scan_type {
> > __u8 status;
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index f756fac95488..6abe1115fa92 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1857,6 +1857,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> > #define lmp_hold_capable(dev) ((dev)->features[0][0] & LMP_HOLD)
> > #define lmp_sniff_capable(dev) ((dev)->features[0][0] & LMP_SNIFF)
> > #define lmp_park_capable(dev) ((dev)->features[0][1] & LMP_PARK)
> > +#define lmp_sco_capable(dev) ((dev)->features[0][1] & LMP_SCO)
> > #define lmp_inq_rssi_capable(dev) ((dev)->features[0][3] & LMP_RSSI_INQ)
> > #define lmp_esco_capable(dev) ((dev)->features[0][3] & LMP_ESCO)
> > #define lmp_bredr_capable(dev) (!((dev)->features[0][4] & LMP_NO_BREDR))
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index e7ec12437c8b..a0a14fb8a496 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -3564,11 +3564,19 @@ static void hci_sched_sco(struct hci_dev *hdev)
> > BT_DBG("skb %p len %d", skb, skb->len);
> > hci_send_frame(hdev, skb);
> >
> > +
> > + hdev->sco_cnt--;
> > conn->sent++;
> > if (conn->sent == ~0)
> > conn->sent = 0;
> > }
> > }
> > +
> > + /* Restore sco_cnt if flow control has not been enabled as
> > + * HCI_EV_NUM_COMP_PKTS won't be generated.
> > + */
> > + if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
I think this check is inverted.
> > + hdev->sco_cnt = hdev->sco_pkts;
If there are unsent SCO skbs in the queue, I think they are not
guaranteed to be sent here.
hdev->tx_work would need to be queued for this to run again. With
FLOWCTL this is guaranteed by hci_num_comp_pkts_evt(), but without it's
not.
Maybe without flowctl this should use the old behavior, like:
if (!hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
hdev->sco_cnt = hdev->sco_pkts;
while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK,
...
if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
hdev->sco_cnt--;
> > }
> >
> > static void hci_sched_esco(struct hci_dev *hdev)
> > @@ -3588,11 +3596,18 @@ static void hci_sched_esco(struct hci_dev *hdev)
> > BT_DBG("skb %p len %d", skb, skb->len);
> > hci_send_frame(hdev, skb);
> >
> > + hdev->sco_cnt--;
> > conn->sent++;
> > if (conn->sent == ~0)
> > conn->sent = 0;
> > }
> > }
> > +
> > + /* Restore sco_cnt if flow control has not been enabled as
> > + * HCI_EV_NUM_COMP_PKTS won't be generated.
> > + */
> > + if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
> > + hdev->sco_cnt = hdev->sco_pkts;
> > }
> >
> > static void hci_sched_acl_pkt(struct hci_dev *hdev)
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index dd770ef5ec36..42ee9c5e11ad 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -3766,6 +3766,27 @@ static int hci_write_ca_timeout_sync(struct hci_dev *hdev)
> > sizeof(param), ¶m, HCI_CMD_TIMEOUT);
> > }
> >
> > +/* Enable SCO flow control if supported */
> > +static int hci_write_sync_flowctl_sync(struct hci_dev *hdev)
> > +{
> > + struct hci_cp_write_sync_flowctl cp;
> > + int err;
> > +
> > + /* Check if the controller supports SCO and HCI_OP_WRITE_SYNC_FLOWCTL */
> > + if (!lmp_sco_capable(hdev) || !(hdev->commands[10] & BIT(4)))
> > + return 0;
> > +
> > + memset(&cp, 0, sizeof(cp));
> > + cp.enable = 0x01;
> > +
> > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SYNC_FLOWCTL,
> > + sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> > + if (!err)
> > + hci_dev_set_flag(hdev, HCI_SCO_FLOWCTL);
> > +
> > + return err;
> > +}
> > +
> > /* BR Controller init stage 2 command sequence */
> > static const struct hci_init_stage br_init2[] = {
> > /* HCI_OP_READ_BUFFER_SIZE */
> > @@ -3784,6 +3805,8 @@ static const struct hci_init_stage br_init2[] = {
> > HCI_INIT(hci_clear_event_filter_sync),
> > /* HCI_OP_WRITE_CA_TIMEOUT */
> > HCI_INIT(hci_write_ca_timeout_sync),
> > + /* HCI_OP_WRITE_SYNC_FLOWCTL */
> > + HCI_INIT(hci_write_sync_flowctl_sync),
> > {}
> > };
> >
> > --
> > 2.48.1
> >
>
> Let me know if this addresses your concerns, I figure this was why we
> could not track packet completion of SCO/eSCO so I guess it is a plus
> if we can finally enable it across all sockets.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Bluetooth: hci_core: Enable buffer flow control for SCO/eSCO
2025-02-21 18:51 ` Pauli Virtanen
@ 2025-02-21 20:28 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2025-02-21 20:28 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: linux-bluetooth
Hi Pauli,
On Fri, Feb 21, 2025 at 1:51 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi,
>
> to, 2025-02-20 kello 16:59 -0500, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Thu, Feb 20, 2025 at 4:57 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > >
> > > This enables buffer flow control for SCO/eSCO
> > > (see: Bluetooth Core 6.0 spec: 6.22. Synchronous Flow Control Enable),
> > > recently this has caused the following problem and is actually a nice
> > > addition for the likes of Socket TX complete:
>
> I tested this on Intel AX210 (current firmware) -- it doesn't seem to
> work:
>
> < HCI Command: Write Sync Flow Control Enable (0x03|0x002f) plen 1 #43 [hci0] 7.611473
> Flow control: Enabled (0x01)
> > HCI Event: Command Complete (0x0e) plen 4 #44 [hci0] 7.612831
> Write Sync Flow Control Enable (0x03|0x002f) ncmd 1
> Status: Success (0x00)
Ouch, so it claims the command is supported, return success but then
don't generate the event anyway, hmm sounds broken.
> ...
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #494 [hci0] 21.120883
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #495 [hci0] 21.120883
> wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #496 [hci0] 21.131190
> wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #497 [hci0] 21.131272
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #498 [hci0] 21.132282
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #499 [hci0] 21.132287
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #500 [hci0] 21.132288
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #501 [hci0] 21.132288
> wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #502 [hci0] 21.137072
> wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #503 [hci0] 21.137144
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #504 [hci0] 21.140898
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #505 [hci0] 21.140903
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #506 [hci0] 21.140903
> wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #507 [hci0] 21.142969
> wireplumber[1533]: < SCO Data TX: Handle 257 flags 0x00 dlen 48 #508 [hci0] 21.143108
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #509 [hci0] 21.150848
> > SCO Data RX: Handle 257 flags 0x00 dlen 48 #510 [hci0] 21.150854
>
> No Number of Completed Packets are generated for SCO Data TX, so it
> gets stuck here and no sound output.
>
> Same situation with Realtek 8761BU.
>
> Maybe that hardware does not seem to be implementing this as required
> in spec, is the reason why flow control for SCO was not enabled?
Yeah, no wonder it wasn't implemented for this long, which is really
too bad since then there is no flow control and no way to do TX
complete either.
> > >
> > > < HCI Command: Read Buffer Size (0x04|0x0005) plen 0
> > > > HCI Event: Command Complete (0x0e) plen 11
> > > Read Buffer Size (0x04|0x0005) ncmd 1
> > > Status: Success (0x00)
> > > ACL MTU: 1021 ACL max packet: 5
> > > SCO MTU: 240 SCO max packet: 8
> > > ...
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > < SCO Data TX: Handle 257 flags 0x00 dlen 120
> > > > HCI Event: Hardware Error (0x10) plen 1
> > > Code: 0x0a
> > >
> > > Fixes: 7fedd3bb6b77 ("Bluetooth: Prioritize SCO traffic")
> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > ---
> > > include/net/bluetooth/hci.h | 6 ++++++
> > > include/net/bluetooth/hci_core.h | 1 +
> > > net/bluetooth/hci_core.c | 15 +++++++++++++++
> > > net/bluetooth/hci_sync.c | 23 +++++++++++++++++++++++
> > > 4 files changed, 45 insertions(+)
> > >
> > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > index 0d51970d809f..a6a375cf97f9 100644
> > > --- a/include/net/bluetooth/hci.h
> > > +++ b/include/net/bluetooth/hci.h
> > > @@ -432,6 +432,7 @@ enum {
> > > HCI_WIDEBAND_SPEECH_ENABLED,
> > > HCI_EVENT_FILTER_CONFIGURED,
> > > HCI_PA_SYNC,
> > > + HCI_SCO_FLOWCTL,
> > >
> > > HCI_DUT_MODE,
> > > HCI_VENDOR_DIAG,
> > > @@ -1528,6 +1529,11 @@ struct hci_rp_read_tx_power {
> > > __s8 tx_power;
> > > } __packed;
> > >
> > > +#define HCI_OP_WRITE_SYNC_FLOWCTL 0x0c2f
> > > +struct hci_cp_write_sync_flowctl {
> > > + __u8 enable;
> > > +} __packed;
> > > +
> > > #define HCI_OP_READ_PAGE_SCAN_TYPE 0x0c46
> > > struct hci_rp_read_page_scan_type {
> > > __u8 status;
> > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > index f756fac95488..6abe1115fa92 100644
> > > --- a/include/net/bluetooth/hci_core.h
> > > +++ b/include/net/bluetooth/hci_core.h
> > > @@ -1857,6 +1857,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> > > #define lmp_hold_capable(dev) ((dev)->features[0][0] & LMP_HOLD)
> > > #define lmp_sniff_capable(dev) ((dev)->features[0][0] & LMP_SNIFF)
> > > #define lmp_park_capable(dev) ((dev)->features[0][1] & LMP_PARK)
> > > +#define lmp_sco_capable(dev) ((dev)->features[0][1] & LMP_SCO)
> > > #define lmp_inq_rssi_capable(dev) ((dev)->features[0][3] & LMP_RSSI_INQ)
> > > #define lmp_esco_capable(dev) ((dev)->features[0][3] & LMP_ESCO)
> > > #define lmp_bredr_capable(dev) (!((dev)->features[0][4] & LMP_NO_BREDR))
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index e7ec12437c8b..a0a14fb8a496 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -3564,11 +3564,19 @@ static void hci_sched_sco(struct hci_dev *hdev)
> > > BT_DBG("skb %p len %d", skb, skb->len);
> > > hci_send_frame(hdev, skb);
> > >
> > > +
> > > + hdev->sco_cnt--;
> > > conn->sent++;
> > > if (conn->sent == ~0)
> > > conn->sent = 0;
> > > }
> > > }
> > > +
> > > + /* Restore sco_cnt if flow control has not been enabled as
> > > + * HCI_EV_NUM_COMP_PKTS won't be generated.
> > > + */
> > > + if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
>
> I think this check is inverted.
Yeah, I will fix it.
> > > + hdev->sco_cnt = hdev->sco_pkts;
>
> If there are unsent SCO skbs in the queue, I think they are not
> guaranteed to be sent here.
>
> hdev->tx_work would need to be queued for this to run again. With
> FLOWCTL this is guaranteed by hci_num_comp_pkts_evt(), but without it's
> not.
>
> Maybe without flowctl this should use the old behavior, like:
>
> if (!hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
> hdev->sco_cnt = hdev->sco_pkts;
I left at the end we don't try to send more than sco_cnt in a row, but
you are probably right that if there are still packets enqueued
without the hci_num_comp_pkts_evt to submit the tx_work won't resume
so we might need to trigger it tx_work to resume in that case.
> while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK,
> ...
> if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
> hdev->sco_cnt--;
>
> > > }
> > >
> > > static void hci_sched_esco(struct hci_dev *hdev)
> > > @@ -3588,11 +3596,18 @@ static void hci_sched_esco(struct hci_dev *hdev)
> > > BT_DBG("skb %p len %d", skb, skb->len);
> > > hci_send_frame(hdev, skb);
> > >
> > > + hdev->sco_cnt--;
> > > conn->sent++;
> > > if (conn->sent == ~0)
> > > conn->sent = 0;
> > > }
> > > }
> > > +
> > > + /* Restore sco_cnt if flow control has not been enabled as
> > > + * HCI_EV_NUM_COMP_PKTS won't be generated.
> > > + */
> > > + if (hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
> > > + hdev->sco_cnt = hdev->sco_pkts;
> > > }
> > >
> > > static void hci_sched_acl_pkt(struct hci_dev *hdev)
> > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > > index dd770ef5ec36..42ee9c5e11ad 100644
> > > --- a/net/bluetooth/hci_sync.c
> > > +++ b/net/bluetooth/hci_sync.c
> > > @@ -3766,6 +3766,27 @@ static int hci_write_ca_timeout_sync(struct hci_dev *hdev)
> > > sizeof(param), ¶m, HCI_CMD_TIMEOUT);
> > > }
> > >
> > > +/* Enable SCO flow control if supported */
> > > +static int hci_write_sync_flowctl_sync(struct hci_dev *hdev)
> > > +{
> > > + struct hci_cp_write_sync_flowctl cp;
> > > + int err;
> > > +
> > > + /* Check if the controller supports SCO and HCI_OP_WRITE_SYNC_FLOWCTL */
> > > + if (!lmp_sco_capable(hdev) || !(hdev->commands[10] & BIT(4)))
> > > + return 0;
> > > +
> > > + memset(&cp, 0, sizeof(cp));
> > > + cp.enable = 0x01;
> > > +
> > > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SYNC_FLOWCTL,
> > > + sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> > > + if (!err)
> > > + hci_dev_set_flag(hdev, HCI_SCO_FLOWCTL);
> > > +
> > > + return err;
> > > +}
> > > +
> > > /* BR Controller init stage 2 command sequence */
> > > static const struct hci_init_stage br_init2[] = {
> > > /* HCI_OP_READ_BUFFER_SIZE */
> > > @@ -3784,6 +3805,8 @@ static const struct hci_init_stage br_init2[] = {
> > > HCI_INIT(hci_clear_event_filter_sync),
> > > /* HCI_OP_WRITE_CA_TIMEOUT */
> > > HCI_INIT(hci_write_ca_timeout_sync),
> > > + /* HCI_OP_WRITE_SYNC_FLOWCTL */
> > > + HCI_INIT(hci_write_sync_flowctl_sync),
> > > {}
> > > };
> > >
> > > --
> > > 2.48.1
> > >
> >
> > Let me know if this addresses your concerns, I figure this was why we
> > could not track packet completion of SCO/eSCO so I guess it is a plus
> > if we can finally enable it across all sockets.
> >
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-02-21 20:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-20 21:57 [PATCH v2] Bluetooth: hci_core: Enable buffer flow control for SCO/eSCO Luiz Augusto von Dentz
2025-02-20 21:59 ` Luiz Augusto von Dentz
2025-02-21 18:51 ` Pauli Virtanen
2025-02-21 20:28 ` Luiz Augusto von Dentz
2025-02-20 22:15 ` [v2] " bluez.test.bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.