[PATCH 0/2] Disconnect complete refactoring

[PATCH 0/2] Disconnect complete refactoring

[Andre Guedes]

Hi all,

This small patch set contains the remaining patches from "[RFC] Disconnect
complete refactoring" patch set which weren't applied due to coding style
issues.

Andre Guedes (2):
  Bluetooth: Remove unneeded check in hci_disconn_complete_evt()
  Bluetooth: Refactor hci_disconn_complete_evt

 net/bluetooth/hci_event.c | 59 ++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

-- 
1.8.4

[PATCH 1/2] 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 142aa61..560296d 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1792,16 +1792,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

Re: [PATCH 1/2] Bluetooth: Remove unneeded check in hci_disconn_complete_evt()

[Johan Hedberg]

Hi Andre,

On Mon, Nov 04, 2013, Andre Guedes wrote:
> 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 142aa61..560296d 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1792,16 +1792,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) {

It looks to me like this would cause an invalid mgmt_device_disconnected
event to be sent of the ACL goes down before we notify over mgmt that
it's connected. This could e.g. happen if the ACL disconnection happens
before we complete the implicit name resolving (which is the main reason
why this flag exists to begin with). Am I missing something here?

Johan

Re: [PATCH 1/2] Bluetooth: Remove unneeded check in hci_disconn_complete_evt()

[Andre Guedes]

Hi Johan,

On 05/11/13 05:09, Johan Hedberg wrote:
> Hi Andre,
>
> On Mon, Nov 04, 2013, Andre Guedes wrote:
>> 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 142aa61..560296d 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -1792,16 +1792,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) {
>
> It looks to me like this would cause an invalid mgmt_device_disconnected
> event to be sent of the ACL goes down before we notify over mgmt that
> it's connected. This could e.g. happen if the ACL disconnection happens
> before we complete the implicit name resolving (which is the main reason
> why this flag exists to begin with). Am I missing something here?

I guess you are right. I'll fix this patch by checking the flag before 
calling mgmt_device_disconnected().

Thanks for your review.

Andre

[PATCH 2/2] 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 | 53 +++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 560296d..8054e3c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1779,7 +1779,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);
 
@@ -1789,40 +1791,37 @@ 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);
+		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