* [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular @ 2020-08-13 8:41 Joseph Hwang 2020-08-13 8:41 ` [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts Joseph Hwang ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Joseph Hwang @ 2020-08-13 8:41 UTC (permalink / raw) To: linux-bluetooth, marcel, luiz.dentz Cc: josephsih, chromeos-bluetooth-upstreaming, Joseph Hwang, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev USB alternate seeting of which the packet size is distinct. The patches are to expose the packet size to user space so that the user space does not need to hard code those values. We have verified this patch on Chromebooks which use - Realtek 8822CE controller with USB alt setting 1 - Intel controller with USB alt setting 6 Our user space audio server, cras, can get the correct packet length from the socket option. Joseph Hwang (2): Bluetooth: btusb: define HCI packet sizes of USB Alts Bluetooth: sco: expose WBS packet length in socket option drivers/bluetooth/btusb.c | 43 +++++++++++++++++++++++-------- include/net/bluetooth/bluetooth.h | 2 ++ include/net/bluetooth/hci_core.h | 1 + net/bluetooth/sco.c | 8 ++++++ 4 files changed, 43 insertions(+), 11 deletions(-) -- 2.28.0.236.gb10cc79966-goog ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts 2020-08-13 8:41 [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular Joseph Hwang @ 2020-08-13 8:41 ` Joseph Hwang 2020-08-14 20:07 ` Luiz Augusto von Dentz 2020-08-13 8:41 ` [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option Joseph Hwang 2020-08-19 2:48 ` [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular Shyh-In Hwang 2 siblings, 1 reply; 11+ messages in thread From: Joseph Hwang @ 2020-08-13 8:41 UTC (permalink / raw) To: linux-bluetooth, marcel, luiz.dentz Cc: josephsih, chromeos-bluetooth-upstreaming, Joseph Hwang, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev It is desirable to define the HCI packet payload sizes of USB alternate settings so that they can be exposed to user space. Reviewed-by: Alain Michaud <alainm@chromium.org> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> Signed-off-by: Joseph Hwang <josephsih@chromium.org> --- drivers/bluetooth/btusb.c | 43 ++++++++++++++++++++++++-------- include/net/bluetooth/hci_core.h | 1 + 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 8d2608ddfd0875..df7cadf6385868 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -459,6 +459,22 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = { #define BTUSB_WAKEUP_DISABLE 14 #define BTUSB_USE_ALT1_FOR_WBS 15 +/* Per core spec 5, vol 4, part B, table 2.1, + * list the hci packet payload sizes for various ALT settings. + * This is used to set the packet length for the wideband speech. + * If a controller does not probe its usb alt setting, the default + * value will be 0. Any clients at upper layers should interpret it + * as a default value and set a proper packet length accordingly. + * + * To calculate the HCI packet payload length: + * for alternate settings 1 - 5: + * hci_packet_size = suggested_max_packet_size * 3 (packets) - + * 3 (HCI header octets) + * for alternate setting 6: + * hci_packet_size = suggested_max_packet_size - 3 (HCI header octets) + */ +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60 }; + struct btusb_data { struct hci_dev *hdev; struct usb_device *udev; @@ -3958,6 +3974,15 @@ static int btusb_probe(struct usb_interface *intf, hdev->notify = btusb_notify; hdev->prevent_wake = btusb_prevent_wake; + if (id->driver_info & BTUSB_AMP) { + /* AMP controllers do not support SCO packets */ + data->isoc = NULL; + } else { + /* Interface orders are hardcoded in the specification */ + data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1); + data->isoc_ifnum = ifnum_base + 1; + } + #ifdef CONFIG_PM err = btusb_config_oob_wake(hdev); if (err) @@ -4021,6 +4046,10 @@ static int btusb_probe(struct usb_interface *intf, hdev->set_diag = btintel_set_diag; hdev->set_bdaddr = btintel_set_bdaddr; hdev->cmd_timeout = btusb_intel_cmd_timeout; + + if (btusb_find_altsetting(data, 6)) + hdev->sco_pkt_len = hci_packet_size_usb_alt[6]; + set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); @@ -4062,15 +4091,6 @@ static int btusb_probe(struct usb_interface *intf, btusb_check_needs_reset_resume(intf); } - if (id->driver_info & BTUSB_AMP) { - /* AMP controllers do not support SCO packets */ - data->isoc = NULL; - } else { - /* Interface orders are hardcoded in the specification */ - data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1); - data->isoc_ifnum = ifnum_base + 1; - } - if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) && (id->driver_info & BTUSB_REALTEK)) { hdev->setup = btrtl_setup_realtek; @@ -4082,9 +4102,10 @@ static int btusb_probe(struct usb_interface *intf, * (DEVICE_REMOTE_WAKEUP) */ set_bit(BTUSB_WAKEUP_DISABLE, &data->flags); - if (btusb_find_altsetting(data, 1)) + if (btusb_find_altsetting(data, 1)) { set_bit(BTUSB_USE_ALT1_FOR_WBS, &data->flags); - else + hdev->sco_pkt_len = hci_packet_size_usb_alt[1]; + } else bt_dev_err(hdev, "Device does not support ALT setting 1"); } diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 8caac20556b499..0624496328fc09 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -417,6 +417,7 @@ struct hci_dev { unsigned int acl_pkts; unsigned int sco_pkts; unsigned int le_pkts; + unsigned int sco_pkt_len; __u16 block_len; __u16 block_mtu; -- 2.28.0.236.gb10cc79966-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts 2020-08-13 8:41 ` [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts Joseph Hwang @ 2020-08-14 20:07 ` Luiz Augusto von Dentz 2020-08-19 14:38 ` Pali Rohár 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2020-08-14 20:07 UTC (permalink / raw) To: Joseph Hwang Cc: linux-bluetooth, Marcel Holtmann, Joseph Hwang, ChromeOS Bluetooth Upstreaming, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, Linux Kernel Mailing List, open list:NETWORKING [GENERAL] Hi Joseph, On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > It is desirable to define the HCI packet payload sizes of > USB alternate settings so that they can be exposed to user > space. > > Reviewed-by: Alain Michaud <alainm@chromium.org> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > --- > > drivers/bluetooth/btusb.c | 43 ++++++++++++++++++++++++-------- > include/net/bluetooth/hci_core.h | 1 + > 2 files changed, 33 insertions(+), 11 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 8d2608ddfd0875..df7cadf6385868 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -459,6 +459,22 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = { > #define BTUSB_WAKEUP_DISABLE 14 > #define BTUSB_USE_ALT1_FOR_WBS 15 > > +/* Per core spec 5, vol 4, part B, table 2.1, > + * list the hci packet payload sizes for various ALT settings. > + * This is used to set the packet length for the wideband speech. > + * If a controller does not probe its usb alt setting, the default > + * value will be 0. Any clients at upper layers should interpret it > + * as a default value and set a proper packet length accordingly. > + * > + * To calculate the HCI packet payload length: > + * for alternate settings 1 - 5: > + * hci_packet_size = suggested_max_packet_size * 3 (packets) - > + * 3 (HCI header octets) > + * for alternate setting 6: > + * hci_packet_size = suggested_max_packet_size - 3 (HCI header octets) > + */ > +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60 }; > + > struct btusb_data { > struct hci_dev *hdev; > struct usb_device *udev; > @@ -3958,6 +3974,15 @@ static int btusb_probe(struct usb_interface *intf, > hdev->notify = btusb_notify; > hdev->prevent_wake = btusb_prevent_wake; > > + if (id->driver_info & BTUSB_AMP) { > + /* AMP controllers do not support SCO packets */ > + data->isoc = NULL; > + } else { > + /* Interface orders are hardcoded in the specification */ > + data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1); > + data->isoc_ifnum = ifnum_base + 1; > + } > + > #ifdef CONFIG_PM > err = btusb_config_oob_wake(hdev); > if (err) > @@ -4021,6 +4046,10 @@ static int btusb_probe(struct usb_interface *intf, > hdev->set_diag = btintel_set_diag; > hdev->set_bdaddr = btintel_set_bdaddr; > hdev->cmd_timeout = btusb_intel_cmd_timeout; > + > + if (btusb_find_altsetting(data, 6)) > + hdev->sco_pkt_len = hci_packet_size_usb_alt[6]; > + > set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); > set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); > set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); > @@ -4062,15 +4091,6 @@ static int btusb_probe(struct usb_interface *intf, > btusb_check_needs_reset_resume(intf); > } > > - if (id->driver_info & BTUSB_AMP) { > - /* AMP controllers do not support SCO packets */ > - data->isoc = NULL; > - } else { > - /* Interface orders are hardcoded in the specification */ > - data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1); > - data->isoc_ifnum = ifnum_base + 1; > - } > - > if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) && > (id->driver_info & BTUSB_REALTEK)) { > hdev->setup = btrtl_setup_realtek; > @@ -4082,9 +4102,10 @@ static int btusb_probe(struct usb_interface *intf, > * (DEVICE_REMOTE_WAKEUP) > */ > set_bit(BTUSB_WAKEUP_DISABLE, &data->flags); > - if (btusb_find_altsetting(data, 1)) > + if (btusb_find_altsetting(data, 1)) { > set_bit(BTUSB_USE_ALT1_FOR_WBS, &data->flags); > - else > + hdev->sco_pkt_len = hci_packet_size_usb_alt[1]; > + } else > bt_dev_err(hdev, "Device does not support ALT setting 1"); > } > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index 8caac20556b499..0624496328fc09 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -417,6 +417,7 @@ struct hci_dev { > unsigned int acl_pkts; > unsigned int sco_pkts; > unsigned int le_pkts; > + unsigned int sco_pkt_len; Id use sco_mtu to so the following check actually does what it intended to do: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/sco.c#n283 Right now it seems we are using the buffer length as MTU but I think we should actually use the packet length if it is lower than the buffer length, actually it doesn't seems SCO packets can be fragmented so the buffer length must always be big enough to carry a full packet so I assume setting the packet length as conn->mtu will always be correct. > > __u16 block_len; > __u16 block_mtu; > -- > 2.28.0.236.gb10cc79966-goog > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts 2020-08-14 20:07 ` Luiz Augusto von Dentz @ 2020-08-19 14:38 ` Pali Rohár 0 siblings, 0 replies; 11+ messages in thread From: Pali Rohár @ 2020-08-19 14:38 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: Joseph Hwang, linux-bluetooth, Marcel Holtmann, Joseph Hwang, ChromeOS Bluetooth Upstreaming, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, Linux Kernel Mailing List, open list:NETWORKING [GENERAL] On Friday 14 August 2020 13:07:25 Luiz Augusto von Dentz wrote: > Hi Joseph, > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > It is desirable to define the HCI packet payload sizes of > > USB alternate settings so that they can be exposed to user > > space. > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > --- > > > > drivers/bluetooth/btusb.c | 43 ++++++++++++++++++++++++-------- > > include/net/bluetooth/hci_core.h | 1 + > > 2 files changed, 33 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > > index 8d2608ddfd0875..df7cadf6385868 100644 > > --- a/drivers/bluetooth/btusb.c > > +++ b/drivers/bluetooth/btusb.c > > @@ -459,6 +459,22 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = { > > #define BTUSB_WAKEUP_DISABLE 14 > > #define BTUSB_USE_ALT1_FOR_WBS 15 > > > > +/* Per core spec 5, vol 4, part B, table 2.1, > > + * list the hci packet payload sizes for various ALT settings. > > + * This is used to set the packet length for the wideband speech. > > + * If a controller does not probe its usb alt setting, the default > > + * value will be 0. Any clients at upper layers should interpret it > > + * as a default value and set a proper packet length accordingly. > > + * > > + * To calculate the HCI packet payload length: > > + * for alternate settings 1 - 5: > > + * hci_packet_size = suggested_max_packet_size * 3 (packets) - > > + * 3 (HCI header octets) > > + * for alternate setting 6: > > + * hci_packet_size = suggested_max_packet_size - 3 (HCI header octets) > > + */ > > +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60 }; > > + > > struct btusb_data { > > struct hci_dev *hdev; > > struct usb_device *udev; > > @@ -3958,6 +3974,15 @@ static int btusb_probe(struct usb_interface *intf, > > hdev->notify = btusb_notify; > > hdev->prevent_wake = btusb_prevent_wake; > > > > + if (id->driver_info & BTUSB_AMP) { > > + /* AMP controllers do not support SCO packets */ > > + data->isoc = NULL; > > + } else { > > + /* Interface orders are hardcoded in the specification */ > > + data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1); > > + data->isoc_ifnum = ifnum_base + 1; > > + } > > + > > #ifdef CONFIG_PM > > err = btusb_config_oob_wake(hdev); > > if (err) > > @@ -4021,6 +4046,10 @@ static int btusb_probe(struct usb_interface *intf, > > hdev->set_diag = btintel_set_diag; > > hdev->set_bdaddr = btintel_set_bdaddr; > > hdev->cmd_timeout = btusb_intel_cmd_timeout; > > + > > + if (btusb_find_altsetting(data, 6)) > > + hdev->sco_pkt_len = hci_packet_size_usb_alt[6]; > > + > > set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); > > set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); > > set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); > > @@ -4062,15 +4091,6 @@ static int btusb_probe(struct usb_interface *intf, > > btusb_check_needs_reset_resume(intf); > > } > > > > - if (id->driver_info & BTUSB_AMP) { > > - /* AMP controllers do not support SCO packets */ > > - data->isoc = NULL; > > - } else { > > - /* Interface orders are hardcoded in the specification */ > > - data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1); > > - data->isoc_ifnum = ifnum_base + 1; > > - } > > - > > if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) && > > (id->driver_info & BTUSB_REALTEK)) { > > hdev->setup = btrtl_setup_realtek; > > @@ -4082,9 +4102,10 @@ static int btusb_probe(struct usb_interface *intf, > > * (DEVICE_REMOTE_WAKEUP) > > */ > > set_bit(BTUSB_WAKEUP_DISABLE, &data->flags); > > - if (btusb_find_altsetting(data, 1)) > > + if (btusb_find_altsetting(data, 1)) { > > set_bit(BTUSB_USE_ALT1_FOR_WBS, &data->flags); > > - else > > + hdev->sco_pkt_len = hci_packet_size_usb_alt[1]; > > + } else > > bt_dev_err(hdev, "Device does not support ALT setting 1"); > > } > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index 8caac20556b499..0624496328fc09 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -417,6 +417,7 @@ struct hci_dev { > > unsigned int acl_pkts; > > unsigned int sco_pkts; > > unsigned int le_pkts; > > + unsigned int sco_pkt_len; > > Id use sco_mtu to so the following check actually does what it intended to do: > > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/sco.c#n283 > > Right now it seems we are using the buffer length as MTU but I think > we should actually use the packet length if it is lower than the > buffer length, actually it doesn't seems SCO packets can be fragmented > so the buffer length must always be big enough to carry a full packet > so I assume setting the packet length as conn->mtu will always be > correct. I agree. We should use sco mtu for this purpose. > > > > __u16 block_len; > > __u16 block_mtu; > > -- > > 2.28.0.236.gb10cc79966-goog > > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option 2020-08-13 8:41 [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular Joseph Hwang 2020-08-13 8:41 ` [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts Joseph Hwang @ 2020-08-13 8:41 ` Joseph Hwang 2020-08-14 19:56 ` Luiz Augusto von Dentz 2020-08-19 2:48 ` [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular Shyh-In Hwang 2 siblings, 1 reply; 11+ messages in thread From: Joseph Hwang @ 2020-08-13 8:41 UTC (permalink / raw) To: linux-bluetooth, marcel, luiz.dentz Cc: josephsih, chromeos-bluetooth-upstreaming, Joseph Hwang, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev It is desirable to expose the wideband speech packet length via a socket option to the user space so that the user space can set the value correctly in configuring the sco connection. Reviewed-by: Alain Michaud <alainm@chromium.org> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> Signed-off-by: Joseph Hwang <josephsih@chromium.org> --- include/net/bluetooth/bluetooth.h | 2 ++ net/bluetooth/sco.c | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h index 9125effbf4483d..922cc03143def4 100644 --- a/include/net/bluetooth/bluetooth.h +++ b/include/net/bluetooth/bluetooth.h @@ -153,6 +153,8 @@ struct bt_voice { #define BT_SCM_PKT_STATUS 0x03 +#define BT_SCO_PKT_LEN 17 + __printf(1, 2) void bt_info(const char *fmt, ...); __printf(1, 2) diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index dcf7f96ff417e6..97e4e7c7b8cf62 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -67,6 +67,7 @@ struct sco_pinfo { __u32 flags; __u16 setting; __u8 cmsg_mask; + __u32 pkt_len; struct sco_conn *conn; }; @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) sco_sock_set_timer(sk, sk->sk_sndtimeo); } + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; + done: hci_dev_unlock(hdev); hci_dev_put(hdev); @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, err = -EFAULT; break; + case BT_SCO_PKT_LEN: + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) + err = -EFAULT; + break; + default: err = -ENOPROTOOPT; break; -- 2.28.0.236.gb10cc79966-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option 2020-08-13 8:41 ` [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option Joseph Hwang @ 2020-08-14 19:56 ` Luiz Augusto von Dentz 2020-08-19 14:37 ` Pali Rohár 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2020-08-14 19:56 UTC (permalink / raw) To: Joseph Hwang Cc: linux-bluetooth, Marcel Holtmann, Joseph Hwang, ChromeOS Bluetooth Upstreaming, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, Linux Kernel Mailing List, open list:NETWORKING [GENERAL] Hi Joseph, On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > It is desirable to expose the wideband speech packet length via > a socket option to the user space so that the user space can set > the value correctly in configuring the sco connection. > > Reviewed-by: Alain Michaud <alainm@chromium.org> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > --- > > include/net/bluetooth/bluetooth.h | 2 ++ > net/bluetooth/sco.c | 8 ++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > index 9125effbf4483d..922cc03143def4 100644 > --- a/include/net/bluetooth/bluetooth.h > +++ b/include/net/bluetooth/bluetooth.h > @@ -153,6 +153,8 @@ struct bt_voice { > > #define BT_SCM_PKT_STATUS 0x03 > > +#define BT_SCO_PKT_LEN 17 > + > __printf(1, 2) > void bt_info(const char *fmt, ...); > __printf(1, 2) > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > --- a/net/bluetooth/sco.c > +++ b/net/bluetooth/sco.c > @@ -67,6 +67,7 @@ struct sco_pinfo { > __u32 flags; > __u16 setting; > __u8 cmsg_mask; > + __u32 pkt_len; > struct sco_conn *conn; > }; > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > sco_sock_set_timer(sk, sk->sk_sndtimeo); > } > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > + > done: > hci_dev_unlock(hdev); > hci_dev_put(hdev); > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > err = -EFAULT; > break; > > + case BT_SCO_PKT_LEN: > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > + err = -EFAULT; > + break; Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? > default: > err = -ENOPROTOOPT; > break; > -- > 2.28.0.236.gb10cc79966-goog > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option 2020-08-14 19:56 ` Luiz Augusto von Dentz @ 2020-08-19 14:37 ` Pali Rohár 2020-08-19 18:21 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 11+ messages in thread From: Pali Rohár @ 2020-08-19 14:37 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: Joseph Hwang, linux-bluetooth, Marcel Holtmann, Joseph Hwang, ChromeOS Bluetooth Upstreaming, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, Linux Kernel Mailing List, open list:NETWORKING [GENERAL] On Friday 14 August 2020 12:56:05 Luiz Augusto von Dentz wrote: > Hi Joseph, > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > It is desirable to expose the wideband speech packet length via > > a socket option to the user space so that the user space can set > > the value correctly in configuring the sco connection. > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > --- > > > > include/net/bluetooth/bluetooth.h | 2 ++ > > net/bluetooth/sco.c | 8 ++++++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > index 9125effbf4483d..922cc03143def4 100644 > > --- a/include/net/bluetooth/bluetooth.h > > +++ b/include/net/bluetooth/bluetooth.h > > @@ -153,6 +153,8 @@ struct bt_voice { > > > > #define BT_SCM_PKT_STATUS 0x03 > > > > +#define BT_SCO_PKT_LEN 17 > > + > > __printf(1, 2) > > void bt_info(const char *fmt, ...); > > __printf(1, 2) > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > > --- a/net/bluetooth/sco.c > > +++ b/net/bluetooth/sco.c > > @@ -67,6 +67,7 @@ struct sco_pinfo { > > __u32 flags; > > __u16 setting; > > __u8 cmsg_mask; > > + __u32 pkt_len; > > struct sco_conn *conn; > > }; > > > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > > sco_sock_set_timer(sk, sk->sk_sndtimeo); > > } > > > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > > + > > done: > > hci_dev_unlock(hdev); > > hci_dev_put(hdev); > > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > > err = -EFAULT; > > break; > > > > + case BT_SCO_PKT_LEN: > > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > > + err = -EFAULT; > > + break; > > Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? Hello! There is already SCO_OPTIONS sock option, uses struct sco_options and contains 'mtu' member. I think that instead of adding new sock option, existing SCO_OPTIONS option should be used. > > default: > > err = -ENOPROTOOPT; > > break; > > -- > > 2.28.0.236.gb10cc79966-goog > > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option 2020-08-19 14:37 ` Pali Rohár @ 2020-08-19 18:21 ` Luiz Augusto von Dentz 2020-08-19 18:23 ` Pali Rohár 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2020-08-19 18:21 UTC (permalink / raw) To: Pali Rohár Cc: Joseph Hwang, linux-bluetooth, Marcel Holtmann, Joseph Hwang, ChromeOS Bluetooth Upstreaming, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, Linux Kernel Mailing List, open list:NETWORKING [GENERAL] Hi Pali, On Wed, Aug 19, 2020 at 7:37 AM Pali Rohár <pali@kernel.org> wrote: > > On Friday 14 August 2020 12:56:05 Luiz Augusto von Dentz wrote: > > Hi Joseph, > > > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > > > It is desirable to expose the wideband speech packet length via > > > a socket option to the user space so that the user space can set > > > the value correctly in configuring the sco connection. > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > > --- > > > > > > include/net/bluetooth/bluetooth.h | 2 ++ > > > net/bluetooth/sco.c | 8 ++++++++ > > > 2 files changed, 10 insertions(+) > > > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > > index 9125effbf4483d..922cc03143def4 100644 > > > --- a/include/net/bluetooth/bluetooth.h > > > +++ b/include/net/bluetooth/bluetooth.h > > > @@ -153,6 +153,8 @@ struct bt_voice { > > > > > > #define BT_SCM_PKT_STATUS 0x03 > > > > > > +#define BT_SCO_PKT_LEN 17 > > > + > > > __printf(1, 2) > > > void bt_info(const char *fmt, ...); > > > __printf(1, 2) > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > > > --- a/net/bluetooth/sco.c > > > +++ b/net/bluetooth/sco.c > > > @@ -67,6 +67,7 @@ struct sco_pinfo { > > > __u32 flags; > > > __u16 setting; > > > __u8 cmsg_mask; > > > + __u32 pkt_len; > > > struct sco_conn *conn; > > > }; > > > > > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > > > sco_sock_set_timer(sk, sk->sk_sndtimeo); > > > } > > > > > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > > > + > > > done: > > > hci_dev_unlock(hdev); > > > hci_dev_put(hdev); > > > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > > > err = -EFAULT; > > > break; > > > > > > + case BT_SCO_PKT_LEN: > > > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > > > + err = -EFAULT; > > > + break; > > > > Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? > > Hello! > > There is already SCO_OPTIONS sock option, uses struct sco_options and > contains 'mtu' member. > > I think that instead of adding new sock option, existing SCO_OPTIONS > option should be used. We are moving away from type specific options to so options like BT_SNDMTU/BT_RCVMTU should be supported in all socket types. > > > > default: > > > err = -ENOPROTOOPT; > > > break; > > > -- > > > 2.28.0.236.gb10cc79966-goog > > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option 2020-08-19 18:21 ` Luiz Augusto von Dentz @ 2020-08-19 18:23 ` Pali Rohár 2020-08-19 18:25 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 11+ messages in thread From: Pali Rohár @ 2020-08-19 18:23 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: Joseph Hwang, linux-bluetooth, Marcel Holtmann, Joseph Hwang, ChromeOS Bluetooth Upstreaming, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, Linux Kernel Mailing List, open list:NETWORKING [GENERAL] On Wednesday 19 August 2020 11:21:00 Luiz Augusto von Dentz wrote: > Hi Pali, > > On Wed, Aug 19, 2020 at 7:37 AM Pali Rohár <pali@kernel.org> wrote: > > > > On Friday 14 August 2020 12:56:05 Luiz Augusto von Dentz wrote: > > > Hi Joseph, > > > > > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > > > > > It is desirable to expose the wideband speech packet length via > > > > a socket option to the user space so that the user space can set > > > > the value correctly in configuring the sco connection. > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > > > --- > > > > > > > > include/net/bluetooth/bluetooth.h | 2 ++ > > > > net/bluetooth/sco.c | 8 ++++++++ > > > > 2 files changed, 10 insertions(+) > > > > > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > > > index 9125effbf4483d..922cc03143def4 100644 > > > > --- a/include/net/bluetooth/bluetooth.h > > > > +++ b/include/net/bluetooth/bluetooth.h > > > > @@ -153,6 +153,8 @@ struct bt_voice { > > > > > > > > #define BT_SCM_PKT_STATUS 0x03 > > > > > > > > +#define BT_SCO_PKT_LEN 17 > > > > + > > > > __printf(1, 2) > > > > void bt_info(const char *fmt, ...); > > > > __printf(1, 2) > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > > > > --- a/net/bluetooth/sco.c > > > > +++ b/net/bluetooth/sco.c > > > > @@ -67,6 +67,7 @@ struct sco_pinfo { > > > > __u32 flags; > > > > __u16 setting; > > > > __u8 cmsg_mask; > > > > + __u32 pkt_len; > > > > struct sco_conn *conn; > > > > }; > > > > > > > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > > > > sco_sock_set_timer(sk, sk->sk_sndtimeo); > > > > } > > > > > > > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > > > > + > > > > done: > > > > hci_dev_unlock(hdev); > > > > hci_dev_put(hdev); > > > > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > > > > err = -EFAULT; > > > > break; > > > > > > > > + case BT_SCO_PKT_LEN: > > > > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > > > > + err = -EFAULT; > > > > + break; > > > > > > Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? > > > > Hello! > > > > There is already SCO_OPTIONS sock option, uses struct sco_options and > > contains 'mtu' member. > > > > I think that instead of adding new sock option, existing SCO_OPTIONS > > option should be used. > > We are moving away from type specific options to so options like > BT_SNDMTU/BT_RCVMTU should be supported in all socket types. Yes, this make sense. But I guess that SCO_OPTIONS should be provided for backward compatibility as it is already used by lot of userspace applications. So for me it looks like that BT_SNDMTU/BT_RCVMTU should return same value as SCO_OPTIONS. > > > > > > default: > > > > err = -ENOPROTOOPT; > > > > break; > > > > -- > > > > 2.28.0.236.gb10cc79966-goog > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option 2020-08-19 18:23 ` Pali Rohár @ 2020-08-19 18:25 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 11+ messages in thread From: Luiz Augusto von Dentz @ 2020-08-19 18:25 UTC (permalink / raw) To: Pali Rohár Cc: Joseph Hwang, linux-bluetooth, Marcel Holtmann, Joseph Hwang, ChromeOS Bluetooth Upstreaming, Alain Michaud, Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski, Johan Hedberg, Linux Kernel Mailing List, open list:NETWORKING [GENERAL] On Wed, Aug 19, 2020 at 11:23 AM Pali Rohár <pali@kernel.org> wrote: > > On Wednesday 19 August 2020 11:21:00 Luiz Augusto von Dentz wrote: > > Hi Pali, > > > > On Wed, Aug 19, 2020 at 7:37 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > On Friday 14 August 2020 12:56:05 Luiz Augusto von Dentz wrote: > > > > Hi Joseph, > > > > > > > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > > > > > > > It is desirable to expose the wideband speech packet length via > > > > > a socket option to the user space so that the user space can set > > > > > the value correctly in configuring the sco connection. > > > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > > > > --- > > > > > > > > > > include/net/bluetooth/bluetooth.h | 2 ++ > > > > > net/bluetooth/sco.c | 8 ++++++++ > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > > > > index 9125effbf4483d..922cc03143def4 100644 > > > > > --- a/include/net/bluetooth/bluetooth.h > > > > > +++ b/include/net/bluetooth/bluetooth.h > > > > > @@ -153,6 +153,8 @@ struct bt_voice { > > > > > > > > > > #define BT_SCM_PKT_STATUS 0x03 > > > > > > > > > > +#define BT_SCO_PKT_LEN 17 > > > > > + > > > > > __printf(1, 2) > > > > > void bt_info(const char *fmt, ...); > > > > > __printf(1, 2) > > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > > > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > > > > > --- a/net/bluetooth/sco.c > > > > > +++ b/net/bluetooth/sco.c > > > > > @@ -67,6 +67,7 @@ struct sco_pinfo { > > > > > __u32 flags; > > > > > __u16 setting; > > > > > __u8 cmsg_mask; > > > > > + __u32 pkt_len; > > > > > struct sco_conn *conn; > > > > > }; > > > > > > > > > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > > > > > sco_sock_set_timer(sk, sk->sk_sndtimeo); > > > > > } > > > > > > > > > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > > > > > + > > > > > done: > > > > > hci_dev_unlock(hdev); > > > > > hci_dev_put(hdev); > > > > > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > > > > > err = -EFAULT; > > > > > break; > > > > > > > > > > + case BT_SCO_PKT_LEN: > > > > > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > > > > > + err = -EFAULT; > > > > > + break; > > > > > > > > Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? > > > > > > Hello! > > > > > > There is already SCO_OPTIONS sock option, uses struct sco_options and > > > contains 'mtu' member. > > > > > > I think that instead of adding new sock option, existing SCO_OPTIONS > > > option should be used. > > > > We are moving away from type specific options to so options like > > BT_SNDMTU/BT_RCVMTU should be supported in all socket types. > > Yes, this make sense. > > But I guess that SCO_OPTIONS should be provided for backward > compatibility as it is already used by lot of userspace applications. > > So for me it looks like that BT_SNDMTU/BT_RCVMTU should return same > value as SCO_OPTIONS. Yep, luckily we can do this here because SCO MTU is symmetric. > > > > > > > > default: > > > > > err = -ENOPROTOOPT; > > > > > break; > > > > > -- > > > > > 2.28.0.236.gb10cc79966-goog > > > > > > > > > > > > > > > > > -- > > > > Luiz Augusto von Dentz > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular 2020-08-13 8:41 [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular Joseph Hwang 2020-08-13 8:41 ` [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts Joseph Hwang 2020-08-13 8:41 ` [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option Joseph Hwang @ 2020-08-19 2:48 ` Shyh-In Hwang 2 siblings, 0 replies; 11+ messages in thread From: Shyh-In Hwang @ 2020-08-19 2:48 UTC (permalink / raw) To: linux-bluetooth, marcel, luiz.dentz Cc: Joseph Hwang, chromeos-bluetooth-upstreaming, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev Dear maintainers: These two patches are to expose the WBS sco packet length to the user space. Since different vendors may choose different USB alternate settings which result in different packet lengths, we need the kernel to expose the lengths to the user space to handle the packets properly. Thanks! Joseph On Thu, Aug 13, 2020 at 4:42 PM Joseph Hwang <josephsih@chromium.org> wrote: > > USB alternate seeting of which the packet size is distinct. > The patches are to expose the packet size to user space so that > the user space does not need to hard code those values. > > We have verified this patch on Chromebooks which use > - Realtek 8822CE controller with USB alt setting 1 > - Intel controller with USB alt setting 6 > Our user space audio server, cras, can get the correct > packet length from the socket option. > > > Joseph Hwang (2): > Bluetooth: btusb: define HCI packet sizes of USB Alts > Bluetooth: sco: expose WBS packet length in socket option > > drivers/bluetooth/btusb.c | 43 +++++++++++++++++++++++-------- > include/net/bluetooth/bluetooth.h | 2 ++ > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/sco.c | 8 ++++++ > 4 files changed, 43 insertions(+), 11 deletions(-) > > -- > 2.28.0.236.gb10cc79966-goog > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-08-19 18:25 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-13 8:41 [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular Joseph Hwang 2020-08-13 8:41 ` [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts Joseph Hwang 2020-08-14 20:07 ` Luiz Augusto von Dentz 2020-08-19 14:38 ` Pali Rohár 2020-08-13 8:41 ` [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option Joseph Hwang 2020-08-14 19:56 ` Luiz Augusto von Dentz 2020-08-19 14:37 ` Pali Rohár 2020-08-19 18:21 ` Luiz Augusto von Dentz 2020-08-19 18:23 ` Pali Rohár 2020-08-19 18:25 ` Luiz Augusto von Dentz 2020-08-19 2:48 ` [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular Shyh-In Hwang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).