* [PATCH] Bluetooth: Introduce HCI Driver Packet
@ 2025-04-02 16:26 Hsin-chen Chuang
2025-04-02 17:06 ` bluez.test.bot
2025-04-03 20:01 ` [PATCH] " Luiz Augusto von Dentz
0 siblings, 2 replies; 6+ messages in thread
From: Hsin-chen Chuang @ 2025-04-02 16:26 UTC (permalink / raw)
To: luiz.dentz
Cc: Hsin-chen Chuang, chromeos-bluetooth-upstreaming, David S. Miller,
Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann,
Paolo Abeni, Simon Horman, Ying Hsu, linux-bluetooth,
linux-kernel, netdev
From: Hsin-chen Chuang <chharry@chromium.org>
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@chromium.org
Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
---
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;
+ }
+ 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
+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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: Bluetooth: Introduce HCI Driver Packet
2025-04-02 16:26 [PATCH] Bluetooth: Introduce HCI Driver Packet Hsin-chen Chuang
@ 2025-04-02 17:06 ` bluez.test.bot
2025-04-03 20:01 ` [PATCH] " Luiz Augusto von Dentz
1 sibling, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2025-04-02 17:06 UTC (permalink / raw)
To: linux-bluetooth, chharry
[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]
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=949380
---Test result---
Test Summary:
CheckPatch PENDING 0.30 seconds
GitLint PENDING 0.24 seconds
SubjectPrefix PASS 0.17 seconds
BuildKernel PASS 24.34 seconds
CheckAllWarning PASS 26.42 seconds
CheckSparse PASS 29.87 seconds
BuildKernel32 PASS 23.85 seconds
TestRunnerSetup PASS 426.60 seconds
TestRunner_l2cap-tester PASS 20.75 seconds
TestRunner_iso-tester PASS 30.34 seconds
TestRunner_bnep-tester PASS 4.66 seconds
TestRunner_mgmt-tester FAIL 120.68 seconds
TestRunner_rfcomm-tester PASS 7.70 seconds
TestRunner_sco-tester PASS 12.52 seconds
TestRunner_ioctl-tester PASS 8.11 seconds
TestRunner_mesh-tester PASS 6.02 seconds
TestRunner_smp-tester PASS 7.36 seconds
TestRunner_userchan-tester PASS 4.89 seconds
IncrementalBuild PENDING 0.66 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4
Failed Test Cases
LL Privacy - Add Device 3 (AL is full) Failed 0.198 seconds
LL Privacy - Set Flags 2 (Enable RL) Failed 0.141 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: Introduce HCI Driver Packet
2025-04-02 16:26 [PATCH] Bluetooth: Introduce HCI Driver Packet Hsin-chen Chuang
2025-04-02 17:06 ` bluez.test.bot
@ 2025-04-03 20:01 ` Luiz Augusto von Dentz
2025-04-04 0:01 ` Hsin-chen Chuang
1 sibling, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2025-04-03 20:01 UTC (permalink / raw)
To: Hsin-chen Chuang
Cc: Hsin-chen Chuang, chromeos-bluetooth-upstreaming, David S. Miller,
Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann,
Paolo Abeni, Simon Horman, Ying Hsu, linux-bluetooth,
linux-kernel, netdev
Hi Hsin-chen,
On Wed, Apr 2, 2025 at 12:28 PM Hsin-chen Chuang <chharry@google.com> wrote:
>
> From: Hsin-chen Chuang <chharry@chromium.org>
>
> 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@chromium.org
> Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> ---
>
> 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
opcodes, and we can probably add a description to Read Supported
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: Introduce HCI Driver Packet
2025-04-03 20:01 ` [PATCH] " Luiz Augusto von Dentz
@ 2025-04-04 0:01 ` Hsin-chen Chuang
2025-04-09 0:00 ` Hsin-chen Chuang
2025-04-09 14:18 ` Luiz Augusto von Dentz
0 siblings, 2 replies; 6+ messages in thread
From: Hsin-chen Chuang @ 2025-04-04 0:01 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Hsin-chen Chuang, chromeos-bluetooth-upstreaming, David S. Miller,
Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann,
Paolo Abeni, Simon Horman, Ying Hsu, linux-bluetooth,
linux-kernel, netdev
Hi Luiz,
On Fri, Apr 4, 2025 at 4:01 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Hsin-chen,
>
> On Wed, Apr 2, 2025 at 12:28 PM Hsin-chen Chuang <chharry@google.com> wrote:
> >
> > From: Hsin-chen Chuang <chharry@chromium.org>
> >
> > 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@chromium.org
> > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > ---
> >
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: Introduce HCI Driver Packet
2025-04-04 0:01 ` Hsin-chen Chuang
@ 2025-04-09 0:00 ` Hsin-chen Chuang
2025-04-09 14:18 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 6+ messages in thread
From: Hsin-chen Chuang @ 2025-04-09 0:00 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Hsin-chen Chuang, chromeos-bluetooth-upstreaming, David S. Miller,
Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann,
Paolo Abeni, Simon Horman, Ying Hsu, linux-bluetooth,
linux-kernel, netdev
Hi Luiz,
On Fri, Apr 4, 2025 at 8:01 AM Hsin-chen Chuang <chharry@google.com> wrote:
>
> Hi Luiz,
>
> On Fri, Apr 4, 2025 at 4:01 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Wed, Apr 2, 2025 at 12:28 PM Hsin-chen Chuang <chharry@google.com> wrote:
> > >
> > > From: Hsin-chen Chuang <chharry@chromium.org>
> > >
> > > 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@chromium.org
> > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > ---
> > >
> > > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: Introduce HCI Driver Packet
2025-04-04 0:01 ` Hsin-chen Chuang
2025-04-09 0:00 ` Hsin-chen Chuang
@ 2025-04-09 14:18 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2025-04-09 14:18 UTC (permalink / raw)
To: Hsin-chen Chuang
Cc: Hsin-chen Chuang, chromeos-bluetooth-upstreaming, David S. Miller,
Eric Dumazet, Jakub Kicinski, Johan Hedberg, Marcel Holtmann,
Paolo Abeni, Simon Horman, Ying Hsu, linux-bluetooth,
linux-kernel, netdev
Hi Hsin-chen,
On Thu, Apr 3, 2025 at 8:01 PM Hsin-chen Chuang <chharry@google.com> wrote:
>
> Hi Luiz,
>
> On Fri, Apr 4, 2025 at 4:01 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Wed, Apr 2, 2025 at 12:28 PM Hsin-chen Chuang <chharry@google.com> wrote:
> > >
> > > From: Hsin-chen Chuang <chharry@chromium.org>
> > >
> > > 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@chromium.org
> > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > ---
> > >
> > > 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?
Yeah, the Read Supported Driver Commands shall probably be reserved to
0 and perhaps return the driver name as well so the client understand
the command domain is related to the driver.
> 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.
I was thinking more for logging though, anyway we could actually have
it decoded and logged directly by the driver itself e.g. via vendor
diagnostic or user message for example.
> > 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
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-04-09 14:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-02 16:26 [PATCH] Bluetooth: Introduce HCI Driver Packet Hsin-chen Chuang
2025-04-02 17:06 ` bluez.test.bot
2025-04-03 20:01 ` [PATCH] " Luiz Augusto von Dentz
2025-04-04 0:01 ` Hsin-chen Chuang
2025-04-09 0:00 ` Hsin-chen Chuang
2025-04-09 14:18 ` Luiz Augusto von Dentz
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.