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

[PATCH v2 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.

Changes in v2:
- 1/2: Used sco_mtu instead of a new sco_pkt_len member in hdev.
- 1/2: Do not overwrite hdev->sco_mtu in hci_cc_read_buffer_size
       if it has been set in the USB interface.
- 2/2: Used BT_SNDMTU/BT_RCVMTU instead of creating a new opt name.
- 2/2: Used the existing conn->mtu instead of creating a new member
       in struct sco_pinfo.
- 2/2: Noted that the old SCO_OPTIONS in sco_sock_getsockopt_old()
       would just work as it uses sco_pi(sk)->conn->mtu.

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 +++++++++++++++++++++++++++++----------
 net/bluetooth/hci_event.c |  7 ++++++-
 net/bluetooth/sco.c       |  6 ++++++
 3 files changed, 44 insertions(+), 12 deletions(-)

-- 
2.28.0.526.ge36021eeef-goog

[PATCH v2 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>
---

Changes in v2:
- 1/2: Used sco_mtu instead of a new sco_pkt_len member in hdev.
- 1/2: Do not overwrite hdev->sco_mtu in hci_cc_read_buffer_size
  if it has been set in the USB interface.
- 2/2: Used BT_SNDMTU/BT_RCVMTU instead of creating a new opt name.
- 2/2: Used the existing conn->mtu instead of creating a new member
       in struct sco_pinfo.
- 2/2: Noted that the old SCO_OPTIONS in sco_sock_getsockopt_old()
       would just work as it uses sco_pi(sk)->conn->mtu.

 drivers/bluetooth/btusb.c | 43 +++++++++++++++++++++++++++++----------
 net/bluetooth/hci_event.c |  7 ++++++-
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index fe80588c7bd3a8..a710233382afff 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;
@@ -3959,6 +3975,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)
@@ -4022,6 +4047,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_mtu = 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);
@@ -4063,15 +4092,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;
@@ -4083,9 +4103,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_mtu = hci_packet_size_usb_alt[1];
+		} else
 			bt_dev_err(hdev, "Device does not support ALT setting 1");
 	}
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 33d8458fdd4adc..b3e9247fdbcfa1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -730,7 +730,12 @@ static void hci_cc_read_buffer_size(struct hci_dev *hdev, struct sk_buff *skb)
 		return;
 
 	hdev->acl_mtu  = __le16_to_cpu(rp->acl_mtu);
-	hdev->sco_mtu  = rp->sco_mtu;
+	/* Set sco_mtu only when not yet.
+	 * The sco_mtu would be set in btusb.c
+	 * if the host controller interface is USB.
+	 */
+	if (hdev->sco_mtu == 0)
+		hdev->sco_mtu  = rp->sco_mtu;
 	hdev->acl_pkts = __le16_to_cpu(rp->acl_max_pkt);
 	hdev->sco_pkts = __le16_to_cpu(rp->sco_max_pkt);
 
-- 
2.28.0.526.ge36021eeef-goog

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

[Pali Rohár]

On Wednesday 09 September 2020 17:42:01 Joseph Hwang 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>
> ---
> 
> Changes in v2:
> - 1/2: Used sco_mtu instead of a new sco_pkt_len member in hdev.
> - 1/2: Do not overwrite hdev->sco_mtu in hci_cc_read_buffer_size
>   if it has been set in the USB interface.
> - 2/2: Used BT_SNDMTU/BT_RCVMTU instead of creating a new opt name.
> - 2/2: Used the existing conn->mtu instead of creating a new member
>        in struct sco_pinfo.
> - 2/2: Noted that the old SCO_OPTIONS in sco_sock_getsockopt_old()
>        would just work as it uses sco_pi(sk)->conn->mtu.
> 
>  drivers/bluetooth/btusb.c | 43 +++++++++++++++++++++++++++++----------
>  net/bluetooth/hci_event.c |  7 ++++++-
>  2 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index fe80588c7bd3a8..a710233382afff 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)

Hello! What is value for 'suggested_max_packet_size' used in above
calculation algorithm?

> + */
> +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;
> @@ -3959,6 +3975,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)
> @@ -4022,6 +4047,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_mtu = 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);
> @@ -4063,15 +4092,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;
> @@ -4083,9 +4103,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_mtu = hci_packet_size_usb_alt[1];
> +		} else
>  			bt_dev_err(hdev, "Device does not support ALT setting 1");
>  	}
>  
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 33d8458fdd4adc..b3e9247fdbcfa1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -730,7 +730,12 @@ static void hci_cc_read_buffer_size(struct hci_dev *hdev, struct sk_buff *skb)
>  		return;
>  
>  	hdev->acl_mtu  = __le16_to_cpu(rp->acl_mtu);
> -	hdev->sco_mtu  = rp->sco_mtu;
> +	/* Set sco_mtu only when not yet.
> +	 * The sco_mtu would be set in btusb.c
> +	 * if the host controller interface is USB.
> +	 */
> +	if (hdev->sco_mtu == 0)
> +		hdev->sco_mtu  = rp->sco_mtu;

Should not hdev->sco_mtu contains minimum from the rp->sco_mtu and your
calculated value defined in hci_packet_size_usb_alt[i]?

Socket MTU cannot be bigger then value rp->sco_mtu, right?

>  	hdev->acl_pkts = __le16_to_cpu(rp->acl_max_pkt);
>  	hdev->sco_pkts = __le16_to_cpu(rp->sco_max_pkt);
>  
> -- 
> 2.28.0.526.ge36021eeef-goog
> 

[PATCH v2 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>
---

(no changes since v1)

 net/bluetooth/sco.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index dcf7f96ff417e6..79ffcdef0b7ad5 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -1001,6 +1001,12 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 			err = -EFAULT;
 		break;
 
+	case BT_SNDMTU:
+	case BT_RCVMTU:
+		if (put_user(sco_pi(sk)->conn->mtu, (u32 __user *)optval))
+			err = -EFAULT;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
-- 
2.28.0.526.ge36021eeef-goog

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

[Pali Rohár]

On Wednesday 09 September 2020 17:42:02 Joseph Hwang 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.

Hello! I'm fine with change below, but I would suggest to put more
details into commit message. This change has nothing to do with wideband
nor with exporting socket option to userspace -- which is already done
via SCO_OPTIONS option. Also it is relevant to SCO socket with any codec
data, not only wideband.

This commit description should rather mention that it defines new
getsockopt options BT_SNDMTU/BT_RCVMTU for SCO socket to be compatible
with other bluetooth sockets and that these options return same value as
option SCO_OPTIONS which is already present on existing kernels.

> 
> Reviewed-by: Alain Michaud <alainm@chromium.org>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  net/bluetooth/sco.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index dcf7f96ff417e6..79ffcdef0b7ad5 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -1001,6 +1001,12 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
>  			err = -EFAULT;
>  		break;
>  
> +	case BT_SNDMTU:
> +	case BT_RCVMTU:
> +		if (put_user(sco_pi(sk)->conn->mtu, (u32 __user *)optval))
> +			err = -EFAULT;
> +		break;
> +
>  	default:
>  		err = -ENOPROTOOPT;
>  		break;
> -- 
> 2.28.0.526.ge36021eeef-goog
>