* Re: [PATCH v2 7/7] Bluetooth: Add new mgmt_set_advertising command
From: Gustavo Padovan @ 2013-09-25 17:30 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380104770-8022-8-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
2013-09-25 johan.hedberg@gmail.com <johan.hedberg@gmail.com>:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> This patch adds a new mgmt command for enabling and disabling
> LE advertising. The command depends on the LE setting being enabled
> first and will return a "rejected" response otherwise. The patch also
> adds safeguards so that there will ever only be one set_le or
> set_advertising command pending per adapter.
>
> The response handling and new_settings event sending is done in an
> asynchronous request callback, meaning raw HCI access from user space to
> enable advertising (e.g. hciconfig leadv) will not trigger the
> new_settings event. This is intentional since trying to support mixed
> raw HCI and mgmt access would mean adding extra state tracking or new
> helper functions, essentially negating the benefit of using the
> asynchronous request framework. The HCI_LE_ENABLED and HCI_LE_PERIPHERAL
> flags however are updated correctly even with raw HCI access so this
> will not completely break subsequent access over mgmt.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/mgmt.h | 2 +
> net/bluetooth/mgmt.c | 97 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 98 insertions(+), 1 deletion(-)
All patches have been applied to bluetooth-next. Thanks.
Gustavo
^ permalink raw reply
* Re: [PATCH v2 7/7] Bluetooth: Add new mgmt_set_advertising command
From: Marcel Holtmann @ 2013-09-25 10:31 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380104770-8022-8-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> This patch adds a new mgmt command for enabling and disabling
> LE advertising. The command depends on the LE setting being enabled
> first and will return a "rejected" response otherwise. The patch also
> adds safeguards so that there will ever only be one set_le or
> set_advertising command pending per adapter.
>
> The response handling and new_settings event sending is done in an
> asynchronous request callback, meaning raw HCI access from user space to
> enable advertising (e.g. hciconfig leadv) will not trigger the
> new_settings event. This is intentional since trying to support mixed
> raw HCI and mgmt access would mean adding extra state tracking or new
> helper functions, essentially negating the benefit of using the
> asynchronous request framework. The HCI_LE_ENABLED and HCI_LE_PERIPHERAL
> flags however are updated correctly even with raw HCI access so this
> will not completely break subsequent access over mgmt.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/mgmt.h | 2 +
> net/bluetooth/mgmt.c | 97 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 98 insertions(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2 2/7] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
From: Marcel Holtmann @ 2013-09-25 10:30 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380104770-8022-3-git-send-email-johan.hedberg@gmail.com>
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 | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising
From: Johan Hedberg @ 2013-09-25 10:28 UTC (permalink / raw)
To: Anderson Lizardo, BlueZ development
In-Reply-To: <20130924172120.GA30191@x220.p-661hnu-f1>
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
* [PATCH v2 7/7] Bluetooth: Add new mgmt_set_advertising command
From: johan.hedberg @ 2013-09-25 10:26 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380104770-8022-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
This patch adds a new mgmt command for enabling and disabling
LE advertising. The command depends on the LE setting being enabled
first and will return a "rejected" response otherwise. The patch also
adds safeguards so that there will ever only be one set_le or
set_advertising command pending per adapter.
The response handling and new_settings event sending is done in an
asynchronous request callback, meaning raw HCI access from user space to
enable advertising (e.g. hciconfig leadv) will not trigger the
new_settings event. This is intentional since trying to support mixed
raw HCI and mgmt access would mean adding extra state tracking or new
helper functions, essentially negating the benefit of using the
asynchronous request framework. The HCI_LE_ENABLED and HCI_LE_PERIPHERAL
flags however are updated correctly even with raw HCI access so this
will not completely break subsequent access over mgmt.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/mgmt.h | 2 +
net/bluetooth/mgmt.c | 97 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 98 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 9a2faa3..1b5b10f 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[] = {
@@ -1431,7 +1432,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;
@@ -3136,6 +3138,98 @@ 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;
+ }
+
+ 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;
@@ -3347,6 +3441,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.3.1
^ permalink raw reply related
* [PATCH v2 6/7] Bluetooth: Add new mgmt setting for LE advertising
From: johan.hedberg @ 2013-09-25 10:26 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380104770-8022-1-git-send-email-johan.hedberg@gmail.com>
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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/mgmt.h | 1 +
net/bluetooth/hci_event.c | 1 +
net/bluetooth/mgmt.c | 21 ++++++++++++++++++++-
3 files changed, 22 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/hci_event.c b/net/bluetooth/hci_event.c
index 48db81f..917c7c8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1000,6 +1000,7 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
} else {
hdev->features[1][0] &= ~LMP_HOST_LE;
clear_bit(HCI_LE_ENABLED, &hdev->dev_flags);
+ clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
}
if (sent->simul)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 4c3984e..9a2faa3 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;
}
@@ -1411,6 +1416,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;
@@ -1442,6 +1452,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);
@@ -3517,6 +3530,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.3.1
^ permalink raw reply related
* [PATCH v2 5/7] Bluetooth: Use async request for LE enable/disable
From: johan.hedberg @ 2013-09-25 10:26 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380104770-8022-1-git-send-email-johan.hedberg@gmail.com>
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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci_core.h | 1 -
net/bluetooth/hci_event.c | 11 +++----
net/bluetooth/mgmt.c | 67 ++++++++++++++++------------------------
3 files changed, 32 insertions(+), 47 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..48db81f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -994,20 +994,19 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
return;
if (!status) {
- if (sent->le)
+ if (sent->le) {
hdev->features[1][0] |= LMP_HOST_LE;
- else
+ set_bit(HCI_LE_ENABLED, &hdev->dev_flags);
+ } else {
hdev->features[1][0] &= ~LMP_HOST_LE;
+ clear_bit(HCI_LE_ENABLED, &hdev->dev_flags);
+ }
if (sent->simul)
hdev->features[1][0] |= LMP_HOST_LE_BREDR;
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 61d4b19..4c3984e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1354,11 +1354,32 @@ 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;
+ }
+
+ 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 +1440,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 +4166,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.3.1
^ permalink raw reply related
* [PATCH v2 4/7] Bluetooth: Move mgmt response convenience functions to a better location
From: johan.hedberg @ 2013-09-25 10:26 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380104770-8022-1-git-send-email-johan.hedberg@gmail.com>
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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
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 85bfa21..61d4b19 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.3.1
^ permalink raw reply related
* [PATCH v2 3/7] Bluetooth: Fix busy return for mgmt_set_powered in some cases
From: johan.hedberg @ 2013-09-25 10:26 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380104770-8022-1-git-send-email-johan.hedberg@gmail.com>
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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
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 3070e77..85bfa21 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.3.1
^ permalink raw reply related
* [PATCH v2 2/7] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
From: johan.hedberg @ 2013-09-25 10:26 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380104770-8022-1-git-send-email-johan.hedberg@gmail.com>
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 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ad95b42..c85537c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -805,8 +805,8 @@ 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;
+ err = 0;
+ goto done;
}
release_sock(sk);
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 1/7] Bluetooth: Add clarifying comment to bt_sock_wait_state()
From: johan.hedberg @ 2013-09-25 10:26 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380104770-8022-1-git-send-email-johan.hedberg@gmail.com>
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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
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.3.1
^ permalink raw reply related
* [PATCH v2 0/7] Bluetooth: Cleanups and LE advertising support
From: johan.hedberg @ 2013-09-25 10:26 UTC (permalink / raw)
To: linux-bluetooth
Hi,
Here's an updated set based on feedback on the first one. The
mgmt_valid_hdev patch has been dropped since it's a trivial one and I
didn't want the debate over it to slow down the overall progress of this
set.
The main difference to the initial set_le and set_advertising code is
that I now let the hci_event.c handlers take care of setting the right
values of the dev_flags bits. This works fine since these handlers are
always executed before the async request callbacks which then get called
with dev_flags already having the correct value. The benefit of using
the event handlers like this is that we retain at least partial support
for raw HCI access (hciconfig) by ensuring that the kernel state flags
are always correct.
Johan
----------------------------------------------------------------
Johan Hedberg (7):
Bluetooth: Add clarifying comment to bt_sock_wait_state()
Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
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_event.c | 12 +-
net/bluetooth/l2cap_sock.c | 4 +-
net/bluetooth/mgmt.c | 257 ++++++++++++++++++++++++++------------
6 files changed, 191 insertions(+), 87 deletions(-)
^ permalink raw reply
* Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
From: Johan Hedberg @ 2013-09-25 10:03 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <DFCF34C2-5AA9-4644-BB4B-3D6073DA9D42@holtmann.org>
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
* RE: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support
From: Bing Zhao @ 2013-09-24 19:54 UTC (permalink / raw)
To: Marcel Holtmann
Cc: linux-bluetooth@vger.kernel.org, Gustavo Padovan, Johan Hedberg,
linux-wireless@vger.kernel.org, Mike Frysinger, Hyuckjoo Lee,
Amitkumar Karwar
In-Reply-To: <C42E9D11-1E8B-499E-96E0-74469F7649EC@holtmann.org>
Hi Marcel,
> > We can extend __hci_cmd_sync() function with a new parameter 'type'.
> > This way we can pass HCI_VENDOR_PKT into __hci_cmd_sync(), while other =
drivers will pass in
> HCI_COMMAND_PKT.
>=20
> That will actually not work. And I also do not want to do that. The __hci=
_cmd_sync() is for real HCI
> packets. That means types 0x01 and 0x04 only. They need to adhere to the =
HCI flow control mechanism
> for commands.
I see.
>=20
> > Our driver will make HCI_VENDOR_PKT -> MRVL_VENDOR_PKT conversion befor=
e downloading the frame to
> firmware. And the MRVL_VENDOR_PKT frame from firmware will be replaced wi=
th HCI_VENDOR_PKT while
> uploading the frame to stack.
> >
> > Please let us know if this approach works for you or not.
>=20
> I think this is best kept inside the driver. However you might consider b=
uilding something like
> __hci_cmd_sync() that is specific to your driver, but allows for a simila=
r flow within ->setup().
Sure, we will consider building a function in the driver to handle this.
Thanks,
Bing
^ permalink raw reply
* Re: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support
From: Marcel Holtmann @ 2013-09-24 19:43 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth@vger.kernel.org, Gustavo Padovan, Johan Hedberg,
linux-wireless@vger.kernel.org, Mike Frysinger, Hyuckjoo Lee,
Amitkumar Karwar
In-Reply-To: <477F20668A386D41ADCC57781B1F70430F450779BE@SC-VEXCH1.marvell.com>
Hi Bing,
>>> The reason of not using __hci_cmd_sync() is that we are sending vendor specific command here
>> (MRVL_VENDOR_PKT). The __hci_cmd_sync seems handle HCI_COMMAND_PKT only.
>>> Please let us know if you have any suggestion to solve this problem.
>>
>> what is a MRVL_VENDOR_PKT actually?
>
> It's defined as 0xfe in our driver. The firmware doesn't understand 0xff (HCI_VENDOR_PKT).
so it is actually out-of-channel vendor packet.
>>
>> If you guys are not using standard HCI command/event for vendor operation, then this obviously does
>> not fit. However a similar model might make sense instead of manually building packets all the time.
>
> We can extend __hci_cmd_sync() function with a new parameter 'type'.
> This way we can pass HCI_VENDOR_PKT into __hci_cmd_sync(), while other drivers will pass in HCI_COMMAND_PKT.
That will actually not work. And I also do not want to do that. The __hci_cmd_sync() is for real HCI packets. That means types 0x01 and 0x04 only. They need to adhere to the HCI flow control mechanism for commands.
> Our driver will make HCI_VENDOR_PKT -> MRVL_VENDOR_PKT conversion before downloading the frame to firmware. And the MRVL_VENDOR_PKT frame from firmware will be replaced with HCI_VENDOR_PKT while uploading the frame to stack.
>
> Please let us know if this approach works for you or not.
I think this is best kept inside the driver. However you might consider building something like __hci_cmd_sync() that is specific to your driver, but allows for a similar flow within ->setup().
Regards
Marcel
^ permalink raw reply
* RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler
From: Bing Zhao @ 2013-09-24 19:42 UTC (permalink / raw)
To: Johan Hedberg
Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org, Gustavo Padovan,
linux-wireless@vger.kernel.org, Mike Frysinger, Hyuckjoo Lee,
Amitkumar Karwar
In-Reply-To: <20130924193010.GA2584@x220.p-661hnu-f1>
Hi Johan,
> Hi Bing,
>=20
> On Tue, Sep 24, 2013, Bing Zhao wrote:
> > > that is a bug. It should only be ever called once. Could this be due
> > > to RFKILL issue we had? Please re-test with Johan's patches applied
> > > and check if it makes a difference. Otherwise please send some logs
> > > since we want to get this fixed.
> >
> > Amitkumar Karwar has tested it with latest code on bluetooth-next tree
> > but the result is the same.
> > Apparently two threads race to call hci_dev_open(). If the thread from
> > hci_sock calls hci_dev_open earlier, it ends up not updating HCI_SETUP
> > hdev flag in hci_power_on(). This results that the setup handler gets
> > called again when user brings up the interface later.
>=20
> Let's see if I understood this right: the only hci_dev_open call in
> hci_sock.c is the one for the HCIDEVUP ioctl. So what you're doing is
> having user space call the HCIDEVUP ioctl before our own hci_power_on
> callback gets called to initialize the adapter?
That's right. The ioctl is initiated by the Bluetooth daemon.
Amitkumar has a setup that can reproduce this corner case easily.
I tested it on my Ubuntu but I couldn't replicate it.
>=20
> You're right that we're missing the clearing of the HCI_SETUP flag for
> such a scenario. Could you try the attached patch. It should fix the
> issue. One problem that it does have is that if the HCIDEVUP ioctl path
> goes through before hci_power_on gets called we will never notify mgmt
> of the adapter. However, that might be acceptable here since if you're
> using HCIDEVUP like this it seems it's not a mgmt based system anyway.
>=20
> > I checked the bluetooth-next tree, the following two patches (by
> > Johan) are not present in this tree.
> >
> > bf54303 Bluetooth: Fix rfkill functionality during the HCI setup stage
> > 5e13036 Bluetooth: Introduce a new HCI_RFKILLED flag
> >
> > They are in bluetooth.git tree. So, I'm not certain if Amitkumar has
> > applied them manually or not. Anyway we will re-test with Johan's
> > patches applied and confirm if they fix the race or not.
>=20
> I don't think these patches will help you in this case.
OK, we will test your patch instead.
Thanks,
Bing
^ permalink raw reply
* Re: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler
From: Johan Hedberg @ 2013-09-24 19:30 UTC (permalink / raw)
To: Bing Zhao
Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org, Gustavo Padovan,
linux-wireless@vger.kernel.org, Mike Frysinger, Hyuckjoo Lee,
Amitkumar Karwar
In-Reply-To: <477F20668A386D41ADCC57781B1F70430F450779A4@SC-VEXCH1.marvell.com>
[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]
Hi Bing,
On Tue, Sep 24, 2013, Bing Zhao wrote:
> > that is a bug. It should only be ever called once. Could this be due
> > to RFKILL issue we had? Please re-test with Johan's patches applied
> > and check if it makes a difference. Otherwise please send some logs
> > since we want to get this fixed.
>
> Amitkumar Karwar has tested it with latest code on bluetooth-next tree
> but the result is the same.
> Apparently two threads race to call hci_dev_open(). If the thread from
> hci_sock calls hci_dev_open earlier, it ends up not updating HCI_SETUP
> hdev flag in hci_power_on(). This results that the setup handler gets
> called again when user brings up the interface later.
Let's see if I understood this right: the only hci_dev_open call in
hci_sock.c is the one for the HCIDEVUP ioctl. So what you're doing is
having user space call the HCIDEVUP ioctl before our own hci_power_on
callback gets called to initialize the adapter?
You're right that we're missing the clearing of the HCI_SETUP flag for
such a scenario. Could you try the attached patch. It should fix the
issue. One problem that it does have is that if the HCIDEVUP ioctl path
goes through before hci_power_on gets called we will never notify mgmt
of the adapter. However, that might be acceptable here since if you're
using HCIDEVUP like this it seems it's not a mgmt based system anyway.
> I checked the bluetooth-next tree, the following two patches (by
> Johan) are not present in this tree.
>
> bf54303 Bluetooth: Fix rfkill functionality during the HCI setup stage
> 5e13036 Bluetooth: Introduce a new HCI_RFKILLED flag
>
> They are in bluetooth.git tree. So, I'm not certain if Amitkumar has
> applied them manually or not. Anyway we will re-test with Johan's
> patches applied and confirm if they fix the race or not.
I don't think these patches will help you in this case.
Johan
[-- Attachment #2: hci-setup.patch --]
[-- Type: text/plain, Size: 1114 bytes --]
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 634deba..c48bf1a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1164,7 +1164,7 @@ int hci_dev_open(__u16 dev)
atomic_set(&hdev->cmd_cnt, 1);
set_bit(HCI_INIT, &hdev->flags);
- if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
+ if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags) && hdev->setup)
ret = hdev->setup(hdev);
if (!ret) {
@@ -1581,10 +1581,13 @@ static const struct rfkill_ops hci_rfkill_ops = {
static void hci_power_on(struct work_struct *work)
{
struct hci_dev *hdev = container_of(work, struct hci_dev, power_on);
+ bool setup;
int err;
BT_DBG("%s", hdev->name);
+ setup = test_bit(HCI_SETUP, &hdev->dev_flags);
+
err = hci_dev_open(hdev->id);
if (err < 0) {
mgmt_set_powered_failed(hdev, err);
@@ -1595,7 +1598,7 @@ static void hci_power_on(struct work_struct *work)
queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
HCI_AUTO_OFF_TIMEOUT);
- if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags))
+ if (setup)
mgmt_index_added(hdev);
}
^ permalink raw reply related
* RE: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support
From: Bing Zhao @ 2013-09-24 19:22 UTC (permalink / raw)
To: Marcel Holtmann
Cc: linux-bluetooth@vger.kernel.org, Gustavo Padovan, Johan Hedberg,
linux-wireless@vger.kernel.org, Mike Frysinger, Hyuckjoo Lee,
Amitkumar Karwar
In-Reply-To: <34088279-04B9-49D0-9387-88A602F518EC@holtmann.org>
Hi Marcel,
> > The reason of not using __hci_cmd_sync() is that we are sending vendor =
specific command here
> (MRVL_VENDOR_PKT). The __hci_cmd_sync seems handle HCI_COMMAND_PKT only.
> > Please let us know if you have any suggestion to solve this problem.
>=20
> what is a MRVL_VENDOR_PKT actually?
It's defined as 0xfe in our driver. The firmware doesn't understand 0xff (H=
CI_VENDOR_PKT).
>=20
> If you guys are not using standard HCI command/event for vendor operation=
, then this obviously does
> not fit. However a similar model might make sense instead of manually bui=
lding packets all the time.
We can extend __hci_cmd_sync() function with a new parameter 'type'.
This way we can pass HCI_VENDOR_PKT into __hci_cmd_sync(), while other driv=
ers will pass in HCI_COMMAND_PKT.
Our driver will make HCI_VENDOR_PKT -> MRVL_VENDOR_PKT conversion before do=
wnloading the frame to firmware. And the MRVL_VENDOR_PKT frame from firmwar=
e will be replaced with HCI_VENDOR_PKT while uploading the frame to stack.
Please let us know if this approach works for you or not.
Thanks,
Bing
^ permalink raw reply
* RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler
From: Bing Zhao @ 2013-09-24 19:04 UTC (permalink / raw)
To: Marcel Holtmann
Cc: linux-bluetooth@vger.kernel.org, Gustavo Padovan, Johan Hedberg,
linux-wireless@vger.kernel.org, Mike Frysinger, Hyuckjoo Lee,
Amitkumar Karwar
In-Reply-To: <DF6283E3-8476-400F-A230-CB7CA0F98564@holtmann.org>
[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]
Hi Marcel,
> > It's observed that sometimes the setup handler is called twice when Bluetooth daemon is running in
> background. We will rebase to latest commit on bluetooth-next tree and test again. If the issue is
> gone with the latest code in -next tree we will remove the setup_done flag.
>
> that is a bug. It should only be ever called once. Could this be due to RFKILL issue we had? Please
> re-test with Johan's patches applied and check if it makes a difference. Otherwise please send some
> logs since we want to get this fixed.
Amitkumar Karwar has tested it with latest code on bluetooth-next tree but the result is the same.
Apparently two threads race to call hci_dev_open(). If the thread from hci_sock calls hci_dev_open earlier, it ends up not updating HCI_SETUP hdev flag in hci_power_on(). This results that the setup handler gets called again when user brings up the interface later.
Attached are the debug logs and the patch used to generate them.
I checked the bluetooth-next tree, the following two patches (by Johan) are not present in this tree.
bf54303 Bluetooth: Fix rfkill functionality during the HCI setup stage
5e13036 Bluetooth: Introduce a new HCI_RFKILLED flag
They are in bluetooth.git tree. So, I'm not certain if Amitkumar has applied them manually or not. Anyway we will re-test with Johan's patches applied and confirm if they fix the race or not.
Thanks,
Bing
[-- Attachment #2: success.log --]
[-- Type: application/octet-stream, Size: 1097 bytes --]
[ 1717.100628] mmc1: new SDIO card at address 0001
[ 1717.100862] Bluetooth: vendor=0x2df, device=0x912a, class=255, fn=2
[ 1718.381306] BT_DBG: enter: hci_register_dev
[ 1718.381668] BT_DBG: in hci_register_dev queueing work power_on
[ 1718.381676] BT_DBG: exit: hci_register_dev
[ 1718.382658] BT_DBG: enter: hci_power_on line=1673 pid=3686
[ 1718.382664] BT_DBG: hci_dev_open line=1186 pid=3686
[ 1718.382669] BT_DBG: hci_dev_open line=1200 pid=3686
[ 1718.382673] BT_DBG: hci_dev_open line=1206 pid=3686
[ 1718.382677] BT_DBG: hci_dev_open line=1212 pid=3686
[ 1718.382681] BT_DBG: hci_dev_open line=1217 pid=3686
[ 1718.382684] btmrvl: enter btmrvl_setup()
[ 1718.383231] BT_DBG: calling hci_dev_open from hci_sock.c pid=3689
[ 1718.383236] BT_DBG: hci_dev_open line=1186 pid=3689
[ 1718.489930] BT_DBG: hci_dev_open line=1225 pid=3686
[ 1718.635973] BT_DBG: hci_power_on line=1684 updating HCI_SETUP hdev flag pid=3686
[ 1718.635984] BT_DBG: exit: hci_power_on line=1687 pid=3686
[ 1718.636282] BT_DBG: hci_dev_open line=1200 pid=3689
[ 1718.636288] BT_DBG: hci_dev_open line=1206 pid=3689
[-- Attachment #3: failure.log --]
[-- Type: application/octet-stream, Size: 951 bytes --]
[ 132.180560] mmc1: new SDIO card at address 0001
[ 132.415282] Bluetooth: vendor=0x2df, device=0x912a, class=255, fn=2
[ 133.784309] BT_DBG: enter: hci_register_dev
[ 133.784694] BT_DBG: in hci_register_dev queueing work power_on
[ 133.784703] BT_DBG: exit: hci_register_dev
[ 133.786616] BT_DBG: calling hci_dev_open from hci_sock.c pid=3287
[ 133.786624] BT_DBG: hci_dev_open line=1186 pid=3287
[ 133.786628] BT_DBG: hci_dev_open line=1200 pid=3287
[ 133.786633] BT_DBG: hci_dev_open line=1206 pid=3287
[ 133.786636] BT_DBG: hci_dev_open line=1212 pid=3287
[ 133.786640] BT_DBG: hci_dev_open line=1217 pid=3287
[ 133.786644] btmrvl: enter btmrvl_setup()
[ 133.789683] BT_DBG: enter: hci_power_on line=1673 pid=3283
[ 133.789691] BT_DBG: hci_dev_open line=1186 pid=3283
[ 133.954749] BT_DBG: hci_dev_open line=1225 pid=3287
[ 134.101863] BT_DBG: hci_dev_open line=1200 pid=3283
[ 134.101871] BT_DBG: hci_dev_open line=1206 pid=3283
[-- Attachment #4: bt_debug.diff --]
[-- Type: application/octet-stream, Size: 3517 bytes --]
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 6eea188..b474dde 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -621,6 +621,7 @@ static int btmrvl_setup(struct hci_dev *hdev)
struct btmrvl_private *priv = hci_get_drvdata(hdev);
struct btmrvl_adapter *adapter = priv->adapter;
+ printk("btmrvl: enter btmrvl_setup()\n");
if (adapter->setup_done)
return 0;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3d9f02b..d0795e9 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1183,6 +1183,7 @@ int hci_dev_open(__u16 dev)
struct hci_dev *hdev;
int ret = 0;
+ printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
hdev = hci_dev_get(dev);
if (!hdev)
return -ENODEV;
@@ -1196,20 +1197,24 @@ int hci_dev_open(__u16 dev)
goto done;
}
+ printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
ret = -ERFKILL;
goto done;
}
+ printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
if (test_bit(HCI_UP, &hdev->flags)) {
ret = -EALREADY;
goto done;
}
+ printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
if (hdev->open(hdev)) {
ret = -EIO;
goto done;
}
+ printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
atomic_set(&hdev->cmd_cnt, 1);
set_bit(HCI_INIT, &hdev->flags);
@@ -1217,6 +1222,7 @@ int hci_dev_open(__u16 dev)
if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
ret = hdev->setup(hdev);
+ printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
if (!ret) {
/* Treat all non BR/EDR controllers as raw devices if
* enable_hs is not set.
@@ -1664,6 +1670,7 @@ static void hci_power_on(struct work_struct *work)
BT_DBG("%s", hdev->name);
+ printk("BT_DBG: enter: hci_power_on line=%d pid=%d\n", __LINE__, current->pid);
err = hci_dev_open(hdev->id);
if (err < 0) {
mgmt_set_powered_failed(hdev, err);
@@ -1674,8 +1681,10 @@ static void hci_power_on(struct work_struct *work)
queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
HCI_AUTO_OFF_TIMEOUT);
+ printk("BT_DBG: hci_power_on line=%d updating HCI_SETUP hdev flag pid=%d\n", __LINE__, current->pid);
if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags))
mgmt_index_added(hdev);
+ printk("BT_DBG: exit: hci_power_on line=%d pid=%d\n", __LINE__, current->pid);
}
static void hci_power_off(struct work_struct *work)
@@ -2234,6 +2243,7 @@ int hci_register_dev(struct hci_dev *hdev)
{
int id, error;
+ printk("BT_DBG: enter: hci_register_dev\n");
if (!hdev->open || !hdev->close)
return -EINVAL;
@@ -2300,7 +2310,9 @@ int hci_register_dev(struct hci_dev *hdev)
hci_notify(hdev, HCI_DEV_REG);
hci_dev_hold(hdev);
+ printk("BT_DBG: in hci_register_dev queueing work power_on\n");
queue_work(hdev->req_workqueue, &hdev->power_on);
+ printk("BT_DBG: exit: hci_register_dev\n");
return id;
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index c09e976..ddefcbe 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -587,6 +587,7 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
case HCIDEVUP:
if (!capable(CAP_NET_ADMIN))
return -EPERM;
+ printk("BT_DBG: calling hci_dev_open from hci_sock.c pid=%d\n", current->pid);
return hci_dev_open(arg);
case HCIDEVDOWN:
^ permalink raw reply related
* Re: [PATCH 7/8] Bluetooth: Add new mgmt setting for LE advertising
From: Marcel Holtmann @ 2013-09-24 18:29 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380031363-1266-8-git-send-email-johan.hedberg@gmail.com>
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
* Re: [PATCH 6/8] Bluetooth: Use async request for LE enable/disable
From: Marcel Holtmann @ 2013-09-24 18:20 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380031363-1266-7-git-send-email-johan.hedberg@gmail.com>
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
* Re: [PATCH 3/8] Bluetooth: Test for HCI_SETUP and HCI_USER_CHANNEL in mgmt_valid_hdev()
From: Marcel Holtmann @ 2013-09-24 18:19 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380031363-1266-4-git-send-email-johan.hedberg@gmail.com>
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
* Re: [PATCH 4/8] Bluetooth: Fix busy return for mgmt_set_powered in some cases
From: Marcel Holtmann @ 2013-09-24 18:17 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380031363-1266-5-git-send-email-johan.hedberg@gmail.com>
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
* Re: [PATCH 2/8] Bluetooth: Clean up socket locking in l2cap_sock_recvmsg
From: Marcel Holtmann @ 2013-09-24 18:16 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380031363-1266-3-git-send-email-johan.hedberg@gmail.com>
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
* Re: [PATCH 1/8] Bluetooth: Add clarifying comment to bt_sock_wait_state()
From: Marcel Holtmann @ 2013-09-24 18:11 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380031363-1266-2-git-send-email-johan.hedberg@gmail.com>
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
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox