2011-11-11 22:50:18

by Andre Guedes

Subject: [PATCH 0/6] LE-Only discovery procedure support

Hi all,

This patch series implements the LE-Only discovery procedure.

These patches are based on the last patches I sent to ML (already
acked by Marcel) but not upstream yet.

BTW, I'm still working on interleaved discovery and should send
its patches soon.

BR,

Andre

Andre Guedes (6):
Bluetooth: Add hci_flags to struct hci_dev
Bluetooth: Add LE Set Scan Parameter Command
Bluetooth: LE scan infra-structure
Bluetooth: Add 'eir_len' param to mgmt_device_found()
Bluetooth: Report LE devices
Bluetooth: Support LE-Only discovery procedure

include/net/bluetooth/hci.h | 18 ++++++++++
include/net/bluetooth/hci_core.h | 11 +++++-
net/bluetooth/hci_core.c | 70 ++++++++++++++++++++++++++++++++++++++
net/bluetooth/hci_event.c | 45 ++++++++++++++++++++----
net/bluetooth/mgmt.c | 33 ++++++++++++++++--
5 files changed, 165 insertions(+), 12 deletions(-)

--
1.7.7.1

2011-11-23 20:15:10

by Vinicius Costa Gomes

Subject: Re: [PATCH 5/6] Bluetooth: Report LE devices

Hi,

On 08:14 Sat 12 Nov, Marcel Holtmann wrote:
> Hi Andre,
>
> > Devices found during LE scan should be reported to userspace through
> > mgmt_device_found events.
> >
> > Signed-off-by: Andre Guedes <[email protected]>
> > ---
> > net/bluetooth/hci_event.c | 5 +++++
> > 1 files changed, 5 insertions(+), 0 deletions(-)
>
> Acked-by: Marcel Holtmann <[email protected]>

Now that the pair_device mgmt command requires address type information,
we need a way to get the address type information into userspace.

Applying this patch (and 4/6 of this same series) is the easiest way to
have that. We can use hcitool lescan to trigger an LE Scan and the kernel
will send mgmt device found events with address type information.

So, I suggest applying these two patches and solving the rest of the
issues left in another series.

>
> However keep in mind that we might have to filter these with the changes
> that Johan is doing with triggering scan for random vs public addresses.
>
> Regards
>
> Marcel
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Cheers,
--
Vinicius

2011-11-21 17:24:26

by Andre Guedes

Subject: Re: [PATCH 3/6] Bluetooth: LE scan infra-structure

Hi Marcel,

On Nov 19, 2011, at 3:11 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>>>> This patch adds to hci_core the infra-structure to carry out the
>>>> LE scan. Functions were created to init the LE scan and cancel
>>>> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
>>>>=20
>>>> Signed-off-by: Andre Guedes <[email protected]>
>>>> ---
>>>> include/net/bluetooth/hci_core.h | 5 +++
>>>> net/bluetooth/hci_core.c | 69 =
++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 74 insertions(+), 0 deletions(-)
>>>>=20
>>>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>>>> index f6d5d90..69dda9b 100644
>>>> --- a/include/net/bluetooth/hci_core.h
>>>> +++ b/include/net/bluetooth/hci_core.h
>>>> @@ -232,6 +232,8 @@ struct hci_dev {
>>>> struct list_head adv_entries;
>>>> struct timer_list adv_timer;
>>>>=20
>>>> + struct timer_list le_scan_timer;
>>>> +
>>>> struct hci_dev_stats stat;
>>>>=20
>>>> struct sk_buff_head driver_init;
>>>> @@ -987,5 +989,8 @@ void hci_le_ltk_neg_reply(struct hci_conn =
*conn);
>>>>=20
>>>> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
>>>> int hci_cancel_inquiry(struct hci_dev *hdev);
>>>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, =
u16 window,
>>>> + int =
timeout);
>>>> +int hci_cancel_le_scan(struct hci_dev *hdev);
>>>>=20
>>>> #endif /* __HCI_CORE_H */
>>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>>> index 64cdafe..c07dc15 100644
>>>> --- a/net/bluetooth/hci_core.c
>>>> +++ b/net/bluetooth/hci_core.c
>>>> @@ -1423,6 +1423,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
>>>> return 0;
>>>> }
>>>>=20
>>>> +static int le_scan(struct hci_dev *hdev, u8 enable)
>>>> +{
>>>> + struct hci_cp_le_set_scan_enable cp;
>>>> +
>>>> + memset(&cp, 0, sizeof(cp));
>>>> + cp.enable =3D enable;
>>>> +
>>>> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), =
&cp);
>>>> +}
>>>> +
>>>> +static void le_scan_timeout(unsigned long arg)
>>>> +{
>>>> + struct hci_dev *hdev =3D (void *) arg;
>>>> +
>>>> + le_scan(hdev, 0);
>>>> +}
>>>> +
>>>> /* Register HCI device */
>>>> int hci_register_dev(struct hci_dev *hdev)
>>>> {
>>>> @@ -1501,6 +1518,9 @@ int hci_register_dev(struct hci_dev *hdev)
>>>> setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
>>>> (unsigned long) hdev);
>>>>=20
>>>> + setup_timer(&hdev->le_scan_timer, le_scan_timeout,
>>>> + (unsigned long) hdev);
>>>> +
>>>> INIT_WORK(&hdev->power_on, hci_power_on);
>>>> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>>>>=20
>>>> @@ -1587,6 +1607,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
>>>> hci_del_sysfs(hdev);
>>>>=20
>>>> del_timer(&hdev->adv_timer);
>>>> + del_timer(&hdev->le_scan_timer);
>>>>=20
>>>> destroy_workqueue(hdev->workqueue);
>>>>=20
>>>> @@ -2615,3 +2636,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>>>>=20
>>>> return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>>>> }
>>>> +
>>>> +static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 =
interval,
>>>> + u16 =
window)
>>>> +{
>>>> + struct hci_cp_le_set_scan_param cp;
>>>> +
>>>> + memset(&cp, 0, sizeof(cp));
>>>> + cp.type =3D type;
>>>> + cp.interval =3D cpu_to_le16(interval);
>>>> + cp.window =3D cpu_to_le16(window);
>>>> +
>>>> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), =
&cp);
>>>> +}
>>>> +
>>>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, =
u16 window,
>>>> + int =
timeout)
>>>> +{
>>>> + int err;
>>>> +
>>>> + if (test_bit(HCI_LE_SCAN, &hdev->hci_flags))
>>>> + return -EINPROGRESS;
>>>> +
>>>> + BT_DBG("%s", hdev->name);
>>>> +
>>>> + err =3D set_le_scan_param(hdev, type, interval, window);
>>>> + if (err < 0)
>>>> + return err;
>>>> +
>>>> + err =3D le_scan(hdev, 1);
>>>> + if (err < 0)
>>>> + return err;
>>>=20
>>> since you are using hci_send_cmd, you never check the error from the
>>> controller for set_le_scan_param. We should be doing exactly that =
before
>>> just going ahead with a scan.
>>=20
>> Yes, you're right, there is no error checking at this point.
>>=20
>> I took some time thinking about this problem and I concluded we
>> should not bother so much about it (at least for now). The reasons
>> are:
>>=20
>> 1. The spec says the Set LE Scan Parameters command fails if it
>> is issued when LE scan is enabled. We guarantee this doesn't happen
>> since we check the HCI_LE_SCAN flag before sending any command to
>> the controller.
>>=20
>> 2. Even if the Set LE Scan Parameters command fails for some other
>> unknown reason, we would do the LE scanning with the last parameters
>> set. This doesn't seem to be a big deal.
>>=20
>> 3. We've done lots of tests (with different dongles), but I've not
>> seen this error happening so far. It seems to be difficult to
>> reproduce it.
>>=20
>> I also took some time thinking about a fix for that, but I didn't =
find
>> any easy/clean way to do it.=20
>>=20
>> So I think we should just log if the Set LE Scan Parameters command
>> fails and, if somehow, this becomes often we come up with a fix for =
it.
>=20
> so the proposed fix is to ignore the problem?
>=20
>>> It is also not guaranteed that the controller queues up these =
commands,
>>> it might just return busy from le_scan() if it can have more than =
one
>>> outstanding commands (which many controller can do).
>>=20
>> I didn't follow you here. We have a mechanism to keep track of how
>> many commands the host is allowed to send to the controller. The
>> tasklet hdev->cmd_task only issue a command if the controller is
>> able to handle it.
>=20
> Yes, we do handle that, but we do not handle the controllers radio
> resources. There is no requirement for a controller to queue the =
command
> internally. If it does not have radio resources or is still busy with
> the previous command, it can just return busy.

Ok, I got that. Then, I'll fix this and therefore the Set LE Scan
Parameters command failing case.

>=20
>> Besides, other parts of the code send more than one command in =
sequence
>> and it doesn't seem to be a problem (see set_fast_connectable() in
>> mgmt.c, hci_init_req() in hci_core.c and hci_setup() in hci_event.c).
>=20
> The fast connectable thing is a problem then as well. The init code
> works different here. That is done via hci_request in a context that =
can
> sleep.
>=20
>> But anyway, if the controller returns error from le_scan(), we do the
>> proper handling in hci_cc_le_set_scan_enable().
>=20
> That is not the point here. That is obviously required.
>=20
> Regards
>=20
> Marcel
>=20
>=20

BR,

Andre

2011-11-19 06:11:29

by Marcel Holtmann

Subject: Re: [PATCH 3/6] Bluetooth: LE scan infra-structure

Hi Andre,

> >> This patch adds to hci_core the infra-structure to carry out the
> >> LE scan. Functions were created to init the LE scan and cancel
> >> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
> >>
> >> Signed-off-by: Andre Guedes <[email protected]>
> >> ---
> >> include/net/bluetooth/hci_core.h | 5 +++
> >> net/bluetooth/hci_core.c | 69 ++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 74 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index f6d5d90..69dda9b 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -232,6 +232,8 @@ struct hci_dev {
> >> struct list_head adv_entries;
> >> struct timer_list adv_timer;
> >>
> >> + struct timer_list le_scan_timer;
> >> +
> >> struct hci_dev_stats stat;
> >>
> >> struct sk_buff_head driver_init;
> >> @@ -987,5 +989,8 @@ void hci_le_ltk_neg_reply(struct hci_conn *conn);
> >>
> >> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
> >> int hci_cancel_inquiry(struct hci_dev *hdev);
> >> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> >> + int timeout);
> >> +int hci_cancel_le_scan(struct hci_dev *hdev);
> >>
> >> #endif /* __HCI_CORE_H */
> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> index 64cdafe..c07dc15 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -1423,6 +1423,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
> >> return 0;
> >> }
> >>
> >> +static int le_scan(struct hci_dev *hdev, u8 enable)
> >> +{
> >> + struct hci_cp_le_set_scan_enable cp;
> >> +
> >> + memset(&cp, 0, sizeof(cp));
> >> + cp.enable = enable;
> >> +
> >> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> >> +}
> >> +
> >> +static void le_scan_timeout(unsigned long arg)
> >> +{
> >> + struct hci_dev *hdev = (void *) arg;
> >> +
> >> + le_scan(hdev, 0);
> >> +}
> >> +
> >> /* Register HCI device */
> >> int hci_register_dev(struct hci_dev *hdev)
> >> {
> >> @@ -1501,6 +1518,9 @@ int hci_register_dev(struct hci_dev *hdev)
> >> setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
> >> (unsigned long) hdev);
> >>
> >> + setup_timer(&hdev->le_scan_timer, le_scan_timeout,
> >> + (unsigned long) hdev);
> >> +
> >> INIT_WORK(&hdev->power_on, hci_power_on);
> >> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
> >>
> >> @@ -1587,6 +1607,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
> >> hci_del_sysfs(hdev);
> >>
> >> del_timer(&hdev->adv_timer);
> >> + del_timer(&hdev->le_scan_timer);
> >>
> >> destroy_workqueue(hdev->workqueue);
> >>
> >> @@ -2615,3 +2636,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
> >>
> >> return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> >> }
> >> +
> >> +static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 interval,
> >> + u16 window)
> >> +{
> >> + struct hci_cp_le_set_scan_param cp;
> >> +
> >> + memset(&cp, 0, sizeof(cp));
> >> + cp.type = type;
> >> + cp.interval = cpu_to_le16(interval);
> >> + cp.window = cpu_to_le16(window);
> >> +
> >> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
> >> +}
> >> +
> >> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> >> + int timeout)
> >> +{
> >> + int err;
> >> +
> >> + if (test_bit(HCI_LE_SCAN, &hdev->hci_flags))
> >> + return -EINPROGRESS;
> >> +
> >> + BT_DBG("%s", hdev->name);
> >> +
> >> + err = set_le_scan_param(hdev, type, interval, window);
> >> + if (err < 0)
> >> + return err;
> >> +
> >> + err = le_scan(hdev, 1);
> >> + if (err < 0)
> >> + return err;
> >
> > since you are using hci_send_cmd, you never check the error from the
> > controller for set_le_scan_param. We should be doing exactly that before
> > just going ahead with a scan.
>
> Yes, you're right, there is no error checking at this point.
>
> I took some time thinking about this problem and I concluded we
> should not bother so much about it (at least for now). The reasons
> are:
>
> 1. The spec says the Set LE Scan Parameters command fails if it
> is issued when LE scan is enabled. We guarantee this doesn't happen
> since we check the HCI_LE_SCAN flag before sending any command to
> the controller.
>
> 2. Even if the Set LE Scan Parameters command fails for some other
> unknown reason, we would do the LE scanning with the last parameters
> set. This doesn't seem to be a big deal.
>
> 3. We've done lots of tests (with different dongles), but I've not
> seen this error happening so far. It seems to be difficult to
> reproduce it.
>
> I also took some time thinking about a fix for that, but I didn't find
> any easy/clean way to do it.
>
> So I think we should just log if the Set LE Scan Parameters command
> fails and, if somehow, this becomes often we come up with a fix for it.

so the proposed fix is to ignore the problem?

> > It is also not guaranteed that the controller queues up these commands,
> > it might just return busy from le_scan() if it can have more than one
> > outstanding commands (which many controller can do).
>
> I didn't follow you here. We have a mechanism to keep track of how
> many commands the host is allowed to send to the controller. The
> tasklet hdev->cmd_task only issue a command if the controller is
> able to handle it.

Yes, we do handle that, but we do not handle the controllers radio
resources. There is no requirement for a controller to queue the command
internally. If it does not have radio resources or is still busy with
the previous command, it can just return busy.

> Besides, other parts of the code send more than one command in sequence
> and it doesn't seem to be a problem (see set_fast_connectable() in
> mgmt.c, hci_init_req() in hci_core.c and hci_setup() in hci_event.c).

The fast connectable thing is a problem then as well. The init code
works different here. That is done via hci_request in a context that can
sleep.

> But anyway, if the controller returns error from le_scan(), we do the
> proper handling in hci_cc_le_set_scan_enable().

That is not the point here. That is obviously required.

Regards

Marcel

2011-11-18 23:04:56

by Andre Guedes

Subject: Re: [PATCH 3/6] Bluetooth: LE scan infra-structure

Hi Marcel,

On Nov 11, 2011, at 8:13 PM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch adds to hci_core the infra-structure to carry out the
>> LE scan. Functions were created to init the LE scan and cancel
>> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
>>=20
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> include/net/bluetooth/hci_core.h | 5 +++
>> net/bluetooth/hci_core.c | 69 =
++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 74 insertions(+), 0 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index f6d5d90..69dda9b 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -232,6 +232,8 @@ struct hci_dev {
>> struct list_head adv_entries;
>> struct timer_list adv_timer;
>>=20
>> + struct timer_list le_scan_timer;
>> +
>> struct hci_dev_stats stat;
>>=20
>> struct sk_buff_head driver_init;
>> @@ -987,5 +989,8 @@ void hci_le_ltk_neg_reply(struct hci_conn *conn);
>>=20
>> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
>> int hci_cancel_inquiry(struct hci_dev *hdev);
>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 =
window,
>> + int =
timeout);
>> +int hci_cancel_le_scan(struct hci_dev *hdev);
>>=20
>> #endif /* __HCI_CORE_H */
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 64cdafe..c07dc15 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1423,6 +1423,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
>> return 0;
>> }
>>=20
>> +static int le_scan(struct hci_dev *hdev, u8 enable)
>> +{
>> + struct hci_cp_le_set_scan_enable cp;
>> +
>> + memset(&cp, 0, sizeof(cp));
>> + cp.enable =3D enable;
>> +
>> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), =
&cp);
>> +}
>> +
>> +static void le_scan_timeout(unsigned long arg)
>> +{
>> + struct hci_dev *hdev =3D (void *) arg;
>> +
>> + le_scan(hdev, 0);
>> +}
>> +
>> /* Register HCI device */
>> int hci_register_dev(struct hci_dev *hdev)
>> {
>> @@ -1501,6 +1518,9 @@ int hci_register_dev(struct hci_dev *hdev)
>> setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
>> (unsigned long) hdev);
>>=20
>> + setup_timer(&hdev->le_scan_timer, le_scan_timeout,
>> + (unsigned long) hdev);
>> +
>> INIT_WORK(&hdev->power_on, hci_power_on);
>> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>>=20
>> @@ -1587,6 +1607,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
>> hci_del_sysfs(hdev);
>>=20
>> del_timer(&hdev->adv_timer);
>> + del_timer(&hdev->le_scan_timer);
>>=20
>> destroy_workqueue(hdev->workqueue);
>>=20
>> @@ -2615,3 +2636,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>>=20
>> return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>> }
>> +
>> +static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 =
interval,
>> + u16 =
window)
>> +{
>> + struct hci_cp_le_set_scan_param cp;
>> +
>> + memset(&cp, 0, sizeof(cp));
>> + cp.type =3D type;
>> + cp.interval =3D cpu_to_le16(interval);
>> + cp.window =3D cpu_to_le16(window);
>> +
>> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), =
&cp);
>> +}
>> +
>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 =
window,
>> + int =
timeout)
>> +{
>> + int err;
>> +
>> + if (test_bit(HCI_LE_SCAN, &hdev->hci_flags))
>> + return -EINPROGRESS;
>> +
>> + BT_DBG("%s", hdev->name);
>> +
>> + err =3D set_le_scan_param(hdev, type, interval, window);
>> + if (err < 0)
>> + return err;
>> +
>> + err =3D le_scan(hdev, 1);
>> + if (err < 0)
>> + return err;
>=20
> since you are using hci_send_cmd, you never check the error from the
> controller for set_le_scan_param. We should be doing exactly that =
before
> just going ahead with a scan.

Yes, you're right, there is no error checking at this point.

I took some time thinking about this problem and I concluded we
should not bother so much about it (at least for now). The reasons
are:

1. The spec says the Set LE Scan Parameters command fails if it
is issued when LE scan is enabled. We guarantee this doesn't happen
since we check the HCI_LE_SCAN flag before sending any command to
the controller.

2. Even if the Set LE Scan Parameters command fails for some other
unknown reason, we would do the LE scanning with the last parameters
set. This doesn't seem to be a big deal.

3. We've done lots of tests (with different dongles), but I've not
seen this error happening so far. It seems to be difficult to
reproduce it.

I also took some time thinking about a fix for that, but I didn't find
any easy/clean way to do it.=20

So I think we should just log if the Set LE Scan Parameters command
fails and, if somehow, this becomes often we come up with a fix for it.

> It is also not guaranteed that the controller queues up these =
commands,
> it might just return busy from le_scan() if it can have more than one
> outstanding commands (which many controller can do).

I didn't follow you here. We have a mechanism to keep track of how
many commands the host is allowed to send to the controller. The
tasklet hdev->cmd_task only issue a command if the controller is
able to handle it.

Besides, other parts of the code send more than one command in sequence
and it doesn't seem to be a problem (see set_fast_connectable() in
mgmt.c, hci_init_req() in hci_core.c and hci_setup() in hci_event.c).

But anyway, if the controller returns error from le_scan(), we do the
proper handling in hci_cc_le_set_scan_enable().

BR,

Andre

2011-11-17 16:40:58

by Andre Guedes

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Andrei,

On Nov 17, 2011, at 5:59 AM, Andrei Emeltchenko wrote:

> Hi Andre,
>
> On Wed, Nov 16, 2011 at 05:36:38PM -0300, Andre Guedes wrote:
>
> ...
>
>>>>
>>>> if (cp->enable == 0x01) {
>>>
>>> Can we fix also magic number here and below?
>>
>> I see your point here, but this is not a magic number, it is
>> just a true/false value.
>
> If this true/false no need to compare with hex numbers why not
>
> if(cp->enable)
> ...

Yes, sure, it could be done like that. I might do it in a later
patch so.

Thanks,

Andre

2011-11-17 08:59:07

by Andrei Emeltchenko

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Andre,

On Wed, Nov 16, 2011 at 05:36:38PM -0300, Andre Guedes wrote:

...

> >>
> >> if (cp->enable == 0x01) {
> >
> > Can we fix also magic number here and below?
>
> I see your point here, but this is not a magic number, it is
> just a true/false value.

If this true/false no need to compare with hex numbers why not

if(cp->enable)
...

Best regards
Andrei Emeltchenko

2011-11-17 00:59:38

by Marcel Holtmann

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Andre,

> >>> And here, I have a serious problem with how the code is done. I realize
> >>> that from using hdev->flags this ends up this crappy, but this is not
> >>> how I wanna see things done.
> >>>
> >>> What you are doing is this:
> >>>
> >>> - We call a complete unrelated function anyway
> >>> - And if it fails with a specific error code then we call something
> >>> else instead
> >>>
> >>> I think it becomes pretty obvious now that we should just have had
> >>> hdev->mgmt_flags that tell us with discovery procedure is running right
> >>> now. And thus we know how to cancel it.
> >>
> >> I'm not sure this will help us in case we have a interleaved discovery
> >> running. My point is, even if we know what discovery procedure is running,
> >> we need to know what the controller is doing right now (inquiring or le
> >> scanning) so we can properly stop it, and, therefore, stop the ongoing
> >> discovery procedure. IOW, telling us we have a interleaved discovery
> >> running does not help us to decide the right function to call
> >> (hci_cancel_inquiry or hci_cancel_le_scan).
> >>
> >> So, I think we need to check the controller flags (HCI_INQUIRY and
> >> HCI_LE_SCAN) in order to stop the ongoing discovery procedure properly.
> >
> > I have nothing against separate flags. That makes fully sense. I have
> > something against weirdly calling one function and expecting it to error
> > out. Relying on an error is a bad idea. You want to keep track of what
> > is currently going on.
>
> Ok, I agree with that too.
>
> The way I see to fix that is we have something like we had before:
>
> if (HCI_INQUIRY)
> hci_cancel_inquiry()
> else if (HCI_LE_SCAN)
> hci_cancel_le_scan()
>
> The drawback, as you already pointed, is the double check of these
> flags.

there is nothing you can do about it actually. However with the changes
we are doing, the flags are internal. That is really important to me to
be able to move over to a proper userspace side API in the future that
does not rely on a magic bitmask.

And also it is in one place here so people can easily understand what
code is run with what dependency.

> But, as I said before, the flags checking in stop_discovery is to
> decide the right cancel helper function to call. The flag checking
> in hci_cancel_*() helper functions guarantees no cancel command is
> sent to the controller if there is no ongoing inquiry or le scan.
> The flag checking in stop_discovery() and hci_cancel_*() have
> different purposes.
>
> Since hci_cancel_*() are helper functions and they can be reused in
> future by other parts of the code, I think it is a good idea we keep
> the flag checking internally. This way we don't rely on the programmer
> doing the proper checking before calling these helper functions.

I lost you here. I need to see this in code.

Regards

Marcel

2011-11-16 22:41:06

by Andre Guedes

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Marcel,

On Nov 16, 2011, at 6:45 PM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>>>> Signed-off-by: Andre Guedes <[email protected]>
>>>> ---
>>>> include/net/bluetooth/hci.h | 1 +
>>>> include/net/bluetooth/hci_core.h | 1 +
>>>> net/bluetooth/hci_event.c | 25 ++++++++++++++++++++++---
>>>> net/bluetooth/mgmt.c | 25 +++++++++++++++++++++++--
>>>> 4 files changed, 47 insertions(+), 5 deletions(-)
>>>>=20
>>>> diff --git a/include/net/bluetooth/hci.h =
b/include/net/bluetooth/hci.h
>>>> index bd3cecd..ca09998 100644
>>>> --- a/include/net/bluetooth/hci.h
>>>> +++ b/include/net/bluetooth/hci.h
>>>> @@ -210,6 +210,7 @@ enum {
>>>>=20
>>>> #define LMP_EV4 0x01
>>>> #define LMP_EV5 0x02
>>>> +#define LMP_NO_BREDR 0x20
>>>> #define LMP_LE 0x40
>>>>=20
>>>> #define LMP_SNIFF_SUBR 0x02
>>>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>>>> index 9da5b69..55b78ec 100644
>>>> --- a/include/net/bluetooth/hci_core.h
>>>> +++ b/include/net/bluetooth/hci_core.h
>>>> @@ -678,6 +678,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>>>> #define lmp_ssp_capable(dev) ((dev)->features[6] & =
LMP_SIMPLE_PAIR)
>>>> #define lmp_no_flush_capable(dev) ((dev)->features[6] & =
LMP_NO_FLUSH)
>>>> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
>>>> +#define lmp_bredr_capable(dev) (!((dev)->features[4] & =
LMP_NO_BREDR))
>>>=20
>>> can you just split this into a separate patch first. We can just go
>>> ahead and merge it.
>>>=20
>>>> /* ----- Extended LMP capabilities ----- */
>>>> #define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & =
LMP_HOST_LE)
>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>> index dcbbffe..037c7c0 100644
>>>> --- a/net/bluetooth/hci_event.c
>>>> +++ b/net/bluetooth/hci_event.c
>>>> @@ -954,24 +954,43 @@ static void hci_cc_le_set_scan_enable(struct =
hci_dev *hdev,
>>>>=20
>>>> BT_DBG("%s status 0x%x", hdev->name, status);
>>>>=20
>>>> - if (status)
>>>> - return;
>>>> -
>>>> cp =3D hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
>>>> if (!cp)
>>>> return;
>>>>=20
>>>> if (cp->enable =3D=3D 0x01) {
>>>> + if (status) {
>>>> + hci_dev_lock(hdev);
>>>> + mgmt_start_discovery_failed(hdev, status);
>>>> + hci_dev_unlock(hdev);
>>>> + return;
>>>> + }
>>>> +
>>>> set_bit(HCI_LE_SCAN, &hdev->hci_flags);
>>>>=20
>>>> del_timer(&hdev->adv_timer);
>>>>=20
>>>> hci_dev_lock(hdev);
>>>> +
>>>> + mgmt_discovering(hdev, 1);
>>>> +
>>>> hci_adv_entries_clear(hdev);
>>>> +
>>>> hci_dev_unlock(hdev);
>>>> } else if (cp->enable =3D=3D 0x00) {
>>>> + if (status) {
>>>> + hci_dev_lock(hdev);
>>>> + mgmt_stop_discovery_failed(hdev, status);
>>>> + hci_dev_unlock(hdev);
>>>> + return;
>>>> + }
>>>> +
>>>> clear_bit(HCI_LE_SCAN, &hdev->hci_flags);
>>>>=20
>>>> + hci_dev_lock(hdev);
>>>> + mgmt_discovering(hdev, 0);
>>>> + hci_dev_unlock(hdev);
>>>> +
>>>> mod_timer(&hdev->adv_timer, jiffies + =
ADV_CLEAR_TIMEOUT);
>>>> }
>>>> }
>>>=20
>>> This part looks fine to me.
>>>=20
>>>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>>> index b63a7d0..6ca6e5d 100644
>>>> --- a/net/bluetooth/mgmt.c
>>>> +++ b/net/bluetooth/mgmt.c
>>>> @@ -32,7 +32,16 @@
>>>> #define MGMT_VERSION 0
>>>> #define MGMT_REVISION 1
>>>>=20
>>>> -#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>>>> +/*
>>>> + * These LE scan and inquiry parameters were chosen according to =
LE General
>>>> + * Discovery Procedure specification.
>>>> + */
>>>> +#define LE_SCAN_TYPE 0x01
>>>> +#define LE_SCAN_WIN 0x12
>>>> +#define LE_SCAN_INT 0x12
>>>> +#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* =
TGAP(gen_disc_scan_min) */
>>>> +
>>>> +#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>>>>=20
>>>> struct pending_cmd {
>>>> struct list_head list;
>>>> @@ -1669,7 +1678,16 @@ static int start_discovery(struct sock *sk, =
u16 index)
>>>> goto failed;
>>>> }
>>>>=20
>>>> - err =3D hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>>>> + if (lmp_host_le_capable(hdev)) {
>>>> + if (lmp_bredr_capable(hdev))
>>>> + err =3D -ENOSYS;
>>>> + else
>>>> + err =3D hci_do_le_scan(hdev, LE_SCAN_TYPE, =
LE_SCAN_INT,
>>>> + LE_SCAN_WIN, =
LE_SCAN_TIMEOUT_LE_ONLY);
>>>> + } else {
>>>> + err =3D hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>>>> + }
>>>> +
>>>> if (err < 0)
>>>> mgmt_pending_remove(cmd);
>>>>=20
>>>> @@ -1701,6 +1719,9 @@ static int stop_discovery(struct sock *sk, =
u16 index)
>>>> }
>>>>=20
>>>> err =3D hci_cancel_inquiry(hdev);
>>>> + if (err =3D=3D -EPERM)
>>>> + err =3D hci_cancel_le_scan(hdev);
>>>> +
>>>=20
>>> And here, I have a serious problem with how the code is done. I =
realize
>>> that from using hdev->flags this ends up this crappy, but this is =
not
>>> how I wanna see things done.
>>>=20
>>> What you are doing is this:
>>>=20
>>> - We call a complete unrelated function anyway
>>> - And if it fails with a specific error code then we call something
>>> else instead
>>>=20
>>> I think it becomes pretty obvious now that we should just have had
>>> hdev->mgmt_flags that tell us with discovery procedure is running =
right
>>> now. And thus we know how to cancel it.
>>=20
>> I'm not sure this will help us in case we have a interleaved =
discovery
>> running. My point is, even if we know what discovery procedure is =
running,
>> we need to know what the controller is doing right now (inquiring or =
le
>> scanning) so we can properly stop it, and, therefore, stop the =
ongoing
>> discovery procedure. IOW, telling us we have a interleaved discovery
>> running does not help us to decide the right function to call
>> (hci_cancel_inquiry or hci_cancel_le_scan).
>>=20
>> So, I think we need to check the controller flags (HCI_INQUIRY and
>> HCI_LE_SCAN) in order to stop the ongoing discovery procedure =
properly.
>=20
> I have nothing against separate flags. That makes fully sense. I have
> something against weirdly calling one function and expecting it to =
error
> out. Relying on an error is a bad idea. You want to keep track of what
> is currently going on.

Ok, I agree with that too.

The way I see to fix that is we have something like we had before:

if (HCI_INQUIRY)
hci_cancel_inquiry()
else if (HCI_LE_SCAN)
hci_cancel_le_scan()

The drawback, as you already pointed, is the double check of these
flags.

But, as I said before, the flags checking in stop_discovery is to
decide the right cancel helper function to call. The flag checking
in hci_cancel_*() helper functions guarantees no cancel command is
sent to the controller if there is no ongoing inquiry or le scan.
The flag checking in stop_discovery() and hci_cancel_*() have
different purposes.

Since hci_cancel_*() are helper functions and they can be reused in
future by other parts of the code, I think it is a good idea we keep
the flag checking internally. This way we don't rely on the programmer
doing the proper checking before calling these helper functions.

BR,

Andre

2011-11-16 21:50:24

by Andre Guedes

Subject: Re: [PATCH 1/6] Bluetooth: Add hci_flags to struct hci_dev

Hi Marcel,

On Nov 16, 2011, at 6:44 PM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>>>> This patch adds the hci_flags field to struct hci_dev. This new
>>>> flags variable should be used to define flags related to BR/EDR
>>>> and/or LE controller itself. It should be used to define flags
>>>> which represents states from the controller. The hci_flags is
>>>> cleared in case the controller sends a Reset Command Complete
>>>> Event to the host.
>>>>=20
>>>> Also, this patch adds the HCI_LE_SCAN flag which was created to
>>>> track if the controller is performing LE scan or not. The flag
>>>> is set/cleared when the controller starts/stops scanning.
>>>>=20
>>>> This is an initial effort to stop using hdev->flags to define
>>>> internal flags since it is exported to userspace by an ioctl.
>>>>=20
>>>> Signed-off-by: Andre Guedes <[email protected]>
>>>> ---
>>>> include/net/bluetooth/hci.h | 8 ++++++++
>>>> include/net/bluetooth/hci_core.h | 2 ++
>>>> net/bluetooth/hci_core.c | 1 +
>>>> net/bluetooth/hci_event.c | 6 ++++++
>>>> 4 files changed, 17 insertions(+), 0 deletions(-)
>>>>=20
>>>> diff --git a/include/net/bluetooth/hci.h =
b/include/net/bluetooth/hci.h
>>>> index 139ce2a..70321a1 100644
>>>> --- a/include/net/bluetooth/hci.h
>>>> +++ b/include/net/bluetooth/hci.h
>>>> @@ -88,6 +88,14 @@ enum {
>>>> HCI_RESET,
>>>> };
>>>>=20
>>>> +/*
>>>> + * BR/EDR and/or LE controller flags: the flags defined here =
should represent
>>>> + * states from the controller.
>>>> + */
>>>> +enum {
>>>> + HCI_LE_SCAN,
>>>> +};
>>>> +
>>>> /* HCI ioctl defines */
>>>> #define HCIDEVUP _IOW('H', 201, int)
>>>> #define HCIDEVDOWN _IOW('H', 202, int)
>>>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>>>> index 1795257..f6d5d90 100644
>>>> --- a/include/net/bluetooth/hci_core.h
>>>> +++ b/include/net/bluetooth/hci_core.h
>>>> @@ -250,6 +250,8 @@ struct hci_dev {
>>>>=20
>>>> struct module *owner;
>>>>=20
>>>> + unsigned long hci_flags;
>>>> +
>>>=20
>>> so I remember that I said, we call these mgmt_flags and make sure =
that
>>> all the flags are bound the mgmt interface. Why are we calling this
>>> hci_flags now?
>>=20
>> I realized this flags variable is more related to the controller
>> itself than to management interface. For instance, HCI_LE_SCAN,
>> HCI_INQUIRY, HCI_PSCAN, HCI_ISCAN and others flags pretty much
>> related to the _controller_. Additionally, the PINQUIRY flag, which
>> we might add soon, might be defined in hci_flags too, since it is
>> related to the controller.
>>=20
>> About the mgmt_flags, I was thinking in using this flags variable
>> to define management interface related flags. Flags such as
>> HCI_MGMT, HCI_PAIRABLE, HCI_SERVICE_CACHE and HCI_LINK_KEYS could
>> be added to mgmt_flags since they are all related to management
>> interface itself. In a patch in interleaved discovery support series
>> (as I said, I'll send it to ML soon), I create the mgmt_flags =
variable
>> and define the MGMT_DISCOV flags to track if we are carrying out a
>> discovery or not.=20
>>=20
>> So, summarizing we would have two flags variables: hci_flags (which
>> holds flags related to the controller) and mgmt_flags (which holds
>> flags related to management interface).
>>=20
>> Do we keep hci_flags or rename it to mgmt_flags and mix up controller
>> and management interface flags?
>=20
> then just call them dev_flags (like we have dev_type). Prefixing =
things
> with hci_ seems wrong to me.

Ok, I'll rename it.

Andre

2011-11-16 21:45:38

by Marcel Holtmann

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Andre,

> >> Signed-off-by: Andre Guedes <[email protected]>
> >> ---
> >> include/net/bluetooth/hci.h | 1 +
> >> include/net/bluetooth/hci_core.h | 1 +
> >> net/bluetooth/hci_event.c | 25 ++++++++++++++++++++++---
> >> net/bluetooth/mgmt.c | 25 +++++++++++++++++++++++--
> >> 4 files changed, 47 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> >> index bd3cecd..ca09998 100644
> >> --- a/include/net/bluetooth/hci.h
> >> +++ b/include/net/bluetooth/hci.h
> >> @@ -210,6 +210,7 @@ enum {
> >>
> >> #define LMP_EV4 0x01
> >> #define LMP_EV5 0x02
> >> +#define LMP_NO_BREDR 0x20
> >> #define LMP_LE 0x40
> >>
> >> #define LMP_SNIFF_SUBR 0x02
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index 9da5b69..55b78ec 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -678,6 +678,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> >> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
> >> #define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
> >> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
> >> +#define lmp_bredr_capable(dev) (!((dev)->features[4] & LMP_NO_BREDR))
> >
> > can you just split this into a separate patch first. We can just go
> > ahead and merge it.
> >
> >> /* ----- Extended LMP capabilities ----- */
> >> #define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & LMP_HOST_LE)
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index dcbbffe..037c7c0 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -954,24 +954,43 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> >>
> >> BT_DBG("%s status 0x%x", hdev->name, status);
> >>
> >> - if (status)
> >> - return;
> >> -
> >> cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
> >> if (!cp)
> >> return;
> >>
> >> if (cp->enable == 0x01) {
> >> + if (status) {
> >> + hci_dev_lock(hdev);
> >> + mgmt_start_discovery_failed(hdev, status);
> >> + hci_dev_unlock(hdev);
> >> + return;
> >> + }
> >> +
> >> set_bit(HCI_LE_SCAN, &hdev->hci_flags);
> >>
> >> del_timer(&hdev->adv_timer);
> >>
> >> hci_dev_lock(hdev);
> >> +
> >> + mgmt_discovering(hdev, 1);
> >> +
> >> hci_adv_entries_clear(hdev);
> >> +
> >> hci_dev_unlock(hdev);
> >> } else if (cp->enable == 0x00) {
> >> + if (status) {
> >> + hci_dev_lock(hdev);
> >> + mgmt_stop_discovery_failed(hdev, status);
> >> + hci_dev_unlock(hdev);
> >> + return;
> >> + }
> >> +
> >> clear_bit(HCI_LE_SCAN, &hdev->hci_flags);
> >>
> >> + hci_dev_lock(hdev);
> >> + mgmt_discovering(hdev, 0);
> >> + hci_dev_unlock(hdev);
> >> +
> >> mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
> >> }
> >> }
> >
> > This part looks fine to me.
> >
> >> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >> index b63a7d0..6ca6e5d 100644
> >> --- a/net/bluetooth/mgmt.c
> >> +++ b/net/bluetooth/mgmt.c
> >> @@ -32,7 +32,16 @@
> >> #define MGMT_VERSION 0
> >> #define MGMT_REVISION 1
> >>
> >> -#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
> >> +/*
> >> + * These LE scan and inquiry parameters were chosen according to LE General
> >> + * Discovery Procedure specification.
> >> + */
> >> +#define LE_SCAN_TYPE 0x01
> >> +#define LE_SCAN_WIN 0x12
> >> +#define LE_SCAN_INT 0x12
> >> +#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
> >> +
> >> +#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
> >>
> >> struct pending_cmd {
> >> struct list_head list;
> >> @@ -1669,7 +1678,16 @@ static int start_discovery(struct sock *sk, u16 index)
> >> goto failed;
> >> }
> >>
> >> - err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> >> + if (lmp_host_le_capable(hdev)) {
> >> + if (lmp_bredr_capable(hdev))
> >> + err = -ENOSYS;
> >> + else
> >> + err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> >> + LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
> >> + } else {
> >> + err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> >> + }
> >> +
> >> if (err < 0)
> >> mgmt_pending_remove(cmd);
> >>
> >> @@ -1701,6 +1719,9 @@ static int stop_discovery(struct sock *sk, u16 index)
> >> }
> >>
> >> err = hci_cancel_inquiry(hdev);
> >> + if (err == -EPERM)
> >> + err = hci_cancel_le_scan(hdev);
> >> +
> >
> > And here, I have a serious problem with how the code is done. I realize
> > that from using hdev->flags this ends up this crappy, but this is not
> > how I wanna see things done.
> >
> > What you are doing is this:
> >
> > - We call a complete unrelated function anyway
> > - And if it fails with a specific error code then we call something
> > else instead
> >
> > I think it becomes pretty obvious now that we should just have had
> > hdev->mgmt_flags that tell us with discovery procedure is running right
> > now. And thus we know how to cancel it.
>
> I'm not sure this will help us in case we have a interleaved discovery
> running. My point is, even if we know what discovery procedure is running,
> we need to know what the controller is doing right now (inquiring or le
> scanning) so we can properly stop it, and, therefore, stop the ongoing
> discovery procedure. IOW, telling us we have a interleaved discovery
> running does not help us to decide the right function to call
> (hci_cancel_inquiry or hci_cancel_le_scan).
>
> So, I think we need to check the controller flags (HCI_INQUIRY and
> HCI_LE_SCAN) in order to stop the ongoing discovery procedure properly.

I have nothing against separate flags. That makes fully sense. I have
something against weirdly calling one function and expecting it to error
out. Relying on an error is a bad idea. You want to keep track of what
is currently going on.

Regards

Marcel

2011-11-16 21:44:26

by Marcel Holtmann

Subject: Re: [PATCH 1/6] Bluetooth: Add hci_flags to struct hci_dev

Hi Andre,

> >> This patch adds the hci_flags field to struct hci_dev. This new
> >> flags variable should be used to define flags related to BR/EDR
> >> and/or LE controller itself. It should be used to define flags
> >> which represents states from the controller. The hci_flags is
> >> cleared in case the controller sends a Reset Command Complete
> >> Event to the host.
> >>
> >> Also, this patch adds the HCI_LE_SCAN flag which was created to
> >> track if the controller is performing LE scan or not. The flag
> >> is set/cleared when the controller starts/stops scanning.
> >>
> >> This is an initial effort to stop using hdev->flags to define
> >> internal flags since it is exported to userspace by an ioctl.
> >>
> >> Signed-off-by: Andre Guedes <[email protected]>
> >> ---
> >> include/net/bluetooth/hci.h | 8 ++++++++
> >> include/net/bluetooth/hci_core.h | 2 ++
> >> net/bluetooth/hci_core.c | 1 +
> >> net/bluetooth/hci_event.c | 6 ++++++
> >> 4 files changed, 17 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> >> index 139ce2a..70321a1 100644
> >> --- a/include/net/bluetooth/hci.h
> >> +++ b/include/net/bluetooth/hci.h
> >> @@ -88,6 +88,14 @@ enum {
> >> HCI_RESET,
> >> };
> >>
> >> +/*
> >> + * BR/EDR and/or LE controller flags: the flags defined here should represent
> >> + * states from the controller.
> >> + */
> >> +enum {
> >> + HCI_LE_SCAN,
> >> +};
> >> +
> >> /* HCI ioctl defines */
> >> #define HCIDEVUP _IOW('H', 201, int)
> >> #define HCIDEVDOWN _IOW('H', 202, int)
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index 1795257..f6d5d90 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -250,6 +250,8 @@ struct hci_dev {
> >>
> >> struct module *owner;
> >>
> >> + unsigned long hci_flags;
> >> +
> >
> > so I remember that I said, we call these mgmt_flags and make sure that
> > all the flags are bound the mgmt interface. Why are we calling this
> > hci_flags now?
>
> I realized this flags variable is more related to the controller
> itself than to management interface. For instance, HCI_LE_SCAN,
> HCI_INQUIRY, HCI_PSCAN, HCI_ISCAN and others flags pretty much
> related to the _controller_. Additionally, the PINQUIRY flag, which
> we might add soon, might be defined in hci_flags too, since it is
> related to the controller.
>
> About the mgmt_flags, I was thinking in using this flags variable
> to define management interface related flags. Flags such as
> HCI_MGMT, HCI_PAIRABLE, HCI_SERVICE_CACHE and HCI_LINK_KEYS could
> be added to mgmt_flags since they are all related to management
> interface itself. In a patch in interleaved discovery support series
> (as I said, I'll send it to ML soon), I create the mgmt_flags variable
> and define the MGMT_DISCOV flags to track if we are carrying out a
> discovery or not.
>
> So, summarizing we would have two flags variables: hci_flags (which
> holds flags related to the controller) and mgmt_flags (which holds
> flags related to management interface).
>
> Do we keep hci_flags or rename it to mgmt_flags and mix up controller
> and management interface flags?

then just call them dev_flags (like we have dev_type). Prefixing things
with hci_ seems wrong to me.

Regards

Marcel

2011-11-16 21:04:01

by Andre Guedes

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Johan,

On Nov 12, 2011, at 6:54 AM, Johan Hedberg wrote:

> Hi Andre,
>
> On Fri, Nov 11, 2011, Andre Guedes wrote:
>> @@ -1669,7 +1678,16 @@ static int start_discovery(struct sock *sk, u16 index)
>> goto failed;
>> }
>>
>> - err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>> + if (lmp_host_le_capable(hdev)) {
>> + if (lmp_bredr_capable(hdev))
>> + err = -ENOSYS;
>> + else
>> + err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
>> + LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
>> + } else {
>> + err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>> + }
>
> This should really be looking at type parameter that gets passed in the
> mgmt_start_discovery command. User space has been sending it already for
> some time but the kernel side wasn't yet updated to care about it. I
> just sent a patch to add the necessary mgmt.h struct and to pass the
> data to the start_discovery function.

The bluetooth-next tree didn't have those patches you are talking about
when I sent this series.

I'll consider the type parameter in v2. Thanks.

Andre

2011-11-16 20:36:38

by Andre Guedes

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Andrei,

On Nov 14, 2011, at 7:08 AM, Andrei Emeltchenko wrote:

> Hi Andre,
>
> On Fri, Nov 11, 2011 at 07:50:24PM -0300, Andre Guedes wrote:
>> This patch adds support for LE-Only discovery procedure through
>> management interface.
>>
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> include/net/bluetooth/hci.h | 1 +
>> include/net/bluetooth/hci_core.h | 1 +
>> net/bluetooth/hci_event.c | 25 ++++++++++++++++++++++---
>> net/bluetooth/mgmt.c | 25 +++++++++++++++++++++++--
>> 4 files changed, 47 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index bd3cecd..ca09998 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -210,6 +210,7 @@ enum {
>>
>> #define LMP_EV4 0x01
>> #define LMP_EV5 0x02
>> +#define LMP_NO_BREDR 0x20
>> #define LMP_LE 0x40
>>
>> #define LMP_SNIFF_SUBR 0x02
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 9da5b69..55b78ec 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -678,6 +678,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
>> #define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
>> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
>> +#define lmp_bredr_capable(dev) (!((dev)->features[4] & LMP_NO_BREDR))
>>
>> /* ----- Extended LMP capabilities ----- */
>> #define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & LMP_HOST_LE)
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index dcbbffe..037c7c0 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -954,24 +954,43 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>>
>> BT_DBG("%s status 0x%x", hdev->name, status);
>>
>> - if (status)
>> - return;
>> -
>> cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
>> if (!cp)
>> return;
>>
>> if (cp->enable == 0x01) {
>
> Can we fix also magic number here and below?

I see your point here, but this is not a magic number, it is
just a true/false value.

BR,

Andre

2011-11-16 20:25:00

by Andre Guedes

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Marcel,

On Nov 12, 2011, at 3:43 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch adds support for LE-Only discovery procedure through
>> management interface.
>>=20
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> include/net/bluetooth/hci.h | 1 +
>> include/net/bluetooth/hci_core.h | 1 +
>> net/bluetooth/hci_event.c | 25 ++++++++++++++++++++++---
>> net/bluetooth/mgmt.c | 25 +++++++++++++++++++++++--
>> 4 files changed, 47 insertions(+), 5 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/hci.h =
b/include/net/bluetooth/hci.h
>> index bd3cecd..ca09998 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -210,6 +210,7 @@ enum {
>>=20
>> #define LMP_EV4 0x01
>> #define LMP_EV5 0x02
>> +#define LMP_NO_BREDR 0x20
>> #define LMP_LE 0x40
>>=20
>> #define LMP_SNIFF_SUBR 0x02
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index 9da5b69..55b78ec 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -678,6 +678,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>> #define lmp_ssp_capable(dev) ((dev)->features[6] & =
LMP_SIMPLE_PAIR)
>> #define lmp_no_flush_capable(dev) ((dev)->features[6] & =
LMP_NO_FLUSH)
>> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
>> +#define lmp_bredr_capable(dev) (!((dev)->features[4] & =
LMP_NO_BREDR))
>=20
> can you just split this into a separate patch first. We can just go
> ahead and merge it.
>=20
>> /* ----- Extended LMP capabilities ----- */
>> #define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & =
LMP_HOST_LE)
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index dcbbffe..037c7c0 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -954,24 +954,43 @@ static void hci_cc_le_set_scan_enable(struct =
hci_dev *hdev,
>>=20
>> BT_DBG("%s status 0x%x", hdev->name, status);
>>=20
>> - if (status)
>> - return;
>> -
>> cp =3D hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
>> if (!cp)
>> return;
>>=20
>> if (cp->enable =3D=3D 0x01) {
>> + if (status) {
>> + hci_dev_lock(hdev);
>> + mgmt_start_discovery_failed(hdev, status);
>> + hci_dev_unlock(hdev);
>> + return;
>> + }
>> +
>> set_bit(HCI_LE_SCAN, &hdev->hci_flags);
>>=20
>> del_timer(&hdev->adv_timer);
>>=20
>> hci_dev_lock(hdev);
>> +
>> + mgmt_discovering(hdev, 1);
>> +
>> hci_adv_entries_clear(hdev);
>> +
>> hci_dev_unlock(hdev);
>> } else if (cp->enable =3D=3D 0x00) {
>> + if (status) {
>> + hci_dev_lock(hdev);
>> + mgmt_stop_discovery_failed(hdev, status);
>> + hci_dev_unlock(hdev);
>> + return;
>> + }
>> +
>> clear_bit(HCI_LE_SCAN, &hdev->hci_flags);
>>=20
>> + hci_dev_lock(hdev);
>> + mgmt_discovering(hdev, 0);
>> + hci_dev_unlock(hdev);
>> +
>> mod_timer(&hdev->adv_timer, jiffies + =
ADV_CLEAR_TIMEOUT);
>> }
>> }
>=20
> This part looks fine to me.
>=20
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index b63a7d0..6ca6e5d 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -32,7 +32,16 @@
>> #define MGMT_VERSION 0
>> #define MGMT_REVISION 1
>>=20
>> -#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>> +/*
>> + * These LE scan and inquiry parameters were chosen according to LE =
General
>> + * Discovery Procedure specification.
>> + */
>> +#define LE_SCAN_TYPE 0x01
>> +#define LE_SCAN_WIN 0x12
>> +#define LE_SCAN_INT 0x12
>> +#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* =
TGAP(gen_disc_scan_min) */
>> +
>> +#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>>=20
>> struct pending_cmd {
>> struct list_head list;
>> @@ -1669,7 +1678,16 @@ static int start_discovery(struct sock *sk, =
u16 index)
>> goto failed;
>> }
>>=20
>> - err =3D hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>> + if (lmp_host_le_capable(hdev)) {
>> + if (lmp_bredr_capable(hdev))
>> + err =3D -ENOSYS;
>> + else
>> + err =3D hci_do_le_scan(hdev, LE_SCAN_TYPE, =
LE_SCAN_INT,
>> + LE_SCAN_WIN, =
LE_SCAN_TIMEOUT_LE_ONLY);
>> + } else {
>> + err =3D hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>> + }
>> +
>> if (err < 0)
>> mgmt_pending_remove(cmd);
>>=20
>> @@ -1701,6 +1719,9 @@ static int stop_discovery(struct sock *sk, u16 =
index)
>> }
>>=20
>> err =3D hci_cancel_inquiry(hdev);
>> + if (err =3D=3D -EPERM)
>> + err =3D hci_cancel_le_scan(hdev);
>> +
>=20
> And here, I have a serious problem with how the code is done. I =
realize
> that from using hdev->flags this ends up this crappy, but this is not
> how I wanna see things done.
>=20
> What you are doing is this:
>=20
> - We call a complete unrelated function anyway
> - And if it fails with a specific error code then we call something
> else instead
>=20
> I think it becomes pretty obvious now that we should just have had
> hdev->mgmt_flags that tell us with discovery procedure is running =
right
> now. And thus we know how to cancel it.

I'm not sure this will help us in case we have a interleaved discovery
running. My point is, even if we know what discovery procedure is =
running,
we need to know what the controller is doing right now (inquiring or le
scanning) so we can properly stop it, and, therefore, stop the ongoing
discovery procedure. IOW, telling us we have a interleaved discovery
running does not help us to decide the right function to call
(hci_cancel_inquiry or hci_cancel_le_scan).

So, I think we need to check the controller flags (HCI_INQUIRY and
HCI_LE_SCAN) in order to stop the ongoing discovery procedure properly.

BR,

Andre

2011-11-16 17:42:24

by Andre Guedes

Subject: Re: [PATCH 1/6] Bluetooth: Add hci_flags to struct hci_dev

Hi Marcel,

On Nov 11, 2011, at 8:09 PM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch adds the hci_flags field to struct hci_dev. This new
>> flags variable should be used to define flags related to BR/EDR
>> and/or LE controller itself. It should be used to define flags
>> which represents states from the controller. The hci_flags is
>> cleared in case the controller sends a Reset Command Complete
>> Event to the host.
>>=20
>> Also, this patch adds the HCI_LE_SCAN flag which was created to
>> track if the controller is performing LE scan or not. The flag
>> is set/cleared when the controller starts/stops scanning.
>>=20
>> This is an initial effort to stop using hdev->flags to define
>> internal flags since it is exported to userspace by an ioctl.
>>=20
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> include/net/bluetooth/hci.h | 8 ++++++++
>> include/net/bluetooth/hci_core.h | 2 ++
>> net/bluetooth/hci_core.c | 1 +
>> net/bluetooth/hci_event.c | 6 ++++++
>> 4 files changed, 17 insertions(+), 0 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/hci.h =
b/include/net/bluetooth/hci.h
>> index 139ce2a..70321a1 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -88,6 +88,14 @@ enum {
>> HCI_RESET,
>> };
>>=20
>> +/*
>> + * BR/EDR and/or LE controller flags: the flags defined here should =
represent
>> + * states from the controller.
>> + */
>> +enum {
>> + HCI_LE_SCAN,
>> +};
>> +
>> /* HCI ioctl defines */
>> #define HCIDEVUP _IOW('H', 201, int)
>> #define HCIDEVDOWN _IOW('H', 202, int)
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index 1795257..f6d5d90 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -250,6 +250,8 @@ struct hci_dev {
>>=20
>> struct module *owner;
>>=20
>> + unsigned long hci_flags;
>> +
>=20
> so I remember that I said, we call these mgmt_flags and make sure that
> all the flags are bound the mgmt interface. Why are we calling this
> hci_flags now?

I realized this flags variable is more related to the controller
itself than to management interface. For instance, HCI_LE_SCAN,
HCI_INQUIRY, HCI_PSCAN, HCI_ISCAN and others flags pretty much
related to the _controller_. Additionally, the PINQUIRY flag, which
we might add soon, might be defined in hci_flags too, since it is
related to the controller.

About the mgmt_flags, I was thinking in using this flags variable
to define management interface related flags. Flags such as
HCI_MGMT, HCI_PAIRABLE, HCI_SERVICE_CACHE and HCI_LINK_KEYS could
be added to mgmt_flags since they are all related to management
interface itself. In a patch in interleaved discovery support series
(as I said, I'll send it to ML soon), I create the mgmt_flags variable
and define the MGMT_DISCOV flags to track if we are carrying out a
discovery or not.=20

So, summarizing we would have two flags variables: hci_flags (which
holds flags related to the controller) and mgmt_flags (which holds
flags related to management interface).

Do we keep hci_flags or rename it to mgmt_flags and mix up controller
and management interface flags?

BR,

Andre=

2011-11-14 10:08:08

by Andrei Emeltchenko

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Andre,

On Fri, Nov 11, 2011 at 07:50:24PM -0300, Andre Guedes wrote:
> This patch adds support for LE-Only discovery procedure through
> management interface.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci.h | 1 +
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_event.c | 25 ++++++++++++++++++++++---
> net/bluetooth/mgmt.c | 25 +++++++++++++++++++++++--
> 4 files changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index bd3cecd..ca09998 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -210,6 +210,7 @@ enum {
>
> #define LMP_EV4 0x01
> #define LMP_EV5 0x02
> +#define LMP_NO_BREDR 0x20
> #define LMP_LE 0x40
>
> #define LMP_SNIFF_SUBR 0x02
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 9da5b69..55b78ec 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -678,6 +678,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
> #define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
> +#define lmp_bredr_capable(dev) (!((dev)->features[4] & LMP_NO_BREDR))
>
> /* ----- Extended LMP capabilities ----- */
> #define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & LMP_HOST_LE)
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index dcbbffe..037c7c0 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -954,24 +954,43 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>
> BT_DBG("%s status 0x%x", hdev->name, status);
>
> - if (status)
> - return;
> -
> cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
> if (!cp)
> return;
>
> if (cp->enable == 0x01) {

Can we fix also magic number here and below?

Best regards
Andrei Emeltchenko

> + if (status) {
> + hci_dev_lock(hdev);
> + mgmt_start_discovery_failed(hdev, status);
> + hci_dev_unlock(hdev);
> + return;
> + }
> +
> set_bit(HCI_LE_SCAN, &hdev->hci_flags);
>
> del_timer(&hdev->adv_timer);
>
> hci_dev_lock(hdev);
> +
> + mgmt_discovering(hdev, 1);
> +
> hci_adv_entries_clear(hdev);
> +
> hci_dev_unlock(hdev);
> } else if (cp->enable == 0x00) {
> + if (status) {
> + hci_dev_lock(hdev);
> + mgmt_stop_discovery_failed(hdev, status);
> + hci_dev_unlock(hdev);
> + return;
> + }
> +
> clear_bit(HCI_LE_SCAN, &hdev->hci_flags);
>
> + hci_dev_lock(hdev);
> + mgmt_discovering(hdev, 0);
> + hci_dev_unlock(hdev);
> +
> mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
> }
> }
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index b63a7d0..6ca6e5d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -32,7 +32,16 @@
> #define MGMT_VERSION 0
> #define MGMT_REVISION 1
>
> -#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
> +/*
> + * These LE scan and inquiry parameters were chosen according to LE General
> + * Discovery Procedure specification.
> + */
> +#define LE_SCAN_TYPE 0x01
> +#define LE_SCAN_WIN 0x12
> +#define LE_SCAN_INT 0x12
> +#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
> +
> +#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>
> struct pending_cmd {
> struct list_head list;
> @@ -1669,7 +1678,16 @@ static int start_discovery(struct sock *sk, u16 index)
> goto failed;
> }
>
> - err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> + if (lmp_host_le_capable(hdev)) {
> + if (lmp_bredr_capable(hdev))
> + err = -ENOSYS;
> + else
> + err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> + LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
> + } else {
> + err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> + }
> +
> if (err < 0)
> mgmt_pending_remove(cmd);
>
> @@ -1701,6 +1719,9 @@ static int stop_discovery(struct sock *sk, u16 index)
> }
>
> err = hci_cancel_inquiry(hdev);
> + if (err == -EPERM)
> + err = hci_cancel_le_scan(hdev);
> +
> if (err < 0)
> mgmt_pending_remove(cmd);
>
> --
> 1.7.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2011-11-12 09:54:33

by Johan Hedberg

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Andre,

On Fri, Nov 11, 2011, Andre Guedes wrote:
> @@ -1669,7 +1678,16 @@ static int start_discovery(struct sock *sk, u16 index)
> goto failed;
> }
>
> - err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> + if (lmp_host_le_capable(hdev)) {
> + if (lmp_bredr_capable(hdev))
> + err = -ENOSYS;
> + else
> + err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> + LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
> + } else {
> + err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> + }

This should really be looking at type parameter that gets passed in the
mgmt_start_discovery command. User space has been sending it already for
some time but the kernel side wasn't yet updated to care about it. I
just sent a patch to add the necessary mgmt.h struct and to pass the
data to the start_discovery function.

Johan

2011-11-12 06:43:41

by Marcel Holtmann

Subject: Re: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

Hi Andre,

> This patch adds support for LE-Only discovery procedure through
> management interface.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci.h | 1 +
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_event.c | 25 ++++++++++++++++++++++---
> net/bluetooth/mgmt.c | 25 +++++++++++++++++++++++--
> 4 files changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index bd3cecd..ca09998 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -210,6 +210,7 @@ enum {
>
> #define LMP_EV4 0x01
> #define LMP_EV5 0x02
> +#define LMP_NO_BREDR 0x20
> #define LMP_LE 0x40
>
> #define LMP_SNIFF_SUBR 0x02
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 9da5b69..55b78ec 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -678,6 +678,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
> #define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
> +#define lmp_bredr_capable(dev) (!((dev)->features[4] & LMP_NO_BREDR))

can you just split this into a separate patch first. We can just go
ahead and merge it.

> /* ----- Extended LMP capabilities ----- */
> #define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & LMP_HOST_LE)
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index dcbbffe..037c7c0 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -954,24 +954,43 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>
> BT_DBG("%s status 0x%x", hdev->name, status);
>
> - if (status)
> - return;
> -
> cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
> if (!cp)
> return;
>
> if (cp->enable == 0x01) {
> + if (status) {
> + hci_dev_lock(hdev);
> + mgmt_start_discovery_failed(hdev, status);
> + hci_dev_unlock(hdev);
> + return;
> + }
> +
> set_bit(HCI_LE_SCAN, &hdev->hci_flags);
>
> del_timer(&hdev->adv_timer);
>
> hci_dev_lock(hdev);
> +
> + mgmt_discovering(hdev, 1);
> +
> hci_adv_entries_clear(hdev);
> +
> hci_dev_unlock(hdev);
> } else if (cp->enable == 0x00) {
> + if (status) {
> + hci_dev_lock(hdev);
> + mgmt_stop_discovery_failed(hdev, status);
> + hci_dev_unlock(hdev);
> + return;
> + }
> +
> clear_bit(HCI_LE_SCAN, &hdev->hci_flags);
>
> + hci_dev_lock(hdev);
> + mgmt_discovering(hdev, 0);
> + hci_dev_unlock(hdev);
> +
> mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
> }
> }

This part looks fine to me.

> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index b63a7d0..6ca6e5d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -32,7 +32,16 @@
> #define MGMT_VERSION 0
> #define MGMT_REVISION 1
>
> -#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
> +/*
> + * These LE scan and inquiry parameters were chosen according to LE General
> + * Discovery Procedure specification.
> + */
> +#define LE_SCAN_TYPE 0x01
> +#define LE_SCAN_WIN 0x12
> +#define LE_SCAN_INT 0x12
> +#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
> +
> +#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>
> struct pending_cmd {
> struct list_head list;
> @@ -1669,7 +1678,16 @@ static int start_discovery(struct sock *sk, u16 index)
> goto failed;
> }
>
> - err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> + if (lmp_host_le_capable(hdev)) {
> + if (lmp_bredr_capable(hdev))
> + err = -ENOSYS;
> + else
> + err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> + LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
> + } else {
> + err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> + }
> +
> if (err < 0)
> mgmt_pending_remove(cmd);
>
> @@ -1701,6 +1719,9 @@ static int stop_discovery(struct sock *sk, u16 index)
> }
>
> err = hci_cancel_inquiry(hdev);
> + if (err == -EPERM)
> + err = hci_cancel_le_scan(hdev);
> +

And here, I have a serious problem with how the code is done. I realize
that from using hdev->flags this ends up this crappy, but this is not
how I wanna see things done.

What you are doing is this:

- We call a complete unrelated function anyway
- And if it fails with a specific error code then we call something
else instead

I think it becomes pretty obvious now that we should just have had
hdev->mgmt_flags that tell us with discovery procedure is running right
now. And thus we know how to cancel it.

Regards

Marcel

2011-11-11 23:14:50

by Marcel Holtmann

Subject: Re: [PATCH 5/6] Bluetooth: Report LE devices

Hi Andre,

> Devices found during LE scan should be reported to userspace through
> mgmt_device_found events.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> net/bluetooth/hci_event.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

However keep in mind that we might have to filter these with the changes
that Johan is doing with triggering scan for random vs public addresses.

Regards

Marcel

2011-11-11 23:13:37

by Marcel Holtmann

Subject: Re: [PATCH 3/6] Bluetooth: LE scan infra-structure

Hi Andre,

> This patch adds to hci_core the infra-structure to carry out the
> LE scan. Functions were created to init the LE scan and cancel
> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 5 +++
> net/bluetooth/hci_core.c | 69 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 74 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index f6d5d90..69dda9b 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -232,6 +232,8 @@ struct hci_dev {
> struct list_head adv_entries;
> struct timer_list adv_timer;
>
> + struct timer_list le_scan_timer;
> +
> struct hci_dev_stats stat;
>
> struct sk_buff_head driver_init;
> @@ -987,5 +989,8 @@ void hci_le_ltk_neg_reply(struct hci_conn *conn);
>
> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
> int hci_cancel_inquiry(struct hci_dev *hdev);
> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> + int timeout);
> +int hci_cancel_le_scan(struct hci_dev *hdev);
>
> #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 64cdafe..c07dc15 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1423,6 +1423,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
> return 0;
> }
>
> +static int le_scan(struct hci_dev *hdev, u8 enable)
> +{
> + struct hci_cp_le_set_scan_enable cp;
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.enable = enable;
> +
> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> +}
> +
> +static void le_scan_timeout(unsigned long arg)
> +{
> + struct hci_dev *hdev = (void *) arg;
> +
> + le_scan(hdev, 0);
> +}
> +
> /* Register HCI device */
> int hci_register_dev(struct hci_dev *hdev)
> {
> @@ -1501,6 +1518,9 @@ int hci_register_dev(struct hci_dev *hdev)
> setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
> (unsigned long) hdev);
>
> + setup_timer(&hdev->le_scan_timer, le_scan_timeout,
> + (unsigned long) hdev);
> +
> INIT_WORK(&hdev->power_on, hci_power_on);
> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>
> @@ -1587,6 +1607,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
> hci_del_sysfs(hdev);
>
> del_timer(&hdev->adv_timer);
> + del_timer(&hdev->le_scan_timer);
>
> destroy_workqueue(hdev->workqueue);
>
> @@ -2615,3 +2636,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>
> return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> }
> +
> +static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 interval,
> + u16 window)
> +{
> + struct hci_cp_le_set_scan_param cp;
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.type = type;
> + cp.interval = cpu_to_le16(interval);
> + cp.window = cpu_to_le16(window);
> +
> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
> +}
> +
> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> + int timeout)
> +{
> + int err;
> +
> + if (test_bit(HCI_LE_SCAN, &hdev->hci_flags))
> + return -EINPROGRESS;
> +
> + BT_DBG("%s", hdev->name);
> +
> + err = set_le_scan_param(hdev, type, interval, window);
> + if (err < 0)
> + return err;
> +
> + err = le_scan(hdev, 1);
> + if (err < 0)
> + return err;

since you are using hci_send_cmd, you never check the error from the
controller for set_le_scan_param. We should be doing exactly that before
just going ahead with a scan.

It is also not guaranteed that the controller queues up these commands,
it might just return busy from le_scan() if it can have more than one
outstanding commands (which many controller can do).

Regards

Marcel

2011-11-11 23:10:55

by Marcel Holtmann

Subject: Re: [PATCH 2/6] Bluetooth: Add LE Set Scan Parameter Command

Hi Andre,

> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci.h | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel

2011-11-11 23:09:53

by Marcel Holtmann

Subject: Re: [PATCH 1/6] Bluetooth: Add hci_flags to struct hci_dev

Hi Andre,

> This patch adds the hci_flags field to struct hci_dev. This new
> flags variable should be used to define flags related to BR/EDR
> and/or LE controller itself. It should be used to define flags
> which represents states from the controller. The hci_flags is
> cleared in case the controller sends a Reset Command Complete
> Event to the host.
>
> Also, this patch adds the HCI_LE_SCAN flag which was created to
> track if the controller is performing LE scan or not. The flag
> is set/cleared when the controller starts/stops scanning.
>
> This is an initial effort to stop using hdev->flags to define
> internal flags since it is exported to userspace by an ioctl.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci.h | 8 ++++++++
> include/net/bluetooth/hci_core.h | 2 ++
> net/bluetooth/hci_core.c | 1 +
> net/bluetooth/hci_event.c | 6 ++++++
> 4 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 139ce2a..70321a1 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -88,6 +88,14 @@ enum {
> HCI_RESET,
> };
>
> +/*
> + * BR/EDR and/or LE controller flags: the flags defined here should represent
> + * states from the controller.
> + */
> +enum {
> + HCI_LE_SCAN,
> +};
> +
> /* HCI ioctl defines */
> #define HCIDEVUP _IOW('H', 201, int)
> #define HCIDEVDOWN _IOW('H', 202, int)
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 1795257..f6d5d90 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -250,6 +250,8 @@ struct hci_dev {
>
> struct module *owner;
>
> + unsigned long hci_flags;
> +

so I remember that I said, we call these mgmt_flags and make sure that
all the flags are bound the mgmt interface. Why are we calling this
hci_flags now?

Regards

Marcel

2011-11-11 22:50:24

by Andre Guedes

Subject: [PATCH 6/6] Bluetooth: Support LE-Only discovery procedure

This patch adds support for LE-Only discovery procedure through
management interface.

Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci.h | 1 +
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_event.c | 25 ++++++++++++++++++++++---
net/bluetooth/mgmt.c | 25 +++++++++++++++++++++++--
4 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index bd3cecd..ca09998 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -210,6 +210,7 @@ enum {

#define LMP_EV4 0x01
#define LMP_EV5 0x02
+#define LMP_NO_BREDR 0x20
#define LMP_LE 0x40

#define LMP_SNIFF_SUBR 0x02
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9da5b69..55b78ec 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -678,6 +678,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
#define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
#define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
#define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
+#define lmp_bredr_capable(dev) (!((dev)->features[4] & LMP_NO_BREDR))

/* ----- Extended LMP capabilities ----- */
#define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & LMP_HOST_LE)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index dcbbffe..037c7c0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -954,24 +954,43 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,

BT_DBG("%s status 0x%x", hdev->name, status);

- if (status)
- return;
-
cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
if (!cp)
return;

if (cp->enable == 0x01) {
+ if (status) {
+ hci_dev_lock(hdev);
+ mgmt_start_discovery_failed(hdev, status);
+ hci_dev_unlock(hdev);
+ return;
+ }
+
set_bit(HCI_LE_SCAN, &hdev->hci_flags);

del_timer(&hdev->adv_timer);

hci_dev_lock(hdev);
+
+ mgmt_discovering(hdev, 1);
+
hci_adv_entries_clear(hdev);
+
hci_dev_unlock(hdev);
} else if (cp->enable == 0x00) {
+ if (status) {
+ hci_dev_lock(hdev);
+ mgmt_stop_discovery_failed(hdev, status);
+ hci_dev_unlock(hdev);
+ return;
+ }
+
clear_bit(HCI_LE_SCAN, &hdev->hci_flags);

+ hci_dev_lock(hdev);
+ mgmt_discovering(hdev, 0);
+ hci_dev_unlock(hdev);
+
mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
}
}
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index b63a7d0..6ca6e5d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -32,7 +32,16 @@
#define MGMT_VERSION 0
#define MGMT_REVISION 1

-#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
+/*
+ * These LE scan and inquiry parameters were chosen according to LE General
+ * Discovery Procedure specification.
+ */
+#define LE_SCAN_TYPE 0x01
+#define LE_SCAN_WIN 0x12
+#define LE_SCAN_INT 0x12
+#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
+
+#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */

struct pending_cmd {
struct list_head list;
@@ -1669,7 +1678,16 @@ static int start_discovery(struct sock *sk, u16 index)
goto failed;
}

- err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
+ if (lmp_host_le_capable(hdev)) {
+ if (lmp_bredr_capable(hdev))
+ err = -ENOSYS;
+ else
+ err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
+ LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
+ } else {
+ err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
+ }
+
if (err < 0)
mgmt_pending_remove(cmd);

@@ -1701,6 +1719,9 @@ static int stop_discovery(struct sock *sk, u16 index)
}

err = hci_cancel_inquiry(hdev);
+ if (err == -EPERM)
+ err = hci_cancel_le_scan(hdev);
+
if (err < 0)
mgmt_pending_remove(cmd);

--
1.7.7.1

2011-11-11 22:50:23

by Andre Guedes

Subject: [PATCH 5/6] Bluetooth: Report LE devices

Devices found during LE scan should be reported to userspace through
mgmt_device_found events.

Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_event.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6d60fec..dcbbffe 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2909,6 +2909,7 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
{
u8 num_reports = skb->data[0];
void *ptr = &skb->data[1];
+ s8 rssi;

hci_dev_lock(hdev);

@@ -2917,6 +2918,10 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,

hci_add_adv_entry(hdev, ev);

+ rssi = ev->data[ev->length];
+ mgmt_device_found(hdev, &ev->bdaddr, LE_LINK, ev->bdaddr_type,
+ NULL, rssi, ev->data, ev->length);
+
ptr += sizeof(*ev) + ev->length + 1;
}

--
1.7.7.1

2011-11-11 22:50:21

by Andre Guedes

Subject: [PATCH 3/6] Bluetooth: LE scan infra-structure

This patch adds to hci_core the infra-structure to carry out the
LE scan. Functions were created to init the LE scan and cancel
an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).

Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 5 +++
net/bluetooth/hci_core.c | 69 ++++++++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f6d5d90..69dda9b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -232,6 +232,8 @@ struct hci_dev {
struct list_head adv_entries;
struct timer_list adv_timer;

+ struct timer_list le_scan_timer;
+
struct hci_dev_stats stat;

struct sk_buff_head driver_init;
@@ -987,5 +989,8 @@ void hci_le_ltk_neg_reply(struct hci_conn *conn);

int hci_do_inquiry(struct hci_dev *hdev, u8 length);
int hci_cancel_inquiry(struct hci_dev *hdev);
+int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
+ int timeout);
+int hci_cancel_le_scan(struct hci_dev *hdev);

#endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 64cdafe..c07dc15 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1423,6 +1423,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
return 0;
}

+static int le_scan(struct hci_dev *hdev, u8 enable)
+{
+ struct hci_cp_le_set_scan_enable cp;
+
+ memset(&cp, 0, sizeof(cp));
+ cp.enable = enable;
+
+ return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+}
+
+static void le_scan_timeout(unsigned long arg)
+{
+ struct hci_dev *hdev = (void *) arg;
+
+ le_scan(hdev, 0);
+}
+
/* Register HCI device */
int hci_register_dev(struct hci_dev *hdev)
{
@@ -1501,6 +1518,9 @@ int hci_register_dev(struct hci_dev *hdev)
setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
(unsigned long) hdev);

+ setup_timer(&hdev->le_scan_timer, le_scan_timeout,
+ (unsigned long) hdev);
+
INIT_WORK(&hdev->power_on, hci_power_on);
INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);

@@ -1587,6 +1607,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
hci_del_sysfs(hdev);

del_timer(&hdev->adv_timer);
+ del_timer(&hdev->le_scan_timer);

destroy_workqueue(hdev->workqueue);

@@ -2615,3 +2636,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)

return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
}
+
+static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 interval,
+ u16 window)
+{
+ struct hci_cp_le_set_scan_param cp;
+
+ memset(&cp, 0, sizeof(cp));
+ cp.type = type;
+ cp.interval = cpu_to_le16(interval);
+ cp.window = cpu_to_le16(window);
+
+ return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
+}
+
+int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
+ int timeout)
+{
+ int err;
+
+ if (test_bit(HCI_LE_SCAN, &hdev->hci_flags))
+ return -EINPROGRESS;
+
+ BT_DBG("%s", hdev->name);
+
+ err = set_le_scan_param(hdev, type, interval, window);
+ if (err < 0)
+ return err;
+
+ err = le_scan(hdev, 1);
+ if (err < 0)
+ return err;
+
+ mod_timer(&hdev->le_scan_timer, jiffies + msecs_to_jiffies(timeout));
+
+ return 0;
+}
+
+int hci_cancel_le_scan(struct hci_dev *hdev)
+{
+ if (!test_bit(HCI_LE_SCAN, &hdev->hci_flags))
+ return -EPERM;
+
+ BT_DBG("%s", hdev->name);
+
+ del_timer(&hdev->le_scan_timer);
+
+ return le_scan(hdev, 0);
+}
--
1.7.7.1

2011-11-11 22:50:22

by Andre Guedes

Subject: [PATCH 4/6] Bluetooth: Add 'eir_len' param to mgmt_device_found()

This patch adds a new parameter to mgmt_device_found() to inform
the length of 'eir' pointer.

EIR data from LE advertising report event doesn't have a fixed length
as EIR data from extended inquiry result event does. We needed to
change mgmt_device_found() so it copies 'eir_len' bytes instead of
HCI_MAX_EIR_LENGTH.

Signed-off-by: Andre Guedes <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
---
include/net/bluetooth/hci_core.h | 3 ++-
net/bluetooth/hci_event.c | 9 +++++----
net/bluetooth/mgmt.c | 8 ++++++--
3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 69dda9b..9da5b69 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -942,7 +942,8 @@ int mgmt_set_local_name_complete(struct hci_dev *hdev, u8 *name, u8 status);
int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
u8 *randomizer, u8 status);
int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
- u8 addr_type, u8 *dev_class, s8 rssi, u8 *eir);
+ u8 addr_type, u8 *dev_class, s8 rssi,
+ u8 *eir, u8 eir_len);
int mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 *name);
int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status);
int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0beb633..6d60fec 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1448,7 +1448,7 @@ static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *
data.ssp_mode = 0x00;
hci_inquiry_cache_update(hdev, &data);
mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
- info->dev_class, 0, NULL);
+ info->dev_class, 0, NULL, 0);
}

hci_dev_unlock(hdev);
@@ -2458,7 +2458,7 @@ static inline void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, struct
hci_inquiry_cache_update(hdev, &data);
mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
info->dev_class, info->rssi,
- NULL);
+ NULL, 0);
}
} else {
struct inquiry_info_with_rssi *info = (void *) (skb->data + 1);
@@ -2475,7 +2475,7 @@ static inline void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, struct
hci_inquiry_cache_update(hdev, &data);
mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
info->dev_class, info->rssi,
- NULL);
+ NULL, 0);
}
}

@@ -2617,7 +2617,8 @@ static inline void hci_extended_inquiry_result_evt(struct hci_dev *hdev, struct
data.ssp_mode = 0x01;
hci_inquiry_cache_update(hdev, &data);
mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
- info->dev_class, info->rssi, info->data);
+ info->dev_class, info->rssi,
+ info->data, sizeof(info->data));
}

hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index bd77f54..b63a7d0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2397,10 +2397,14 @@ int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
}

int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
- u8 addr_type, u8 *dev_class, s8 rssi, u8 *eir)
+ u8 addr_type, u8 *dev_class, s8 rssi,
+ u8 *eir, u8 eir_len)
{
struct mgmt_ev_device_found ev;

+ if (eir_len > sizeof(ev.eir))
+ return -EINVAL;
+
memset(&ev, 0, sizeof(ev));

bacpy(&ev.addr.bdaddr, bdaddr);
@@ -2408,7 +2412,7 @@ int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
ev.rssi = rssi;

if (eir)
- memcpy(ev.eir, eir, sizeof(ev.eir));
+ memcpy(ev.eir, eir, eir_len);

if (dev_class)
memcpy(ev.dev_class, dev_class, sizeof(ev.dev_class));
--
1.7.7.1

2011-11-11 22:50:20

by Andre Guedes

Subject: [PATCH 2/6] Bluetooth: Add LE Set Scan Parameter Command

Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci.h | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 70321a1..bd3cecd 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -768,6 +768,15 @@ struct hci_rp_le_read_buffer_size {
__u8 le_max_pkt;
} __packed;

+#define HCI_OP_LE_SET_SCAN_PARAM 0x200b
+struct hci_cp_le_set_scan_param {
+ __u8 type;
+ __le16 interval;
+ __le16 window;
+ __u8 own_address_type;
+ __u8 filter_policy;
+} __packed;
+
#define HCI_OP_LE_SET_SCAN_ENABLE 0x200c
struct hci_cp_le_set_scan_enable {
__u8 enable;
--
1.7.7.1

2011-11-11 22:50:19

by Andre Guedes

Subject: [PATCH 1/6] Bluetooth: Add hci_flags to struct hci_dev

This patch adds the hci_flags field to struct hci_dev. This new
flags variable should be used to define flags related to BR/EDR
and/or LE controller itself. It should be used to define flags
which represents states from the controller. The hci_flags is
cleared in case the controller sends a Reset Command Complete
Event to the host.

Also, this patch adds the HCI_LE_SCAN flag which was created to
track if the controller is performing LE scan or not. The flag
is set/cleared when the controller starts/stops scanning.

This is an initial effort to stop using hdev->flags to define
internal flags since it is exported to userspace by an ioctl.

Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci.h | 8 ++++++++
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 1 +
net/bluetooth/hci_event.c | 6 ++++++
4 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 139ce2a..70321a1 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -88,6 +88,14 @@ enum {
HCI_RESET,
};

+/*
+ * BR/EDR and/or LE controller flags: the flags defined here should represent
+ * states from the controller.
+ */
+enum {
+ HCI_LE_SCAN,
+};
+
/* HCI ioctl defines */
#define HCIDEVUP _IOW('H', 201, int)
#define HCIDEVDOWN _IOW('H', 202, int)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1795257..f6d5d90 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -250,6 +250,8 @@ struct hci_dev {

struct module *owner;

+ unsigned long hci_flags;
+
int (*open)(struct hci_dev *hdev);
int (*close)(struct hci_dev *hdev);
int (*flush)(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fb3feeb..64cdafe 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1457,6 +1457,7 @@ int hci_register_dev(struct hci_dev *hdev)
spin_lock_init(&hdev->lock);

hdev->flags = 0;
+ hdev->hci_flags = 0;
hdev->pkt_type = (HCI_DM1 | HCI_DH1 | HCI_HV1);
hdev->esco_type = (ESCO_HV1);
hdev->link_mode = (HCI_LM_ACCEPT);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index dfe6fbc..0beb633 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -194,6 +194,8 @@ static void hci_cc_reset(struct hci_dev *hdev, struct sk_buff *skb)
clear_bit(HCI_RESET, &hdev->flags);

hci_req_complete(hdev, HCI_OP_RESET, status);
+
+ hdev->hci_flags = 0;
}

static void hci_cc_write_local_name(struct hci_dev *hdev, struct sk_buff *skb)
@@ -960,12 +962,16 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
return;

if (cp->enable == 0x01) {
+ set_bit(HCI_LE_SCAN, &hdev->hci_flags);
+
del_timer(&hdev->adv_timer);

hci_dev_lock(hdev);
hci_adv_entries_clear(hdev);
hci_dev_unlock(hdev);
} else if (cp->enable == 0x00) {
+ clear_bit(HCI_LE_SCAN, &hdev->hci_flags);
+
mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
}
}
--
1.7.7.1