[PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[Suraj Sumangala]

This patch adds SCO over HCI support to Atheros AR300x HCI transport
driver.

Signed-off-by: Suraj Sumangala <suraj@atheros.com>
---
 drivers/bluetooth/hci_ath.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index 4093935..8e15c31 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -162,11 +162,6 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 {
 	struct ath_struct *ath = hu->priv;
 
-	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
-		kfree_skb(skb);
-		return 0;
-	}
-
 	/*
 	 * Update power management enable flag with parameters of
 	 * HCI sleep enable vendor specific HCI command.
@@ -184,9 +179,14 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
 
 	skb_queue_tail(&ath->txq, skb);
-	set_bit(HCI_UART_SENDING, &hu->tx_state);
 
-	schedule_work(&ath->ctxtsw);
+	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
+		clear_bit(HCI_UART_SENDING, &hu->tx_state);
+		hci_uart_tx_wakeup(hu);
+	} else {
+		set_bit(HCI_UART_SENDING, &hu->tx_state);
+		schedule_work(&ath->ctxtsw);
+	}
 
 	return 0;
 }
-- 
1.7.0.4

Re: [PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[Gustavo F. Padovan]

Hi Suraj,

* Suraj Sumangala <suraj@atheros.com> [2011-05-20 13:36:11 +0530]:

> This patch adds SCO over HCI support to Atheros AR300x HCI transport
> driver.
> 
> Signed-off-by: Suraj Sumangala <suraj@atheros.com>
> ---
>  drivers/bluetooth/hci_ath.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> index 4093935..8e15c31 100644
> --- a/drivers/bluetooth/hci_ath.c
> +++ b/drivers/bluetooth/hci_ath.c
> @@ -162,11 +162,6 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>  {
>  	struct ath_struct *ath = hu->priv;
>  
> -	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
> -		kfree_skb(skb);
> -		return 0;
> -	}
> -
>  	/*
>  	 * Update power management enable flag with parameters of
>  	 * HCI sleep enable vendor specific HCI command.
> @@ -184,9 +179,14 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>  	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
>  
>  	skb_queue_tail(&ath->txq, skb);
> -	set_bit(HCI_UART_SENDING, &hu->tx_state);
>  
> -	schedule_work(&ath->ctxtsw);
> +	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
> +		clear_bit(HCI_UART_SENDING, &hu->tx_state);
> +		hci_uart_tx_wakeup(hu);

We can't do that, prioritize SCO over HCI Events. This can lead to weird sync
problems, so please remove this and send me a new patch.

-- 
Gustavo F. Padovan
http://profusion.mobi

RE: [PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[Sumangala, Suraj]

Hi Gustavo,
________________________________________
From: Gustavo F. Padovan [pao@profusion.mobi] on behalf of Gustavo F. Padovan [padovan@profusion.mobi]
Sent: Tuesday, May 31, 2011 12:32 AM
To: Sumangala, Suraj
Cc: linux-bluetooth@vger.kernel.org; Somi Ramasamy Mothilal, JothiKumar
Subject: Re: [PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

Hi Suraj,

* Suraj Sumangala <suraj@atheros.com> [2011-05-20 13:36:11 +0530]:

> This patch adds SCO over HCI support to Atheros AR300x HCI transport
> driver.
>
> Signed-off-by: Suraj Sumangala <suraj@atheros.com>
> ---
>  drivers/bluetooth/hci_ath.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> index 4093935..8e15c31 100644
> --- a/drivers/bluetooth/hci_ath.c
> +++ b/drivers/bluetooth/hci_ath.c
> @@ -162,11 +162,6 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>  {
>       struct ath_struct *ath = hu->priv;
>
> -     if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
> -             kfree_skb(skb);
> -             return 0;
> -     }
> -
>       /*
>        * Update power management enable flag with parameters of
>        * HCI sleep enable vendor specific HCI command.
> @@ -184,9 +179,14 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>       memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
>
>       skb_queue_tail(&ath->txq, skb);
> -     set_bit(HCI_UART_SENDING, &hu->tx_state);
>
> -     schedule_work(&ath->ctxtsw);
> +     if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
> +             clear_bit(HCI_UART_SENDING, &hu->tx_state);
> +             hci_uart_tx_wakeup(hu);

We can't do that, prioritize SCO over HCI Events. This can lead to weird sync
problems, so please remove this and send me a new patch.

The reason why I went with this approach was
1. In the schedule_work path, we were doing some power management logic which was relevent only for ACL packets.
2. Without this somehow the SCO audio quality was bad and the controller team mention that they were not getting SCO packets in this rate.

Any suggestions from you?

Regards
Suraj

RE: [PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[Marcel Holtmann]

Hi Suraj,

> * Suraj Sumangala <suraj@atheros.com> [2011-05-20 13:36:11 +0530]:
> 
> > This patch adds SCO over HCI support to Atheros AR300x HCI transport
> > driver.
> >
> > Signed-off-by: Suraj Sumangala <suraj@atheros.com>
> > ---
> >  drivers/bluetooth/hci_ath.c |   14 +++++++-------
> >  1 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> > index 4093935..8e15c31 100644
> > --- a/drivers/bluetooth/hci_ath.c
> > +++ b/drivers/bluetooth/hci_ath.c
> > @@ -162,11 +162,6 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> >  {
> >       struct ath_struct *ath = hu->priv;
> >
> > -     if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
> > -             kfree_skb(skb);
> > -             return 0;
> > -     }
> > -
> >       /*
> >        * Update power management enable flag with parameters of
> >        * HCI sleep enable vendor specific HCI command.
> > @@ -184,9 +179,14 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> >       memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> >
> >       skb_queue_tail(&ath->txq, skb);
> > -     set_bit(HCI_UART_SENDING, &hu->tx_state);
> >
> > -     schedule_work(&ath->ctxtsw);
> > +     if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
> > +             clear_bit(HCI_UART_SENDING, &hu->tx_state);
> > +             hci_uart_tx_wakeup(hu);
> 
> We can't do that, prioritize SCO over HCI Events. This can lead to weird sync
> problems, so please remove this and send me a new patch.
> 
> The reason why I went with this approach was
> 1. In the schedule_work path, we were doing some power management logic which was relevent only for ACL packets.
> 2. Without this somehow the SCO audio quality was bad and the controller team mention that they were not getting SCO packets in this rate.

you might actually have to re-design your driver then. Since essentially
you need to ensure that events like HCI_Conn_Complete arrive in the
proper order to make sure that ACL and SCO handles are properly
available before ACL or SCO packets are send.

This has been always the problem. Especially with USB endpoints where
SCO packets and ACL packets are on different endpoints. Getting the
order right is pretty tricky.

In addition you have the completed packet events that need to be handled
properly. Otherwise you might screw up the flow control.

Regards

Marcel

Re: [PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[Suraj Sumangala]

Hi Gustavo,

On 2/2/2011 9:46 PM, Gustavo F. Padovan wrote:
> Hi Suraj,
>
> * Suraj Sumangala<suraj@atheros.com>  [2011-01-28 16:19:04 +0530]:
>
>> This patch adds SCO over HCI support to Atheros AR300x HCI transport
>> driver.
>>
>> Signed-off-by: Suraj Sumangala<suraj@atheros.com>
>> ---
>>   drivers/bluetooth/hci_ath.c |   18 +++++++++---------
>>   1 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
>> index 6a160c1..161bd20 100644
>> --- a/drivers/bluetooth/hci_ath.c
>> +++ b/drivers/bluetooth/hci_ath.c
>> @@ -162,11 +162,6 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>>   {
>>   	struct ath_struct *ath = hu->priv;
>>
>> -	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
>> -		kfree_skb(skb);
>> -		return 0;
>> -	}
>> -
>>   	/*
>>   	 * Update power management enable flag with parameters of
>>   	 * HCI sleep enable vendor specific HCI command.
>> @@ -183,10 +178,15 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>>   	/* Prepend skb with frame type */
>>   	memcpy(skb_push(skb, 1),&bt_cb(skb)->pkt_type, 1);
>>
>> -	skb_queue_tail(&ath->txq, skb);
>> -	set_bit(HCI_UART_SENDING,&hu->tx_state);
>> -
>> -	schedule_work(&ath->ctxtsw);
>> +	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
>> +		skb_queue_head(&ath->txq, skb);
>> +		clear_bit(HCI_UART_SENDING,&hu->tx_state);
>> +		hci_uart_tx_wakeup(hu);
>
> Seems you are giving priority to SCO packets, right? why?
>

Yes, There was some degradation in audio quality which improved when we 
gave priority to SCO.

Do you see any potential problem with that? I will re-verify this anyway.

Regards
Suraj

Re: [PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[Gustavo F. Padovan]

Hi Suraj,

* Suraj Sumangala <suraj@atheros.com> [2011-01-28 16:19:04 +0530]:

> This patch adds SCO over HCI support to Atheros AR300x HCI transport
> driver.
> 
> Signed-off-by: Suraj Sumangala <suraj@atheros.com>
> ---
>  drivers/bluetooth/hci_ath.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> index 6a160c1..161bd20 100644
> --- a/drivers/bluetooth/hci_ath.c
> +++ b/drivers/bluetooth/hci_ath.c
> @@ -162,11 +162,6 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>  {
>  	struct ath_struct *ath = hu->priv;
>  
> -	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
> -		kfree_skb(skb);
> -		return 0;
> -	}
> -
>  	/*
>  	 * Update power management enable flag with parameters of
>  	 * HCI sleep enable vendor specific HCI command.
> @@ -183,10 +178,15 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>  	/* Prepend skb with frame type */
>  	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
>  
> -	skb_queue_tail(&ath->txq, skb);
> -	set_bit(HCI_UART_SENDING, &hu->tx_state);
> -
> -	schedule_work(&ath->ctxtsw);
> +	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
> +		skb_queue_head(&ath->txq, skb);
> +		clear_bit(HCI_UART_SENDING, &hu->tx_state);
> +		hci_uart_tx_wakeup(hu);

Seems you are giving priority to SCO packets, right? why?

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[Suraj Sumangala]

Hi,

On 1/28/2011 4:19 PM, Suraj Sumangala wrote:
> This patch adds SCO over HCI support to Atheros AR300x HCI transport
> driver.
>
> Signed-off-by: Suraj Sumangala<suraj@atheros.com>
> ---
>   drivers/bluetooth/hci_ath.c |   18 +++++++++---------
>   1 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> index 6a160c1..161bd20 100644
> --- a/drivers/bluetooth/hci_ath.c
> +++ b/drivers/bluetooth/hci_ath.c

*ping*

Regards
Suraj

[PATCH] Bluetooth: Support SCO over HCI for Atheros AR300x Bluetooth device

[Suraj Sumangala]

This patch adds SCO over HCI support to Atheros AR300x HCI transport
driver.

Signed-off-by: Suraj Sumangala <suraj@atheros.com>
---
 drivers/bluetooth/hci_ath.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index 6a160c1..161bd20 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -162,11 +162,6 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 {
 	struct ath_struct *ath = hu->priv;
 
-	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
-		kfree_skb(skb);
-		return 0;
-	}
-
 	/*
 	 * Update power management enable flag with parameters of
 	 * HCI sleep enable vendor specific HCI command.
@@ -183,10 +178,15 @@ static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 	/* Prepend skb with frame type */
 	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
 
-	skb_queue_tail(&ath->txq, skb);
-	set_bit(HCI_UART_SENDING, &hu->tx_state);
-
-	schedule_work(&ath->ctxtsw);
+	if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
+		skb_queue_head(&ath->txq, skb);
+		clear_bit(HCI_UART_SENDING, &hu->tx_state);
+		hci_uart_tx_wakeup(hu);
+	} else {
+		skb_queue_tail(&ath->txq, skb);
+		set_bit(HCI_UART_SENDING, &hu->tx_state);
+		schedule_work(&ath->ctxtsw);
+	}
 
 	return 0;
 }
-- 
1.7.0.4