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 <tedd.an@xxxxxxxxx>
> ---
> 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
