* Add CSA 4 commands and events in hci.h
From: Dohyun Pyun @ 2013-10-02 9:11 UTC (permalink / raw)
To: linux-bluetooth; +Cc: steve.jun
Hi all,
This patchset contains several HCI commands and events addition.
These HCI commands and events are included in Bluetooth Core
Specification Addendum 4, and it will be used to implement 3DS
profile.
Also I'm still implementing 3DS profile's master role in the BR/EDR
controller. Summit and I'll send the patchsets about 3DS MGMT APIs
soon.
Regards,
Pyun
^ permalink raw reply
* [PATCH 2/2] Bluetooth: Add a new mgmt_set_bredr command
From: Johan Hedberg @ 2013-10-02 9:10 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380705040-16300-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
This patch introduces a new mgmt command for enabling/disabling BR/EDR
functionality. This can be convenient when one wants to make a dual-mode
controller behave like a single-mode one. The command is only available
for dual-mode controllers and requires that LE is enabled before using
it. The BR/EDR setting can be enabled at any point, however disabling it
requires the controller to be powered off (otherwise a "rejected"
response will be sent).
Disabling the BR/EDR setting will automatically disable all other BR/EDR
related settings.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/mgmt.h | 2 +
net/bluetooth/hci_event.c | 5 ++
net/bluetooth/mgmt.c | 124 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 131 insertions(+)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 421d763..7347df8 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -354,6 +354,8 @@ struct mgmt_cp_set_device_id {
#define MGMT_OP_SET_ADVERTISING 0x0029
+#define MGMT_OP_SET_BREDR 0x002A
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d171c04b..4785ab0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -297,6 +297,11 @@ static void hci_cc_write_scan_enable(struct hci_dev *hdev, struct sk_buff *skb)
goto done;
}
+ /* We need to ensure that we set this back on if someone changed
+ * the scan mode through a raw HCI socket.
+ */
+ set_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+
old_pscan = test_and_clear_bit(HCI_PSCAN, &hdev->flags);
old_iscan = test_and_clear_bit(HCI_ISCAN, &hdev->flags);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e1c41b0..8b9b337 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -75,6 +75,7 @@ static const u16 mgmt_commands[] = {
MGMT_OP_UNBLOCK_DEVICE,
MGMT_OP_SET_DEVICE_ID,
MGMT_OP_SET_ADVERTISING,
+ MGMT_OP_SET_BREDR,
};
static const u16 mgmt_events[] = {
@@ -3337,6 +3338,125 @@ unlock:
return err;
}
+static void set_bredr_complete(struct hci_dev *hdev, u8 status)
+{
+ struct pending_cmd *cmd;
+
+ BT_DBG("status 0x%02x", status);
+
+ hci_dev_lock(hdev);
+
+ cmd = mgmt_pending_find(MGMT_OP_SET_BREDR, hdev);
+ if (!cmd)
+ goto unlock;
+
+ if (status) {
+ u8 mgmt_err = mgmt_status(status);
+
+ /* We need to restore the flag if related HCI commands
+ * failed.
+ */
+ clear_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+
+ cmd_status(cmd->sk, cmd->index, cmd->opcode, mgmt_err);
+ } else {
+ send_settings_rsp(cmd->sk, MGMT_OP_SET_BREDR, hdev);
+ new_settings(hdev, cmd->sk);
+ }
+
+ mgmt_pending_remove(cmd);
+
+unlock:
+ hci_dev_unlock(hdev);
+}
+
+static int set_bredr(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_bredr_capable(hdev) || !lmp_le_capable(hdev))
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
+ MGMT_STATUS_REJECTED);
+
+ if (!test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
+ MGMT_STATUS_REJECTED);
+
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
+ MGMT_STATUS_INVALID_PARAMS);
+
+ hci_dev_lock(hdev);
+
+ val = !!cp->val;
+ enabled = test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+
+ if (val == enabled) {
+ err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
+ goto unlock;
+ }
+
+ if (!hdev_is_powered(hdev)) {
+ if (!val) {
+ clear_bit(HCI_CONNECTABLE, &hdev->dev_flags);
+ clear_bit(HCI_DISCOVERABLE, &hdev->dev_flags);
+ clear_bit(HCI_SSP_ENABLED, &hdev->dev_flags);
+ clear_bit(HCI_LINK_SECURITY, &hdev->dev_flags);
+ clear_bit(HCI_FAST_CONNECTABLE, &hdev->dev_flags);
+ clear_bit(HCI_HS_ENABLED, &hdev->dev_flags);
+ }
+
+ change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+
+ err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
+ if (err < 0)
+ goto unlock;
+
+ err = new_settings(hdev, sk);
+ goto unlock;
+ }
+
+ /* Reject disabling when powered on */
+ if (!val) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
+ MGMT_STATUS_REJECTED);
+ goto unlock;
+ }
+
+ if (mgmt_pending_find(MGMT_OP_SET_BREDR, hdev)) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
+ MGMT_STATUS_BUSY);
+ goto unlock;
+ }
+
+ cmd = mgmt_pending_add(sk, MGMT_OP_SET_BREDR, hdev, data, len);
+ if (!cmd) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ /* We need to flip the bit already here so that hci_update_ad
+ * generates the correct flags.
+ */
+ set_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+
+ hci_req_init(&req, hdev);
+ hci_update_ad(&req);
+ err = hci_req_run(&req, set_bredr_complete);
+ if (err < 0)
+ mgmt_pending_remove(cmd);
+
+unlock:
+ hci_dev_unlock(hdev);
+ return err;
+}
+
static bool ltk_is_valid(struct mgmt_ltk_info *key)
{
if (key->authenticated != 0x00 && key->authenticated != 0x01)
@@ -3452,6 +3572,7 @@ static const struct mgmt_handler {
{ unblock_device, false, MGMT_UNBLOCK_DEVICE_SIZE },
{ set_device_id, false, MGMT_SET_DEVICE_ID_SIZE },
{ set_advertising, false, MGMT_SETTING_SIZE },
+ { set_bredr, false, MGMT_SETTING_SIZE },
};
@@ -3633,6 +3754,9 @@ static int powered_update_hci(struct hci_dev *hdev)
cp.simul != lmp_host_le_br_capable(hdev))
hci_req_add(&req, HCI_OP_WRITE_LE_HOST_SUPPORTED,
sizeof(cp), &cp);
+
+ /* In case BR/EDR was toggled during the AUTO_OFF phase */
+ hci_update_ad(&req);
}
if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags)) {
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/2] Bluetooth: Introduce a new HCI_BREDR_ENABLED flag
From: Johan Hedberg @ 2013-10-02 9:10 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1380705040-16300-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
To allow treating dual-mode (BR/EDR/LE) controllers as single-mode ones
(LE-only) we want to introduce a new HCI_BREDR_ENABLED flag to track
whether BR/EDR is enabled or not (previously we simply looked at the
feature bit with lmp_bredr_enabled).
This patch add the new flag and updates the relevant places to test
against it instead of using lmp_bredr_enabled. The flag is by default
enabled when registering an adapter and only cleared if necessary once
the local features have been read during the HCI init procedure.
We cannot completely block BR/EDR usage in case user space uses raw HCI
sockets but the patch tries to block this in places where possible, such
as the various BR/EDR specific ioctls.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_conn.c | 3 +++
net/bluetooth/hci_core.c | 21 +++++++++++++++++++--
net/bluetooth/mgmt.c | 24 +++++++++++++-----------
4 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7ede266..4fa08d7 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -122,6 +122,7 @@ enum {
HCI_LINK_SECURITY,
HCI_PERIODIC_INQ,
HCI_FAST_CONNECTABLE,
+ HCI_BREDR_ENABLED,
};
/* A mask for the flags that are supposed to remain when a reset happens
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d2380e0..514148b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -581,6 +581,9 @@ static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
{
struct hci_conn *acl;
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
+ return ERR_PTR(-ENOTSUPP);
+
acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
if (!acl) {
acl = hci_conn_add(hdev, ACL_LINK, dst);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7cbdd33..ed310e9 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -519,6 +519,8 @@ static void hci_init2_req(struct hci_request *req, unsigned long opt)
if (lmp_bredr_capable(hdev))
bredr_setup(req);
+ else
+ clear_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
if (lmp_le_capable(hdev))
le_setup(req);
@@ -1034,6 +1036,11 @@ int hci_inquiry(void __user *arg)
goto done;
}
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
+ err = -ENOTSUPP;
+ goto done;
+ }
+
hci_dev_lock(hdev);
if (inquiry_cache_age(hdev) > INQUIRY_CACHE_AGE_MAX ||
inquiry_cache_empty(hdev) || ir.flags & IREQ_CACHE_FLUSH) {
@@ -1101,7 +1108,7 @@ static u8 create_ad(struct hci_dev *hdev, u8 *ptr)
if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
flags |= LE_AD_GENERAL;
- if (!lmp_bredr_capable(hdev))
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
flags |= LE_AD_NO_BREDR;
if (lmp_le_br_capable(hdev))
@@ -1493,6 +1500,11 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg)
goto done;
}
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
+ err = -ENOTSUPP;
+ goto done;
+ }
+
switch (cmd) {
case HCISETAUTH:
err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt,
@@ -2318,8 +2330,13 @@ int hci_register_dev(struct hci_dev *hdev)
set_bit(HCI_SETUP, &hdev->dev_flags);
- if (hdev->dev_type != HCI_AMP)
+ if (hdev->dev_type != HCI_AMP) {
set_bit(HCI_AUTO_OFF, &hdev->dev_flags);
+ /* Assume BR/EDR support until proven otherwise (such as
+ * through reading supported features during init.
+ */
+ set_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+ }
write_lock(&hci_dev_list_lock);
list_add(&hdev->list, &hci_dev_list);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ad38629..e1c41b0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -408,7 +408,7 @@ static u32 get_current_settings(struct hci_dev *hdev)
if (test_bit(HCI_PAIRABLE, &hdev->dev_flags))
settings |= MGMT_SETTING_PAIRABLE;
- if (lmp_bredr_capable(hdev))
+ if (test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
settings |= MGMT_SETTING_BREDR;
if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
@@ -929,7 +929,7 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
BT_DBG("request for %s", hdev->name);
- if (!lmp_bredr_capable(hdev))
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
MGMT_STATUS_NOT_SUPPORTED);
@@ -1085,7 +1085,7 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
BT_DBG("request for %s", hdev->name);
- if (!lmp_bredr_capable(hdev))
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
MGMT_STATUS_NOT_SUPPORTED);
@@ -1208,7 +1208,7 @@ static int set_link_security(struct sock *sk, struct hci_dev *hdev, void *data,
BT_DBG("request for %s", hdev->name);
- if (!lmp_bredr_capable(hdev))
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
MGMT_STATUS_NOT_SUPPORTED);
@@ -1342,7 +1342,7 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
BT_DBG("request for %s", hdev->name);
- if (!lmp_bredr_capable(hdev))
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
MGMT_STATUS_NOT_SUPPORTED);
@@ -1409,7 +1409,7 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
MGMT_STATUS_INVALID_PARAMS);
/* LE-only devices do not allow toggling LE on/off */
- if (!lmp_bredr_capable(hdev))
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
MGMT_STATUS_REJECTED);
@@ -1720,7 +1720,7 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
BT_DBG("request for %s", hdev->name);
- if (!lmp_bredr_capable(hdev))
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
return cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
MGMT_STATUS_NOT_SUPPORTED);
@@ -2803,7 +2803,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
switch (hdev->discovery.type) {
case DISCOV_TYPE_BREDR:
- if (!lmp_bredr_capable(hdev)) {
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
MGMT_STATUS_NOT_SUPPORTED);
mgmt_pending_remove(cmd);
@@ -2835,7 +2835,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
}
if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
- !lmp_bredr_capable(hdev)) {
+ !test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
MGMT_STATUS_NOT_SUPPORTED);
mgmt_pending_remove(cmd);
@@ -3282,7 +3282,8 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
BT_DBG("%s", hdev->name);
- if (!lmp_bredr_capable(hdev) || hdev->hci_ver < BLUETOOTH_VER_1_2)
+ if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags) ||
+ hdev->hci_ver < BLUETOOTH_VER_1_2)
return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
MGMT_STATUS_NOT_SUPPORTED);
@@ -3646,7 +3647,8 @@ static int powered_update_hci(struct hci_dev *hdev)
sizeof(link_sec), &link_sec);
if (lmp_bredr_capable(hdev)) {
- set_bredr_scan(&req);
+ if (test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
+ set_bredr_scan(&req);
update_class(&req);
update_name(&req);
update_eir(&req);
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/2] Bluetooth: Add a new mgmt_set_bredr command
From: Johan Hedberg @ 2013-10-02 9:10 UTC (permalink / raw)
To: linux-bluetooth
Hi,
This is a new revision of the previous RFC with all feedback taken into
account. Additionally, the setting can now only be disabled while
powered off which means that the second patch from my previous set
becomes unnecessary (and has therefore been dropped).
Johan
----------------------------------------------------------------
Johan Hedberg (2):
Bluetooth: Introduce a new HCI_BREDR_ENABLED flag
Bluetooth: Add a new mgmt_set_bredr command
include/net/bluetooth/hci.h | 1 +
include/net/bluetooth/mgmt.h | 2 +
net/bluetooth/hci_conn.c | 3 +
net/bluetooth/hci_core.c | 21 +++++-
net/bluetooth/hci_event.c | 5 ++
net/bluetooth/mgmt.c | 148 ++++++++++++++++++++++++++++++++++++++----
6 files changed, 167 insertions(+), 13 deletions(-)
^ permalink raw reply
* Re: [PATCH v6 0/4] Bluetooth: btmrvl cal data downloading
From: Marcel Holtmann @ 2013-10-02 7:37 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth, Gustavo Padovan, Johan Hedberg, linux-wireless,
Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar
In-Reply-To: <1380655155-10007-1-git-send-email-bzhao@marvell.com>
Hi Bing,
> This series adds the calibration data downloading support
> along with improvements in sending commands and setup handler.
>
> Amitkumar Karwar (4):
> Bluetooth: btmrvl: add btmrvl_send_sync_cmd() function
> Bluetooth: btmrvl: get rid of struct btmrvl_cmd
> Bluetooth: btmrvl: add setup handler
> Bluetooth: btmrvl: add calibration data download support
>
> drivers/bluetooth/btmrvl_drv.h | 12 +-
> drivers/bluetooth/btmrvl_main.c | 269 ++++++++++++++++++++++++++--------------
> drivers/bluetooth/btmrvl_sdio.c | 15 +--
> drivers/bluetooth/btmrvl_sdio.h | 2 +
> 4 files changed, 193 insertions(+), 105 deletions(-)
I have decided to apply all 4 patches to bluetooth-next. However please send a follow up patch that changes the code to operate on 16-bit opcodes and not the OGC/OCF and its packing.
Regards
Marcel
^ permalink raw reply
* [PATCH] Bluetooth: Restrict disabling of HS when controller is powered off
From: Marcel Holtmann @ 2013-10-02 7:27 UTC (permalink / raw)
To: linux-bluetooth
Disabling the high speed setting when the controller is powered on has
too many side effects that are not taken care of. And in general it
is not an useful operation anyway. So just make such a command fail
with a rejection error message.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/mgmt.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ad38629..35d3c12 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1352,10 +1352,17 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
hci_dev_lock(hdev);
- if (cp->val)
+ if (cp->val) {
changed = !test_and_set_bit(HCI_HS_ENABLED, &hdev->dev_flags);
- else
+ } else {
+ if (hdev_is_powered(hdev)) {
+ err = cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
+ MGMT_STATUS_REJECTED);
+ goto unlock;
+ }
+
changed = test_and_clear_bit(HCI_HS_ENABLED, &hdev->dev_flags);
+ }
err = send_settings_rsp(sk, MGMT_OP_SET_HS, hdev);
if (err < 0)
--
1.8.3.1
^ permalink raw reply related
* Re: [RFC 3/3] Bluetooth: Add a new mgmt_set_bredr command
From: Andrei Emeltchenko @ 2013-10-02 7:07 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380640922-18647-4-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
On Tue, Oct 01, 2013 at 06:22:02PM +0300, johan.hedberg@gmail.com wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> This patch introduces a new mgmt command for enabling/disabling BR/EDR
> functionality. This can be convenient when one wants to make a dual-mode
> controller behave like a single-mode one. The command is only available
> for dual-mode controllers and requires that LE is enabled before using
> it.
...
> +
> + 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;
You have already checked that cp->val is 0x0 or 0x1, right?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH] Bluetooth: Enable -D__CHECK_ENDIAN__ for sparse by default
From: Johan Hedberg @ 2013-10-02 6:41 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1380694007-7946-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Tue, Oct 01, 2013, Marcel Holtmann wrote:
> The Bluetooth protocol and hardware is pretty much all little endian
> and so when running sparse via "make C=2" for example, enable the
> endian checks by default.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> drivers/bluetooth/Makefile | 2 ++
> net/bluetooth/Makefile | 2 ++
> 2 files changed, 4 insertions(+)
Applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: Require CAP_NET_ADMIN for HCI User Channel operation
From: Johan Hedberg @ 2013-10-02 6:40 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1380638401-39890-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Tue, Oct 01, 2013, Marcel Holtmann wrote:
> The HCI User Channel operation is an admin operation that puts the
> device into promiscuous mode for single use. It is more suitable
> to require CAP_NET_ADMIN than CAP_NET_RAW.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/hci_sock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: Send new settings event when changing high speed option
From: Johan Hedberg @ 2013-10-02 6:40 UTC (permalink / raw)
To: Marcel Holtmann, linux-bluetooth
In-Reply-To: <20131001062829.GA5699@x220.p-661hnu-f1>
Hi Marcel,
On Tue, Oct 01, 2013, Johan Hedberg wrote:
> On Mon, Sep 30, 2013, Marcel Holtmann wrote:
> > When enabling or disabling high speed setting it is required to send
> > a new settings event to inform other management interface users about
> > the changed settings.
> >
> > Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> > ---
> > net/bluetooth/mgmt.c | 19 ++++++++++++++++---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
>
> Acked-by: Johan Hedberg <johan.hedberg@intel.com>
Applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 1/3] Bluetooth: Use only 2 bits for controller type information
From: Johan Hedberg @ 2013-10-02 6:39 UTC (permalink / raw)
To: Marcel Holtmann, linux-bluetooth
In-Reply-To: <20130930181632.GB26405@x220.p-661hnu-f1>
Hi Marcel,
On Mon, Sep 30, 2013, Johan Hedberg wrote:
> On Sun, Sep 29, 2013, Marcel Holtmann wrote:
> > The controller type is limited to BR/EDR/LE and AMP controllers. This
> > can be easily encoded with just 2 bits and still leave enough room
> > for future controller types.
> >
> > Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> > ---
> > net/bluetooth/hci_core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Acked-by: Johan Hedberg <johan.hedberg@intel.com>
All three patches in this set have been applied to bluetooth-next.
Johan
^ permalink raw reply
* Re: [PATCH v2 0/2] Bluetooth: Fix hci_dev_open race condition
From: Marcel Holtmann @ 2013-10-02 6:30 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380656690-22246-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> Here's an updated set including fixes based on feedback. The patches
> should go to bluetooth.git, but I'm not sure how important it is to try
> to get them to the stable tree since we haven't had this issue reported
> with the Intel setup procedure which (afaik) should be the only one that
> could be affected in older kernels.
>
> Johan
>
> ----------------------------------------------------------------
> Johan Hedberg (2):
> Bluetooth: Refactor hci_dev_open to a separate hci_dev_do_open function
> Bluetooth: Fix workqueue synchronization in hci_dev_open
>
> net/bluetooth/hci_core.c | 40 ++++++++++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 10 deletions(-)
both patches have been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* [PATCH] Bluetooth: Enable -D__CHECK_ENDIAN__ for sparse by default
From: Marcel Holtmann @ 2013-10-02 6:06 UTC (permalink / raw)
To: linux-bluetooth
The Bluetooth protocol and hardware is pretty much all little endian
and so when running sparse via "make C=2" for example, enable the
endian checks by default.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
drivers/bluetooth/Makefile | 2 ++
net/bluetooth/Makefile | 2 ++
2 files changed, 4 insertions(+)
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 4afae20..9fe8a87 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -30,3 +30,5 @@ hci_uart-$(CONFIG_BT_HCIUART_LL) += hci_ll.o
hci_uart-$(CONFIG_BT_HCIUART_ATH3K) += hci_ath.o
hci_uart-$(CONFIG_BT_HCIUART_3WIRE) += hci_h5.o
hci_uart-objs := $(hci_uart-y)
+
+ccflags-y += -D__CHECK_ENDIAN__
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index dea6a28..6a791e7 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_BT_HIDP) += hidp/
bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
hci_sock.o hci_sysfs.o l2cap_core.o l2cap_sock.o smp.o sco.o lib.o \
a2mp.o amp.o
+
+subdir-ccflags-y += -D__CHECK_ENDIAN__
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v6 4/4] Bluetooth: btmrvl: add calibration data download support
From: Marcel Holtmann @ 2013-10-02 5:32 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth, Gustavo Padovan, Johan Hedberg, linux-wireless,
Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar
In-Reply-To: <1380655155-10007-5-git-send-email-bzhao@marvell.com>
Hi Bing,
> A text file containing calibration data in hex format can
> be provided at following path:
>
> /lib/firmware/mrvl/sd8797_caldata.conf
>
> The data will be downloaded to firmware during initialization.
>
> Reviewed-by: Mike Frysinger <vapier@chromium.org>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> Signed-off-by: Hyuckjoo Lee <hyuckjoo.lee@samsung.com>
> ---
> v2: Remove module parameter. The calibration data will be downloaded
> only when the device speicific data file is provided.
> (Marcel Holtmann)
> v3: Fix crash (misaligned memory access) on ARM
> v4: Simplify white space parsing and save some CPU cycles (Mike Frysinger)
> v5: Improvements in cal data parsing logic. Add explanatory comments.
> Replace GFP_ATOMIC flag with GFP_KERNEL (Mike Frysinger)
> v6: Remove redundant label 'done' and 'cfg' check, and a new line character check
> (Mike Frysinger)
> Use btmrvl_send_sync_cmd() for downloading calibration data.
> (Marcel Holtmann)
>
> drivers/bluetooth/btmrvl_drv.h | 8 +++
> drivers/bluetooth/btmrvl_main.c | 116 ++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btmrvl_sdio.c | 9 +++-
> drivers/bluetooth/btmrvl_sdio.h | 2 +
> 4 files changed, 134 insertions(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v6 1/4] Bluetooth: btmrvl: add btmrvl_send_sync_cmd() function
From: Marcel Holtmann @ 2013-10-02 5:30 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth, Gustavo Padovan, Johan Hedberg, linux-wireless,
Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar
In-Reply-To: <1380655155-10007-2-git-send-email-bzhao@marvell.com>
Hi Bing,
> Command preparation code is used multiple times. This patch
> separate out this common code and create btmrvl_send_sync_cmd()
> function.
>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
> v6: separate out common code
>
> drivers/bluetooth/btmrvl_main.c | 129 +++++++++++++---------------------------
> 1 file changed, 41 insertions(+), 88 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v6 3/4] Bluetooth: btmrvl: add setup handler
From: Marcel Holtmann @ 2013-10-02 5:28 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth, Gustavo Padovan, Johan Hedberg, linux-wireless,
Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar
In-Reply-To: <1380655155-10007-4-git-send-email-bzhao@marvell.com>
Hi Bing,
> Move initialization code to hdev's setup handler.
>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
> v6: remove setup_done variable (Marcel Holtmann)
> This change requires a fix in hci_core for hci_setup.
> v5: make use of hdev's setup handler (Marcel Holtmann)
>
> drivers/bluetooth/btmrvl_main.c | 18 ++++++++++++++++--
> drivers/bluetooth/btmrvl_sdio.c | 6 ------
> 2 files changed, 16 insertions(+), 8 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v6 2/4] Bluetooth: btmrvl: get rid of struct btmrvl_cmd
From: Marcel Holtmann @ 2013-10-02 5:26 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth, Gustavo Padovan, Johan Hedberg, linux-wireless,
Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar
In-Reply-To: <1380655155-10007-3-git-send-email-bzhao@marvell.com>
Hi Bing,
> Replace this proprietary structure with the standard one
> (struct hci_command_hdr).
>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
> v6: remove proprietary struct btmrvl_cmd
>
> drivers/bluetooth/btmrvl_drv.h | 6 ------
> drivers/bluetooth/btmrvl_main.c | 12 ++++++------
> 2 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
> index 27068d1..42f7028 100644
> --- a/drivers/bluetooth/btmrvl_drv.h
> +++ b/drivers/bluetooth/btmrvl_drv.h
> @@ -116,12 +116,6 @@ struct btmrvl_private {
> #define PS_SLEEP 0x01
> #define PS_AWAKE 0x00
>
> -struct btmrvl_cmd {
> - __le16 ocf_ogf;
> - u8 length;
> - u8 data[4];
> -} __packed;
> -
> struct btmrvl_event {
> u8 ec; /* event counter */
> u8 length;
> diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
> index d9d4229..a4da7c8 100644
> --- a/drivers/bluetooth/btmrvl_main.c
> +++ b/drivers/bluetooth/btmrvl_main.c
> @@ -170,20 +170,20 @@ static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
> const void *param, u8 len)
> {
> struct sk_buff *skb;
> - struct btmrvl_cmd *cmd;
> + struct hci_command_hdr *hdr;
>
> - skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
> + skb = bt_skb_alloc(HCI_COMMAND_HDR_SIZE + len, GFP_ATOMIC);
> if (skb == NULL) {
> BT_ERR("No free skb");
> return -ENOMEM;
> }
>
> - cmd = (struct btmrvl_cmd *) skb_put(skb, sizeof(*cmd));
> - cmd->ocf_ogf = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
> - cmd->length = len;
> + hdr = (struct hci_command_hdr *)skb_put(skb, HCI_COMMAND_HDR_SIZE);
> + hdr->opcode = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
> + hdr->plen = len;
I like this change a lot since it makes the code much simpler. Now if you would also just use the full 16-bit opcode instead of this pack function it would become dead simple. Especially then you can use __constant_cpu_to_le16.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 7/7] Bluetooth: Locking in hci_le_conn_complete_evt
From: Marcel Holtmann @ 2013-10-02 5:23 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380668636-30654-8-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch moves hci_dev_lock and hci_dev_unlock calls to where they
> are really required, reducing the critical region in hci_le_conn_
> complete_evt function. hdev->lock is required only in hci_conn_del
> and hci_conn_add call to protect concurrent add and remove operations
> in hci_conn_hash list.
is this statement actually true? Because we have done this for so many HCI event, that I highly doubt that your statement tis correct. And if it is correct, you need to fix all other users first.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler
From: Marcel Holtmann @ 2013-10-02 5:21 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380668636-30654-7-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch does some code refactorig in LE Connection Complete HCI
> event handler. It basically adds a switch statement to separate new
> master connection code from new slave connection code.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_event.c | 55 ++++++++++++++++++++++++++++++++-------------
> 2 files changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 7ede266..8c98f60 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
>
> /* Low energy meta events */
> #define LE_CONN_ROLE_MASTER 0x00
> +#define LE_CONN_ROLE_SLAVE 0x01
>
> #define HCI_EV_LE_CONN_COMPLETE 0x01
> struct hci_ev_le_conn_complete {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 1d1ffa6..0e4a9f4 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>
> hci_dev_lock(hdev);
>
> - conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> - if (!conn) {
> + if (ev->status) {
> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + if (!conn)
> + goto unlock;
> +
> + mgmt_connect_failed(hdev, &conn->dst, conn->type,
> + conn->dst_type, ev->status);
> + hci_proto_connect_cfm(conn, ev->status);
> + conn->state = BT_CLOSED;
> + hci_conn_del(conn);
> + goto unlock;
> + }
> +
> + switch (ev->role) {
> + case LE_CONN_ROLE_MASTER:
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr);
> + /* If there is no hci_conn object with the given address, it
> + * means this new connection was triggered through HCI socket
> + * interface. For that case, we should create a new hci_conn
> + * object.
> + */
this comments belong one level down inside the if block. You already commenting on the negative outcome of the if check.
> + if (!conn) {
> + conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
> + if (!conn) {
> + BT_ERR("No memory for new connection");
> + goto unlock;
> + }
> +
> + conn->out = true;
> + conn->link_mode |= HCI_LM_MASTER;
> + conn->sec_level = BT_SECURITY_LOW;
> + conn->dst_type = ev->bdaddr_type;
> + }
> + break;
> +
> + case LE_CONN_ROLE_SLAVE:
And why are we not checking for an existing connection here? At least a small comment is needed to make that part clear.
> conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
> if (!conn) {
> BT_ERR("No memory for new connection");
> @@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> }
>
> conn->dst_type = ev->bdaddr_type;
> + conn->sec_level = BT_SECURITY_LOW;
> + break;
>
> - if (ev->role == LE_CONN_ROLE_MASTER) {
> - conn->out = true;
> - conn->link_mode |= HCI_LM_MASTER;
> - }
> - }
> -
> - if (ev->status) {
> - mgmt_connect_failed(hdev, &conn->dst, conn->type,
> - conn->dst_type, ev->status);
> - hci_proto_connect_cfm(conn, ev->status);
> - conn->state = BT_CLOSED;
> - hci_conn_del(conn);
> + default:
> + BT_ERR("Used reserved Role parameter %d", ev->role);
> goto unlock;
> }
>
> @@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
> conn->dst_type, 0, NULL, 0, NULL);
>
> - conn->sec_level = BT_SECURITY_LOW;
> conn->handle = __le16_to_cpu(ev->handle);
> conn->state = BT_CONNECTED;
All in all, I am not really understanding why this makes it this code simpler. I actually think it turns it into more complicated code. So please explain what we are really gaining here. I just see more hash table lookup and for hci_conn_add calls with more error checks.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler
From: Marcel Holtmann @ 2013-10-02 5:11 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380668636-30654-5-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch removes the hci_cs_le_create_conn event handler since this
> handling is now done in create_le_connection_complete() callback in
> hci_conn.c.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_event.c | 31 -------------------------------
> 1 file changed, 31 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index d171c04b..1d1ffa6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1465,33 +1465,6 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
> hci_dev_unlock(hdev);
> }
>
> -static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status)
> -{
> - struct hci_conn *conn;
> -
> - BT_DBG("%s status 0x%2.2x", hdev->name, status);
> -
> - if (status) {
> - hci_dev_lock(hdev);
> -
> - conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> - if (!conn) {
> - hci_dev_unlock(hdev);
> - return;
> - }
> -
> - BT_DBG("%s bdaddr %pMR conn %p", hdev->name, &conn->dst, conn);
> -
> - conn->state = BT_CLOSED;
> - mgmt_connect_failed(hdev, &conn->dst, conn->type,
> - conn->dst_type, status);
> - hci_proto_connect_cfm(conn, status);
> - hci_conn_del(conn);
> -
> - hci_dev_unlock(hdev);
> - }
> -}
this is dangerous since it actually breaks bisection. The code is never complete. So while this might turn into a larger patch, you might need to do it all 3 patches at once. With a length commit message explaining exactly what happens and why this is correct.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 3/7] Bluetooth: Remove hci_le_create_connection
From: Marcel Holtmann @ 2013-10-02 5:08 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380668636-30654-4-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> Since we now use hci_initiate_le_connection() helper for creating new
> LE connections, we can safely remove hci_le_create_connection().
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 19 -------------------
> 1 file changed, 19 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 24d1a0a..b89522f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -49,25 +49,6 @@ static const struct sco_param sco_param_wideband[] = {
> { EDR_ESCO_MASK | ESCO_EV3, 0x0008 }, /* T1 */
> };
>
> -static void hci_le_create_connection(struct hci_conn *conn)
> -{
> - struct hci_dev *hdev = conn->hdev;
> - struct hci_cp_le_create_conn cp;
> -
> - memset(&cp, 0, sizeof(cp));
> - cp.scan_interval = __constant_cpu_to_le16(0x0060);
> - cp.scan_window = __constant_cpu_to_le16(0x0030);
> - bacpy(&cp.peer_addr, &conn->dst);
> - cp.peer_addr_type = conn->dst_type;
> - cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> - cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> - cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> - cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> - cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> -
> - hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> -}
> -
> static void hci_le_create_connection_cancel(struct hci_conn *conn)
> {
> hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
I really start to dislike all the super long naming here. Existing one and new that you are defining.
Why not name the new handler hci_le_create_conn() instead of all this initiate_something and remove the existing function at the same time. I mean you can bisect the code, but it will throw a warning of an unused function.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/7] Bluetooth: Use HCI request for LE connection
From: Marcel Holtmann @ 2013-10-02 5:04 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380668636-30654-3-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch adds a new helper for initiating LE conneciton which uses
> the HCI request framework. This patch also changes the hci_connect_le()
> so it uses the new helper instead of the old hci_le_create_connection().
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h | 2 ++
> net/bluetooth/hci_conn.c | 7 +++++-
> net/bluetooth/hci_core.c | 46 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 26cc9f7..6aa172c 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1216,6 +1216,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
>
> u8 bdaddr_to_le(u8 bdaddr_type);
>
> +int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type);
> +
> #define SCO_AIRMODE_MASK 0x0003
> #define SCO_AIRMODE_CVSD 0x0000
> #define SCO_AIRMODE_TRANSP 0x0003
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index f473605..24d1a0a 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -545,6 +545,7 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> u8 dst_type, u8 sec_level, u8 auth_type)
> {
> struct hci_conn *le;
> + int err;
>
> if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
> return ERR_PTR(-ENOTSUPP);
> @@ -565,7 +566,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> le->link_mode |= HCI_LM_MASTER;
> le->sec_level = BT_SECURITY_LOW;
>
> - hci_le_create_connection(le);
> + err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> + if (err) {
> + hci_conn_del(le);
> + return ERR_PTR(err);
> + }
> }
>
> le->pending_sec_level = sec_level;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 4549b5c..51c1796 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3631,3 +3631,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
> return ADDR_LE_DEV_RANDOM;
> }
> }
> +
> +static void initiate_le_connection_complete(struct hci_dev *hdev, u8 status)
> +{
> + struct hci_conn *conn;
> +
> + if (status == 0)
> + return;
> +
> + BT_ERR("HCI request failed to initiate LE connection: status 0x%2.2x",
> + status);
> +
> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + if (!conn)
> + return;
> +
> + mgmt_connect_failed(hdev, &conn->dst, conn->type, conn->dst_type,
> + status);
> +
> + hci_proto_connect_cfm(conn, status);
> +
> + hci_dev_lock(hdev);
> + hci_conn_del(conn);
> + hci_dev_unlock(hdev);
> +}
> +
> +int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
> +{
> + struct hci_cp_le_create_conn cp;
> + struct hci_request req;
> +
> + hci_req_init(&req, hdev);
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.scan_interval = __constant_cpu_to_le16(0x0060);
> + cp.scan_window = __constant_cpu_to_le16(0x0030);
> + bacpy(&cp.peer_addr, addr);
> + cp.peer_addr_type = type;
> + cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> + cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> + cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> + cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> + cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> + hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> +
> + return hci_req_run(&req, initiate_le_connection_complete);
> +}
so how does this actually work. The command status handling for errors is now run twice? Once in hci_cs_le_create_conn() and once in the complete callback.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/7] Bluetooth: Initialize hci_conn fields in hci_connect_le
From: Marcel Holtmann @ 2013-10-02 4:57 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1380668636-30654-2-git-send-email-andre.guedes@openbossa.org>
Hi Andre,
> This patch moves some hci_conn fields initialization from hci_le_
> create_connection() to hci_connect_le(). It makes more sense to
> initialize these fields within the function that creates the hci_
> conn object.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index d2380e0..f473605 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -54,11 +54,6 @@ static void hci_le_create_connection(struct hci_conn *conn)
> struct hci_dev *hdev = conn->hdev;
> struct hci_cp_le_create_conn cp;
>
> - conn->state = BT_CONNECT;
> - conn->out = true;
> - conn->link_mode |= HCI_LM_MASTER;
> - conn->sec_level = BT_SECURITY_LOW;
> -
> memset(&cp, 0, sizeof(cp));
> cp.scan_interval = __constant_cpu_to_le16(0x0060);
> cp.scan_window = __constant_cpu_to_le16(0x0030);
> @@ -565,6 +560,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> return ERR_PTR(-ENOMEM);
>
> le->dst_type = bdaddr_to_le(dst_type);
> + le->state = BT_CONNECT;
> + le->out = true;
> + le->link_mode |= HCI_LM_MASTER;
> + le->sec_level = BT_SECURITY_LOW;
> +
> hci_le_create_connection(le);
> }
I do not understand on how this is the same. Maybe the confusion is the use of le-> instead of conn-> as variable for hci_conn. Seems that should be fixed first.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2 2/2] Bluetooth: Fix workqueue synchronization in hci_dev_open
From: Marcel Holtmann @ 2013-10-02 4:52 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380656690-22246-3-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> When hci_sock.c calls hci_dev_open it needs to ensure that there isn't
> pending work in progress, such as that which is scheduled for the
> initial setup procedure or the one for automatically powering off after
> the setup procedure. This adds the necessary calls to ensure that any
> previously scheduled work is completed before attempting to call
> hci_dev_do_open.
>
> This patch fixes a race with old user space versions where we might
> receive a HCIDEVUP ioctl before the setup procedure has been completed.
> When that happens the setup procedures callback may fail early and leave
> the device in an inconsistent state, causing e.g. the setup callback to
> be (incorrectly) called more than once.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 5/7] Bluetooth: Refactor hci_connect_le
From: Anderson Lizardo @ 2013-10-02 0:05 UTC (permalink / raw)
To: Andre Guedes; +Cc: BlueZ development
In-Reply-To: <1380668636-30654-6-git-send-email-andre.guedes@openbossa.org>
Hi Guedes,
On Tue, Oct 1, 2013 at 7:03 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
> + /* If already exists a hci_conn object for the following connection
> + * attempt, we simply update pending_sec_level and auth_type fields
> + * and return the object found.
> + */
Small textual improvement: "If a hci_conn object already exists [...]"
> le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
> - if (!le) {
> - le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> - if (le)
> - return ERR_PTR(-EBUSY);
> -
> - le = hci_conn_add(hdev, LE_LINK, dst);
> - if (!le)
> - return ERR_PTR(-ENOMEM);
> -
> - le->dst_type = bdaddr_to_le(dst_type);
> - le->state = BT_CONNECT;
> - le->out = true;
> - le->link_mode |= HCI_LM_MASTER;
> - le->sec_level = BT_SECURITY_LOW;
> -
> - err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> - if (err) {
> - hci_conn_del(le);
> - return ERR_PTR(err);
> - }
> + if (le) {
> + le->pending_sec_level = sec_level;
> + le->auth_type = auth_type;
> + goto out;
> }
>
> - le->pending_sec_level = sec_level;
> + /* Since the controller supports only one LE connection attempt at the
> + * time, we return busy if there is any connection attempt running.
> + */
s/at the time/at a time/
s/busy/EBUSY/
> + le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + if (le)
> + return ERR_PTR(-EBUSY);
> +
> + le = hci_conn_add(hdev, LE_LINK, dst);
> + if (!le)
> + return ERR_PTR(-ENOMEM);
> +
> + le->dst_type = bdaddr_to_le(dst_type);
> + le->state = BT_CONNECT;
> + le->out = true;
> + le->link_mode |= HCI_LM_MASTER;
> + le->sec_level = BT_SECURITY_LOW;
> + le->pending_sec_level = BT_SECURITY_LOW;
I think the previous statement should be:
le->pending_sec_level = sec_level;
Otherwise, we are changing semantics.
> le->auth_type = auth_type;
>
> - hci_conn_hold(le);
> + err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> + if (err) {
> + hci_conn_del(le);
> + return ERR_PTR(err);
> + }
>
> +out:
> + hci_conn_hold(le);
> return le;
> }
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ 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