Hi,
On Tue, Mar 4, 2025 at 1:10 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> include/net/bluetooth/hci.h | 6 ++++
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 54 +++++++++++++-------------------
> net/bluetooth/hci_event.c | 5 +++
> net/bluetooth/hci_sync.c | 18 +++++++++++
> 5 files changed, 52 insertions(+), 32 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index b99818df8ee7..70169533c940 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -448,6 +448,7 @@ enum {
> HCI_WIDEBAND_SPEECH_ENABLED,
> HCI_EVENT_FILTER_CONFIGURED,
> HCI_PA_SYNC,
> + HCI_SCO_FLOWCTL,
>
> HCI_DUT_MODE,
> HCI_VENDOR_DIAG,
> @@ -1544,6 +1545,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 2f320eeddfec..cf18cf65fe5e 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..9ea9b4091f8f 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3548,45 +3548,35 @@ static void __check_timeout(struct hci_dev *hdev, unsigned int cnt, u8 type)
> }
>
> /* Schedule SCO */
> -static void hci_sched_sco(struct hci_dev *hdev)
> +static void hci_sched_sco_type(struct hci_dev *hdev, __u8 type)
> {
> struct hci_conn *conn;
> struct sk_buff *skb;
> int quote;
> + unsigned int pkts;
>
> - BT_DBG("%s", hdev->name);
> + bt_dev_dbg(hdev, "type %u", type);
>
> - if (!hci_conn_num(hdev, SCO_LINK))
> + if (!hci_conn_num(hdev, type))
> return;
>
> - while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, "e))) {
> + /* Use sco_pkts if flow control has not been enabled yet which will
> + * limit the amount of buffer sent in a row.
> + */
> + if (!hci_dev_test_flag(hdev, HCI_SCO_FLOWCTL))
> + pkts = hdev->sco_pkts;
> + else
> + pkts = hdev->sco_cnt;
> +
> + while (pkts && (conn = hci_low_sent(hdev, type, "e))) {
> while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> BT_DBG("skb %p len %d", skb, skb->len);
> hci_send_frame(hdev, skb);
>
> - conn->sent++;
> - if (conn->sent == ~0)
> - conn->sent = 0;
> - }
> - }
> -}
> + pkts--;
>
> -static void hci_sched_esco(struct hci_dev *hdev)
> -{
> - struct hci_conn *conn;
> - struct sk_buff *skb;
> - int quote;
> -
> - BT_DBG("%s", hdev->name);
> -
> - if (!hci_conn_num(hdev, ESCO_LINK))
> - return;
> -
> - while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK,
> - "e))) {
> - while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> - BT_DBG("skb %p len %d", skb, skb->len);
> - hci_send_frame(hdev, skb);
> + if (hdev->sco_cnt > 0)
> + hdev->sco_cnt--;
>
> conn->sent++;
> if (conn->sent == ~0)
> @@ -3628,8 +3618,8 @@ static void hci_sched_acl_pkt(struct hci_dev *hdev)
> chan->conn->sent++;
>
> /* Send pending SCO packets right away */
> - hci_sched_sco(hdev);
> - hci_sched_esco(hdev);
> + hci_sched_sco_type(hdev, SCO_LINK);
> + hci_sched_sco_type(hdev, ESCO_LINK);
> }
> }
>
> @@ -3684,8 +3674,8 @@ static void hci_sched_le(struct hci_dev *hdev)
> chan->conn->sent++;
>
> /* Send pending SCO packets right away */
> - hci_sched_sco(hdev);
> - hci_sched_esco(hdev);
> + hci_sched_sco_type(hdev, SCO_LINK);
> + hci_sched_sco_type(hdev, ESCO_LINK);
> }
> }
>
> @@ -3730,8 +3720,8 @@ static void hci_tx_work(struct work_struct *work)
>
> if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> /* Schedule queues and send stuff to HCI driver */
> - hci_sched_sco(hdev);
> - hci_sched_esco(hdev);
> + hci_sched_sco_type(hdev, SCO_LINK);
> + hci_sched_sco_type(hdev, ESCO_LINK);
> hci_sched_iso(hdev);
> hci_sched_acl(hdev);
> hci_sched_le(hdev);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 19e19c9f5e68..e6eadcc13484 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4448,6 +4448,11 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
> hdev->sco_cnt += count;
> if (hdev->sco_cnt > hdev->sco_pkts)
> hdev->sco_cnt = hdev->sco_pkts;
> +
> + /* Enable HCI_SCO_FLOWCTL so sco_cnt is used instead of
> + * sco_pkts.
> + */
> + hci_dev_set_flag(hdev, HCI_SCO_FLOWCTL);
I may spin another version implement this logic but before triggering
hci_connect_cfm do send a empty packet just to test if flowctl is
really enabled, this maybe better than always setting the flag above
since in that case the socket interface is not involved so we can
guarantee that only one packet is outstanding.
> break;
>
> case ISO_LINK:
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index c4c2cf51b219..91cfb7c2cb08 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3769,6 +3769,22 @@ 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;
> +
> + /* 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;
> +
> + return __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SYNC_FLOWCTL,
> + sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> +}
> +
> /* BR Controller init stage 2 command sequence */
> static const struct hci_init_stage br_init2[] = {
> /* HCI_OP_READ_BUFFER_SIZE */
> @@ -3787,6 +3803,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
>
--
Luiz Augusto von Dentz
