* [PATCH 0/8] Bluetooth: Cleanups and LE advertising support
@ 2013-09-24 14:02 johan.hedberg
2013-09-24 14:02 ` [PATCH 1/8] Bluetooth: Add clarifying comment to bt_sock_wait_state() johan.hedberg
` (7 more replies)
0 siblings, 8 replies; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
Hi,
This patch set is a combination of cleanups that I had in my tree
(patches 1-3) a fix for set_powered (patch 4) and a new mgmt setting to
allow enabling LE advertising (patches 5-8). What's still open is
whether we want to rename the internal HCI_LE_PERIPHERAL flag to match
the "advertising" name used for the mgmt setting, however that can be
done as an independent patch on top of this set if necessary.
I've tested this through my own additions to user space btmgmt and
mgmt-tester tools. Once the general API is acked I'll push those
additions to bluez.git.
Johan
----------------------------------------------------------------
Johan Hedberg (8):
Bluetooth: Add clarifying comment to bt_sock_wait_state()
Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
Bluetooth: Fix busy return for mgmt_set_powered in some cases
Bluetooth: Move mgmt response convenience functions to a better location
Bluetooth: Use async request for LE enable/disable
Bluetooth: Add new mgmt setting for LE advertising
Bluetooth: Add new mgmt_set_advertising command
include/net/bluetooth/hci_core.h | 1 -
include/net/bluetooth/mgmt.h | 3 +
net/bluetooth/af_bluetooth.c | 1 +
net/bluetooth/hci_core.c | 4 +-
net/bluetooth/hci_event.c | 11 --
net/bluetooth/l2cap_sock.c | 5 +-
net/bluetooth/mgmt.c | 276 ++++++++++++++++++++++++++------------
7 files changed, 199 insertions(+), 102 deletions(-)
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/8] Bluetooth: Add clarifying comment to bt_sock_wait_state()
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
@ 2013-09-24 14:02 ` johan.hedberg
2013-09-24 18:11 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 2/8] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg johan.hedberg
` (6 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
The bt_sock_wait_state requires the sk lock to be held (through
lock_sock) so document it clearly in the code.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/af_bluetooth.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index c600631..e6e1278 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -490,6 +490,7 @@ int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
}
EXPORT_SYMBOL(bt_sock_ioctl);
+/* This function expects the sk lock to be held when called */
int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
{
DECLARE_WAITQUEUE(wait, current);
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/8] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
2013-09-24 14:02 ` [PATCH 1/8] Bluetooth: Add clarifying comment to bt_sock_wait_state() johan.hedberg
@ 2013-09-24 14:02 ` johan.hedberg
2013-09-24 18:16 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev() johan.hedberg
` (5 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
This patch cleans up the locking login in l2cap_sock_recvmsg by pairing
up each lock_sock call with a release_sock call. The function already
has a "done" label that handles releasing the socket and returning from
the function so the fix is rather simple.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_sock.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ad95b42..5853c1e 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -795,7 +795,7 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
{
struct sock *sk = sock->sk;
struct l2cap_pinfo *pi = l2cap_pi(sk);
- int err;
+ int err = 0;
lock_sock(sk);
@@ -805,8 +805,7 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
pi->chan->state = BT_CONFIG;
__l2cap_connect_rsp_defer(pi->chan);
- release_sock(sk);
- return 0;
+ goto done;
}
release_sock(sk);
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
2013-09-24 14:02 ` [PATCH 1/8] Bluetooth: Add clarifying comment to bt_sock_wait_state() johan.hedberg
2013-09-24 14:02 ` [PATCH 2/8] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg johan.hedberg
@ 2013-09-24 14:02 ` johan.hedberg
2013-09-24 18:19 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 4/8] Bluetooth: Fix busy return for mgmt_set_powered in some cases johan.hedberg
` (4 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
If either one of the HCI_SETUP or HCI_USER_CHANNEL flags is set the
device is not considered valid for mgmt. By having these checks inside
the mgmt_valid_hdev function the a couple of places using it can be
simplified.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/hci_core.c | 4 +---
net/bluetooth/mgmt.c | 12 ++++++------
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3d9f02b..1940147 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1238,9 +1238,7 @@ int hci_dev_open(__u16 dev)
hci_dev_hold(hdev);
set_bit(HCI_UP, &hdev->flags);
hci_notify(hdev, HCI_DEV_UP);
- if (!test_bit(HCI_SETUP, &hdev->dev_flags) &&
- !test_bit(HCI_USER_CHANNEL, &hdev->dev_flags) &&
- mgmt_valid_hdev(hdev)) {
+ if (mgmt_valid_hdev(hdev)) {
hci_dev_lock(hdev);
mgmt_powered(hdev, 1);
hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3070e77..5319a94 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -183,6 +183,12 @@ static u8 mgmt_status_table[] = {
bool mgmt_valid_hdev(struct hci_dev *hdev)
{
+ if (test_bit(HCI_SETUP, &hdev->dev_flags))
+ return false;
+
+ if (test_bit(HCI_USER_CHANNEL, &hdev->dev_flags))
+ return false;
+
return hdev->dev_type == HCI_BREDR;
}
@@ -336,12 +342,6 @@ static int read_index_list(struct sock *sk, struct hci_dev *hdev, void *data,
count = 0;
list_for_each_entry(d, &hci_dev_list, list) {
- if (test_bit(HCI_SETUP, &d->dev_flags))
- continue;
-
- if (test_bit(HCI_USER_CHANNEL, &d->dev_flags))
- continue;
-
if (!mgmt_valid_hdev(d))
continue;
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 4/8] Bluetooth: Fix busy return for mgmt_set_powered in some cases
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
` (2 preceding siblings ...)
2013-09-24 14:02 ` [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev() johan.hedberg
@ 2013-09-24 14:02 ` johan.hedberg
2013-09-24 18:17 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 5/8] Bluetooth: Move mgmt response convenience functions to a better location johan.hedberg
` (3 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
We should return a "busy" error always when there is another
mgmt_set_powered operation in progress. Previously when powering on
while the auto off timer was still set the code could have let two or
more pending power on commands to be queued. This patch fixes the issue
by moving the check for duplicate commands to an earlier point in the
set_powered handler.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/mgmt.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5319a94..2a91346 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -807,6 +807,12 @@ static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
hci_dev_lock(hdev);
+ if (mgmt_pending_find(MGMT_OP_SET_POWERED, hdev)) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_SET_POWERED,
+ MGMT_STATUS_BUSY);
+ goto failed;
+ }
+
if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags)) {
cancel_delayed_work(&hdev->power_off);
@@ -823,12 +829,6 @@ static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
goto failed;
}
- if (mgmt_pending_find(MGMT_OP_SET_POWERED, hdev)) {
- err = cmd_status(sk, hdev->id, MGMT_OP_SET_POWERED,
- MGMT_STATUS_BUSY);
- goto failed;
- }
-
cmd = mgmt_pending_add(sk, MGMT_OP_SET_POWERED, hdev, data, len);
if (!cmd) {
err = -ENOMEM;
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 5/8] Bluetooth: Move mgmt response convenience functions to a better location
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
` (3 preceding siblings ...)
2013-09-24 14:02 ` [PATCH 4/8] Bluetooth: Fix busy return for mgmt_set_powered in some cases johan.hedberg
@ 2013-09-24 14:02 ` johan.hedberg
2013-09-24 18:11 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 6/8] Bluetooth: Use async request for LE enable/disable johan.hedberg
` (2 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
The settings_rsp and cmd_status_rsp functions can be useful for all mgmt
command handlers when asynchronous request callbacks are used. They will
e.g. be used by subsequent patches to change set_le to use an async
request as well as a new set_advertising command. Therefore, move them
higher up in the mgmt.c file to avoid unnecessary forward declarations
or mixing this trivial change with other patches.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/mgmt.c | 60 ++++++++++++++++++++++++++--------------------------
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 2a91346..47f4c07 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -886,6 +886,36 @@ static int new_settings(struct hci_dev *hdev, struct sock *skip)
return mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev), skip);
}
+struct cmd_lookup {
+ struct sock *sk;
+ struct hci_dev *hdev;
+ u8 mgmt_status;
+};
+
+static void settings_rsp(struct pending_cmd *cmd, void *data)
+{
+ struct cmd_lookup *match = data;
+
+ send_settings_rsp(cmd->sk, cmd->opcode, match->hdev);
+
+ list_del(&cmd->list);
+
+ if (match->sk == NULL) {
+ match->sk = cmd->sk;
+ sock_hold(match->sk);
+ }
+
+ mgmt_pending_free(cmd);
+}
+
+static void cmd_status_rsp(struct pending_cmd *cmd, void *data)
+{
+ u8 *status = data;
+
+ cmd_status(cmd->sk, cmd->index, cmd->opcode, *status);
+ mgmt_pending_remove(cmd);
+}
+
static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
u16 len)
{
@@ -3374,14 +3404,6 @@ done:
return err;
}
-static void cmd_status_rsp(struct pending_cmd *cmd, void *data)
-{
- u8 *status = data;
-
- cmd_status(cmd->sk, cmd->index, cmd->opcode, *status);
- mgmt_pending_remove(cmd);
-}
-
int mgmt_index_added(struct hci_dev *hdev)
{
if (!mgmt_valid_hdev(hdev))
@@ -3402,28 +3424,6 @@ int mgmt_index_removed(struct hci_dev *hdev)
return mgmt_event(MGMT_EV_INDEX_REMOVED, hdev, NULL, 0, NULL);
}
-struct cmd_lookup {
- struct sock *sk;
- struct hci_dev *hdev;
- u8 mgmt_status;
-};
-
-static void settings_rsp(struct pending_cmd *cmd, void *data)
-{
- struct cmd_lookup *match = data;
-
- send_settings_rsp(cmd->sk, cmd->opcode, match->hdev);
-
- list_del(&cmd->list);
-
- if (match->sk == NULL) {
- match->sk = cmd->sk;
- sock_hold(match->sk);
- }
-
- mgmt_pending_free(cmd);
-}
-
static void set_bredr_scan(struct hci_request *req)
{
struct hci_dev *hdev = req->hdev;
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 6/8] Bluetooth: Use async request for LE enable/disable
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
` (4 preceding siblings ...)
2013-09-24 14:02 ` [PATCH 5/8] Bluetooth: Move mgmt response convenience functions to a better location johan.hedberg
@ 2013-09-24 14:02 ` johan.hedberg
2013-09-24 18:20 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising johan.hedberg
2013-09-24 14:02 ` [PATCH 8/8] Bluetooth: Add new mgmt_set_advertising command johan.hedberg
7 siblings, 1 reply; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
This patch updates the code to use an asynchronous request for handling
the enabling and disabling of LE support. This refactoring is necessary
as a preparation for adding advertising support, since when LE is
disabled we should also disable advertising, and the cleanest way to do
this is to perform the two respective HCI commands in the same
asynchronous request.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/hci_core.h | 1 -
net/bluetooth/hci_event.c | 11 -------
net/bluetooth/mgmt.c | 69 +++++++++++++++++-----------------------
3 files changed, 29 insertions(+), 52 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3ede820..26cc9f7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1168,7 +1168,6 @@ int mgmt_set_class_of_dev_complete(struct hci_dev *hdev, u8 *dev_class,
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_le_enable_complete(struct hci_dev *hdev, u8 enable, 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 cfm_name,
u8 ssp, u8 *eir, u16 eir_len);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 94aab73..835a384 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -911,13 +911,6 @@ static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
hci_dev_lock(hdev);
- if (!status) {
- if (*sent)
- set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
- else
- clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
- }
-
if (!test_bit(HCI_INIT, &hdev->flags)) {
struct hci_request req;
@@ -1004,10 +997,6 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
else
hdev->features[1][0] &= ~LMP_HOST_LE_BREDR;
}
-
- if (test_bit(HCI_MGMT, &hdev->dev_flags) &&
- !test_bit(HCI_INIT, &hdev->flags))
- mgmt_le_enable_complete(hdev, sent->le, status);
}
static void hci_cc_write_remote_amp_assoc(struct hci_dev *hdev,
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 47f4c07..e595b59 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1354,11 +1354,34 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
return send_settings_rsp(sk, MGMT_OP_SET_HS, hdev);
}
+static void le_enable_complete(struct hci_dev *hdev, u8 status)
+{
+ struct cmd_lookup match = { NULL, hdev };
+
+ if (status) {
+ u8 mgmt_err = mgmt_status(status);
+
+ mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, cmd_status_rsp,
+ &mgmt_err);
+ return;
+ }
+
+ change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
+
+ mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, settings_rsp, &match);
+
+ new_settings(hdev, match.sk);
+
+ if (match.sk)
+ sock_put(match.sk);
+}
+
static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
{
struct mgmt_mode *cp = data;
struct hci_cp_write_le_host_supported hci_cp;
struct pending_cmd *cmd;
+ struct hci_request req;
int err;
u8 val, enabled;
@@ -1419,8 +1442,12 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
hci_cp.simul = lmp_le_br_capable(hdev);
}
- err = hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(hci_cp),
- &hci_cp);
+ hci_req_init(&req, hdev);
+
+ hci_req_add(&req, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(hci_cp),
+ &hci_cp);
+
+ err = hci_req_run(&req, le_enable_complete);
if (err < 0)
mgmt_pending_remove(cmd);
@@ -4141,44 +4168,6 @@ int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
return err;
}
-int mgmt_le_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
-{
- struct cmd_lookup match = { NULL, hdev };
- bool changed = false;
- int err = 0;
-
- if (status) {
- u8 mgmt_err = mgmt_status(status);
-
- if (enable && test_and_clear_bit(HCI_LE_ENABLED,
- &hdev->dev_flags))
- err = new_settings(hdev, NULL);
-
- mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, cmd_status_rsp,
- &mgmt_err);
-
- return err;
- }
-
- if (enable) {
- if (!test_and_set_bit(HCI_LE_ENABLED, &hdev->dev_flags))
- changed = true;
- } else {
- if (test_and_clear_bit(HCI_LE_ENABLED, &hdev->dev_flags))
- changed = true;
- }
-
- mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, settings_rsp, &match);
-
- if (changed)
- err = new_settings(hdev, match.sk);
-
- if (match.sk)
- sock_put(match.sk);
-
- return err;
-}
-
int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name, u8
ssp, u8 *eir, u16 eir_len)
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
` (5 preceding siblings ...)
2013-09-24 14:02 ` [PATCH 6/8] Bluetooth: Use async request for LE enable/disable johan.hedberg
@ 2013-09-24 14:02 ` johan.hedberg
2013-09-24 15:45 ` Anderson Lizardo
2013-09-24 18:29 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 8/8] Bluetooth: Add new mgmt_set_advertising command johan.hedberg
7 siblings, 2 replies; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
This patch adds a new mgmt setting for LE advertising and hooks up the
necessary places in the mgmt code to operate on the HCI_LE_PERIPHERAL
flag (which corresponds to this setting). This patch does not yet add
any new command for enabling the setting - that is left for a subsequent
patch.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/mgmt.h | 1 +
net/bluetooth/mgmt.c | 24 +++++++++++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 9944c3e..6cc72b6 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -93,6 +93,7 @@ struct mgmt_rp_read_index_list {
#define MGMT_SETTING_BREDR 0x00000080
#define MGMT_SETTING_HS 0x00000100
#define MGMT_SETTING_LE 0x00000200
+#define MGMT_SETTING_ADVERTISING 0x00000400
#define MGMT_OP_READ_INFO 0x0004
#define MGMT_READ_INFO_SIZE 0
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e595b59..d11899d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -384,8 +384,10 @@ static u32 get_supported_settings(struct hci_dev *hdev)
if (enable_hs)
settings |= MGMT_SETTING_HS;
- if (lmp_le_capable(hdev))
+ if (lmp_le_capable(hdev)) {
settings |= MGMT_SETTING_LE;
+ settings |= MGMT_SETTING_ADVERTISING;
+ }
return settings;
}
@@ -424,6 +426,9 @@ static u32 get_current_settings(struct hci_dev *hdev)
if (test_bit(HCI_HS_ENABLED, &hdev->dev_flags))
settings |= MGMT_SETTING_HS;
+ if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
+ settings |= MGMT_SETTING_ADVERTISING;
+
return settings;
}
@@ -1368,6 +1373,9 @@ static void le_enable_complete(struct hci_dev *hdev, u8 status)
change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
+ if (!test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
+ clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+
mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, settings_rsp, &match);
new_settings(hdev, match.sk);
@@ -1413,6 +1421,11 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
changed = true;
}
+ if (!val && test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags)) {
+ clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+ changed = true;
+ }
+
err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
if (err < 0)
goto unlock;
@@ -1444,6 +1457,9 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
hci_req_init(&req, hdev);
+ if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags) && !val)
+ hci_req_add(&req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(val), &val);
+
hci_req_add(&req, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(hci_cp),
&hci_cp);
@@ -3519,6 +3535,12 @@ static int powered_update_hci(struct hci_dev *hdev)
sizeof(cp), &cp);
}
+ if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags)) {
+ u8 adv = 0x01;
+
+ hci_req_add(&req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(adv), &adv);
+ }
+
link_sec = test_bit(HCI_LINK_SECURITY, &hdev->dev_flags);
if (link_sec != test_bit(HCI_AUTH, &hdev->flags))
hci_req_add(&req, HCI_OP_WRITE_AUTH_ENABLE,
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 8/8] Bluetooth: Add new mgmt_set_advertising command
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
` (6 preceding siblings ...)
2013-09-24 14:02 ` [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising johan.hedberg
@ 2013-09-24 14:02 ` johan.hedberg
2013-09-24 15:42 ` Johan Hedberg
7 siblings, 1 reply; 25+ messages in thread
From: johan.hedberg @ 2013-09-24 14:02 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
This patch adds a new mgmt command for enabling and disabling
LE advertising.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/mgmt.h | 2 +
net/bluetooth/mgmt.c | 99 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 100 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 6cc72b6..421d763 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -352,6 +352,8 @@ struct mgmt_cp_set_device_id {
} __packed;
#define MGMT_SET_DEVICE_ID_SIZE 8
+#define MGMT_OP_SET_ADVERTISING 0x0029
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d11899d..4f5b0f3 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -76,6 +76,7 @@ static const u16 mgmt_commands[] = {
MGMT_OP_BLOCK_DEVICE,
MGMT_OP_UNBLOCK_DEVICE,
MGMT_OP_SET_DEVICE_ID,
+ MGMT_OP_SET_ADVERTISING,
};
static const u16 mgmt_events[] = {
@@ -1436,7 +1437,8 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
goto unlock;
}
- if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
+ if (mgmt_pending_find(MGMT_OP_SET_LE, hdev) ||
+ mgmt_pending_find(MGMT_OP_SET_ADVERTISING, hdev)) {
err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
MGMT_STATUS_BUSY);
goto unlock;
@@ -3141,6 +3143,100 @@ static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data,
return err;
}
+static void set_advertising_complete(struct hci_dev *hdev, u8 status)
+{
+ struct cmd_lookup match = { NULL, hdev };
+
+ if (status) {
+ u8 mgmt_err = mgmt_status(status);
+
+ mgmt_pending_foreach(MGMT_OP_SET_ADVERTISING, hdev,
+ cmd_status_rsp, &mgmt_err);
+ return;
+ }
+
+ change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+
+ mgmt_pending_foreach(MGMT_OP_SET_ADVERTISING, hdev, settings_rsp,
+ &match);
+
+ new_settings(hdev, match.sk);
+
+ if (match.sk)
+ sock_put(match.sk);
+}
+
+static int set_advertising(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
+{
+ struct mgmt_mode *cp = data;
+ struct pending_cmd *cmd;
+ struct hci_request req;
+ u8 val, enabled;
+ int err;
+
+ BT_DBG("request for %s", hdev->name);
+
+ if (!lmp_le_capable(hdev))
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
+ MGMT_STATUS_NOT_SUPPORTED);
+
+ if (!test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
+ MGMT_STATUS_REJECTED);
+
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
+ MGMT_STATUS_INVALID_PARAMS);
+
+ hci_dev_lock(hdev);
+
+ val = !!cp->val;
+ enabled = test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+
+ if (!hdev_is_powered(hdev) || val == enabled) {
+ bool changed = false;
+
+ if (val != test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags)) {
+ change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+ changed = true;
+ }
+
+ err = send_settings_rsp(sk, MGMT_OP_SET_ADVERTISING, hdev);
+ if (err < 0)
+ goto unlock;
+
+ if (changed)
+ err = new_settings(hdev, sk);
+
+ goto unlock;
+ }
+
+ if (mgmt_pending_find(MGMT_OP_SET_ADVERTISING, hdev) ||
+ mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
+ MGMT_STATUS_BUSY);
+ goto unlock;
+ }
+
+ cmd = mgmt_pending_add(sk, MGMT_OP_SET_ADVERTISING, hdev, data, len);
+ if (!cmd) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ hci_req_init(&req, hdev);
+
+ hci_req_add(&req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(val), &val);
+
+ err = hci_req_run(&req, set_advertising_complete);
+ if (err < 0)
+ mgmt_pending_remove(cmd);
+
+unlock:
+ hci_dev_unlock(hdev);
+ return err;
+}
+
static void fast_connectable_complete(struct hci_dev *hdev, u8 status)
{
struct pending_cmd *cmd;
@@ -3352,6 +3448,7 @@ static const struct mgmt_handler {
{ block_device, false, MGMT_BLOCK_DEVICE_SIZE },
{ unblock_device, false, MGMT_UNBLOCK_DEVICE_SIZE },
{ set_device_id, false, MGMT_SET_DEVICE_ID_SIZE },
+ { set_advertising, false, MGMT_SETTING_SIZE },
};
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 8/8] Bluetooth: Add new mgmt_set_advertising command
2013-09-24 14:02 ` [PATCH 8/8] Bluetooth: Add new mgmt_set_advertising command johan.hedberg
@ 2013-09-24 15:42 ` Johan Hedberg
0 siblings, 0 replies; 25+ messages in thread
From: Johan Hedberg @ 2013-09-24 15:42 UTC (permalink / raw)
To: linux-bluetooth
Hi,
On Tue, Sep 24, 2013, johan.hedberg@gmail.com wrote:
> This patch adds a new mgmt command for enabling and disabling
> LE advertising.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/mgmt.h | 2 +
> net/bluetooth/mgmt.c | 99 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 100 insertions(+), 1 deletion(-)
Apologies for this really short commit message. I was going to fill this
in before sending but seems I forgot. I'll fix this for the next
revision but I'll first wait at least for some feedback before that.
Johan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising
2013-09-24 14:02 ` [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising johan.hedberg
@ 2013-09-24 15:45 ` Anderson Lizardo
2013-09-24 17:21 ` Johan Hedberg
2013-09-24 18:29 ` Marcel Holtmann
1 sibling, 1 reply; 25+ messages in thread
From: Anderson Lizardo @ 2013-09-24 15:45 UTC (permalink / raw)
To: Johan Hedberg; +Cc: BlueZ development
Hi Johan,
On Tue, Sep 24, 2013 at 10:02 AM, <johan.hedberg@gmail.com> wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> This patch adds a new mgmt setting for LE advertising and hooks up the
> necessary places in the mgmt code to operate on the HCI_LE_PERIPHERAL
> flag (which corresponds to this setting). This patch does not yet add
> any new command for enabling the setting - that is left for a subsequent
> patch.
How this code behaves if we enable/disable LE advertising using
hciconfig hci0 leadv/noleadv? IIRC the LE_SET_ADV_ENABLE command will
fail if advertising is already set on the controller.
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising
2013-09-24 15:45 ` Anderson Lizardo
@ 2013-09-24 17:21 ` Johan Hedberg
2013-09-25 10:28 ` Johan Hedberg
0 siblings, 1 reply; 25+ messages in thread
From: Johan Hedberg @ 2013-09-24 17:21 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: BlueZ development
Hi Lizardo,
On Tue, Sep 24, 2013, Anderson Lizardo wrote:
> On Tue, Sep 24, 2013 at 10:02 AM, <johan.hedberg@gmail.com> wrote:
> > This patch adds a new mgmt setting for LE advertising and hooks up the
> > necessary places in the mgmt code to operate on the HCI_LE_PERIPHERAL
> > flag (which corresponds to this setting). This patch does not yet add
> > any new command for enabling the setting - that is left for a subsequent
> > patch.
>
> How this code behaves if we enable/disable LE advertising using
> hciconfig hci0 leadv/noleadv? IIRC the LE_SET_ADV_ENABLE command will
> fail if advertising is already set on the controller.
You're right that a mix of mgmt and hciconfig will mix things up on the
kernel side. This is something I was aware of but didn't investigate
much further since I was assuming it would add too much complexity to
the code. The principle has always been that we keep compatibility/good
behavior with mixed mgmg/raw HCI access only as long as it doesn't
needlessly complicate the code.
That said, I'll take a another look if the flag setting could be moved
to a hci_event.c handler from the request callback without requiring the
addition of a second flag or state variable. This issue is not unique to
this new setting but actually exists for many of them. What we probably
need is a generic mgmt_send_new_settings function that hci_event.c
handlers can call when they know that new_settings should be emitted. I
suspect that might solve the issue.
Johan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/8] Bluetooth: Move mgmt response convenience functions to a better location
2013-09-24 14:02 ` [PATCH 5/8] Bluetooth: Move mgmt response convenience functions to a better location johan.hedberg
@ 2013-09-24 18:11 ` Marcel Holtmann
0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-24 18:11 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> The settings_rsp and cmd_status_rsp functions can be useful for all mgmt
> command handlers when asynchronous request callbacks are used. They will
> e.g. be used by subsequent patches to change set_le to use an async
> request as well as a new set_advertising command. Therefore, move them
> higher up in the mgmt.c file to avoid unnecessary forward declarations
> or mixing this trivial change with other patches.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 60 ++++++++++++++++++++++++++--------------------------
> 1 file changed, 30 insertions(+), 30 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] Bluetooth: Add clarifying comment to bt_sock_wait_state()
2013-09-24 14:02 ` [PATCH 1/8] Bluetooth: Add clarifying comment to bt_sock_wait_state() johan.hedberg
@ 2013-09-24 18:11 ` Marcel Holtmann
0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-24 18:11 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> The bt_sock_wait_state requires the sk lock to be held (through
> lock_sock) so document it clearly in the code.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/af_bluetooth.c | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/8] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
2013-09-24 14:02 ` [PATCH 2/8] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg johan.hedberg
@ 2013-09-24 18:16 ` Marcel Holtmann
0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-24 18:16 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> This patch cleans up the locking login in l2cap_sock_recvmsg by pairing
> up each lock_sock call with a release_sock call. The function already
> has a "done" label that handles releasing the socket and returning from
> the function so the fix is rather simple.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_sock.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index ad95b42..5853c1e 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -795,7 +795,7 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> {
> struct sock *sk = sock->sk;
> struct l2cap_pinfo *pi = l2cap_pi(sk);
> - int err;
> + int err = 0;
>
> lock_sock(sk);
>
> @@ -805,8 +805,7 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> pi->chan->state = BT_CONFIG;
>
> __l2cap_connect_rsp_defer(pi->chan);
> - release_sock(sk);
> - return 0;
can we just do
err = 0;
here instead of declaring it global.
> + goto done;
> }
>
> release_sock(sk);
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/8] Bluetooth: Fix busy return for mgmt_set_powered in some cases
2013-09-24 14:02 ` [PATCH 4/8] Bluetooth: Fix busy return for mgmt_set_powered in some cases johan.hedberg
@ 2013-09-24 18:17 ` Marcel Holtmann
0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-24 18:17 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> We should return a "busy" error always when there is another
> mgmt_set_powered operation in progress. Previously when powering on
> while the auto off timer was still set the code could have let two or
> more pending power on commands to be queued. This patch fixes the issue
> by moving the check for duplicate commands to an earlier point in the
> set_powered handler.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
2013-09-24 14:02 ` [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev() johan.hedberg
@ 2013-09-24 18:19 ` Marcel Holtmann
2013-09-25 10:03 ` Johan Hedberg
0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-24 18:19 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> If either one of the HCI_SETUP or HCI_USER_CHANNEL flags is set the
> device is not considered valid for mgmt. By having these checks inside
> the mgmt_valid_hdev function the a couple of places using it can be
> simplified.
I looked at doing this and decided not to. Reason was that the device gets removed from mgmt anyway.
So I need a bit more detail why we better do it this way.
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/8] Bluetooth: Use async request for LE enable/disable
2013-09-24 14:02 ` [PATCH 6/8] Bluetooth: Use async request for LE enable/disable johan.hedberg
@ 2013-09-24 18:20 ` Marcel Holtmann
0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-24 18:20 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> This patch updates the code to use an asynchronous request for handling
> the enabling and disabling of LE support. This refactoring is necessary
> as a preparation for adding advertising support, since when LE is
> disabled we should also disable advertising, and the cleanest way to do
> this is to perform the two respective HCI commands in the same
> asynchronous request.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 1 -
> net/bluetooth/hci_event.c | 11 -------
> net/bluetooth/mgmt.c | 69 +++++++++++++++++-----------------------
> 3 files changed, 29 insertions(+), 52 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising
2013-09-24 14:02 ` [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising johan.hedberg
2013-09-24 15:45 ` Anderson Lizardo
@ 2013-09-24 18:29 ` Marcel Holtmann
1 sibling, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-24 18:29 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> This patch adds a new mgmt setting for LE advertising and hooks up the
> necessary places in the mgmt code to operate on the HCI_LE_PERIPHERAL
> flag (which corresponds to this setting). This patch does not yet add
> any new command for enabling the setting - that is left for a subsequent
> patch.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/mgmt.h | 1 +
> net/bluetooth/mgmt.c | 24 +++++++++++++++++++++++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
2013-09-24 18:19 ` Marcel Holtmann
@ 2013-09-25 10:03 ` Johan Hedberg
2013-09-27 2:12 ` Marcel Holtmann
0 siblings, 1 reply; 25+ messages in thread
From: Johan Hedberg @ 2013-09-25 10:03 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Tue, Sep 24, 2013, Marcel Holtmann wrote:
> > If either one of the HCI_SETUP or HCI_USER_CHANNEL flags is set the
> > device is not considered valid for mgmt. By having these checks inside
> > the mgmt_valid_hdev function the a couple of places using it can be
> > simplified.
>
> I looked at doing this and decided not to. Reason was that the device
> gets removed from mgmt anyway.
I'm not sure what significance you think "device gets removed from mgmt"
has. All that is is a mgmt event saying that index has been removed. In
addition to that we need to ensure that we don't send any more mgmt
events for such devices and that we don't include such devices in the
response to mgmt_read_index list.
These are the two places of the code that my patch simplifies, one is
the check for whether to send a "power on" mgmt event and the other the
response handling of read_index_list.
Johan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising
2013-09-24 17:21 ` Johan Hedberg
@ 2013-09-25 10:28 ` Johan Hedberg
0 siblings, 0 replies; 25+ messages in thread
From: Johan Hedberg @ 2013-09-25 10:28 UTC (permalink / raw)
To: Anderson Lizardo, BlueZ development
Hi,
On Tue, Sep 24, 2013, Johan Hedberg wrote:
> On Tue, Sep 24, 2013, Anderson Lizardo wrote:
> > On Tue, Sep 24, 2013 at 10:02 AM, <johan.hedberg@gmail.com> wrote:
> > > This patch adds a new mgmt setting for LE advertising and hooks up the
> > > necessary places in the mgmt code to operate on the HCI_LE_PERIPHERAL
> > > flag (which corresponds to this setting). This patch does not yet add
> > > any new command for enabling the setting - that is left for a subsequent
> > > patch.
> >
> > How this code behaves if we enable/disable LE advertising using
> > hciconfig hci0 leadv/noleadv? IIRC the LE_SET_ADV_ENABLE command will
> > fail if advertising is already set on the controller.
>
> You're right that a mix of mgmt and hciconfig will mix things up on the
> kernel side. This is something I was aware of but didn't investigate
> much further since I was assuming it would add too much complexity to
> the code. The principle has always been that we keep compatibility/good
> behavior with mixed mgmg/raw HCI access only as long as it doesn't
> needlessly complicate the code.
>
> That said, I'll take a another look if the flag setting could be moved
> to a hci_event.c handler from the request callback without requiring the
> addition of a second flag or state variable. This issue is not unique to
> this new setting but actually exists for many of them. What we probably
> need is a generic mgmt_send_new_settings function that hci_event.c
> handlers can call when they know that new_settings should be emitted. I
> suspect that might solve the issue.
So I did take another look and this gets tricky since we don't just need
to emit a new_settings, but also ensure that it does not get emitted on
the mgmt socket that may have caused the command to be sent. Therefore,
I decided against trying to have "full" support of mixing mgmt with raw
HCI. What I did do is ensure that our dev_flags remain correct by using
the hci_event.c handler to set them. This ensures that subsequent mgmt
commands still work even though bluetoothd may not always be completely
up to date with these settings.
Johan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
2013-09-25 10:03 ` Johan Hedberg
@ 2013-09-27 2:12 ` Marcel Holtmann
2013-09-27 11:45 ` Johan Hedberg
0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-27 2:12 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
Hi Johan,
>>> If either one of the HCI_SETUP or HCI_USER_CHANNEL flags is set the
>>> device is not considered valid for mgmt. By having these checks inside
>>> the mgmt_valid_hdev function the a couple of places using it can be
>>> simplified.
>>
>> I looked at doing this and decided not to. Reason was that the device
>> gets removed from mgmt anyway.
>
> I'm not sure what significance you think "device gets removed from mgmt"
> has. All that is is a mgmt event saying that index has been removed. In
> addition to that we need to ensure that we don't send any more mgmt
> events for such devices and that we don't include such devices in the
> response to mgmt_read_index list.
>
> These are the two places of the code that my patch simplifies, one is
> the check for whether to send a "power on" mgmt event and the other the
> response handling of read_index_list.
I have taken care of that part. It is done inside mgmt_control() and in case of a controller in user channel mode it returns invalid index.
For the events, I have no idea on how we would generate events if we are not going through hci_event.c when the user channel is active.
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
2013-09-27 2:12 ` Marcel Holtmann
@ 2013-09-27 11:45 ` Johan Hedberg
2013-09-28 8:06 ` Marcel Holtmann
0 siblings, 1 reply; 25+ messages in thread
From: Johan Hedberg @ 2013-09-27 11:45 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Fri, Sep 27, 2013, Marcel Holtmann wrote:
> >>> If either one of the HCI_SETUP or HCI_USER_CHANNEL flags is set the
> >>> device is not considered valid for mgmt. By having these checks inside
> >>> the mgmt_valid_hdev function the a couple of places using it can be
> >>> simplified.
> >>
> >> I looked at doing this and decided not to. Reason was that the device
> >> gets removed from mgmt anyway.
> >
> > I'm not sure what significance you think "device gets removed from mgmt"
> > has. All that is is a mgmt event saying that index has been removed. In
> > addition to that we need to ensure that we don't send any more mgmt
> > events for such devices and that we don't include such devices in the
> > response to mgmt_read_index list.
> >
> > These are the two places of the code that my patch simplifies, one is
> > the check for whether to send a "power on" mgmt event and the other the
> > response handling of read_index_list.
>
> I have taken care of that part. It is done inside mgmt_control() and
> in case of a controller in user channel mode it returns invalid index.
That's great, but it doesn't cover the read_index_list command which we
were talking about here since the command doesn't target any specific
controller (as per our mgmt-api.txt it has the value <non-controller> as
the controller index). The command handler will in this case be invoked
that that's where it'd be convenient to rely on mgmt_valid_hdev() for
constructing the response.
> For the events, I have no idea on how we would generate events if we
> are not going through hci_event.c when the user channel is active.
The "powered on" event in question doesn't come from hci_event.c but
from the hci_dev_open() function, so again this is not covered by the
existing user channel checks.
Johan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
2013-09-27 11:45 ` Johan Hedberg
@ 2013-09-28 8:06 ` Marcel Holtmann
2013-09-28 15:34 ` Johan Hedberg
0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2013-09-28 8:06 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
Hi Johan,
>>>>> If either one of the HCI_SETUP or HCI_USER_CHANNEL flags is set the
>>>>> device is not considered valid for mgmt. By having these checks inside
>>>>> the mgmt_valid_hdev function the a couple of places using it can be
>>>>> simplified.
>>>>
>>>> I looked at doing this and decided not to. Reason was that the device
>>>> gets removed from mgmt anyway.
>>>
>>> I'm not sure what significance you think "device gets removed from mgmt"
>>> has. All that is is a mgmt event saying that index has been removed. In
>>> addition to that we need to ensure that we don't send any more mgmt
>>> events for such devices and that we don't include such devices in the
>>> response to mgmt_read_index list.
>>>
>>> These are the two places of the code that my patch simplifies, one is
>>> the check for whether to send a "power on" mgmt event and the other the
>>> response handling of read_index_list.
>>
>> I have taken care of that part. It is done inside mgmt_control() and
>> in case of a controller in user channel mode it returns invalid index.
>
> That's great, but it doesn't cover the read_index_list command which we
> were talking about here since the command doesn't target any specific
> controller (as per our mgmt-api.txt it has the value <non-controller> as
> the controller index). The command handler will in this case be invoked
> that that's where it'd be convenient to rely on mgmt_valid_hdev() for
> constructing the response.
the read index list command has a special check for user channel. As I said, I looked into this and opted against extending valid hdev macro since we only need it in a few specific cases. Adding it there resulted in simpler code then some magic macro that is used in more places.
When I wrote the patches for user channel this felt cleaner. I had patches for both ways, but decided to go this route.
>> For the events, I have no idea on how we would generate events if we
>> are not going through hci_event.c when the user channel is active.
>
> The "powered on" event in question doesn't come from hci_event.c but
> from the hci_dev_open() function, so again this is not covered by the
> existing user channel checks.
How would you trigger a power on event. When user channel is active everything else is blocked. So every other operation will fail. That is taken care of.
In addition you can only activate user channel on a controller that is powered off. That was a conscious design decision. Keep it really simple. User channel mode is exclusive and exclusive to one user at a time.
Regards
Marcel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
2013-09-28 8:06 ` Marcel Holtmann
@ 2013-09-28 15:34 ` Johan Hedberg
0 siblings, 0 replies; 25+ messages in thread
From: Johan Hedberg @ 2013-09-28 15:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Sat, Sep 28, 2013, Marcel Holtmann wrote:
> >>>>> If either one of the HCI_SETUP or HCI_USER_CHANNEL flags is set the
> >>>>> device is not considered valid for mgmt. By having these checks inside
> >>>>> the mgmt_valid_hdev function the a couple of places using it can be
> >>>>> simplified.
> >>>>
> >>>> I looked at doing this and decided not to. Reason was that the device
> >>>> gets removed from mgmt anyway.
> >>>
> >>> I'm not sure what significance you think "device gets removed from mgmt"
> >>> has. All that is is a mgmt event saying that index has been removed. In
> >>> addition to that we need to ensure that we don't send any more mgmt
> >>> events for such devices and that we don't include such devices in the
> >>> response to mgmt_read_index list.
> >>>
> >>> These are the two places of the code that my patch simplifies, one is
> >>> the check for whether to send a "power on" mgmt event and the other the
> >>> response handling of read_index_list.
> >>
> >> I have taken care of that part. It is done inside mgmt_control() and
> >> in case of a controller in user channel mode it returns invalid index.
> >
> > That's great, but it doesn't cover the read_index_list command which we
> > were talking about here since the command doesn't target any specific
> > controller (as per our mgmt-api.txt it has the value <non-controller> as
> > the controller index). The command handler will in this case be invoked
> > that that's where it'd be convenient to rely on mgmt_valid_hdev() for
> > constructing the response.
>
> the read index list command has a special check for user channel. As I
> said, I looked into this and opted against extending valid hdev macro
> since we only need it in a few specific cases. Adding it there
> resulted in simpler code then some magic macro that is used in more
> places.
>
> When I wrote the patches for user channel this felt cleaner. I had
> patches for both ways, but decided to go this route.
Fair enough.
> >> For the events, I have no idea on how we would generate events if we
> >> are not going through hci_event.c when the user channel is active.
> >
> > The "powered on" event in question doesn't come from hci_event.c but
> > from the hci_dev_open() function, so again this is not covered by the
> > existing user channel checks.
>
> How would you trigger a power on event. When user channel is active
> everything else is blocked. So every other operation will fail. That
> is taken care of.
>
> In addition you can only activate user channel on a controller that is
> powered off. That was a conscious design decision. Keep it really
> simple. User channel mode is exclusive and exclusive to one user at a
> time.
Good point. The only hci_dev_open() call that comes to mind is the one
automatically triggered by the setup stage, but user space doesn't yet
know about the controller at that point.
Johan
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-09-28 15:34 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-24 14:02 [PATCH 0/8] Bluetooth: Cleanups and LE advertising support johan.hedberg
2013-09-24 14:02 ` [PATCH 1/8] Bluetooth: Add clarifying comment to bt_sock_wait_state() johan.hedberg
2013-09-24 18:11 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 2/8] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg johan.hedberg
2013-09-24 18:16 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev() johan.hedberg
2013-09-24 18:19 ` Marcel Holtmann
2013-09-25 10:03 ` Johan Hedberg
2013-09-27 2:12 ` Marcel Holtmann
2013-09-27 11:45 ` Johan Hedberg
2013-09-28 8:06 ` Marcel Holtmann
2013-09-28 15:34 ` Johan Hedberg
2013-09-24 14:02 ` [PATCH 4/8] Bluetooth: Fix busy return for mgmt_set_powered in some cases johan.hedberg
2013-09-24 18:17 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 5/8] Bluetooth: Move mgmt response convenience functions to a better location johan.hedberg
2013-09-24 18:11 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 6/8] Bluetooth: Use async request for LE enable/disable johan.hedberg
2013-09-24 18:20 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising johan.hedberg
2013-09-24 15:45 ` Anderson Lizardo
2013-09-24 17:21 ` Johan Hedberg
2013-09-25 10:28 ` Johan Hedberg
2013-09-24 18:29 ` Marcel Holtmann
2013-09-24 14:02 ` [PATCH 8/8] Bluetooth: Add new mgmt_set_advertising command johan.hedberg
2013-09-24 15:42 ` Johan Hedberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox