Return-Path: <cyrus@holtmann.org>
Content-Type: text/plain; charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\))
Subject: Re: [PATCH v5] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <1413037644-15858-1-git-send-email-fons@spotify.com>
Date: Sat, 11 Oct 2014 18:13:14 +0200
Cc: linux-bluetooth@vger.kernel.org
Message-Id: <2F926C4D-DBA0-4B6C-A368-FD8E95509A5A@holtmann.org>
References: <1413037644-15858-1-git-send-email-fons@spotify.com>
To: Alfonso Acosta <fons@spotify.com>
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Alfonso,

> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
> 
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
> 
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> 	HCI_CONN_STK_ENCRYPT,
> 	HCI_CONN_AUTH_INITIATOR,
> 	HCI_CONN_DROP,
> +	HCI_CONN_PARAM_REMOVAL_PEND,
> };
> 
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..11aac06 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
> 
> 	hci_conn_del_sysfs(conn);
> 
> +	if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> +		hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> 	hci_dev_put(hdev);
> 
> 	hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..063b0a7 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2725,10 +2725,40 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> 	}
> 
> 	if (cp->addr.type == BDADDR_BREDR) {
> +		/* If disconnection is requested, then look up the
> +		 * connection. If the remote device is connected, it
> +		 * will be later used to terminate the link.
> +		 *
> +		 * Setting it to NULL explicitly will cause no
> +		 * termination of the link.
> +		 */
> +		if (cp->disconnect)
> +			conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> +						       &cp->addr.bdaddr);
> +		else
> +			conn = NULL;
> +
> 		err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
> 	} else {
> 		u8 addr_type;
> 
> +		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> +					       &cp->addr.bdaddr);
> +		if (conn) {
> +			/* Defer clearing up the connection parameters
> +			 * until closing to give a chance of keeping
> +			 * them if a repairing happens.
> +			 */
> +			set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> +
> +			/* If disconnection is not requested, then
> +			 * clear the connection variable so that the
> +			 * link is not terminated.
> +			 */
> +			if (!cp->disconnect)
> +				conn = NULL;
> +		}
> +
> 		if (cp->addr.type == BDADDR_LE_PUBLIC)
> 			addr_type = ADDR_LE_DEV_PUBLIC;
> 		else
> @@ -2736,8 +2766,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> 
> 		hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
> 
> -		hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> -
> 		err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> 	}
> 
> @@ -2747,17 +2775,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> 		goto unlock;
> 	}
> 
> -	if (cp->disconnect) {
> -		if (cp->addr.type == BDADDR_BREDR)
> -			conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> -						       &cp->addr.bdaddr);
> -		else
> -			conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> -						       &cp->addr.bdaddr);
> -	} else {
> -		conn = NULL;
> -	}
> -

I would have put an extra comment here as well to remind us that conn is for indicating to terminate the connection or not. See my review of v4 of your patch.

> 	if (!conn) {
> 		err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
> 				   &rp, sizeof(rp));

Regards

Marcel