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 <[email protected]>
Signed-off-by: Sathish Narasimman <[email protected]>
Signed-off-by: Raghuram Hegde <[email protected]>
Signed-off-by: Hsin-Yu Chao <[email protected]>
Signed-off-by: Amit K Bag <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Sathish Narasimman <[email protected]>
> Signed-off-by: Raghuram Hegde <[email protected]>
> Signed-off-by: Hsin-Yu Chao <[email protected]>
> Signed-off-by: Amit K Bag <[email protected]>
> ---
> 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
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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Sathish Narasimman <[email protected]>
> > Signed-off-by: Raghuram Hegde <[email protected]>
> > Signed-off-by: Hsin-Yu Chao <[email protected]>
> > Signed-off-by: Amit K Bag <[email protected]>
> > ---
> > 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