[RFC 0/5] Disconnect complete refactoring

[RFC 0/5] Disconnect complete refactoring

[Andre Guedes]

Hi all,

This patch set does some code refactoring in disconnect complete event handler
and related functions from Mgmt layer.

This refactoring used to be one patch from LE auto connection patch set, but
since Marcel asked me to do more changes, I put all these refactoring patches
in its own patch set.

Regards,

Andre Guedes (5):
  Bluetooth: Check address in mgmt_disconnect_failed()
  Bluetooth: Add an extra check in mgmt_device_disconnected()
  Bluetooth: Remove link type check in hci_disconn_complete_evt()
  Bluetooth: Remove unneeded check in hci_disconn_complete_evt()
  Bluetooth: Refactor hci_disconn_complete_evt

 net/bluetooth/hci_event.c | 61 ++++++++++++++++++++++-------------------------
 net/bluetooth/mgmt.c      | 15 +++++++++++-
 2 files changed, 43 insertions(+), 33 deletions(-)

-- 
1.8.4

[RFC 1/5] Bluetooth: Check address in mgmt_disconnect_failed()

[Andre Guedes]

Check the address and address type in mgmt_disconnect_failed() otherwise
we may wrongly fail the MGMT_OP_DISCONNECT command.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/mgmt.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 22cf547..6a74aa7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4613,6 +4613,8 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
 void mgmt_disconnect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr,
 			    u8 link_type, u8 addr_type, u8 status)
 {
+	u8 bdaddr_type = link_to_bdaddr(link_type, addr_type);
+	struct mgmt_cp_disconnect *cp;
 	struct mgmt_rp_disconnect rp;
 	struct pending_cmd *cmd;
 
@@ -4623,8 +4625,16 @@ void mgmt_disconnect_failed(struct hci_dev *hdev, bdaddr_t *bdaddr,
 	if (!cmd)
 		return;
 
+	cp = cmd->param;
+
+	if (bacmp(bdaddr, &cp->addr.bdaddr))
+		return;
+
+	if (cp->addr.type != bdaddr_type)
+		return;
+
 	bacpy(&rp.addr.bdaddr, bdaddr);
-	rp.addr.type = link_to_bdaddr(link_type, addr_type);
+	rp.addr.type = bdaddr_type;
 
 	cmd_complete(cmd->sk, cmd->index, MGMT_OP_DISCONNECT,
 		     mgmt_status(status), &rp, sizeof(rp));
-- 
1.8.4

[RFC 2/5] Bluetooth: Add an extra check in mgmt_device_disconnected()

[Andre Guedes]

This patch adds an extra check in mgmt_device_disconnected() so we only
send the "Device Disconnected" event if it is ACL_LINK or LE_LINK link
type.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/mgmt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6a74aa7..a03ca3c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4595,6 +4595,9 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
 	struct mgmt_ev_device_disconnected ev;
 	struct sock *sk = NULL;
 
+	if (link_type != ACL_LINK && link_type != LE_LINK)
+		return;
+
 	mgmt_pending_foreach(MGMT_OP_DISCONNECT, hdev, disconnect_rsp, &sk);
 
 	bacpy(&ev.addr.bdaddr, bdaddr);
-- 
1.8.4

[RFC 3/5] Bluetooth: Remove link type check in hci_disconn_complete_evt()

[Andre Guedes]

We can safely remove the link type check from hci_disconn_complete_
evt() since this check in not required for mgmt_disconnect_failed()
and mgmt_device_disconnected() does it internally.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 5935f74..4673cfb 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1795,8 +1795,7 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (ev->status == 0)
 		conn->state = BT_CLOSED;
 
-	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags) &&
-	    (conn->type == ACL_LINK || conn->type == LE_LINK)) {
+	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags)) {
 		if (ev->status) {
 			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
 					       conn->dst_type, ev->status);
-- 
1.8.4

[RFC 4/5] Bluetooth: Remove unneeded check in hci_disconn_complete_evt()

[Andre Guedes]

According to b644ba336 (patch that introduced HCI_CONN_MGMT_CONNECTED
flag), the HCI_CONN_MGMT_CONNECTED flag tracks when mgmt has been
notified about the connection.

That being said, there is no point in checking this flag in hci_
disconn_complete_evt() since neither mgmt_disconnect_failed() nor
mgmt_device_disconnected() depend on it. Below follows more details:
  * mgmt_disconnect_failed() removes pending MGMT_OP_DISCONNECT
    commands, it doesn't matter if that connection was notified or not.
  * mgmt_device_disconnected() sends the mgmt event only if the link
    type is ACL_LINK or LE_LINK. For those connection type, the flag is
    always set.

So this patch removes the HCI_CONN_MGMT_CONNECTED check.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 4673cfb..9225a9c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1795,16 +1795,14 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (ev->status == 0)
 		conn->state = BT_CLOSED;
 
-	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags)) {
-		if (ev->status) {
-			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
-					       conn->dst_type, ev->status);
-		} else {
-			u8 reason = hci_to_mgmt_reason(ev->reason);
+	if (ev->status) {
+		mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
+				conn->dst_type, ev->status);
+	} else {
+		u8 reason = hci_to_mgmt_reason(ev->reason);
 
-			mgmt_device_disconnected(hdev, &conn->dst, conn->type,
-						 conn->dst_type, reason);
-		}
+		mgmt_device_disconnected(hdev, &conn->dst, conn->type,
+				conn->dst_type, reason);
 	}
 
 	if (ev->status == 0) {
-- 
1.8.4

[RFC 5/5] Bluetooth: Refactor hci_disconn_complete_evt

[Andre Guedes]

hci_disconn_complete_evt() logic is more complicated than what it
should be, making it hard to follow and add new features.

So this patch does some code refactoring by handling the error cases
in the beginning of the function and by moving the main flow into the
first level of function scope. No change is done in the event handling
logic itself.

Besides organizing this messy code, this patch makes easier to add
code for handling LE auto connection (which will be added in a further
patch).

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 56 +++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 9225a9c..981d858 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1782,7 +1782,9 @@ static u8 hci_to_mgmt_reason(u8 err)
 static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_disconn_complete *ev = (void *) skb->data;
+	u8 reason = hci_to_mgmt_reason(ev->reason);
 	struct hci_conn *conn;
+	u8 type;
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
 
@@ -1792,40 +1794,38 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (!conn)
 		goto unlock;
 
-	if (ev->status == 0)
-		conn->state = BT_CLOSED;
-
 	if (ev->status) {
 		mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
-				conn->dst_type, ev->status);
-	} else {
-		u8 reason = hci_to_mgmt_reason(ev->reason);
-
-		mgmt_device_disconnected(hdev, &conn->dst, conn->type,
-				conn->dst_type, reason);
+				       conn->dst_type, ev->status);
+		goto unlock;
 	}
 
-	if (ev->status == 0) {
-		u8 type = conn->type;
+	conn->state = BT_CLOSED;
 
-		if (type == ACL_LINK && conn->flush_key)
-			hci_remove_link_key(hdev, &conn->dst);
-		hci_proto_disconn_cfm(conn, ev->reason);
-		hci_conn_del(conn);
+	mgmt_device_disconnected(hdev, &conn->dst, conn->type,
+				 conn->dst_type, reason);
 
-		/* Re-enable advertising if necessary, since it might
-		 * have been disabled by the connection. From the
-		 * HCI_LE_Set_Advertise_Enable command description in
-		 * the core specification (v4.0):
-		 * "The Controller shall continue advertising until the Host
-		 * issues an LE_Set_Advertise_Enable command with
-		 * Advertising_Enable set to 0x00 (Advertising is disabled)
-		 * or until a connection is created or until the Advertising
-		 * is timed out due to Directed Advertising."
-		 */
-		if (type == LE_LINK)
-			mgmt_reenable_advertising(hdev);
-	}
+
+	if (conn->type == ACL_LINK && conn->flush_key)
+		hci_remove_link_key(hdev, &conn->dst);
+
+	type = conn->type;
+
+	hci_proto_disconn_cfm(conn, ev->reason);
+	hci_conn_del(conn);
+
+	/* Re-enable advertising if necessary, since it might
+	 * have been disabled by the connection. From the
+	 * HCI_LE_Set_Advertise_Enable command description in
+	 * the core specification (v4.0):
+	 * "The Controller shall continue advertising until the Host
+	 * issues an LE_Set_Advertise_Enable command with
+	 * Advertising_Enable set to 0x00 (Advertising is disabled)
+	 * or until a connection is created or until the Advertising
+	 * is timed out due to Directed Advertising."
+	 */
+	if (type == LE_LINK)
+	   mgmt_reenable_advertising(hdev);
 
 unlock:
 	hci_dev_unlock(hdev);
-- 
1.8.4

Re: [RFC 4/5] Bluetooth: Remove unneeded check in hci_disconn_complete_evt()

[Marcel Holtmann]

Hi Andre,

> According to b644ba336 (patch that introduced HCI_CONN_MGMT_CONNECTED
> flag), the HCI_CONN_MGMT_CONNECTED flag tracks when mgmt has been
> notified about the connection.
> 
> That being said, there is no point in checking this flag in hci_
> disconn_complete_evt() since neither mgmt_disconnect_failed() nor
> mgmt_device_disconnected() depend on it. Below follows more details:
>  * mgmt_disconnect_failed() removes pending MGMT_OP_DISCONNECT
>    commands, it doesn't matter if that connection was notified or not.
>  * mgmt_device_disconnected() sends the mgmt event only if the link
>    type is ACL_LINK or LE_LINK. For those connection type, the flag is
>    always set.
> 
> So this patch removes the HCI_CONN_MGMT_CONNECTED check.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_event.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 4673cfb..9225a9c 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1795,16 +1795,14 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 	if (ev->status == 0)
> 		conn->state = BT_CLOSED;
> 
> -	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags)) {
> -		if (ev->status) {
> -			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
> -					       conn->dst_type, ev->status);
> -		} else {
> -			u8 reason = hci_to_mgmt_reason(ev->reason);
> +	if (ev->status) {
> +		mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
> +				conn->dst_type, ev->status);

something went wrong with the coding style here.

> +	} else {
> +		u8 reason = hci_to_mgmt_reason(ev->reason);
> 
> -			mgmt_device_disconnected(hdev, &conn->dst, conn->type,
> -						 conn->dst_type, reason);
> -		}
> +		mgmt_device_disconnected(hdev, &conn->dst, conn->type,
> +				conn->dst_type, reason);

And here.

> 	}
> 
> 	if (ev->status == 0) {

Regards

Marcel

Re: [RFC 0/5] Disconnect complete refactoring

[Marcel Holtmann]

Hi Andre,

> This patch set does some code refactoring in disconnect complete event handler
> and related functions from Mgmt layer.
> 
> This refactoring used to be one patch from LE auto connection patch set, but
> since Marcel asked me to do more changes, I put all these refactoring patches
> in its own patch set.
> 
> Regards,
> 
> Andre Guedes (5):
>  Bluetooth: Check address in mgmt_disconnect_failed()
>  Bluetooth: Add an extra check in mgmt_device_disconnected()
>  Bluetooth: Remove link type check in hci_disconn_complete_evt()
>  Bluetooth: Remove unneeded check in hci_disconn_complete_evt()
>  Bluetooth: Refactor hci_disconn_complete_evt
> 
> net/bluetooth/hci_event.c | 61 ++++++++++++++++++++++-------------------------
> net/bluetooth/mgmt.c      | 15 +++++++++++-
> 2 files changed, 43 insertions(+), 33 deletions(-)

I applied the first 3 patches of this series to bluetooth-next tree. You need to fix up patch 4 and resend the rest.

Regards

Marcel

Re: [RFC 4/5] Bluetooth: Remove unneeded check in hci_disconn_complete_evt()

[Andre Guedes]

Hi Marcel,

On 02/11/13 15:14, Marcel Holtmann wrote:
> Hi Andre,
>
>> According to b644ba336 (patch that introduced HCI_CONN_MGMT_CONNECTED
>> flag), the HCI_CONN_MGMT_CONNECTED flag tracks when mgmt has been
>> notified about the connection.
>>
>> That being said, there is no point in checking this flag in hci_
>> disconn_complete_evt() since neither mgmt_disconnect_failed() nor
>> mgmt_device_disconnected() depend on it. Below follows more details:
>>   * mgmt_disconnect_failed() removes pending MGMT_OP_DISCONNECT
>>     commands, it doesn't matter if that connection was notified or not.
>>   * mgmt_device_disconnected() sends the mgmt event only if the link
>>     type is ACL_LINK or LE_LINK. For those connection type, the flag is
>>     always set.
>>
>> So this patch removes the HCI_CONN_MGMT_CONNECTED check.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_event.c | 16 +++++++---------
>> 1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 4673cfb..9225a9c 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -1795,16 +1795,14 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>> 	if (ev->status == 0)
>> 		conn->state = BT_CLOSED;
>>
>> -	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags)) {
>> -		if (ev->status) {
>> -			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
>> -					       conn->dst_type, ev->status);
>> -		} else {
>> -			u8 reason = hci_to_mgmt_reason(ev->reason);
>> +	if (ev->status) {
>> +		mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
>> +				conn->dst_type, ev->status);
>
> something went wrong with the coding style here.
>
>> +	} else {
>> +		u8 reason = hci_to_mgmt_reason(ev->reason);
>>
>> -			mgmt_device_disconnected(hdev, &conn->dst, conn->type,
>> -						 conn->dst_type, reason);
>> -		}
>> +		mgmt_device_disconnected(hdev, &conn->dst, conn->type,
>> +				conn->dst_type, reason);
>
> And here.

I'm fixing it.

Thanks,

Andre