[RFC 0/7] RSSI Monitor

[RFC 0/7] RSSI Monitor

[Anderson Briglia]

This RFC is about RSSI monitoring since controllers do not have a specific
command for this and we need it for Proximity Profile, at least.

The RFC implements a mechanism to set threshold levels in order to send
alerts to userspace when a RSSI value reaches one of the configured thresholds.

Management Interface contains a list of RSSI Monitors. Elements are inserted
or deleted from this list using two new Management commands: Enable and Disable
RSSI Monitor.

A timer was added to do HCI Read RSSI requests to the controller and get RSSI
values updated. If there is monitored connection (eg.: RSSI Monitor list size >
0), the timer is reseted after two seconds and the RSSI for each monitored conn
is updated. Using the RSSI value, a Management Event is sent if one of the
thresholds were reached and the alert is different from the last one sent.

Userspace is responsible to add and remove RSSI Monitors. Each monitor must
have low and high thresholds values for RSSI. A RSSI Monitor is disabled when
the connection is no longer available or the userspace sends a command to
remove it. If there is no RSSI Monitor remaining in the list, the RSSI Read
polling timer is removed.

This RFC is dependent of Andre Guedes discovery procedure patches, available
here: git://github.com/aguedes/linux-2.6.git branch discovery

This RFC is also the kernel side of this one:
http://marc.info/?l=linux-bluetooth&m=131111375130918&w=2

Anderson Briglia (7):
  Bluetooth: Add RSSI Monitor commands
  Bluetooth: Implement Enable RSSI Monitor
  Bluetooth: Implement Disable RSSI Monitor
  Bluetooth: Implement RSSI Monitor Alert event
  Bluetooth: Implement Read RSSI command
  Bluetooth: Add RSSI Monitor timer and list lock
  Bluetooth: Remove RSSI monitor on disconnection

 include/net/bluetooth/hci.h      |   10 ++
 include/net/bluetooth/hci_core.h |    4 +
 include/net/bluetooth/mgmt.h     |   18 +++
 net/bluetooth/hci_core.c         |   12 ++
 net/bluetooth/hci_event.c        |   24 ++++
 net/bluetooth/mgmt.c             |  251 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 319 insertions(+), 0 deletions(-)

-- 
1.7.4.1

[RFC 1/7] Bluetooth: Add RSSI Monitor commands

[Anderson Briglia]

This patch adds two new Management commands: Enable/Disable RSSI
Monitor. This command is used to handle a list of monitors in order to
monitor the RSSI value of a connection. It is useful on Proximity
profile implementation.

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
---
 include/net/bluetooth/mgmt.h |   12 ++++++++++++
 net/bluetooth/mgmt.c         |   22 ++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 5428fd3..ef3b636 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -211,6 +211,18 @@ struct mgmt_cp_unblock_device {
 	bdaddr_t bdaddr;
 } __packed;
 
+#define MGMT_OP_ENABLE_RSSI_MONITOR	0x001F
+struct mgmt_cp_enable_rssi_monitor {
+	bdaddr_t bdaddr;
+	__s8 low_alert_trigger;
+	__s8 high_alert_trigger;
+} __packed;
+
+#define MGMT_OP_DISABLE_RSSI_MONITOR	0x0020
+struct mgmt_cp_disable_rssi_monitor {
+	bdaddr_t bdaddr;
+} __packed;
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16 opcode;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7c0f4f6..36ed168 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1884,6 +1884,22 @@ static int unblock_device(struct sock *sk, u16 index, unsigned char *data,
 	return err;
 }
 
+static int enable_rssi_monitor(struct sock *sk, u16 index,
+					unsigned char *data, u16 len)
+{
+	BT_DBG("hci%u", index);
+
+	return -ENOSYS;
+}
+
+static int disable_rssi_monitor(struct sock *sk, u16 index,
+					unsigned char *data, u16 len)
+{
+	BT_DBG("hci%u", index);
+
+	return -ENOSYS;
+}
+
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 {
 	unsigned char *buf;
@@ -2004,6 +2020,12 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 	case MGMT_OP_UNBLOCK_DEVICE:
 		err = unblock_device(sk, index, buf + sizeof(*hdr), len);
 		break;
+	case MGMT_OP_ENABLE_RSSI_MONITOR:
+		err = enable_rssi_monitor(sk, index, buf + sizeof(*hdr), len);
+		break;
+	case MGMT_OP_DISABLE_RSSI_MONITOR:
+		err = disable_rssi_monitor(sk, index, buf + sizeof(*hdr), len);
+		break;
 	default:
 		BT_DBG("Unknown op %u", opcode);
 		err = cmd_status(sk, index, opcode, 0x01);
-- 
1.7.4.1

Re: [RFC 1/7] Bluetooth: Add RSSI Monitor commands

[Gustavo Padovan]

Hi Briglia,

* Anderson Briglia <anderson.briglia@openbossa.org> [2011-08-02 14:35:03 -0400]:

> This patch adds two new Management commands: Enable/Disable RSSI
> Monitor. This command is used to handle a list of monitors in order to
> monitor the RSSI value of a connection. It is useful on Proximity
> profile implementation.
> 
> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
> ---
>  include/net/bluetooth/mgmt.h |   12 ++++++++++++
>  net/bluetooth/mgmt.c         |   22 ++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 5428fd3..ef3b636 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -211,6 +211,18 @@ struct mgmt_cp_unblock_device {
>  	bdaddr_t bdaddr;
>  } __packed;
>  
> +#define MGMT_OP_ENABLE_RSSI_MONITOR	0x001F
> +struct mgmt_cp_enable_rssi_monitor {
> +	bdaddr_t bdaddr;
> +	__s8 low_alert_trigger;
> +	__s8 high_alert_trigger;
> +} __packed;
> +
> +#define MGMT_OP_DISABLE_RSSI_MONITOR	0x0020
> +struct mgmt_cp_disable_rssi_monitor {
> +	bdaddr_t bdaddr;
> +} __packed;
> +

Make much more sense to me call this ADD/REMOVE. We add an rssi monitor to the
list and then remove it from the list. 

	Gustavo

Re: [RFC 1/7] Bluetooth: Add RSSI Monitor commands

[Anderson Briglia]

Hi Padovan,

On Wed, Aug 3, 2011 at 1:58 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> Hi Briglia,
>
> * Anderson Briglia <anderson.briglia@openbossa.org> [2011-08-02 14:35:03 -0400]:
>
>> This patch adds two new Management commands: Enable/Disable RSSI
>> Monitor. This command is used to handle a list of monitors in order to
>> monitor the RSSI value of a connection. It is useful on Proximity
>> profile implementation.
>>
>> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
>> ---
>>  include/net/bluetooth/mgmt.h |   12 ++++++++++++
>>  net/bluetooth/mgmt.c         |   22 ++++++++++++++++++++++
>>  2 files changed, 34 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>> index 5428fd3..ef3b636 100644
>> --- a/include/net/bluetooth/mgmt.h
>> +++ b/include/net/bluetooth/mgmt.h
>> @@ -211,6 +211,18 @@ struct mgmt_cp_unblock_device {
>>       bdaddr_t bdaddr;
>>  } __packed;
>>
>> +#define MGMT_OP_ENABLE_RSSI_MONITOR  0x001F
>> +struct mgmt_cp_enable_rssi_monitor {
>> +     bdaddr_t bdaddr;
>> +     __s8 low_alert_trigger;
>> +     __s8 high_alert_trigger;
>> +} __packed;
>> +
>> +#define MGMT_OP_DISABLE_RSSI_MONITOR 0x0020
>> +struct mgmt_cp_disable_rssi_monitor {
>> +     bdaddr_t bdaddr;
>> +} __packed;
>> +
>
> Make much more sense to me call this ADD/REMOVE. We add an rssi monitor to the
> list and then remove it from the list.
>

Actually it depends. From the userspace point of view, there is no
list to add or remove elements. The user/application just enable or
disable the RSSI monitoring for a connection. But, I can't see
problems in changing the commands names if you prefer.

>        Gustavo
>



-- 
INdT - Instituto Nokia de tecnologia
+55 92 2126 1122
+55 92 8423 3183

[RFC 2/7] Bluetooth: Implement Enable RSSI Monitor

[Anderson Briglia]

This patch implements Enable RSSI Monitor Management command. This
command is responsible to add monitors into a list. This list stores all
RSSI monitors, one for each monitored connection.

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
---
 net/bluetooth/mgmt.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 36ed168..ccfc19b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -55,6 +55,17 @@ struct pending_cmd {
 
 static LIST_HEAD(cmd_list);
 
+struct rssi_monitor {
+	struct list_head list;
+	int index;
+	bdaddr_t bdaddr;
+	s8 low_trigger;
+	s8 high_trigger;
+	u8 last_alert;
+};
+
+static LIST_HEAD(rssi_monitor_list);
+
 static int cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status)
 {
 	struct sk_buff *skb;
@@ -1884,12 +1895,64 @@ static int unblock_device(struct sock *sk, u16 index, unsigned char *data,
 	return err;
 }
 
+static struct rssi_monitor *rssi_monitor_find(u16 index, bdaddr_t *bdaddr)
+{
+	struct rssi_monitor *rm;
+
+	list_for_each_entry(rm, &rssi_monitor_list, list) {
+		if (rm->index != index)
+			continue;
+
+		if (bacmp(&rm->bdaddr, bdaddr) != 0)
+			continue;
+
+		return rm;
+	}
+
+	return NULL;
+}
+
+static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
+							s8 high_trigger)
+{
+	struct rssi_monitor *rm;
+
+	if (rssi_monitor_find(index, bdaddr))
+		return -EEXIST;
+
+	if (low_trigger < -127 || low_trigger > 128 ||
+			high_trigger < -127 || high_trigger > 128)
+		return -EINVAL;
+
+	rm = kzalloc(sizeof(*rm), GFP_ATOMIC);
+	if (!rm)
+		return -ENOMEM;
+
+	rm->index = index;
+	bacpy(&rm->bdaddr, bdaddr);
+	rm->low_trigger = low_trigger;
+	rm->high_trigger = high_trigger;
+
+	list_add(&rm->list, &rssi_monitor_list);
+
+	return 0;
+}
+
 static int enable_rssi_monitor(struct sock *sk, u16 index,
 					unsigned char *data, u16 len)
 {
+	struct mgmt_cp_enable_rssi_monitor *cp;
+
 	BT_DBG("hci%u", index);
 
-	return -ENOSYS;
+	cp = (void *) data;
+
+	if (len != sizeof(*cp))
+		return cmd_status(sk, index,
+					MGMT_OP_ENABLE_RSSI_MONITOR, EINVAL);
+
+	return rssi_monitor_add(index, &cp->bdaddr,
+			cp->low_alert_trigger, cp->high_alert_trigger);
 }
 
 static int disable_rssi_monitor(struct sock *sk, u16 index,
-- 
1.7.4.1

[RFC 3/7] Bluetooth: Implement Disable RSSI Monitor

[Anderson Briglia]

This patch implements Disable RSSI Monitor command in order to remove
elements from RSSI monitor list.

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
---
 net/bluetooth/mgmt.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ccfc19b..e655542 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1938,6 +1938,19 @@ static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
 	return 0;
 }
 
+static int rssi_monitor_remove(u16 index, bdaddr_t *bdaddr)
+{
+	struct rssi_monitor *rm;
+
+	rm = rssi_monitor_find(index, bdaddr);
+	if (!rm)
+		return -EINVAL;
+
+	list_del(&rm->list);
+
+	return 0;
+}
+
 static int enable_rssi_monitor(struct sock *sk, u16 index,
 					unsigned char *data, u16 len)
 {
@@ -1958,9 +1971,17 @@ static int enable_rssi_monitor(struct sock *sk, u16 index,
 static int disable_rssi_monitor(struct sock *sk, u16 index,
 					unsigned char *data, u16 len)
 {
+	struct mgmt_cp_disable_rssi_monitor *cp;
+
 	BT_DBG("hci%u", index);
 
-	return -ENOSYS;
+	cp = (void *) data;
+
+	if (len != sizeof(*cp))
+		return cmd_status(sk, index,
+				MGMT_OP_DISABLE_RSSI_MONITOR, EINVAL);
+
+	return rssi_monitor_remove(index, &cp->bdaddr);
 }
 
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
-- 
1.7.4.1

[RFC 4/7] Bluetooth: Implement RSSI Monitor Alert event

[Anderson Briglia]

This patch implements functions to calculate and send a Management event
according to RSSI value from a monitored connection. The event was
divided in three types: high, in range and low. The RSSI Monitor Alert
event is sent when a monitored RSSI value (stored into RSSI Monitor
list), reaches one of the two thresholds (low and high).

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
---
 include/net/bluetooth/mgmt.h |    6 ++++++
 net/bluetooth/mgmt.c         |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index ef3b636..f254f40 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -313,3 +313,9 @@ struct mgmt_ev_remote_name {
 } __packed;
 
 #define MGMT_EV_DISCOVERING		0x0014
+
+#define MGMT_EV_RSSI_MONITOR_ALERT	0x0015
+struct mgmt_ev_rssi_monitor_alert {
+	bdaddr_t bdaddr;
+	__u8 alert_type;
+} __packed;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e655542..a1955df 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -44,6 +44,10 @@ enum bt_device_type {
 #define BREDR_LE_SCAN_TIMEOUT 5120 /* TGAP(100)/2 */
 #define LE_ONLY_SCAN_TIMEOUT 10240 /* TGAP(gen_disc_scan_min) */
 
+#define RSSI_MONITOR_ALERT_IN_RANGE	0x00
+#define RSSI_MONITOR_ALERT_LOW		0x01
+#define RSSI_MONITOR_ALERT_HIGH		0x02
+
 struct pending_cmd {
 	struct list_head list;
 	__u16 opcode;
@@ -1932,6 +1936,7 @@ static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
 	bacpy(&rm->bdaddr, bdaddr);
 	rm->low_trigger = low_trigger;
 	rm->high_trigger = high_trigger;
+	rm->last_alert = RSSI_MONITOR_ALERT_HIGH;
 
 	list_add(&rm->list, &rssi_monitor_list);
 
@@ -1984,6 +1989,33 @@ static int disable_rssi_monitor(struct sock *sk, u16 index,
 	return rssi_monitor_remove(index, &cp->bdaddr);
 }
 
+static int rssi_monitor_send_alert(u16 index, bdaddr_t *bdaddr, s8 rssi)
+{
+	struct rssi_monitor *rm;
+	struct mgmt_ev_rssi_monitor_alert ev;
+	u8 alert;
+
+	rm = rssi_monitor_find(index, bdaddr);
+	if (!rm)
+		return -EINVAL;
+
+	if (rssi <= rm->high_trigger)
+		alert = RSSI_MONITOR_ALERT_HIGH;
+	else if (rssi <= rm->low_trigger)
+		alert = RSSI_MONITOR_ALERT_LOW;
+	else
+		alert = RSSI_MONITOR_ALERT_IN_RANGE;
+
+	if (rm->last_alert == alert)
+		return 0;
+
+	bacpy(&ev.bdaddr, bdaddr);
+	ev.alert_type = alert;
+	rm->last_alert = alert;
+	return mgmt_event(MGMT_EV_RSSI_MONITOR_ALERT, index, &ev,
+							sizeof(ev), NULL);
+}
+
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 {
 	unsigned char *buf;
-- 
1.7.4.1

[RFC 5/7] Bluetooth: Implement Read RSSI command

[Anderson Briglia]

This command is used for RSSI Monitoring since the controller did not
have this feature.
When Read RSSI Command completes, the RSSI alert level is calculated for
monitored connections and if the alert type has changed, an alert event
is sent to user space according to configured threshold values.

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
---
 include/net/bluetooth/hci.h      |   10 ++++++++++
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_event.c        |   24 ++++++++++++++++++++++++
 net/bluetooth/mgmt.c             |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c4fdeeb..276be6e 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -550,6 +550,16 @@ struct hci_cp_set_event_flt {
 	__u8     condition[0];
 } __packed;
 
+#define HCI_OP_READ_RSSI		0x1405
+struct hci_cp_read_rssi {
+	__le16	handle;
+} __packed;
+struct hci_rp_read_rssi {
+	__u8	status;
+	__le16	handle;
+	__s8	rssi;
+} __packed;
+
 /* Filter types */
 #define HCI_FLT_CLEAR_ALL	0x00
 #define HCI_FLT_INQ_RESULT	0x01
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1bc1c3a..a3cf433 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -856,6 +856,7 @@ int mgmt_auth_failed(u16 index, bdaddr_t *bdaddr, u8 status);
 int mgmt_set_local_name_complete(u16 index, u8 *name, u8 status);
 int mgmt_read_local_oob_data_reply_complete(u16 index, u8 *hash, u8 *randomizer,
 								u8 status);
+void mgmt_read_rssi_complete(u16 index, bdaddr_t *bdaddr, s8 rssi, u8 status);
 int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 *dev_class, s8 rssi,
 							u8 *eir, u8 eir_len);
 int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 55872ff..4379de0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -982,6 +982,26 @@ static inline void hci_cc_write_le_host_supported(struct hci_dev *hdev,
 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(cp), &cp);
 }
 
+static void hci_cc_read_rssi(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_rp_read_rssi *rp = (void *) skb->data;
+	struct hci_conn *conn;
+
+	BT_DBG("%s status 0x%x", hdev->name, rp->status);
+
+	if (!test_bit(HCI_MGMT, &hdev->flags))
+		return;
+
+	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp->handle));
+	if (!conn) {
+		BT_DBG("%s read rssi failed", hdev->name);
+		return;
+	}
+
+	mgmt_read_rssi_complete(hdev->id, &conn->dst, rp->rssi,
+							rp->status);
+}
+
 static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 {
 	BT_DBG("%s status 0x%x", hdev->name, status);
@@ -2012,6 +2032,10 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
 		hci_cc_write_le_host_supported(hdev, skb);
 		break;
 
+	case HCI_OP_READ_RSSI:
+		hci_cc_read_rssi(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%x", hdev->name, opcode);
 		break;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a1955df..65ddbff 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2686,3 +2686,35 @@ int mgmt_do_interleaved_discovery(u16 index)
 
 	return err;
 }
+
+static int mgmt_read_rssi(struct hci_dev *hdev, bdaddr_t *bdaddr)
+{
+	struct hci_cp_read_rssi rs;
+	struct hci_conn *conn;
+
+	BT_DBG("hci%u", hdev->id);
+
+	if (!test_bit(HCI_UP, &hdev->flags))
+		return -ENETDOWN;
+
+	conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, bdaddr);
+	if (!conn)
+		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
+
+	if (!conn)
+		return -ENOTCONN;
+
+	put_unaligned_le16(conn->handle, &rs.handle);
+
+	return hci_send_cmd(hdev, HCI_OP_READ_RSSI, sizeof(rs), &rs);
+}
+
+void mgmt_read_rssi_complete(u16 index, bdaddr_t *bdaddr, s8 rssi, u8 status)
+{
+	BT_DBG("hci%d status %0x", index, status);
+
+	if (status)
+		return -EINVAL;
+
+	rssi_monitor_send_alert(index, bdaddr, rssi);
+}
-- 
1.7.4.1

[RFC 6/7] Bluetooth: Add RSSI Monitor timer and list lock

[Anderson Briglia]

This is the polling mechanism to send HCI Read RSSI commands. The
command responses are used by the RSSI Monitor to get the current RSSI
value and calculate the current alert level for a given monitored
connection.

This patch also adds read and write locks on rssi_monitor_list in
order to avoid race conditions.

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    3 +
 net/bluetooth/hci_core.c         |   12 +++++
 net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++----
 3 files changed, 104 insertions(+), 10 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a3cf433..8ff3448 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -211,6 +211,8 @@ struct hci_dev {
 
 	struct timer_list	le_scan_timer;
 
+	struct timer_list	rssi_monitor_timer;
+
 	struct hci_dev_stats	stat;
 
 	struct sk_buff_head	driver_init;
@@ -869,6 +871,7 @@ int mgmt_has_pending_stop_discov(u16 index);
 int mgmt_cancel_discovery(u16 index);
 int mgmt_is_interleaved_discovery(u16 index);
 int mgmt_do_interleaved_discovery(u16 index);
+void mgmt_do_read_rssi(struct hci_dev *hdev);
 
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index c0c46bf..eb51b94 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1449,6 +1449,14 @@ static void hci_disable_le_scan(unsigned long arg)
 	hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
 }
 
+static void hci_do_read_rssi(unsigned long arg)
+{
+	struct hci_dev *hdev = (void *) arg;
+
+	if (test_bit(HCI_UP, &hdev->flags))
+		mgmt_do_read_rssi(hdev);
+}
+
 /* Register HCI device */
 int hci_register_dev(struct hci_dev *hdev)
 {
@@ -1522,6 +1530,9 @@ int hci_register_dev(struct hci_dev *hdev)
 	setup_timer(&hdev->le_scan_timer, hci_disable_le_scan,
 						(unsigned long) hdev);
 
+	setup_timer(&hdev->rssi_monitor_timer, hci_do_read_rssi,
+						(unsigned long) hdev);
+
 	INIT_WORK(&hdev->power_on, hci_power_on);
 	INIT_WORK(&hdev->power_off, hci_power_off);
 	setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
@@ -1604,6 +1615,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
 	hci_del_off_timer(hdev);
 	del_timer(&hdev->adv_timer);
 	del_timer(&hdev->le_scan_timer);
+	del_timer(&hdev->rssi_monitor_timer);
 
 	destroy_workqueue(hdev->workqueue);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 65ddbff..b034290 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -48,6 +48,8 @@ enum bt_device_type {
 #define RSSI_MONITOR_ALERT_LOW		0x01
 #define RSSI_MONITOR_ALERT_HIGH		0x02
 
+#define RSSI_MONITOR_TIMEOUT		2000
+
 struct pending_cmd {
 	struct list_head list;
 	__u16 opcode;
@@ -70,6 +72,9 @@ struct rssi_monitor {
 
 static LIST_HEAD(rssi_monitor_list);
 
+static rwlock_t rssi_monitor_list_lock;
+static DEFINE_RWLOCK(rssi_monitor_list_lock);
+
 static int cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status)
 {
 	struct sk_buff *skb;
@@ -1916,21 +1921,46 @@ static struct rssi_monitor *rssi_monitor_find(u16 index, bdaddr_t *bdaddr)
 	return NULL;
 }
 
+static void rssi_monitor_start(u16 index)
+{
+	struct hci_dev *hdev;
+
+	if (list_empty(&rssi_monitor_list))
+		return;
+
+	hdev = hci_dev_get(index);
+	if (!hdev)
+		return;
+
+	mod_timer(&hdev->rssi_monitor_timer, jiffies +
+			msecs_to_jiffies(RSSI_MONITOR_TIMEOUT));
+	hci_dev_put(hdev);
+}
+
 static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
 							s8 high_trigger)
 {
 	struct rssi_monitor *rm;
+	int err = 0;
 
-	if (rssi_monitor_find(index, bdaddr))
-		return -EEXIST;
+	write_lock_bh(&rssi_monitor_list_lock);
+
+	if (rssi_monitor_find(index, bdaddr)) {
+		err = -EEXIST;
+		goto out;
+	}
 
 	if (low_trigger < -127 || low_trigger > 128 ||
-			high_trigger < -127 || high_trigger > 128)
-		return -EINVAL;
+			high_trigger < -127 || high_trigger > 128) {
+		err = -EINVAL;
+		goto out;
+	}
 
 	rm = kzalloc(sizeof(*rm), GFP_ATOMIC);
-	if (!rm)
-		return -ENOMEM;
+	if (!rm) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	rm->index = index;
 	bacpy(&rm->bdaddr, bdaddr);
@@ -1940,20 +1970,46 @@ static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
 
 	list_add(&rm->list, &rssi_monitor_list);
 
-	return 0;
+	rssi_monitor_start(index);
+
+out:
+	write_unlock_bh(&rssi_monitor_list_lock);
+	return err;
 }
 
 static int rssi_monitor_remove(u16 index, bdaddr_t *bdaddr)
 {
 	struct rssi_monitor *rm;
+	struct hci_dev *hdev;
+	int err = 0;
+
+	write_lock_bh(&rssi_monitor_list_lock);
 
 	rm = rssi_monitor_find(index, bdaddr);
-	if (!rm)
-		return -EINVAL;
+	if (!rm) {
+		err = -EINVAL;
+		goto out;
+	}
 
 	list_del(&rm->list);
+	kfree(rm);
 
-	return 0;
+	if (!list_empty(&rssi_monitor_list))
+		goto out;
+
+	hdev = hci_dev_get(index);
+	if (!hdev) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	del_timer(&hdev->rssi_monitor_timer);
+
+	hci_dev_put(hdev);
+
+out:
+	write_unlock_bh(&rssi_monitor_list_lock);
+	return err;
 }
 
 static int enable_rssi_monitor(struct sock *sk, u16 index,
@@ -1995,7 +2051,11 @@ static int rssi_monitor_send_alert(u16 index, bdaddr_t *bdaddr, s8 rssi)
 	struct mgmt_ev_rssi_monitor_alert ev;
 	u8 alert;
 
+	read_lock(&rssi_monitor_list_lock);
+
 	rm = rssi_monitor_find(index, bdaddr);
+
+	read_unlock(&rssi_monitor_list_lock);
 	if (!rm)
 		return -EINVAL;
 
@@ -2718,3 +2778,22 @@ void mgmt_read_rssi_complete(u16 index, bdaddr_t *bdaddr, s8 rssi, u8 status)
 
 	rssi_monitor_send_alert(index, bdaddr, rssi);
 }
+
+void mgmt_do_read_rssi(struct hci_dev *hdev)
+{
+	struct rssi_monitor *rm;
+
+	read_lock(&rssi_monitor_list_lock);
+
+	list_for_each_entry(rm, &rssi_monitor_list, list) {
+
+		if (rm->index != hdev->id)
+			continue;
+
+		mgmt_read_rssi(hdev, &rm->bdaddr);
+	}
+
+	read_unlock(&rssi_monitor_list_lock);
+
+	rssi_monitor_start(hdev->id);
+}
-- 
1.7.4.1

Re: [RFC 6/7] Bluetooth: Add RSSI Monitor timer and list lock

[Gustavo Padovan]

Hi Briglia,

* Anderson Briglia <anderson.briglia@openbossa.org> [2011-08-02 14:35:08 -0400]:

> This is the polling mechanism to send HCI Read RSSI commands. The
> command responses are used by the RSSI Monitor to get the current RSSI
> value and calculate the current alert level for a given monitored
> connection.
> 
> This patch also adds read and write locks on rssi_monitor_list in
> order to avoid race conditions.
> 
> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    3 +
>  net/bluetooth/hci_core.c         |   12 +++++
>  net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++----
>  3 files changed, 104 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a3cf433..8ff3448 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -211,6 +211,8 @@ struct hci_dev {
>  
>  	struct timer_list	le_scan_timer;
>  
> +	struct timer_list	rssi_monitor_timer;

call this rssi_timer is enough.

> +
>  	struct hci_dev_stats	stat;
>  
>  	struct sk_buff_head	driver_init;
> @@ -869,6 +871,7 @@ int mgmt_has_pending_stop_discov(u16 index);
>  int mgmt_cancel_discovery(u16 index);
>  int mgmt_is_interleaved_discovery(u16 index);
>  int mgmt_do_interleaved_discovery(u16 index);
> +void mgmt_do_read_rssi(struct hci_dev *hdev);
>  
>  /* HCI info for socket */
>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index c0c46bf..eb51b94 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1449,6 +1449,14 @@ static void hci_disable_le_scan(unsigned long arg)
>  	hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>  }
>  
> +static void hci_do_read_rssi(unsigned long arg)
> +{
> +	struct hci_dev *hdev = (void *) arg;
> +
> +	if (test_bit(HCI_UP, &hdev->flags))
> +		mgmt_do_read_rssi(hdev);
> +}
> +
>  /* Register HCI device */
>  int hci_register_dev(struct hci_dev *hdev)
>  {
> @@ -1522,6 +1530,9 @@ int hci_register_dev(struct hci_dev *hdev)
>  	setup_timer(&hdev->le_scan_timer, hci_disable_le_scan,
>  						(unsigned long) hdev);
>  
> +	setup_timer(&hdev->rssi_monitor_timer, hci_do_read_rssi,
> +						(unsigned long) hdev);
> +
>  	INIT_WORK(&hdev->power_on, hci_power_on);
>  	INIT_WORK(&hdev->power_off, hci_power_off);
>  	setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
> @@ -1604,6 +1615,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
>  	hci_del_off_timer(hdev);
>  	del_timer(&hdev->adv_timer);
>  	del_timer(&hdev->le_scan_timer);
> +	del_timer(&hdev->rssi_monitor_timer);
>  
>  	destroy_workqueue(hdev->workqueue);
>  
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 65ddbff..b034290 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -48,6 +48,8 @@ enum bt_device_type {
>  #define RSSI_MONITOR_ALERT_LOW		0x01
>  #define RSSI_MONITOR_ALERT_HIGH		0x02
>  
> +#define RSSI_MONITOR_TIMEOUT		2000
> +
>  struct pending_cmd {
>  	struct list_head list;
>  	__u16 opcode;
> @@ -70,6 +72,9 @@ struct rssi_monitor {
>  
>  static LIST_HEAD(rssi_monitor_list);
>  
> +static rwlock_t rssi_monitor_list_lock;
> +static DEFINE_RWLOCK(rssi_monitor_list_lock);

Use a normal spinlock here.

> +
>  static int cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status)
>  {
>  	struct sk_buff *skb;
> @@ -1916,21 +1921,46 @@ static struct rssi_monitor *rssi_monitor_find(u16 index, bdaddr_t *bdaddr)
>  	return NULL;
>  }
>  
> +static void rssi_monitor_start(u16 index)
> +{
> +	struct hci_dev *hdev;
> +
> +	if (list_empty(&rssi_monitor_list))
> +		return;
> +
> +	hdev = hci_dev_get(index);
> +	if (!hdev)
> +		return;
> +
> +	mod_timer(&hdev->rssi_monitor_timer, jiffies +
> +			msecs_to_jiffies(RSSI_MONITOR_TIMEOUT));
> +	hci_dev_put(hdev);
> +}
> +
>  static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
>  							s8 high_trigger)
>  {
>  	struct rssi_monitor *rm;
> +	int err = 0;
>  
> -	if (rssi_monitor_find(index, bdaddr))
> -		return -EEXIST;
> +	write_lock_bh(&rssi_monitor_list_lock);
> +
> +	if (rssi_monitor_find(index, bdaddr)) {
> +		err = -EEXIST;
> +		goto out;
> +	}
>  
>  	if (low_trigger < -127 || low_trigger > 128 ||
> -			high_trigger < -127 || high_trigger > 128)
> -		return -EINVAL;
> +			high_trigger < -127 || high_trigger > 128) {
> +		err = -EINVAL;
> +		goto out;
> +	}
>  
>  	rm = kzalloc(sizeof(*rm), GFP_ATOMIC);
> -	if (!rm)
> -		return -ENOMEM;
> +	if (!rm) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
>  
>  	rm->index = index;
>  	bacpy(&rm->bdaddr, bdaddr);
> @@ -1940,20 +1970,46 @@ static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
>  
>  	list_add(&rm->list, &rssi_monitor_list);
>  
> -	return 0;
> +	rssi_monitor_start(index);
> +
> +out:
> +	write_unlock_bh(&rssi_monitor_list_lock);
> +	return err;
>  }
>  
>  static int rssi_monitor_remove(u16 index, bdaddr_t *bdaddr)
>  {
>  	struct rssi_monitor *rm;
> +	struct hci_dev *hdev;
> +	int err = 0;
> +
> +	write_lock_bh(&rssi_monitor_list_lock);
>  
>  	rm = rssi_monitor_find(index, bdaddr);
> -	if (!rm)
> -		return -EINVAL;
> +	if (!rm) {
> +		err = -EINVAL;
> +		goto out;
> +	}
>  
>  	list_del(&rm->list);
> +	kfree(rm);
>  
> -	return 0;
> +	if (!list_empty(&rssi_monitor_list))
> +		goto out;

This need to be a per hdev list, otherwise we will only disable the timer if
there is no monitor for all adapters.

> +
> +	hdev = hci_dev_get(index);
> +	if (!hdev) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	del_timer(&hdev->rssi_monitor_timer);
> +
> +	hci_dev_put(hdev);
> +
> +out:
> +	write_unlock_bh(&rssi_monitor_list_lock);
> +	return err;
>  }
>  
>  static int enable_rssi_monitor(struct sock *sk, u16 index,
> @@ -1995,7 +2051,11 @@ static int rssi_monitor_send_alert(u16 index, bdaddr_t *bdaddr, s8 rssi)
>  	struct mgmt_ev_rssi_monitor_alert ev;
>  	u8 alert;
>  
> +	read_lock(&rssi_monitor_list_lock);
> +
>  	rm = rssi_monitor_find(index, bdaddr);
> +
> +	read_unlock(&rssi_monitor_list_lock);
>  	if (!rm)
>  		return -EINVAL;
>  
> @@ -2718,3 +2778,22 @@ void mgmt_read_rssi_complete(u16 index, bdaddr_t *bdaddr, s8 rssi, u8 status)
>  
>  	rssi_monitor_send_alert(index, bdaddr, rssi);
>  }
> +
> +void mgmt_do_read_rssi(struct hci_dev *hdev)
> +{
> +	struct rssi_monitor *rm;
> +
> +	read_lock(&rssi_monitor_list_lock);
> +
> +	list_for_each_entry(rm, &rssi_monitor_list, list) {
> +
> +		if (rm->index != hdev->id)
> +			continue;
> +
> +		mgmt_read_rssi(hdev, &rm->bdaddr);
> +	}

Per adapter list could make this code better as well.

	Gustavo

Re: [RFC 6/7] Bluetooth: Add RSSI Monitor timer and list lock

[Anderson Briglia]

Hi Padovan,

On Wed, Aug 3, 2011 at 2:48 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> Hi Briglia,
>
> * Anderson Briglia <anderson.briglia@openbossa.org> [2011-08-02 14:35:08 -0400]:
>
>> This is the polling mechanism to send HCI Read RSSI commands. The
>> command responses are used by the RSSI Monitor to get the current RSSI
>> value and calculate the current alert level for a given monitored
>> connection.
>>
>> This patch also adds read and write locks on rssi_monitor_list in
>> order to avoid race conditions.
>>
>> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
>> ---
>>  include/net/bluetooth/hci_core.h |    3 +
>>  net/bluetooth/hci_core.c         |   12 +++++
>>  net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++----
>>  3 files changed, 104 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index a3cf433..8ff3448 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -211,6 +211,8 @@ struct hci_dev {
>>
>>       struct timer_list       le_scan_timer;
>>
>> +     struct timer_list       rssi_monitor_timer;
>
> call this rssi_timer is enough.
>
>> +
>>       struct hci_dev_stats    stat;
>>
>>       struct sk_buff_head     driver_init;
>> @@ -869,6 +871,7 @@ int mgmt_has_pending_stop_discov(u16 index);
>>  int mgmt_cancel_discovery(u16 index);
>>  int mgmt_is_interleaved_discovery(u16 index);
>>  int mgmt_do_interleaved_discovery(u16 index);
>> +void mgmt_do_read_rssi(struct hci_dev *hdev);
>>
>>  /* HCI info for socket */
>>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index c0c46bf..eb51b94 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1449,6 +1449,14 @@ static void hci_disable_le_scan(unsigned long arg)
>>       hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>>  }
>>
>> +static void hci_do_read_rssi(unsigned long arg)
>> +{
>> +     struct hci_dev *hdev = (void *) arg;
>> +
>> +     if (test_bit(HCI_UP, &hdev->flags))
>> +             mgmt_do_read_rssi(hdev);
>> +}
>> +
>>  /* Register HCI device */
>>  int hci_register_dev(struct hci_dev *hdev)
>>  {
>> @@ -1522,6 +1530,9 @@ int hci_register_dev(struct hci_dev *hdev)
>>       setup_timer(&hdev->le_scan_timer, hci_disable_le_scan,
>>                                               (unsigned long) hdev);
>>
>> +     setup_timer(&hdev->rssi_monitor_timer, hci_do_read_rssi,
>> +                                             (unsigned long) hdev);
>> +
>>       INIT_WORK(&hdev->power_on, hci_power_on);
>>       INIT_WORK(&hdev->power_off, hci_power_off);
>>       setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
>> @@ -1604,6 +1615,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
>>       hci_del_off_timer(hdev);
>>       del_timer(&hdev->adv_timer);
>>       del_timer(&hdev->le_scan_timer);
>> +     del_timer(&hdev->rssi_monitor_timer);
>>
>>       destroy_workqueue(hdev->workqueue);
>>
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 65ddbff..b034290 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -48,6 +48,8 @@ enum bt_device_type {
>>  #define RSSI_MONITOR_ALERT_LOW               0x01
>>  #define RSSI_MONITOR_ALERT_HIGH              0x02
>>
>> +#define RSSI_MONITOR_TIMEOUT         2000
>> +
>>  struct pending_cmd {
>>       struct list_head list;
>>       __u16 opcode;
>> @@ -70,6 +72,9 @@ struct rssi_monitor {
>>
>>  static LIST_HEAD(rssi_monitor_list);
>>
>> +static rwlock_t rssi_monitor_list_lock;
>> +static DEFINE_RWLOCK(rssi_monitor_list_lock);
>
> Use a normal spinlock here.

Ok.
>
>> +
>>  static int cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status)
>>  {
>>       struct sk_buff *skb;
>> @@ -1916,21 +1921,46 @@ static struct rssi_monitor *rssi_monitor_find(u16 index, bdaddr_t *bdaddr)
>>       return NULL;
>>  }
>>
>> +static void rssi_monitor_start(u16 index)
>> +{
>> +     struct hci_dev *hdev;
>> +
>> +     if (list_empty(&rssi_monitor_list))
>> +             return;
>> +
>> +     hdev = hci_dev_get(index);
>> +     if (!hdev)
>> +             return;
>> +
>> +     mod_timer(&hdev->rssi_monitor_timer, jiffies +
>> +                     msecs_to_jiffies(RSSI_MONITOR_TIMEOUT));
>> +     hci_dev_put(hdev);
>> +}
>> +
>>  static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
>>                                                       s8 high_trigger)
>>  {
>>       struct rssi_monitor *rm;
>> +     int err = 0;
>>
>> -     if (rssi_monitor_find(index, bdaddr))
>> -             return -EEXIST;
>> +     write_lock_bh(&rssi_monitor_list_lock);
>> +
>> +     if (rssi_monitor_find(index, bdaddr)) {
>> +             err = -EEXIST;
>> +             goto out;
>> +     }
>>
>>       if (low_trigger < -127 || low_trigger > 128 ||
>> -                     high_trigger < -127 || high_trigger > 128)
>> -             return -EINVAL;
>> +                     high_trigger < -127 || high_trigger > 128) {
>> +             err = -EINVAL;
>> +             goto out;
>> +     }
>>
>>       rm = kzalloc(sizeof(*rm), GFP_ATOMIC);
>> -     if (!rm)
>> -             return -ENOMEM;
>> +     if (!rm) {
>> +             err = -ENOMEM;
>> +             goto out;
>> +     }
>>
>>       rm->index = index;
>>       bacpy(&rm->bdaddr, bdaddr);
>> @@ -1940,20 +1970,46 @@ static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
>>
>>       list_add(&rm->list, &rssi_monitor_list);
>>
>> -     return 0;
>> +     rssi_monitor_start(index);
>> +
>> +out:
>> +     write_unlock_bh(&rssi_monitor_list_lock);
>> +     return err;
>>  }
>>
>>  static int rssi_monitor_remove(u16 index, bdaddr_t *bdaddr)
>>  {
>>       struct rssi_monitor *rm;
>> +     struct hci_dev *hdev;
>> +     int err = 0;
>> +
>> +     write_lock_bh(&rssi_monitor_list_lock);
>>
>>       rm = rssi_monitor_find(index, bdaddr);
>> -     if (!rm)
>> -             return -EINVAL;
>> +     if (!rm) {
>> +             err = -EINVAL;
>> +             goto out;
>> +     }
>>
>>       list_del(&rm->list);
>> +     kfree(rm);
>>
>> -     return 0;
>> +     if (!list_empty(&rssi_monitor_list))
>> +             goto out;
>
> This need to be a per hdev list, otherwise we will only disable the timer if
> there is no monitor for all adapters.

Agreed. Will fix it.

>
>> +
>> +     hdev = hci_dev_get(index);
>> +     if (!hdev) {
>> +             err = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     del_timer(&hdev->rssi_monitor_timer);
>> +
>> +     hci_dev_put(hdev);
>> +
>> +out:
>> +     write_unlock_bh(&rssi_monitor_list_lock);
>> +     return err;
>>  }
>>
>>  static int enable_rssi_monitor(struct sock *sk, u16 index,
>> @@ -1995,7 +2051,11 @@ static int rssi_monitor_send_alert(u16 index, bdaddr_t *bdaddr, s8 rssi)
>>       struct mgmt_ev_rssi_monitor_alert ev;
>>       u8 alert;
>>
>> +     read_lock(&rssi_monitor_list_lock);
>> +
>>       rm = rssi_monitor_find(index, bdaddr);
>> +
>> +     read_unlock(&rssi_monitor_list_lock);
>>       if (!rm)
>>               return -EINVAL;
>>
>> @@ -2718,3 +2778,22 @@ void mgmt_read_rssi_complete(u16 index, bdaddr_t *bdaddr, s8 rssi, u8 status)
>>
>>       rssi_monitor_send_alert(index, bdaddr, rssi);
>>  }
>> +
>> +void mgmt_do_read_rssi(struct hci_dev *hdev)
>> +{
>> +     struct rssi_monitor *rm;
>> +
>> +     read_lock(&rssi_monitor_list_lock);
>> +
>> +     list_for_each_entry(rm, &rssi_monitor_list, list) {
>> +
>> +             if (rm->index != hdev->id)
>> +                     continue;
>> +
>> +             mgmt_read_rssi(hdev, &rm->bdaddr);
>> +     }
>
> Per adapter list could make this code better as well.

The idea to keep this list only into Management module was to avoid
modifications in other part of bluetooth code since this RSSI
monitoring is expected to be implemented inside the controllers. But I
agree with you, if we have this list per adapter it could simplify the
code. What should I do? Move the list to hdev?

Briglia

>
>        Gustavo
>



-- 
INdT - Instituto Nokia de tecnologia
+55 92 2126 1122
+55 92 8423 3183

Re: [RFC 6/7] Bluetooth: Add RSSI Monitor timer and list lock

[Gustavo Padovan]

Hi Briglia,

* Anderson Briglia <anderson.briglia@openbossa.org> [2011-08-03 15:09:50 -0400]:

> Hi Padovan,
> 
> On Wed, Aug 3, 2011 at 2:48 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> > Hi Briglia,
> >
> > * Anderson Briglia <anderson.briglia@openbossa.org> [2011-08-02 14:35:08 -0400]:
> >
> >> This is the polling mechanism to send HCI Read RSSI commands. The
> >> command responses are used by the RSSI Monitor to get the current RSSI
> >> value and calculate the current alert level for a given monitored
> >> connection.
> >>
> >> This patch also adds read and write locks on rssi_monitor_list in
> >> order to avoid race conditions.
> >>
> >> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
> >> ---
> >>  include/net/bluetooth/hci_core.h |    3 +
> >>  net/bluetooth/hci_core.c         |   12 +++++
> >>  net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++----
> >>  3 files changed, 104 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index a3cf433..8ff3448 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -211,6 +211,8 @@ struct hci_dev {
> >>
> >>       struct timer_list       le_scan_timer;
> >>
> >> +     struct timer_list       rssi_monitor_timer;
> >
> > call this rssi_timer is enough.
> >
> >> +
> >>       struct hci_dev_stats    stat;
> >>
> >>       struct sk_buff_head     driver_init;
> >> @@ -869,6 +871,7 @@ int mgmt_has_pending_stop_discov(u16 index);
> >>  int mgmt_cancel_discovery(u16 index);
> >>  int mgmt_is_interleaved_discovery(u16 index);
> >>  int mgmt_do_interleaved_discovery(u16 index);
> >> +void mgmt_do_read_rssi(struct hci_dev *hdev);
> >>
> >>  /* HCI info for socket */
> >>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> index c0c46bf..eb51b94 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -1449,6 +1449,14 @@ static void hci_disable_le_scan(unsigned long arg)
> >>       hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> >>  }
> >>
> >> +static void hci_do_read_rssi(unsigned long arg)
> >> +{
> >> +     struct hci_dev *hdev = (void *) arg;
> >> +
> >> +     if (test_bit(HCI_UP, &hdev->flags))
> >> +             mgmt_do_read_rssi(hdev);
> >> +}
> >> +
> >>  /* Register HCI device */
> >>  int hci_register_dev(struct hci_dev *hdev)
> >>  {
> >> @@ -1522,6 +1530,9 @@ int hci_register_dev(struct hci_dev *hdev)
> >>       setup_timer(&hdev->le_scan_timer, hci_disable_le_scan,
> >>                                               (unsigned long) hdev);
> >>
> >> +     setup_timer(&hdev->rssi_monitor_timer, hci_do_read_rssi,
> >> +                                             (unsigned long) hdev);
> >> +
> >>       INIT_WORK(&hdev->power_on, hci_power_on);
> >>       INIT_WORK(&hdev->power_off, hci_power_off);
> >>       setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
> >> @@ -1604,6 +1615,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
> >>       hci_del_off_timer(hdev);
> >>       del_timer(&hdev->adv_timer);
> >>       del_timer(&hdev->le_scan_timer);
> >> +     del_timer(&hdev->rssi_monitor_timer);
> >>
> >>       destroy_workqueue(hdev->workqueue);
> >>
> >> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >> index 65ddbff..b034290 100644
> >> --- a/net/bluetooth/mgmt.c
> >> +++ b/net/bluetooth/mgmt.c
> >> @@ -48,6 +48,8 @@ enum bt_device_type {
> >>  #define RSSI_MONITOR_ALERT_LOW               0x01
> >>  #define RSSI_MONITOR_ALERT_HIGH              0x02
> >>
> >> +#define RSSI_MONITOR_TIMEOUT         2000
> >> +
> >>  struct pending_cmd {
> >>       struct list_head list;
> >>       __u16 opcode;
> >> @@ -70,6 +72,9 @@ struct rssi_monitor {
> >>
> >>  static LIST_HEAD(rssi_monitor_list);
> >>
> >> +static rwlock_t rssi_monitor_list_lock;
> >> +static DEFINE_RWLOCK(rssi_monitor_list_lock);
> >
> > Use a normal spinlock here.
> 
> Ok.
> >
> >> +
> >>  static int cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status)
> >>  {
> >>       struct sk_buff *skb;
> >> @@ -1916,21 +1921,46 @@ static struct rssi_monitor *rssi_monitor_find(u16 index, bdaddr_t *bdaddr)
> >>       return NULL;
> >>  }
> >>
> >> +static void rssi_monitor_start(u16 index)
> >> +{
> >> +     struct hci_dev *hdev;
> >> +
> >> +     if (list_empty(&rssi_monitor_list))
> >> +             return;
> >> +
> >> +     hdev = hci_dev_get(index);
> >> +     if (!hdev)
> >> +             return;
> >> +
> >> +     mod_timer(&hdev->rssi_monitor_timer, jiffies +
> >> +                     msecs_to_jiffies(RSSI_MONITOR_TIMEOUT));
> >> +     hci_dev_put(hdev);
> >> +}
> >> +
> >>  static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
> >>                                                       s8 high_trigger)
> >>  {
> >>       struct rssi_monitor *rm;
> >> +     int err = 0;
> >>
> >> -     if (rssi_monitor_find(index, bdaddr))
> >> -             return -EEXIST;
> >> +     write_lock_bh(&rssi_monitor_list_lock);
> >> +
> >> +     if (rssi_monitor_find(index, bdaddr)) {
> >> +             err = -EEXIST;
> >> +             goto out;
> >> +     }
> >>
> >>       if (low_trigger < -127 || low_trigger > 128 ||
> >> -                     high_trigger < -127 || high_trigger > 128)
> >> -             return -EINVAL;
> >> +                     high_trigger < -127 || high_trigger > 128) {
> >> +             err = -EINVAL;
> >> +             goto out;
> >> +     }
> >>
> >>       rm = kzalloc(sizeof(*rm), GFP_ATOMIC);
> >> -     if (!rm)
> >> -             return -ENOMEM;
> >> +     if (!rm) {
> >> +             err = -ENOMEM;
> >> +             goto out;
> >> +     }
> >>
> >>       rm->index = index;
> >>       bacpy(&rm->bdaddr, bdaddr);
> >> @@ -1940,20 +1970,46 @@ static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
> >>
> >>       list_add(&rm->list, &rssi_monitor_list);
> >>
> >> -     return 0;
> >> +     rssi_monitor_start(index);
> >> +
> >> +out:
> >> +     write_unlock_bh(&rssi_monitor_list_lock);
> >> +     return err;
> >>  }
> >>
> >>  static int rssi_monitor_remove(u16 index, bdaddr_t *bdaddr)
> >>  {
> >>       struct rssi_monitor *rm;
> >> +     struct hci_dev *hdev;
> >> +     int err = 0;
> >> +
> >> +     write_lock_bh(&rssi_monitor_list_lock);
> >>
> >>       rm = rssi_monitor_find(index, bdaddr);
> >> -     if (!rm)
> >> -             return -EINVAL;
> >> +     if (!rm) {
> >> +             err = -EINVAL;
> >> +             goto out;
> >> +     }
> >>
> >>       list_del(&rm->list);
> >> +     kfree(rm);
> >>
> >> -     return 0;
> >> +     if (!list_empty(&rssi_monitor_list))
> >> +             goto out;
> >
> > This need to be a per hdev list, otherwise we will only disable the timer if
> > there is no monitor for all adapters.
> 
> Agreed. Will fix it.
> 
> >
> >> +
> >> +     hdev = hci_dev_get(index);
> >> +     if (!hdev) {
> >> +             err = -EINVAL;
> >> +             goto out;
> >> +     }
> >> +
> >> +     del_timer(&hdev->rssi_monitor_timer);
> >> +
> >> +     hci_dev_put(hdev);
> >> +
> >> +out:
> >> +     write_unlock_bh(&rssi_monitor_list_lock);
> >> +     return err;
> >>  }
> >>
> >>  static int enable_rssi_monitor(struct sock *sk, u16 index,
> >> @@ -1995,7 +2051,11 @@ static int rssi_monitor_send_alert(u16 index, bdaddr_t *bdaddr, s8 rssi)
> >>       struct mgmt_ev_rssi_monitor_alert ev;
> >>       u8 alert;
> >>
> >> +     read_lock(&rssi_monitor_list_lock);
> >> +
> >>       rm = rssi_monitor_find(index, bdaddr);
> >> +
> >> +     read_unlock(&rssi_monitor_list_lock);
> >>       if (!rm)
> >>               return -EINVAL;
> >>
> >> @@ -2718,3 +2778,22 @@ void mgmt_read_rssi_complete(u16 index, bdaddr_t *bdaddr, s8 rssi, u8 status)
> >>
> >>       rssi_monitor_send_alert(index, bdaddr, rssi);
> >>  }
> >> +
> >> +void mgmt_do_read_rssi(struct hci_dev *hdev)
> >> +{
> >> +     struct rssi_monitor *rm;
> >> +
> >> +     read_lock(&rssi_monitor_list_lock);
> >> +
> >> +     list_for_each_entry(rm, &rssi_monitor_list, list) {
> >> +
> >> +             if (rm->index != hdev->id)
> >> +                     continue;
> >> +
> >> +             mgmt_read_rssi(hdev, &rm->bdaddr);
> >> +     }
> >
> > Per adapter list could make this code better as well.
> 
> The idea to keep this list only into Management module was to avoid
> modifications in other part of bluetooth code since this RSSI
> monitoring is expected to be implemented inside the controllers. But I
> agree with you, if we have this list per adapter it could simplify the
> code. What should I do? Move the list to hdev?

Just add it to hdev.

I also would like to move all the rssi timer logic to mgmt, to avoid some
round trips between HCI and MGMT

like hci_do_read_rssi -> mgmt_do_read_rssi -> hci_send_cmd.

Also the setup_timer is in hci_core.c and mod_timer is mgmt.c.

I'm still thinking on a good way to do this.

	Gustavo

Re: [RFC 6/7] Bluetooth: Add RSSI Monitor timer and list lock

[Claudio Takahasi]

Hi Briglia,

On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
<anderson.briglia@openbossa.org> wrote:
> This is the polling mechanism to send HCI Read RSSI commands. The
> command responses are used by the RSSI Monitor to get the current RSSI
> value and calculate the current alert level for a given monitored
> connection.
>
> This patch also adds read and write locks on rssi_monitor_list in
> order to avoid race conditions.
>
> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    3 +
>  net/bluetooth/hci_core.c         |   12 +++++
>  net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++----
>  3 files changed, 104 insertions(+), 10 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a3cf433..8ff3448 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -211,6 +211,8 @@ struct hci_dev {
>
>        struct timer_list       le_scan_timer;
>
> +       struct timer_list       rssi_monitor_timer;
> +
>        struct hci_dev_stats    stat;
>
>        struct sk_buff_head     driver_init;
> @@ -869,6 +871,7 @@ int mgmt_has_pending_stop_discov(u16 index);
>  int mgmt_cancel_discovery(u16 index);
>  int mgmt_is_interleaved_discovery(u16 index);
>  int mgmt_do_interleaved_discovery(u16 index);
> +void mgmt_do_read_rssi(struct hci_dev *hdev);
>
>  /* HCI info for socket */
>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index c0c46bf..eb51b94 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1449,6 +1449,14 @@ static void hci_disable_le_scan(unsigned long arg)
>        hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>  }
>
> +static void hci_do_read_rssi(unsigned long arg)
> +{
> +       struct hci_dev *hdev = (void *) arg;
> +
> +       if (test_bit(HCI_UP, &hdev->flags))
> +               mgmt_do_read_rssi(hdev);
> +}
> +
>  /* Register HCI device */
>  int hci_register_dev(struct hci_dev *hdev)
>  {
> @@ -1522,6 +1530,9 @@ int hci_register_dev(struct hci_dev *hdev)
>        setup_timer(&hdev->le_scan_timer, hci_disable_le_scan,
>                                                (unsigned long) hdev);
>
> +       setup_timer(&hdev->rssi_monitor_timer, hci_do_read_rssi,
> +                                               (unsigned long) hdev);
> +
>        INIT_WORK(&hdev->power_on, hci_power_on);
>        INIT_WORK(&hdev->power_off, hci_power_off);
>        setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
> @@ -1604,6 +1615,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
>        hci_del_off_timer(hdev);
>        del_timer(&hdev->adv_timer);
>        del_timer(&hdev->le_scan_timer);
> +       del_timer(&hdev->rssi_monitor_timer);
>
>        destroy_workqueue(hdev->workqueue);
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 65ddbff..b034290 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -48,6 +48,8 @@ enum bt_device_type {
>  #define RSSI_MONITOR_ALERT_LOW         0x01
>  #define RSSI_MONITOR_ALERT_HIGH                0x02
>
> +#define RSSI_MONITOR_TIMEOUT           2000
> +
>  struct pending_cmd {
>        struct list_head list;
>        __u16 opcode;
> @@ -70,6 +72,9 @@ struct rssi_monitor {
>
>  static LIST_HEAD(rssi_monitor_list);
>
> +static rwlock_t rssi_monitor_list_lock;
> +static DEFINE_RWLOCK(rssi_monitor_list_lock);
> +
>  static int cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status)
>  {
>        struct sk_buff *skb;
> @@ -1916,21 +1921,46 @@ static struct rssi_monitor *rssi_monitor_find(u16 index, bdaddr_t *bdaddr)
>        return NULL;
>  }
>
> +static void rssi_monitor_start(u16 index)
> +{
> +       struct hci_dev *hdev;
> +
> +       if (list_empty(&rssi_monitor_list))
> +               return;
> +
> +       hdev = hci_dev_get(index);
> +       if (!hdev)
> +               return;
> +
> +       mod_timer(&hdev->rssi_monitor_timer, jiffies +
> +                       msecs_to_jiffies(RSSI_MONITOR_TIMEOUT));
> +       hci_dev_put(hdev);
> +}
> +
>  static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
>                                                        s8 high_trigger)
>  {
>        struct rssi_monitor *rm;
> +       int err = 0;
>
> -       if (rssi_monitor_find(index, bdaddr))
> -               return -EEXIST;
> +       write_lock_bh(&rssi_monitor_list_lock);
> +
> +       if (rssi_monitor_find(index, bdaddr)) {
> +               err = -EEXIST;
> +               goto out;
> +       }
>
>        if (low_trigger < -127 || low_trigger > 128 ||
> -                       high_trigger < -127 || high_trigger > 128)
> -               return -EINVAL;
> +                       high_trigger < -127 || high_trigger > 128) {
> +               err = -EINVAL;
> +               goto out;
> +       }

Threshold validation could be the first operation in this function.
It is also missing low < high

BR,
Claudio.

Re: [RFC 6/7] Bluetooth: Add RSSI Monitor timer and list lock

[Anderson Briglia]

Hi Claudio,

On Wed, Aug 3, 2011 at 3:01 PM, Claudio Takahasi
<claudio.takahasi@openbossa.org> wrote:
> Hi Briglia,
>
> On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
> <anderson.briglia@openbossa.org> wrote:
>> This is the polling mechanism to send HCI Read RSSI commands. The
>> command responses are used by the RSSI Monitor to get the current RSSI
>> value and calculate the current alert level for a given monitored
>> connection.
>>
>> This patch also adds read and write locks on rssi_monitor_list in
>> order to avoid race conditions.
>>
>> Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
>> ---
>>  include/net/bluetooth/hci_core.h |    3 +
>>  net/bluetooth/hci_core.c         |   12 +++++
>>  net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++----
>>  3 files changed, 104 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index a3cf433..8ff3448 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -211,6 +211,8 @@ struct hci_dev {
>>
>>        struct timer_list       le_scan_timer;
>>
>> +       struct timer_list       rssi_monitor_timer;
>> +
>>        struct hci_dev_stats    stat;
>>
>>        struct sk_buff_head     driver_init;
>> @@ -869,6 +871,7 @@ int mgmt_has_pending_stop_discov(u16 index);
>>  int mgmt_cancel_discovery(u16 index);
>>  int mgmt_is_interleaved_discovery(u16 index);
>>  int mgmt_do_interleaved_discovery(u16 index);
>> +void mgmt_do_read_rssi(struct hci_dev *hdev);
>>
>>  /* HCI info for socket */
>>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index c0c46bf..eb51b94 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1449,6 +1449,14 @@ static void hci_disable_le_scan(unsigned long arg)
>>        hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>>  }
>>
>> +static void hci_do_read_rssi(unsigned long arg)
>> +{
>> +       struct hci_dev *hdev = (void *) arg;
>> +
>> +       if (test_bit(HCI_UP, &hdev->flags))
>> +               mgmt_do_read_rssi(hdev);
>> +}
>> +
>>  /* Register HCI device */
>>  int hci_register_dev(struct hci_dev *hdev)
>>  {
>> @@ -1522,6 +1530,9 @@ int hci_register_dev(struct hci_dev *hdev)
>>        setup_timer(&hdev->le_scan_timer, hci_disable_le_scan,
>>                                                (unsigned long) hdev);
>>
>> +       setup_timer(&hdev->rssi_monitor_timer, hci_do_read_rssi,
>> +                                               (unsigned long) hdev);
>> +
>>        INIT_WORK(&hdev->power_on, hci_power_on);
>>        INIT_WORK(&hdev->power_off, hci_power_off);
>>        setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
>> @@ -1604,6 +1615,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
>>        hci_del_off_timer(hdev);
>>        del_timer(&hdev->adv_timer);
>>        del_timer(&hdev->le_scan_timer);
>> +       del_timer(&hdev->rssi_monitor_timer);
>>
>>        destroy_workqueue(hdev->workqueue);
>>
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 65ddbff..b034290 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -48,6 +48,8 @@ enum bt_device_type {
>>  #define RSSI_MONITOR_ALERT_LOW         0x01
>>  #define RSSI_MONITOR_ALERT_HIGH                0x02
>>
>> +#define RSSI_MONITOR_TIMEOUT           2000
>> +
>>  struct pending_cmd {
>>        struct list_head list;
>>        __u16 opcode;
>> @@ -70,6 +72,9 @@ struct rssi_monitor {
>>
>>  static LIST_HEAD(rssi_monitor_list);
>>
>> +static rwlock_t rssi_monitor_list_lock;
>> +static DEFINE_RWLOCK(rssi_monitor_list_lock);
>> +
>>  static int cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status)
>>  {
>>        struct sk_buff *skb;
>> @@ -1916,21 +1921,46 @@ static struct rssi_monitor *rssi_monitor_find(u16 index, bdaddr_t *bdaddr)
>>        return NULL;
>>  }
>>
>> +static void rssi_monitor_start(u16 index)
>> +{
>> +       struct hci_dev *hdev;
>> +
>> +       if (list_empty(&rssi_monitor_list))
>> +               return;
>> +
>> +       hdev = hci_dev_get(index);
>> +       if (!hdev)
>> +               return;
>> +
>> +       mod_timer(&hdev->rssi_monitor_timer, jiffies +
>> +                       msecs_to_jiffies(RSSI_MONITOR_TIMEOUT));
>> +       hci_dev_put(hdev);
>> +}
>> +
>>  static int rssi_monitor_add(u16 index, bdaddr_t *bdaddr, s8 low_trigger,
>>                                                        s8 high_trigger)
>>  {
>>        struct rssi_monitor *rm;
>> +       int err = 0;
>>
>> -       if (rssi_monitor_find(index, bdaddr))
>> -               return -EEXIST;
>> +       write_lock_bh(&rssi_monitor_list_lock);
>> +
>> +       if (rssi_monitor_find(index, bdaddr)) {
>> +               err = -EEXIST;
>> +               goto out;
>> +       }
>>
>>        if (low_trigger < -127 || low_trigger > 128 ||
>> -                       high_trigger < -127 || high_trigger > 128)
>> -               return -EINVAL;
>> +                       high_trigger < -127 || high_trigger > 128) {
>> +               err = -EINVAL;
>> +               goto out;
>> +       }
>
> Threshold validation could be the first operation in this function.
> It is also missing low < high

Ok. Will fix it.

>
> BR,
> Claudio.
>



-- 
INdT - Instituto Nokia de tecnologia
+55 92 2126 1122
+55 92 8423 3183

[RFC 7/7] Bluetooth: Remove RSSI monitor on disconnection

[Anderson Briglia]

Remove RSSI monitor from list when remote has been disconnected.

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
---
 net/bluetooth/mgmt.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index b034290..e2379d15 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2374,6 +2374,8 @@ int mgmt_disconnected(u16 index, bdaddr_t *bdaddr)
 	if (sk)
 		sock_put(sk);
 
+	err = rssi_monitor_remove(index, bdaddr);
+
 	return err;
 }
 
-- 
1.7.4.1

Re: [RFC 0/7] RSSI Monitor

[Claudio Takahasi]

Hi Briglia/Marcel/Padovan:

On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
<anderson.briglia@openbossa.org> wrote:
> This RFC is about RSSI monitoring since controllers do not have a specific
> command for this and we need it for Proximity Profile, at least.
>
> The RFC implements a mechanism to set threshold levels in order to send
> alerts to userspace when a RSSI value reaches one of the configured thresholds.
>
> Management Interface contains a list of RSSI Monitors. Elements are inserted
> or deleted from this list using two new Management commands: Enable and Disable
> RSSI Monitor.
>
> A timer was added to do HCI Read RSSI requests to the controller and get RSSI
> values updated. If there is monitored connection (eg.: RSSI Monitor list size >
> 0), the timer is reseted after two seconds and the RSSI for each monitored conn
> is updated. Using the RSSI value, a Management Event is sent if one of the
> thresholds were reached and the alert is different from the last one sent.
>
> Userspace is responsible to add and remove RSSI Monitors. Each monitor must
> have low and high thresholds values for RSSI. A RSSI Monitor is disabled when
> the connection is no longer available or the userspace sends a command to
> remove it. If there is no RSSI Monitor remaining in the list, the RSSI Read
> polling timer is removed.

Should be the entries persistent across connections? In the suggested
approach the RSSI threshold monitor for a given address is
automatically removed when the connection is lost.
By other hand, persistence introduces another problem: trust that the
userspace will disable the monitor.

Regards,
Claudio.

Re: [RFC 0/7] RSSI Monitor

[Gustavo Padovan]

* Claudio Takahasi <claudio.takahasi@openbossa.org> [2011-08-03 16:36:43 -0300]:

> Hi Briglia/Marcel/Padovan:
> 
> On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
> <anderson.briglia@openbossa.org> wrote:
> > This RFC is about RSSI monitoring since controllers do not have a specific
> > command for this and we need it for Proximity Profile, at least.
> >
> > The RFC implements a mechanism to set threshold levels in order to send
> > alerts to userspace when a RSSI value reaches one of the configured thresholds.
> >
> > Management Interface contains a list of RSSI Monitors. Elements are inserted
> > or deleted from this list using two new Management commands: Enable and Disable
> > RSSI Monitor.
> >
> > A timer was added to do HCI Read RSSI requests to the controller and get RSSI
> > values updated. If there is monitored connection (eg.: RSSI Monitor list size >
> > 0), the timer is reseted after two seconds and the RSSI for each monitored conn
> > is updated. Using the RSSI value, a Management Event is sent if one of the
> > thresholds were reached and the alert is different from the last one sent.
> >
> > Userspace is responsible to add and remove RSSI Monitors. Each monitor must
> > have low and high thresholds values for RSSI. A RSSI Monitor is disabled when
> > the connection is no longer available or the userspace sends a command to
> > remove it. If there is no RSSI Monitor remaining in the list, the RSSI Read
> > polling timer is removed.
> 
> Should be the entries persistent across connections? In the suggested
> approach the RSSI threshold monitor for a given address is
> automatically removed when the connection is lost.
> By other hand, persistence introduces another problem: trust that the
> userspace will disable the monitor.

Is it really worth? There are some reconnections to justify the persistent
entry? The wast of memory worth the savings in interactions between
kernel-userpace through the mgmt?

	Gustavo

Re: [RFC 0/7] RSSI Monitor

[Claudio Takahasi]

Hi Gustavo,

On Wed, Aug 3, 2011 at 5:25 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> * Claudio Takahasi <claudio.takahasi@openbossa.org> [2011-08-03 16:36:43 -0300]:
>
>> Hi Briglia/Marcel/Padovan:
>>
>> On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
>> <anderson.briglia@openbossa.org> wrote:
>> > This RFC is about RSSI monitoring since controllers do not have a specific
>> > command for this and we need it for Proximity Profile, at least.
>> >
>> > The RFC implements a mechanism to set threshold levels in order to send
>> > alerts to userspace when a RSSI value reaches one of the configured thresholds.
>> >
>> > Management Interface contains a list of RSSI Monitors. Elements are inserted
>> > or deleted from this list using two new Management commands: Enable and Disable
>> > RSSI Monitor.
>> >
>> > A timer was added to do HCI Read RSSI requests to the controller and get RSSI
>> > values updated. If there is monitored connection (eg.: RSSI Monitor list size >
>> > 0), the timer is reseted after two seconds and the RSSI for each monitored conn
>> > is updated. Using the RSSI value, a Management Event is sent if one of the
>> > thresholds were reached and the alert is different from the last one sent.
>> >
>> > Userspace is responsible to add and remove RSSI Monitors. Each monitor must
>> > have low and high thresholds values for RSSI. A RSSI Monitor is disabled when
>> > the connection is no longer available or the userspace sends a command to
>> > remove it. If there is no RSSI Monitor remaining in the list, the RSSI Read
>> > polling timer is removed.
>>
>> Should be the entries persistent across connections? In the suggested
>> approach the RSSI threshold monitor for a given address is
>> automatically removed when the connection is lost.
>> By other hand, persistence introduces another problem: trust that the
>> userspace will disable the monitor.
>
> Is it really worth? There are some reconnections to justify the persistent
> entry? The wast of memory worth the savings in interactions between
> kernel-userpace through the mgmt?
>
>        Gustavo
>

It saves logic in the userspace. Memory is not critical, how many
keyfobs a "normal" user will have?
Removing automatically means the kernel and userspace will be out of
sync, there isn't an event notifying that an entry was removed.

For Proximity, the RSSI threshold needs to be calculated, remote's Tx
power(read using ATT) is an input. In theory, Tx Power will not change
in the next connection, meaning that an additional logic can be added
in the Proximity Monitor to avoid enable/disable the RSSI threshold
monitor if the new calculated RSSI threshold doesn't change.

Regards,
Claudio

Re: [RFC 0/7] RSSI Monitor

[Anderson Briglia]

Hi Claudio,

On Wed, Aug 3, 2011 at 4:52 PM, Claudio Takahasi
<claudio.takahasi@openbossa.org> wrote:
> Hi Gustavo,
>
> On Wed, Aug 3, 2011 at 5:25 PM, Gustavo Padovan <padovan@profusion.mobi> =
wrote:
>> * Claudio Takahasi <claudio.takahasi@openbossa.org> [2011-08-03 16:36:43=
 -0300]:
>>
>>> Hi Briglia/Marcel/Padovan:
>>>
>>> On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
>>> <anderson.briglia@openbossa.org> wrote:
>>> > This RFC is about RSSI monitoring since controllers do not have a spe=
cific
>>> > command for this and we need it for Proximity Profile, at least.
>>> >
>>> > The RFC implements a mechanism to set threshold levels in order to se=
nd
>>> > alerts to userspace when a RSSI value reaches one of the configured t=
hresholds.
>>> >
>>> > Management Interface contains a list of RSSI Monitors. Elements are i=
nserted
>>> > or deleted from this list using two new Management commands: Enable a=
nd Disable
>>> > RSSI Monitor.
>>> >
>>> > A timer was added to do HCI Read RSSI requests to the controller and =
get RSSI
>>> > values updated. If there is monitored connection (eg.: RSSI Monitor l=
ist size >
>>> > 0), the timer is reseted after two seconds and the RSSI for each moni=
tored conn
>>> > is updated. Using the RSSI value, a Management Event is sent if one o=
f the
>>> > thresholds were reached and the alert is different from the last one =
sent.
>>> >
>>> > Userspace is responsible to add and remove RSSI Monitors. Each monito=
r must
>>> > have low and high thresholds values for RSSI. A RSSI Monitor is disab=
led when
>>> > the connection is no longer available or the userspace sends a comman=
d to
>>> > remove it. If there is no RSSI Monitor remaining in the list, the RSS=
I Read
>>> > polling timer is removed.
>>>
>>> Should be the entries persistent across connections? In the suggested
>>> approach the RSSI threshold monitor for a given address is
>>> automatically removed when the connection is lost.
>>> By other hand, persistence introduces another problem: trust that the
>>> userspace will disable the monitor.
>>
>> Is it really worth? There are some reconnections to justify the persiste=
nt
>> entry? The wast of memory worth the savings in interactions between
>> kernel-userpace through the mgmt?
>>
>> =A0 =A0 =A0 =A0Gustavo
>>
>
> It saves logic in the userspace. Memory is not critical, how many
> keyfobs a "normal" user will have?
> Removing automatically means the kernel and userspace will be out of
> sync, there isn't an event notifying that an entry was removed.
>
> For Proximity, the RSSI threshold needs to be calculated, remote's Tx
> power(read using ATT) is an input. In theory, Tx Power will not change
> in the next connection, meaning that an additional logic can be added
> in the Proximity Monitor to avoid enable/disable the RSSI threshold
> monitor if the new calculated RSSI threshold doesn't change.

Please, correct me if I'm wrong, but this persistency will be used to
save one command from userspace to kernel? I mean, Proximity
application still needs to tell to the kernel that it needs to start a
RSSI monitoring even if the thresholds had not changed. For example,
if a connection with the same remote address is established but we do
not want to use Proximity, userspace needs to tell to Management that
no RSSI monitoring should be started for that connection. In this
case, Management will not be able to enable/disable RSSI monitors
automatically.

IMHO, the userspace should be responsible to enable/disable RSSI monitors.

>
> Regards,
> Claudio
>



--=20
INdT - Instituto Nokia de tecnologia
+55 92 2126 1122
+55 92 8423 3183