* [PATCH] Bluetooth: Add support for entering limited discoverable mode
@ 2013-10-15 16:13 Marcel Holtmann
2013-10-15 18:34 ` Johan Hedberg
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2013-10-15 16:13 UTC (permalink / raw)
To: linux-bluetooth
The limited discoverable mode should be used when a device is only
discoverable for a certain amount of time and after that it returns
back into being non-discoverable.
This adds another option to the set discoverable management command
to clearly distinguish limited discoverable from general discoverable
mode.
While the general discoverable mode can be set with a specific
timeout or as permanent setting, the limited discoverable mode
requires a timeout. The timeout is flexible and the kernel will
not enforce any specific limitations. That GAP part of this is
required by userspace to enforce according to the Bluetooth core
specification.
Devices in limited discoverable mode can still be found by the
general discovery procedure. It is mandatory that a device sets
both GIAC and LIAC when entering limited discoverable mode.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hci_core.c | 8 ++++++
net/bluetooth/mgmt.c | 67 ++++++++++++++++++++++++++++++++++++++++--------
2 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index a49ca48..7a3d179 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1282,6 +1282,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
cancel_delayed_work(&hdev->discov_off);
hdev->discov_timeout = 0;
clear_bit(HCI_DISCOVERABLE, &hdev->dev_flags);
+ clear_bit(HCI_LIMITED_DISCOVERABLE, &hdev->dev_flags);
}
if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->dev_flags))
@@ -1717,6 +1718,13 @@ static void hci_discov_off(struct work_struct *work)
hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, sizeof(scan), &scan);
hci_req_run(&req, NULL);
+ /* When discoverable timeout triggers, then just make sure
+ * the limited discoverable flag is cleared. Even in the case
+ * of a timeout triggered from general discoverable, it is
+ * safe to unconditionally clear the flag.
+ */
+ clear_bit(HCI_LIMITED_DISCOVERABLE, &hdev->dev_flags);
+
hdev->discov_timeout = 0;
hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1d608ca..c700940 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1039,6 +1039,7 @@ static void set_discoverable_complete(struct hci_dev *hdev, u8 status)
if (status) {
u8 mgmt_err = mgmt_status(status);
cmd_status(cmd->sk, cmd->index, cmd->opcode, mgmt_err);
+ clear_bit(HCI_LIMITED_DISCOVERABLE, &hdev->dev_flags);
goto remove_cmd;
}
@@ -1094,12 +1095,17 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
status);
- if (cp->val != 0x00 && cp->val != 0x01)
+ if (cp->val != 0x00 && cp->val != 0x01 && cp->val != 0x02)
return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
MGMT_STATUS_INVALID_PARAMS);
timeout = __le16_to_cpu(cp->timeout);
- if (!cp->val && timeout > 0)
+
+ /* Disabling discoverable requires that no timeout is set,
+ * and enabling limited discoverable requires a timeout.
+ */
+ if ((cp->val == 0x00 && timeout > 0) ||
+ (cp->val == 0x02 && timeout == 0))
return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
MGMT_STATUS_INVALID_PARAMS);
@@ -1127,6 +1133,10 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
if (!hdev_is_powered(hdev)) {
bool changed = false;
+ /* Setting limited discoverable when powered off is
+ * not a valid operation since it requires a timeout
+ * and so no need to check HCI_LIMITED_DISCOVERABLE.
+ */
if (!!cp->val != test_bit(HCI_DISCOVERABLE, &hdev->dev_flags)) {
change_bit(HCI_DISCOVERABLE, &hdev->dev_flags);
changed = true;
@@ -1142,7 +1152,13 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
goto failed;
}
- if (!!cp->val == test_bit(HCI_DISCOVERABLE, &hdev->dev_flags)) {
+ /* If the current mode is the same, then just update the timeout
+ * value with the new value. And if only the timeout gets updated,
+ * then no need for any HCI transactions.
+ */
+ if (!!cp->val == test_bit(HCI_DISCOVERABLE, &hdev->dev_flags) &&
+ (cp->val == 0x02) == test_bit(HCI_LIMITED_DISCOVERABLE,
+ &hdev->dev_flags)) {
cancel_delayed_work(&hdev->discov_off);
hdev->discov_timeout = timeout;
@@ -1162,24 +1178,55 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
goto failed;
}
+ /* Cancel any potential discoverable timeout that might be
+ * still active and store new timeout value. The arming of
+ * the timeout happens in the complete handler.
+ */
+ cancel_delayed_work(&hdev->discov_off);
+ hdev->discov_timeout = timeout;
+
hci_req_init(&req, hdev);
scan = SCAN_PAGE;
- if (cp->val)
+ if (cp->val) {
+ struct hci_cp_write_current_iac_lap hci_cp;
+
+ if (cp->val == 0x02) {
+ /* Limited discoverable mode */
+ set_bit(HCI_LIMITED_DISCOVERABLE, &hdev->dev_flags);
+
+ hci_cp.num_iac = 2;
+ hci_cp.iac_lap[0] = 0x00; /* LIAC */
+ hci_cp.iac_lap[1] = 0x8b;
+ hci_cp.iac_lap[2] = 0x9e;
+ hci_cp.iac_lap[3] = 0x33; /* GIAC */
+ hci_cp.iac_lap[4] = 0x8b;
+ hci_cp.iac_lap[5] = 0x9e;
+ } else {
+ /* General discoverable mode */
+ clear_bit(HCI_LIMITED_DISCOVERABLE, &hdev->dev_flags);
+
+ hci_cp.num_iac = 1;
+ hci_cp.iac_lap[0] = 0x33; /* GIAC */
+ hci_cp.iac_lap[1] = 0x8b;
+ hci_cp.iac_lap[2] = 0x9e;
+ }
+
+ hci_req_add(&req, HCI_OP_WRITE_CURRENT_IAC_LAP,
+ (hci_cp.num_iac * 3) + 1, &hci_cp);
+
scan |= SCAN_INQUIRY;
- else
- cancel_delayed_work(&hdev->discov_off);
+ } else {
+ clear_bit(HCI_LIMITED_DISCOVERABLE, &hdev->dev_flags);
+ }
- hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
+ hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, sizeof(scan), &scan);
err = hci_req_run(&req, set_discoverable_complete);
if (err < 0)
mgmt_pending_remove(cmd);
- if (cp->val)
- hdev->discov_timeout = timeout;
-
failed:
hci_dev_unlock(hdev);
return err;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Add support for entering limited discoverable mode
2013-10-15 16:13 [PATCH] Bluetooth: Add support for entering limited discoverable mode Marcel Holtmann
@ 2013-10-15 18:34 ` Johan Hedberg
0 siblings, 0 replies; 10+ messages in thread
From: Johan Hedberg @ 2013-10-15 18:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Tue, Oct 15, 2013, Marcel Holtmann wrote:
> The limited discoverable mode should be used when a device is only
> discoverable for a certain amount of time and after that it returns
> back into being non-discoverable.
>
> This adds another option to the set discoverable management command
> to clearly distinguish limited discoverable from general discoverable
> mode.
>
> While the general discoverable mode can be set with a specific
> timeout or as permanent setting, the limited discoverable mode
> requires a timeout. The timeout is flexible and the kernel will
> not enforce any specific limitations. That GAP part of this is
> required by userspace to enforce according to the Bluetooth core
> specification.
>
> Devices in limited discoverable mode can still be found by the
> general discovery procedure. It is mandatory that a device sets
> both GIAC and LIAC when entering limited discoverable mode.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/hci_core.c | 8 ++++++
> net/bluetooth/mgmt.c | 67 ++++++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 65 insertions(+), 10 deletions(-)
Applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Bluetooth: Add support for entering limited discoverable mode
@ 2013-10-14 23:08 Marcel Holtmann
2013-10-15 7:31 ` Johan Hedberg
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2013-10-14 23:08 UTC (permalink / raw)
To: linux-bluetooth
The limited discoverable mode should be used when a device is only
discoverable for a certain amount of time and after that it returns
back into being non-discoverable.
The management interface allows to specify a timeout parameter when
makeing a device discoverable. If such a timeout is specified or
changed, enter limited discoverable mode. Otherwise enter general
discoverable mode.
Devices in limited discoverable mode can still be found by the
general discovery procedure. It is mandatory that a device sets
both GIAC and LIAC when entering limited discoverable mode.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci.h | 6 ++++++
net/bluetooth/mgmt.c | 41 +++++++++++++++++++++++++++++++++++++----
2 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index b096f5f..ab96f3b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -823,6 +823,12 @@ struct hci_rp_read_num_supported_iac {
#define HCI_OP_READ_CURRENT_IAC_LAP 0x0c39
+#define HCI_OP_WRITE_CURRENT_IAC_LAP 0x0c3a
+struct hci_cp_write_current_iac_lap {
+ __u8 num_iac;
+ __u8 iac_lap[6];
+} __packed;
+
#define HCI_OP_WRITE_INQUIRY_MODE 0x0c45
#define HCI_MAX_EIR_LENGTH 240
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 861e389..cba15da 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1041,6 +1041,8 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (!!cp->val == test_bit(HCI_DISCOVERABLE, &hdev->dev_flags)) {
+ u16 current_timeout = hdev->discov_timeout;
+
if (hdev->discov_timeout > 0) {
cancel_delayed_work(&hdev->discov_off);
hdev->discov_timeout = 0;
@@ -1052,8 +1054,16 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
msecs_to_jiffies(hdev->discov_timeout * 1000));
}
- err = send_settings_rsp(sk, MGMT_OP_SET_DISCOVERABLE, hdev);
- goto failed;
+ /* A change in timeout means that the device is switching
+ * from one discoverable mode to another. In that case
+ * the IAC LAP needs to be changed.
+ */
+ if ((current_timeout > 0 && timeout > 0) ||
+ (current_timeout == 0 && timeout == 0)) {
+ err = send_settings_rsp(sk, MGMT_OP_SET_DISCOVERABLE,
+ hdev);
+ goto failed;
+ }
}
cmd = mgmt_pending_add(sk, MGMT_OP_SET_DISCOVERABLE, hdev, data, len);
@@ -1066,10 +1076,33 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
scan = SCAN_PAGE;
- if (cp->val)
+ if (cp->val) {
+ struct hci_cp_write_current_iac_lap cp;
+
+ if (timeout > 0) {
+ /* Limited discoverable mode */
+ cp.num_iac = 2;
+ cp.iac_lap[0] = 0x00; /* LIAC */
+ cp.iac_lap[1] = 0x8b;
+ cp.iac_lap[2] = 0x9e;
+ cp.iac_lap[3] = 0x33; /* GIAC */
+ cp.iac_lap[4] = 0x8b;
+ cp.iac_lap[5] = 0x9e;
+ } else {
+ /* General discoverable mode */
+ cp.num_iac = 1;
+ cp.iac_lap[0] = 0x33; /* GIAC */
+ cp.iac_lap[1] = 0x8b;
+ cp.iac_lap[2] = 0x9e;
+ }
+
+ hci_req_add(&req, HCI_OP_WRITE_CURRENT_IAC_LAP,
+ (cp.num_iac * 3) + 1, &cp);
+
scan |= SCAN_INQUIRY;
- else
+ } else {
cancel_delayed_work(&hdev->discov_off);
+ }
hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Add support for entering limited discoverable mode
2013-10-14 23:08 Marcel Holtmann
@ 2013-10-15 7:31 ` Johan Hedberg
2013-10-15 7:40 ` Johan Hedberg
2013-10-15 8:52 ` Marcel Holtmann
0 siblings, 2 replies; 10+ messages in thread
From: Johan Hedberg @ 2013-10-15 7:31 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Mon, Oct 14, 2013, Marcel Holtmann wrote:
> + if (cp->val) {
> + struct hci_cp_write_current_iac_lap cp;
> +
> + if (timeout > 0) {
> + /* Limited discoverable mode */
Wouldn't we want to follow the recommendation from the Core spec here,
volume 3, part C, section 4.1.2.1:
"A Bluetooth device should not be in limited discoverable mode for more than
TGAP(104)"
TGAP(104) in turn is defined as 1 minute (vol 3, part C, Appendix A).
Johan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Add support for entering limited discoverable mode
2013-10-15 7:31 ` Johan Hedberg
@ 2013-10-15 7:40 ` Johan Hedberg
2013-10-15 8:52 ` Marcel Holtmann
1 sibling, 0 replies; 10+ messages in thread
From: Johan Hedberg @ 2013-10-15 7:40 UTC (permalink / raw)
To: Marcel Holtmann, linux-bluetooth
Hi,
On Tue, Oct 15, 2013, Johan Hedberg wrote:
> On Mon, Oct 14, 2013, Marcel Holtmann wrote:
> > + if (cp->val) {
> > + struct hci_cp_write_current_iac_lap cp;
> > +
> > + if (timeout > 0) {
> > + /* Limited discoverable mode */
>
> Wouldn't we want to follow the recommendation from the Core spec here,
> volume 3, part C, section 4.1.2.1:
>
> "A Bluetooth device should not be in limited discoverable mode for more than
> TGAP(104)"
>
> TGAP(104) in turn is defined as 1 minute (vol 3, part C, Appendix A).
We might even want to limit this to 30 seconds so the same can be used
for LE once this is hooked up to advertising:
TGAP(lim_adv_timeout) 30.72s Maximum time to remain Required value
advertising when in the
limited discoverable mode
Johan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Add support for entering limited discoverable mode
2013-10-15 7:31 ` Johan Hedberg
2013-10-15 7:40 ` Johan Hedberg
@ 2013-10-15 8:52 ` Marcel Holtmann
2013-10-15 9:16 ` Johan Hedberg
1 sibling, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2013-10-15 8:52 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
Hi Johan,
>> + if (cp->val) {
>> + struct hci_cp_write_current_iac_lap cp;
>> +
>> + if (timeout > 0) {
>> + /* Limited discoverable mode */
>
> Wouldn't we want to follow the recommendation from the Core spec here,
> volume 3, part C, section 4.1.2.1:
>
> "A Bluetooth device should not be in limited discoverable mode for more than
> TGAP(104)"
>
> TGAP(104) in turn is defined as 1 minute (vol 3, part C, Appendix A).
I was not going to be this strict. So yes, userspace can violate the GAP part here, but it might be a good idea to ensure that when a too large value is entered we stick with general discoverable mode. However is this really worth it.
Another option is to just reject too large timeout values as invalid parameters and force userspace to implement large timeouts with general discoverable mode in userspace.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Add support for entering limited discoverable mode
2013-10-15 8:52 ` Marcel Holtmann
@ 2013-10-15 9:16 ` Johan Hedberg
2013-10-15 9:30 ` Marcel Holtmann
0 siblings, 1 reply; 10+ messages in thread
From: Johan Hedberg @ 2013-10-15 9:16 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Tue, Oct 15, 2013, Marcel Holtmann wrote:
> >> + if (cp->val) {
> >> + struct hci_cp_write_current_iac_lap cp;
> >> +
> >> + if (timeout > 0) {
> >> + /* Limited discoverable mode */
> >
> > Wouldn't we want to follow the recommendation from the Core spec here,
> > volume 3, part C, section 4.1.2.1:
> >
> > "A Bluetooth device should not be in limited discoverable mode for more than
> > TGAP(104)"
> >
> > TGAP(104) in turn is defined as 1 minute (vol 3, part C, Appendix A).
>
> I was not going to be this strict. So yes, userspace can violate the
> GAP part here, but it might be a good idea to ensure that when a too
> large value is entered we stick with general discoverable mode.
> However is this really worth it.
For Advertising the max value is actually not just a recommendation but
a requirement (check my other mail), so at least there we should
probably enforce it. And if we have to enforce it for LE we might as
well do it for BR/EDR too. It's anyway a rather simple change to the
if-statement.
> Another option is to just reject too large timeout values as invalid
> parameters and force userspace to implement large timeouts with
> general discoverable mode in userspace.
I don't think it's a good idea to force this kind of special handling to
user space (not just the requirement to know where the limits go but
also the need to implement a separate timer).
Johan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Add support for entering limited discoverable mode
2013-10-15 9:16 ` Johan Hedberg
@ 2013-10-15 9:30 ` Marcel Holtmann
2013-10-15 9:49 ` Johan Hedberg
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2013-10-15 9:30 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
Hi Johan,
>>>> + if (cp->val) {
>>>> + struct hci_cp_write_current_iac_lap cp;
>>>> +
>>>> + if (timeout > 0) {
>>>> + /* Limited discoverable mode */
>>>
>>> Wouldn't we want to follow the recommendation from the Core spec here,
>>> volume 3, part C, section 4.1.2.1:
>>>
>>> "A Bluetooth device should not be in limited discoverable mode for more than
>>> TGAP(104)"
>>>
>>> TGAP(104) in turn is defined as 1 minute (vol 3, part C, Appendix A).
>>
>> I was not going to be this strict. So yes, userspace can violate the
>> GAP part here, but it might be a good idea to ensure that when a too
>> large value is entered we stick with general discoverable mode.
>> However is this really worth it.
>
> For Advertising the max value is actually not just a recommendation but
> a requirement (check my other mail), so at least there we should
> probably enforce it. And if we have to enforce it for LE we might as
> well do it for BR/EDR too. It's anyway a rather simple change to the
> if-statement.
>
>> Another option is to just reject too large timeout values as invalid
>> parameters and force userspace to implement large timeouts with
>> general discoverable mode in userspace.
>
> I don't think it's a good idea to force this kind of special handling to
> user space (not just the requirement to know where the limits go but
> also the need to implement a separate timer).
This sounds also a bit like black magic. Userspace needs to know that a small value enters limited discoverable mode and a large one sticks with general discoverable mode. We might should have added a flag to the command in the first place. However I think our general intention was that with a timeout we do limited discoverable mode and without a timeout we do general discoverable mode.
I was complaining on why we bothered with a timeout for this parameter when building src/share/mgmt.[ch] and cleaning up some of the core adapter handling. Mainly because for PairableTimeout D-Bus property we have to run our own timer and for DiscoverableTimeout property we rely on the kernel. And in the end just running your own timeout was a lot simpler, then integrating with the kernel.
So I would be actually fine just saying with timeout that is limited discoverable mode. If you want general discoverable mode with a timeout, then you have to run this by yourself.
Then again, nothing stops us from introducing mode 0x02 for limited discoverable if you feel that we should keep the general discoverable mode with a timeout.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Add support for entering limited discoverable mode
2013-10-15 9:30 ` Marcel Holtmann
@ 2013-10-15 9:49 ` Johan Hedberg
2013-10-15 10:02 ` Marcel Holtmann
0 siblings, 1 reply; 10+ messages in thread
From: Johan Hedberg @ 2013-10-15 9:49 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Tue, Oct 15, 2013, Marcel Holtmann wrote:
> >>>> + if (cp->val) {
> >>>> + struct hci_cp_write_current_iac_lap cp;
> >>>> +
> >>>> + if (timeout > 0) {
> >>>> + /* Limited discoverable mode */
> >>>
> >>> Wouldn't we want to follow the recommendation from the Core spec
> >>> here, volume 3, part C, section 4.1.2.1:
> >>>
> >>> "A Bluetooth device should not be in limited discoverable mode for
> >>> more than TGAP(104)"
> >>>
> >>> TGAP(104) in turn is defined as 1 minute (vol 3, part C, Appendix A).
> >>
> >> I was not going to be this strict. So yes, userspace can violate
> >> the GAP part here, but it might be a good idea to ensure that when
> >> a too large value is entered we stick with general discoverable
> >> mode. However is this really worth it.
> >
> > For Advertising the max value is actually not just a recommendation
> > but a requirement (check my other mail), so at least there we should
> > probably enforce it. And if we have to enforce it for LE we might as
> > well do it for BR/EDR too. It's anyway a rather simple change to the
> > if-statement.
> >
> >> Another option is to just reject too large timeout values as
> >> invalid parameters and force userspace to implement large timeouts
> >> with general discoverable mode in userspace.
> >
> > I don't think it's a good idea to force this kind of special
> > handling to user space (not just the requirement to know where the
> > limits go but also the need to implement a separate timer).
>
> This sounds also a bit like black magic. Userspace needs to know that
> a small value enters limited discoverable mode and a large one sticks
> with general discoverable mode. We might should have added a flag to
> the command in the first place. However I think our general intention
> was that with a timeout we do limited discoverable mode and without a
> timeout we do general discoverable mode.
>
> I was complaining on why we bothered with a timeout for this parameter
> when building src/share/mgmt.[ch] and cleaning up some of the core
> adapter handling. Mainly because for PairableTimeout D-Bus property we
> have to run our own timer and for DiscoverableTimeout property we rely
> on the kernel. And in the end just running your own timeout was a lot
> simpler, then integrating with the kernel.
>
> So I would be actually fine just saying with timeout that is limited
> discoverable mode. If you want general discoverable mode with a
> timeout, then you have to run this by yourself.
Fair enough, and I really don't want to complicate this more by adding a
new 0x02 value either. The patch has now been pushed to bluetooth-next.
Johan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Add support for entering limited discoverable mode
2013-10-15 9:49 ` Johan Hedberg
@ 2013-10-15 10:02 ` Marcel Holtmann
0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2013-10-15 10:02 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
Hi Johan,
>>>>>> + if (cp->val) {
>>>>>> + struct hci_cp_write_current_iac_lap cp;
>>>>>> +
>>>>>> + if (timeout > 0) {
>>>>>> + /* Limited discoverable mode */
>>>>>
>>>>> Wouldn't we want to follow the recommendation from the Core spec
>>>>> here, volume 3, part C, section 4.1.2.1:
>>>>>
>>>>> "A Bluetooth device should not be in limited discoverable mode for
>>>>> more than TGAP(104)"
>>>>>
>>>>> TGAP(104) in turn is defined as 1 minute (vol 3, part C, Appendix A).
>>>>
>>>> I was not going to be this strict. So yes, userspace can violate
>>>> the GAP part here, but it might be a good idea to ensure that when
>>>> a too large value is entered we stick with general discoverable
>>>> mode. However is this really worth it.
>>>
>>> For Advertising the max value is actually not just a recommendation
>>> but a requirement (check my other mail), so at least there we should
>>> probably enforce it. And if we have to enforce it for LE we might as
>>> well do it for BR/EDR too. It's anyway a rather simple change to the
>>> if-statement.
>>>
>>>> Another option is to just reject too large timeout values as
>>>> invalid parameters and force userspace to implement large timeouts
>>>> with general discoverable mode in userspace.
>>>
>>> I don't think it's a good idea to force this kind of special
>>> handling to user space (not just the requirement to know where the
>>> limits go but also the need to implement a separate timer).
>>
>> This sounds also a bit like black magic. Userspace needs to know that
>> a small value enters limited discoverable mode and a large one sticks
>> with general discoverable mode. We might should have added a flag to
>> the command in the first place. However I think our general intention
>> was that with a timeout we do limited discoverable mode and without a
>> timeout we do general discoverable mode.
>>
>> I was complaining on why we bothered with a timeout for this parameter
>> when building src/share/mgmt.[ch] and cleaning up some of the core
>> adapter handling. Mainly because for PairableTimeout D-Bus property we
>> have to run our own timer and for DiscoverableTimeout property we rely
>> on the kernel. And in the end just running your own timeout was a lot
>> simpler, then integrating with the kernel.
>>
>> So I would be actually fine just saying with timeout that is limited
>> discoverable mode. If you want general discoverable mode with a
>> timeout, then you have to run this by yourself.
>
> Fair enough, and I really don't want to complicate this more by adding a
> new 0x02 value either. The patch has now been pushed to bluetooth-next.
and I was tending towards reworking the patch to add a new 0x02 mode. In that mode we enforce the correct timeout values ahead of time.
I am no longer convinced that we are doing the right thing here. I was 100% sure yesterday and today, I would start exploring other options.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-10-15 18:34 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-15 16:13 [PATCH] Bluetooth: Add support for entering limited discoverable mode Marcel Holtmann
2013-10-15 18:34 ` Johan Hedberg
-- strict thread matches above, loose matches on Subject: below --
2013-10-14 23:08 Marcel Holtmann
2013-10-15 7:31 ` Johan Hedberg
2013-10-15 7:40 ` Johan Hedberg
2013-10-15 8:52 ` Marcel Holtmann
2013-10-15 9:16 ` Johan Hedberg
2013-10-15 9:30 ` Marcel Holtmann
2013-10-15 9:49 ` Johan Hedberg
2013-10-15 10:02 ` Marcel Holtmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox