Hi Luiz,
On Fri, Apr 4, 2025 at 8:01 AM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> On Fri, Apr 4, 2025 at 4:01 AM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Wed, Apr 2, 2025 at 12:28 PM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
> > >
> > > From: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> > >
> > > Although commit 75ddcd5ad40e ("Bluetooth: btusb: Configure altsetting
> > > for HCI_USER_CHANNEL") has enabled the HCI_USER_CHANNEL user to send out
> > > SCO data through USB Bluetooth chips, it's observed that with the patch
> > > HFP is flaky on most of the existing USB Bluetooth controllers: Intel
> > > chips sometimes send out no packet for Transparent codec; MTK chips may
> > > generate SCO data with a wrong handle for CVSD codec; RTK could split
> > > the data with a wrong packet size for Transparent codec; ... etc.
> > >
> > > To address the issue above one needs to reset the altsetting back to
> > > zero when there is no active SCO connection, which is the same as the
> > > BlueZ behavior, and another benefit is the bus doesn't need to reserve
> > > bandwidth when no SCO connection.
> > >
> > > This patch introduces a fundamental solution that lets the user space
> > > program to configure the altsetting freely:
> > > - Define the new packet type HCI_DRV_PKT which is specifically used for
> > > communication between the user space program and the Bluetooth drviers
> > > - Define the btusb driver command HCI_DRV_OP_SWITCH_ALT_SETTING which
> > > indicates the expected altsetting from the user space program
> > > - btusb intercepts the command and adjusts the Isoc endpoint
> > > correspondingly
> > >
> > > This patch is tested on ChromeOS devices. The USB Bluetooth models
> > > (CVSD, TRANS alt3, and TRANS alt6) could pass the stress HFP test narrow
> > > band speech and wide band speech.
> > >
> > > Cc: chromeos-bluetooth-upstreaming@xxxxxxxxxxxx
> > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > Signed-off-by: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> > > ---
> > >
> > > drivers/bluetooth/btusb.c | 112 ++++++++++++++++++++++++++++++++
> > > drivers/bluetooth/hci_drv_pkt.h | 62 ++++++++++++++++++
> > > include/net/bluetooth/hci.h | 1 +
> > > include/net/bluetooth/hci_mon.h | 2 +
> > > net/bluetooth/hci_core.c | 2 +
> > > net/bluetooth/hci_sock.c | 12 +++-
> > > 6 files changed, 189 insertions(+), 2 deletions(-)
> > > create mode 100644 drivers/bluetooth/hci_drv_pkt.h
> > >
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index 5012b5ff92c8..644a0f13f8ee 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -26,6 +26,7 @@
> > > #include "btbcm.h"
> > > #include "btrtl.h"
> > > #include "btmtk.h"
> > > +#include "hci_drv_pkt.h"
> > >
> > > #define VERSION "0.8"
> > >
> > > @@ -2151,6 +2152,111 @@ static int submit_or_queue_tx_urb(struct hci_dev *hdev, struct urb *urb)
> > > return 0;
> > > }
> > >
> > > +static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts);
> > > +
> > > +static int btusb_drv_process_cmd(struct hci_dev *hdev, struct sk_buff *cmd_skb)
> > > +{
> > > + struct hci_drv_cmd_hdr *hdr;
> > > + u16 opcode, cmd_len;
> > > +
> > > + hdr = skb_pull_data(cmd_skb, sizeof(*hdr));
> > > + if (!hdr)
> > > + return -EILSEQ;
> > > +
> > > + opcode = le16_to_cpu(hdr->opcode);
> > > + cmd_len = le16_to_cpu(hdr->len);
> > > + if (cmd_len != cmd_skb->len)
> > > + return -EILSEQ;
> > > +
> > > + switch (opcode) {
> > > + case HCI_DRV_OP_READ_SUPPORTED_DRIVER_COMMANDS: {
> > > + struct hci_drv_resp_read_supported_driver_commands *resp;
> > > + struct sk_buff *resp_skb;
> > > + struct btusb_data *data = hci_get_drvdata(hdev);
> > > + int ret;
> > > + u16 num_commands = 1; /* SUPPORTED_DRIVER_COMMANDS */
> > > +
> > > + if (data->isoc)
> > > + num_commands++; /* SWITCH_ALT_SETTING */
> > > +
> > > + resp_skb = hci_drv_skb_alloc(
> > > + opcode, sizeof(*resp) + num_commands * sizeof(__le16),
> > > + GFP_KERNEL);
> > > + if (!resp_skb)
> > > + return -ENOMEM;
> > > +
> > > + resp = skb_put(resp_skb,
> > > + sizeof(*resp) + num_commands * sizeof(__le16));
> > > + resp->status = HCI_DRV_STATUS_SUCCESS;
> > > + resp->num_commands = cpu_to_le16(num_commands);
> > > + resp->commands[0] =
> > > + cpu_to_le16(HCI_DRV_OP_READ_SUPPORTED_DRIVER_COMMANDS);
> > > +
> > > + if (data->isoc)
> > > + resp->commands[1] =
> > > + cpu_to_le16(HCI_DRV_OP_SWITCH_ALT_SETTING);
> > > +
> > > + ret = hci_recv_frame(hdev, resp_skb);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + kfree_skb(cmd_skb);
> > > + return 0;
> > > + }
> >
> > If you have to enclose a case with {} then it probably makes more
> > sense to add a dedicated function to do that, that said it would
> > probably be best to add a struct table that can be used to define
> > supported commands. I also recommend splitting the commit adding the
> > command from the introduction of HCI_DRV_PKT.
> >
> > > + case HCI_DRV_OP_SWITCH_ALT_SETTING: {
> > > + struct hci_drv_cmd_switch_alt_setting *cmd;
> > > + struct hci_drv_resp_status *resp;
> > > + struct sk_buff *resp_skb;
> > > + int ret;
> > > + u8 status;
> > > +
> > > + resp_skb = hci_drv_skb_alloc(opcode, sizeof(*resp), GFP_KERNEL);
> > > + if (!resp_skb)
> > > + return -ENOMEM;
> > > +
> > > + cmd = skb_pull_data(cmd_skb, sizeof(*cmd));
> > > + if (!cmd || cmd_skb->len || cmd->new_alt > 6) {
> > > + status = HCI_DRV_STATUS_INVALID_PARAMETERS;
> > > + } else {
> > > + ret = btusb_switch_alt_setting(hdev, cmd->new_alt);
> > > + if (ret)
> > > + status = HCI_DRV_STATUS_UNSPECIFIED_ERROR;
> > > + else
> > > + status = HCI_DRV_STATUS_SUCCESS;
> > > + }
> > > +
> > > + resp = skb_put(resp_skb, sizeof(*resp));
> > > + resp->status = status;
> > > +
> > > + ret = hci_recv_frame(hdev, resp_skb);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + kfree_skb(cmd_skb);
> > > + return 0;
> > > + }
> > > + default: {
> > > + struct hci_drv_resp_status *resp;
> > > + struct sk_buff *resp_skb;
> > > + int ret;
> > > +
> > > + resp_skb = hci_drv_skb_alloc(opcode, sizeof(*resp), GFP_KERNEL);
> > > + if (!resp_skb)
> > > + return -ENOMEM;
> > > +
> > > + resp = skb_put(resp_skb, sizeof(*resp));
> > > + resp->status = HCI_DRV_STATUS_UNKNOWN_COMMAND;
> > > +
> > > + ret = hci_recv_frame(hdev, resp_skb);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + kfree_skb(cmd_skb);
> > > + return 0;
> > > + }
> > > + }
> > > +}
> > > +
> > > static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> > > {
> > > struct urb *urb;
> > > @@ -2192,6 +2298,9 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> > > return PTR_ERR(urb);
> > >
> > > return submit_or_queue_tx_urb(hdev, urb);
> > > +
> > > + case HCI_DRV_PKT:
> > > + return btusb_drv_process_cmd(hdev, skb);
> > > }
> > >
> > > return -EILSEQ;
> > > @@ -2669,6 +2778,9 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > return PTR_ERR(urb);
> > >
> > > return submit_or_queue_tx_urb(hdev, urb);
> > > +
> > > + case HCI_DRV_PKT:
> > > + return btusb_drv_process_cmd(hdev, skb);
> > > }
> > >
> > > return -EILSEQ;
> > > diff --git a/drivers/bluetooth/hci_drv_pkt.h b/drivers/bluetooth/hci_drv_pkt.h
> > > new file mode 100644
> > > index 000000000000..800e0090f816
> > > --- /dev/null
> > > +++ b/drivers/bluetooth/hci_drv_pkt.h
> > > @@ -0,0 +1,62 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (C) 2025 Google Corporation
> > > + */
> > > +
> > > +#include <net/bluetooth/bluetooth.h>
> > > +#include <net/bluetooth/hci.h>
> > > +
> > > +struct hci_drv_cmd_hdr {
> > > + __le16 opcode;
> > > + __le16 len;
> > > +} __packed;
> > > +
> > > +struct hci_drv_resp_hdr {
> > > + __le16 opcode;
> > > + __le16 len;
> > > +} __packed;
> > > +
> > > +struct hci_drv_resp_status {
> > > + __u8 status;
> > > +} __packed;
> > > +
> > > +#define HCI_DRV_STATUS_SUCCESS 0x00
> > > +#define HCI_DRV_STATUS_UNSPECIFIED_ERROR 0x01
> > > +#define HCI_DRV_STATUS_UNKNOWN_COMMAND 0x02
> > > +#define HCI_DRV_STATUS_INVALID_PARAMETERS 0x03
> > > +
> > > +/* Common commands that make sense on all drivers start from 0x0000. */
> > > +
> > > +#define HCI_DRV_OP_READ_SUPPORTED_DRIVER_COMMANDS 0x0000
> > > +struct hci_drv_resp_read_supported_driver_commands {
> > > + __u8 status;
> > > + __le16 num_commands;
> > > + __le16 commands[];
> > > +} __packed;
> > > +
> > > +/* btusb specific commands start from 0x1135.
> > > + * No particular reason - It's my lucky number.
> > > + */
> > > +
> > > +#define HCI_DRV_OP_SWITCH_ALT_SETTING 0x1135
> >
> > Id actually start from 0x00, each driver can have its own command
>
> If each driver can have its own command opcodes, how could the user
> know which one to begin with?
> I think at least the opcode of the Read Supported Driver Commands
> shall be the same across all drivers. And if we do so, don't we
> reserve some space in case there are more commands that need to be
> shared?
>
> We could make a small change here - not btusb specific, but "driver
> specific" - that is, starting from this code the meaning could be
> different on each driver.
>
> > opcodes, and we can probably add a description to Read Supported
>
> Do you mean a human readable description? I doubt that's really useful
> if we have the opcode well defined and by human readable it's hard for
> the user space program to parse.
>
> > Driver Commands in case it is not clear or for decoding purposes, we
> > could also return some driver info so the upper layers know what is
> > the driver.
> >
> > > +struct hci_drv_cmd_switch_alt_setting {
> > > + __u8 new_alt;
> > > +} __packed;
> > > +
> > > +static inline struct sk_buff *hci_drv_skb_alloc(u16 opcode, u16 plen, gfp_t how)
> > > +{
> > > + struct hci_drv_resp_hdr *hdr;
> > > + struct sk_buff *skb;
> > > +
> > > + skb = bt_skb_alloc(sizeof(*hdr) + plen, how);
> > > + if (!skb)
> > > + return NULL;
> > > +
> > > + hdr = skb_put(skb, sizeof(*hdr));
> > > + hdr->opcode = __cpu_to_le16(opcode);
> > > + hdr->len = __cpu_to_le16(plen);
> > > +
> > > + hci_skb_pkt_type(skb) = HCI_DRV_PKT;
> > > +
> > > + return skb;
> > > +}
> > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > index a8586c3058c7..e297b312d2b7 100644
> > > --- a/include/net/bluetooth/hci.h
> > > +++ b/include/net/bluetooth/hci.h
> > > @@ -494,6 +494,7 @@ enum {
> > > #define HCI_EVENT_PKT 0x04
> > > #define HCI_ISODATA_PKT 0x05
> > > #define HCI_DIAG_PKT 0xf0
> > > +#define HCI_DRV_PKT 0xf1
> > > #define HCI_VENDOR_PKT 0xff
> > >
> > > /* HCI packet types */
> > > diff --git a/include/net/bluetooth/hci_mon.h b/include/net/bluetooth/hci_mon.h
> > > index 082f89531b88..bbd752494ef9 100644
> > > --- a/include/net/bluetooth/hci_mon.h
> > > +++ b/include/net/bluetooth/hci_mon.h
> > > @@ -51,6 +51,8 @@ struct hci_mon_hdr {
> > > #define HCI_MON_CTRL_EVENT 17
> > > #define HCI_MON_ISO_TX_PKT 18
> > > #define HCI_MON_ISO_RX_PKT 19
> > > +#define HCI_MON_DRV_TX_PKT 20
> > > +#define HCI_MON_DRV_RX_PKT 21
> > >
> > > struct hci_mon_new_index {
> > > __u8 type;
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index 5eb0600bbd03..bb4e1721edc2 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -2911,6 +2911,8 @@ int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
> > > break;
> > > case HCI_ISODATA_PKT:
> > > break;
> > > + case HCI_DRV_PKT:
> > > + break;
> > > default:
> > > kfree_skb(skb);
> > > return -EINVAL;
> > > diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> > > index 022b86797acd..428ee5c7de7e 100644
> > > --- a/net/bluetooth/hci_sock.c
> > > +++ b/net/bluetooth/hci_sock.c
> > > @@ -234,7 +234,8 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
> > > if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT &&
> > > hci_skb_pkt_type(skb) != HCI_ACLDATA_PKT &&
> > > hci_skb_pkt_type(skb) != HCI_SCODATA_PKT &&
> > > - hci_skb_pkt_type(skb) != HCI_ISODATA_PKT)
> > > + hci_skb_pkt_type(skb) != HCI_ISODATA_PKT &&
> > > + hci_skb_pkt_type(skb) != HCI_DRV_PKT)
> > > continue;
> > > } else {
> > > /* Don't send frame to other channel types */
> > > @@ -391,6 +392,12 @@ void hci_send_to_monitor(struct hci_dev *hdev, struct sk_buff *skb)
> > > else
> > > opcode = cpu_to_le16(HCI_MON_ISO_TX_PKT);
> > > break;
> > > + case HCI_DRV_PKT:
> > > + if (bt_cb(skb)->incoming)
> > > + opcode = cpu_to_le16(HCI_MON_DRV_RX_PKT);
> > > + else
> > > + opcode = cpu_to_le16(HCI_MON_DRV_TX_PKT);
> > > + break;
> > > case HCI_DIAG_PKT:
> > > opcode = cpu_to_le16(HCI_MON_VENDOR_DIAG);
> > > break;
> > > @@ -1860,7 +1867,8 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> > > if (hci_skb_pkt_type(skb) != HCI_COMMAND_PKT &&
> > > hci_skb_pkt_type(skb) != HCI_ACLDATA_PKT &&
> > > hci_skb_pkt_type(skb) != HCI_SCODATA_PKT &&
> > > - hci_skb_pkt_type(skb) != HCI_ISODATA_PKT) {
> > > + hci_skb_pkt_type(skb) != HCI_ISODATA_PKT &&
> > > + hci_skb_pkt_type(skb) != HCI_DRV_PKT) {
> > > err = -EINVAL;
> > > goto drop;
> > > }
> > > --
> > > 2.49.0.504.g3bcea36a83-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
Please kindly let me know what you think, thanks!
Best Regards,
Hsin-chen
