Re: [RFC PATCH V2] Bluetooth: vhci: Add support extended opcode for HCI Vendor packet

["An, Tedd" <tedd.an@intel.com>]

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
>