[PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined

[PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined

[Justin P. Mattock]

Im seeing this building the kernel with gcc 4.6.0
 CC [M]  drivers/bluetooth/hci_bcsp.o
drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined

Hopefully the below is a fix for this. Please let me know.
 Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/bluetooth/hci_bcsp.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index 40aec0f..0f892e7 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -182,7 +182,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
 	struct sk_buff *nskb;
 	u8 hdr[4], chan;
 	u16 BCSP_CRC_INIT(bcsp_txmsg_crc);
-	int rel, i;
+	int rel, i, ret;
 
 	switch (pkt_type) {
 	case HCI_ACLDATA_PKT:
@@ -243,8 +243,8 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
 
 	if (rel) {
 		hdr[0] |= 0x80 + bcsp->msgq_txseq;
-		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
-		bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
+		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
+		ret = ++(bcsp->msgq_txseq) & 0x07;
 	}
 
 	if (bcsp->use_crc)
-- 
1.7.1.rc1.21.gf3bd6

Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined

[Gustavo F. Padovan]

Hi Justin,

* Justin P. Mattock <justinmattock@gmail.com> [2010-06-26 23:47:26 -0700]:

> Im seeing this building the kernel with gcc 4.6.0
>  CC [M]  drivers/bluetooth/hci_bcsp.o
> drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
> drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
> 
> Hopefully the below is a fix for this. Please let me know.
>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
> 
> ---
>  drivers/bluetooth/hci_bcsp.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 40aec0f..0f892e7 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -182,7 +182,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
>  	struct sk_buff *nskb;
>  	u8 hdr[4], chan;
>  	u16 BCSP_CRC_INIT(bcsp_txmsg_crc);
> -	int rel, i;
> +	int rel, i, ret;
>  
>  	switch (pkt_type) {
>  	case HCI_ACLDATA_PKT:
> @@ -243,8 +243,8 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
>  
>  	if (rel) {
>  		hdr[0] |= 0x80 + bcsp->msgq_txseq;
> -		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> -		bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> +		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
> +		ret = ++(bcsp->msgq_txseq) & 0x07;
>  	}

What are trying to do here? That is completely wrong, you are losting
the next txseq to be sent.

And please do not bother the linux-bluetooth mailing list with patches
to other subsystems. Send them in a way that only Bluetooth patches will
come to linux-bluetooth.

Regards,

-- 
Gustavo F. Padovan
http://padovan.org

[PATCH] Bluetooth: Fix abuse of the preincrement operator

[David Howells]

Fix abuse of the preincrement operator as detected when building with gcc
4.6.0:

	 CC [M]  drivers/bluetooth/hci_bcsp.o
	drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
	drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined

Reported-by: Justin P. Mattock <justinmattock@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/bluetooth/hci_bcsp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index 40aec0f..42d69d4 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -244,7 +244,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
 	if (rel) {
 		hdr[0] |= 0x80 + bcsp->msgq_txseq;
 		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
-		bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
+		bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;
 	}
 
 	if (bcsp->use_crc)

Re: [PATCH] Bluetooth: Fix abuse of the preincrement operator

[Gustavo F. Padovan]

Hi David,

* David Howells <dhowells@redhat.com> [2010-06-28 13:57:52 +0100]:

> Fix abuse of the preincrement operator as detected when building with gcc
> 4.6.0:
> 
> 	 CC [M]  drivers/bluetooth/hci_bcsp.o
> 	drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
> 	drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
> 
> Reported-by: Justin P. Mattock <justinmattock@gmail.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  drivers/bluetooth/hci_bcsp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 40aec0f..42d69d4 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -244,7 +244,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
>  	if (rel) {
>  		hdr[0] |= 0x80 + bcsp->msgq_txseq;
>  		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> -		bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> +		bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;
>  	}
>  
>  	if (bcsp->use_crc)
> 

Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>

-- 
Gustavo F. Padovan
http://padovan.org

Re: [PATCH] Bluetooth: Fix abuse of the preincrement operator

[David Miller]

From: "Gustavo F. Padovan" <gustavo@padovan.org>
Date: Mon, 28 Jun 2010 10:12:30 -0300

> Hi David,
> 
> * David Howells <dhowells@redhat.com> [2010-06-28 13:57:52 +0100]:
> 
>> Fix abuse of the preincrement operator as detected when building with gcc
>> 4.6.0:
>> 
>> 	 CC [M]  drivers/bluetooth/hci_bcsp.o
>> 	drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
>> 	drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
>> 
>> Reported-by: Justin P. Mattock <justinmattock@gmail.com>
>> Signed-off-by: David Howells <dhowells@redhat.com>
...
> Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>

Applied, thanks everyone.

Re: [PATCH] Bluetooth: Fix abuse of the preincrement operator

[Justin P. Mattock]

On 06/28/2010 05:57 AM, David Howells wrote:
> Fix abuse of the preincrement operator as detected when building with gcc
> 4.6.0:
>
> 	 CC [M]  drivers/bluetooth/hci_bcsp.o
> 	drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
> 	drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
>
> Reported-by: Justin P. Mattock<justinmattock@gmail.com>
> Signed-off-by: David Howells<dhowells@redhat.com>
> ---
>
>   drivers/bluetooth/hci_bcsp.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 40aec0f..42d69d4 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -244,7 +244,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
>   	if (rel) {
>   		hdr[0] |= 0x80 + bcsp->msgq_txseq;
>   		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> -		bcsp->msgq_txseq = ++(bcsp->msgq_txseq)&  0x07;
> +		bcsp->msgq_txseq = (bcsp->msgq_txseq + 1)&  0x07;
>   	}
>
>   	if (bcsp->use_crc)
>
>

ahh.. so it's o.k. to add the value after bcsp->msgq_txseq instead of 
before. Anyways build clean over here..

Thanks!

Justin P. Mattock

Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined

[David Howells]

Justin P. Mattock <justinmattock@gmail.com> wrote:

> -		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> -		bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> +		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
> +		ret = ++(bcsp->msgq_txseq) & 0x07;

I don't know what you're trying to do here, but you seem to be trying to send
the computed value back in time.

The problem is that the compiler is confused about why a '++' operator makes
any sense here.  It doesn't.  It should be a '+ 1' instead.  I think what you
want is:

	-	bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
	+	bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;

David

Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined

[Bernd Petrovitsch]

On Mon, 2010-06-28 at 13:52 +0100, David Howells wrote:
> Justin P. Mattock <justinmattock@gmail.com> wrote:
> 
> > -		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> > -		bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> > +		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
> > +		ret = ++(bcsp->msgq_txseq) & 0x07;
> 
> I don't know what you're trying to do here, but you seem to be trying to send
> the computed value back in time.
> 
> The problem is that the compiler is confused about why a '++' operator makes

It's even worse as that expression is explicitly undefined (and should
be fixed anyways and unconditionally).

> any sense here.  It doesn't.  It should be a '+ 1' instead.  I think what you
> want is:
> 
> 	-	bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> 	+	bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;

Yes, that's looks like the most probable intention of it - at least for
one who doesn't know the bluetooth code.

	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined

[Justin P. Mattock]

On 06/28/2010 05:52 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com>  wrote:
>
>> -		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
>> -		bcsp->msgq_txseq = ++(bcsp->msgq_txseq)&  0x07;
>> +		BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
>> +		ret = ++(bcsp->msgq_txseq)&  0x07;
>
> I don't know what you're trying to do here, but you seem to be trying to send
> the computed value back in time.
>

I was under the impression that hdr[0] |= 0x80 + bcsp->msgq_txseq;
is computing a value for BT_DBG then ret = ++(bcsp->msgq_txseq)&  0x07
computes a value as well i.e.

BT_DBG("Sending packet with seqno %u", hdr[0] | ret);

> The problem is that the compiler is confused about why a '++' operator makes
> any sense here.  It doesn't.  It should be a '+ 1' instead.  I think what you
> want is:
>
> 	-	bcsp->msgq_txseq = ++(bcsp->msgq_txseq)&  0x07;
> 	+	bcsp->msgq_txseq = (bcsp->msgq_txseq + 1)&  0x07;
>
> David
>

yeah I did play around with the ++ and noticed the compiler would be 
satisfied if the ++ was not there, but didn't think to just add a + 1

Justin P. Mattock