Fixes built-in Bluetooth not working on Apple PowerBooks, regression from
commit 75fb0e324daa48ec458fb5c2960eb07b80cfad9d ('Bluetooth: Fix init sequence
for some CSR based controllers').
Cc: [email protected] [v3.4]
Signed-off-by: Michel Dänzer <[email protected]>
---
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
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
> > > 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.
>
> 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
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
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: [email protected] [v3.4]
> > > > Signed-off-by: Michel D=E4nzer <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> > > 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
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: [email protected] [v3.4]
> > > Signed-off-by: Michel Dänzer <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > 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
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: [email protected] [v3.4]
> > Signed-off-by: Michel D=E4nzer <[email protected]>
> > ---
> > 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 <[email protected]>
> 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
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: [email protected] [v3.4]
> Signed-off-by: Michel Dänzer <[email protected]>
> ---
> 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 <[email protected]>
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