2012-03-09 12:57:40

by Andre Guedes

Subject: [PATCH v2 0/2] LE support for MGMT Stop Discovery

>From the previous cover letter:

This small patch series adds LE support for Stop Discovery MGMT command.
Now, we are able to stop LE discovery procedues (LE-only and interleaved).

A brief word about patch 1/2: due to discovery state machine, we don't need
to worry about canceling hdev->le_scan work since it is never pending or
running when hci_cancel_le_scan is called.

Andre Guedes (2):
Bluetooth: Add hci_cancel_le_scan() to hci_core
Bluetooth: LE support for MGMT stop discovery

include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 21 +++++++++++++++++++++
net/bluetooth/hci_event.c | 6 +++++-
net/bluetooth/mgmt.c | 6 +++++-
4 files changed, 32 insertions(+), 2 deletions(-)

--
1.7.9.3

2012-03-15 19:04:18

by Andre Guedes

Subject: Re: [PATCH v2 1/2] Bluetooth: Add hci_cancel_le_scan() to hci_core

Hi Marcel,

On Thu, Mar 15, 2012 at 3:36 PM, Marcel Holtmann <[email protected]> wrot=
e:
> Hi Andre,
>
>> >> This patch adds to hci_core the hci_cancel_le_scan function which
>> >> should be used to cancel an ongoing LE scan.
>> >>
>> >> Signed-off-by: Andre Guedes <[email protected]>
>> >> ---
>> >> =A0include/net/bluetooth/hci_core.h | =A0 =A01 +
>> >> =A0net/bluetooth/hci_core.c =A0 =A0 =A0 =A0 | =A0 21 ++++++++++++++++=
+++++
>> >> =A02 files changed, 22 insertions(+)
>> >>
>> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth=
/hci_core.h
>> >> index daefaac..87bca3e 100644
>> >> --- a/include/net/bluetooth/hci_core.h
>> >> +++ b/include/net/bluetooth/hci_core.h
>> >> @@ -1071,5 +1071,6 @@ int hci_do_inquiry(struct hci_dev *hdev, u8 len=
gth);
>> >> =A0int hci_cancel_inquiry(struct hci_dev *hdev);
>> >> =A0int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 w=
indow,
>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 int timeout);
>> >> +int hci_cancel_le_scan(struct hci_dev *hdev);
>> >>
>> >> =A0#endif /* __HCI_CORE_H */
>> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> >> index 59ec99e..0320a02 100644
>> >> --- a/net/bluetooth/hci_core.c
>> >> +++ b/net/bluetooth/hci_core.c
>> >> @@ -1667,6 +1667,27 @@ static int hci_do_le_scan(struct hci_dev *hdev=
, u8 type, u16 interval,
>> >> =A0 =A0 =A0 return 0;
>> >> =A0}
>> >>
>> >> +int hci_cancel_le_scan(struct hci_dev *hdev)
>> >> +{
>> >> + =A0 =A0 bool canceled;
>> >> +
>> >> + =A0 =A0 BT_DBG("%s", hdev->name);
>> >> +
>> >> + =A0 =A0 if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> >> + =A0 =A0 =A0 =A0 =A0 =A0 return -EALREADY;
>> >> +
>> >> + =A0 =A0 canceled =3D cancel_delayed_work(&hdev->le_scan_disable);
>> >
>> > why are we using the canceled variable here? Seems pointless.
>>
>> Because le_scan_disable work may be running when cancel_delayed_work
>> returns (canceled =3D=3D 0). If le_scan_disable work is running, we shou=
ld
>> not send HCI_OP_LE_SET_SCAN_ENABLE command since le_scan_disable work
>> already sends it.
>
> I got that part, but I meant why using an extra variable here?
>
> =A0 =A0 =A0 =A0if (cancel_delayed_work(&hdev->le_scan_disable)) {
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0...
>
> =A0 =A0 =A0 =A0}

Oh, sorry, now I see your point.

> This is as clean and does exactly the same job. Or do I miss something
> here?

You're right, I'll remove the variable and send a new version.

Thanks,

Andre

2012-03-15 18:36:08

by Marcel Holtmann

Subject: Re: [PATCH v2 1/2] Bluetooth: Add hci_cancel_le_scan() to hci_core

Hi Andre,

> >> This patch adds to hci_core the hci_cancel_le_scan function which
> >> should be used to cancel an ongoing LE scan.
> >>
> >> Signed-off-by: Andre Guedes <[email protected]>
> >> ---
> >> include/net/bluetooth/hci_core.h | 1 +
> >> net/bluetooth/hci_core.c | 21 +++++++++++++++++++++
> >> 2 files changed, 22 insertions(+)
> >>
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index daefaac..87bca3e 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -1071,5 +1071,6 @@ int hci_do_inquiry(struct hci_dev *hdev, u8 length);
> >> int hci_cancel_inquiry(struct hci_dev *hdev);
> >> int hci_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 59ec99e..0320a02 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -1667,6 +1667,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
> >> return 0;
> >> }
> >>
> >> +int hci_cancel_le_scan(struct hci_dev *hdev)
> >> +{
> >> + bool canceled;
> >> +
> >> + BT_DBG("%s", hdev->name);
> >> +
> >> + if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> >> + return -EALREADY;
> >> +
> >> + canceled = cancel_delayed_work(&hdev->le_scan_disable);
> >
> > why are we using the canceled variable here? Seems pointless.
>
> Because le_scan_disable work may be running when cancel_delayed_work
> returns (canceled == 0). If le_scan_disable work is running, we should
> not send HCI_OP_LE_SET_SCAN_ENABLE command since le_scan_disable work
> already sends it.

I got that part, but I meant why using an extra variable here?

if (cancel_delayed_work(&hdev->le_scan_disable)) {

...

}

This is as clean and does exactly the same job. Or do I miss something
here?

Regards

Marcel

2012-03-15 18:31:10

by Andre Guedes

Subject: Re: [PATCH v2 1/2] Bluetooth: Add hci_cancel_le_scan() to hci_core

Hi Marcel,

On Thu, Mar 15, 2012 at 12:26 PM, Marcel Holtmann <[email protected]> wro=
te:
> Hi Andre,
>
>> This patch adds to hci_core the hci_cancel_le_scan function which
>> should be used to cancel an ongoing LE scan.
>>
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> =A0include/net/bluetooth/hci_core.h | =A0 =A01 +
>> =A0net/bluetooth/hci_core.c =A0 =A0 =A0 =A0 | =A0 21 +++++++++++++++++++=
++
>> =A02 files changed, 22 insertions(+)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hc=
i_core.h
>> index daefaac..87bca3e 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1071,5 +1071,6 @@ int hci_do_inquiry(struct hci_dev *hdev, u8 length=
);
>> =A0int hci_cancel_inquiry(struct hci_dev *hdev);
>> =A0int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 wind=
ow,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 int timeout);
>> +int hci_cancel_le_scan(struct hci_dev *hdev);
>>
>> =A0#endif /* __HCI_CORE_H */
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 59ec99e..0320a02 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1667,6 +1667,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u=
8 type, u16 interval,
>> =A0 =A0 =A0 return 0;
>> =A0}
>>
>> +int hci_cancel_le_scan(struct hci_dev *hdev)
>> +{
>> + =A0 =A0 bool canceled;
>> +
>> + =A0 =A0 BT_DBG("%s", hdev->name);
>> +
>> + =A0 =A0 if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EALREADY;
>> +
>> + =A0 =A0 canceled =3D cancel_delayed_work(&hdev->le_scan_disable);
>
> why are we using the canceled variable here? Seems pointless.

Because le_scan_disable work may be running when cancel_delayed_work
returns (canceled =3D=3D 0). If le_scan_disable work is running, we should
not send HCI_OP_LE_SET_SCAN_ENABLE command since le_scan_disable work
already sends it.

This way we avoid sending two HCI_OP_LE_SET_SCAN_ENABLE commands when
this race condition occurs.

BR,

Andre

2012-03-15 15:28:21

by Marcel Holtmann

Subject: Re: [PATCH v2 2/2] Bluetooth: LE support for MGMT stop discovery

Hi Andre,

> This patch adds LE support to MGMT stop discovery command. So,
> now we are able to cancel LE discovery procedures (LE-only and
> interleaved).
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> net/bluetooth/hci_event.c | 6 +++++-
> net/bluetooth/mgmt.c | 6 +++++-
> 2 files changed, 10 insertions(+), 2 deletions(-)

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

Regards

Marcel

2012-03-15 15:26:56

by Marcel Holtmann

Subject: Re: [PATCH v2 1/2] Bluetooth: Add hci_cancel_le_scan() to hci_core

Hi Andre,

> This patch adds to hci_core the hci_cancel_le_scan function which
> should be used to cancel an ongoing LE scan.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 21 +++++++++++++++++++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index daefaac..87bca3e 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1071,5 +1071,6 @@ int hci_do_inquiry(struct hci_dev *hdev, u8 length);
> int hci_cancel_inquiry(struct hci_dev *hdev);
> int hci_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 59ec99e..0320a02 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1667,6 +1667,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
> return 0;
> }
>
> +int hci_cancel_le_scan(struct hci_dev *hdev)
> +{
> + bool canceled;
> +
> + BT_DBG("%s", hdev->name);
> +
> + if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> + return -EALREADY;
> +
> + canceled = cancel_delayed_work(&hdev->le_scan_disable);

why are we using the canceled variable here? Seems pointless.

> + if (canceled) {
> + struct hci_cp_le_set_scan_enable cp;
> +
> + /* Send HCI command to disable LE Scan */
> + memset(&cp, 0, sizeof(cp));
> + hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> + }
> +
> + return 0;
> +}
> +

Regards

Marcel

2012-03-09 12:57:41

by Andre Guedes

Subject: [PATCH v2 1/2] Bluetooth: Add hci_cancel_le_scan() to hci_core

This patch adds to hci_core the hci_cancel_le_scan function which
should be used to cancel an ongoing LE scan.

Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 21 +++++++++++++++++++++
2 files changed, 22 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index daefaac..87bca3e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1071,5 +1071,6 @@ int hci_do_inquiry(struct hci_dev *hdev, u8 length);
int hci_cancel_inquiry(struct hci_dev *hdev);
int hci_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 59ec99e..0320a02 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1667,6 +1667,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
return 0;
}

+int hci_cancel_le_scan(struct hci_dev *hdev)
+{
+ bool canceled;
+
+ BT_DBG("%s", hdev->name);
+
+ if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+ return -EALREADY;
+
+ canceled = cancel_delayed_work(&hdev->le_scan_disable);
+ if (canceled) {
+ struct hci_cp_le_set_scan_enable cp;
+
+ /* Send HCI command to disable LE Scan */
+ memset(&cp, 0, sizeof(cp));
+ hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+ }
+
+ return 0;
+}
+
static void le_scan_disable_work(struct work_struct *work)
{
struct hci_dev *hdev = container_of(work, struct hci_dev,
--
1.7.9.3

2012-03-09 12:57:42

by Andre Guedes

Subject: [PATCH v2 2/2] Bluetooth: LE support for MGMT stop discovery

This patch adds LE support to MGMT stop discovery command. So,
now we are able to cancel LE discovery procedures (LE-only and
interleaved).

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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index badb785..b896447 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1092,8 +1092,12 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
break;

case LE_SCANNING_DISABLED:
- if (status)
+ if (status) {
+ hci_dev_lock(hdev);
+ mgmt_stop_discovery_failed(hdev, status);
+ hci_dev_unlock(hdev);
return;
+ }

clear_bit(HCI_LE_SCAN, &hdev->dev_flags);

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7fcff88..ad4eba0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2382,7 +2382,11 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
}

if (hdev->discovery.state == DISCOVERY_FINDING) {
- err = hci_cancel_inquiry(hdev);
+ if (test_bit(HCI_INQUIRY, &hdev->flags))
+ err = hci_cancel_inquiry(hdev);
+ else
+ err = hci_cancel_le_scan(hdev);
+
if (err < 0)
mgmt_pending_remove(cmd);
else
--
1.7.9.3