[PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular

[PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular

[Joseph Hwang]

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

[PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts

[Joseph Hwang]

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

Re: [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts

[Luiz Augusto von Dentz]

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

Re: [PATCH v1 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts

[Pali Rohár]

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

[PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option

[Joseph Hwang]

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

Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option

[Luiz Augusto von Dentz]

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

Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option

[Pali Rohár]

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

Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option

[Luiz Augusto von Dentz]

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

Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option

[Pali Rohár]

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

Re: [PATCH v1 2/2] Bluetooth: sco: expose WBS packet length in socket option

[Luiz Augusto von Dentz]

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

Re: [PATCH v1 0/2] To support the HFP WBS, a chip vendor may choose a particular

[Shyh-In Hwang]

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
>