[PATCH] Revert "Bluetooth: Always enable management interface"

[PATCH] Revert "Bluetooth: Always enable management interface"

[Keith Packard]

This reverts commit 4b95a24ce12c4545fd7d2e3075841dc3119d1d71.

My USB bluetooth device does not show up with this patch in place.

Signed-off-by: Keith Packard <keithp@keithp.com>
---

This patch seems so innocuous, but when added to the kernel, it keeps
my USB bluetooth device from appearing to user mode at all. With the
mgmt interface enabled:

	$ hcitool dev
	Devices:

With this patch reverted:

	$ hcitool dev
	Devices:
		hci0	04:0C:CE:DF:7B:C9

I bisected to this revision, and then reverted it on top of version

	v3.3-6972-ge22057c	e22057c8599373e5caef0bc42bdb95d2a361ab0d

(which is where I'm trying to move drm-intel-fixes to)

I think this is the third consecutive merge window that has broken my
bluetooth interface in some way? At least I know enough to check
now...

 net/bluetooth/hci_sock.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 63afd23..d8b16cf 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -50,6 +50,8 @@
 #include <net/bluetooth/hci_core.h>
 #include <net/bluetooth/hci_mon.h>
 
+static bool enable_mgmt;
+
 static atomic_t monitor_promisc = ATOMIC_INIT(0);
 
 /* ----- HCI socket interface ----- */
@@ -649,7 +651,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
 		break;
 
 	case HCI_CHANNEL_CONTROL:
-		if (haddr.hci_dev != HCI_DEV_NONE) {
+		if (haddr.hci_dev != HCI_DEV_NONE || !enable_mgmt) {
 			err = -EINVAL;
 			goto done;
 		}
@@ -1127,3 +1129,6 @@ void hci_sock_cleanup(void)
 
 	proto_unregister(&hci_sk_proto);
 }
+
+module_param(enable_mgmt, bool, 0644);
+MODULE_PARM_DESC(enable_mgmt, "Enable Management interface");
-- 
1.7.9

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[David Miller]

From: Keith Packard <keithp@keithp.com>
Date: Wed, 28 Mar 2012 18:19:18 -0700

> This reverts commit 4b95a24ce12c4545fd7d2e3075841dc3119d1d71.
> 
> My USB bluetooth device does not show up with this patch in place.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Bluetooth folks please look over this, thanks.

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Marcel Holtmann]

Hi Keith,

> This reverts commit 4b95a24ce12c4545fd7d2e3075841dc3119d1d71.
> 
> My USB bluetooth device does not show up with this patch in place.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
> 
> This patch seems so innocuous, but when added to the kernel, it keeps
> my USB bluetooth device from appearing to user mode at all. With the
> mgmt interface enabled:
> 
> 	$ hcitool dev
> 	Devices:
> 
> With this patch reverted:
> 
> 	$ hcitool dev
> 	Devices:
> 		hci0	04:0C:CE:DF:7B:C9

so this is seriously strange and I don't have an explanation for it
right now. Something breaking on this level means that Bluetooth devices
are not even registered. And that would be an obvious bug to detect.

Can you check with hciconfig -a if the device is actually there, but
might have some weird flags set.

> I bisected to this revision, and then reverted it on top of version
> 
> 	v3.3-6972-ge22057c	e22057c8599373e5caef0bc42bdb95d2a361ab0d
> 
> (which is where I'm trying to move drm-intel-fixes to)
> 
> I think this is the third consecutive merge window that has broken my
> bluetooth interface in some way? At least I know enough to check
> now...
> 
>  net/bluetooth/hci_sock.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 63afd23..d8b16cf 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -50,6 +50,8 @@
>  #include <net/bluetooth/hci_core.h>
>  #include <net/bluetooth/hci_mon.h>
>  
> +static bool enable_mgmt;
> +
>  static atomic_t monitor_promisc = ATOMIC_INIT(0);
>  
>  /* ----- HCI socket interface ----- */
> @@ -649,7 +651,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
>  		break;
>  
>  	case HCI_CHANNEL_CONTROL:
> -		if (haddr.hci_dev != HCI_DEV_NONE) {
> +		if (haddr.hci_dev != HCI_DEV_NONE || !enable_mgmt) {
>  			err = -EINVAL;
>  			goto done;
>  		}

I have no idea on how this can break anything for you. Since tools like
hcitool and hciconfig use HCI_CHANNEL_RAW. This must be a nasty side
effect somewhere if this is really fix it for you.

Can you start your bluetoothd with -P mgmtops to check if this might be
causing some side effects.

Regards

Marcel

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Gustavo Padovan]

Hi Keith,

* Keith Packard <keithp@keithp.com> [2012-03-28 18:19:18 -0700]:

> This reverts commit 4b95a24ce12c4545fd7d2e3075841dc3119d1d71.
> 
> My USB bluetooth device does not show up with this patch in place.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
> 
> This patch seems so innocuous, but when added to the kernel, it keeps
> my USB bluetooth device from appearing to user mode at all. With the
> mgmt interface enabled:

It is because it changes the way the userspace <-> kernel interface. We added a
new interface and that flag was preventing the code to use it by default, but
now that we finished to implement the new interface it will be enabled by default.

A consequence is that you'll need to use newer version of bluez, at least 4.99.
Or call the bluetooth daemon (from a pre 4.99 version) with a parameter to
disable the new interface:

$ bluetoothd -P mgmtops

Then your bluetooth will be back.

	Gustavo

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Keith Packard]

<#part sign=pgpmime>
On Wed, 28 Mar 2012 23:28:39 -0300, Gustavo Padovan <gustavo@padovan.org> wrote:

> A consequence is that you'll need to use newer version of bluez, at least 4.99.
> Or call the bluetooth daemon (from a pre 4.99 version) with a parameter to
> disable the new interface:

That's not OK -- you're breaking user space with this kernel change. I
know I get bashed every time I suggest that we 'fix' the kernel and
require new user space X bits...

-- 
keith.packard@intel.com

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Marcel Holtmann]

Hi Keith,

> > A consequence is that you'll need to use newer version of bluez, at least 4.99.
> > Or call the bluetooth daemon (from a pre 4.99 version) with a parameter to
> > disable the new interface:
> 
> That's not OK -- you're breaking user space with this kernel change. I
> know I get bashed every time I suggest that we 'fix' the kernel and
> require new user space X bits...

I think we actually have a broken user space in this case. I do have a
hunch on what went wrong. It is not the kernel's fault here.

If I am anywhere right, then bluetoothd fails to bring up the device.
That is why you don't see anything with "hcitool dev", but the device
should be present. Check "hciconfig -a".

Check your bluetoothd startup line and add "-P mgmtops". That will
disable the broken plugin.

Regards

Marcel

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[David Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Thu, 29 Mar 2012 05:33:42 +0200

> I think we actually have a broken user space in this case. I do have a
> hunch on what went wrong. It is not the kernel's fault here.

I call bullshit.

It doesn't matter what userspace is doing, "broken" or however you
want to call it.  The bluez on everyone's computer broke because of
the kernel change.

Therefore you must revert the kernel change or modify it so that
existing userland continues to work properly.

There is no other proper course of action.

And to think, I gave you bluetooth guys shit because of coding style
problems, that appears to have been just the tip of the iceberg.

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[David Miller]

From: Keith Packard <keithp@keithp.com>
Date: Wed, 28 Mar 2012 19:35:44 -0700

> <#part sign=pgpmime>
> On Wed, 28 Mar 2012 23:28:39 -0300, Gustavo Padovan <gustavo@padovan.org> wrote:
> 
>> A consequence is that you'll need to use newer version of bluez, at least 4.99.
>> Or call the bluetooth daemon (from a pre 4.99 version) with a parameter to
>> disable the new interface:
> 
> That's not OK -- you're breaking user space with this kernel change.

+1

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Gustavo Padovan]

Hi Keith,

* Keith Packard <keithp@keithp.com> [2012-03-28 19:35:44 -0700]:

> <#part sign=pgpmime>
> On Wed, 28 Mar 2012 23:28:39 -0300, Gustavo Padovan <gustavo@padovan.org> wrote:
> 
> > A consequence is that you'll need to use newer version of bluez, at least 4.99.
> > Or call the bluetooth daemon (from a pre 4.99 version) with a parameter to
> > disable the new interface:
> 
> That's not OK -- you're breaking user space with this kernel change. I
> know I get bashed every time I suggest that we 'fix' the kernel and
> require new user space X bits...

Can you try the following patch? It should fix the compatibility problem you had.

	Gustavo
--

commit d21c1177b9cf067809ccee2746633cfea3a8b062
Author: Gustavo Padovan <gustavo@padovan.org>
Date:   Thu Mar 29 09:47:53 2012 -0300

    Bluetooth: Fix userspace compatibility issue with mgmt interface
    
    To ensure that old user space versions do not accidentally pick up and
    try to use the management channel, use a different channel number.
    
    Reported-by: Keith Packard <keithp@keithp.com>
    Acked-by: Johan Hedberg <johan.hedberg@intel.com>
    Acked-by: Marcel Holtmann <marcel@holtmann.org>
    Signed-off-by: Gustavo Padovan <gustavo@padovan.org>

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 344b0f9..ba7f148 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1327,8 +1327,8 @@ struct sockaddr_hci {
 #define HCI_DEV_NONE   0xffff
 
 #define HCI_CHANNEL_RAW                0
-#define HCI_CHANNEL_CONTROL    1
 #define HCI_CHANNEL_MONITOR    2
+#define HCI_CHANNEL_CONTROL    3
 
 struct hci_filter {
        unsigned long type_mask;

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Gustavo Padovan]

Hi Keith,

* Gustavo Padovan <gustavo@padovan.org> [2012-03-30 03:54:56 -0300]:

> Hi Keith,
> 
> * Keith Packard <keithp@keithp.com> [2012-03-28 19:35:44 -0700]:
> 
> > <#part sign=pgpmime>
> > On Wed, 28 Mar 2012 23:28:39 -0300, Gustavo Padovan <gustavo@padovan.org> wrote:
> > 
> > > A consequence is that you'll need to use newer version of bluez, at least 4.99.
> > > Or call the bluetooth daemon (from a pre 4.99 version) with a parameter to
> > > disable the new interface:
> > 
> > That's not OK -- you're breaking user space with this kernel change. I
> > know I get bashed every time I suggest that we 'fix' the kernel and
> > require new user space X bits...
> 
> Can you try the following patch? It should fix the compatibility problem you had.

Did you had chance to test this patch?

	Gustavo

> 
> commit d21c1177b9cf067809ccee2746633cfea3a8b062
> Author: Gustavo Padovan <gustavo@padovan.org>
> Date:   Thu Mar 29 09:47:53 2012 -0300
> 
>     Bluetooth: Fix userspace compatibility issue with mgmt interface
>     
>     To ensure that old user space versions do not accidentally pick up and
>     try to use the management channel, use a different channel number.
>     
>     Reported-by: Keith Packard <keithp@keithp.com>
>     Acked-by: Johan Hedberg <johan.hedberg@intel.com>
>     Acked-by: Marcel Holtmann <marcel@holtmann.org>
>     Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 344b0f9..ba7f148 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1327,8 +1327,8 @@ struct sockaddr_hci {
>  #define HCI_DEV_NONE   0xffff
>  
>  #define HCI_CHANNEL_RAW                0
> -#define HCI_CHANNEL_CONTROL    1
>  #define HCI_CHANNEL_MONITOR    2
> +#define HCI_CHANNEL_CONTROL    3
>  
>  struct hci_filter {
>         unsigned long type_mask;

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Keith Packard]

<#part sign=pgpmime>
On Fri, 30 Mar 2012 03:54:56 -0300, Gustavo Padovan <gustavo@padovan.org> wrote:

> Can you try the following patch? It should fix the compatibility
> problem you had.

Yes, the kernel with that patch (without the revert) works now.

-- 
keith.packard@intel.com

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Gustavo Padovan]

Hi Keith,

* Keith Packard <keithp@keithp.com> [2012-04-05 15:24:58 -0700]:

> <#part sign=pgpmime>
> On Fri, 30 Mar 2012 03:54:56 -0300, Gustavo Padovan <gustavo@padovan.org> wrote:
> 
> > Can you try the following patch? It should fix the compatibility
> > problem you had.
> 
> Yes, the kernel with that patch (without the revert) works now.

Thanks for testing, I will push this to mainline ASAP.

	Gustavo

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[Keith Packard]

<#part sign=pgpmime>
On Wed, 28 Mar 2012 23:28:39 -0300, Gustavo Padovan <gustavo@padovan.org> wrote:

> $ bluetoothd -P mgmtops
> 
> Then your bluetooth will be back.

Indeed. Please fix the kernel so that existing user space works again.

-- 
keith.packard@intel.com

Re: [PATCH] Revert "Bluetooth: Always enable management interface"

[David Miller]

From: Gustavo Padovan <gustavo@padovan.org>
Date: Wed, 28 Mar 2012 23:28:39 -0300

> A consequence is that you'll need to use newer version of bluez, at
> least 4.99.

That's complete bullshit and you know it.

Existing userspace worked, you guys broke it with a kernel change.

Therefore the kernel change needs to be reverted.