All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC v3 0/2] Disconnect reason in Kernel API
@ 2012-08-08  7:42 Mikel Astiz
  2012-08-08  7:42 ` [RFC v3 1/2] Bluetooth: Add more HCI error codes Mikel Astiz
  2012-08-08  7:42 ` [RFC v3 2/2] Bluetooth: mgmt: Add device disconnect reason Mikel Astiz
  0 siblings, 2 replies; 6+ messages in thread
From: Mikel Astiz @ 2012-08-08  7:42 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This patchset (split now in two patches) extends the management API to expose the disconnect reason in the device disconnection event.

The original motivation was to distinguish the timeout case from the rest, thus the first versions of the patch provided only this information.

v3 extends the original approach to also determine which side (local host or remote) initiated the ACL disconnection. This is IMO not very useful and misleading, but has still been considered interesting by the subsystem maintainers.

In order to test these patches, the old userland patch ("[RFC BlueZ v1]: mgmt-api: Add reason to device disconnect event") can be used, in terms of log traces, even though it needs the corresponding update.

>From original patch:

During the BlueZ meeting in Brazil it was proposed to add two more values to this enum: "Connection terminated by local host" and "Connection terminated by remote host". However, after some testing, it seems the result can be quite misleading. Therefore and given that there are no known use-cases that need this information (local vs remote disconnection), these two values have been dropped.

Mikel Astiz (2):
  Bluetooth: Add more HCI error codes
  Bluetooth: mgmt: Add device disconnect reason

 include/net/bluetooth/hci.h      |    3 +++
 include/net/bluetooth/hci_core.h |    2 +-
 include/net/bluetooth/mgmt.h     |    8 ++++++++
 net/bluetooth/hci_event.c        |   24 +++++++++++++++++++++---
 net/bluetooth/mgmt.c             |    9 +++++----
 5 files changed, 38 insertions(+), 8 deletions(-)

-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [RFC v3 1/2] Bluetooth: Add more HCI error codes
  2012-08-08  7:42 [RFC v3 0/2] Disconnect reason in Kernel API Mikel Astiz
@ 2012-08-08  7:42 ` Mikel Astiz
  2012-08-08 13:48   ` Marcel Holtmann
  2012-08-08  7:42 ` [RFC v3 2/2] Bluetooth: mgmt: Add device disconnect reason Mikel Astiz
  1 sibling, 1 reply; 6+ messages in thread
From: Mikel Astiz @ 2012-08-08  7:42 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Add more HCI error codes as defined in the specification.

Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
---
 include/net/bluetooth/hci.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7f19556..7253bdd 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -302,8 +302,11 @@ enum {
 
 /* ---- HCI Error Codes ---- */
 #define HCI_ERROR_AUTH_FAILURE		0x05
+#define HCI_ERROR_CONNECTION_TIMEOUT	0x08
 #define HCI_ERROR_REJ_BAD_ADDR		0x0f
 #define HCI_ERROR_REMOTE_USER_TERM	0x13
+#define HCI_ERROR_REMOTE_LOW_RESOURCES	0x14
+#define HCI_ERROR_REMOTE_POWER_OFF	0x15
 #define HCI_ERROR_LOCAL_HOST_TERM	0x16
 #define HCI_ERROR_PAIRING_NOT_ALLOWED	0x18
 
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [RFC v3 2/2] Bluetooth: mgmt: Add device disconnect reason
  2012-08-08  7:42 [RFC v3 0/2] Disconnect reason in Kernel API Mikel Astiz
  2012-08-08  7:42 ` [RFC v3 1/2] Bluetooth: Add more HCI error codes Mikel Astiz
@ 2012-08-08  7:42 ` Mikel Astiz
  2012-08-08 13:52   ` Marcel Holtmann
  1 sibling, 1 reply; 6+ messages in thread
From: Mikel Astiz @ 2012-08-08  7:42 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

MGMT_EV_DEVICE_DISCONNECTED will now expose the disconnection reason to
userland, distinguishing four possible values:

	0x00	Reason not known or unspecified
	0x01	ACL connection timeout
	0x02	ACL connection terminated by local host
	0x03	ACL connection terminated by remote host

Note that the local/remote distinction just determines which side
terminated the low-level ACL connection, regardless of the disconnection
of the higher-level profiles.

This can sometimes be misleading and thus must be used with care. For
example, some hardware combinations would report a locally initiated
disconnection even if the user turned Bluetooth off in the remote side.

Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
---
 include/net/bluetooth/hci_core.h |    2 +-
 include/net/bluetooth/mgmt.h     |    8 ++++++++
 net/bluetooth/hci_event.c        |   24 +++++++++++++++++++++---
 net/bluetooth/mgmt.c             |    9 +++++----
 4 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 41d9439..4fb0323 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1004,7 +1004,7 @@ int mgmt_device_connected(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 			  u8 addr_type, u32 flags, u8 *name, u8 name_len,
 			  u8 *dev_class);
 int mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
-			     u8 link_type, u8 addr_type);
+			     u8 link_type, u8 addr_type, u8 reason);
 int mgmt_disconnect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr,
 			   u8 link_type, u8 addr_type, u8 status);
 int mgmt_connect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 4348ee8..e8b86e0 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -405,7 +405,15 @@ struct mgmt_ev_device_connected {
 	__u8	eir[0];
 } __packed;
 
+#define MGMT_DEV_DISCONN_TIMEOUT	0x01
+#define MGMT_DEV_DISCONN_LOCAL_HOST	0x02
+#define MGMT_DEV_DISCONN_REMOTE		0x03
+
 #define MGMT_EV_DEVICE_DISCONNECTED	0x000C
+struct mgmt_ev_device_disconnected {
+	struct mgmt_addr_info addr;
+	__u8	reason;
+} __packed;
 
 #define MGMT_EV_CONNECT_FAILED		0x000D
 struct mgmt_ev_connect_failed {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0386e1e..1323341 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -29,6 +29,7 @@
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
+#include <net/bluetooth/mgmt.h>
 
 /* Handle HCI Event packets */
 
@@ -1906,12 +1907,29 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags) &&
 	    (conn->type == ACL_LINK || conn->type == LE_LINK)) {
-		if (ev->status != 0)
+		if (ev->status != 0) {
 			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
 					       conn->dst_type, ev->status);
-		else
+		} else {
+			u8 reason = 0;
+
+			switch (ev->reason) {
+			case HCI_ERROR_CONNECTION_TIMEOUT:
+				reason = MGMT_DEV_DISCONN_TIMEOUT;
+				break;
+			case HCI_ERROR_REMOTE_USER_TERM:
+			case HCI_ERROR_REMOTE_LOW_RESOURCES:
+			case HCI_ERROR_REMOTE_POWER_OFF:
+				reason = MGMT_DEV_DISCONN_REMOTE;
+				break;
+			case HCI_ERROR_LOCAL_HOST_TERM:
+				reason = MGMT_DEV_DISCONN_LOCAL_HOST;
+				break;
+			}
+
 			mgmt_device_disconnected(hdev, &conn->dst, conn->type,
-						 conn->dst_type);
+						 conn->dst_type, reason);
+		}
 	}
 
 	if (ev->status == 0) {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a3329cb..05d4b83 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3077,16 +3077,17 @@ static void unpair_device_rsp(struct pending_cmd *cmd, void *data)
 }
 
 int mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
-			     u8 link_type, u8 addr_type)
+			     u8 link_type, u8 addr_type, u8 reason)
 {
-	struct mgmt_addr_info ev;
+	struct mgmt_ev_device_disconnected ev;
 	struct sock *sk = NULL;
 	int err;
 
 	mgmt_pending_foreach(MGMT_OP_DISCONNECT, hdev, disconnect_rsp, &sk);
 
-	bacpy(&ev.bdaddr, bdaddr);
-	ev.type = link_to_bdaddr(link_type, addr_type);
+	bacpy(&ev.addr.bdaddr, bdaddr);
+	ev.addr.type = link_to_bdaddr(link_type, addr_type);
+	ev.reason = reason;
 
 	err = mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev),
 			 sk);
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [RFC v3 1/2] Bluetooth: Add more HCI error codes
  2012-08-08  7:42 ` [RFC v3 1/2] Bluetooth: Add more HCI error codes Mikel Astiz
@ 2012-08-08 13:48   ` Marcel Holtmann
  0 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2012-08-08 13:48 UTC (permalink / raw)
  To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz

Hi Mikel,

> Add more HCI error codes as defined in the specification.
> 
> Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
> ---
>  include/net/bluetooth/hci.h |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC v3 2/2] Bluetooth: mgmt: Add device disconnect reason
  2012-08-08  7:42 ` [RFC v3 2/2] Bluetooth: mgmt: Add device disconnect reason Mikel Astiz
@ 2012-08-08 13:52   ` Marcel Holtmann
  2012-08-09  7:43     ` Mikel Astiz
  0 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2012-08-08 13:52 UTC (permalink / raw)
  To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz

Hi Mikel,

> MGMT_EV_DEVICE_DISCONNECTED will now expose the disconnection reason to
> userland, distinguishing four possible values:
> 
> 	0x00	Reason not known or unspecified
> 	0x01	ACL connection timeout
> 	0x02	ACL connection terminated by local host
> 	0x03	ACL connection terminated by remote host

I think we need to leave ACL out here. Since that is not how we defined
what a connection is in mgmt terms.

> Note that the local/remote distinction just determines which side
> terminated the low-level ACL connection, regardless of the disconnection
> of the higher-level profiles.
> 
> This can sometimes be misleading and thus must be used with care. For
> example, some hardware combinations would report a locally initiated
> disconnection even if the user turned Bluetooth off in the remote side.

Please make sure that this comment makes it also into the API
description.

> 
> Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
> ---
>  include/net/bluetooth/hci_core.h |    2 +-
>  include/net/bluetooth/mgmt.h     |    8 ++++++++
>  net/bluetooth/hci_event.c        |   24 +++++++++++++++++++++---
>  net/bluetooth/mgmt.c             |    9 +++++----
>  4 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 41d9439..4fb0323 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1004,7 +1004,7 @@ int mgmt_device_connected(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>  			  u8 addr_type, u32 flags, u8 *name, u8 name_len,
>  			  u8 *dev_class);
>  int mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
> -			     u8 link_type, u8 addr_type);
> +			     u8 link_type, u8 addr_type, u8 reason);
>  int mgmt_disconnect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr,
>  			   u8 link_type, u8 addr_type, u8 status);
>  int mgmt_connect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 4348ee8..e8b86e0 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -405,7 +405,15 @@ struct mgmt_ev_device_connected {
>  	__u8	eir[0];
>  } __packed;
>  

Please also list MGMT_DEV_DISCONN_UNKNOWN here.

> +#define MGMT_DEV_DISCONN_TIMEOUT	0x01
> +#define MGMT_DEV_DISCONN_LOCAL_HOST	0x02
> +#define MGMT_DEV_DISCONN_REMOTE		0x03
> +
>  #define MGMT_EV_DEVICE_DISCONNECTED	0x000C
> +struct mgmt_ev_device_disconnected {
> +	struct mgmt_addr_info addr;
> +	__u8	reason;
> +} __packed;
>  
>  #define MGMT_EV_CONNECT_FAILED		0x000D
>  struct mgmt_ev_connect_failed {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 0386e1e..1323341 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -29,6 +29,7 @@
>  
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
> +#include <net/bluetooth/mgmt.h>
>  
>  /* Handle HCI Event packets */
>  
> @@ -1906,12 +1907,29 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>  
>  	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags) &&
>  	    (conn->type == ACL_LINK || conn->type == LE_LINK)) {
> -		if (ev->status != 0)
> +		if (ev->status != 0) {

While at it, turn this into if (ev->status).

>  			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
>  					       conn->dst_type, ev->status);
> -		else
> +		} else {
> +			u8 reason = 0;
> +
> +			switch (ev->reason) {
> +			case HCI_ERROR_CONNECTION_TIMEOUT:
> +				reason = MGMT_DEV_DISCONN_TIMEOUT;
> +				break;
> +			case HCI_ERROR_REMOTE_USER_TERM:
> +			case HCI_ERROR_REMOTE_LOW_RESOURCES:
> +			case HCI_ERROR_REMOTE_POWER_OFF:
> +				reason = MGMT_DEV_DISCONN_REMOTE;
> +				break;
> +			case HCI_ERROR_LOCAL_HOST_TERM:
> +				reason = MGMT_DEV_DISCONN_LOCAL_HOST;
> +				break;
> +			}

I would prefer a helper function to turn HCI error into reason.

> +
>  			mgmt_device_disconnected(hdev, &conn->dst, conn->type,
> -						 conn->dst_type);
> +						 conn->dst_type, reason);
> +		}
>  	}
>  
>  	if (ev->status == 0) {
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index a3329cb..05d4b83 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3077,16 +3077,17 @@ static void unpair_device_rsp(struct pending_cmd *cmd, void *data)
>  }
>  
>  int mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
> -			     u8 link_type, u8 addr_type)
> +			     u8 link_type, u8 addr_type, u8 reason)
>  {
> -	struct mgmt_addr_info ev;
> +	struct mgmt_ev_device_disconnected ev;
>  	struct sock *sk = NULL;
>  	int err;
>  
>  	mgmt_pending_foreach(MGMT_OP_DISCONNECT, hdev, disconnect_rsp, &sk);
>  
> -	bacpy(&ev.bdaddr, bdaddr);
> -	ev.type = link_to_bdaddr(link_type, addr_type);
> +	bacpy(&ev.addr.bdaddr, bdaddr);
> +	ev.addr.type = link_to_bdaddr(link_type, addr_type);
> +	ev.reason = reason;
>  
>  	err = mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev),
>  			 sk);

Otherwise I am fine with this.

Regards

Marcel



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC v3 2/2] Bluetooth: mgmt: Add device disconnect reason
  2012-08-08 13:52   ` Marcel Holtmann
@ 2012-08-09  7:43     ` Mikel Astiz
  0 siblings, 0 replies; 6+ messages in thread
From: Mikel Astiz @ 2012-08-09  7:43 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth, Mikel Astiz

Hi Marcel,

On Wed, Aug 8, 2012 at 3:52 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Mikel,
>
>> MGMT_EV_DEVICE_DISCONNECTED will now expose the disconnection reason to
>> userland, distinguishing four possible values:
>>
>>       0x00    Reason not known or unspecified
>>       0x01    ACL connection timeout
>>       0x02    ACL connection terminated by local host
>>       0x03    ACL connection terminated by remote host
>
> I think we need to leave ACL out here. Since that is not how we defined
> what a connection is in mgmt terms.
>
>> Note that the local/remote distinction just determines which side
>> terminated the low-level ACL connection, regardless of the disconnection
>> of the higher-level profiles.
>>
>> This can sometimes be misleading and thus must be used with care. For
>> example, some hardware combinations would report a locally initiated
>> disconnection even if the user turned Bluetooth off in the remote side.
>
> Please make sure that this comment makes it also into the API
> description.

I will send an updated version soon, with all your suggestions, along
with the updated userland patch where you can see the API
documentation.

<snip>

>> @@ -1906,12 +1907,29 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>
>>       if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags) &&
>>           (conn->type == ACL_LINK || conn->type == LE_LINK)) {
>> -             if (ev->status != 0)
>> +             if (ev->status != 0) {
>
> While at it, turn this into if (ev->status).

I will send a separate patch for this and other similar occurrences.
Let me know if you prefer to squash the change.

Cheers,
Mikel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-09  7:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-08  7:42 [RFC v3 0/2] Disconnect reason in Kernel API Mikel Astiz
2012-08-08  7:42 ` [RFC v3 1/2] Bluetooth: Add more HCI error codes Mikel Astiz
2012-08-08 13:48   ` Marcel Holtmann
2012-08-08  7:42 ` [RFC v3 2/2] Bluetooth: mgmt: Add device disconnect reason Mikel Astiz
2012-08-08 13:52   ` Marcel Holtmann
2012-08-09  7:43     ` Mikel Astiz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.