From: Tedd Ho-Jeong An <[email protected]>
This patch adds new opcode(0x03) for HCI Vendor packet to support
creating extended device mode. In order to avoid the conflict with the
legacy opcode, it has to be 0x03 only and all other bits must be set to
zero.
Then, it is followed by the extended configuration data that contains
the device type and the flags to be used.
Signed-off-by: Tedd Ho-Jeong An <[email protected]>
---
drivers/bluetooth/hci_vhci.c | 200 ++++++++++++++++++++++++++++-------
1 file changed, 162 insertions(+), 38 deletions(-)
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 49ac884d996e..4c0cfb29c0e8 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -30,6 +30,24 @@
static bool amp;
+/* This is the struct for extended device configuration.
+ * The opcode 0x03 is used for creating an extended device and followed by
+ * the configuration data below.
+ * dev_type is Primay or AMP.
+ * flag_len is the length of flag array
+ * flag array contains the flag to use/set while creating the device.
+ */
+struct vhci_ext_config {
+ __u8 dev_type;
+ __u8 flag_len;
+ __u8 flags[0];
+};
+
+#define VHCI_EXT_FLAG_ENABLE_AOSP 0x01
+#define VHCI_EXT_FLAG_QUIRK_RAW_DEVICE 0x02
+#define VHCI_EXT_FLAG_QUIARK_EXTERNAL_CONFIG 0x03
+#define VHCI_EXT_FLAG_QUIRK_INVALID_BDADDR 0x04
+
struct vhci_data {
struct hci_dev *hdev;
@@ -278,11 +296,52 @@ static int vhci_setup(struct hci_dev *hdev)
return 0;
}
+static int vhci_register_hdev(struct hci_dev *hdev, __u8 opcode)
+{
+ struct vhci_data *data = hci_get_drvdata(hdev);
+ struct sk_buff *skb;
+
+ skb = bt_skb_alloc(4, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;
+
+ if (hci_register_dev(hdev) < 0) {
+ BT_ERR("Can't register HCI device");
+ kfree_skb(skb);
+ return -EBUSY;
+ }
+
+ debugfs_create_file("force_suspend", 0644, hdev->debugfs, data,
+ &force_suspend_fops);
+
+ debugfs_create_file("force_wakeup", 0644, hdev->debugfs, data,
+ &force_wakeup_fops);
+
+ if (IS_ENABLED(CONFIG_BT_MSFTEXT))
+ debugfs_create_file("msft_opcode", 0644, hdev->debugfs, data,
+ &msft_opcode_fops);
+
+ if (IS_ENABLED(CONFIG_BT_AOSPEXT))
+ debugfs_create_file("aosp_capable", 0644, hdev->debugfs, data,
+ &aosp_capable_fops);
+
+ hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
+
+ skb_put_u8(skb, 0xff);
+ skb_put_u8(skb, opcode);
+ put_unaligned_le16(hdev->id, skb_put(skb, 2));
+ skb_queue_tail(&data->readq, skb);
+
+ wake_up_interruptible(&data->read_wait);
+
+ return 0;
+}
+
static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
{
struct hci_dev *hdev;
- struct sk_buff *skb;
__u8 dev_type;
+ int ret;
if (data->hdev)
return -EBADFD;
@@ -297,15 +356,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
if (opcode & 0x3c)
return -EINVAL;
- skb = bt_skb_alloc(4, GFP_KERNEL);
- if (!skb)
- return -ENOMEM;
-
hdev = hci_alloc_dev();
- if (!hdev) {
- kfree_skb(skb);
+ if (!hdev)
return -ENOMEM;
- }
data->hdev = hdev;
@@ -331,45 +384,108 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
if (opcode & 0x80)
set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
- if (hci_register_dev(hdev) < 0) {
- BT_ERR("Can't register HCI device");
+ /* Legacy method returns opcode instead of dev type */
+ ret = vhci_register_hdev(hdev, opcode);
+ if (ret < 0) {
hci_free_dev(hdev);
data->hdev = NULL;
- kfree_skb(skb);
- return -EBUSY;
}
- debugfs_create_file("force_suspend", 0644, hdev->debugfs, data,
- &force_suspend_fops);
+ return ret;
+}
- debugfs_create_file("force_wakeup", 0644, hdev->debugfs, data,
- &force_wakeup_fops);
+static int vhci_create_device(struct vhci_data *data, __u8 opcode)
+{
+ int err;
- if (IS_ENABLED(CONFIG_BT_MSFTEXT))
- debugfs_create_file("msft_opcode", 0644, hdev->debugfs, data,
- &msft_opcode_fops);
+ mutex_lock(&data->open_mutex);
+ err = __vhci_create_device(data, opcode);
+ mutex_unlock(&data->open_mutex);
- if (IS_ENABLED(CONFIG_BT_AOSPEXT))
- debugfs_create_file("aosp_capable", 0644, hdev->debugfs, data,
- &aosp_capable_fops);
+ return err;
+}
- hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
+static int __vhci_create_extended_device(struct vhci_data *data,
+ struct sk_buff *skb)
+{
+ struct hci_dev *hdev;
+ struct vhci_ext_config *config;
+ int i, ret;
+ __u8 flag;
- skb_put_u8(skb, 0xff);
- skb_put_u8(skb, opcode);
- put_unaligned_le16(hdev->id, skb_put(skb, 2));
- skb_queue_tail(&data->readq, skb);
+ if (data->hdev)
+ return -EBADFD;
- wake_up_interruptible(&data->read_wait);
- return 0;
+ /* Make sure the skb has a minimum valid length */
+ if (skb->len < sizeof(*config))
+ return -EINVAL;
+
+ config = (void *)(skb->data);
+ if (skb->len < sizeof(*config) + config->flag_len)
+ return -EINVAL;
+
+ if (config->dev_type != HCI_PRIMARY && config->dev_type != HCI_AMP)
+ return -EINVAL;
+
+ hdev = hci_alloc_dev();
+ if (!hdev)
+ return -ENOMEM;
+
+ data->hdev = hdev;
+
+ hdev->bus = HCI_VIRTUAL;
+ hdev->dev_type = config->dev_type;
+ hci_set_drvdata(hdev, data);
+
+ hdev->open = vhci_open_dev;
+ hdev->close = vhci_close_dev;
+ hdev->flush = vhci_flush;
+ hdev->send = vhci_send_frame;
+ hdev->get_data_path_id = vhci_get_data_path_id;
+ hdev->get_codec_config_data = vhci_get_codec_config_data;
+ hdev->wakeup = vhci_wakeup;
+ hdev->setup = vhci_setup;
+ set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+
+ for (i = 0; i < config->flag_len; i++) {
+ flag = config->flags[i];
+ switch (flag) {
+ case VHCI_EXT_FLAG_ENABLE_AOSP:
+ data->aosp_capable = 1;
+ break;
+ case VHCI_EXT_FLAG_QUIRK_RAW_DEVICE:
+ set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
+ break;
+ case VHCI_EXT_FLAG_QUIARK_EXTERNAL_CONFIG:
+ set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks);
+ break;
+ case VHCI_EXT_FLAG_QUIRK_INVALID_BDADDR:
+ set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
+ break;
+ default:
+ BT_ERR("Invalid flag");
+ hci_free_dev(hdev);
+ data->hdev = NULL;
+ return -EINVAL;
+ }
+ }
+
+ /* Extended method returns the fixed extension opcode 0x03 */
+ ret = vhci_register_hdev(hdev, 0x03);
+ if (ret < 0) {
+ hci_free_dev(hdev);
+ data->hdev = NULL;
+ }
+
+ return ret;
}
-static int vhci_create_device(struct vhci_data *data, __u8 opcode)
+static int vhci_create_extended_device(struct vhci_data *data,
+ struct sk_buff *skb)
{
int err;
-
mutex_lock(&data->open_mutex);
- err = __vhci_create_device(data, opcode);
+ err = __vhci_create_extended_device(data, skb);
mutex_unlock(&data->open_mutex);
return err;
@@ -419,14 +535,22 @@ static inline ssize_t vhci_get_user(struct vhci_data *data,
opcode = *((__u8 *) skb->data);
skb_pull(skb, 1);
- if (skb->len > 0) {
- kfree_skb(skb);
- return -EINVAL;
+ /* The dev_type 3 is used as an escape opcode for extension
+ * handling. If dev_type is set to 3 all other bits must be
+ * set to zero.
+ */
+ if (opcode == 0x03) {
+ if (skb->len < 1)
+ ret = -EINVAL;
+ else
+ ret = vhci_create_extended_device(data, skb);
+ } else {
+ if (skb->len > 0)
+ ret = -EINVAL;
+ else
+ ret = vhci_create_device(data, opcode);
}
-
kfree_skb(skb);
-
- ret = vhci_create_device(data, opcode);
break;
default:
--
2.25.1
Hi Tedd,
> This patch adds new opcode(0x03) for HCI Vendor packet to support
> creating extended device mode. In order to avoid the conflict with the
> legacy opcode, it has to be 0x03 only and all other bits must be set to
> zero.
>
> Then, it is followed by the extended configuration data that contains
> the device type and the flags to be used.
>
> Signed-off-by: Tedd Ho-Jeong An <[email protected]>
> ---
> drivers/bluetooth/hci_vhci.c | 200 ++++++++++++++++++++++++++++-------
> 1 file changed, 162 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
> index 49ac884d996e..4c0cfb29c0e8 100644
> --- a/drivers/bluetooth/hci_vhci.c
> +++ b/drivers/bluetooth/hci_vhci.c
> @@ -30,6 +30,24 @@
>
> static bool amp;
>
> +/* This is the struct for extended device configuration.
> + * The opcode 0x03 is used for creating an extended device and followed by
> + * the configuration data below.
> + * dev_type is Primay or AMP.
> + * flag_len is the length of flag array
> + * flag array contains the flag to use/set while creating the device.
> + */
> +struct vhci_ext_config {
> + __u8 dev_type;
> + __u8 flag_len;
> + __u8 flags[0];
> +};
> +
> +#define VHCI_EXT_FLAG_ENABLE_AOSP 0x01
> +#define VHCI_EXT_FLAG_QUIRK_RAW_DEVICE 0x02
> +#define VHCI_EXT_FLAG_QUIARK_EXTERNAL_CONFIG 0x03
> +#define VHCI_EXT_FLAG_QUIRK_INVALID_BDADDR 0x04
QUIARK ;)
> +
> struct vhci_data {
> struct hci_dev *hdev;
>
> @@ -278,11 +296,52 @@ static int vhci_setup(struct hci_dev *hdev)
> return 0;
> }
>
> +static int vhci_register_hdev(struct hci_dev *hdev, __u8 opcode)
> +{
> + struct vhci_data *data = hci_get_drvdata(hdev);
> + struct sk_buff *skb;
> +
> + skb = bt_skb_alloc(4, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + if (hci_register_dev(hdev) < 0) {
> + BT_ERR("Can't register HCI device");
> + kfree_skb(skb);
> + return -EBUSY;
> + }
> +
> + debugfs_create_file("force_suspend", 0644, hdev->debugfs, data,
> + &force_suspend_fops);
> +
> + debugfs_create_file("force_wakeup", 0644, hdev->debugfs, data,
> + &force_wakeup_fops);
> +
> + if (IS_ENABLED(CONFIG_BT_MSFTEXT))
> + debugfs_create_file("msft_opcode", 0644, hdev->debugfs, data,
> + &msft_opcode_fops);
> +
> + if (IS_ENABLED(CONFIG_BT_AOSPEXT))
> + debugfs_create_file("aosp_capable", 0644, hdev->debugfs, data,
> + &aosp_capable_fops);
> +
> + hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
> +
> + skb_put_u8(skb, 0xff);
> + skb_put_u8(skb, opcode);
> + put_unaligned_le16(hdev->id, skb_put(skb, 2));
> + skb_queue_tail(&data->readq, skb);
> +
> + wake_up_interruptible(&data->read_wait);
> +
> + return 0;
> +}
> +
I don’t think it is the best idea to generalize this. I have the feeling we need to discuss the return packet and format as well. So in the initial patch, I would not much mess with existing code. We can optimize that once we have a good handle on this. Especially since this makes the review more complicated.
That said, I was also keeping in mind something Luiz and I discussed a while back that we might want to use the 0xff channel (or some other defined channel) for extra protocols running between hci_vhci and btvirt.
> static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> {
> struct hci_dev *hdev;
> - struct sk_buff *skb;
> __u8 dev_type;
> + int ret;
>
> if (data->hdev)
> return -EBADFD;
> @@ -297,15 +356,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> if (opcode & 0x3c)
> return -EINVAL;
>
> - skb = bt_skb_alloc(4, GFP_KERNEL);
> - if (!skb)
> - return -ENOMEM;
> -
> hdev = hci_alloc_dev();
> - if (!hdev) {
> - kfree_skb(skb);
> + if (!hdev)
> return -ENOMEM;
> - }
>
> data->hdev = hdev;
>
> @@ -331,45 +384,108 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> if (opcode & 0x80)
> set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
>
> - if (hci_register_dev(hdev) < 0) {
> - BT_ERR("Can't register HCI device");
> + /* Legacy method returns opcode instead of dev type */
> + ret = vhci_register_hdev(hdev, opcode);
> + if (ret < 0) {
> hci_free_dev(hdev);
> data->hdev = NULL;
> - kfree_skb(skb);
> - return -EBUSY;
> }
>
> - debugfs_create_file("force_suspend", 0644, hdev->debugfs, data,
> - &force_suspend_fops);
> + return ret;
> +}
>
> - debugfs_create_file("force_wakeup", 0644, hdev->debugfs, data,
> - &force_wakeup_fops);
> +static int vhci_create_device(struct vhci_data *data, __u8 opcode)
> +{
> + int err;
>
> - if (IS_ENABLED(CONFIG_BT_MSFTEXT))
> - debugfs_create_file("msft_opcode", 0644, hdev->debugfs, data,
> - &msft_opcode_fops);
> + mutex_lock(&data->open_mutex);
> + err = __vhci_create_device(data, opcode);
> + mutex_unlock(&data->open_mutex);
>
> - if (IS_ENABLED(CONFIG_BT_AOSPEXT))
> - debugfs_create_file("aosp_capable", 0644, hdev->debugfs, data,
> - &aosp_capable_fops);
> + return err;
> +}
>
> - hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
> +static int __vhci_create_extended_device(struct vhci_data *data,
> + struct sk_buff *skb)
> +{
> + struct hci_dev *hdev;
> + struct vhci_ext_config *config;
> + int i, ret;
> + __u8 flag;
>
> - skb_put_u8(skb, 0xff);
> - skb_put_u8(skb, opcode);
> - put_unaligned_le16(hdev->id, skb_put(skb, 2));
> - skb_queue_tail(&data->readq, skb);
> + if (data->hdev)
> + return -EBADFD;
>
> - wake_up_interruptible(&data->read_wait);
> - return 0;
> + /* Make sure the skb has a minimum valid length */
> + if (skb->len < sizeof(*config))
> + return -EINVAL;
> +
> + config = (void *)(skb->data);
> + if (skb->len < sizeof(*config) + config->flag_len)
> + return -EINVAL;
> +
> + if (config->dev_type != HCI_PRIMARY && config->dev_type != HCI_AMP)
> + return -EINVAL;
> +
> + hdev = hci_alloc_dev();
> + if (!hdev)
> + return -ENOMEM;
> +
> + data->hdev = hdev;
> +
> + hdev->bus = HCI_VIRTUAL;
> + hdev->dev_type = config->dev_type;
> + hci_set_drvdata(hdev, data);
> +
> + hdev->open = vhci_open_dev;
> + hdev->close = vhci_close_dev;
> + hdev->flush = vhci_flush;
> + hdev->send = vhci_send_frame;
> + hdev->get_data_path_id = vhci_get_data_path_id;
> + hdev->get_codec_config_data = vhci_get_codec_config_data;
> + hdev->wakeup = vhci_wakeup;
> + hdev->setup = vhci_setup;
> + set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> +
> + for (i = 0; i < config->flag_len; i++) {
> + flag = config->flags[i];
> + switch (flag) {
> + case VHCI_EXT_FLAG_ENABLE_AOSP:
> + data->aosp_capable = 1;
> + break;
> + case VHCI_EXT_FLAG_QUIRK_RAW_DEVICE:
> + set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
> + break;
> + case VHCI_EXT_FLAG_QUIARK_EXTERNAL_CONFIG:
> + set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks);
> + break;
> + case VHCI_EXT_FLAG_QUIRK_INVALID_BDADDR:
> + set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
> + break;
> + default:
> + BT_ERR("Invalid flag");
> + hci_free_dev(hdev);
> + data->hdev = NULL;
> + return -EINVAL;
> + }
> + }
So this part, I think you misunderstood me.
struct virtio_bt_config {
__u8 type;
__u16 vendor;
__u16 msft_opcode;
} __attribute__((packed));
This part above is a flexible struct. I can be extended over time. However the validity of fields are defined by flags.
/* Feature bits */
#define VIRTIO_BT_F_VND_HCI 0 /* Indicates vendor command support */
#define VIRTIO_BT_F_MSFT_EXT 1 /* Indicates MSFT vendor support */
#define VIRTIO_BT_F_AOSP_EXT 2 /* Indicates AOSP vendor support */
These feature bits need to have its space. If we want more features, then we should add them here and also enable in virtio_bt.
With that in mind, scrap EXTERNAL_CONFIG and RAW_DEVICE since that you can do via existing legacy mode. And also they make no real sense. The raw device part is really legacy that I rather completely remove. We have User Channel these days and that is a lot better. The external config is something we haven’t used at all and so keep it in the legacy realm.
For the invalid bd_addr quirk, I need to consider if it is better to say 00:00:.. address is concluded as invalid or we allow providing a magic invalid address to match against or just a flag as above. The problem is also that we need to define the vendor opcode that is used to set the public address. Otherwise such a flag is useless.
Regards
Marcel