Hi Pauli,
On Fri, Feb 21, 2025 at 1:51 PM Pauli Virtanen <pav@xxxxxx> 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@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:
>
> 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@xxxxxxxxx>
> > > ---
> > > 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
