* [RFC PATCH V2] Bluetooth: vhci: Add support extended opcode for HCI Vendor packet @ 2021-11-02 2:56 Tedd Ho-Jeong An 2021-11-02 3:01 ` An, Tedd 2021-11-02 8:31 ` Marcel Holtmann 0 siblings, 2 replies; 4+ messages in thread From: Tedd Ho-Jeong An @ 2021-11-02 2:56 UTC (permalink / raw) To: linux-bluetooth; +Cc: Tedd Ho-Jeong An From: Tedd Ho-Jeong An <tedd.an@intel.com> Current implementation uses the Vendor packet type (0xff) with opcode parameter. But there is no way to expand the opcode and no available bits to use. Also it cannot be changed due to the backward compatibility with older kernel. THis patch adds new opcode(0x03) for HCI Vendor packet for VHCI for extended device creation. This new opcode will not conflict with existing legacy opcode because the legacy expects to set either bit 0 or bit 1, but not both of bits. It aslo requires new extra parameters for device type and flags to apply to the VHCI device. Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com> --- drivers/bluetooth/hci_vhci.c | 65 ++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 7 deletions(-) diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c index 49ac884d996e..5fccab136543 100644 --- a/drivers/bluetooth/hci_vhci.c +++ b/drivers/bluetooth/hci_vhci.c @@ -30,6 +30,16 @@ static bool amp; +#define VHCI_EXT_OPCODE 0x03 +struct vhci_ext_config { + __u8 dev_type; + __u32 flags; +} __packed; + +#define VHCI_FLAG_QUIRK_RAW_DEVICE 0x01 +#define VHCI_FLAG_QUIRK_EXTERNAL_CONFIG 0x02 +#define VHCI_FLAG_QUIRKS_INVALID_BDADDR 0x04 + struct vhci_data { struct hci_dev *hdev; @@ -278,7 +288,8 @@ static int vhci_setup(struct hci_dev *hdev) return 0; } -static int __vhci_create_device(struct vhci_data *data, __u8 opcode) +static int __vhci_create_device(struct vhci_data *data, __u8 opcode, + struct vhci_ext_config *ext_config) { struct hci_dev *hdev; struct sk_buff *skb; @@ -287,8 +298,20 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) if (data->hdev) return -EBADFD; - /* bits 0-1 are dev_type (Primary or AMP) */ - dev_type = opcode & 0x03; + /* In case of legacy opcode, it doesn't allow to have 0x03 as an opcode, + * So, it is ok to assume that device is in the extended device + * creation mode when the opcode is 0x03. Also, it is required to have + * a ext_config and check it here. + */ + if (ext_config && opcode != VHCI_EXT_OPCODE) + return -EINVAL; + + if (opcode == VHCI_EXT_OPCODE) + dev_type = ext_config->dev_type; + else { + /* bits 0-1 are dev_type (Primary or AMP) */ + dev_type = opcode & 0x03; + } if (dev_type != HCI_PRIMARY && dev_type != HCI_AMP) return -EINVAL; @@ -331,6 +354,16 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) if (opcode & 0x80) set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); + /* Flags for extended configuration */ + if (ext_config && opcode == VHCI_EXT_OPCODE) { + if (ext_config->flags & VHCI_FLAG_QUIRK_EXTERNAL_CONFIG) + set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks); + if (ext_config->flags & VHCI_FLAG_QUIRK_RAW_DEVICE) + set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); + if (ext_config->flags & VHCI_FLAG_QUIRKS_INVALID_BDADDR) + set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); + } + if (hci_register_dev(hdev) < 0) { BT_ERR("Can't register HCI device"); hci_free_dev(hdev); @@ -364,12 +397,13 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) return 0; } -static int vhci_create_device(struct vhci_data *data, __u8 opcode) +static int vhci_create_device(struct vhci_data *data, __u8 opcode, + struct vhci_ext_config *ext_config) { int err; mutex_lock(&data->open_mutex); - err = __vhci_create_device(data, opcode); + err = __vhci_create_device(data, opcode, ext_config); mutex_unlock(&data->open_mutex); return err; @@ -379,6 +413,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, struct iov_iter *from) { size_t len = iov_iter_count(from); + struct vhci_ext_config *ext_config; struct sk_buff *skb; __u8 pkt_type, opcode; int ret; @@ -419,6 +454,21 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, opcode = *((__u8 *) skb->data); skb_pull(skb, 1); + /* This opcode(0x03) is for extended device creation and it + * requires the extra parameters for extra configuration. + */ + if (opcode == 0x03) { + if (skb->len != sizeof(*ext_config)) { + kfree_skb(skb); + return -EINVAL; + } + + ext_config = (void *) (skb->data); + ret = vhci_create_device(data, opcode, ext_config); + kfree_skb(skb); + goto done; + } + if (skb->len > 0) { kfree_skb(skb); return -EINVAL; @@ -426,7 +476,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, kfree_skb(skb); - ret = vhci_create_device(data, opcode); + ret = vhci_create_device(data, opcode, NULL); break; default: @@ -434,6 +484,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, return -EINVAL; } +done: return (ret < 0) ? ret : len; } @@ -526,7 +577,7 @@ static void vhci_open_timeout(struct work_struct *work) struct vhci_data *data = container_of(work, struct vhci_data, open_timeout.work); - vhci_create_device(data, amp ? HCI_AMP : HCI_PRIMARY); + vhci_create_device(data, amp ? HCI_AMP : HCI_PRIMARY, NULL); } static int vhci_open(struct inode *inode, struct file *file) -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH V2] Bluetooth: vhci: Add support extended opcode for HCI Vendor packet 2021-11-02 2:56 [RFC PATCH V2] Bluetooth: vhci: Add support extended opcode for HCI Vendor packet Tedd Ho-Jeong An @ 2021-11-02 3:01 ` An, Tedd 2021-11-02 8:31 ` Marcel Holtmann 1 sibling, 0 replies; 4+ messages in thread From: An, Tedd @ 2021-11-02 3:01 UTC (permalink / raw) To: Tedd Ho-Jeong An, Marcel Holtmann, Von Dentz, Luiz; +Cc: linux-bluetooth Hi Marcel and Luiz I will send the userspace change(btvir, vhci, and etc) in a day or so. It takes more time than I initially thought. Regards Tedd On 11/1/21, 7:56 PM, "Tedd Ho-Jeong An" <hj.tedd.an@gmail.com> wrote: From: Tedd Ho-Jeong An <tedd.an@intel.com> Current implementation uses the Vendor packet type (0xff) with opcode parameter. But there is no way to expand the opcode and no available bits to use. Also it cannot be changed due to the backward compatibility with older kernel. THis patch adds new opcode(0x03) for HCI Vendor packet for VHCI for extended device creation. This new opcode will not conflict with existing legacy opcode because the legacy expects to set either bit 0 or bit 1, but not both of bits. It aslo requires new extra parameters for device type and flags to apply to the VHCI device. Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com> --- drivers/bluetooth/hci_vhci.c | 65 ++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 7 deletions(-) diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c index 49ac884d996e..5fccab136543 100644 --- a/drivers/bluetooth/hci_vhci.c +++ b/drivers/bluetooth/hci_vhci.c @@ -30,6 +30,16 @@ static bool amp; +#define VHCI_EXT_OPCODE 0x03 +struct vhci_ext_config { + __u8 dev_type; + __u32 flags; +} __packed; + +#define VHCI_FLAG_QUIRK_RAW_DEVICE 0x01 +#define VHCI_FLAG_QUIRK_EXTERNAL_CONFIG 0x02 +#define VHCI_FLAG_QUIRKS_INVALID_BDADDR 0x04 + struct vhci_data { struct hci_dev *hdev; @@ -278,7 +288,8 @@ static int vhci_setup(struct hci_dev *hdev) return 0; } -static int __vhci_create_device(struct vhci_data *data, __u8 opcode) +static int __vhci_create_device(struct vhci_data *data, __u8 opcode, + struct vhci_ext_config *ext_config) { struct hci_dev *hdev; struct sk_buff *skb; @@ -287,8 +298,20 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) if (data->hdev) return -EBADFD; - /* bits 0-1 are dev_type (Primary or AMP) */ - dev_type = opcode & 0x03; + /* In case of legacy opcode, it doesn't allow to have 0x03 as an opcode, + * So, it is ok to assume that device is in the extended device + * creation mode when the opcode is 0x03. Also, it is required to have + * a ext_config and check it here. + */ + if (ext_config && opcode != VHCI_EXT_OPCODE) + return -EINVAL; + + if (opcode == VHCI_EXT_OPCODE) + dev_type = ext_config->dev_type; + else { + /* bits 0-1 are dev_type (Primary or AMP) */ + dev_type = opcode & 0x03; + } if (dev_type != HCI_PRIMARY && dev_type != HCI_AMP) return -EINVAL; @@ -331,6 +354,16 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) if (opcode & 0x80) set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); + /* Flags for extended configuration */ + if (ext_config && opcode == VHCI_EXT_OPCODE) { + if (ext_config->flags & VHCI_FLAG_QUIRK_EXTERNAL_CONFIG) + set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks); + if (ext_config->flags & VHCI_FLAG_QUIRK_RAW_DEVICE) + set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); + if (ext_config->flags & VHCI_FLAG_QUIRKS_INVALID_BDADDR) + set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); + } + if (hci_register_dev(hdev) < 0) { BT_ERR("Can't register HCI device"); hci_free_dev(hdev); @@ -364,12 +397,13 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) return 0; } -static int vhci_create_device(struct vhci_data *data, __u8 opcode) +static int vhci_create_device(struct vhci_data *data, __u8 opcode, + struct vhci_ext_config *ext_config) { int err; mutex_lock(&data->open_mutex); - err = __vhci_create_device(data, opcode); + err = __vhci_create_device(data, opcode, ext_config); mutex_unlock(&data->open_mutex); return err; @@ -379,6 +413,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, struct iov_iter *from) { size_t len = iov_iter_count(from); + struct vhci_ext_config *ext_config; struct sk_buff *skb; __u8 pkt_type, opcode; int ret; @@ -419,6 +454,21 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, opcode = *((__u8 *) skb->data); skb_pull(skb, 1); + /* This opcode(0x03) is for extended device creation and it + * requires the extra parameters for extra configuration. + */ + if (opcode == 0x03) { + if (skb->len != sizeof(*ext_config)) { + kfree_skb(skb); + return -EINVAL; + } + + ext_config = (void *) (skb->data); + ret = vhci_create_device(data, opcode, ext_config); + kfree_skb(skb); + goto done; + } + if (skb->len > 0) { kfree_skb(skb); return -EINVAL; @@ -426,7 +476,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, kfree_skb(skb); - ret = vhci_create_device(data, opcode); + ret = vhci_create_device(data, opcode, NULL); break; default: @@ -434,6 +484,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, return -EINVAL; } +done: return (ret < 0) ? ret : len; } @@ -526,7 +577,7 @@ static void vhci_open_timeout(struct work_struct *work) struct vhci_data *data = container_of(work, struct vhci_data, open_timeout.work); - vhci_create_device(data, amp ? HCI_AMP : HCI_PRIMARY); + vhci_create_device(data, amp ? HCI_AMP : HCI_PRIMARY, NULL); } static int vhci_open(struct inode *inode, struct file *file) -- 2.25.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH V2] Bluetooth: vhci: Add support extended opcode for HCI Vendor packet 2021-11-02 2:56 [RFC PATCH V2] Bluetooth: vhci: Add support extended opcode for HCI Vendor packet Tedd Ho-Jeong An 2021-11-02 3:01 ` An, Tedd @ 2021-11-02 8:31 ` Marcel Holtmann 2021-11-02 21:44 ` An, Tedd 1 sibling, 1 reply; 4+ messages in thread From: Marcel Holtmann @ 2021-11-02 8:31 UTC (permalink / raw) To: Tedd Ho-Jeong An; +Cc: linux-bluetooth, Tedd Ho-Jeong An Hi Tedd, > Current implementation uses the Vendor packet type (0xff) with opcode > parameter. But there is no way to expand the opcode and no available bits > to use. Also it cannot be changed due to the backward compatibility > with older kernel. > > THis patch adds new opcode(0x03) for HCI Vendor packet for VHCI for > extended device creation. This new opcode will not conflict with > existing legacy opcode because the legacy expects to set either bit 0 or > bit 1, but not both of bits. > > It aslo requires new extra parameters for device type and flags to apply > to the VHCI device. > > Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com> > --- > drivers/bluetooth/hci_vhci.c | 65 ++++++++++++++++++++++++++++++++---- > 1 file changed, 58 insertions(+), 7 deletions(-) > > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c > index 49ac884d996e..5fccab136543 100644 > --- a/drivers/bluetooth/hci_vhci.c > +++ b/drivers/bluetooth/hci_vhci.c > @@ -30,6 +30,16 @@ > > static bool amp; > > +#define VHCI_EXT_OPCODE 0x03 > +struct vhci_ext_config { > + __u8 dev_type; > + __u32 flags; > +} __packed; > + > +#define VHCI_FLAG_QUIRK_RAW_DEVICE 0x01 > +#define VHCI_FLAG_QUIRK_EXTERNAL_CONFIG 0x02 > +#define VHCI_FLAG_QUIRKS_INVALID_BDADDR 0x04 > + > struct vhci_data { > struct hci_dev *hdev; > > @@ -278,7 +288,8 @@ static int vhci_setup(struct hci_dev *hdev) > return 0; > } > > -static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > +static int __vhci_create_device(struct vhci_data *data, __u8 opcode, > + struct vhci_ext_config *ext_config) > { > struct hci_dev *hdev; > struct sk_buff *skb; > @@ -287,8 +298,20 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > if (data->hdev) > return -EBADFD; > > - /* bits 0-1 are dev_type (Primary or AMP) */ > - dev_type = opcode & 0x03; > + /* In case of legacy opcode, it doesn't allow to have 0x03 as an opcode, > + * So, it is ok to assume that device is in the extended device > + * creation mode when the opcode is 0x03. Also, it is required to have > + * a ext_config and check it here. > + */ > + if (ext_config && opcode != VHCI_EXT_OPCODE) > + return -EINVAL; > + > + if (opcode == VHCI_EXT_OPCODE) > + dev_type = ext_config->dev_type; > + else { > + /* bits 0-1 are dev_type (Primary or AMP) */ > + dev_type = opcode & 0x03; > + } > > if (dev_type != HCI_PRIMARY && dev_type != HCI_AMP) > return -EINVAL; > @@ -331,6 +354,16 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > if (opcode & 0x80) > set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); > > + /* Flags for extended configuration */ > + if (ext_config && opcode == VHCI_EXT_OPCODE) { > + if (ext_config->flags & VHCI_FLAG_QUIRK_EXTERNAL_CONFIG) > + set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks); > + if (ext_config->flags & VHCI_FLAG_QUIRK_RAW_DEVICE) > + set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); > + if (ext_config->flags & VHCI_FLAG_QUIRKS_INVALID_BDADDR) > + set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); > + } > + > if (hci_register_dev(hdev) < 0) { > BT_ERR("Can't register HCI device"); > hci_free_dev(hdev); > @@ -364,12 +397,13 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > return 0; > } > > -static int vhci_create_device(struct vhci_data *data, __u8 opcode) > +static int vhci_create_device(struct vhci_data *data, __u8 opcode, > + struct vhci_ext_config *ext_config) > { > int err; > > mutex_lock(&data->open_mutex); > - err = __vhci_create_device(data, opcode); > + err = __vhci_create_device(data, opcode, ext_config); > mutex_unlock(&data->open_mutex); > > return err; > @@ -379,6 +413,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, > struct iov_iter *from) > { > size_t len = iov_iter_count(from); > + struct vhci_ext_config *ext_config; > struct sk_buff *skb; > __u8 pkt_type, opcode; > int ret; > @@ -419,6 +454,21 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, > opcode = *((__u8 *) skb->data); > skb_pull(skb, 1); > > + /* This opcode(0x03) is for extended device creation and it > + * requires the extra parameters for extra configuration. > + */ > + if (opcode == 0x03) { > + if (skb->len != sizeof(*ext_config)) { > + kfree_skb(skb); > + return -EINVAL; > + } > + > + ext_config = (void *) (skb->data); > + ret = vhci_create_device(data, opcode, ext_config); > + kfree_skb(skb); > + goto done; > + } > + > if (skb->len > 0) { > kfree_skb(skb); > return -EINVAL; > @@ -426,7 +476,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, > > kfree_skb(skb); > > - ret = vhci_create_device(data, opcode); > + ret = vhci_create_device(data, opcode, NULL); > break; > > default: > @@ -434,6 +484,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, > return -EINVAL; > } > > +done: > return (ret < 0) ? ret : len; > } > > @@ -526,7 +577,7 @@ static void vhci_open_timeout(struct work_struct *work) > struct vhci_data *data = container_of(work, struct vhci_data, > open_timeout.work); > > - vhci_create_device(data, amp ? HCI_AMP : HCI_PRIMARY); > + vhci_create_device(data, amp ? HCI_AMP : HCI_PRIMARY, NULL); > } I think this is a bit convoluted in the end. diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c index 49ac884d996e..ce33ed63d021 100644 --- a/drivers/bluetooth/hci_vhci.c +++ b/drivers/bluetooth/hci_vhci.c @@ -419,14 +419,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; I don’t fully like the nesting yet, but I would do it something like that. Main point is that we keep the old way as it is and create a new clean path since otherwise the code becomes really hard to follow if you have to look at it in a few month. I would also just include virtio_bt.h since that is actually an UAPI header and shared with userspace properly. I currently made extension struct just >= 1 and that might be good enough. We can check what the flags size is in virtio space. Otherwise we might just to a flexible flags array after 0x03 opcode. Something that Bluetooth uses for EIR/AD flags data type. So you have {opcode}, {flag_len}, {flags}[0..n], {bt_config} as your structure. That means the default is then 0x03, 0x00, 0x00 packet to be sent. And if you want to enable AOSP, you send 0x03, 0x01, 0x04, 0x00. Regards Marcel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH V2] Bluetooth: vhci: Add support extended opcode for HCI Vendor packet 2021-11-02 8:31 ` Marcel Holtmann @ 2021-11-02 21:44 ` An, Tedd 0 siblings, 0 replies; 4+ messages in thread From: An, Tedd @ 2021-11-02 21:44 UTC (permalink / raw) To: marcel, tedd.an; +Cc: linux-bluetooth Hi Marcel, On Tue, 2021-11-02 at 09:31 +0100, Marcel Holtmann wrote: > Hi Tedd, > > > Current implementation uses the Vendor packet type (0xff) with opcode > > parameter. But there is no way to expand the opcode and no available bits > > to use. Also it cannot be changed due to the backward compatibility > > with older kernel. > > > > THis patch adds new opcode(0x03) for HCI Vendor packet for VHCI for > > extended device creation. This new opcode will not conflict with > > existing legacy opcode because the legacy expects to set either bit 0 or > > bit 1, but not both of bits. > > > > It aslo requires new extra parameters for device type and flags to apply > > to the VHCI device. > > > > Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com> > > --- > > drivers/bluetooth/hci_vhci.c | 65 ++++++++++++++++++++++++++++++++---- > > 1 file changed, 58 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c > > index 49ac884d996e..5fccab136543 100644 > > --- a/drivers/bluetooth/hci_vhci.c > > +++ b/drivers/bluetooth/hci_vhci.c > > @@ -30,6 +30,16 @@ > > > > static bool amp; > > > > +#define VHCI_EXT_OPCODE 0x03 > > +struct vhci_ext_config { > > + __u8 dev_type; > > + __u32 flags; > > +} __packed; > > + > > +#define VHCI_FLAG_QUIRK_RAW_DEVICE 0x01 > > +#define VHCI_FLAG_QUIRK_EXTERNAL_CONFIG 0x02 > > +#define VHCI_FLAG_QUIRKS_INVALID_BDADDR 0x04 > > + > > struct vhci_data { > > struct hci_dev *hdev; > > > > @@ -278,7 +288,8 @@ static int vhci_setup(struct hci_dev *hdev) > > return 0; > > } > > > > -static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > > +static int __vhci_create_device(struct vhci_data *data, __u8 opcode, > > + struct vhci_ext_config *ext_config) > > { > > struct hci_dev *hdev; > > struct sk_buff *skb; > > @@ -287,8 +298,20 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > > if (data->hdev) > > return -EBADFD; > > > > - /* bits 0-1 are dev_type (Primary or AMP) */ > > - dev_type = opcode & 0x03; > > + /* In case of legacy opcode, it doesn't allow to have 0x03 as an opcode, > > + * So, it is ok to assume that device is in the extended device > > + * creation mode when the opcode is 0x03. Also, it is required to have > > + * a ext_config and check it here. > > + */ > > + if (ext_config && opcode != VHCI_EXT_OPCODE) > > + return -EINVAL; > > + > > + if (opcode == VHCI_EXT_OPCODE) > > + dev_type = ext_config->dev_type; > > + else { > > + /* bits 0-1 are dev_type (Primary or AMP) */ > > + dev_type = opcode & 0x03; > > + } > > > > if (dev_type != HCI_PRIMARY && dev_type != HCI_AMP) > > return -EINVAL; > > @@ -331,6 +354,16 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > > if (opcode & 0x80) > > set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); > > > > + /* Flags for extended configuration */ > > + if (ext_config && opcode == VHCI_EXT_OPCODE) { > > + if (ext_config->flags & VHCI_FLAG_QUIRK_EXTERNAL_CONFIG) > > + set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks); > > + if (ext_config->flags & VHCI_FLAG_QUIRK_RAW_DEVICE) > > + set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); > > + if (ext_config->flags & VHCI_FLAG_QUIRKS_INVALID_BDADDR) > > + set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); > > + } > > + > > if (hci_register_dev(hdev) < 0) { > > BT_ERR("Can't register HCI device"); > > hci_free_dev(hdev); > > @@ -364,12 +397,13 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > > return 0; > > } > > > > -static int vhci_create_device(struct vhci_data *data, __u8 opcode) > > +static int vhci_create_device(struct vhci_data *data, __u8 opcode, > > + struct vhci_ext_config *ext_config) > > { > > int err; > > > > mutex_lock(&data->open_mutex); > > - err = __vhci_create_device(data, opcode); > > + err = __vhci_create_device(data, opcode, ext_config); > > mutex_unlock(&data->open_mutex); > > > > return err; > > @@ -379,6 +413,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, > > struct iov_iter *from) > > { > > size_t len = iov_iter_count(from); > > + struct vhci_ext_config *ext_config; > > struct sk_buff *skb; > > __u8 pkt_type, opcode; > > int ret; > > @@ -419,6 +454,21 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, > > opcode = *((__u8 *) skb->data); > > skb_pull(skb, 1); > > > > + /* This opcode(0x03) is for extended device creation and it > > + * requires the extra parameters for extra configuration. > > + */ > > + if (opcode == 0x03) { > > + if (skb->len != sizeof(*ext_config)) { > > + kfree_skb(skb); > > + return -EINVAL; > > + } > > + > > + ext_config = (void *) (skb->data); > > + ret = vhci_create_device(data, opcode, ext_config); > > + kfree_skb(skb); > > + goto done; > > + } > > + > > if (skb->len > 0) { > > kfree_skb(skb); > > return -EINVAL; > > @@ -426,7 +476,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, > > > > kfree_skb(skb); > > > > - ret = vhci_create_device(data, opcode); > > + ret = vhci_create_device(data, opcode, NULL); > > break; > > > > default: > > @@ -434,6 +484,7 @@ static inline ssize_t vhci_get_user(struct vhci_data *data, > > return -EINVAL; > > } > > > > +done: > > return (ret < 0) ? ret : len; > > } > > > > @@ -526,7 +577,7 @@ static void vhci_open_timeout(struct work_struct *work) > > struct vhci_data *data = container_of(work, struct vhci_data, > > open_timeout.work); > > > > - vhci_create_device(data, amp ? HCI_AMP : HCI_PRIMARY); > > + vhci_create_device(data, amp ? HCI_AMP : HCI_PRIMARY, NULL); > > } > > I think this is a bit convoluted in the end. > > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c > index 49ac884d996e..ce33ed63d021 100644 > --- a/drivers/bluetooth/hci_vhci.c > +++ b/drivers/bluetooth/hci_vhci.c > @@ -419,14 +419,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; > > I don’t fully like the nesting yet, but I would do it something like that. > > Main point is that we keep the old way as it is and create a new clean path since otherwise the code > becomes really hard to follow if you have to look at it in a few month. Got it. It seems my first patch is more close to it, except new Vendor code. I will combine it with your code above. > I would also just include virtio_bt.h since that is actually an UAPI header and shared with > userspace properly. I currently made extension struct just >= 1 and that might be good enough. We > can check what the flags size is in virtio space. Otherwise we might just to a flexible flags > array after 0x03 opcode. Do you mean to include virtio_bt.h to share the data sturct between userspace and the kerenl? On my ubuntu 20.04, virtio_bt.h is not available for some reason (on generic kernel) > > Something that Bluetooth uses for EIR/AD flags data type. So you have {opcode}, {flag_len}, > {flags}[0..n], {bt_config} as your structure. That means the default is then 0x03, 0x00, 0x00 > packet to be sent. And if you want to enable AOSP, you send 0x03, 0x01, 0x04, 0x00. I will use these method for the parameters. I only thought to support the quarks but it can even expand to enable/disable MSFT EXT with opcode and no need to use debugfs to set it. > > > Regards > > Marcel > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-02 21:44 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-02 2:56 [RFC PATCH V2] Bluetooth: vhci: Add support extended opcode for HCI Vendor packet Tedd Ho-Jeong An 2021-11-02 3:01 ` An, Tedd 2021-11-02 8:31 ` Marcel Holtmann 2021-11-02 21:44 ` An, Tedd
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.