[PATCH 0/2] bluetooth: hci_event: make coding style more consistent

[PATCH 0/2] bluetooth: hci_event: make coding style more consistent

[Sergey Shtylyov]

   Here are 2 patches against the 'bluetooth-next,git' repo. Mainly, I'm reducing
the indentation levels in the HCI driver, mostly by using *goto* in the error paths
in the functions where this hasn't been done -- this makes the coding style more
consistent across the HCI driver...

[1/2] bluetooth: hci_event: consolidate error paths in hci_phy_link_complete_evt()
[2/2] bluetooth: hci_event: reduce indentation levels

[PATCH 1/2] bluetooth: hci_event: consolidate error paths in hci_phy_link_complete_evt()

[Sergey Shtylyov]

hci_phy_link_complete_evt() has several duplicate error paths -- consolidate
them, using the *goto* statements.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>

---
 net/bluetooth/hci_event.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Index: bluetooth-next/net/bluetooth/hci_event.c
===================================================================
--- bluetooth-next.orig/net/bluetooth/hci_event.c
+++ bluetooth-next/net/bluetooth/hci_event.c
@@ -4936,20 +4936,15 @@ static void hci_phy_link_complete_evt(st
 	hci_dev_lock(hdev);
 
 	hcon = hci_conn_hash_lookup_handle(hdev, ev->phy_handle);
-	if (!hcon) {
-		hci_dev_unlock(hdev);
-		return;
-	}
+	if (!hcon)
+		goto unlock;
 
-	if (!hcon->amp_mgr) {
-		hci_dev_unlock(hdev);
-		return;
-	}
+	if (!hcon->amp_mgr)
+		goto unlock;
 
 	if (ev->status) {
 		hci_conn_del(hcon);
-		hci_dev_unlock(hdev);
-		return;
+		goto unlock;
 	}
 
 	bredr_hcon = hcon->amp_mgr->l2cap_conn->hcon;
@@ -4966,6 +4961,7 @@ static void hci_phy_link_complete_evt(st
 
 	amp_physical_cfm(bredr_hcon, hcon);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 

Re: [PATCH 1/2] bluetooth: hci_event: consolidate error paths in hci_phy_link_complete_evt()

[Marcel Holtmann]

Hi Sergey,

> hci_phy_link_complete_evt() has several duplicate error paths -- consolidate
> them, using the *goto* statements.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
> 
> ---
> net/bluetooth/hci_event.c |   16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 1/2] bluetooth: hci_event: consolidate error paths in hci_phy_link_complete_evt()

[Sergey Shtylyov]

Hello!

On 11.11.2020 14:12, Marcel Holtmann wrote:

>> hci_phy_link_complete_evt() has several duplicate error paths -- consolidate
>> them, using the *goto* statements.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>
>> ---
>> net/bluetooth/hci_event.c |   16 ++++++----------
>> 1 file changed, 6 insertions(+), 10 deletions(-)
> 
> patch has been applied to bluetooth-next tree.

    What about the 2nd patch?

> Regards
> 
> Marcel

MBR, Sergey

Re: [PATCH 1/2] bluetooth: hci_event: consolidate error paths in hci_phy_link_complete_evt()

[Marcel Holtmann]

Hi Sergey,

>>> hci_phy_link_complete_evt() has several duplicate error paths -- consolidate
>>> them, using the *goto* statements.
>>> 
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>> 
>>> ---
>>> net/bluetooth/hci_event.c |   16 ++++++----------
>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>> patch has been applied to bluetooth-next tree.
> 
>   What about the 2nd patch?

must have been slipping somehow. Can you please re-send against bluetooth-next.

Regards

Marcel

Re: [PATCH 1/2] bluetooth: hci_event: consolidate error paths in hci_phy_link_complete_evt()

[Sergey Shtylyov]

On 11/23/20 2:58 PM, Marcel Holtmann wrote:

>>>> hci_phy_link_complete_evt() has several duplicate error paths -- consolidate
>>>> them, using the *goto* statements.
>>>>
>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>>>
>>>> ---
>>>> net/bluetooth/hci_event.c |   16 ++++++----------
>>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>> patch has been applied to bluetooth-next tree.
>>
>>   What about the 2nd patch?
> 
> must have been slipping somehow. Can you please re-send against bluetooth-next.

   I have but to no avail -- the patch has been silently ignored...

> Regards
> 
> Marcel

MBR, Sergei

[PATCH 2/2] bluetooth: hci_event: reduce indentation levels

[Sergey Shtylyov]

Reduce the indentation levels in the HCI driver, mostly by using *goto* in
the error paths (and also by collapsing "double" *if* statements into the
"singular" ones).  This makes the coding style more consistent across the
HCI driver.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>

---
 net/bluetooth/hci_event.c |  316 ++++++++++++++++++++++++----------------------
 1 file changed, 170 insertions(+), 146 deletions(-)

Index: bluetooth-next/net/bluetooth/hci_event.c
===================================================================
--- bluetooth-next.orig/net/bluetooth/hci_event.c
+++ bluetooth-next/net/bluetooth/hci_event.c
@@ -1861,13 +1861,11 @@ static void hci_cs_create_conn(struct hc
 			} else
 				conn->state = BT_CONNECT2;
 		}
-	} else {
-		if (!conn) {
-			conn = hci_conn_add(hdev, ACL_LINK, &cp->bdaddr,
-					    HCI_ROLE_MASTER);
-			if (!conn)
-				bt_dev_err(hdev, "no memory for new connection");
-		}
+	} else if (!conn) {
+		conn = hci_conn_add(hdev, ACL_LINK, &cp->bdaddr,
+				    HCI_ROLE_MASTER);
+		if (!conn)
+			bt_dev_err(hdev, "no memory for new connection");
 	}
 
 	hci_dev_unlock(hdev);
@@ -1895,16 +1893,19 @@ static void hci_cs_add_sco(struct hci_de
 	hci_dev_lock(hdev);
 
 	acl = hci_conn_hash_lookup_handle(hdev, handle);
-	if (acl) {
-		sco = acl->link;
-		if (sco) {
-			sco->state = BT_CLOSED;
+	if (!acl)
+		goto unlock;
 
-			hci_connect_cfm(sco, status);
-			hci_conn_del(sco);
-		}
-	}
+	sco = acl->link;
+	if (!sco)
+		goto unlock;
 
+	sco->state = BT_CLOSED;
+
+	hci_connect_cfm(sco, status);
+	hci_conn_del(sco);
+
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -1925,13 +1926,13 @@ static void hci_cs_auth_requested(struct
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn) {
-		if (conn->state == BT_CONFIG) {
-			hci_connect_cfm(conn, status);
-			hci_conn_drop(conn);
-		}
-	}
+	if (!conn || conn->state != BT_CONFIG)
+		goto unlock;
+
+	hci_connect_cfm(conn, status);
+	hci_conn_drop(conn);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -1952,13 +1953,13 @@ static void hci_cs_set_conn_encrypt(stru
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn) {
-		if (conn->state == BT_CONFIG) {
-			hci_connect_cfm(conn, status);
-			hci_conn_drop(conn);
-		}
-	}
+	if (!conn || conn->state != BT_CONFIG)
+		goto unlock;
 
+	hci_connect_cfm(conn, status);
+	hci_conn_drop(conn);
+
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -2128,13 +2129,13 @@ static void hci_cs_read_remote_features(
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn) {
-		if (conn->state == BT_CONFIG) {
-			hci_connect_cfm(conn, status);
-			hci_conn_drop(conn);
-		}
-	}
+	if (!conn || conn->state != BT_CONFIG)
+		goto unlock;
 
+	hci_connect_cfm(conn, status);
+	hci_conn_drop(conn);
+
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -2155,13 +2156,13 @@ static void hci_cs_read_remote_ext_featu
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn) {
-		if (conn->state == BT_CONFIG) {
-			hci_connect_cfm(conn, status);
-			hci_conn_drop(conn);
-		}
-	}
+	if (!conn || conn->state != BT_CONFIG)
+		goto unlock;
+
+	hci_connect_cfm(conn, status);
+	hci_conn_drop(conn);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -2187,16 +2188,19 @@ static void hci_cs_setup_sync_conn(struc
 	hci_dev_lock(hdev);
 
 	acl = hci_conn_hash_lookup_handle(hdev, handle);
-	if (acl) {
-		sco = acl->link;
-		if (sco) {
-			sco->state = BT_CLOSED;
+	if (!acl)
+		goto unlock;
 
-			hci_connect_cfm(sco, status);
-			hci_conn_del(sco);
-		}
-	}
+	sco = acl->link;
+	if (!sco)
+		goto unlock;
+
+	sco->state = BT_CLOSED;
+
+	hci_connect_cfm(sco, status);
+	hci_conn_del(sco);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -2217,13 +2221,15 @@ static void hci_cs_sniff_mode(struct hci
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn) {
-		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags);
+	if (!conn)
+		goto unlock;
 
-		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->flags))
-			hci_sco_setup(conn, status);
-	}
+	clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags);
+
+	if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->flags))
+		hci_sco_setup(conn, status);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -2244,13 +2250,14 @@ static void hci_cs_exit_sniff_mode(struc
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn) {
-		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags);
+	if (!conn)
+		goto unlock;
 
-		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->flags))
-			hci_sco_setup(conn, status);
-	}
+	clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags);
 
+	if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->flags))
+		hci_sco_setup(conn, status);
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -2406,13 +2413,13 @@ static void hci_cs_le_read_remote_featur
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn) {
-		if (conn->state == BT_CONFIG) {
-			hci_connect_cfm(conn, status);
-			hci_conn_drop(conn);
-		}
-	}
+	if (!conn || conn->state != BT_CONFIG)
+		goto unlock;
+
+	hci_connect_cfm(conn, status);
+	hci_conn_drop(conn);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -3171,15 +3178,17 @@ static void hci_change_link_key_complete
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
-	if (conn) {
-		if (!ev->status)
-			set_bit(HCI_CONN_SECURE, &conn->flags);
+	if (!conn)
+		goto unlock;
 
-		clear_bit(HCI_CONN_AUTH_PEND, &conn->flags);
+	if (!ev->status)
+		set_bit(HCI_CONN_SECURE, &conn->flags);
 
-		hci_key_change_cfm(conn, ev->status);
-	}
+	clear_bit(HCI_CONN_AUTH_PEND, &conn->flags);
+
+	hci_key_change_cfm(conn, ev->status);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -3737,15 +3746,17 @@ static void hci_role_change_evt(struct h
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
-	if (conn) {
-		if (!ev->status)
-			conn->role = ev->role;
+	if (!conn)
+		goto unlock;
 
-		clear_bit(HCI_CONN_RSWITCH_PEND, &conn->flags);
+	if (!ev->status)
+		conn->role = ev->role;
 
-		hci_role_switch_cfm(conn, ev->status, ev->role);
-	}
+	clear_bit(HCI_CONN_RSWITCH_PEND, &conn->flags);
+
+	hci_role_switch_cfm(conn, ev->status, ev->role);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -3898,21 +3909,22 @@ static void hci_mode_change_evt(struct h
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
-	if (conn) {
-		conn->mode = ev->mode;
+	if (!conn)
+		goto unlock;
 
-		if (!test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND,
-					&conn->flags)) {
-			if (conn->mode == HCI_CM_ACTIVE)
-				set_bit(HCI_CONN_POWER_SAVE, &conn->flags);
-			else
-				clear_bit(HCI_CONN_POWER_SAVE, &conn->flags);
-		}
+	conn->mode = ev->mode;
 
-		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->flags))
-			hci_sco_setup(conn, ev->status);
+	if (!test_and_clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->flags)) {
+		if (conn->mode == HCI_CM_ACTIVE)
+			set_bit(HCI_CONN_POWER_SAVE, &conn->flags);
+		else
+			clear_bit(HCI_CONN_POWER_SAVE, &conn->flags);
 	}
 
+	if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->flags))
+		hci_sco_setup(conn, ev->status);
+
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -4011,27 +4023,29 @@ static void hci_link_key_request_evt(str
 	       &ev->bdaddr);
 
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
-	if (conn) {
-		clear_bit(HCI_CONN_NEW_LINK_KEY, &conn->flags);
+	if (!conn)
+		goto found;
 
-		if ((key->type == HCI_LK_UNAUTH_COMBINATION_P192 ||
-		     key->type == HCI_LK_UNAUTH_COMBINATION_P256) &&
-		    conn->auth_type != 0xff && (conn->auth_type & 0x01)) {
-			BT_DBG("%s ignoring unauthenticated key", hdev->name);
-			goto not_found;
-		}
+	clear_bit(HCI_CONN_NEW_LINK_KEY, &conn->flags);
 
-		if (key->type == HCI_LK_COMBINATION && key->pin_len < 16 &&
-		    (conn->pending_sec_level == BT_SECURITY_HIGH ||
-		     conn->pending_sec_level == BT_SECURITY_FIPS)) {
-			BT_DBG("%s ignoring key unauthenticated for high security",
-			       hdev->name);
-			goto not_found;
-		}
+	if ((key->type == HCI_LK_UNAUTH_COMBINATION_P192 ||
+	     key->type == HCI_LK_UNAUTH_COMBINATION_P256) &&
+	    conn->auth_type != 0xff && (conn->auth_type & 0x01)) {
+		BT_DBG("%s ignoring unauthenticated key", hdev->name);
+		goto not_found;
+	}
 
-		conn_set_key(conn, key->type, key->pin_len);
+	if (key->type == HCI_LK_COMBINATION && key->pin_len < 16 &&
+	    (conn->pending_sec_level == BT_SECURITY_HIGH ||
+	     conn->pending_sec_level == BT_SECURITY_FIPS)) {
+		BT_DBG("%s ignoring key unauthenticated for high security",
+		       hdev->name);
+		goto not_found;
 	}
 
+	conn_set_key(conn, key->type, key->pin_len);
+
+found:
 	bacpy(&cp.bdaddr, &ev->bdaddr);
 	memcpy(cp.link_key, key->val, HCI_LINK_KEY_SIZE);
 
@@ -4155,11 +4169,13 @@ static void hci_pscan_rep_mode_evt(struc
 	hci_dev_lock(hdev);
 
 	ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
-	if (ie) {
-		ie->data.pscan_rep_mode = ev->pscan_rep_mode;
-		ie->timestamp = jiffies;
-	}
+	if (!ie)
+		goto unlock;
+
+	ie->data.pscan_rep_mode = ev->pscan_rep_mode;
+	ie->timestamp = jiffies;
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -5041,11 +5057,13 @@ static void hci_disconn_phylink_complete
 	hci_dev_lock(hdev);
 
 	hcon = hci_conn_hash_lookup_handle(hdev, ev->phy_handle);
-	if (hcon) {
-		hcon->state = BT_CLOSED;
-		hci_conn_del(hcon);
-	}
+	if (!hcon)
+		goto unlock;
+
+	hcon->state = BT_CLOSED;
+	hci_conn_del(hcon);
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 #endif
@@ -5198,14 +5216,16 @@ static void le_conn_complete_evt(struct
 
 	params = hci_pend_le_action_lookup(&hdev->pend_le_conns, &conn->dst,
 					   conn->dst_type);
-	if (params) {
-		list_del_init(&params->action);
-		if (params->conn) {
-			hci_conn_drop(params->conn);
-			hci_conn_put(params->conn);
-			params->conn = NULL;
-		}
-	}
+	if (!params)
+		goto unlock;
+
+	list_del_init(&params->action);
+	if (!params->conn)
+		goto unlock;
+
+	hci_conn_drop(params->conn);
+	hci_conn_put(params->conn);
+	params->conn = NULL;
 
 unlock:
 	hci_update_background_scan(hdev);
@@ -5286,12 +5306,14 @@ static void hci_le_conn_update_complete_
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
-	if (conn) {
-		conn->le_conn_interval = le16_to_cpu(ev->interval);
-		conn->le_conn_latency = le16_to_cpu(ev->latency);
-		conn->le_supv_timeout = le16_to_cpu(ev->supervision_timeout);
-	}
+	if (!conn)
+		goto unlock;
 
+	conn->le_conn_interval = le16_to_cpu(ev->interval);
+	conn->le_conn_latency = le16_to_cpu(ev->latency);
+	conn->le_supv_timeout = le16_to_cpu(ev->supervision_timeout);
+
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -5700,34 +5722,36 @@ static void hci_le_remote_feat_complete_
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
-	if (conn) {
-		if (!ev->status)
-			memcpy(conn->features[0], ev->features, 8);
+	if (!conn)
+		goto unlock;
 
-		if (conn->state == BT_CONFIG) {
-			__u8 status;
+	if (!ev->status)
+		memcpy(conn->features[0], ev->features, 8);
 
-			/* If the local controller supports slave-initiated
-			 * features exchange, but the remote controller does
-			 * not, then it is possible that the error code 0x1a
-			 * for unsupported remote feature gets returned.
-			 *
-			 * In this specific case, allow the connection to
-			 * transition into connected state and mark it as
-			 * successful.
-			 */
-			if ((hdev->le_features[0] & HCI_LE_SLAVE_FEATURES) &&
-			    !conn->out && ev->status == 0x1a)
-				status = 0x00;
-			else
-				status = ev->status;
+	if (conn->state == BT_CONFIG) {
+		__u8 status;
 
-			conn->state = BT_CONNECTED;
-			hci_connect_cfm(conn, status);
-			hci_conn_drop(conn);
-		}
+		/* If the local controller supports slave-initiated
+		 * features exchange, but the remote controller does
+		 * not, then it is possible that the error code 0x1a
+		 * for unsupported remote feature gets returned.
+		 *
+		 * In this specific case, allow the connection to
+		 * transition into connected state and mark it as
+		 * successful.
+		 */
+		if ((hdev->le_features[0] & HCI_LE_SLAVE_FEATURES) &&
+		    !conn->out && ev->status == 0x1a)
+			status = 0x00;
+		else
+			status = ev->status;
+
+		conn->state = BT_CONNECTED;
+		hci_connect_cfm(conn, status);
+		hci_conn_drop(conn);
 	}
 
+unlock:
 	hci_dev_unlock(hdev);
 }