[PATCH] Bluetooth: btusb: Handling MSBC encoded Audio stream

[PATCH] Bluetooth: btusb: Handling MSBC encoded Audio stream

[Sathish Narasimman]

Msbc encodec audio stream over USB transport should be directed
through alternate setting 6 as per Bluetooth core spec 5.0.
The patch is made keeping some part of discussion from the below
link.

https://www.spinics.net/lists/linux-bluetooth/msg64577.html

Sathish Narasimman (1):
  Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

 drivers/bluetooth/btusb.c   | 95 +++++++++++++++++++++++++--------------------
 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_event.c   |  5 +++
 3 files changed, 59 insertions(+), 42 deletions(-)

-- 
2.7.4

[PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Sathish Narasimman]

For msbc encoded audio stream over usb transport, btusb driver
to be set to alternate settings 6 as per BT core spec 5.0. This
done from  hci_sync_conn_complete_evt. The type of air mode is known
during this event. For this reason the btusb is to be notifed
about the TRANSPARENT air mode and the ALT setting 6 is selected.
The changes are made considering some discussion over the similar
patch submitted earlier from Kuba Pawlak(link below)
https://www.spinics.net/lists/linux-bluetooth/msg64577.html

Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
---
 drivers/bluetooth/btusb.c   | 95 +++++++++++++++++++++++++--------------------
 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_event.c   |  5 +++
 3 files changed, 59 insertions(+), 42 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index cd2e5cf..ae924b6 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1390,18 +1390,6 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	return -EILSEQ;
 }
 
-static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
-{
-	struct btusb_data *data = hci_get_drvdata(hdev);
-
-	BT_DBG("%s evt %d", hdev->name, evt);
-
-	if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
-		data->sco_num = hci_conn_num(hdev, SCO_LINK);
-		schedule_work(&data->work);
-	}
-}
-
 static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
@@ -1445,6 +1433,58 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
 	return 0;
 }
 
+static void bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+
+	if (data->isoc_altsetting != new_alts) {
+		unsigned long flags;
+
+		clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+		usb_kill_anchored_urbs(&data->isoc_anchor);
+
+		/* When isochronous alternate setting needs to be
+		 * changed, because SCO connection has been added
+		 * or removed, a packet fragment may be left in the
+		 * reassembling state. This could lead to wrongly
+		 * assembled fragments.
+		 *
+		 * Clear outstanding fragment when selecting a new
+		 * alternate setting.
+		 */
+		spin_lock_irqsave(&data->rxlock, flags);
+		kfree_skb(data->sco_skb);
+		data->sco_skb = NULL;
+		spin_unlock_irqrestore(&data->rxlock, flags);
+
+		if (__set_isoc_interface(hdev, new_alts) < 0)
+			return;
+	}
+	if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
+		if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
+			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+		else
+			btusb_submit_isoc_urb(hdev, GFP_KERNEL);
+	}
+}
+
+static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+
+	BT_DBG("%s evt %d", hdev->name, evt);
+
+	if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
+		data->sco_num = hci_conn_num(hdev, SCO_LINK);
+		schedule_work(&data->work);
+	}
+
+	if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
+		/* Alt setting 6 is for msbc encoded audio channel */
+		bt_switch_alt_setting(hdev, 6);
+	}
+}
+
 static void btusb_work(struct work_struct *work)
 {
 	struct btusb_data *data = container_of(work, struct btusb_data, work);
@@ -1472,36 +1512,7 @@ static void btusb_work(struct work_struct *work)
 			new_alts = data->sco_num;
 		}
 
-		if (data->isoc_altsetting != new_alts) {
-			unsigned long flags;
-
-			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
-			usb_kill_anchored_urbs(&data->isoc_anchor);
-
-			/* When isochronous alternate setting needs to be
-			 * changed, because SCO connection has been added
-			 * or removed, a packet fragment may be left in the
-			 * reassembling state. This could lead to wrongly
-			 * assembled fragments.
-			 *
-			 * Clear outstanding fragment when selecting a new
-			 * alternate setting.
-			 */
-			spin_lock_irqsave(&data->rxlock, flags);
-			kfree_skb(data->sco_skb);
-			data->sco_skb = NULL;
-			spin_unlock_irqrestore(&data->rxlock, flags);
-
-			if (__set_isoc_interface(hdev, new_alts) < 0)
-				return;
-		}
-
-		if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
-			if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
-				clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
-			else
-				btusb_submit_isoc_urb(hdev, GFP_KERNEL);
-		}
+		bt_switch_alt_setting(hdev, new_alts);
 	} else {
 		clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
 		usb_kill_anchored_urbs(&data->isoc_anchor);
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index cdd9f1f..3498c6b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -52,6 +52,7 @@
 #define HCI_NOTIFY_CONN_ADD		1
 #define HCI_NOTIFY_CONN_DEL		2
 #define HCI_NOTIFY_VOICE_SETTING	3
+#define HCI_NOTIFY_AIR_MODE_TRANSP	4
 
 /* HCI bus types */
 #define HCI_VIRTUAL	0
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index f12555f..8ef5220 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4100,6 +4100,11 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
 		break;
 	}
 
+	if (ev->air_mode == SCO_AIRMODE_TRANSP) {
+		if (hdev->notify)
+			hdev->notify(hdev, HCI_NOTIFY_AIR_MODE_TRANSP);
+	}
+
 	hci_connect_cfm(conn, ev->status);
 	if (ev->status)
 		hci_conn_del(conn);
-- 
2.7.4

[PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Sathish Narasimman]

For msbc encoded audio stream over usb transport, btusb driver
to be set to alternate settings 6 as per BT core spec 5.0. This
done from  hci_sync_conn_complete_evt. The type of air mode is known
during this event. For this reason the btusb is to be notifed
about the TRANSPARENT air mode and the ALT setting 6 is selected.
The changes are made considering some discussion over the similar
patch submitted earlier from Kuba Pawlak(link below)
https://www.spinics.net/lists/linux-bluetooth/msg64577.html

Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
---
 drivers/bluetooth/btusb.c   | 95 +++++++++++++++++++++++++--------------------
 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_event.c   |  5 +++
 3 files changed, 59 insertions(+), 42 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index cd2e5cf..ae924b6 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1390,18 +1390,6 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	return -EILSEQ;
 }
 
-static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
-{
-	struct btusb_data *data = hci_get_drvdata(hdev);
-
-	BT_DBG("%s evt %d", hdev->name, evt);
-
-	if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
-		data->sco_num = hci_conn_num(hdev, SCO_LINK);
-		schedule_work(&data->work);
-	}
-}
-
 static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
@@ -1445,6 +1433,58 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
 	return 0;
 }
 
+static void bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+
+	if (data->isoc_altsetting != new_alts) {
+		unsigned long flags;
+
+		clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+		usb_kill_anchored_urbs(&data->isoc_anchor);
+
+		/* When isochronous alternate setting needs to be
+		 * changed, because SCO connection has been added
+		 * or removed, a packet fragment may be left in the
+		 * reassembling state. This could lead to wrongly
+		 * assembled fragments.
+		 *
+		 * Clear outstanding fragment when selecting a new
+		 * alternate setting.
+		 */
+		spin_lock_irqsave(&data->rxlock, flags);
+		kfree_skb(data->sco_skb);
+		data->sco_skb = NULL;
+		spin_unlock_irqrestore(&data->rxlock, flags);
+
+		if (__set_isoc_interface(hdev, new_alts) < 0)
+			return;
+	}
+	if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
+		if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
+			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+		else
+			btusb_submit_isoc_urb(hdev, GFP_KERNEL);
+	}
+}
+
+static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+
+	BT_DBG("%s evt %d", hdev->name, evt);
+
+	if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
+		data->sco_num = hci_conn_num(hdev, SCO_LINK);
+		schedule_work(&data->work);
+	}
+
+	if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
+		/* Alt setting 6 is for msbc encoded audio channel */
+		bt_switch_alt_setting(hdev, 6);
+	}
+}
+
 static void btusb_work(struct work_struct *work)
 {
 	struct btusb_data *data = container_of(work, struct btusb_data, work);
@@ -1472,36 +1512,7 @@ static void btusb_work(struct work_struct *work)
 			new_alts = data->sco_num;
 		}
 
-		if (data->isoc_altsetting != new_alts) {
-			unsigned long flags;
-
-			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
-			usb_kill_anchored_urbs(&data->isoc_anchor);
-
-			/* When isochronous alternate setting needs to be
-			 * changed, because SCO connection has been added
-			 * or removed, a packet fragment may be left in the
-			 * reassembling state. This could lead to wrongly
-			 * assembled fragments.
-			 *
-			 * Clear outstanding fragment when selecting a new
-			 * alternate setting.
-			 */
-			spin_lock_irqsave(&data->rxlock, flags);
-			kfree_skb(data->sco_skb);
-			data->sco_skb = NULL;
-			spin_unlock_irqrestore(&data->rxlock, flags);
-
-			if (__set_isoc_interface(hdev, new_alts) < 0)
-				return;
-		}
-
-		if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
-			if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
-				clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
-			else
-				btusb_submit_isoc_urb(hdev, GFP_KERNEL);
-		}
+		bt_switch_alt_setting(hdev, new_alts);
 	} else {
 		clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
 		usb_kill_anchored_urbs(&data->isoc_anchor);
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index cdd9f1f..3498c6b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -52,6 +52,7 @@
 #define HCI_NOTIFY_CONN_ADD		1
 #define HCI_NOTIFY_CONN_DEL		2
 #define HCI_NOTIFY_VOICE_SETTING	3
+#define HCI_NOTIFY_AIR_MODE_TRANSP	4
 
 /* HCI bus types */
 #define HCI_VIRTUAL	0
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index f12555f..8ef5220 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4100,6 +4100,11 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
 		break;
 	}
 
+	if (ev->air_mode == SCO_AIRMODE_TRANSP) {
+		if (hdev->notify)
+			hdev->notify(hdev, HCI_NOTIFY_AIR_MODE_TRANSP);
+	}
+
 	hci_connect_cfm(conn, ev->status);
 	if (ev->status)
 		hci_conn_del(conn);
-- 
2.7.4

Re: [PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Luiz Augusto von Dentz]

Hi Sathish,

On Mon, Aug 20, 2018 at 4:32 PM, Sathish Narasimman
<nsathish41@gmail.com> wrote:
> For msbc encoded audio stream over usb transport, btusb driver
> to be set to alternate settings 6 as per BT core spec 5.0. This
> done from  hci_sync_conn_complete_evt. The type of air mode is known
> during this event. For this reason the btusb is to be notifed
> about the TRANSPARENT air mode and the ALT setting 6 is selected.
> The changes are made considering some discussion over the similar
> patch submitted earlier from Kuba Pawlak(link below)
> https://www.spinics.net/lists/linux-bluetooth/msg64577.html
>
> Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
> ---
>  drivers/bluetooth/btusb.c   | 95 +++++++++++++++++++++++++--------------------
>  include/net/bluetooth/hci.h |  1 +
>  net/bluetooth/hci_event.c   |  5 +++
>  3 files changed, 59 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index cd2e5cf..ae924b6 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1390,18 +1390,6 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>         return -EILSEQ;
>  }
>
> -static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> -{
> -       struct btusb_data *data = hci_get_drvdata(hdev);
> -
> -       BT_DBG("%s evt %d", hdev->name, evt);
> -
> -       if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> -               data->sco_num = hci_conn_num(hdev, SCO_LINK);
> -               schedule_work(&data->work);
> -       }
> -}
> -
>  static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
>  {
>         struct btusb_data *data = hci_get_drvdata(hdev);
> @@ -1445,6 +1433,58 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
>         return 0;
>  }
>
> +static void bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> +{
> +       struct btusb_data *data = hci_get_drvdata(hdev);
> +
> +       if (data->isoc_altsetting != new_alts) {
> +               unsigned long flags;
> +
> +               clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> +               usb_kill_anchored_urbs(&data->isoc_anchor);
> +
> +               /* When isochronous alternate setting needs to be
> +                * changed, because SCO connection has been added
> +                * or removed, a packet fragment may be left in the
> +                * reassembling state. This could lead to wrongly
> +                * assembled fragments.
> +                *
> +                * Clear outstanding fragment when selecting a new
> +                * alternate setting.
> +                */
> +               spin_lock_irqsave(&data->rxlock, flags);
> +               kfree_skb(data->sco_skb);
> +               data->sco_skb = NULL;
> +               spin_unlock_irqrestore(&data->rxlock, flags);
> +
> +               if (__set_isoc_interface(hdev, new_alts) < 0)
> +                       return;
> +       }
> +       if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> +               if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> +                       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> +               else
> +                       btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> +       }
> +}
> +
> +static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> +{
> +       struct btusb_data *data = hci_get_drvdata(hdev);
> +
> +       BT_DBG("%s evt %d", hdev->name, evt);
> +
> +       if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> +               data->sco_num = hci_conn_num(hdev, SCO_LINK);
> +               schedule_work(&data->work);
> +       }
> +
> +       if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
> +               /* Alt setting 6 is for msbc encoded audio channel */
> +               bt_switch_alt_setting(hdev, 6);

Have you checked that this works on older controllers? I suppose those
don't have alt_set 6.

> +       }
> +}
> +
>  static void btusb_work(struct work_struct *work)
>  {
>         struct btusb_data *data = container_of(work, struct btusb_data, work);
> @@ -1472,36 +1512,7 @@ static void btusb_work(struct work_struct *work)
>                         new_alts = data->sco_num;
>                 }
>
> -               if (data->isoc_altsetting != new_alts) {
> -                       unsigned long flags;
> -
> -                       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> -                       usb_kill_anchored_urbs(&data->isoc_anchor);
> -
> -                       /* When isochronous alternate setting needs to be
> -                        * changed, because SCO connection has been added
> -                        * or removed, a packet fragment may be left in the
> -                        * reassembling state. This could lead to wrongly
> -                        * assembled fragments.
> -                        *
> -                        * Clear outstanding fragment when selecting a new
> -                        * alternate setting.
> -                        */
> -                       spin_lock_irqsave(&data->rxlock, flags);
> -                       kfree_skb(data->sco_skb);
> -                       data->sco_skb = NULL;
> -                       spin_unlock_irqrestore(&data->rxlock, flags);
> -
> -                       if (__set_isoc_interface(hdev, new_alts) < 0)
> -                               return;
> -               }
> -
> -               if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> -                       if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> -                               clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> -                       else
> -                               btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> -               }
> +               bt_switch_alt_setting(hdev, new_alts);
>         } else {
>                 clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>                 usb_kill_anchored_urbs(&data->isoc_anchor);
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index cdd9f1f..3498c6b 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -52,6 +52,7 @@
>  #define HCI_NOTIFY_CONN_ADD            1
>  #define HCI_NOTIFY_CONN_DEL            2
>  #define HCI_NOTIFY_VOICE_SETTING       3
> +#define HCI_NOTIFY_AIR_MODE_TRANSP     4
>
>  /* HCI bus types */
>  #define HCI_VIRTUAL    0
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index f12555f..8ef5220 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4100,6 +4100,11 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
>                 break;
>         }
>
> +       if (ev->air_mode == SCO_AIRMODE_TRANSP) {
> +               if (hdev->notify)
> +                       hdev->notify(hdev, HCI_NOTIFY_AIR_MODE_TRANSP);
> +       }
> +
>         hci_connect_cfm(conn, ev->status);
>         if (ev->status)
>                 hci_conn_del(conn);
> --
> 2.7.4
>



-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Luiz Augusto von Dentz]

Hi Sathish,

On Tue, Aug 21, 2018 at 8:58 AM, Sathish Narasimman
<nsathish41@gmail.com> wrote:
> Hi Luiz,
>
> On Mon, Aug 20, 2018 at 9:35 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>>
>> Hi Sathish,
>>
>> On Mon, Aug 20, 2018 at 4:32 PM, Sathish Narasimman
>> <nsathish41@gmail.com> wrote:
>> > For msbc encoded audio stream over usb transport, btusb driver
>> > to be set to alternate settings 6 as per BT core spec 5.0. This
>> > done from  hci_sync_conn_complete_evt. The type of air mode is known
>> > during this event. For this reason the btusb is to be notifed
>> > about the TRANSPARENT air mode and the ALT setting 6 is selected.
>> > The changes are made considering some discussion over the similar
>> > patch submitted earlier from Kuba Pawlak(link below)
>> > https://www.spinics.net/lists/linux-bluetooth/msg64577.html
>> >
>> > Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
>> > ---
>> >  drivers/bluetooth/btusb.c   | 95
>> > +++++++++++++++++++++++++--------------------
>> >  include/net/bluetooth/hci.h |  1 +
>> >  net/bluetooth/hci_event.c   |  5 +++
>> >  3 files changed, 59 insertions(+), 42 deletions(-)
>> >
>> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> > index cd2e5cf..ae924b6 100644
>> > --- a/drivers/bluetooth/btusb.c
>> > +++ b/drivers/bluetooth/btusb.c
>> > @@ -1390,18 +1390,6 @@ static int btusb_send_frame(struct hci_dev *hdev,
>> > struct sk_buff *skb)
>> >         return -EILSEQ;
>> >  }
>> >
>> > -static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
>> > -{
>> > -       struct btusb_data *data = hci_get_drvdata(hdev);
>> > -
>> > -       BT_DBG("%s evt %d", hdev->name, evt);
>> > -
>> > -       if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
>> > -               data->sco_num = hci_conn_num(hdev, SCO_LINK);
>> > -               schedule_work(&data->work);
>> > -       }
>> > -}
>> > -
>> >  static inline int __set_isoc_interface(struct hci_dev *hdev, int
>> > altsetting)
>> >  {
>> >         struct btusb_data *data = hci_get_drvdata(hdev);
>> > @@ -1445,6 +1433,58 @@ static inline int __set_isoc_interface(struct
>> > hci_dev *hdev, int altsetting)
>> >         return 0;
>> >  }
>> >
>> > +static void bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
>> > +{
>> > +       struct btusb_data *data = hci_get_drvdata(hdev);
>> > +
>> > +       if (data->isoc_altsetting != new_alts) {
>> > +               unsigned long flags;
>> > +
>> > +               clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>> > +               usb_kill_anchored_urbs(&data->isoc_anchor);
>> > +
>> > +               /* When isochronous alternate setting needs to be
>> > +                * changed, because SCO connection has been added
>> > +                * or removed, a packet fragment may be left in the
>> > +                * reassembling state. This could lead to wrongly
>> > +                * assembled fragments.
>> > +                *
>> > +                * Clear outstanding fragment when selecting a new
>> > +                * alternate setting.
>> > +                */
>> > +               spin_lock_irqsave(&data->rxlock, flags);
>> > +               kfree_skb(data->sco_skb);
>> > +               data->sco_skb = NULL;
>> > +               spin_unlock_irqrestore(&data->rxlock, flags);
>> > +
>> > +               if (__set_isoc_interface(hdev, new_alts) < 0)
>> > +                       return;
>> > +       }
>> > +       if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
>> > +               if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
>> > +                       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>> > +               else
>> > +                       btusb_submit_isoc_urb(hdev, GFP_KERNEL);
>> > +       }
>> > +}
>> > +
>> > +static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
>> > +{
>> > +       struct btusb_data *data = hci_get_drvdata(hdev);
>> > +
>> > +       BT_DBG("%s evt %d", hdev->name, evt);
>> > +
>> > +       if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
>> > +               data->sco_num = hci_conn_num(hdev, SCO_LINK);
>> > +               schedule_work(&data->work);
>> > +       }
>> > +
>> > +       if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
>> > +               /* Alt setting 6 is for msbc encoded audio channel */
>> > +               bt_switch_alt_setting(hdev, 6);
>>
>> Have you checked that this works on older controllers? I suppose those
>> don't have alt_set 6.
>
> I took the reference from the core spec 5, Vol 4, Part B, Table 2.1
> For USB transport the ALT setting 6 should be used for one msbc voice
> channel.
> Yes, for the controllers that does not support ALT setting 6. this patch
> will not work.

It _must_ keep working with controller that don't support such
setting, there is no option on that, so there should be a way to just
read what supported alt settings that are supported and fallback if 6
is not supported.

> Also, I failed to submit a small part of the patch to maintain 7.5ms HCI
> packet intervel in ALT 6 setting i will submitt the next set with that
> patch.
> Table 2.1: USB Primary firmware interface and endpoint settings
>>
>> > +       }
>> > +}
>> > +
>> >  static void btusb_work(struct work_struct *work)
>> >  {
>> >         struct btusb_data *data = container_of(work, struct btusb_data,
>> > work);
>> > @@ -1472,36 +1512,7 @@ static void btusb_work(struct work_struct *work)
>> >                         new_alts = data->sco_num;
>> >                 }
>> >
>> > -               if (data->isoc_altsetting != new_alts) {
>> > -                       unsigned long flags;
>> > -
>> > -                       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>> > -                       usb_kill_anchored_urbs(&data->isoc_anchor);
>> > -
>> > -                       /* When isochronous alternate setting needs to
>> > be
>> > -                        * changed, because SCO connection has been
>> > added
>> > -                        * or removed, a packet fragment may be left in
>> > the
>> > -                        * reassembling state. This could lead to
>> > wrongly
>> > -                        * assembled fragments.
>> > -                        *
>> > -                        * Clear outstanding fragment when selecting a
>> > new
>> > -                        * alternate setting.
>> > -                        */
>> > -                       spin_lock_irqsave(&data->rxlock, flags);
>> > -                       kfree_skb(data->sco_skb);
>> > -                       data->sco_skb = NULL;
>> > -                       spin_unlock_irqrestore(&data->rxlock, flags);
>> > -
>> > -                       if (__set_isoc_interface(hdev, new_alts) < 0)
>> > -                               return;
>> > -               }
>> > -
>> > -               if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags))
>> > {
>> > -                       if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
>> > -                               clear_bit(BTUSB_ISOC_RUNNING,
>> > &data->flags);
>> > -                       else
>> > -                               btusb_submit_isoc_urb(hdev, GFP_KERNEL);
>> > -               }
>> > +               bt_switch_alt_setting(hdev, new_alts);
>> >         } else {
>> >                 clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>> >                 usb_kill_anchored_urbs(&data->isoc_anchor);
>> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> > index cdd9f1f..3498c6b 100644
>> > --- a/include/net/bluetooth/hci.h
>> > +++ b/include/net/bluetooth/hci.h
>> > @@ -52,6 +52,7 @@
>> >  #define HCI_NOTIFY_CONN_ADD            1
>> >  #define HCI_NOTIFY_CONN_DEL            2
>> >  #define HCI_NOTIFY_VOICE_SETTING       3
>> > +#define HCI_NOTIFY_AIR_MODE_TRANSP     4
>> >
>> >  /* HCI bus types */
>> >  #define HCI_VIRTUAL    0
>> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> > index f12555f..8ef5220 100644
>> > --- a/net/bluetooth/hci_event.c
>> > +++ b/net/bluetooth/hci_event.c
>> > @@ -4100,6 +4100,11 @@ static void hci_sync_conn_complete_evt(struct
>> > hci_dev *hdev,
>> >                 break;
>> >         }
>> >
>> > +       if (ev->air_mode == SCO_AIRMODE_TRANSP) {
>> > +               if (hdev->notify)
>> > +                       hdev->notify(hdev, HCI_NOTIFY_AIR_MODE_TRANSP);
>> > +       }
>> > +
>> >         hci_connect_cfm(conn, ev->status);
>> >         if (ev->status)
>> >                 hci_conn_del(conn);
>> > --
>> > 2.7.4
>> >
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
> Thanks,
> Sathish N



-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Sathish Narasimman]

Hi Luiz,

On Tue, Aug 21, 2018 at 1:42 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sathish,
>
> On Tue, Aug 21, 2018 at 8:58 AM, Sathish Narasimman
> <nsathish41@gmail.com> wrote:
> > Hi Luiz,
> >
> > On Mon, Aug 20, 2018 at 9:35 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> >>
> >> Hi Sathish,
> >>
> >> On Mon, Aug 20, 2018 at 4:32 PM, Sathish Narasimman
> >> <nsathish41@gmail.com> wrote:
> >> > For msbc encoded audio stream over usb transport, btusb driver
> >> > to be set to alternate settings 6 as per BT core spec 5.0. This
> >> > done from  hci_sync_conn_complete_evt. The type of air mode is known
> >> > during this event. For this reason the btusb is to be notifed
> >> > about the TRANSPARENT air mode and the ALT setting 6 is selected.
> >> > The changes are made considering some discussion over the similar
> >> > patch submitted earlier from Kuba Pawlak(link below)
> >> > https://www.spinics.net/lists/linux-bluetooth/msg64577.html
> >> >
> >> > Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
> >> > ---
> >> >  drivers/bluetooth/btusb.c   | 95
> >> > +++++++++++++++++++++++++--------------------
> >> >  include/net/bluetooth/hci.h |  1 +
> >> >  net/bluetooth/hci_event.c   |  5 +++
> >> >  3 files changed, 59 insertions(+), 42 deletions(-)
> >> >
> >> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> >> > index cd2e5cf..ae924b6 100644
> >> > --- a/drivers/bluetooth/btusb.c
> >> > +++ b/drivers/bluetooth/btusb.c
> >> > @@ -1390,18 +1390,6 @@ static int btusb_send_frame(struct hci_dev *hdev,
> >> > struct sk_buff *skb)
> >> >         return -EILSEQ;
> >> >  }
> >> >
> >> > -static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> >> > -{
> >> > -       struct btusb_data *data = hci_get_drvdata(hdev);
> >> > -
> >> > -       BT_DBG("%s evt %d", hdev->name, evt);
> >> > -
> >> > -       if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> >> > -               data->sco_num = hci_conn_num(hdev, SCO_LINK);
> >> > -               schedule_work(&data->work);
> >> > -       }
> >> > -}
> >> > -
> >> >  static inline int __set_isoc_interface(struct hci_dev *hdev, int
> >> > altsetting)
> >> >  {
> >> >         struct btusb_data *data = hci_get_drvdata(hdev);
> >> > @@ -1445,6 +1433,58 @@ static inline int __set_isoc_interface(struct
> >> > hci_dev *hdev, int altsetting)
> >> >         return 0;
> >> >  }
> >> >
> >> > +static void bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> >> > +{
> >> > +       struct btusb_data *data = hci_get_drvdata(hdev);
> >> > +
> >> > +       if (data->isoc_altsetting != new_alts) {
> >> > +               unsigned long flags;
> >> > +
> >> > +               clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> >> > +               usb_kill_anchored_urbs(&data->isoc_anchor);
> >> > +
> >> > +               /* When isochronous alternate setting needs to be
> >> > +                * changed, because SCO connection has been added
> >> > +                * or removed, a packet fragment may be left in the
> >> > +                * reassembling state. This could lead to wrongly
> >> > +                * assembled fragments.
> >> > +                *
> >> > +                * Clear outstanding fragment when selecting a new
> >> > +                * alternate setting.
> >> > +                */
> >> > +               spin_lock_irqsave(&data->rxlock, flags);
> >> > +               kfree_skb(data->sco_skb);
> >> > +               data->sco_skb = NULL;
> >> > +               spin_unlock_irqrestore(&data->rxlock, flags);
> >> > +
> >> > +               if (__set_isoc_interface(hdev, new_alts) < 0)
> >> > +                       return;
If controller does not support ALT 6. above function will return negative.
will take care of falling back to ALT 2.
> >> > +       }
> >> > +       if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> >> > +               if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> >> > +                       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> >> > +               else
> >> > +                       btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> >> > +       }
> >> > +}
> >> > +
> >> > +static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> >> > +{
> >> > +       struct btusb_data *data = hci_get_drvdata(hdev);
> >> > +
> >> > +       BT_DBG("%s evt %d", hdev->name, evt);
> >> > +
> >> > +       if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> >> > +               data->sco_num = hci_conn_num(hdev, SCO_LINK);
> >> > +               schedule_work(&data->work);
> >> > +       }
> >> > +
> >> > +       if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
> >> > +               /* Alt setting 6 is for msbc encoded audio channel */
> >> > +               bt_switch_alt_setting(hdev, 6);
> >>
> >> Have you checked that this works on older controllers? I suppose those
> >> don't have alt_set 6.
> >
> > I took the reference from the core spec 5, Vol 4, Part B, Table 2.1
> > For USB transport the ALT setting 6 should be used for one msbc voice
> > channel.
> > Yes, for the controllers that does not support ALT setting 6. this patch
> > will not work.
>
> It _must_ keep working with controller that don't support such
> setting, there is no option on that, so there should be a way to just
> read what supported alt settings that are supported and fallback if 6
> is not supported.
>
If we trying to set to ALT SET 6 and controller does not support. in
btusb.c __set_isoc_interface function
will through error and returns negative if fails to set. So the error
check was available already.
I need to take care of returning to ALT Setting 2 if controller fails
to set ALT 6.
This can be done. will update the patch. please take a look into patch
v2 where the 7.5ms time interval is maintained.

> > Also, I failed to submit a small part of the patch to maintain 7.5ms HCI
> > packet intervel in ALT 6 setting i will submitt the next set with that
> > patch.
> > Table 2.1: USB Primary firmware interface and endpoint settings
> >>
> >> > +       }
> >> > +}
> >> > +
> >> >  static void btusb_work(struct work_struct *work)
> >> >  {
> >> >         struct btusb_data *data = container_of(work, struct btusb_data,
> >> > work);
> >> > @@ -1472,36 +1512,7 @@ static void btusb_work(struct work_struct *work)
> >> >                         new_alts = data->sco_num;
> >> >                 }
> >> >
> >> > -               if (data->isoc_altsetting != new_alts) {
> >> > -                       unsigned long flags;
> >> > -
> >> > -                       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> >> > -                       usb_kill_anchored_urbs(&data->isoc_anchor);
> >> > -
> >> > -                       /* When isochronous alternate setting needs to
> >> > be
> >> > -                        * changed, because SCO connection has been
> >> > added
> >> > -                        * or removed, a packet fragment may be left in
> >> > the
> >> > -                        * reassembling state. This could lead to
> >> > wrongly
> >> > -                        * assembled fragments.
> >> > -                        *
> >> > -                        * Clear outstanding fragment when selecting a
> >> > new
> >> > -                        * alternate setting.
> >> > -                        */
> >> > -                       spin_lock_irqsave(&data->rxlock, flags);
> >> > -                       kfree_skb(data->sco_skb);
> >> > -                       data->sco_skb = NULL;
> >> > -                       spin_unlock_irqrestore(&data->rxlock, flags);
> >> > -
> >> > -                       if (__set_isoc_interface(hdev, new_alts) < 0)
> >> > -                               return;
> >> > -               }
> >> > -
> >> > -               if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags))
> >> > {
> >> > -                       if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> >> > -                               clear_bit(BTUSB_ISOC_RUNNING,
> >> > &data->flags);
> >> > -                       else
> >> > -                               btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> >> > -               }
> >> > +               bt_switch_alt_setting(hdev, new_alts);
> >> >         } else {
> >> >                 clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> >> >                 usb_kill_anchored_urbs(&data->isoc_anchor);
> >> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> >> > index cdd9f1f..3498c6b 100644
> >> > --- a/include/net/bluetooth/hci.h
> >> > +++ b/include/net/bluetooth/hci.h
> >> > @@ -52,6 +52,7 @@
> >> >  #define HCI_NOTIFY_CONN_ADD            1
> >> >  #define HCI_NOTIFY_CONN_DEL            2
> >> >  #define HCI_NOTIFY_VOICE_SETTING       3
> >> > +#define HCI_NOTIFY_AIR_MODE_TRANSP     4
> >> >
> >> >  /* HCI bus types */
> >> >  #define HCI_VIRTUAL    0
> >> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> > index f12555f..8ef5220 100644
> >> > --- a/net/bluetooth/hci_event.c
> >> > +++ b/net/bluetooth/hci_event.c
> >> > @@ -4100,6 +4100,11 @@ static void hci_sync_conn_complete_evt(struct
> >> > hci_dev *hdev,
> >> >                 break;
> >> >         }
> >> >
> >> > +       if (ev->air_mode == SCO_AIRMODE_TRANSP) {
> >> > +               if (hdev->notify)
> >> > +                       hdev->notify(hdev, HCI_NOTIFY_AIR_MODE_TRANSP);
> >> > +       }
> >> > +
> >> >         hci_connect_cfm(conn, ev->status);
> >> >         if (ev->status)
> >> >                 hci_conn_del(conn);
> >> > --
> >> > 2.7.4
> >> >
> >>
> >>
> >>
> >> --
> >> Luiz Augusto von Dentz
> >
> >
> > Thanks,
> > Sathish N
>
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Sathish N

Re: [PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Marcel Holtmann]

Hi Sathish,

>>>>> For msbc encoded audio stream over usb transport, btusb driver
>>>>> to be set to alternate settings 6 as per BT core spec 5.0. This
>>>>> done from  hci_sync_conn_complete_evt. The type of air mode is known
>>>>> during this event. For this reason the btusb is to be notifed
>>>>> about the TRANSPARENT air mode and the ALT setting 6 is selected.
>>>>> The changes are made considering some discussion over the similar
>>>>> patch submitted earlier from Kuba Pawlak(link below)
>>>>> https://www.spinics.net/lists/linux-bluetooth/msg64577.html
>>>>> 
>>>>> Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
>>>>> ---
>>>>> drivers/bluetooth/btusb.c   | 95
>>>>> +++++++++++++++++++++++++--------------------
>>>>> include/net/bluetooth/hci.h |  1 +
>>>>> net/bluetooth/hci_event.c   |  5 +++
>>>>> 3 files changed, 59 insertions(+), 42 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index cd2e5cf..ae924b6 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -1390,18 +1390,6 @@ static int btusb_send_frame(struct hci_dev *hdev,
>>>>> struct sk_buff *skb)
>>>>>        return -EILSEQ;
>>>>> }
>>>>> 
>>>>> -static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
>>>>> -{
>>>>> -       struct btusb_data *data = hci_get_drvdata(hdev);
>>>>> -
>>>>> -       BT_DBG("%s evt %d", hdev->name, evt);
>>>>> -
>>>>> -       if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
>>>>> -               data->sco_num = hci_conn_num(hdev, SCO_LINK);
>>>>> -               schedule_work(&data->work);
>>>>> -       }
>>>>> -}
>>>>> -
>>>>> static inline int __set_isoc_interface(struct hci_dev *hdev, int
>>>>> altsetting)
>>>>> {
>>>>>        struct btusb_data *data = hci_get_drvdata(hdev);
>>>>> @@ -1445,6 +1433,58 @@ static inline int __set_isoc_interface(struct
>>>>> hci_dev *hdev, int altsetting)
>>>>>        return 0;
>>>>> }
>>>>> 
>>>>> +static void bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
>>>>> +{
>>>>> +       struct btusb_data *data = hci_get_drvdata(hdev);
>>>>> +
>>>>> +       if (data->isoc_altsetting != new_alts) {
>>>>> +               unsigned long flags;
>>>>> +
>>>>> +               clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>>>>> +               usb_kill_anchored_urbs(&data->isoc_anchor);
>>>>> +
>>>>> +               /* When isochronous alternate setting needs to be
>>>>> +                * changed, because SCO connection has been added
>>>>> +                * or removed, a packet fragment may be left in the
>>>>> +                * reassembling state. This could lead to wrongly
>>>>> +                * assembled fragments.
>>>>> +                *
>>>>> +                * Clear outstanding fragment when selecting a new
>>>>> +                * alternate setting.
>>>>> +                */
>>>>> +               spin_lock_irqsave(&data->rxlock, flags);
>>>>> +               kfree_skb(data->sco_skb);
>>>>> +               data->sco_skb = NULL;
>>>>> +               spin_unlock_irqrestore(&data->rxlock, flags);
>>>>> +
>>>>> +               if (__set_isoc_interface(hdev, new_alts) < 0)
>>>>> +                       return;
> If controller does not support ALT 6. above function will return negative.
> will take care of falling back to ALT 2.
>>>>> +       }
>>>>> +       if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
>>>>> +               if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
>>>>> +                       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>>>>> +               else
>>>>> +                       btusb_submit_isoc_urb(hdev, GFP_KERNEL);
>>>>> +       }
>>>>> +}
>>>>> +
>>>>> +static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
>>>>> +{
>>>>> +       struct btusb_data *data = hci_get_drvdata(hdev);
>>>>> +
>>>>> +       BT_DBG("%s evt %d", hdev->name, evt);
>>>>> +
>>>>> +       if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
>>>>> +               data->sco_num = hci_conn_num(hdev, SCO_LINK);
>>>>> +               schedule_work(&data->work);
>>>>> +       }
>>>>> +
>>>>> +       if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
>>>>> +               /* Alt setting 6 is for msbc encoded audio channel */
>>>>> +               bt_switch_alt_setting(hdev, 6);
>>>> 
>>>> Have you checked that this works on older controllers? I suppose those
>>>> don't have alt_set 6.
>>> 
>>> I took the reference from the core spec 5, Vol 4, Part B, Table 2.1
>>> For USB transport the ALT setting 6 should be used for one msbc voice
>>> channel.
>>> Yes, for the controllers that does not support ALT setting 6. this patch
>>> will not work.
>> 
>> It _must_ keep working with controller that don't support such
>> setting, there is no option on that, so there should be a way to just
>> read what supported alt settings that are supported and fallback if 6
>> is not supported.
>> 
> If we trying to set to ALT SET 6 and controller does not support. in
> btusb.c __set_isoc_interface function
> will through error and returns negative if fails to set. So the error
> check was available already.
> I need to take care of returning to ALT Setting 2 if controller fails
> to set ALT 6.
> This can be done. will update the patch. please take a look into patch
> v2 where the 7.5ms time interval is maintained.

we are not doing try-and-error here. In probe() do a proper enumeration of the alternate settings and check if setting 6 is available.

Regards

Marcel

[PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Amit K Bag]

For msbc encoded audio stream over usb transport, btusb driver
to be set to alternate settings 6 as per BT core spec 5.0. This
done from  hci_sync_conn_complete_evt.  The type of air mode is known
during this event.  For this reason the btusb is to be notifed
about the TRANSPARENT air mode and the ALT setting 6 is selected.
The changes are made considering some discussion over the similar
patch submitted earlier from Kuba Pawlak(link below)
https://www.spinics.net/lists/linux-bluetooth/msg64577.html

(am from https://www.spinics.net/lists/linux-bluetooth/msg76982.html)

Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
---
 drivers/bluetooth/btusb.c   | 145 ++++++++++++++++++++++++++++++--------------
 include/net/bluetooth/hci.h |   1 +
 net/bluetooth/hci_event.c   |   5 ++
 3 files changed, 106 insertions(+), 45 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a9c35ebb30f8..368cc93cb17e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -57,6 +57,9 @@ static struct usb_driver btusb_driver;
 #define BTUSB_IFNUM_2		0x80000
 #define BTUSB_CW6622		0x100000
 #define BTUSB_MEDIATEK		0x200000
+#define BTUSB_ALT6_FLOW_CNTRL	6
+
+static int set_hci_packet_interval_flow = BTUSB_ALT6_FLOW_CNTRL;
 
 static const struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
@@ -974,6 +977,38 @@ static void btusb_isoc_complete(struct urb *urb)
 	}
 }
 
+static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
+					       int mtu)
+{
+	int i, offset = 0;
+
+	/* For msbc ALT 6 setting the host will send the packet at continuous
+	 * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting
+	 * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets.
+	 * To maintain the rate we send 63bytes of usb packets alternatively for
+	 * 7ms and 8ms to maintain the rate as 7.5ms.
+	 */
+	if (set_hci_packet_interval_flow == 6)
+		set_hci_packet_interval_flow = 7;
+	else if (set_hci_packet_interval_flow == 7)
+		set_hci_packet_interval_flow = 6;
+
+	BT_DBG("len %d mtu %d", len, mtu);
+
+	for (i = 0; i < set_hci_packet_interval_flow; i++) {
+		urb->iso_frame_desc[i].offset = offset;
+		urb->iso_frame_desc[i].length = offset;
+	}
+
+	if (len && i < BTUSB_MAX_ISOC_FRAMES) {
+		urb->iso_frame_desc[i].offset = offset;
+		urb->iso_frame_desc[i].length = len;
+		i++;
+	}
+
+	urb->number_of_packets = i;
+}
+
 static inline void __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
 {
 	int i, offset = 0;
@@ -1376,9 +1411,12 @@ static struct urb *alloc_isoc_urb(struct hci_dev *hdev, struct sk_buff *skb)
 
 	urb->transfer_flags  = URB_ISO_ASAP;
 
-	__fill_isoc_descriptor(urb, skb->len,
+	if (data->isoc_altsetting == 6)
+		__fill_isoc_descriptor_msbc(urb, skb->len,
+			       le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
+	else
+		__fill_isoc_descriptor(urb, skb->len,
 			       le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
-
 	skb->dev = (void *)hdev;
 
 	return urb;
@@ -1466,18 +1504,6 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	return -EILSEQ;
 }
 
-static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
-{
-	struct btusb_data *data = hci_get_drvdata(hdev);
-
-	BT_DBG("%s evt %d", hdev->name, evt);
-
-	if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
-		data->sco_num = hci_conn_num(hdev, SCO_LINK);
-		schedule_work(&data->work);
-	}
-}
-
 static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
@@ -1521,6 +1547,65 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
 	return 0;
 }
 
+static int bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	int err;
+
+	if (data->isoc_altsetting != new_alts) {
+		unsigned long flags;
+
+		clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+		usb_kill_anchored_urbs(&data->isoc_anchor);
+
+		/* When isochronous alternate setting needs to be
+		 * changed, because SCO connection has been added
+		 * or removed, a packet fragment may be left in the
+		 * reassembling state. This could lead to wrongly
+		 * assembled fragments.
+		 *
+		 * Clear outstanding fragment when selecting a new
+		 * alternate setting.
+		 */
+		spin_lock_irqsave(&data->rxlock, flags);
+		kfree_skb(data->sco_skb);
+		data->sco_skb = NULL;
+		spin_unlock_irqrestore(&data->rxlock, flags);
+
+		err = __set_isoc_interface(hdev, new_alts);
+		if (err < 0)
+			return err;
+	}
+	if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
+		if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
+			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+		else
+			btusb_submit_isoc_urb(hdev, GFP_KERNEL);
+	}
+
+	return 0;
+}
+
+static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+
+	BT_DBG("%s evt %d", hdev->name, evt);
+
+	if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
+		data->sco_num = hci_conn_num(hdev, SCO_LINK);
+		schedule_work(&data->work);
+	}
+
+	if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
+		/* Alt setting 6 is used for msbc encoded
+		 * audio channel
+		 */
+		if (bt_switch_alt_setting(hdev, 6) < 0)
+			BT_ERR("%s Set USB Alt6 failed", hdev->name);
+	}
+}
+
 static void btusb_work(struct work_struct *work)
 {
 	struct btusb_data *data = container_of(work, struct btusb_data, work);
@@ -1547,37 +1632,7 @@ static void btusb_work(struct work_struct *work)
 		} else {
 			new_alts = data->sco_num;
 		}
-
-		if (data->isoc_altsetting != new_alts) {
-			unsigned long flags;
-
-			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
-			usb_kill_anchored_urbs(&data->isoc_anchor);
-
-			/* When isochronous alternate setting needs to be
-			 * changed, because SCO connection has been added
-			 * or removed, a packet fragment may be left in the
-			 * reassembling state. This could lead to wrongly
-			 * assembled fragments.
-			 *
-			 * Clear outstanding fragment when selecting a new
-			 * alternate setting.
-			 */
-			spin_lock_irqsave(&data->rxlock, flags);
-			kfree_skb(data->sco_skb);
-			data->sco_skb = NULL;
-			spin_unlock_irqrestore(&data->rxlock, flags);
-
-			if (__set_isoc_interface(hdev, new_alts) < 0)
-				return;
-		}
-
-		if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
-			if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
-				clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
-			else
-				btusb_submit_isoc_urb(hdev, GFP_KERNEL);
-		}
+		bt_switch_alt_setting(hdev, new_alts);
 	} else {
 		clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
 		usb_kill_anchored_urbs(&data->isoc_anchor);
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 5bc1e30dedde..89ac29f1dffa 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -52,6 +52,7 @@
 #define HCI_NOTIFY_CONN_ADD		1
 #define HCI_NOTIFY_CONN_DEL		2
 #define HCI_NOTIFY_VOICE_SETTING	3
+#define HCI_NOTIFY_AIR_MODE_TRANSP	4
 
 /* HCI bus types */
 #define HCI_VIRTUAL	0
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index c1d3a303d97f..1c268932422c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4231,6 +4231,11 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
 		break;
 	}
 
+	if (ev->air_mode == SCO_AIRMODE_TRANSP) {
+		if (hdev->notify)
+			hdev->notify(hdev, HCI_NOTIFY_AIR_MODE_TRANSP);
+	}
+
 	hci_connect_cfm(conn, ev->status);
 	if (ev->status)
 		hci_conn_del(conn);
-- 
2.7.4

Re: [PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Marcel Holtmann]

Hi Amit,

> For msbc encoded audio stream over usb transport, btusb driver
> to be set to alternate settings 6 as per BT core spec 5.0. This
> done from  hci_sync_conn_complete_evt.  The type of air mode is known
> during this event.  For this reason the btusb is to be notifed
> about the TRANSPARENT air mode and the ALT setting 6 is selected.
> The changes are made considering some discussion over the similar
> patch submitted earlier from Kuba Pawlak(link below)
> https://www.spinics.net/lists/linux-bluetooth/msg64577.html
> 
> (am from https://www.spinics.net/lists/linux-bluetooth/msg76982.html)
> 
> Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
> Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
> Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> ---
> drivers/bluetooth/btusb.c   | 145 ++++++++++++++++++++++++++++++--------------
> include/net/bluetooth/hci.h |   1 +
> net/bluetooth/hci_event.c   |   5 ++
> 3 files changed, 106 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index a9c35ebb30f8..368cc93cb17e 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -57,6 +57,9 @@ static struct usb_driver btusb_driver;
> #define BTUSB_IFNUM_2		0x80000
> #define BTUSB_CW6622		0x100000
> #define BTUSB_MEDIATEK		0x200000
> +#define BTUSB_ALT6_FLOW_CNTRL	6
> +
> +static int set_hci_packet_interval_flow = BTUSB_ALT6_FLOW_CNTRL;

this can never be a global variable. We allow multiple devices be attached to the same host. You need to move this into btusb_data struct.

> 
> static const struct usb_device_id btusb_table[] = {
> 	/* Generic Bluetooth USB device */
> @@ -974,6 +977,38 @@ static void btusb_isoc_complete(struct urb *urb)
> 	}
> }
> 
> +static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
> +					       int mtu)
> +{
> +	int i, offset = 0;
> +
> +	/* For msbc ALT 6 setting the host will send the packet at continuous
> +	 * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting
> +	 * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets.
> +	 * To maintain the rate we send 63bytes of usb packets alternatively for
> +	 * 7ms and 8ms to maintain the rate as 7.5ms.
> +	 */
> +	if (set_hci_packet_interval_flow == 6)
> +		set_hci_packet_interval_flow = 7;
> +	else if (set_hci_packet_interval_flow == 7)
> +		set_hci_packet_interval_flow = 6;

can’t this just be a bool that you just toggle.

> +
> +	BT_DBG("len %d mtu %d", len, mtu);
> +
> +	for (i = 0; i < set_hci_packet_interval_flow; i++) {
> +		urb->iso_frame_desc[i].offset = offset;
> +		urb->iso_frame_desc[i].length = offset;
> +	}
> +
> +	if (len && i < BTUSB_MAX_ISOC_FRAMES) {
> +		urb->iso_frame_desc[i].offset = offset;
> +		urb->iso_frame_desc[i].length = len;
> +		i++;
> +	}
> +
> +	urb->number_of_packets = i;
> +}
> +
> static inline void __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
> {
> 	int i, offset = 0;
> @@ -1376,9 +1411,12 @@ static struct urb *alloc_isoc_urb(struct hci_dev *hdev, struct sk_buff *skb)
> 
> 	urb->transfer_flags  = URB_ISO_ASAP;
> 
> -	__fill_isoc_descriptor(urb, skb->len,
> +	if (data->isoc_altsetting == 6)
> +		__fill_isoc_descriptor_msbc(urb, skb->len,
> +			       le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
> +	else
> +		__fill_isoc_descriptor(urb, skb->len,
> 			       le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
> -
> 	skb->dev = (void *)hdev;
> 
> 	return urb;
> @@ -1466,18 +1504,6 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> 	return -EILSEQ;
> }
> 
> -static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> -{
> -	struct btusb_data *data = hci_get_drvdata(hdev);
> -
> -	BT_DBG("%s evt %d", hdev->name, evt);
> -
> -	if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> -		data->sco_num = hci_conn_num(hdev, SCO_LINK);
> -		schedule_work(&data->work);
> -	}
> -}
> -
> static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> {
> 	struct btusb_data *data = hci_get_drvdata(hdev);
> @@ -1521,6 +1547,65 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> 	return 0;
> }
> 
> +static int bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> +{
> +	struct btusb_data *data = hci_get_drvdata(hdev);
> +	int err;
> +
> +	if (data->isoc_altsetting != new_alts) {
> +		unsigned long flags;
> +
> +		clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> +		usb_kill_anchored_urbs(&data->isoc_anchor);
> +
> +		/* When isochronous alternate setting needs to be
> +		 * changed, because SCO connection has been added
> +		 * or removed, a packet fragment may be left in the
> +		 * reassembling state. This could lead to wrongly
> +		 * assembled fragments.
> +		 *
> +		 * Clear outstanding fragment when selecting a new
> +		 * alternate setting.
> +		 */
> +		spin_lock_irqsave(&data->rxlock, flags);
> +		kfree_skb(data->sco_skb);
> +		data->sco_skb = NULL;
> +		spin_unlock_irqrestore(&data->rxlock, flags);
> +
> +		err = __set_isoc_interface(hdev, new_alts);
> +		if (err < 0)
> +			return err;
> +	}
> +	if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> +		if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> +			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> +		else
> +			btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> +	}
> +
> +	return 0;
> +}
> +
> +static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> +{
> +	struct btusb_data *data = hci_get_drvdata(hdev);
> +
> +	BT_DBG("%s evt %d", hdev->name, evt);
> +
> +	if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> +		data->sco_num = hci_conn_num(hdev, SCO_LINK);
> +		schedule_work(&data->work);
> +	}

Since the internal processing of packets moved from a tasklet to a workqueue, I think the extra workqueue inside btusb can be actually removed. So I would get rid of this also for the non-mSBC case.

> +
> +	if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
> +		/* Alt setting 6 is used for msbc encoded
> +		 * audio channel
> +		 */
> +		if (bt_switch_alt_setting(hdev, 6) < 0)
> +			BT_ERR("%s Set USB Alt6 failed", hdev->name);
> +	}
> +}
> +

I think this is really a one or the other. If the controller supports alternate setting 6 (which you actually have to check first) and it is air mode transparent then switch to alternate setting 6. Otherwise do what we have done before.

> static void btusb_work(struct work_struct *work)
> {
> 	struct btusb_data *data = container_of(work, struct btusb_data, work);
> @@ -1547,37 +1632,7 @@ static void btusb_work(struct work_struct *work)
> 		} else {
> 			new_alts = data->sco_num;
> 		}
> -
> -		if (data->isoc_altsetting != new_alts) {
> -			unsigned long flags;
> -
> -			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> -			usb_kill_anchored_urbs(&data->isoc_anchor);
> -
> -			/* When isochronous alternate setting needs to be
> -			 * changed, because SCO connection has been added
> -			 * or removed, a packet fragment may be left in the
> -			 * reassembling state. This could lead to wrongly
> -			 * assembled fragments.
> -			 *
> -			 * Clear outstanding fragment when selecting a new
> -			 * alternate setting.
> -			 */
> -			spin_lock_irqsave(&data->rxlock, flags);
> -			kfree_skb(data->sco_skb);
> -			data->sco_skb = NULL;
> -			spin_unlock_irqrestore(&data->rxlock, flags);
> -
> -			if (__set_isoc_interface(hdev, new_alts) < 0)
> -				return;
> -		}
> -
> -		if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> -			if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> -				clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> -			else
> -				btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> -		}
> +		bt_switch_alt_setting(hdev, new_alts);
> 	} else {
> 		clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> 		usb_kill_anchored_urbs(&data->isoc_anchor);
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 5bc1e30dedde..89ac29f1dffa 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -52,6 +52,7 @@
> #define HCI_NOTIFY_CONN_ADD		1
> #define HCI_NOTIFY_CONN_DEL		2
> #define HCI_NOTIFY_VOICE_SETTING	3
> +#define HCI_NOTIFY_AIR_MODE_TRANSP	4
> 
> /* HCI bus types */
> #define HCI_VIRTUAL	0
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index c1d3a303d97f..1c268932422c 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4231,6 +4231,11 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
> 		break;
> 	}
> 
> +	if (ev->air_mode == SCO_AIRMODE_TRANSP) {
> +		if (hdev->notify)
> +			hdev->notify(hdev, HCI_NOTIFY_AIR_MODE_TRANSP);
> +	}
> +

So this might work, but causes the USB subsystem to change the alternate setting twice. Once for CONN_ADD and once for SCO_AIRMODE_TRANSP. I prefer we just change the alternate setting once.

Maybe it is better to change CONN_ADD and CONN_DEL to an explicit ENABLE_SCO_CVSD and ENABLE_SCO_TRANSP and DISABLE_SCO.

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints

[Sathish Narasimman]

Hi Marcel,

I will submit the next version of the patch and tried to fix all your comments.
Please check the same.

On Thu, Sep 26, 2019 at 3:52 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Amit,
>
> > For msbc encoded audio stream over usb transport, btusb driver
> > to be set to alternate settings 6 as per BT core spec 5.0. This
> > done from  hci_sync_conn_complete_evt.  The type of air mode is known
> > during this event.  For this reason the btusb is to be notifed
> > about the TRANSPARENT air mode and the ALT setting 6 is selected.
> > The changes are made considering some discussion over the similar
> > patch submitted earlier from Kuba Pawlak(link below)
> > https://www.spinics.net/lists/linux-bluetooth/msg64577.html
> >
> > (am from https://www.spinics.net/lists/linux-bluetooth/msg76982.html)
> >
> > Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
> > Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> > Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
> > Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> > ---
> > drivers/bluetooth/btusb.c   | 145 ++++++++++++++++++++++++++++++--------------
> > include/net/bluetooth/hci.h |   1 +
> > net/bluetooth/hci_event.c   |   5 ++
> > 3 files changed, 106 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index a9c35ebb30f8..368cc93cb17e 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -57,6 +57,9 @@ static struct usb_driver btusb_driver;
> > #define BTUSB_IFNUM_2         0x80000
> > #define BTUSB_CW6622          0x100000
> > #define BTUSB_MEDIATEK                0x200000
> > +#define BTUSB_ALT6_FLOW_CNTRL        6
> > +
> > +static int set_hci_packet_interval_flow = BTUSB_ALT6_FLOW_CNTRL;
>
> this can never be a global variable. We allow multiple devices be attached to the same host. You need to move this into btusb_data struct.
>
> >
> > static const struct usb_device_id btusb_table[] = {
> >       /* Generic Bluetooth USB device */
> > @@ -974,6 +977,38 @@ static void btusb_isoc_complete(struct urb *urb)
> >       }
> > }
> >
> > +static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
> > +                                            int mtu)
> > +{
> > +     int i, offset = 0;
> > +
> > +     /* For msbc ALT 6 setting the host will send the packet at continuous
> > +      * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting
> > +      * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets.
> > +      * To maintain the rate we send 63bytes of usb packets alternatively for
> > +      * 7ms and 8ms to maintain the rate as 7.5ms.
> > +      */
> > +     if (set_hci_packet_interval_flow == 6)
> > +             set_hci_packet_interval_flow = 7;
> > +     else if (set_hci_packet_interval_flow == 7)
> > +             set_hci_packet_interval_flow = 6;
>
> can’t this just be a bool that you just toggle.
Done.
>
> > +
> > +     BT_DBG("len %d mtu %d", len, mtu);
> > +
> > +     for (i = 0; i < set_hci_packet_interval_flow; i++) {
> > +             urb->iso_frame_desc[i].offset = offset;
> > +             urb->iso_frame_desc[i].length = offset;
> > +     }
> > +
> > +     if (len && i < BTUSB_MAX_ISOC_FRAMES) {
> > +             urb->iso_frame_desc[i].offset = offset;
> > +             urb->iso_frame_desc[i].length = len;
> > +             i++;
> > +     }
> > +
> > +     urb->number_of_packets = i;
> > +}
> > +
> > static inline void __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
> > {
> >       int i, offset = 0;
> > @@ -1376,9 +1411,12 @@ static struct urb *alloc_isoc_urb(struct hci_dev *hdev, struct sk_buff *skb)
> >
> >       urb->transfer_flags  = URB_ISO_ASAP;
> >
> > -     __fill_isoc_descriptor(urb, skb->len,
> > +     if (data->isoc_altsetting == 6)
> > +             __fill_isoc_descriptor_msbc(urb, skb->len,
> > +                            le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
> > +     else
> > +             __fill_isoc_descriptor(urb, skb->len,
> >                              le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
> > -
> >       skb->dev = (void *)hdev;
> >
> >       return urb;
> > @@ -1466,18 +1504,6 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> >       return -EILSEQ;
> > }
> >
> > -static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> > -{
> > -     struct btusb_data *data = hci_get_drvdata(hdev);
> > -
> > -     BT_DBG("%s evt %d", hdev->name, evt);
> > -
> > -     if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> > -             data->sco_num = hci_conn_num(hdev, SCO_LINK);
> > -             schedule_work(&data->work);
> > -     }
> > -}
> > -
> > static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> > {
> >       struct btusb_data *data = hci_get_drvdata(hdev);
> > @@ -1521,6 +1547,65 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> >       return 0;
> > }
> >
> > +static int bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> > +{
> > +     struct btusb_data *data = hci_get_drvdata(hdev);
> > +     int err;
> > +
> > +     if (data->isoc_altsetting != new_alts) {
> > +             unsigned long flags;
> > +
> > +             clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > +             usb_kill_anchored_urbs(&data->isoc_anchor);
> > +
> > +             /* When isochronous alternate setting needs to be
> > +              * changed, because SCO connection has been added
> > +              * or removed, a packet fragment may be left in the
> > +              * reassembling state. This could lead to wrongly
> > +              * assembled fragments.
> > +              *
> > +              * Clear outstanding fragment when selecting a new
> > +              * alternate setting.
> > +              */
> > +             spin_lock_irqsave(&data->rxlock, flags);
> > +             kfree_skb(data->sco_skb);
> > +             data->sco_skb = NULL;
> > +             spin_unlock_irqrestore(&data->rxlock, flags);
> > +
> > +             err = __set_isoc_interface(hdev, new_alts);
> > +             if (err < 0)
> > +                     return err;
> > +     }
> > +     if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> > +             if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> > +                     clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > +             else
> > +                     btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> > +{
> > +     struct btusb_data *data = hci_get_drvdata(hdev);
> > +
> > +     BT_DBG("%s evt %d", hdev->name, evt);
> > +
> > +     if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> > +             data->sco_num = hci_conn_num(hdev, SCO_LINK);
> > +             schedule_work(&data->work);
> > +     }
>
> Since the internal processing of packets moved from a tasklet to a workqueue, I think the extra workqueue inside btusb can be actually removed. So I would get rid of this also for the non-mSBC case.
Have left as it is. Expecting you will change this.
>
> > +
> > +     if (evt == HCI_NOTIFY_AIR_MODE_TRANSP) {
> > +             /* Alt setting 6 is used for msbc encoded
> > +              * audio channel
> > +              */
> > +             if (bt_switch_alt_setting(hdev, 6) < 0)
> > +                     BT_ERR("%s Set USB Alt6 failed", hdev->name);
> > +     }
> > +}
> > +
>
> I think this is really a one or the other. If the controller supports alternate setting 6 (which you actually have to check first) and it is air mode transparent then switch to alternate setting 6. Otherwise do what we have done before.
Done.
>
> > static void btusb_work(struct work_struct *work)
> > {
> >       struct btusb_data *data = container_of(work, struct btusb_data, work);
> > @@ -1547,37 +1632,7 @@ static void btusb_work(struct work_struct *work)
> >               } else {
> >                       new_alts = data->sco_num;
> >               }
> > -
> > -             if (data->isoc_altsetting != new_alts) {
> > -                     unsigned long flags;
> > -
> > -                     clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > -                     usb_kill_anchored_urbs(&data->isoc_anchor);
> > -
> > -                     /* When isochronous alternate setting needs to be
> > -                      * changed, because SCO connection has been added
> > -                      * or removed, a packet fragment may be left in the
> > -                      * reassembling state. This could lead to wrongly
> > -                      * assembled fragments.
> > -                      *
> > -                      * Clear outstanding fragment when selecting a new
> > -                      * alternate setting.
> > -                      */
> > -                     spin_lock_irqsave(&data->rxlock, flags);
> > -                     kfree_skb(data->sco_skb);
> > -                     data->sco_skb = NULL;
> > -                     spin_unlock_irqrestore(&data->rxlock, flags);
> > -
> > -                     if (__set_isoc_interface(hdev, new_alts) < 0)
> > -                             return;
> > -             }
> > -
> > -             if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> > -                     if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> > -                             clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > -                     else
> > -                             btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> > -             }
> > +             bt_switch_alt_setting(hdev, new_alts);
> >       } else {
> >               clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> >               usb_kill_anchored_urbs(&data->isoc_anchor);
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 5bc1e30dedde..89ac29f1dffa 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -52,6 +52,7 @@
> > #define HCI_NOTIFY_CONN_ADD           1
> > #define HCI_NOTIFY_CONN_DEL           2
> > #define HCI_NOTIFY_VOICE_SETTING      3
> > +#define HCI_NOTIFY_AIR_MODE_TRANSP   4
> >
> > /* HCI bus types */
> > #define HCI_VIRTUAL   0
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index c1d3a303d97f..1c268932422c 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4231,6 +4231,11 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
> >               break;
> >       }
> >
> > +     if (ev->air_mode == SCO_AIRMODE_TRANSP) {
> > +             if (hdev->notify)
> > +                     hdev->notify(hdev, HCI_NOTIFY_AIR_MODE_TRANSP);
> > +     }
> > +
>
> So this might work, but causes the USB subsystem to change the alternate setting twice. Once for CONN_ADD and once for SCO_AIRMODE_TRANSP. I prefer we just change the alternate setting once.

> Maybe it is better to change CONN_ADD and CONN_DEL to an explicit ENABLE_SCO_CVSD and ENABLE_SCO_TRANSP and DISABLE_SCO.

Done.

>
> Regards
>
> Marcel
>

Regards
Sathish N