[RFC 0/3] LE Connection parameter tuning (first step)

[RFC 0/3] LE Connection parameter tuning (first step)

[Vinicius Costa Gomes]

Hi,

Right now, we are using only one set of (hardcoded) connection
parameters for LE.  I guess the first step to have different
connection parameters classes for different Profiles requirements
(think "Low Latency", "Low Power", etc.) is to have a nice set of
parameters. For that we need to measure and see which are the sweet
spots, This series aims to help the measuring part.

This is an idea so we can easily change the connection parameters
without need for recompiling the kernel side, so tests can be more
practical.

It works, but just lightly tested. This is more to get some feedback
on how to proceed with the LE connection parameters thing.

Cheers,


Vinicius Costa Gomes (3):
  Bluetooth: Keep the LE connection parameters in its own structure
  Bluetooth: Add LE connection parameters to debugfs
  Bluetooth: Add support for setting LE connection parameters via
    debugfs

 include/net/bluetooth/hci_core.h | 10 ++++++
 net/bluetooth/hci_conn.c         | 11 +++---
 net/bluetooth/hci_event.c        | 10 ++++++
 net/bluetooth/hci_sysfs.c        | 77 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 103 insertions(+), 5 deletions(-)

-- 
1.7.12.4

[RFC 1/3] Bluetooth: Keep the LE connection parameters in its own structure

[Vinicius Costa Gomes]

This will allow for easier parameterization of these fields. This is
the first step for allowing to change the parameters during runtime.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 include/net/bluetooth/hci_core.h | 10 ++++++++++
 net/bluetooth/hci_conn.c         | 11 ++++++-----
 net/bluetooth/hci_event.c        | 10 ++++++++++
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index ce6dbeb..f477d43 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -123,6 +123,14 @@ struct le_scan_params {
 	int timeout;
 };
 
+struct le_conn_params {
+	u16 scan_interval;
+	u16 scan_window;
+	u16 conn_interval_min;
+	u16 conn_interval_max;
+	u16 supervision_timeout;
+};
+
 #define HCI_MAX_SHORT_NAME_LENGTH	10
 
 struct amp_assoc {
@@ -278,6 +286,8 @@ struct hci_dev {
 	struct work_struct	le_scan;
 	struct le_scan_params	le_scan_params;
 
+	struct le_conn_params	le_conn_params;
+
 	__s8			adv_tx_power;
 
 	int (*open)(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 25bfce0..d8ff5c0 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -35,6 +35,7 @@ static void hci_le_create_connection(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_le_create_conn cp;
+	struct le_conn_params *params = &hdev->le_conn_params;
 
 	conn->state = BT_CONNECT;
 	conn->out = true;
@@ -42,13 +43,13 @@ static void hci_le_create_connection(struct hci_conn *conn)
 	conn->sec_level = BT_SECURITY_LOW;
 
 	memset(&cp, 0, sizeof(cp));
-	cp.scan_interval = __constant_cpu_to_le16(0x0060);
-	cp.scan_window = __constant_cpu_to_le16(0x0030);
+	cp.scan_interval = __cpu_to_le16(params->scan_interval);
+	cp.scan_window = __cpu_to_le16(params->scan_window);
 	bacpy(&cp.peer_addr, &conn->dst);
 	cp.peer_addr_type = conn->dst_type;
-	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
-	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
-	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
+	cp.conn_interval_min = __cpu_to_le16(params->conn_interval_min);
+	cp.conn_interval_max = __cpu_to_le16(params->conn_interval_max);
+	cp.supervision_timeout = __cpu_to_le16(params->supervision_timeout);
 	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
 	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index c08ac7c..3675dbd 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1298,6 +1298,16 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
 			hdev->host_features[0] |= LMP_HOST_LE_BREDR;
 		else
 			hdev->host_features[0] &= ~LMP_HOST_LE_BREDR;
+
+		if (sent->le || sent->simul) {
+			struct le_conn_params *params = &hdev->le_conn_params;
+
+			params->scan_interval = 0x0060;
+			params->scan_window = 0x0030;
+			params->conn_interval_min = 0x0028;
+			params->conn_interval_max = 0x0038;
+			params->supervision_timeout = 0x002a;
+		}
 	}
 
 	if (test_bit(HCI_MGMT, &hdev->dev_flags) &&
-- 
1.7.12.4

[RFC 2/3] Bluetooth: Add LE connection parameters to debugfs

[Vinicius Costa Gomes]

Only reading the parameters is supported for now.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/hci_sysfs.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 55cceee..a9554ec 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -532,6 +532,36 @@ static int auto_accept_delay_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(auto_accept_delay_fops, auto_accept_delay_get,
 			auto_accept_delay_set, "%llu\n");
 
+
+static int le_conn_parameters_show(struct seq_file *f, void *p)
+{
+	struct hci_dev *hdev = f->private;
+	struct le_conn_params *params = &hdev->le_conn_params;
+
+	hci_dev_lock(hdev);
+
+	seq_printf(f, "0x%.4x 0x%.4x 0x%.4x 0x%.4x 0x%.4x\n",
+		   params->scan_interval, params->scan_window,
+		   params->conn_interval_min, params->conn_interval_max,
+		   params->supervision_timeout);
+
+	hci_dev_unlock(hdev);
+
+	return 0;
+}
+
+static int le_conn_parameters_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, le_conn_parameters_show, inode->i_private);
+}
+
+static const struct file_operations le_conn_parameters_fops = {
+	.open		= le_conn_parameters_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 void hci_init_sysfs(struct hci_dev *hdev)
 {
 	struct device *dev = &hdev->dev;
@@ -573,6 +603,10 @@ int hci_add_sysfs(struct hci_dev *hdev)
 
 	debugfs_create_file("auto_accept_delay", 0444, hdev->debugfs, hdev,
 			    &auto_accept_delay_fops);
+
+	debugfs_create_file("le_conn_parameters", 0644, hdev->debugfs, hdev,
+			    &le_conn_parameters_fops);
+
 	return 0;
 }
 
-- 
1.7.12.4

[RFC 3/3] Bluetooth: Add support for setting LE connection parameters via debugfs

[Vinicius Costa Gomes]

This will allow for much more pratical measures of the impact of
different connection parameters in different scenarios.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/hci_sysfs.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index a9554ec..8474d02 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -550,6 +550,48 @@ static int le_conn_parameters_show(struct seq_file *f, void *p)
 	return 0;
 }
 
+static ssize_t le_conn_parameters_write(struct file *filp,
+					const char __user *ubuf,
+					size_t cnt, loff_t *ppos)
+{
+	struct seq_file *seqf = filp->private_data;
+	struct hci_dev *hdev = seqf->private;
+	struct le_conn_params params;
+	char buffer[36];
+	int n;
+
+	/* No partial writes */
+	if (*ppos != 0)
+		return 0;
+
+	/* Format: '0x0000 0x0000 0x0000 0x0000 0x0000' length 35 */
+	if (cnt != 35)
+		return -ENOSPC;
+
+	memset(buffer, 0, sizeof(buffer));
+
+	n = copy_from_user(buffer, ubuf, cnt);
+	if (n < 0)
+		return n;
+
+	memset(&params, 0, sizeof(params));
+
+	n = sscanf(buffer, "%hx %hx %hx %hx %hx", &params.scan_interval,
+		   &params.scan_window, &params.conn_interval_min,
+		   &params.conn_interval_max, &params.supervision_timeout);
+
+	if (n < 5)
+		return -EINVAL;
+
+	hci_dev_lock(hdev);
+
+	memcpy(&hdev->le_conn_params, &params, sizeof(params));
+
+	hci_dev_unlock(hdev);
+
+	return cnt;
+}
+
 static int le_conn_parameters_open(struct inode *inode, struct file *file)
 {
 	return single_open(file, le_conn_parameters_show, inode->i_private);
@@ -557,6 +599,7 @@ static int le_conn_parameters_open(struct inode *inode, struct file *file)
 
 static const struct file_operations le_conn_parameters_fops = {
 	.open		= le_conn_parameters_open,
+	.write		= le_conn_parameters_write,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
 	.release	= single_release,
-- 
1.7.12.4

Re: [RFC 0/3] LE Connection parameter tuning (first step)

[Kim Schulz]

Den 2012-11-05 14:25, Vinicius Costa Gomes skrev:
> Hi,
>
> Right now, we are using only one set of (hardcoded) connection
> parameters for LE.  I guess the first step to have different
> connection parameters classes for different Profiles requirements
> (think "Low Latency", "Low Power", etc.) is to have a nice set of
> parameters. For that we need to measure and see which are the sweet
> spots, This series aims to help the measuring part.
>
> This is an idea so we can easily change the connection parameters
> without need for recompiling the kernel side, so tests can be more
> practical.
>
[snip]


Good idea to split them out as every single profile defines their own. 
Have you
concidered what should happen if two LE connections to two different 
profils should
happen at the same time and the connection parameters are different? 
i.e. should bluez
maybe have a per-profile set of connection parameters?
Also make room in the struct for Preferred Connection Paramters (the 
values read
from the peer DB after connection) as these can differ from the ones 
the profile proposes.

-- 
Kim Schulz

Re: [RFC 0/3] LE Connection parameter tuning (first step)

[Vinicius Costa Gomes]

Hi,

On 14:42 Mon 05 Nov, Kim Schulz wrote:
> Den 2012-11-05 14:25, Vinicius Costa Gomes skrev:
> >Hi,
> >
> >Right now, we are using only one set of (hardcoded) connection
> >parameters for LE.  I guess the first step to have different
> >connection parameters classes for different Profiles requirements
> >(think "Low Latency", "Low Power", etc.) is to have a nice set of
> >parameters. For that we need to measure and see which are the sweet
> >spots, This series aims to help the measuring part.
> >
> >This is an idea so we can easily change the connection parameters
> >without need for recompiling the kernel side, so tests can be more
> >practical.
> >
> [snip]
> 
> 
> Good idea to split them out as every single profile defines their
> own. Have you
> concidered what should happen if two LE connections to two different
> profils should
> happen at the same time and the connection parameters are different?
> i.e. should bluez
> maybe have a per-profile set of connection parameters?

The current plan is to have the most restrictive (in terms of latency)
set of parameters to prevail over the others.

> Also make room in the struct for Preferred Connection Paramters (the
> values read
> from the peer DB after connection) as these can differ from the ones
> the profile proposes.

I don't think that this information should be in the kernel side, but thank
you for reminding me of this. About the problem, that could be dealt with
having that as another set of parameters (perhaps with a higher priority). 

> 
> -- 
> Kim Schulz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers,
-- 
Vinicius

Re: [RFC 2/3] Bluetooth: Add LE connection parameters to debugfs

[Andrei Emeltchenko]

Hi Vinicius,

On Mon, Nov 05, 2012 at 02:25:42PM +0100, Vinicius Costa Gomes wrote:
> Only reading the parameters is supported for now.
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> ---
>  net/bluetooth/hci_sysfs.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 55cceee..a9554ec 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -532,6 +532,36 @@ static int auto_accept_delay_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(auto_accept_delay_fops, auto_accept_delay_get,
>  			auto_accept_delay_set, "%llu\n");
>  
> +
> +static int le_conn_parameters_show(struct seq_file *f, void *p)
> +{
> +	struct hci_dev *hdev = f->private;
> +	struct le_conn_params *params = &hdev->le_conn_params;
> +
> +	hci_dev_lock(hdev);
> +
> +	seq_printf(f, "0x%.4x 0x%.4x 0x%.4x 0x%.4x 0x%.4x\n",

BTW: you might want to use format "0x%4.4x" we use in hci_core.c

Best regards 
Andrei Emeltchenko