* [RFCv3 00/14] Basic code for Android BlueZ
From: Andrei Emeltchenko @ 2013-10-03 14:38 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Skeletons for Android Bluetooth and Socket HALs and BlueZ daemon.
Note: Still need rebase after patches from Frederic are applied.
Changes:
* RFCv3: Rebased against recent FDanis patch set, fixed library build,
corrected process capabilities, corrected API header.
* RFCv2: Took comments from mailing list reviewers. Use single
makefile, remove mgmt library and directly link, remove adapter
code and use default_adapter pointer and other style changes.
Dropped some patches due to major changes.
Andrei Emeltchenko (14):
android: Add Adapter Bluetooth HAL template
android: Add Socket Bluetooth HAL template
android: Enable Socket interface
android: Start Android Bluetooth daemon
android: Add basic mgmt initialization sequence
android: Create HAL API header skeleton
android: Add adapter and device struct for BlueZ daemon
android: Add Android Makefile for libbluetooth
android: sdp: Reuse BlueZ SDP server in Android
android: Add cap to bind to port < 1024
android: Implement read_info_complete callback
android: Handle mgmt changed events
android: Add makefile for hciconfig
android: Add makefile for hcitool
Makefile.android | 9 +-
android/Android.mk | 163 +++++++++++++++
android/bt_adapter.c | 59 ++++++
android/bt_adapter.h | 60 ++++++
android/hal.h | 18 ++
android/hal_bluetooth.c | 411 +++++++++++++++++++++++++++++++++++++
android/hal_bt_sock.c | 84 ++++++++
android/hal_msg.h | 255 +++++++++++++++++++++++
android/main.c | 516 +++++++++++++++++++++++++++++++++++++++++++++++
android/main.h | 25 +++
configure.ac | 4 +
11 files changed, 1602 insertions(+), 2 deletions(-)
create mode 100644 android/bt_adapter.c
create mode 100644 android/bt_adapter.h
create mode 100644 android/hal.h
create mode 100644 android/hal_bluetooth.c
create mode 100644 android/hal_bt_sock.c
create mode 100644 android/hal_msg.h
create mode 100644 android/main.h
--
1.7.10.4
^ permalink raw reply
* Re: [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler
From: Marcel Holtmann @ 2013-10-03 14:15 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CACJA=fUFak6t2tEc9oFLF0cu_pPOAxf-pY1L-UH1jsFYUU46wg@mail.gmail.com>
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.
>
> Ok, I'll fix it.
>
>>
>>
>>> + 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.
>
> Differently from master connection, there is no existing hci_conn for
> slave connections. For that reason we don't check for an existing
> connection here. I'll add a comment.
>
>>
>>
>>> 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.
>
> When controller sends a LE Connection Complete event to host, we have
> three different handling: failure, new master connection and new slave
> connection. Additionally, new master connection has a special handling
> since the connection could be triggered by HCI socket interface.
> Before, all these logic were mixed up, making hard to add specific
> code for new master connection for instance.
I do not follow the difference between master and slave roles. Why do we have a difference here in the first place.
> So this patch explicitly separates the three different handling and
> adds extra comments aiming to improve hci_le_conn_complete_evt()
> readability. Besides that, keeping these different handling separated,
> it'll be easier to add the auto connection hooks.
I am getting the feeling the overall code gets more complicated than simpler. The goal is to make the connection handling simpler. And right now, I do not see this.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler
From: Marcel Holtmann @ 2013-10-03 14:13 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CACJA=fXGisEzc86N1YKL1Y84wVs5SMRaedEGuMhWJg9A-FV+xQ@mail.gmail.com>
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.
>
> I failed to see how this breaks bisection since the handling is
> already done in initiate_le_connection_complete(). However, as
> commented in patch 2/7, I'll squash this into patch 2/7 as you
> suggested.
once you actually run the code it will break. Which means that bisecting is not possible anymore since now you are chasing the bug of duplicated connection handling.
Regards
Marcel
^ permalink raw reply
* Re: [RFCv2 06/14] android: Create HAL API header skeleton
From: Szymon Janc @ 2013-10-03 14:07 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1380639799-25790-7-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Header describes the protocol between Android HAL threads and BlueZ
> daemon.
> ---
> android/hal_msg.h | 260 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 260 insertions(+)
> create mode 100644 android/hal_msg.h
>
> diff --git a/android/hal_msg.h b/android/hal_msg.h
> new file mode 100644
> index 0000000..2d4436a
> --- /dev/null
> +++ b/android/hal_msg.h
> @@ -0,0 +1,260 @@
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2013 Intel Corporation. All rights reserved.
> + *
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +#ifndef __packed
> +#define __packed __attribute__((packed))
> +#endif
> +
> +typedef struct {
> + uint8_t b[6];
> +} __packed __bdaddr_t;
> +
> +struct hal_msg_hdr {
> + uint8_t service_id;
> + uint8_t opcode;
> + uint16_t len;
> + uint8_t payload[0];
> +} __packed;
> +
> +#define HAL_SERVICE_ID_CORE 0
> +#define HAL_SERVICE_ID_BLUETOOTH 1
SOCK (id 2) is missing here
> +#define HAL_SERVICE_ID_HIDHOST 3
> +#define HAL_SERVICE_ID_PAN 4
> +#define HAL_SERVICE_ID_HANDSFREE 5
> +#define HAL_SERVICE_ID_AD2P 6
> +#define HAL_SERVICE_ID_HEALTH 7
> +#define HAL_SERVICE_ID_AVRCP 8
> +#define HAL_SERVICE_ID_GATT 9
> +
> +/* Core Service */
> +
> +struct hal_msg_rsp {
> + struct hal_msg_hdr hdr;
> + uint8_t status;
> +} __packed;
Non-error response to request has no status field, just header.
> +
> +#define HAL_MSG_OP_ERROR 0x00
> +struct hal_msg_rp_error {
> + uint8_t status;
> +} __packed;
> +
> +#define HAL_MSG_OP_REGISTER_MODULE 0x02
Should be 0x01
> +struct hal_msg_cp_register_module {
> + uint8_t service_id;
> +} __packed;
> +
> +struct hal_msg_rp_register_module {
> + uint8_t service_id;
> +} __packed;
> +
> +#define HAL_MSG_OP_UNREGISTER_MODULE 0x03
Should be 0x02
> +struct hal_msg_cp_unregister_module {
> + uint8_t service_id;
> +} __packed;
> +
> +/* Bluetooth Core HAL API */
> +
> +#define HAL_MSG_OP_BT_ENABLE 0x01
> +
> +#define HAL_MSG_OP_BT_DISABLE 0x02
> +
> +#define HAL_MSG_OP_BT_GET_ADAPTER_PROPS 0x03
> +
> +#define HAL_MSG_OP_BT_GET_ADAPTER_PROP 0x04
> +struct hal_msg_cp_bt_get_adapter_prop {
> + uint8_t type;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_SET_ADAPTER_PROP 0x05
> +struct hal_msg_cp_bt_set_adapter_prop {
> + uint8_t type;
> + uint16_t len;
> + uint8_t val[0];
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_GET_REMOTE_DEVICE_PROPS 0x06
> +struct hal_msg_cp_bt_get_remote_device_props {
> + __bdaddr_t bdaddr;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_GET_REMOTE_DEVICE_PROP 0x07
> +struct hal_msg_cp_bt_get_remote_device_prop {
> + __bdaddr_t bdaddr;
> + uint8_t type;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_SET_REMOTE_DEVICE_PROP 0x08
> +struct hal_msg_cp_bt_set_remote_device_prop {
> + __bdaddr_t bdaddr;
> + uint8_t type;
> + uint16_t len;
> + uint8_t val[0];
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_GET_REMOTE_SERVICE_REC 0x09
> +struct hal_msg_cp_bt_get_remote_service_rec {
> + __bdaddr_t bdaddr;
> + uint8_t uuid[16];
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_GET_REMOTE_SERVICE 0x0a
> +struct hal_msg_cp_bt_get_remote_service {
> + __bdaddr_t bdaddr;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_START_DISCOVERY 0x0b
> +
> +#define HAL_MSG_OP_BT_CANCEL_DISCOVERY 0x0c
> +
> +#define HAL_MSG_OP_BT_CREATE_BOND 0x0d
> +struct hal_msg_cp_bt_create_bond {
> + __bdaddr_t bdaddr;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_REMOVE_BOND 0x0d
> +struct hal_msg_cp_bt_remove_bond {
> + __bdaddr_t bdaddr;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_CANCEL_BOND 0x0f
> +struct hal_msg_cp_bt_cancel_bond {
> + __bdaddr_t bdaddr;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_PIN_REPLY 0x10
> +struct hal_msg_cp_bt_pin_reply {
> + __bdaddr_t bdaddr;
> + uint8_t accept;
> + uint8_t pin_len;
> + uint8_t pin_code[16];
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_SSP_REPLY 0x11
> +struct hal_msg_cp_bt_ssp_reply {
> + __bdaddr_t bdaddr;
> + uint8_t ssp_variant;
> + uint8_t accept;
> + uint32_t passkey;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_DUT_MODE_CONF 0x12
> +struct hal_msg_cp_bt_dut_mode_conf {
> + uint8_t enable;
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_DUT_MODE_SEND 0x13
> +struct hal_msg_cp_bt_dut_mode_send {
> + uint16_t opcode;
> + uint8_t len;
> + uint8_t data[0];
> +} __packed;
> +
> +#define HAL_MSG_OP_BT_LE_TEST_MODE 0x14
> +struct hal_msg_cp_bt_le_test_mode {
> + uint16_t opcode;
> + uint8_t len;
> + uint8_t data[0];
> +} __packed;
> +
> +/* Notifications and confirmations */
> +
> +#define HAL_MSG_EV_BT_ERROR 0x80
> +
> +#define HAL_MSG_EV_BT_ADAPTER_STATE_CHANGED 0x81
> +struct hal_msg_ev_bt_adapter_state_changed {
> + uint8_t state;
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_ADAPTER_PROPS_CHANGED 0x82
> +struct hal_property {
> + uint8_t type;
> + uint16_t len;
> + uint8_t val[0];
> +} __packed;
> +struct hal_msg_ev_bt_adapter_props_changed {
> + uint8_t status;
> + uint8_t num_props;
> + struct hal_property props[0];
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_REMOTE_DEVICE_PROPS 0x83
> +struct hal_msg_ev_bt_remote_device_props {
> + uint8_t status;
> + __bdaddr_t bdaddr;
> + uint8_t num_props;
> + struct hal_property props[0];
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_DEVICE_FOUND 0x84
> +struct hal_msg_ev_bt_device_found {
> + uint8_t num_props;
> + struct hal_property props[0];
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_DISCOVERY_STATE_CHANGED 0x85
> +struct hal_msg_ev_bt_discovery_state_changed {
> + uint8_t state;
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_PIN_REQUEST 0x86
> +struct hal_msg_ev_bt_pin_request {
> + __bdaddr_t bdaddr;
> + uint8_t name[249 - 1];
> + uint8_t class_of_dev[3];
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_SSP_REQUEST 0x87
> +struct hal_msg_ev_bt_ssp_request {
> + __bdaddr_t bdaddr;
> + uint8_t name[249 - 1];
> + uint8_t class_of_dev[3];
> + uint8_t pairing_variant;
> + uint32_t passkey;
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_BOND_STATE_CHANGED 0x88
> +struct hal_msg_ev_bt_bond_state_changed {
> + uint8_t status;
> + __bdaddr_t bdaddr;
> + uint8_t state;
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_ACL_STATE_CHANGED 0x89
> +struct hal_msg_ev_bt_acl_state_changed {
> + uint8_t status;
> + __bdaddr_t bdaddr;
> + uint8_t state;
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_DUT_MODE_RECEIVE 0x8a
> +struct hal_msg_ev_bt_dut_mode_receive {
> + uint16_t opcode;
> + uint8_t len;
> + uint8_t data[0];
> +} __packed;
> +
> +#define HAL_MSG_EV_BT_LE_TEST_MODE 0x8b
> +struct hal_msg_ev_bt_le_test_mode {
> + uint8_t status;
> + uint16_t num_packets;
> +} __packed;
>
--
BR
Szymon Janc
^ permalink raw reply
* Re: [PATCH 7/7] Bluetooth: Locking in hci_le_conn_complete_evt
From: Andre Guedes @ 2013-10-03 14:06 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <B5036FD2-37E8-491E-B35E-16F1F2625A3E@holtmann.org>
Hi Marcel,
On Wed, Oct 2, 2013 at 2:23 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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.
Well, if we take a look at each function called inside
le_conn_complete_evt(), we'll find:
* hci_conn_hash_lookup_state which traverses hdev->conn_hash
(protected by RCU). So it doesn't require hdev->lock.
* mgmt_connect_failed which accesses hdev->id. hdev->id is written
only at hci_register_dev(). So it doesn't require hdev->lock.
* hci_proto_connect_cfm: it handles connection layer stuff, So it
doesn't require hdev->lock.
* mgmt_device_connected which access hdev->name. hdev->name is written
only at hci_register_dev(). So it doesn't require hdev->lock.
* hci_conn_add_sysfs which access only hdev->name. So it doesn't
require hdev->lock.
* hci_conn_del which removes a element from hdev->conn_hash. Since
hdev->conn_hash is protected by RCU, we have to guarantee updaters
mutual exclusion. So it requires hdev->lock.
* hci_conn_add which adds a new element to hdev->conn_hash. Since we
have to guarantee updaters mutual exclusion, it requires hdev->lock.
That being said, I'm not sure the same applies for all others HCI
handlers. We have to analyze each handler to make sure we can safely
move the locking.
Regards,
Andre
^ permalink raw reply
* Re: [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler
From: Andre Guedes @ 2013-10-03 14:06 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <E33B98BD-8E7D-4384-9893-29837A323066@holtmann.org>
Hi Marcel,
On Wed, Oct 2, 2013 at 2:21 AM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
>
> 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 =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> > - if (!conn) {
> > + if (ev->status) {
> > + conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CON=
NECT);
> > + 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 =3D BT_CLOSED;
> > + hci_conn_del(conn);
> > + goto unlock;
> > + }
> > +
> > + switch (ev->role) {
> > + case LE_CONN_ROLE_MASTER:
> > + conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdad=
dr);
> > + /* If there is no hci_conn object with the given address,=
it
> > + * means this new connection was triggered through HCI so=
cket
> > + * interface. For that case, we should create a new hci_c=
onn
> > + * object.
> > + */
>
> this comments belong one level down inside the if block. You already comm=
enting on the negative outcome of the if check.
Ok, I'll fix it.
>
>
> > + if (!conn) {
> > + conn =3D hci_conn_add(hdev, LE_LINK, &ev->bdaddr)=
;
> > + if (!conn) {
> > + BT_ERR("No memory for new connection");
> > + goto unlock;
> > + }
> > +
> > + conn->out =3D true;
> > + conn->link_mode |=3D HCI_LM_MASTER;
> > + conn->sec_level =3D BT_SECURITY_LOW;
> > + conn->dst_type =3D ev->bdaddr_type;
> > + }
> > + break;
> > +
> > + case LE_CONN_ROLE_SLAVE:
>
> And why are we not checking for an existing connection here? At least a s=
mall comment is needed to make that part clear.
Differently from master connection, there is no existing hci_conn for
slave connections. For that reason we don't check for an existing
connection here. I'll add a comment.
>
>
> > conn =3D 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 =3D ev->bdaddr_type;
> > + conn->sec_level =3D BT_SECURITY_LOW;
> > + break;
> >
> > - if (ev->role =3D=3D LE_CONN_ROLE_MASTER) {
> > - conn->out =3D true;
> > - conn->link_mode |=3D 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 =3D 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_d=
ev *hdev, struct sk_buff *skb)
> > mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
> > conn->dst_type, 0, NULL, 0, NULL);
> >
> > - conn->sec_level =3D BT_SECURITY_LOW;
> > conn->handle =3D __le16_to_cpu(ev->handle);
> > conn->state =3D BT_CONNECTED;
>
> All in all, I am not really understanding why this makes it this code sim=
pler. I actually think it turns it into more complicated code. So please ex=
plain what we are really gaining here. I just see more hash table lookup an=
d for hci_conn_add calls with more error checks.
When controller sends a LE Connection Complete event to host, we have
three different handling: failure, new master connection and new slave
connection. Additionally, new master connection has a special handling
since the connection could be triggered by HCI socket interface.
Before, all these logic were mixed up, making hard to add specific
code for new master connection for instance.
So this patch explicitly separates the three different handling and
adds extra comments aiming to improve hci_le_conn_complete_evt()
readability. Besides that, keeping these different handling separated,
it'll be easier to add the auto connection hooks.
Regards,
Andre
^ permalink raw reply
* Re: [PATCH 5/7] Bluetooth: Refactor hci_connect_le
From: Andre Guedes @ 2013-10-03 14:04 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: BlueZ development
In-Reply-To: <CAJdJm_NoWi2ujbMW8vXbwfZpS56+HNtEm8s5N__yXTJjADTPhQ@mail.gmail.com>
Hi Lizardo,
On Tue, Oct 1, 2013 at 9:05 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
>
> 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 [...]"
I'll fix it.
>
>
>
> > 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/
I'll fix it.
>
>
>
> > + 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.
Yes, you're right. I'll fix this.
>
>
> > 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;
> > }
Thanks for reviewing,
Andre
^ permalink raw reply
* Re: [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler
From: Andre Guedes @ 2013-10-03 14:04 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <3E999DA3-EE5E-41B3-8903-16C4A29FEB95@holtmann.org>
Hi Marcel,
On Wed, Oct 2, 2013 at 2:11 AM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
>
> 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 *hd=
ev, 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 =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CON=
NECT);
> > - if (!conn) {
> > - hci_dev_unlock(hdev);
> > - return;
> > - }
> > -
> > - BT_DBG("%s bdaddr %pMR conn %p", hdev->name, &conn->dst, =
conn);
> > -
> > - conn->state =3D 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 c=
omplete. 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.
I failed to see how this breaks bisection since the handling is
already done in initiate_le_connection_complete(). However, as
commented in patch 2/7, I'll squash this into patch 2/7 as you
suggested.
Regards,
Andre
^ permalink raw reply
* Re: [PATCH 3/7] Bluetooth: Remove hci_le_create_connection
From: Andre Guedes @ 2013-10-03 14:03 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <BC0DAED8-9D11-49E7-86C9-DBD513FB2047@holtmann.org>
Hi Marcel,
On Wed, Oct 2, 2013 at 2:08 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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.
Ok, I'll rename this function.
As said in previous patch, I'll squash this patch into 2/7.
Regards,
Andre
^ permalink raw reply
* Re: [PATCH 2/7] Bluetooth: Use HCI request for LE connection
From: Andre Guedes @ 2013-10-03 14:03 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <AB1C3086-F4EE-47E6-8C0F-8698A4F2C067@holtmann.org>
Hi Marcel,
On Wed, Oct 2, 2013 at 2:04 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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.
The hci_cs_le_create_conn() is removed in patch 4/7 since the handling
is done in initiate_le_connection_complete introduced by this patch.
I thought splitting this in three short patches would be easier to
review but it seems to be more confusing :) I'll squash patches 2, 3
and 4 into a bigger patch though.
Regards,
Andre
^ permalink raw reply
* Re: [PATCH 1/7] Bluetooth: Initialize hci_conn fields in hci_connect_le
From: Andre Guedes @ 2013-10-03 14:03 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <A1F8B46C-982C-4C37-8730-5993D28F324E@holtmann.org>
Hi Marcel,
On Wed, Oct 2, 2013 at 1:57 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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.
Yes, I don't really like le-> either. I'll replace le-> by conn-> in
this function in a first patch.
Andre
^ permalink raw reply
* Re: [PATCH 0/7] MAP notification API
From: Luiz Augusto von Dentz @ 2013-10-03 13:53 UTC (permalink / raw)
To: Christian Fetzer; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1380032167-6440-1-git-send-email-christian.fetzer@oss.bmw-carit.de>
Hi Christian,
On Tue, Sep 24, 2013 at 5:16 PM, Christian Fetzer
<christian.fetzer@oss.bmw-carit.de> wrote:
> From: Christian Fetzer <christian.fetzer@bmw-carit.de>
>
> This patchset adds the event handlers for new message, message shift/deleted
> and message status update events.
>
> New messages are signaled by registering a corresponding message event, that
> is announced using the ObjectManager.
>
> For moved / deleted messages:
> - If we have the Message interface, we simply update property 'Folder'.
> - If the message was moved into the current folder, we register its
> Message interface.
>
> The status update events change the property 'Status'.
>
> Note that this patchset depends on the changed map_msg_create function from
> 'obexd: Fix setting message folder' sent on 23.09.2013.
>
> Christian Fetzer (7):
> obexd: Handle new message event
> obexd: Add function set_reception_status to MAP client
> obexd: Handle message status events
> obexd: Update Status property in map documentation
> obexd: Add function set_folder to MAP client
> obexd: Handle message shift and message deleted events
> obexd: Prefix folders in event reports with leading slash
>
> doc/obex-api.txt | 10 ++++--
> obexd/client/map.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> obexd/client/mns.c | 16 +++++++--
> 3 files changed, 120 insertions(+), 5 deletions(-)
>
> --
> 1.8.3.4
After quite a few changes here and there I pushed this set, note some
things I just ignored for now such as message not know changing
location, if you provide a good explanation I can consider adding it
back but it should probably be done in a separate patch.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [RFC] Bluetooth: Only one command per L2CAP LE signalling is supported
From: Johan Hedberg @ 2013-10-03 13:21 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1380788797-8001-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Thu, Oct 03, 2013, Marcel Holtmann wrote:
> The Bluetooth specification makes it clear that only one command
> should be present in the L2CAP LE signalling packet. So tighten
> the checks here and restrict it to exactly one command.
>
> This is different from L2CAP BR/EDR signalling where multiple
> commands can be part of the same packet.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/l2cap_core.c | 44 +++++++++++++++++++-------------------------
> 1 file changed, 19 insertions(+), 25 deletions(-)
The patch and resulting code looks fine to me. I also did some basic
tests which were fine. The patch is now applied to bluetooth-next.
Thanks.
Johan
^ permalink raw reply
* Re: [RFCv2 09/14] android: sdp: Reuse BlueZ SDP server in Android
From: Szymon Janc @ 2013-10-03 11:47 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <20131003113512.GA2804@aemeltch-MOBL1>
Hi Andrei,
> > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > >
> > > Reuse existing SDP server code in Android GPL daemon.
> > > ---
> > > Makefile.android | 7 +++++--
> > > android/Android.mk | 7 +++++++
> > > android/bt_adapter.c | 5 ++++-
> > > android/main.c | 29 +++++++++++++++++++++++++++++
> > > android/main.h | 25 +++++++++++++++++++++++++
> > > 5 files changed, 70 insertions(+), 3 deletions(-)
> > > create mode 100644 android/main.h
> > >
> > > diff --git a/Makefile.android b/Makefile.android
> > > index 3e6fec0..bf82928 100644
> > > --- a/Makefile.android
> > > +++ b/Makefile.android
> > > @@ -3,7 +3,10 @@ if ANDROID_DAEMON
> > > noinst_PROGRAMS += android/bluezd
> > >
> > > android_bluezd_SOURCES = android/main.c src/log.c \
> > > + src/sdpd-database.c src/sdpd-server.c \
> > > + src/sdpd-service.c src/sdpd-request.c \
> > > src/shared/util.h src/shared/util.c \
> > > - src/shared/mgmt.h src/shared/mgmt.c
> > > -android_bluezd_LDADD = @GLIB_LIBS@
> > > + src/shared/mgmt.h src/shared/mgmt.c \
> > > + android/bt_adapter.h android/bt_adapter.c
> > > +android_bluezd_LDADD = lib/libbluetooth-internal.la @GLIB_LIBS@
> > > endif
> > > diff --git a/android/Android.mk b/android/Android.mk
> > > index 11ac204..b0a531f 100644
> > > --- a/android/Android.mk
> > > +++ b/android/Android.mk
> > > @@ -11,6 +11,11 @@ LOCAL_SRC_FILES := \
> > > main.c \
> > > ../src/shared/mgmt.c \
> > > ../src/shared/util.c \
> > > + bt_adapter.c \
> > > + ../src/sdpd-database.c \
> > > + ../src/sdpd-service.c \
> > > + ../src/sdpd-request.c \
> > > + ../src/sdpd-server.c \
> > >
> > > LOCAL_C_INCLUDES := \
> > > $(call include-path-for, glib) \
> > > @@ -19,6 +24,7 @@ LOCAL_C_INCLUDES := \
> > > LOCAL_C_INCLUDES += \
> > > $(LOCAL_PATH)/../ \
> > > $(LOCAL_PATH)/../src \
> > > + $(LOCAL_PATH)/../lib \
> > >
> > > LOCAL_CFLAGS := -DVERSION=\"$(BLUEZ_VERSION)\"
> > >
> > > @@ -30,6 +36,7 @@ LOCAL_CFLAGS += -DSOCK_CLOEXEC=02000000 -DSOCK_NONBLOCK=04000
> > >
> > > LOCAL_SHARED_LIBRARIES := \
> > > libglib \
> > > + libbluetooth \
> > >
> > > LOCAL_MODULE := bluezd
> > >
> > > diff --git a/android/bt_adapter.c b/android/bt_adapter.c
> > > index e21d50c..5016243 100644
> > > --- a/android/bt_adapter.c
> > > +++ b/android/bt_adapter.c
> > > @@ -23,6 +23,7 @@
> > >
> > > #include "bt_adapter.h"
> > > #include "log.h"
> > > +#include "main.h"
> > > #include "src/shared/mgmt.h"
> > >
> > > struct bt_adapter *bt_adapter_new(uint16_t index, struct mgmt *mgmt_if)
> > > @@ -45,7 +46,7 @@ void adapter_start(struct bt_adapter *adapter)
> > >
> > > /* TODO: CB: report scan mode */
> > >
> > > - /* TODO: SDP start here */
> > > + sdp_start();
> >
> > Why not just start it when daemon starts? Just like in original daemon?
> >
>
> Can it start without adapter initialized? How can I open L2CAP socket?
It binds to BDADDR_ANY address.
--
BR
Szymon Janc
^ permalink raw reply
* Re: [RFCv2 09/14] android: sdp: Reuse BlueZ SDP server in Android
From: Andrei Emeltchenko @ 2013-10-03 11:35 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth
In-Reply-To: <12556011.okBRc3D5Dz@uw000953>
Hi Szymon,
On Thu, Oct 03, 2013 at 04:23:02AM -0700, Szymon Janc wrote:
> Hi Andrei,
>
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Reuse existing SDP server code in Android GPL daemon.
> > ---
> > Makefile.android | 7 +++++--
> > android/Android.mk | 7 +++++++
> > android/bt_adapter.c | 5 ++++-
> > android/main.c | 29 +++++++++++++++++++++++++++++
> > android/main.h | 25 +++++++++++++++++++++++++
> > 5 files changed, 70 insertions(+), 3 deletions(-)
> > create mode 100644 android/main.h
> >
> > diff --git a/Makefile.android b/Makefile.android
> > index 3e6fec0..bf82928 100644
> > --- a/Makefile.android
> > +++ b/Makefile.android
> > @@ -3,7 +3,10 @@ if ANDROID_DAEMON
> > noinst_PROGRAMS += android/bluezd
> >
> > android_bluezd_SOURCES = android/main.c src/log.c \
> > + src/sdpd-database.c src/sdpd-server.c \
> > + src/sdpd-service.c src/sdpd-request.c \
> > src/shared/util.h src/shared/util.c \
> > - src/shared/mgmt.h src/shared/mgmt.c
> > -android_bluezd_LDADD = @GLIB_LIBS@
> > + src/shared/mgmt.h src/shared/mgmt.c \
> > + android/bt_adapter.h android/bt_adapter.c
> > +android_bluezd_LDADD = lib/libbluetooth-internal.la @GLIB_LIBS@
> > endif
> > diff --git a/android/Android.mk b/android/Android.mk
> > index 11ac204..b0a531f 100644
> > --- a/android/Android.mk
> > +++ b/android/Android.mk
> > @@ -11,6 +11,11 @@ LOCAL_SRC_FILES := \
> > main.c \
> > ../src/shared/mgmt.c \
> > ../src/shared/util.c \
> > + bt_adapter.c \
> > + ../src/sdpd-database.c \
> > + ../src/sdpd-service.c \
> > + ../src/sdpd-request.c \
> > + ../src/sdpd-server.c \
> >
> > LOCAL_C_INCLUDES := \
> > $(call include-path-for, glib) \
> > @@ -19,6 +24,7 @@ LOCAL_C_INCLUDES := \
> > LOCAL_C_INCLUDES += \
> > $(LOCAL_PATH)/../ \
> > $(LOCAL_PATH)/../src \
> > + $(LOCAL_PATH)/../lib \
> >
> > LOCAL_CFLAGS := -DVERSION=\"$(BLUEZ_VERSION)\"
> >
> > @@ -30,6 +36,7 @@ LOCAL_CFLAGS += -DSOCK_CLOEXEC=02000000 -DSOCK_NONBLOCK=04000
> >
> > LOCAL_SHARED_LIBRARIES := \
> > libglib \
> > + libbluetooth \
> >
> > LOCAL_MODULE := bluezd
> >
> > diff --git a/android/bt_adapter.c b/android/bt_adapter.c
> > index e21d50c..5016243 100644
> > --- a/android/bt_adapter.c
> > +++ b/android/bt_adapter.c
> > @@ -23,6 +23,7 @@
> >
> > #include "bt_adapter.h"
> > #include "log.h"
> > +#include "main.h"
> > #include "src/shared/mgmt.h"
> >
> > struct bt_adapter *bt_adapter_new(uint16_t index, struct mgmt *mgmt_if)
> > @@ -45,7 +46,7 @@ void adapter_start(struct bt_adapter *adapter)
> >
> > /* TODO: CB: report scan mode */
> >
> > - /* TODO: SDP start here */
> > + sdp_start();
>
> Why not just start it when daemon starts? Just like in original daemon?
>
Can it start without adapter initialized? How can I open L2CAP socket?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [RFCv2 09/14] android: sdp: Reuse BlueZ SDP server in Android
From: Szymon Janc @ 2013-10-03 11:23 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1380639799-25790-10-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Reuse existing SDP server code in Android GPL daemon.
> ---
> Makefile.android | 7 +++++--
> android/Android.mk | 7 +++++++
> android/bt_adapter.c | 5 ++++-
> android/main.c | 29 +++++++++++++++++++++++++++++
> android/main.h | 25 +++++++++++++++++++++++++
> 5 files changed, 70 insertions(+), 3 deletions(-)
> create mode 100644 android/main.h
>
> diff --git a/Makefile.android b/Makefile.android
> index 3e6fec0..bf82928 100644
> --- a/Makefile.android
> +++ b/Makefile.android
> @@ -3,7 +3,10 @@ if ANDROID_DAEMON
> noinst_PROGRAMS += android/bluezd
>
> android_bluezd_SOURCES = android/main.c src/log.c \
> + src/sdpd-database.c src/sdpd-server.c \
> + src/sdpd-service.c src/sdpd-request.c \
> src/shared/util.h src/shared/util.c \
> - src/shared/mgmt.h src/shared/mgmt.c
> -android_bluezd_LDADD = @GLIB_LIBS@
> + src/shared/mgmt.h src/shared/mgmt.c \
> + android/bt_adapter.h android/bt_adapter.c
> +android_bluezd_LDADD = lib/libbluetooth-internal.la @GLIB_LIBS@
> endif
> diff --git a/android/Android.mk b/android/Android.mk
> index 11ac204..b0a531f 100644
> --- a/android/Android.mk
> +++ b/android/Android.mk
> @@ -11,6 +11,11 @@ LOCAL_SRC_FILES := \
> main.c \
> ../src/shared/mgmt.c \
> ../src/shared/util.c \
> + bt_adapter.c \
> + ../src/sdpd-database.c \
> + ../src/sdpd-service.c \
> + ../src/sdpd-request.c \
> + ../src/sdpd-server.c \
>
> LOCAL_C_INCLUDES := \
> $(call include-path-for, glib) \
> @@ -19,6 +24,7 @@ LOCAL_C_INCLUDES := \
> LOCAL_C_INCLUDES += \
> $(LOCAL_PATH)/../ \
> $(LOCAL_PATH)/../src \
> + $(LOCAL_PATH)/../lib \
>
> LOCAL_CFLAGS := -DVERSION=\"$(BLUEZ_VERSION)\"
>
> @@ -30,6 +36,7 @@ LOCAL_CFLAGS += -DSOCK_CLOEXEC=02000000 -DSOCK_NONBLOCK=04000
>
> LOCAL_SHARED_LIBRARIES := \
> libglib \
> + libbluetooth \
>
> LOCAL_MODULE := bluezd
>
> diff --git a/android/bt_adapter.c b/android/bt_adapter.c
> index e21d50c..5016243 100644
> --- a/android/bt_adapter.c
> +++ b/android/bt_adapter.c
> @@ -23,6 +23,7 @@
>
> #include "bt_adapter.h"
> #include "log.h"
> +#include "main.h"
> #include "src/shared/mgmt.h"
>
> struct bt_adapter *bt_adapter_new(uint16_t index, struct mgmt *mgmt_if)
> @@ -45,7 +46,7 @@ void adapter_start(struct bt_adapter *adapter)
>
> /* TODO: CB: report scan mode */
>
> - /* TODO: SDP start here */
> + sdp_start();
Why not just start it when daemon starts? Just like in original daemon?
>
> /* TODO: CB: report state on */
> }
> @@ -53,4 +54,6 @@ void adapter_start(struct bt_adapter *adapter)
> void adapter_stop(struct bt_adapter *adapter)
> {
> DBG("disabled %u", adapter->dev_id);
> +
> + sdp_stop();
> }
> diff --git a/android/main.c b/android/main.c
> index 4792919..db435f9 100644
> --- a/android/main.c
> +++ b/android/main.c
> @@ -36,6 +36,8 @@
>
> #include "log.h"
> #include "hcid.h"
> +#include "sdpd.h"
> +#include "main.h"
>
> #include "lib/bluetooth.h"
> #include "lib/mgmt.h"
> @@ -43,12 +45,39 @@
>
> #define SHUTDOWN_GRACE_SECONDS 10
>
> +struct main_opts main_opts;
> +
> static GMainLoop *event_loop;
> static struct mgmt *mgmt_if = NULL;
>
> static uint8_t mgmt_version = 0;
> static uint8_t mgmt_revision = 0;
>
> +GList *adapter_list = NULL;
> +struct bt_adapter *default_adapter = NULL;
> +
> +int sdp_start(void)
> +{
> + DBG("");
> +
> + /* TODO: add logic */
> +
> + /* sdpd-server use these settings */
> + memset(&main_opts, 0, sizeof(main_opts));
> +
> + /* Use params: mtu = 0, flags = 0 */
> + return start_sdp_server(0, 0);
> +}
> +
> +void sdp_stop(void)
> +{
> + DBG("");
> +
> + /* TODO: add logic */
> +
> + stop_sdp_server();
> +}
> +
> void btd_exit(void)
> {
> g_main_loop_quit(event_loop);
> diff --git a/android/main.h b/android/main.h
> new file mode 100644
> index 0000000..6ecad14
> --- /dev/null
> +++ b/android/main.h
> @@ -0,0 +1,25 @@
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2013 Intel Corporation. All rights reserved.
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + */
> +
> +int sdp_start(void);
> +void sdp_stop(void);
>
--
BR
Szymon Janc
^ permalink raw reply
* [PATCH v2] hid2hci: fix regression in /dev format after moving away from libusb
From: Giovanni Campagna @ 2013-10-03 11:03 UTC (permalink / raw)
To: linux-bluetooth
From: Giovanni Campagna <gcampagna@src.gnome.org>
The paths under /dev, in the default udev configuration, are formatted
with two leading zeros, but the number obtained from sysfs don't have
them, so we must convert them to integers and reformat them.
---
tools/hid2hci.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/hid2hci.c b/tools/hid2hci.c
index bb8a521..95b4abf 100644
--- a/tools/hid2hci.c
+++ b/tools/hid2hci.c
@@ -221,18 +221,21 @@ static int usb_switch_dell(int fd, enum mode mode)
static int find_device(struct udev_device *udev_dev)
{
char path[PATH_MAX];
- const char *busnum, *devnum;
+ const char *busnum_str, *devnum_str;
+ int busnum, devnum;
int fd;
- busnum = udev_device_get_sysattr_value(udev_dev, "busnum");
- if (!busnum)
+ busnum_str = udev_device_get_sysattr_value(udev_dev, "busnum");
+ if (!busnum_str)
return -1;
+ busnum = strtol(busnum_str, NULL, 10);
- devnum = udev_device_get_sysattr_value(udev_dev, "devnum");
- if (!devnum)
+ devnum_str = udev_device_get_sysattr_value(udev_dev, "devnum");
+ if (!devnum_str)
return -1;
+ devnum = strtol(devnum_str, NULL, 10);
- snprintf(path, sizeof(path), "/dev/bus/usb/%s/%s", busnum, devnum);
+ snprintf(path, sizeof(path), "/dev/bus/usb/%03d/%03d", busnum, devnum);
fd = open(path, O_RDWR, O_CLOEXEC);
if (fd < 0) {
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Check minimum length of SMP packets
From: Johan Hedberg @ 2013-10-03 10:08 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1380788588-7897-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Thu, Oct 03, 2013, Marcel Holtmann wrote:
> When SMP packets are received, make sure they contain at least 1 byte
> header for the opcode. If not, drop the packet and disconnect the link.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/smp.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
Applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: Drop packets on ATT fixed channel on BR/EDR
From: Johan Hedberg @ 2013-10-03 10:08 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1380794051-8488-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Thu, Oct 03, 2013, Marcel Holtmann wrote:
> The ATT fixed channel is only valid when using LE connections. On
> BR/EDR it is required to go through L2CAP connection oriented
> channel for ATT.
>
> Drop ATT packets when they are received on a BR/EDR connection.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/l2cap_core.c | 4 ++++
> 1 file changed, 4 insertions(+)
Applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply
* [PATCH] Bluetooth: Drop packets on ATT fixed channel on BR/EDR
From: Marcel Holtmann @ 2013-10-03 9:54 UTC (permalink / raw)
To: linux-bluetooth
The ATT fixed channel is only valid when using LE connections. On
BR/EDR it is required to go through L2CAP connection oriented
channel for ATT.
Drop ATT packets when they are received on a BR/EDR connection.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 102a510..583517e1 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -6431,8 +6431,12 @@ drop:
static void l2cap_att_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
+ struct hci_conn *hcon = conn->hcon;
struct l2cap_chan *chan;
+ if (hcon->type != LE_LINK)
+ goto drop;
+
chan = l2cap_global_chan_by_scid(BT_CONNECTED, L2CAP_CID_ATT,
conn->src, conn->dst);
if (!chan)
--
1.8.3.1
^ permalink raw reply related
* [RFC] Bluetooth: Only one command per L2CAP LE signalling is supported
From: Marcel Holtmann @ 2013-10-03 8:26 UTC (permalink / raw)
To: linux-bluetooth
The Bluetooth specification makes it clear that only one command
should be present in the L2CAP LE signalling packet. So tighten
the checks here and restrict it to exactly one command.
This is different from L2CAP BR/EDR signalling where multiple
commands can be part of the same packet.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 44 +++++++++++++++++++-------------------------
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 102a510..a2c223e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5322,43 +5322,37 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
struct hci_conn *hcon = conn->hcon;
- u8 *data = skb->data;
- int len = skb->len;
- struct l2cap_cmd_hdr cmd;
+ struct l2cap_cmd_hdr *cmd;
+ u16 len;
int err;
if (hcon->type != LE_LINK)
goto drop;
- while (len >= L2CAP_CMD_HDR_SIZE) {
- u16 cmd_len;
- memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
- data += L2CAP_CMD_HDR_SIZE;
- len -= L2CAP_CMD_HDR_SIZE;
+ if (skb->len < L2CAP_CMD_HDR_SIZE)
+ goto drop;
- cmd_len = le16_to_cpu(cmd.len);
+ cmd = (void *) skb->data;
+ skb_pull(skb, L2CAP_CMD_HDR_SIZE);
- BT_DBG("code 0x%2.2x len %d id 0x%2.2x", cmd.code, cmd_len,
- cmd.ident);
+ len = le16_to_cpu(cmd->len);
- if (cmd_len > len || !cmd.ident) {
- BT_DBG("corrupted command");
- break;
- }
+ BT_DBG("code 0x%2.2x len %d id 0x%2.2x", cmd->code, len, cmd->ident);
- err = l2cap_le_sig_cmd(conn, &cmd, data);
- if (err) {
- struct l2cap_cmd_rej_unk rej;
+ if (len != skb->len || !cmd->ident) {
+ BT_DBG("corrupted command");
+ goto drop;
+ }
- BT_ERR("Wrong link type (%d)", err);
+ err = l2cap_le_sig_cmd(conn, cmd, skb->data);
+ if (err) {
+ struct l2cap_cmd_rej_unk rej;
- rej.reason = l2cap_err_to_reason(err);
- l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
- sizeof(rej), &rej);
- }
+ BT_ERR("Wrong link type (%d)", err);
- data += cmd_len;
- len -= cmd_len;
+ rej.reason = l2cap_err_to_reason(err);
+ l2cap_send_cmd(conn, cmd->ident, L2CAP_COMMAND_REJ,
+ sizeof(rej), &rej);
}
drop:
--
1.8.3.1
^ permalink raw reply related
* [PATCH] Bluetooth: Check minimum length of SMP packets
From: Marcel Holtmann @ 2013-10-03 8:23 UTC (permalink / raw)
To: linux-bluetooth
When SMP packets are received, make sure they contain at least 1 byte
header for the opcode. If not, drop the packet and disconnect the link.
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/smp.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 6e049497..884b208 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -848,8 +848,7 @@ static int smp_cmd_master_ident(struct l2cap_conn *conn, struct sk_buff *skb)
int smp_sig_channel(struct l2cap_conn *conn, struct sk_buff *skb)
{
struct hci_conn *hcon = conn->hcon;
- __u8 code = skb->data[0];
- __u8 reason;
+ __u8 code, reason;
int err = 0;
if (hcon->type != LE_LINK) {
@@ -857,12 +856,18 @@ int smp_sig_channel(struct l2cap_conn *conn, struct sk_buff *skb)
return -ENOTSUPP;
}
+ if (skb->len < 1) {
+ kfree_skb(skb);
+ return -EILSEQ;
+ }
+
if (!test_bit(HCI_LE_ENABLED, &conn->hcon->hdev->dev_flags)) {
err = -ENOTSUPP;
reason = SMP_PAIRING_NOTSUPP;
goto done;
}
+ code = skb->data[0];
skb_pull(skb, sizeof(code));
/*
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] Bluetooth: L2CAP connectionless channels are only valid for BR/EDR
From: Johan Hedberg @ 2013-10-03 7:14 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1380783819-47726-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Thu, Oct 03, 2013, Marcel Holtmann wrote:
> When receiving connectionless packets on a LE connection, just drop
> the packet. There is no concept of connectionless channels for LE.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/l2cap_core.c | 4 ++++
> 1 file changed, 4 insertions(+)
Applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: SMP packets are only valid on LE connections
From: Johan Hedberg @ 2013-10-03 7:10 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1380783657-45553-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Thu, Oct 03, 2013, Marcel Holtmann wrote:
> When receiving SMP packets on a BR/EDR connection, then just drop
> the packet and do not try to process it.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/smp.c | 6 ++++++
> 1 file changed, 6 insertions(+)
Applied to bluetooth-next. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: Don't copy L2CAP LE signalling to raw sockets
From: Johan Hedberg @ 2013-10-03 7:10 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1380783109-37739-1-git-send-email-marcel@holtmann.org>
Hi Marcel,
On Wed, Oct 02, 2013, Marcel Holtmann wrote:
> The L2CAP raw sockets are only used for BR/EDR signalling. Packets
> on LE links should not be forwarded there.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> net/bluetooth/l2cap_core.c | 2 --
> 1 file changed, 2 deletions(-)
Applied to bluetooth-next. Thanks.
Johan
^ 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