[PATCH] bluetooth: opcode field of sent commands is little endian.

[PATCH] bluetooth: opcode field of sent commands is little endian.

[Michel Dänzer]

Fixes built-in Bluetooth not working on Apple PowerBooks, regression from
commit 75fb0e324daa48ec458fb5c2960eb07b80cfad9d ('Bluetooth: Fix init sequence
for some CSR based controllers').

Cc: stable@vger.kernel.org [v3.4]
Signed-off-by: Michel Dänzer <michel@daenzer.net>
---
 net/bluetooth/hci_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d6dc44c..e039e3d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -92,7 +92,7 @@ void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result)
 		 * command.
 		 */
 
-		if (cmd != HCI_OP_RESET || sent->opcode == HCI_OP_RESET)
+		if (cmd != HCI_OP_RESET || sent->opcode == cpu_to_le16(HCI_OP_RESET))
 			return;
 
 		skb = skb_clone(hdev->sent_cmd, GFP_ATOMIC);
-- 
1.7.10.4

Re: [PATCH] bluetooth: opcode field of sent commands is little endian.

[Marcel Holtmann]

Hi Michel,

> Fixes built-in Bluetooth not working on Apple PowerBooks, regression from
> commit 75fb0e324daa48ec458fb5c2960eb07b80cfad9d ('Bluetooth: Fix init sequence
> for some CSR based controllers').
> 
> Cc: stable@vger.kernel.org [v3.4]
> Signed-off-by: Michel Dänzer <michel@daenzer.net>
> ---
>  net/bluetooth/hci_core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index d6dc44c..e039e3d 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -92,7 +92,7 @@ void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result)
>  		 * command.
>  		 */
>  
> -		if (cmd != HCI_OP_RESET || sent->opcode == HCI_OP_RESET)
> +		if (cmd != HCI_OP_RESET || sent->opcode == cpu_to_le16(HCI_OP_RESET))
>  			return;

actually you could use __constant_cpu_to_le16() here.

That said, this got actually fixed differently upstream. So I prefer if
that patch gets merged into stable and not this one.

commit 1036b89042df96e71c0cb941be212f8053ecccc0
Author: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Date:   Mon Mar 12 15:59:33 2012 +0200

    Bluetooth: Fix opcode access in hci_complete

In general patches need to get into Linus' tree first before you can
even request them for stable.

Regards

Marcel

Re: [PATCH] bluetooth: opcode field of sent commands is little endian.

[Michel Dänzer]

On Son, 2012-06-24 at 23:51 -0700, Marcel Holtmann wrote:=20
> Hi Michel,
>=20
> > Fixes built-in Bluetooth not working on Apple PowerBooks, regression fr=
om
> > commit 75fb0e324daa48ec458fb5c2960eb07b80cfad9d ('Bluetooth: Fix init s=
equence
> > for some CSR based controllers').
> >=20
> > Cc: stable@vger.kernel.org [v3.4]
> > Signed-off-by: Michel D=E4nzer <michel@daenzer.net>
> > ---
> >  net/bluetooth/hci_core.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >=20
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index d6dc44c..e039e3d 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -92,7 +92,7 @@ void hci_req_complete(struct hci_dev *hdev, __u16 cmd=
, int result)
> >  		 * command.
> >  		 */
> > =20
> > -		if (cmd !=3D HCI_OP_RESET || sent->opcode =3D=3D HCI_OP_RESET)
> > +		if (cmd !=3D HCI_OP_RESET || sent->opcode =3D=3D cpu_to_le16(HCI_OP_=
RESET))
> >  			return;
>=20
> actually you could use __constant_cpu_to_le16() here.

Yes, but I checked and that's not used anywhere in the bluetooth code
yet, so I thought I'd stay consistent for now.


> That said, this got actually fixed differently upstream. So I prefer if
> that patch gets merged into stable and not this one.
>=20
> commit 1036b89042df96e71c0cb941be212f8053ecccc0
> Author: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> Date:   Mon Mar 12 15:59:33 2012 +0200
>=20
>     Bluetooth: Fix opcode access in hci_complete

Fine with me, though FWIW that not only doesn't use
__constant_cpu_to_le16() but actually swaps the non-constant value.

Also, it would have been nice if that fix was promoted to stable, so I
wouldn't have had to spend a good part of the weekend bisecting...


> In general patches need to get into Linus' tree first before you can
> even request them for stable.

I know, that's why I only put the stable tag in commit logs but don't
submit patches there via e-mail.


--=20
Earthling Michel D=E4nzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer

Re: [PATCH] bluetooth: opcode field of sent commands is little endian.

[Marcel Holtmann]

Hi Michel,

> > > Fixes built-in Bluetooth not working on Apple PowerBooks, regression from
> > > commit 75fb0e324daa48ec458fb5c2960eb07b80cfad9d ('Bluetooth: Fix init sequence
> > > for some CSR based controllers').
> > > 
> > > Cc: stable@vger.kernel.org [v3.4]
> > > Signed-off-by: Michel Dänzer <michel@daenzer.net>
> > > ---
> > >  net/bluetooth/hci_core.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index d6dc44c..e039e3d 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -92,7 +92,7 @@ void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result)
> > >  		 * command.
> > >  		 */
> > >  
> > > -		if (cmd != HCI_OP_RESET || sent->opcode == HCI_OP_RESET)
> > > +		if (cmd != HCI_OP_RESET || sent->opcode == cpu_to_le16(HCI_OP_RESET))
> > >  			return;
> > 
> > actually you could use __constant_cpu_to_le16() here.
> 
> Yes, but I checked and that's not used anywhere in the bluetooth code
> yet, so I thought I'd stay consistent for now.

not sure what code you are looking at, but I count 18 occurrences and we
have been fixing the ones we missed initially.

> > That said, this got actually fixed differently upstream. So I prefer if
> > that patch gets merged into stable and not this one.
> > 
> > commit 1036b89042df96e71c0cb941be212f8053ecccc0
> > Author: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > Date:   Mon Mar 12 15:59:33 2012 +0200
> > 
> >     Bluetooth: Fix opcode access in hci_complete
> 
> Fine with me, though FWIW that not only doesn't use
> __constant_cpu_to_le16() but actually swaps the non-constant value.

Don't see what point you are trying to make here. Swapping the value
from the actual command structure is always fine with me.

> Also, it would have been nice if that fix was promoted to stable, so I
> wouldn't have had to spend a good part of the weekend bisecting...

Thinks like this happen. However after you bisected the issue you could
have just checked what is in Linus' or bluetooth-next tree. Since the
first thing I did is go through bluetooth-next and check if we have not
fixed that yet.

Regards

Marcel

Re: [PATCH] bluetooth: opcode field of sent commands is little endian.

[Michel Dänzer]

On Mon, 2012-06-25 at 00:22 -0700, Marcel Holtmann wrote:=20
> Hi Michel,
>=20
> > > > Fixes built-in Bluetooth not working on Apple PowerBooks, regressio=
n from
> > > > commit 75fb0e324daa48ec458fb5c2960eb07b80cfad9d ('Bluetooth: Fix in=
it sequence
> > > > for some CSR based controllers').
> > > >=20
> > > > Cc: stable@vger.kernel.org [v3.4]
> > > > Signed-off-by: Michel D=E4nzer <michel@daenzer.net>
> > > > ---
> > > >  net/bluetooth/hci_core.c |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >=20
> > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > index d6dc44c..e039e3d 100644
> > > > --- a/net/bluetooth/hci_core.c
> > > > +++ b/net/bluetooth/hci_core.c
> > > > @@ -92,7 +92,7 @@ void hci_req_complete(struct hci_dev *hdev, __u16=
 cmd, int result)
> > > >  		 * command.
> > > >  		 */
> > > > =20
> > > > -		if (cmd !=3D HCI_OP_RESET || sent->opcode =3D=3D HCI_OP_RESET)
> > > > +		if (cmd !=3D HCI_OP_RESET || sent->opcode =3D=3D cpu_to_le16(HCI=
_OP_RESET))
> > > >  			return;
> > >=20
> > > actually you could use __constant_cpu_to_le16() here.
> >=20
> > Yes, but I checked and that's not used anywhere in the bluetooth code
> > yet, so I thought I'd stay consistent for now.
>=20
> not sure what code you are looking at, but I count 18 occurrences and we
> have been fixing the ones we missed initially.

Okay, good then. As you probably noticed from the rest of my posts, I
only checked up to 3.4.


> > > That said, this got actually fixed differently upstream. So I prefer =
if
> > > that patch gets merged into stable and not this one.
> > >=20
> > > commit 1036b89042df96e71c0cb941be212f8053ecccc0
> > > Author: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > Date:   Mon Mar 12 15:59:33 2012 +0200
> > >=20
> > >     Bluetooth: Fix opcode access in hci_complete
> >=20
> > Fine with me, though FWIW that not only doesn't use
> > __constant_cpu_to_le16() but actually swaps the non-constant value.
>=20
> Don't see what point you are trying to make here. Swapping the value
> from the actual command structure is always fine with me.

The point is that the result of swapping a constant value is just
another constant value, whereas the fix in mainline swaps a value from
memory. Not a big deal.


> > Also, it would have been nice if that fix was promoted to stable, so I
> > wouldn't have had to spend a good part of the weekend bisecting...
>=20
> Thinks like this happen. However after you bisected the issue you could
> have just checked what is in Linus' or bluetooth-next tree.

You're probably right. It just didn't occur to me that someone would
have fixed this but not forwarded the fix to stable, because I generally
do that. :}


Will you submit the fix to stable, or should I?


--=20
Earthling Michel D=E4nzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer

Re: [PATCH] bluetooth: opcode field of sent commands is little endian.

[Andrei Emeltchenko]

Hi Michel,

On Mon, Jun 25, 2012 at 09:32:50AM +0200, Michel Dänzer wrote:
> > > Also, it would have been nice if that fix was promoted to stable, so I
> > > wouldn't have had to spend a good part of the weekend bisecting...
> > 
> > Thinks like this happen. However after you bisected the issue you could
> > have just checked what is in Linus' or bluetooth-next tree.
> 
> You're probably right. It just didn't occur to me that someone would
> have fixed this but not forwarded the fix to stable, because I generally
> do that. :}

Sorry for that, this and bunch of other fixes for byte order bugs needs to
be promoted to stable.

> Will you submit the fix to stable, or should I?

I added Gustavo to CC as he usually makes pull requests.

Best regards 
Andrei Emeltchenko 

RE: [PATCH] bluetooth: opcode field of sent commands is little endian.

[David Laight]

=20
> > > Fine with me, though FWIW that not only doesn't use
> > > __constant_cpu_to_le16() but actually swaps the non-constant =
value.
> >=20
> > Don't see what point you are trying to make here. Swapping the value
> > from the actual command structure is always fine with me.
>=20
> The point is that the result of swapping a constant value is just
> another constant value, whereas the fix in mainline swaps a value from
> memory. Not a big deal.

Surely, but surely, the definition of cpu_to_le16() uses
gcc 'magic' to determine that the argument is a constant
and then automatically selects the 'constant' form.

	David

Re: [PATCH] bluetooth: opcode field of sent commands is little endian.

[Michel Dänzer]

On Mon, 2012-06-25 at 10:20 +0100, David Laight wrote:=20
> > > > Fine with me, though FWIW that not only doesn't use
> > > > __constant_cpu_to_le16() but actually swaps the non-constant value.
> > >=20
> > > Don't see what point you are trying to make here. Swapping the value
> > > from the actual command structure is always fine with me.
> >=20
> > The point is that the result of swapping a constant value is just
> > another constant value, whereas the fix in mainline swaps a value from
> > memory. Not a big deal.
>=20
> Surely, but surely, the definition of cpu_to_le16() uses
> gcc 'magic' to determine that the argument is a constant
> and then automatically selects the 'constant' form.

It can only do that if the argument is constant in the first place
though. :)


--=20
Earthling Michel D=E4nzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer