| Message ID | 20211011211147.2379624-1-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [RFC] Bluetooth: vhci: Add support for setting msft_opcode | expand |
Hi Luiz, > This adds a debugfs entry to set msft_opcode enabling vhci to emulate > controllers with MSFT extention support. > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > drivers/bluetooth/hci_vhci.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c > index 56c6b22be10b..ac122299bacc 100644 > --- a/drivers/bluetooth/hci_vhci.c > +++ b/drivers/bluetooth/hci_vhci.c > @@ -194,6 +194,34 @@ static const struct file_operations force_wakeup_fops = { > .llseek = default_llseek, > }; > > + > +static int msft_opcode_set(void *data, u64 val) > +{ > + struct vhci_data *vhci = data; > + uint16_t ogf = (val & 0xffff >> 10); > + > + if (val > 0xffff || ogf != 0x3f) I would actually just include it here to avoid any 16-bit overflow. if (val > 0xffff || (val & 0xffff >> 10) != 0x3f) > + return -EINVAL; > + > + hci_set_msft_opcode(vhci->hdev, val); > + > + return 0; > +} > + > +static int msft_opcode_get(void *data, u64 *val) > +{ > + struct vhci_data *vhci = data; > + > + hci_dev_lock(vhci->hdev); > + *val = vhci->hdev->msft_opcode; > + hci_dev_unlock(vhci->hdev); > + > + return 0; > +} > + > +DEFINE_DEBUGFS_ATTRIBUTE(msft_opcode_fops, msft_opcode_get, msft_opcode_set, > + "%llu\n"); > + > static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > { > struct hci_dev *hdev; > @@ -259,6 +287,10 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > 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); > + So my concern is that you can modify this value when the device is up and running. That will cause havoc. Just checking HCI_UP is kinda bad since we just removed that access from the drivers. Regards Marcel
Hi Marcel, On Tue, Oct 12, 2021 at 8:50 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Luiz, > > > This adds a debugfs entry to set msft_opcode enabling vhci to emulate > > controllers with MSFT extention support. > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > drivers/bluetooth/hci_vhci.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c > > index 56c6b22be10b..ac122299bacc 100644 > > --- a/drivers/bluetooth/hci_vhci.c > > +++ b/drivers/bluetooth/hci_vhci.c > > @@ -194,6 +194,34 @@ static const struct file_operations force_wakeup_fops = { > > .llseek = default_llseek, > > }; > > > > + > > +static int msft_opcode_set(void *data, u64 val) > > +{ > > + struct vhci_data *vhci = data; > > + uint16_t ogf = (val & 0xffff >> 10); > > + > > + if (val > 0xffff || ogf != 0x3f) > > I would actually just include it here to avoid any 16-bit overflow. > > if (val > 0xffff || (val & 0xffff >> 10) != 0x3f) Ack. > > + return -EINVAL; > > + > > + hci_set_msft_opcode(vhci->hdev, val); > > + > > + return 0; > > +} > > + > > +static int msft_opcode_get(void *data, u64 *val) > > +{ > > + struct vhci_data *vhci = data; > > + > > + hci_dev_lock(vhci->hdev); > > + *val = vhci->hdev->msft_opcode; > > + hci_dev_unlock(vhci->hdev); > > + > > + return 0; > > +} > > + > > +DEFINE_DEBUGFS_ATTRIBUTE(msft_opcode_fops, msft_opcode_get, msft_opcode_set, > > + "%llu\n"); > > + > > static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > > { > > struct hci_dev *hdev; > > @@ -259,6 +287,10 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) > > 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); > > + > > So my concern is that you can modify this value when the device is up and running. That will cause havoc. > > Just checking HCI_UP is kinda bad since we just removed that access from the drivers. Right but we could add a check to HCI_UP inside hci_set_msft_opcode and make it return an error, actually this might be a good idea anyway even with existing so we prevent bad usage of hci_set_msft_opcode when already up. > Regards > > Marcel >
Hi Luiz, >>> This adds a debugfs entry to set msft_opcode enabling vhci to emulate >>> controllers with MSFT extention support. >>> >>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>> --- >>> drivers/bluetooth/hci_vhci.c | 32 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 32 insertions(+) >>> >>> diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c >>> index 56c6b22be10b..ac122299bacc 100644 >>> --- a/drivers/bluetooth/hci_vhci.c >>> +++ b/drivers/bluetooth/hci_vhci.c >>> @@ -194,6 +194,34 @@ static const struct file_operations force_wakeup_fops = { >>> .llseek = default_llseek, >>> }; >>> >>> + >>> +static int msft_opcode_set(void *data, u64 val) >>> +{ >>> + struct vhci_data *vhci = data; >>> + uint16_t ogf = (val & 0xffff >> 10); >>> + >>> + if (val > 0xffff || ogf != 0x3f) >> >> I would actually just include it here to avoid any 16-bit overflow. >> >> if (val > 0xffff || (val & 0xffff >> 10) != 0x3f) > > Ack. > >>> + return -EINVAL; >>> + >>> + hci_set_msft_opcode(vhci->hdev, val); >>> + >>> + return 0; >>> +} >>> + >>> +static int msft_opcode_get(void *data, u64 *val) >>> +{ >>> + struct vhci_data *vhci = data; >>> + >>> + hci_dev_lock(vhci->hdev); >>> + *val = vhci->hdev->msft_opcode; >>> + hci_dev_unlock(vhci->hdev); >>> + >>> + return 0; >>> +} >>> + >>> +DEFINE_DEBUGFS_ATTRIBUTE(msft_opcode_fops, msft_opcode_get, msft_opcode_set, >>> + "%llu\n"); >>> + >>> static int __vhci_create_device(struct vhci_data *data, __u8 opcode) >>> { >>> struct hci_dev *hdev; >>> @@ -259,6 +287,10 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) >>> 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); >>> + >> >> So my concern is that you can modify this value when the device is up and running. That will cause havoc. >> >> Just checking HCI_UP is kinda bad since we just removed that access from the drivers. > > Right but we could add a check to HCI_UP inside hci_set_msft_opcode > and make it return an error, actually this might be a good idea anyway > even with existing so we prevent bad usage of hci_set_msft_opcode when > already up. I did mean actually HCI_RUNNING, but this still won't work out since you should be able to set the opcode from hdev->setup. You might be able to craft enough tests around HCI_INIT and HCI_SETUP to make it fail hci_set_msft_opcode. So that might be the safest way after all. One other option is to actually just store the msft_opcode from debugfs in vhci_data. And then only set it from within hdev->setup. You need to set HCI_QUIRK_NON_PERSISTENT_SETUP for this, but that might actually work best then. Note: you need an aosp_capable debugfs setting as well. Regards Marcel
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c index 56c6b22be10b..ac122299bacc 100644 --- a/drivers/bluetooth/hci_vhci.c +++ b/drivers/bluetooth/hci_vhci.c @@ -194,6 +194,34 @@ static const struct file_operations force_wakeup_fops = { .llseek = default_llseek, }; + +static int msft_opcode_set(void *data, u64 val) +{ + struct vhci_data *vhci = data; + uint16_t ogf = (val & 0xffff >> 10); + + if (val > 0xffff || ogf != 0x3f) + return -EINVAL; + + hci_set_msft_opcode(vhci->hdev, val); + + return 0; +} + +static int msft_opcode_get(void *data, u64 *val) +{ + struct vhci_data *vhci = data; + + hci_dev_lock(vhci->hdev); + *val = vhci->hdev->msft_opcode; + hci_dev_unlock(vhci->hdev); + + return 0; +} + +DEFINE_DEBUGFS_ATTRIBUTE(msft_opcode_fops, msft_opcode_get, msft_opcode_set, + "%llu\n"); + static int __vhci_create_device(struct vhci_data *data, __u8 opcode) { struct hci_dev *hdev; @@ -259,6 +287,10 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode) 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); + hci_skb_pkt_type(skb) = HCI_VENDOR_PKT; skb_put_u8(skb, 0xff);