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

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

[Andre Guedes]

Hi all,

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.

BR,

Andre


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.2

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

[Andre Guedes]

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 <andre.guedes@openbossa.org>
---
 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 25cb0a1..0db2934 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1072,5 +1072,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 661d65f..0c2ceaa 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1672,6 +1672,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 -EPERM;
+
+	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.2

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

[Andre Guedes]

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 <andre.guedes@openbossa.org>
---
 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 6a817da..7e637f3 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1094,8 +1094,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 4ca0092..e393f5e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2384,7 +2384,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.2

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

[Gustavo Padovan]

Hi Andre,

* Andre Guedes <andre.guedes@openbossa.org> [2012-03-06 19:54:33 -0300]:

> 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 <andre.guedes@openbossa.org>
> ---
>  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 25cb0a1..0db2934 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1072,5 +1072,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 661d65f..0c2ceaa 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1672,6 +1672,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 -EPERM;

Are you sure about this -EPERM return error here? At a glance -EALREADY looks
better to me.

	Gustavo

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

[Andre Guedes]

Hi Gustavo,

On Tue, Mar 6, 2012 at 8:07 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> Hi Andre,
>
> * Andre Guedes <andre.guedes@openbossa.org> [2012-03-06 19:54:33 -0300]:
>
>> 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 <andre.guedes@openbossa.org>
>> ---
>>  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 25cb0a1..0db2934 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1072,5 +1072,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 661d65f..0c2ceaa 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1672,6 +1672,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 -EPERM;
>
> Are you sure about this -EPERM return error here? At a glance -EALREADY looks
> better to me.

This function cancels an operation (LE scan). If the operation is not
running, it makes more sense to me returning "Operation not permitted"
instead of "Operation already in progress".

BR,

Andre

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

[Marcel Holtmann]

Hi Andre,

> > * Andre Guedes <andre.guedes@openbossa.org> [2012-03-06 19:54:33 -0300]:
> >
> >> 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 <andre.guedes@openbossa.org>
> >> ---
> >>  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 25cb0a1..0db2934 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -1072,5 +1072,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 661d65f..0c2ceaa 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -1672,6 +1672,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 -EPERM;
> >
> > Are you sure about this -EPERM return error here? At a glance -EALREADY looks
> > better to me.
> 
> This function cancels an operation (LE scan). If the operation is not
> running, it makes more sense to me returning "Operation not permitted"
> instead of "Operation already in progress".

actually EPERM is for operation not permitted because you do not have
rights to access is. Not because it is invalid operation.

You need to find a better error code.

Regards

Marcel

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

[Andre Guedes]

Hi Marcel,

On Wed, Mar 7, 2012 at 4:23 PM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
> Hi Andre,
>
>> > * Andre Guedes <andre.guedes@openbossa.org> [2012-03-06 19:54:33 -0300=
]:
>> >
>> >> 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 <andre.guedes@openbossa.org>
>> >> ---
>> >> =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 25cb0a1..0db2934 100644
>> >> --- a/include/net/bluetooth/hci_core.h
>> >> +++ b/include/net/bluetooth/hci_core.h
>> >> @@ -1072,5 +1072,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 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =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 661d65f..0c2ceaa 100644
>> >> --- a/net/bluetooth/hci_core.c
>> >> +++ b/net/bluetooth/hci_core.c
>> >> @@ -1672,6 +1672,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 -EPERM;
>> >
>> > Are you sure about this -EPERM return error here? At a glance -EALREAD=
Y looks
>> > better to me.
>>
>> This function cancels an operation (LE scan). If the operation is not
>> running, it makes more sense to me returning "Operation not permitted"
>> instead of "Operation already in progress".
>
> actually EPERM is for operation not permitted because you do not have
> rights to access is. Not because it is invalid operation.
>
> You need to find a better error code.

Ok, then, as Gustavo suggested, EALREADY looks really better.
Are you fine with EALREADY?

Andre

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

[Marcel Holtmann]

Hi Andre,

> >> > * Andre Guedes <andre.guedes@openbossa.org> [2012-03-06 19:54:33 -0300]:
> >> >
> >> >> 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 <andre.guedes@openbossa.org>
> >> >> ---
> >> >>  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 25cb0a1..0db2934 100644
> >> >> --- a/include/net/bluetooth/hci_core.h
> >> >> +++ b/include/net/bluetooth/hci_core.h
> >> >> @@ -1072,5 +1072,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 661d65f..0c2ceaa 100644
> >> >> --- a/net/bluetooth/hci_core.c
> >> >> +++ b/net/bluetooth/hci_core.c
> >> >> @@ -1672,6 +1672,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 -EPERM;
> >> >
> >> > Are you sure about this -EPERM return error here? At a glance -EALREADY looks
> >> > better to me.
> >>
> >> This function cancels an operation (LE scan). If the operation is not
> >> running, it makes more sense to me returning "Operation not permitted"
> >> instead of "Operation already in progress".
> >
> > actually EPERM is for operation not permitted because you do not have
> > rights to access is. Not because it is invalid operation.
> >
> > You need to find a better error code.
> 
> Ok, then, as Gustavo suggested, EALREADY looks really better.
> Are you fine with EALREADY?

seems fine to me.

Regards

Marcel

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

[Andre Guedes]

Hi Marcel/Gustavo,

On Thu, Mar 8, 2012 at 2:53 PM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
> Hi Andre,
>
>> >> > * Andre Guedes <andre.guedes@openbossa.org> [2012-03-06 19:54:33 -0=
300]:
>> >> >
>> >> >> 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 <andre.guedes@openbossa.org>
>> >> >> ---
>> >> >> =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/blueto=
oth/hci_core.h
>> >> >> index 25cb0a1..0db2934 100644
>> >> >> --- a/include/net/bluetooth/hci_core.h
>> >> >> +++ b/include/net/bluetooth/hci_core.h
>> >> >> @@ -1072,5 +1072,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, u1=
6 window,
>> >> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =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 661d65f..0c2ceaa 100644
>> >> >> --- a/net/bluetooth/hci_core.c
>> >> >> +++ b/net/bluetooth/hci_core.c
>> >> >> @@ -1672,6 +1672,27 @@ static int hci_do_le_scan(struct hci_dev *h=
dev, 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 -EPERM;
>> >> >
>> >> > Are you sure about this -EPERM return error here? At a glance -EALR=
EADY looks
>> >> > better to me.
>> >>
>> >> This function cancels an operation (LE scan). If the operation is not
>> >> running, it makes more sense to me returning "Operation not permitted=
"
>> >> instead of "Operation already in progress".
>> >
>> > actually EPERM is for operation not permitted because you do not have
>> > rights to access is. Not because it is invalid operation.
>> >
>> > You need to find a better error code.
>>
>> Ok, then, as Gustavo suggested, EALREADY looks really better.
>> Are you fine with EALREADY?
>
> seems fine to me.

I just realized hci_cancel_inquiry is returning -EPERM. I'm gonna send
a patch fixing this, so hci_cancel_inquiry returns -EALREADY too.

BR,

Andre