diff mbox series

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

Message ID 20200813164059.v1.1.I56de28ec171134cb9f97062e2c304a72822ca38b@changeid (mailing list archive)
State New, archived
Headers show
Series To support the HFP WBS, a chip vendor may choose a particular | expand

Commit Message

Joseph Hwang Aug. 13, 2020, 8:41 a.m. UTC 
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(-)

Comments

Luiz Augusto von Dentz Aug. 14, 2020, 8:07 p.m. UTC | #1 
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
>
Pali Rohár Aug. 19, 2020, 2:38 p.m. UTC | #2 
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
diff mbox series

Patch

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;