2011-08-02 18:35:02

by Anderson Briglia

[permalink] [raw]
Subject: [RFC 0/7] RSSI Monitor

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



2011-08-03 21:52:43

by Anderson Briglia

[permalink] [raw]
Subject: Re: [RFC 0/7] RSSI Monitor

Hi Claudio,

On Wed, Aug 3, 2011 at 4:52 PM, Claudio Takahasi
<[email protected]> wrote:
> Hi Gustavo,
>
> On Wed, Aug 3, 2011 at 5:25 PM, Gustavo Padovan <[email protected]> =
wrote:
>> * Claudio Takahasi <[email protected]> [2011-08-03 16:36:43=
-0300]:
>>
>>> Hi Briglia/Marcel/Padovan:
>>>
>>> On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
>>> <[email protected]> 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

2011-08-03 20:52:20

by Claudio Takahasi

[permalink] [raw]
Subject: Re: [RFC 0/7] RSSI Monitor

Hi Gustavo,

On Wed, Aug 3, 2011 at 5:25 PM, Gustavo Padovan <[email protected]> wrote:
> * Claudio Takahasi <[email protected]> [2011-08-03 16:36:43 -0300]:
>
>> Hi Briglia/Marcel/Padovan:
>>
>> On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
>> <[email protected]> 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

2011-08-03 20:25:50

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [RFC 0/7] RSSI Monitor

* Claudio Takahasi <[email protected]> [2011-08-03 16:36:43 -0300]:

> Hi Briglia/Marcel/Padovan:
>
> On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
> <[email protected]> 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

2011-08-03 19:36:43

by Claudio Takahasi

[permalink] [raw]
Subject: Re: [RFC 0/7] RSSI Monitor

Hi Briglia/Marcel/Padovan:

On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
<[email protected]> 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.

2011-08-03 19:18:54

by Gustavo Padovan

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

Hi Briglia,

* Anderson Briglia <[email protected]> [2011-08-03 15:09:50 -0400]:

> Hi Padovan,
>
> On Wed, Aug 3, 2011 at 2:48 PM, Gustavo Padovan <[email protected]> wrote:
> > Hi Briglia,
> >
> > * Anderson Briglia <[email protected]> [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 <[email protected]>
> >> ---
> >> ?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

2011-08-03 19:10:26

by Anderson Briglia

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

Hi Claudio,

On Wed, Aug 3, 2011 at 3:01 PM, Claudio Takahasi
<[email protected]> wrote:
> Hi Briglia,
>
> On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
> <[email protected]> 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 <[email protected]>
>> ---
>> ?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

2011-08-03 19:09:50

by Anderson Briglia

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

Hi Padovan,

On Wed, Aug 3, 2011 at 2:48 PM, Gustavo Padovan <[email protected]> wrote:
> Hi Briglia,
>
> * Anderson Briglia <[email protected]> [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 <[email protected]>
>> ---
>> ?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

2011-08-03 19:01:41

by Claudio Takahasi

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

Hi Briglia,

On Tue, Aug 2, 2011 at 3:35 PM, Anderson Briglia
<[email protected]> 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 <[email protected]>
> ---
>  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.

2011-08-03 18:48:26

by Gustavo Padovan

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

Hi Briglia,

* Anderson Briglia <[email protected]> [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 <[email protected]>
> ---
> 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

2011-08-03 18:04:41

by Anderson Briglia

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

Hi Padovan,

On Wed, Aug 3, 2011 at 1:58 PM, Gustavo Padovan <[email protected]> wrote:
> Hi Briglia,
>
> * Anderson Briglia <[email protected]> [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 <[email protected]>
>> ---
>> ?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

2011-08-03 17:58:10

by Gustavo Padovan

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

Hi Briglia,

* Anderson Briglia <[email protected]> [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 <[email protected]>
> ---
> 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

2011-08-02 18:35:09

by Anderson Briglia

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

Remove RSSI monitor from list when remote has been disconnected.

Signed-off-by: Anderson Briglia <[email protected]>
---
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


2011-08-02 18:35:08

by Anderson Briglia

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

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 <[email protected]>
---
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


2011-08-02 18:35:07

by Anderson Briglia

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

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 <[email protected]>
---
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


2011-08-02 18:35:06

by Anderson Briglia

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

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 <[email protected]>
---
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


2011-08-02 18:35:05

by Anderson Briglia

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

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

Signed-off-by: Anderson Briglia <[email protected]>
---
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


2011-08-02 18:35:04

by Anderson Briglia

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

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 <[email protected]>
---
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


2011-08-02 18:35:03

by Anderson Briglia

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

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 <[email protected]>
---
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