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

[PATCH v3 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 v3:
- Set hdev->sco_mtu to rp->sco_mtu if the latter is smaller.
- Fixed the commit message.

Changes in v2:
- Used sco_mtu instead of a new sco_pkt_len member in hdev.
- Do not overwrite hdev->sco_mtu in hci_cc_read_buffer_size
  if it has been set in the USB interface.
- Used BT_SNDMTU/BT_RCVMTU instead of creating a new opt name.
- Used the existing conn->mtu instead of creating a new member
  in struct sco_pinfo.
- 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: new getsockopt options BT_SNDMTU/BT_RCVMTU

 drivers/bluetooth/btusb.c | 45 +++++++++++++++++++++++++++++----------
 net/bluetooth/hci_event.c | 14 +++++++++++-
 net/bluetooth/sco.c       |  6 ++++++
 3 files changed, 53 insertions(+), 12 deletions(-)

-- 
2.28.0.618.gf4bc123cb7-goog

[PATCH v3 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 v3:
- Set hdev->sco_mtu to rp->sco_mtu if the latter is smaller.

Changes in v2:
- Used sco_mtu instead of a new sco_pkt_len member in hdev.
- Do not overwrite hdev->sco_mtu in hci_cc_read_buffer_size
  if it has been set in the USB interface.

 drivers/bluetooth/btusb.c | 45 +++++++++++++++++++++++++++++----------
 net/bluetooth/hci_event.c | 14 +++++++++++-
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index fe80588c7bd3a8..651d5731a6c6cf 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -459,6 +459,24 @@ 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)
+ *   where suggested_max_packet_size is {9, 17, 25, 33, 49, 63}
+ *   for alt settings 1 - 6.
+ */
+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 +3977,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 +4049,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 +4094,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 +4105,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..1869dc7ebbb5ac 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -730,7 +730,19 @@ 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;
+	/* If the host controller interface is USB, the hdev->sco_mtu is
+	 * set in btusb.c and is not expected to be larger than the max sco
+	 * buffer size returned from the controller in rp->sco_mtu.
+	 */
+	if (hdev->sco_mtu > 0) {
+		if (rp->sco_mtu < hdev->sco_mtu) {
+			BT_ERR("sco mtu %d changed to max sco buffer size %d",
+			       hdev->sco_mtu, rp->sco_mtu);
+			hdev->sco_mtu = rp->sco_mtu;
+		}
+	} else {
+		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.618.gf4bc123cb7-goog

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

[Pali Rohár]

On Thursday 10 September 2020 14:04: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 v3:
> - Set hdev->sco_mtu to rp->sco_mtu if the latter is smaller.
> 
> Changes in v2:
> - Used sco_mtu instead of a new sco_pkt_len member in hdev.
> - Do not overwrite hdev->sco_mtu in hci_cc_read_buffer_size
>   if it has been set in the USB interface.
> 
>  drivers/bluetooth/btusb.c | 45 +++++++++++++++++++++++++++++----------
>  net/bluetooth/hci_event.c | 14 +++++++++++-
>  2 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index fe80588c7bd3a8..651d5731a6c6cf 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -459,6 +459,24 @@ 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)
> + *   where suggested_max_packet_size is {9, 17, 25, 33, 49, 63}
> + *   for alt settings 1 - 6.

Thank you for update, now I see what you mean!

> + */
> +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60 };

Now the another question, why you are using hci_packet_size_usb_alt[1]
and hci_packet_size_usb_alt[6] values from this array?

> +
>  struct btusb_data {
>  	struct hci_dev       *hdev;
>  	struct usb_device    *udev;
> @@ -3959,6 +3977,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 +4049,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];

Why you are setting this sco_mtu only for Intel adapter? Is not this
whole code generic to USB?

> +
>  		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 +4094,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 +4105,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];

And this part of code which you write is Realtek specific.

I thought that this is something generic to bluetooth usb as you pointed
to bluetooth documentation "core spec 5, vol 4, part B, table 2.1".

> +		} else
>  			bt_dev_err(hdev, "Device does not support ALT setting 1");
>  	}

Also this patch seems to be for me incomplete or not fully correct as
USB altsetting is chosen in function btusb_work() and it depends on
selected AIR mode (which is configured by another setsockopt).

So despite what is written in commit message, this patch looks for me
like some hack for Intel and Realtek bluetooth adapters and does not
solve problems in vendor independent manner.

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

[Joseph Hwang]

On Thu, Sep 10, 2020 at 4:18 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 10 September 2020 14:04: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 v3:
> > - Set hdev->sco_mtu to rp->sco_mtu if the latter is smaller.
> >
> > Changes in v2:
> > - Used sco_mtu instead of a new sco_pkt_len member in hdev.
> > - Do not overwrite hdev->sco_mtu in hci_cc_read_buffer_size
> >   if it has been set in the USB interface.
> >
> >  drivers/bluetooth/btusb.c | 45 +++++++++++++++++++++++++++++----------
> >  net/bluetooth/hci_event.c | 14 +++++++++++-
> >  2 files changed, 47 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index fe80588c7bd3a8..651d5731a6c6cf 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -459,6 +459,24 @@ 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)
> > + *   where suggested_max_packet_size is {9, 17, 25, 33, 49, 63}
> > + *   for alt settings 1 - 6.
>
> Thank you for update, now I see what you mean!
>
> > + */
> > +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60 };
>
> Now the another question, why you are using hci_packet_size_usb_alt[1]
> and hci_packet_size_usb_alt[6] values from this array?

Will answer it per the spec in the next patch series.

>
> > +
> >  struct btusb_data {
> >       struct hci_dev       *hdev;
> >       struct usb_device    *udev;
> > @@ -3959,6 +3977,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 +4049,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];
>
> Why you are setting this sco_mtu only for Intel adapter? Is not this
> whole code generic to USB?

Please refer to the answer to the Realtek adapter below. Thanks.

>
> > +
> >               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 +4094,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 +4105,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];
>
> And this part of code which you write is Realtek specific.

We currently only have Intel and Realtek platforms to test with. If
making it generic without proper testing platforms is fine, I will
make it generic. Or do you think it might be better to make it
customized with particular vendors for now; and make it generic later
when it works well with sufficient vendors?

>
> I thought that this is something generic to bluetooth usb as you pointed
> to bluetooth documentation "core spec 5, vol 4, part B, table 2.1".
>
> > +             } else
> >                       bt_dev_err(hdev, "Device does not support ALT setting 1");
> >       }
>
> Also this patch seems to be for me incomplete or not fully correct as
> USB altsetting is chosen in function btusb_work() and it depends on
> selected AIR mode (which is configured by another setsockopt).
>
> So despite what is written in commit message, this patch looks for me
> like some hack for Intel and Realtek bluetooth adapters and does not
> solve problems in vendor independent manner.

You are right that sco_mtu should be changed according to the air
mode. Here are some issues to handle and what I plan to do. I would
like to solicit your comments before I submit the next series.

[Issue 1] The air mode is determined in btusb_work() which is
triggered by hci_sync_conn_complete_evt(). So “conn->mtu =
hdev->sco_mtu” should not be done in  sco_conn_add() in the early
connecting stage. Instead, it will be moved to near the end of
hci_sync_conn_complete_evt().

[Issue 2] The btusb_work() is performed by a worker. There would be a
timing issue here if we let btusb_work() to do “hdev->sco_mtu =
hci_packet_size_usb_alt[i]” because there is no guarantee how soon the
btusb_work() can be finished and get “hdev->sco_mtu” value set
correctly. In order to avoid the potential race condition, I suggest
to determine air_mode in btusb_notify() before
schedule_work(&data->work) is executed so that “hdev->sco_mtu =
hci_packet_size_usb_alt[i]” is guaranteed to be performed when
btusb_notify() finished. In this way, hci_sync_conn_complete_evt() can
set conn->mtu correctly as described in [Issue 1] above.

[Issue 3] Concerning CVSD: The above flow is good when the transparent
mode is selected. When it is the CVSD mode, we should set
hdev->sco_mtu and conn->mtu back to the original mtu size returned by
hci_cc_read_buffer_size(). This is because we do not have a reliable
way to determine what size is used with CVSD. AFAIK, controllers
connected through USB use 48 bytes; and controllers connected through
UART use 60 bytes. It seems to me that these numbers are not recorded
in the kernel(?). It seems beyond the scope of this patch to set the
proper value for CVSD. So we will let hdev->sco_mtu and conn->mtu go
back to their original values and are not affected by this patch.

I am wondering if such directions of fixing this patch look good to you?

Thanks and regards!
Joseph



-- 

Joseph Shyh-In Hwang
Email: josephsih@google.com

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

[Pali Rohár]

Hello!

On Monday 14 September 2020 20:18:27 Joseph Hwang wrote:
> On Thu, Sep 10, 2020 at 4:18 PM Pali Rohár <pali@kernel.org> wrote:
> > And this part of code which you write is Realtek specific.
> 
> We currently only have Intel and Realtek platforms to test with. If
> making it generic without proper testing platforms is fine, I will
> make it generic. Or do you think it might be better to make it
> customized with particular vendors for now; and make it generic later
> when it works well with sufficient vendors?

I understood that those packet size changes are generic to bluetooth
specification and therefore it is not vendor specific code. Those packet
sizes for me really seems to be USB specific.

Therefore it should apply for all vendors, not only for Realtek and
Intel.

I would propose to fix it globally for all USB adapters and not only for
Intel and Realtek USB adapters.

> >
> > I thought that this is something generic to bluetooth usb as you pointed
> > to bluetooth documentation "core spec 5, vol 4, part B, table 2.1".
> >
> > > +             } else
> > >                       bt_dev_err(hdev, "Device does not support ALT setting 1");
> > >       }
> >
> > Also this patch seems to be for me incomplete or not fully correct as
> > USB altsetting is chosen in function btusb_work() and it depends on
> > selected AIR mode (which is configured by another setsockopt).
> >
> > So despite what is written in commit message, this patch looks for me
> > like some hack for Intel and Realtek bluetooth adapters and does not
> > solve problems in vendor independent manner.
> 
> You are right that sco_mtu should be changed according to the air
> mode. Here are some issues to handle and what I plan to do. I would
> like to solicit your comments before I submit the next series.
> 
> [Issue 1] The air mode is determined in btusb_work() which is
> triggered by hci_sync_conn_complete_evt(). So “conn->mtu =
> hdev->sco_mtu” should not be done in  sco_conn_add() in the early
> connecting stage. Instead, it will be moved to near the end of
> hci_sync_conn_complete_evt().
> 
> [Issue 2] The btusb_work() is performed by a worker. There would be a
> timing issue here if we let btusb_work() to do “hdev->sco_mtu =
> hci_packet_size_usb_alt[i]” because there is no guarantee how soon the
> btusb_work() can be finished and get “hdev->sco_mtu” value set
> correctly. In order to avoid the potential race condition, I suggest
> to determine air_mode in btusb_notify() before
> schedule_work(&data->work) is executed so that “hdev->sco_mtu =
> hci_packet_size_usb_alt[i]” is guaranteed to be performed when
> btusb_notify() finished. In this way, hci_sync_conn_complete_evt() can
> set conn->mtu correctly as described in [Issue 1] above.

If it really fixes this issue, I'm fine with it.

You have Realtek and Intel HW for testing, so I think if doing some
heavy tests did not trigger any issue / race condition then it should be
fine.

> [Issue 3] Concerning CVSD: The above flow is good when the transparent
> mode is selected. When it is the CVSD mode, we should set
> hdev->sco_mtu and conn->mtu back to the original mtu size returned by
> hci_cc_read_buffer_size().

Beware that kernel does not support CVSD mode between (USB) bluetooth
adapter and kernel yet. Therefore it does not make sense to discussion
this option.

MTU size affects packets between kernel (or userspace) and bluetooth
adapter. Bluetooth adapter then do HW encoding/decoding if
non-transparent mode is used.

So I think you rather mean PCMs16 mode between bluetooth adapter and
kernel because this mode is used for CVSD air codec (between two
bluetooth adapters over the air). This is default mode, userspace
application pass PCMs16 data to kernel which pass it over USB to
bluetooth adapter. And bluetooth adapter then do HW encoding, encodes
PCMs16 to CVSD codec and transmit it over the air. So over USB are sent
PCM data, not CVSD.

> This is because we do not have a reliable
> way to determine what size is used with CVSD. AFAIK, controllers
> connected through USB use 48 bytes; and controllers connected through
> UART use 60 bytes.

I think for USB adapters we have that size. From bluetooth specification
I understood that it is that size which you calculated in that array
based on alt interface.

I understood that bluetooth specification that packet size on USB
depends only on chosen alt interface. And choice of alt interface
depends on voice codec (PCM or transparent) and on number of active
transports.

But correct me, if I'm wrong. Maybe I did not understand bluetooth
specification correctly.

And for UART I do not know, I have not looked at it it yet.

> It seems to me that these numbers are not recorded
> in the kernel(?). It seems beyond the scope of this patch to set the
> proper value for CVSD. So we will let hdev->sco_mtu and conn->mtu go
> back to their original values and are not affected by this patch.
> 
> I am wondering if such directions of fixing this patch look good to you?
> 
> Thanks and regards!
> Joseph

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

[Trent Piepho]

On Wednesday, September 23, 2020 3:22:15 AM PST Pali Rohár wrote:
> On Monday 14 September 2020 20:18:27 Joseph Hwang wrote:
> > On Thu, Sep 10, 2020 at 4:18 PM Pali Rohár <pali@kernel.org> wrote:
> > > And this part of code which you write is Realtek specific.
> > 
> > We currently only have Intel and Realtek platforms to test with. If
> > making it generic without proper testing platforms is fine, I will
> > make it generic. Or do you think it might be better to make it
> > customized with particular vendors for now; and make it generic later
> > when it works well with sufficient vendors?
> 
> I understood that those packet size changes are generic to bluetooth
> specification and therefore it is not vendor specific code. Those packet
> sizes for me really seems to be USB specific.
> 
> Therefore it should apply for all vendors, not only for Realtek and
> Intel.

I have tried to test WBS with some different USB adapters.  So far, all use 
these packet sizes.  Tested were:

Broadcom BRCM20702A
Realtek RTL8167B
Realtek RTL8821C
CSR CSR8510 (probably fake)

In all cases, WBS works best with packet size of (USB packet size for alt mode 
selected) * 3 packets - 3 bytes HCI header.  None of these devices support alt 
6 mode, where supposedly one packet is better, but I can find no BT adapter on 
which to test this.

> +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60};

Note that the packet sizes here are based on the max isoc packet length for 
the USB alt mode used, e.g. alt 1 is 9 bytes.  That value is only a 
"recommended" value from the bluetooth spec.  It seems like it would be more 
correct use (btusb_data*)->isoc_tx_ep->wMaxPacketSize to find the MTU.

> > [Issue 2] The btusb_work() is performed by a worker. There would be a
> > timing issue here if we let btusb_work() to do “hdev->sco_mtu =
> > hci_packet_size_usb_alt[i]” because there is no guarantee how soon the
> > btusb_work() can be finished and get “hdev->sco_mtu” value set
> > correctly. In order to avoid the potential race condition, I suggest
> > to determine air_mode in btusb_notify() before
> > schedule_work(&data->work) is executed so that “hdev->sco_mtu =
> > hci_packet_size_usb_alt[i]” is guaranteed to be performed when
> > btusb_notify() finished. In this way, hci_sync_conn_complete_evt() can
> > set conn->mtu correctly as described in [Issue 1] above.

Does this also give userspace a clear point at which to determine MTU setting, 
_before_ data is sent over SCO connection?  It will not work if sco_mtu is not 
valid until after userspace sends data to SCO connection with incorrect mtu.

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

[Pali Rohár]

On Tuesday 08 December 2020 15:04:29 Trent Piepho wrote:
> On Wednesday, September 23, 2020 3:22:15 AM PST Pali Rohár wrote:
> > On Monday 14 September 2020 20:18:27 Joseph Hwang wrote:
> > > On Thu, Sep 10, 2020 at 4:18 PM Pali Rohár <pali@kernel.org> wrote:
> > > > And this part of code which you write is Realtek specific.
> > > 
> > > We currently only have Intel and Realtek platforms to test with. If
> > > making it generic without proper testing platforms is fine, I will
> > > make it generic. Or do you think it might be better to make it
> > > customized with particular vendors for now; and make it generic later
> > > when it works well with sufficient vendors?
> > 
> > I understood that those packet size changes are generic to bluetooth
> > specification and therefore it is not vendor specific code. Those packet
> > sizes for me really seems to be USB specific.
> > 
> > Therefore it should apply for all vendors, not only for Realtek and
> > Intel.
> 
> I have tried to test WBS with some different USB adapters.  So far, all use 
> these packet sizes.  Tested were:
> 
> Broadcom BRCM20702A
> Realtek RTL8167B
> Realtek RTL8821C
> CSR CSR8510 (probably fake)
> 
> In all cases, WBS works best with packet size of (USB packet size for alt mode 
> selected) * 3 packets - 3 bytes HCI header.  None of these devices support alt 
> 6 mode, where supposedly one packet is better, but I can find no BT adapter on 
> which to test this.
> 
> > +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60};
> 
> Note that the packet sizes here are based on the max isoc packet length for 
> the USB alt mode used, e.g. alt 1 is 9 bytes.  That value is only a 
> "recommended" value from the bluetooth spec.  It seems like it would be more 
> correct use (btusb_data*)->isoc_tx_ep->wMaxPacketSize to find the MTU.

Yea, wMaxPacketSize looks like a candidate for determining MTU. Can we
use it or are there any known issues with it?

> > > [Issue 2] The btusb_work() is performed by a worker. There would be a
> > > timing issue here if we let btusb_work() to do “hdev->sco_mtu =
> > > hci_packet_size_usb_alt[i]” because there is no guarantee how soon the
> > > btusb_work() can be finished and get “hdev->sco_mtu” value set
> > > correctly. In order to avoid the potential race condition, I suggest
> > > to determine air_mode in btusb_notify() before
> > > schedule_work(&data->work) is executed so that “hdev->sco_mtu =
> > > hci_packet_size_usb_alt[i]” is guaranteed to be performed when
> > > btusb_notify() finished. In this way, hci_sync_conn_complete_evt() can
> > > set conn->mtu correctly as described in [Issue 1] above.
> 
> Does this also give userspace a clear point at which to determine MTU setting, 
> _before_ data is sent over SCO connection?  It will not work if sco_mtu is not 
> valid until after userspace sends data to SCO connection with incorrect mtu.

IIRC connection is established after sync connection (SCO) complete
event. And sending data is possible after connection is established. So
based on these facts I think that userspace can determinate MTU settings
prior sending data over SCO socket.

Anyway, to whole MTU issue for SCO there is a nice workaround which
worked fine with more tested USB adapters and headsets. As SCO socket is
synchronous and most bluetooth headsets have own clocks, you can
synchronize sending packets to headsets based on time events when you
received packets from other side and also send packets of same size as
you received. I.e. for every received packet send own packet of the same
size.

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

[Trent Piepho]

On Tuesday, December 8, 2020 5:13:36 PM PST Pali Rohár wrote:
> On Tuesday 08 December 2020 15:04:29 Trent Piepho wrote:
> > Does this also give userspace a clear point at which to determine MTU 
setting, 
> > _before_ data is sent over SCO connection?  It will not work if sco_mtu 
is not 
> > valid until after userspace sends data to SCO connection with incorrect 
mtu.
> 
> IIRC connection is established after sync connection (SCO) complete
> event. And sending data is possible after connection is established. So
> based on these facts I think that userspace can determinate MTU settings
> prior sending data over SCO socket.
> 
> Anyway, to whole MTU issue for SCO there is a nice workaround which
> worked fine with more tested USB adapters and headsets. As SCO socket is
> synchronous and most bluetooth headsets have own clocks, you can
> synchronize sending packets to headsets based on time events when you
> received packets from other side and also send packets of same size as
> you received. I.e. for every received packet send own packet of the same
> size.

As I understand it, the RX side from the headset is not guaranteed, so in 
the TX only case this will not work and we still need to be told what MTU 
kernel has selected for the SCO link.

It seems also it would add some latency to start up, since it would be 
necessary to wait for packets to arrive before knowing what size packet to 
send.

Would timing based on matching TX to RX in the case of packet loss on RX 
side?

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

[Pali Rohár]

On Wednesday 09 December 2020 16:19:39 Trent Piepho wrote:
> On Tuesday, December 8, 2020 5:13:36 PM PST Pali Rohár wrote:
> > On Tuesday 08 December 2020 15:04:29 Trent Piepho wrote:
> > > Does this also give userspace a clear point at which to determine MTU 
> setting, 
> > > _before_ data is sent over SCO connection?  It will not work if sco_mtu 
> is not 
> > > valid until after userspace sends data to SCO connection with incorrect 
> mtu.
> > 
> > IIRC connection is established after sync connection (SCO) complete
> > event. And sending data is possible after connection is established. So
> > based on these facts I think that userspace can determinate MTU settings
> > prior sending data over SCO socket.
> > 
> > Anyway, to whole MTU issue for SCO there is a nice workaround which
> > worked fine with more tested USB adapters and headsets. As SCO socket is
> > synchronous and most bluetooth headsets have own clocks, you can
> > synchronize sending packets to headsets based on time events when you
> > received packets from other side and also send packets of same size as
> > you received. I.e. for every received packet send own packet of the same
> > size.
> 
> As I understand it, the RX side from the headset is not guaranteed, so in 
> the TX only case this will not work and we still need to be told what MTU 
> kernel has selected for the SCO link.

I'm not sure if TX-only SCO link is possible. I always thought that SCO
is synchronous bidirectional link.

As I said, this "workaround" is useful for classic bluetooth headsets
and is it possible to use it immediately without any kernel changes.

And I agree that kernel should tell userspace correct MTU value. And
this should be fixed. "Workaround" is useful for immediate action to
deliver at least something which works with most bluetooth headsets.

> It seems also it would add some latency to start up, since it would be 
> necessary to wait for packets to arrive before knowing what size packet to 
> send.

I think this startup latency is negligible in HFP profile where start
needs non-trivial exchange of AT commands.

> Would timing based on matching TX to RX in the case of packet loss on RX 
> side?

That is a good question for some research. I remember that e.g.
pulseaudio used this technique for synchronizing bluetooth SCO RX and TX
streams.

[PATCH v3 2/2] Bluetooth: sco: new getsockopt options BT_SNDMTU/BT_RCVMTU

[Joseph Hwang]

This patch defines new getsockopt options BT_SNDMTU/BT_RCVMTU
for SCO socket to be compatible with other bluetooth sockets.
These new options return the 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>
---

Changes in v3:
- Fixed the commit message.

Changes in v2:
- Used BT_SNDMTU/BT_RCVMTU instead of creating a new opt name.
- Used the existing conn->mtu instead of creating a new member
  in struct sco_pinfo.
- Noted that the old SCO_OPTIONS in sco_sock_getsockopt_old()
  would just work as it uses sco_pi(sk)->conn->mtu.

 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.618.gf4bc123cb7-goog

Re: [PATCH v3 2/2] Bluetooth: sco: new getsockopt options BT_SNDMTU/BT_RCVMTU

[Pali Rohár]

On Thursday 10 September 2020 14:04:02 Joseph Hwang wrote:
> This patch defines new getsockopt options BT_SNDMTU/BT_RCVMTU
> for SCO socket to be compatible with other bluetooth sockets.
> These new options return the 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>

Looks good,

Reviewed-by: Pali Rohár <pali@kernel.org>

> ---
> 
> Changes in v3:
> - Fixed the commit message.
> 
> Changes in v2:
> - Used BT_SNDMTU/BT_RCVMTU instead of creating a new opt name.
> - Used the existing conn->mtu instead of creating a new member
>   in struct sco_pinfo.
> - Noted that the old SCO_OPTIONS in sco_sock_getsockopt_old()
>   would just work as it uses sco_pi(sk)->conn->mtu.
> 
>  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.618.gf4bc123cb7-goog
> 

Re: [PATCH v3 2/2] Bluetooth: sco: new getsockopt options BT_SNDMTU/BT_RCVMTU

[Marcel Holtmann]

Hi Joseph,

> This patch defines new getsockopt options BT_SNDMTU/BT_RCVMTU
> for SCO socket to be compatible with other bluetooth sockets.
> These new options return the 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>
> ---
> 
> Changes in v3:
> - Fixed the commit message.
> 
> Changes in v2:
> - Used BT_SNDMTU/BT_RCVMTU instead of creating a new opt name.
> - Used the existing conn->mtu instead of creating a new member
>  in struct sco_pinfo.
> - Noted that the old SCO_OPTIONS in sco_sock_getsockopt_old()
>  would just work as it uses sco_pi(sk)->conn->mtu.
> 
> net/bluetooth/sco.c | 6 ++++++
> 1 file changed, 6 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel