* Re: [PATCH 04/10] android/ipc-tester: Add missing service opcode boundries test cases
From: Szymon Janc @ 2014-10-13 20:32 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1412858714-2845-5-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Thursday 09 October 2014 14:45:08 Grzegorz Kolodziejczyk wrote:
> This patch adds tests sending out of range opcode for each service.
> ---
> android/ipc-tester.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/android/ipc-tester.c b/android/ipc-tester.c
> index ea71c8d..161777d 100644
> --- a/android/ipc-tester.c
> +++ b/android/ipc-tester.c
> @@ -919,9 +919,23 @@ int main(int argc, char *argv[])
> test_opcode_valid("PAN", HAL_SERVICE_ID_PAN, 0x05, 0,
> HAL_SERVICE_ID_BLUETOOTH, HAL_SERVICE_ID_PAN);
>
> + test_opcode_valid("HANDSFREE", HAL_SERVICE_ID_HANDSFREE, 0x0f, 0,
> + HAL_SERVICE_ID_BLUETOOTH,
> + HAL_SERVICE_ID_HANDSFREE);
> +
> test_opcode_valid("A2DP", HAL_SERVICE_ID_A2DP, 0x03, 0,
> HAL_SERVICE_ID_BLUETOOTH, HAL_SERVICE_ID_A2DP);
>
> + test_opcode_valid("HEALTH", HAL_SERVICE_ID_HEALTH, 0x06, 0,
> + HAL_SERVICE_ID_BLUETOOTH,
> + HAL_SERVICE_ID_HEALTH);
> +
> + test_opcode_valid("AVRCP", HAL_SERVICE_ID_AVRCP, 0x0b, 0,
> + HAL_SERVICE_ID_BLUETOOTH, HAL_SERVICE_ID_AVRCP);
> +
> + test_opcode_valid("GATT", HAL_SERVICE_ID_GATT, 0x24, 0,
> + HAL_SERVICE_ID_BLUETOOTH, HAL_SERVICE_ID_GATT);
> +
> test_opcode_valid("HF_CLIENT", HAL_SERVICE_ID_HANDSFREE_CLIENT, 0x10, 0,
> HAL_SERVICE_ID_BLUETOOTH,
> HAL_SERVICE_ID_HANDSFREE_CLIENT);
In future please don't send unrelated patches as part of serie. This makes
review easier and also makes no unnecessary delays in getting such patches
merged.
Applied. Thanks.
--
Szymon K. Janc
szymon.janc@gmail.com
^ permalink raw reply
* Re: [PATCH 03/10] android/map-client: Add support for get remote MAS instances
From: Szymon Janc @ 2014-10-13 20:19 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1412858714-2845-4-git-send-email-grzegorz.kolodziejczyk@tieto.com>
On Thursday 09 October 2014 14:45:07 Grzegorz Kolodziejczyk wrote:
> This allows to get remote mas instances. In case of wrong sdp record
> fail status will be returned in notification.
> ---
> android/map-client.c | 124
> ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 123
> insertions(+), 1 deletion(-)
>
> diff --git a/android/map-client.c b/android/map-client.c
> index 1001b36..adeae4b 100644
> --- a/android/map-client.c
> +++ b/android/map-client.c
> @@ -30,21 +30,143 @@
> #include <stdint.h>
> #include <glib.h>
>
> +#include "lib/sdp.h"
> +#include "lib/sdp_lib.h"
> +#include "src/sdp-client.h"
> +
> #include "ipc.h"
> #include "lib/bluetooth.h"
> #include "map-client.h"
> #include "src/log.h"
> #include "hal-msg.h"
> +#include "ipc-common.h"
> +#include "utils.h"
> +#include "src/shared/util.h"
>
> static struct ipc *hal_ipc = NULL;
> static bdaddr_t adapter_addr;
>
> +static int fill_mce_inst(void *buf, int32_t id, int32_t scn, int32_t
> msg_type, + const void *name, uint8_t name_len)
> +{
> + struct hal_map_client_mas_instance *inst = buf;
> +
> + inst->id = id;
> + inst->scn = scn;
> + inst->msg_types = msg_type;
> + inst->name_len = name_len;
> +
> + if (name_len)
> + memcpy(inst->name, name, name_len);
> +
> + return sizeof(*inst) + name_len;
> +}
> +
> +static void map_client_sdp_search_cb(sdp_list_t *recs, int err, gpointer
> data) +{
> + uint8_t buf[IPC_MTU];
> + struct hal_ev_map_client_remote_mas_instances *ev = (void *) buf;
> + bdaddr_t *dst = data;
> + sdp_list_t *list, *protos;
> + uint8_t status;
> + int32_t id, scn, msg_type, name_len, num_instances = 0;
> + char *name;
> + size_t size;
> +
> + size = sizeof(*ev);
> + bdaddr2android(dst, &ev->bdaddr);
> +
> + if (err < 0) {
> + error("mce: Unable to get SDP record: %s", strerror(-err));
> + status = HAL_STATUS_FAILED;
> + goto fail;
> + }
> +
> + for (list = recs; list != NULL; list = list->next) {
> + sdp_record_t *rec = list->data;
> + sdp_data_t *data;
> +
> + data = sdp_data_get(rec, SDP_ATTR_MAS_INSTANCE_ID);
> + if (data) {
> + id = data->val.uint8;
> + } else {
> + error("mce: cannot get mas instance id");
> + status = HAL_STATUS_FAILED;
> + goto fail;
I'm not sure if we should fail here. Lets just skip record (we can leave error
message) and continue with search.
Also make it like: if (!data) {error(); continue;};
Not need for if-else
> + }
> +
> + data = sdp_data_get(rec, SDP_ATTR_SVCNAME_PRIMARY);
> + if (data) {
> + name = data->val.str;
> + name_len = data->unitSize;
> + } else {
> + error("mce: cannot get mas instance name");
> + status = HAL_STATUS_FAILED;
> + goto fail;
> + }
> +
> + data = sdp_data_get(rec, SDP_ATTR_SUPPORTED_MESSAGE_TYPES);
> + if (data) {
> + msg_type = data->val.uint8;
> + } else {
> + error("mce: cannot get mas instance msg type");
> + status = HAL_STATUS_FAILED;
> + goto fail;
> + }
> +
> + if (!sdp_get_access_protos(rec, &protos)) {
> + scn = sdp_get_proto_port(protos, RFCOMM_UUID);
> +
> + sdp_list_foreach(protos,
> + (sdp_list_func_t) sdp_list_free, NULL);
> + sdp_list_free(protos, NULL);
> + } else {
> + error("mce: cannot get mas instance rfcomm channel");
> + status = HAL_STATUS_FAILED;
> + goto fail;
> + }
> +
> + size += fill_mce_inst(buf + size, id, scn, msg_type, name,
> + name_len);
> + num_instances++;
> + }
> +
> + status = HAL_STATUS_SUCCESS;
Please check if we are expected to return error if no instances were found.
> +
> +fail:
> + ev->num_instances = num_instances;
> + ev->status = status;
> +
> + ipc_send_notif(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT,
> + HAL_EV_MAP_CLIENT_REMOTE_MAS_INSTANCES, size, buf);
> +
> + free(dst);
> +}
> +
> static void handle_get_instances(const void *buf, uint16_t len)
> {
> + const struct hal_cmd_map_client_get_instances *cmd = buf;
> + uint8_t status;
> + bdaddr_t *dst;
> + uuid_t uuid;
> +
> DBG("");
>
> + dst = new0(bdaddr_t, 1);
Please check if allocation failed.
> + android2bdaddr(&cmd->bdaddr, dst);
> +
> + sdp_uuid16_create(&uuid, MAP_MSE_SVCLASS_ID);
> +
> + if (bt_search_service(&adapter_addr, dst, &uuid,
> + map_client_sdp_search_cb, dst, NULL, 0)) {
Just a hint: you can pass free as destroy function here instead of taking care
of that in callback.
> + error("mce: Failed to search SDP details");
> + status = HAL_STATUS_FAILED;
> + free(dst);
> + }
> +
> + status = HAL_STATUS_SUCCESS;
In case of error status is overwritten.
> ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT,
> - HAL_OP_MAP_CLIENT_GET_INSTANCES, HAL_STATUS_FAILED);
> + HAL_OP_MAP_CLIENT_GET_INSTANCES, status);
> }
>
> static const struct ipc_handler cmd_handlers[] = {
--
Szymon K. Janc
szymon.janc@gmail.com
^ permalink raw reply
* Re: [PATCH 02/10] android/map-client: Add stubs for MAP client commands handlers
From: Szymon Janc @ 2014-10-13 19:58 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1412858714-2845-3-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Thursday 09 October 2014 14:45:06 Grzegorz Kolodziejczyk wrote:
> Add empty handlers for MAP client IPC commands.
> ---
> android/map-client.c | 33 ++++++++++++++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/android/map-client.c b/android/map-client.c
> index 4556461..1001b36 100644
> --- a/android/map-client.c
> +++ b/android/map-client.c
> @@ -28,17 +28,48 @@
> #include <stdbool.h>
> #include <stdlib.h>
> #include <stdint.h>
> +#include <glib.h>
>
> #include "ipc.h"
> #include "lib/bluetooth.h"
> #include "map-client.h"
> +#include "src/log.h"
> +#include "hal-msg.h"
> +
> +static struct ipc *hal_ipc = NULL;
> +static bdaddr_t adapter_addr;
> +
> +static void handle_get_instances(const void *buf, uint16_t len)
> +{
> + DBG("");
> +
> + ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT,
> + HAL_OP_MAP_CLIENT_GET_INSTANCES, HAL_STATUS_FAILED);
> +}
> +
> +static const struct ipc_handler cmd_handlers[] = {
> + {handle_get_instances, false,
> + sizeof(struct hal_cmd_map_client_get_instances)},
Style issue: there should be spaces after { and before }.
Also please add comment about define type just like in other handlers (I'm
aware that there is just 1 handler here but lets be consistent).
> +};
>
> bool bt_map_client_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t
> mode) {
> - return false;
> + DBG("");
> +
> + bacpy(&adapter_addr, addr);
> +
> + hal_ipc = ipc;
> +
> + ipc_register(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT, cmd_handlers,
> + G_N_ELEMENTS(cmd_handlers));
> +
> + return true;
> }
>
> void bt_map_client_unregister(void)
> {
> + DBG("");
>
> + ipc_unregister(hal_ipc, HAL_SERVICE_ID_MAP_CLIENT);
> + hal_ipc = NULL;
> }
--
Szymon K. Janc
szymon.janc@gmail.com
^ permalink raw reply
* Re: [PATCH 1/2] android/pts: Update HSP files for PTS 5.3
From: Szymon Janc @ 2014-10-13 18:55 UTC (permalink / raw)
To: Sebastian Chlad; +Cc: linux-bluetooth
In-Reply-To: <1412943017-17823-1-git-send-email-sebastian.chlad@tieto.com>
Hi Sebastian,
On Friday 10 October 2014 14:10:16 Sebastian Chlad wrote:
> ---
> android/pics-hsp.txt | 2 +-
> android/pixit-hsp.txt | 2 +-
> android/pts-hsp.txt | 4 ++--
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/android/pics-hsp.txt b/android/pics-hsp.txt
> index 330a197..7e71b14 100644
> --- a/android/pics-hsp.txt
> +++ b/android/pics-hsp.txt
> @@ -1,6 +1,6 @@
> HSP PICS for the PTS tool.
>
> -PTS version: 5.2
> +PTS version: 5.3
>
> * - different than PTS defaults
> # - not yet implemented/supported
> diff --git a/android/pixit-hsp.txt b/android/pixit-hsp.txt
> index a097277..6be4731 100644
> --- a/android/pixit-hsp.txt
> +++ b/android/pixit-hsp.txt
> @@ -1,6 +1,6 @@
> HSP PIXIT for the PTS tool.
>
> -PTS version: 5.2
> +PTS version: 5.3
>
> * - different than PTS defaults
> & - should be set to IUT Bluetooth address
> diff --git a/android/pts-hsp.txt b/android/pts-hsp.txt
> index 0df9806..9031b82 100644
> --- a/android/pts-hsp.txt
> +++ b/android/pts-hsp.txt
> @@ -1,7 +1,7 @@
> PTS test results for HSP
>
> -PTS version: 5.2
> -Tested: 15-July-2014
> +PTS version: 5.3
> +Tested: 10-October-2014
> Android version: 4.4.4
>
> Results:
Both patches applied. Thanks.
--
Szymon K. Janc
szymon.janc@gmail.com
^ permalink raw reply
* [PATCH bluetooth] Bluetooth: Fix missing channel unlock in l2cap_le_credits
From: Martin Townsend @ 2014-10-13 18:24 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, jukka.rissanen, johan.hedberg, Martin Townsend
In the error case where credits is greater than max_credits there
is a missing l2cap_chan_unlock before returning.
Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
---
net/bluetooth/l2cap_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 46547b9..bfb6af8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5544,6 +5544,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
if (credits > max_credits) {
BT_ERR("LE credits overflow");
l2cap_send_disconn_req(chan, ECONNRESET);
+ l2cap_chan_unlock(chan);
/* Return 0 so that we don't trigger an unnecessary
* command reject packet.
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Alexander Aring @ 2014-10-13 17:22 UTC (permalink / raw)
To: Jukka Rissanen
Cc: Martin Townsend, Martin Townsend, linux-bluetooth, linux-wpan,
marcel
In-Reply-To: <20141013171111.GA25249@omega>
Hi Jukka,
sorry.
I was a little too fast here, because I am sure now this should solve your
lockdep issue.
On Mon, Oct 13, 2014 at 07:11:11PM +0200, Alexander Aring wrote:
> Hi Jukka,
>
> On Mon, Oct 13, 2014 at 06:09:14PM +0300, Jukka Rissanen wrote:
> > Hi Martin,
> >
> > On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> > > Hi Jukka,
> > >
> > > Does this patch help?
> >
> > Unfortunately no, I still see inconsistent lock state. It would probably
> > have been too easy :)
> >
>
> I remeber something, I think 802.15.4 had similar issues long time ago.
>
s/remeber/remember/
> The fix was 20e7c4e80dcd01dad5e6c8b32455228b8fe9c619 ("6lowpan: fix
> lockdep splats"). Please check that, you need something like that!
>
Something like that:
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 4ebc806..02fd21a 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -642,7 +642,27 @@ static netdev_tx_t bt_xmit(struct sk_buff *skb, struct net_device *netdev)
return err < 0 ? NET_XMIT_DROP : err;
}
+static struct lock_class_key bt_tx_busylock;
+static struct lock_class_key bt_netdev_xmit_lock_key;
+
+static void bt_set_lockdep_class_one(struct net_device *dev,
+ struct netdev_queue *txq,
+ void *_unused)
+{
+ lockdep_set_class(&txq->_xmit_lock,
+ &bt_netdev_xmit_lock_key);
+}
+
+
+static int bt_dev_init(struct net_device *dev)
+{
+ netdev_for_each_tx_queue(dev, bt_set_lockdep_class_one, NULL);
+ dev->qdisc_tx_busylock = &bt_tx_busylock;
+ return 0;
+}
+
static const struct net_device_ops netdev_ops = {
+ .ndo_init = bt_dev_init,
.ndo_start_xmit = bt_xmit,
};
- Alex
[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=20e7c4e80dcd01dad5e6c8b32455228b8fe9c619
^ permalink raw reply related
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Alexander Aring @ 2014-10-13 17:11 UTC (permalink / raw)
To: Jukka Rissanen
Cc: Martin Townsend, Martin Townsend, linux-bluetooth, linux-wpan,
marcel
In-Reply-To: <1413212954.2705.106.camel@jrissane-mobl.ger.corp.intel.com>
Hi Jukka,
On Mon, Oct 13, 2014 at 06:09:14PM +0300, Jukka Rissanen wrote:
> Hi Martin,
>
> On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> > Hi Jukka,
> >
> > Does this patch help?
>
> Unfortunately no, I still see inconsistent lock state. It would probably
> have been too easy :)
>
I remeber something, I think 802.15.4 had similar issues long time ago.
The fix was 20e7c4e80dcd01dad5e6c8b32455228b8fe9c619 ("6lowpan: fix
lockdep splats"). Please check that, you need something like that!
- Alex
[0]
^ permalink raw reply
* Re: [PATCH v3 1/2] core: Add Manufacturer Specific Data EIR field
From: Alfonso Acosta @ 2014-10-13 17:02 UTC (permalink / raw)
To: Alfonso Acosta, BlueZ development
In-Reply-To: <20141013161319.GA27533@t440s.P-661HNU-F1>
Hi Johan and Marcel,
>> + case EIR_MANUFACTURER_DATA:
>> + if (data_len < 2 || data_len > 2 + sizeof(eir->msd->data))
>> + break;
>> + eir->msd = g_malloc(sizeof(*eir->msd));
>> + eir->msd->company = get_le16(data);
>> + eir->msd->data_len = data_len - 2;
>> + memcpy(&eir->msd->data, data + 2, eir->msd->data_len);
>> + break;
>
> Wouldn't this lead to a memory leaks if a device (violating the spec. but
> still) had two or more manufacturer data entries in it's AD/EIR data?
> Taking example from how remote name entries are handled you should
> probably g_free(eir->msd) before allocating a new one.
Very good point. So, should I support multiple MSD fields or just the
first (last) one for now?
In all honesty, just like Johan, I didn't even know it was legal to
provide multiple ones.
Thanks,
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v3 1/2] core: Add Manufacturer Specific Data EIR field
From: Johan Hedberg @ 2014-10-13 16:30 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Alfonso Acosta, linux-bluetooth
In-Reply-To: <D4814A57-3BAC-43BC-8C39-447FADE29B79@holtmann.org>
Hi Marcel,
On Mon, Oct 13, 2014, Marcel Holtmann wrote:
> Hi Johan,
>
> >> + case EIR_MANUFACTURER_DATA:
> >> + if (data_len < 2 || data_len > 2 + sizeof(eir->msd->data))
> >> + break;
> >> + eir->msd = g_malloc(sizeof(*eir->msd));
> >> + eir->msd->company = get_le16(data);
> >> + eir->msd->data_len = data_len - 2;
> >> + memcpy(&eir->msd->data, data + 2, eir->msd->data_len);
> >> + break;
> >
> > Wouldn't this lead to a memory leaks if a device (violating the spec. but
> > still) had two or more manufacturer data entries in it's AD/EIR data?
> > Taking example from how remote name entries are handled you should
> > probably g_free(eir->msd) before allocating a new one.
>
> have multiple manufacturer data entries is not violating the
> specification. That is actually valid.
Right you are. I was somehow assuming this would be similar to e.g. the
UUID fields. Anyway, if we want to represent in the new internal API all
that is allowed in the spec it'd need some redesign. OTOH, I'm not sure
it's worth it until we have an example of an actual product that has
multiple entries).
Johan
^ permalink raw reply
* Re: [PATCH v3 1/2] core: Add Manufacturer Specific Data EIR field
From: Marcel Holtmann @ 2014-10-13 16:18 UTC (permalink / raw)
To: Johan Hedberg; +Cc: Alfonso Acosta, linux-bluetooth
In-Reply-To: <20141013161319.GA27533@t440s.P-661HNU-F1>
Hi Johan,
>> + case EIR_MANUFACTURER_DATA:
>> + if (data_len < 2 || data_len > 2 + sizeof(eir->msd->data))
>> + break;
>> + eir->msd = g_malloc(sizeof(*eir->msd));
>> + eir->msd->company = get_le16(data);
>> + eir->msd->data_len = data_len - 2;
>> + memcpy(&eir->msd->data, data + 2, eir->msd->data_len);
>> + break;
>
> Wouldn't this lead to a memory leaks if a device (violating the spec. but
> still) had two or more manufacturer data entries in it's AD/EIR data?
> Taking example from how remote name entries are handled you should
> probably g_free(eir->msd) before allocating a new one.
have multiple manufacturer data entries is not violating the specification. That is actually valid.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3 1/2] core: Add Manufacturer Specific Data EIR field
From: Johan Hedberg @ 2014-10-13 16:13 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1413200623-31278-3-git-send-email-fons@spotify.com>
Hi Alfonso,
On Mon, Oct 13, 2014, Alfonso Acosta wrote:
> + case EIR_MANUFACTURER_DATA:
> + if (data_len < 2 || data_len > 2 + sizeof(eir->msd->data))
> + break;
> + eir->msd = g_malloc(sizeof(*eir->msd));
> + eir->msd->company = get_le16(data);
> + eir->msd->data_len = data_len - 2;
> + memcpy(&eir->msd->data, data + 2, eir->msd->data_len);
> + break;
Wouldn't this lead to a memory leaks if a device (violating the spec. but
still) had two or more manufacturer data entries in it's AD/EIR data?
Taking example from how remote name entries are handled you should
probably g_free(eir->msd) before allocating a new one.
Johan
^ permalink raw reply
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Johan Hedberg @ 2014-10-13 16:07 UTC (permalink / raw)
To: Jukka Rissanen
Cc: Martin Townsend, Martin Townsend, linux-bluetooth, linux-wpan,
marcel, alex.aring
In-Reply-To: <1413212954.2705.106.camel@jrissane-mobl.ger.corp.intel.com>
Hi,
On Mon, Oct 13, 2014, Jukka Rissanen wrote:
> On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> > Hi Jukka,
> >
> > Does this patch help?
>
> Unfortunately no, I still see inconsistent lock state. It would probably
> have been too easy :)
> >
> > ---
> > net/bluetooth/l2cap_core.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index b6f9777..fb7b2ff 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
> > if (credits > max_credits) {
> > BT_ERR("LE credits overflow");
> > l2cap_send_disconn_req(chan, ECONNRESET);
> > + l2cap_chan_unlock(chan);
> >
> > /* Return 0 so that we don't trigger an unnecessary
> > * command reject packet.
I'd appreciate if you could still send a proper patch for this since
it's clearly a locking bug (something even worth sending to stable
trees).
Johan
^ permalink raw reply
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Martin Townsend @ 2014-10-13 16:00 UTC (permalink / raw)
To: Jukka Rissanen
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel, alex.aring
In-Reply-To: <1413212954.2705.106.camel@jrissane-mobl.ger.corp.intel.com>
Hi Jukka,
>From the last trace it looks like a transmit has been started from the receive worker thread. I notice that both tx and rx use the same workerqueue structure, e.g.
hci_send_xxx
...
queue_work(hdev->workqueue, &hdev->tx_work);
hci_recv_frame
...
queue_work(hdev->workqueue, &hdev->rx_work);
Would this cause problems. I don't know enough about work queues but in workqueue_struct there is a mutex which may be held by the rx worker and if this rx worker start a transmit maybe this would cause the lock? Could test by creating separate queues for rx and tx?
Anyway just a thought.
- Martin.
On 13/10/14 16:09, Jukka Rissanen wrote:
> Hi Martin,
>
> On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
>> Hi Jukka,
>>
>> Does this patch help?
> Unfortunately no, I still see inconsistent lock state. It would probably
> have been too easy :)
>
>> ---
>> net/bluetooth/l2cap_core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index b6f9777..fb7b2ff 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
>> if (credits > max_credits) {
>> BT_ERR("LE credits overflow");
>> l2cap_send_disconn_req(chan, ECONNRESET);
>> + l2cap_chan_unlock(chan);
>>
>> /* Return 0 so that we don't trigger an unnecessary
>> * command reject packet.
>
> Cheers,
> Jukka
>
>
^ permalink raw reply
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Martin Townsend @ 2014-10-13 15:47 UTC (permalink / raw)
To: Jukka Rissanen
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel, alex.aring
In-Reply-To: <1413212954.2705.106.camel@jrissane-mobl.ger.corp.intel.com>
[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]
Hi Jukka,
>From the last trace it looks like a transmit has been started from the receive worker thread. I notice that both tx and rx use the same workerqueue structure, e.g.
hci_send_xxx <http://lxr.free-electrons.com/ident?i=hci_send_acl>
...
queue_work <http://lxr.free-electrons.com/ident?i=queue_work>(hdev <http://lxr.free-electrons.com/ident?i=hdev>->workqueue <http://lxr.free-electrons.com/ident?i=workqueue>, &hdev <http://lxr.free-electrons.com/ident?i=hdev>->tx_work);
hci_recv_frame <http://lxr.free-electrons.com/ident?i=hci_recv_frame>
...
queue_work <http://lxr.free-electrons.com/ident?i=queue_work>(hdev <http://lxr.free-electrons.com/ident?i=hdev>->workqueue <http://lxr.free-electrons.com/ident?i=workqueue>, &hdev <http://lxr.free-electrons.com/ident?i=hdev>->rx_work <http://lxr.free-electrons.com/ident?i=rx_work>);
Would this cause problems. I don't know enough about work queues but in workqueue_struct there is a mutex which may be held by the rx worker and if this rx worker start a transmit maybe this would cause the lock? Could test by creating separate queues for rx and tx?
Anyway just a thought.
- Martin.
On 13/10/14 16:09, Jukka Rissanen wrote:
> Hi Martin,
>
> On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
>> Hi Jukka,
>>
>> Does this patch help?
> Unfortunately no, I still see inconsistent lock state. It would probably
> have been too easy :)
>
>> ---
>> net/bluetooth/l2cap_core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index b6f9777..fb7b2ff 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
>> if (credits > max_credits) {
>> BT_ERR("LE credits overflow");
>> l2cap_send_disconn_req(chan, ECONNRESET);
>> + l2cap_chan_unlock(chan);
>>
>> /* Return 0 so that we don't trigger an unnecessary
>> * command reject packet.
>
> Cheers,
> Jukka
>
>
[-- Attachment #2: Type: text/html, Size: 2931 bytes --]
^ permalink raw reply
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Jukka Rissanen @ 2014-10-13 15:09 UTC (permalink / raw)
To: Martin Townsend
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel, alex.aring
In-Reply-To: <543BE816.80304@xsilon.com>
Hi Martin,
On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> Hi Jukka,
>
> Does this patch help?
Unfortunately no, I still see inconsistent lock state. It would probably
have been too easy :)
>
> ---
> net/bluetooth/l2cap_core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index b6f9777..fb7b2ff 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
> if (credits > max_credits) {
> BT_ERR("LE credits overflow");
> l2cap_send_disconn_req(chan, ECONNRESET);
> + l2cap_chan_unlock(chan);
>
> /* Return 0 so that we don't trigger an unnecessary
> * command reject packet.
Cheers,
Jukka
^ permalink raw reply
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Martin Townsend @ 2014-10-13 14:56 UTC (permalink / raw)
To: Jukka Rissanen
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel, alex.aring
In-Reply-To: <1413211468.2705.104.camel@jrissane-mobl.ger.corp.intel.com>
Hi Jukka,
Does this patch help?
---
net/bluetooth/l2cap_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b6f9777..fb7b2ff 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
if (credits > max_credits) {
BT_ERR("LE credits overflow");
l2cap_send_disconn_req(chan, ECONNRESET);
+ l2cap_chan_unlock(chan);
/* Return 0 so that we don't trigger an unnecessary
* command reject packet.
--
1.9.1
-Martin.
On 13/10/14 15:44, Jukka Rissanen wrote:
> Hi Martin,
>
> and thanks for your analysis.
>
> On ma, 2014-10-13 at 14:10 +0100, Martin Townsend wrote:
>> Hi Jukka,
>>
>> I think there's a lock checking option in the kernel hacking configuration menu. Might be worth trying this to get more info.
>> I had a quick look through the code and there maybe a potential locking problem in l2cap_le_credits
>> it calls l2cap_get_chan_by_dcid which locks the channel lock (chan->lock) which is one of the locks in the deadlock below. If credits > max_credits in l2cap_le_credits it returns 0 but no unlock. Now l2cap_send_disconn_req may do this, I tried searching through but it called a state_change op so I gave up.
>> http://lxr.free-electrons.com/source/net/bluetooth/l2cap_core.c#L5539
>>
>> You could try sticking a l2cap_chan_unlock(chan); in to see if the problem goes away.
>>
> I managed to trigger the locking issue (by running ssh over bt 6lowpan)
> even without your patch. So I am acking the v6 of this patch. I try to
> dig the root cause to that deadlock issue I am seeing.
>
> Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
>
>
> Cheers,
> Jukka
>
>
^ permalink raw reply related
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Jukka Rissanen @ 2014-10-13 14:44 UTC (permalink / raw)
To: Martin Townsend
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel, alex.aring
In-Reply-To: <543BCF44.1030001@xsilon.com>
Hi Martin,
and thanks for your analysis.
On ma, 2014-10-13 at 14:10 +0100, Martin Townsend wrote:
> Hi Jukka,
>
> I think there's a lock checking option in the kernel hacking configuration menu. Might be worth trying this to get more info.
> I had a quick look through the code and there maybe a potential locking problem in l2cap_le_credits
> it calls l2cap_get_chan_by_dcid which locks the channel lock (chan->lock) which is one of the locks in the deadlock below. If credits > max_credits in l2cap_le_credits it returns 0 but no unlock. Now l2cap_send_disconn_req may do this, I tried searching through but it called a state_change op so I gave up.
> http://lxr.free-electrons.com/source/net/bluetooth/l2cap_core.c#L5539
>
> You could try sticking a l2cap_chan_unlock(chan); in to see if the problem goes away.
>
I managed to trigger the locking issue (by running ssh over bt 6lowpan)
even without your patch. So I am acking the v6 of this patch. I try to
dig the root cause to that deadlock issue I am seeing.
Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Cheers,
Jukka
^ permalink raw reply
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Martin Townsend @ 2014-10-13 13:10 UTC (permalink / raw)
To: Jukka Rissanen, Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, alex.aring
In-Reply-To: <1413200959.2705.90.camel@jrissane-mobl.ger.corp.intel.com>
Hi Jukka,
I think there's a lock checking option in the kernel hacking configuration menu. Might be worth trying this to get more info.
I had a quick look through the code and there maybe a potential locking problem in l2cap_le_credits
it calls l2cap_get_chan_by_dcid which locks the channel lock (chan->lock) which is one of the locks in the deadlock below. If credits > max_credits in l2cap_le_credits it returns 0 but no unlock. Now l2cap_send_disconn_req may do this, I tried searching through but it called a state_change op so I gave up.
http://lxr.free-electrons.com/source/net/bluetooth/l2cap_core.c#L5539
You could try sticking a l2cap_chan_unlock(chan); in to see if the problem goes away.
Alex have you tried this patch in a virtual emulator or on real HW for 802.15.4? we would know if it's a bluetooth or general problem.
- Martin
On 13/10/14 12:49, Jukka Rissanen wrote:
> Hi Martin,
>
> with this v6 I started to see similar issues locking issues as in some
> earlier patch version. I will try v5 and v4 just to make sure what is
> going on here.
>
>
> [ 308.736047] =================================
> [ 308.736047] [ INFO: inconsistent lock state ]
> [ 308.736047] 3.17.0-rc1-bt6lowpan #1 Not tainted
> [ 308.736047] ---------------------------------
> [ 308.736047] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
> [ 308.736047] kworker/u3:2/404 [HC0[0]:SC0[0]:HE1:SE1] takes:
> [ 308.736047] (&(&list->lock)->rlock#6){+.?...}, at: [<f82a8d4c>]
> hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] {IN-SOFTIRQ-W} state was registered at:
> [ 308.736047] [<c10915a3>] __lock_acquire+0x6d3/0x1d20
> [ 308.736047] [<c109325d>] lock_acquire+0x9d/0x140
> [ 308.736047] [<c1889c25>] _raw_spin_lock+0x45/0x80
> [ 308.736047] [<f82a8d4c>] hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] [<f82c9bf0>] l2cap_do_send+0x60/0x100 [bluetooth]
> [ 308.736047] [<f82cd7c0>] l2cap_chan_send+0x7f0/0x10e0 [bluetooth]
> [ 308.736047] [<f850d91e>] send_pkt+0x4e/0xa0 [bluetooth_6lowpan]
> [ 308.736047] [<f850dd20>] bt_xmit+0x3b0/0x770 [bluetooth_6lowpan]
> [ 308.736047] [<c17742f4>] dev_hard_start_xmit+0x344/0x670
> [ 308.736047] [<c17749ad>] __dev_queue_xmit+0x38d/0x680
> [ 308.736047] [<c1774caf>] dev_queue_xmit+0xf/0x20
> [ 308.736047] [<c177b8b0>] neigh_connected_output+0x130/0x1a0
> [ 308.736047] [<c1812a63>] ip6_finish_output2+0x173/0x8c0
> [ 308.736047] [<c18182db>] ip6_finish_output+0x7b/0x1b0
> [ 308.736047] [<c18184a7>] ip6_output+0x97/0x2a0
> [ 308.736047] [<c183a46b>] mld_sendpack+0x5eb/0x650
> [ 308.736047] [<c183acc1>] mld_ifc_timer_expire+0x191/0x2f0
> [ 308.736047] [<c10ac385>] call_timer_fn+0x85/0x1c0
> [ 308.736047] [<c10acb72>] run_timer_softirq+0x192/0x280
> [ 308.736047] [<c104fd84>] __do_softirq+0xd4/0x300
> [ 308.736047] [<c10049fc>] do_softirq_own_stack+0x2c/0x40
> [ 308.736047] [<c1050136>] irq_exit+0x86/0xb0
> [ 308.736047] [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
> [ 308.736047] [<c188b6ce>] apic_timer_interrupt+0x32/0x38
> [ 308.736047] [<c115e6b2>] kmem_cache_alloc+0x1c2/0x290
> [ 308.736047] [<c116a9ae>] create_object+0x2e/0x280
> [ 308.736047] [<c187f10c>] kmemleak_alloc+0x3c/0xb0
> [ 308.736047] [<c115e693>] kmem_cache_alloc+0x1a3/0x290
> [ 308.736047] [<c1178835>] getname_flags+0x25/0x110
> [ 308.736047] [<c117d21e>] user_path_at_empty+0x1e/0x80
> [ 308.736047] [<c1173317>] SyS_readlinkat+0x57/0x100
> [ 308.736047] [<c11733ec>] SyS_readlink+0x2c/0x30
> [ 308.736047] [<c188aeb6>] syscall_after_call+0x0/0x4
> [ 308.736047] irq event stamp: 37665
> [ 308.736047] hardirqs last enabled at (37665): [<c188a065>]
> _raw_spin_unlock_irqrestore+0x55/0x70
> [ 308.736047] hardirqs last disabled at (37664): [<c1889e03>]
> _raw_spin_lock_irqsave+0x23/0x90
> [ 308.736047] softirqs last enabled at (37378): [<c104ff5c>]
> __do_softirq+0x2ac/0x300
> [ 308.736047] softirqs last disabled at (37359): [<c10049fc>]
> do_softirq_own_stack+0x2c/0x40
> [ 308.736047]
> [ 308.736047] other info that might help us debug this:
> [ 308.736047] Possible unsafe locking scenario:
> [ 308.736047]
> [ 308.736047] CPU0
> [ 308.736047] ----
> [ 308.736047] lock(&(&list->lock)->rlock#6);
> [ 308.736047] <Interrupt>
> [ 308.736047] lock(&(&list->lock)->rlock#6);
> [ 308.736047]
> [ 308.736047] *** DEADLOCK ***
> [ 308.736047]
> [ 308.736047] 3 locks held by kworker/u3:2/404:
> [ 308.736047] #0: ("%s"hdev->name#2){.+.+.+}, at: [<c10622c3>]
> process_one_work+0x113/0x4a0
> [ 308.736047] #1: ((&hdev->rx_work)){+.+.+.}, at: [<c10622c3>]
> process_one_work+0x113/0x4a0
> [ 308.736047] #2: (&chan->lock){+.+.+.}, at: [<f82c9179>]
> l2cap_get_chan_by_dcid+0x89/0x90 [bluetooth]
> [ 308.736047]
> [ 308.736047] stack backtrace:
> [ 308.736047] CPU: 0 PID: 404 Comm: kworker/u3:2 Not tainted
> 3.17.0-rc1-bt6lowpan #1
> [ 308.736047] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
> VirtualBox 12/01/2006
> [ 308.736047] Workqueue: hci0 hci_rx_work [bluetooth]
> [ 308.736047] efe3d780 00000000 efedbb90 c18821c1 c2345c10 efedbbb0
> c1880a94 c1ad9264
> [ 308.736047] c1ad9201 c1ad95f8 efe3dd94 00000006 c108e0c0 efedbbe0
> c108ea8c 00000006
> [ 308.736047] efe3d780 efedbffc e21128f4 00000047 00000004 efe3d780
> efe3dd98 00000003
> [ 308.736047] Call Trace:
> [ 308.736047] [<c18821c1>] dump_stack+0x4b/0x75
> [ 308.736047] [<c1880a94>] print_usage_bug.part.36+0x209/0x213
> [ 308.736047] [<c108e0c0>] ? check_usage_forwards+0x110/0x110
> [ 308.736047] [<c108ea8c>] mark_lock+0x11c/0x6e0
> [ 308.736047] [<c10915e4>] __lock_acquire+0x714/0x1d20
> [ 308.736047] [<c1004b6f>] ? dump_trace+0xcf/0x1f0
> [ 308.736047] [<c100a5f8>] ? sched_clock+0x8/0x10
> [ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
> [ 308.736047] [<c109325d>] lock_acquire+0x9d/0x140
> [ 308.736047] [<f82a8d4c>] ? hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] [<c1889c25>] _raw_spin_lock+0x45/0x80
> [ 308.736047] [<f82a8d4c>] ? hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] [<f82a8d4c>] hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] [<c108f0b4>] ? mark_held_locks+0x64/0x90
> [ 308.736047] [<c188a065>] ? _raw_spin_unlock_irqrestore+0x55/0x70
> [ 308.736047] [<f82c9bf0>] l2cap_do_send+0x60/0x100 [bluetooth]
> [ 308.736047] [<c108f32b>] ? trace_hardirqs_on+0xb/0x10
> [ 308.736047] [<c188a051>] ? _raw_spin_unlock_irqrestore+0x41/0x70
> [ 308.736047] [<c1763125>] ? skb_dequeue+0x45/0x60
> [ 308.736047] [<f82d4389>] l2cap_recv_frame+0x23d9/0x2db0 [bluetooth]
> [ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
> [ 308.736047] [<c100a5f8>] ? sched_clock+0x8/0x10
> [ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
> [ 308.736047] [<c10761ef>] ? sched_clock_cpu+0x10f/0x160
> [ 308.736047] [<c107183b>] ? get_parent_ip+0xb/0x40
> [ 308.736047] [<c10718bb>] ? preempt_count_add+0x4b/0xa0
> [ 308.736047] [<c13d09e2>] ? debug_smp_processor_id+0x12/0x20
> [ 308.736047] [<c108cc04>] ? get_lock_stats+0x24/0x40
> [ 308.736047] [<c108f0b4>] ? mark_held_locks+0x64/0x90
> [ 308.736047] [<c188716d>] ? __mutex_unlock_slowpath+0xcd/0x1b0
> [ 308.736047] [<c13d09ff>] ? __this_cpu_preempt_check+0xf/0x20
> [ 308.736047] [<c108f1d4>] ? trace_hardirqs_on_caller+0xf4/0x240
> [ 308.736047] [<c108c9a6>] ? trace_hardirqs_off_caller+0xb6/0x160
> [ 308.736047] [<f82d62a9>] l2cap_recv_acldata+0x2f9/0x340 [bluetooth]
> [ 308.736047] [<f82a1a13>] ? hci_rx_work+0x113/0x4a0 [bluetooth]
> [ 308.736047] [<f82a1c69>] hci_rx_work+0x369/0x4a0 [bluetooth]
> [ 308.736047] [<f82a1a13>] ? hci_rx_work+0x113/0x4a0 [bluetooth]
> [ 308.736047] [<c106234a>] process_one_work+0x19a/0x4a0
> [ 308.736047] [<c10622c3>] ? process_one_work+0x113/0x4a0
> [ 308.736047] [<c10629e9>] worker_thread+0x39/0x440
> [ 308.736047] [<c10629b0>] ? init_pwq+0xc0/0xc0
> [ 308.736047] [<c1066dc8>] kthread+0xa8/0xc0
> [ 308.736047] [<c108f32b>] ? trace_hardirqs_on+0xb/0x10
> [ 308.736047] [<c188ad01>] ret_from_kernel_thread+0x21/0x30
> [ 308.736047] [<c1066d20>] ? kthread_create_on_node+0x160/0x160
>
>
>
> Cheers,
> Jukka
>
>
^ permalink raw reply
* Re: [PATCH v6 bluetooth-next] 6lowpan: Use skb_cow in IPHC decompression.
From: Jukka Rissanen @ 2014-10-13 11:49 UTC (permalink / raw)
To: Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, alex.aring, Martin Townsend
In-Reply-To: <1413194456-26351-2-git-send-email-martin.townsend@xsilon.com>
Hi Martin,
with this v6 I started to see similar issues locking issues as in some
earlier patch version. I will try v5 and v4 just to make sure what is
going on here.
[ 308.736047] =================================
[ 308.736047] [ INFO: inconsistent lock state ]
[ 308.736047] 3.17.0-rc1-bt6lowpan #1 Not tainted
[ 308.736047] ---------------------------------
[ 308.736047] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
[ 308.736047] kworker/u3:2/404 [HC0[0]:SC0[0]:HE1:SE1] takes:
[ 308.736047] (&(&list->lock)->rlock#6){+.?...}, at: [<f82a8d4c>]
hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] {IN-SOFTIRQ-W} state was registered at:
[ 308.736047] [<c10915a3>] __lock_acquire+0x6d3/0x1d20
[ 308.736047] [<c109325d>] lock_acquire+0x9d/0x140
[ 308.736047] [<c1889c25>] _raw_spin_lock+0x45/0x80
[ 308.736047] [<f82a8d4c>] hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] [<f82c9bf0>] l2cap_do_send+0x60/0x100 [bluetooth]
[ 308.736047] [<f82cd7c0>] l2cap_chan_send+0x7f0/0x10e0 [bluetooth]
[ 308.736047] [<f850d91e>] send_pkt+0x4e/0xa0 [bluetooth_6lowpan]
[ 308.736047] [<f850dd20>] bt_xmit+0x3b0/0x770 [bluetooth_6lowpan]
[ 308.736047] [<c17742f4>] dev_hard_start_xmit+0x344/0x670
[ 308.736047] [<c17749ad>] __dev_queue_xmit+0x38d/0x680
[ 308.736047] [<c1774caf>] dev_queue_xmit+0xf/0x20
[ 308.736047] [<c177b8b0>] neigh_connected_output+0x130/0x1a0
[ 308.736047] [<c1812a63>] ip6_finish_output2+0x173/0x8c0
[ 308.736047] [<c18182db>] ip6_finish_output+0x7b/0x1b0
[ 308.736047] [<c18184a7>] ip6_output+0x97/0x2a0
[ 308.736047] [<c183a46b>] mld_sendpack+0x5eb/0x650
[ 308.736047] [<c183acc1>] mld_ifc_timer_expire+0x191/0x2f0
[ 308.736047] [<c10ac385>] call_timer_fn+0x85/0x1c0
[ 308.736047] [<c10acb72>] run_timer_softirq+0x192/0x280
[ 308.736047] [<c104fd84>] __do_softirq+0xd4/0x300
[ 308.736047] [<c10049fc>] do_softirq_own_stack+0x2c/0x40
[ 308.736047] [<c1050136>] irq_exit+0x86/0xb0
[ 308.736047] [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
[ 308.736047] [<c188b6ce>] apic_timer_interrupt+0x32/0x38
[ 308.736047] [<c115e6b2>] kmem_cache_alloc+0x1c2/0x290
[ 308.736047] [<c116a9ae>] create_object+0x2e/0x280
[ 308.736047] [<c187f10c>] kmemleak_alloc+0x3c/0xb0
[ 308.736047] [<c115e693>] kmem_cache_alloc+0x1a3/0x290
[ 308.736047] [<c1178835>] getname_flags+0x25/0x110
[ 308.736047] [<c117d21e>] user_path_at_empty+0x1e/0x80
[ 308.736047] [<c1173317>] SyS_readlinkat+0x57/0x100
[ 308.736047] [<c11733ec>] SyS_readlink+0x2c/0x30
[ 308.736047] [<c188aeb6>] syscall_after_call+0x0/0x4
[ 308.736047] irq event stamp: 37665
[ 308.736047] hardirqs last enabled at (37665): [<c188a065>]
_raw_spin_unlock_irqrestore+0x55/0x70
[ 308.736047] hardirqs last disabled at (37664): [<c1889e03>]
_raw_spin_lock_irqsave+0x23/0x90
[ 308.736047] softirqs last enabled at (37378): [<c104ff5c>]
__do_softirq+0x2ac/0x300
[ 308.736047] softirqs last disabled at (37359): [<c10049fc>]
do_softirq_own_stack+0x2c/0x40
[ 308.736047]
[ 308.736047] other info that might help us debug this:
[ 308.736047] Possible unsafe locking scenario:
[ 308.736047]
[ 308.736047] CPU0
[ 308.736047] ----
[ 308.736047] lock(&(&list->lock)->rlock#6);
[ 308.736047] <Interrupt>
[ 308.736047] lock(&(&list->lock)->rlock#6);
[ 308.736047]
[ 308.736047] *** DEADLOCK ***
[ 308.736047]
[ 308.736047] 3 locks held by kworker/u3:2/404:
[ 308.736047] #0: ("%s"hdev->name#2){.+.+.+}, at: [<c10622c3>]
process_one_work+0x113/0x4a0
[ 308.736047] #1: ((&hdev->rx_work)){+.+.+.}, at: [<c10622c3>]
process_one_work+0x113/0x4a0
[ 308.736047] #2: (&chan->lock){+.+.+.}, at: [<f82c9179>]
l2cap_get_chan_by_dcid+0x89/0x90 [bluetooth]
[ 308.736047]
[ 308.736047] stack backtrace:
[ 308.736047] CPU: 0 PID: 404 Comm: kworker/u3:2 Not tainted
3.17.0-rc1-bt6lowpan #1
[ 308.736047] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
VirtualBox 12/01/2006
[ 308.736047] Workqueue: hci0 hci_rx_work [bluetooth]
[ 308.736047] efe3d780 00000000 efedbb90 c18821c1 c2345c10 efedbbb0
c1880a94 c1ad9264
[ 308.736047] c1ad9201 c1ad95f8 efe3dd94 00000006 c108e0c0 efedbbe0
c108ea8c 00000006
[ 308.736047] efe3d780 efedbffc e21128f4 00000047 00000004 efe3d780
efe3dd98 00000003
[ 308.736047] Call Trace:
[ 308.736047] [<c18821c1>] dump_stack+0x4b/0x75
[ 308.736047] [<c1880a94>] print_usage_bug.part.36+0x209/0x213
[ 308.736047] [<c108e0c0>] ? check_usage_forwards+0x110/0x110
[ 308.736047] [<c108ea8c>] mark_lock+0x11c/0x6e0
[ 308.736047] [<c10915e4>] __lock_acquire+0x714/0x1d20
[ 308.736047] [<c1004b6f>] ? dump_trace+0xcf/0x1f0
[ 308.736047] [<c100a5f8>] ? sched_clock+0x8/0x10
[ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
[ 308.736047] [<c109325d>] lock_acquire+0x9d/0x140
[ 308.736047] [<f82a8d4c>] ? hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] [<c1889c25>] _raw_spin_lock+0x45/0x80
[ 308.736047] [<f82a8d4c>] ? hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] [<f82a8d4c>] hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] [<c108f0b4>] ? mark_held_locks+0x64/0x90
[ 308.736047] [<c188a065>] ? _raw_spin_unlock_irqrestore+0x55/0x70
[ 308.736047] [<f82c9bf0>] l2cap_do_send+0x60/0x100 [bluetooth]
[ 308.736047] [<c108f32b>] ? trace_hardirqs_on+0xb/0x10
[ 308.736047] [<c188a051>] ? _raw_spin_unlock_irqrestore+0x41/0x70
[ 308.736047] [<c1763125>] ? skb_dequeue+0x45/0x60
[ 308.736047] [<f82d4389>] l2cap_recv_frame+0x23d9/0x2db0 [bluetooth]
[ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
[ 308.736047] [<c100a5f8>] ? sched_clock+0x8/0x10
[ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
[ 308.736047] [<c10761ef>] ? sched_clock_cpu+0x10f/0x160
[ 308.736047] [<c107183b>] ? get_parent_ip+0xb/0x40
[ 308.736047] [<c10718bb>] ? preempt_count_add+0x4b/0xa0
[ 308.736047] [<c13d09e2>] ? debug_smp_processor_id+0x12/0x20
[ 308.736047] [<c108cc04>] ? get_lock_stats+0x24/0x40
[ 308.736047] [<c108f0b4>] ? mark_held_locks+0x64/0x90
[ 308.736047] [<c188716d>] ? __mutex_unlock_slowpath+0xcd/0x1b0
[ 308.736047] [<c13d09ff>] ? __this_cpu_preempt_check+0xf/0x20
[ 308.736047] [<c108f1d4>] ? trace_hardirqs_on_caller+0xf4/0x240
[ 308.736047] [<c108c9a6>] ? trace_hardirqs_off_caller+0xb6/0x160
[ 308.736047] [<f82d62a9>] l2cap_recv_acldata+0x2f9/0x340 [bluetooth]
[ 308.736047] [<f82a1a13>] ? hci_rx_work+0x113/0x4a0 [bluetooth]
[ 308.736047] [<f82a1c69>] hci_rx_work+0x369/0x4a0 [bluetooth]
[ 308.736047] [<f82a1a13>] ? hci_rx_work+0x113/0x4a0 [bluetooth]
[ 308.736047] [<c106234a>] process_one_work+0x19a/0x4a0
[ 308.736047] [<c10622c3>] ? process_one_work+0x113/0x4a0
[ 308.736047] [<c10629e9>] worker_thread+0x39/0x440
[ 308.736047] [<c10629b0>] ? init_pwq+0xc0/0xc0
[ 308.736047] [<c1066dc8>] kthread+0xa8/0xc0
[ 308.736047] [<c108f32b>] ? trace_hardirqs_on+0xb/0x10
[ 308.736047] [<c188ad01>] ret_from_kernel_thread+0x21/0x30
[ 308.736047] [<c1066d20>] ? kthread_create_on_node+0x160/0x160
Cheers,
Jukka
^ permalink raw reply
* Re: [PATCH] core: Add btd_adapter_recreate_bonding()
From: Alfonso Acosta @ 2014-10-13 11:48 UTC (permalink / raw)
To: BlueZ development
In-Reply-To: <1413200623-31278-1-git-send-email-fons@spotify.com>
This patch is identical to the one sent last week (resent by error).
On Mon, Oct 13, 2014 at 1:43 PM, Alfonso Acosta <fons@spotify.com> wrote:
> This patch adds btd_adapter_recreate_bonding() which rebonds a device,
> i.e. it performs an unbonding operation inmediately followed by a
> bonding operation.
>
> Although there is no internal use for btd_adapter_recreate_bonding()
> yet, it is useful for plugins dealing with devices which require
> renegotiaing their keys. For instance, when dealing with devices
> without persistent storage which lose their keys on reset.
>
> Certain caveats have been taken into account when rebonding with a LE
> device:
>
> * If the device to rebond is already connected, the rebonding is done
> without disconnecting to avoid the extra latency of reestablishing
> the connection.
>
> * If no connection exists, we connect before unbonding anyways to
> avoid losing the device's autoconnection and connection parameters,
> which are removed by the kernel when unpairing if no connection
> exists.
>
> * Not closing the connection requires defering attribute discovery
> until the rebonding is done. Otherwise, the security level could be
> elavated with the old LTK, which may be invalid since we are
> rebonding. When rebonding, attribute discovery is suspended and the
> ATT socket is saved to allow resuming it once bonding is finished.
> ---
> src/adapter.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> src/adapter.h | 7 ++++++-
> src/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> src/device.h | 4 ++++
> 4 files changed, 119 insertions(+), 7 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 92ee1a0..81e8f22 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -5195,14 +5195,15 @@ int btd_adapter_read_clock(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
> }
>
> int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> - const bdaddr_t *bdaddr, uint8_t bdaddr_type)
> + const bdaddr_t *bdaddr, uint8_t bdaddr_type,
> + uint8_t disconnect)
> {
> struct mgmt_cp_unpair_device cp;
>
> memset(&cp, 0, sizeof(cp));
> bacpy(&cp.addr.bdaddr, bdaddr);
> cp.addr.type = bdaddr_type;
> - cp.disconnect = 1;
> + cp.disconnect = disconnect;
>
> if (mgmt_send(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE,
> adapter->dev_id, sizeof(cp), &cp,
> @@ -5212,6 +5213,53 @@ int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> return -EIO;
> }
>
> +int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
> + const bdaddr_t *bdaddr,
> + uint8_t bdaddr_type,
> + uint8_t io_cap)
> +{
> + struct btd_device *device;
> + int err;
> +
> + device = btd_adapter_get_device(adapter, bdaddr, bdaddr_type);
> +
> + if (!device || !device_is_bonded(device, bdaddr_type))
> + return -EINVAL;
> +
> + device_set_rebonding(device, bdaddr_type, true);
> +
> + /* Make sure the device is connected before unbonding to avoid
> + * losing the device's autoconnection and connection
> + * parameters, which are removed by the kernel when unpairing
> + * if no connection exists. We would had anyways implicitly
> + * connected when bonding later on, so we might as well just
> + * do it explicitly now.
> + */
> + if (bdaddr_type != BDADDR_BREDR && !btd_device_is_connected(device)) {
> + err = device_connect_le(device);
> + if (err < 0)
> + goto failed;
> + }
> +
> + /* Unbond without disconnecting to avoid the connection
> + * re-establishment latency
> + */
> + err = btd_adapter_remove_bonding(adapter, bdaddr, bdaddr_type, 0);
> + if (err < 0)
> + goto failed;
> +
> + err = adapter_create_bonding(adapter, bdaddr, bdaddr_type, io_cap);
> + if (err < 0)
> + goto failed;
> +
> + return 0;
> +
> +failed:
> + error("failed: %s", strerror(-err));
> + device_set_rebonding(device, bdaddr_type, false);
> + return err;
> +}
> +
> static void pincode_reply_complete(uint8_t status, uint16_t length,
> const void *param, void *user_data)
> {
> @@ -5594,8 +5642,11 @@ static void bonding_complete(struct btd_adapter *adapter,
> else
> device = btd_adapter_find_device(adapter, bdaddr, addr_type);
>
> - if (device != NULL)
> + if (device != NULL) {
> device_bonding_complete(device, addr_type, status);
> + if (device_is_rebonding(device, addr_type))
> + device_rebonding_complete(device, addr_type);
> + }
>
> resume_discovery(adapter);
>
> diff --git a/src/adapter.h b/src/adapter.h
> index 6801fee..bd00d3e 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -158,7 +158,12 @@ int btd_adapter_disconnect_device(struct btd_adapter *adapter,
> uint8_t bdaddr_type);
>
> int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> - const bdaddr_t *bdaddr, uint8_t bdaddr_type);
> + const bdaddr_t *bdaddr, uint8_t bdaddr_type,
> + uint8_t disconnect);
> +int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
> + const bdaddr_t *bdaddr,
> + uint8_t bdaddr_type,
> + uint8_t io_cap);
>
> int btd_adapter_pincode_reply(struct btd_adapter *adapter,
> const bdaddr_t *bdaddr,
> diff --git a/src/device.c b/src/device.c
> index 875a5c5..4aab349 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -158,6 +158,7 @@ struct att_callbacks {
> struct bearer_state {
> bool paired;
> bool bonded;
> + bool rebonding;
> bool connected;
> bool svc_resolved;
> };
> @@ -221,6 +222,8 @@ struct btd_device {
> int8_t rssi;
>
> GIOChannel *att_io;
> + GIOChannel *att_rebond_io;
> +
> guint cleanup_id;
> guint store_id;
> };
> @@ -522,6 +525,9 @@ static void device_free(gpointer user_data)
>
> attio_cleanup(device);
>
> + if (device->att_rebond_io)
> + g_io_channel_unref(device->att_rebond_io);
> +
> if (device->tmp_records)
> sdp_list_free(device->tmp_records,
> (sdp_free_func_t) sdp_record_free);
> @@ -1739,6 +1745,23 @@ long device_bonding_last_duration(struct btd_device *device)
> return bonding->last_attempt_duration_ms;
> }
>
> +bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type)
> +{
> + struct bearer_state *state = get_state(device, bdaddr_type);
> +
> + return state->rebonding;
> +}
> +
> +void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
> + bool rebonding)
> +{
> + struct bearer_state *state = get_state(device, bdaddr_type);
> +
> + state->rebonding = rebonding;
> +
> + DBG("rebonding = %d", rebonding);
> +}
> +
> static void create_bond_req_exit(DBusConnection *conn, void *user_data)
> {
> struct btd_device *device = user_data;
> @@ -2029,7 +2052,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
>
> if (state->paired && !state->bonded)
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - bdaddr_type);
> + bdaddr_type, 1);
>
> if (device->bredr_state.connected || device->le_state.connected)
> return;
> @@ -2639,13 +2662,13 @@ static void device_remove_stored(struct btd_device *device)
> if (device->bredr_state.bonded) {
> device->bredr_state.bonded = false;
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - BDADDR_BREDR);
> + BDADDR_BREDR, 1);
> }
>
> if (device->le_state.bonded) {
> device->le_state.bonded = false;
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - device->bdaddr_type);
> + device->bdaddr_type, 1);
> }
>
> device->bredr_state.paired = false;
> @@ -3628,6 +3651,18 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
> GAttrib *attrib;
> BtIOSecLevel sec_level;
>
> + DBG("");
> +
> + if (dev->le_state.rebonding) {
> + DBG("postponing due to rebonding");
> + /* Keep the att socket, and defer attaching the attributes
> + * until rebonding is done.
> + */
> + if (!dev->att_rebond_io)
> + dev->att_rebond_io = g_io_channel_ref(io);
> + return false;
> + }
> +
> bt_io_get(io, &gerr, BT_IO_OPT_SEC_LEVEL, &sec_level,
> BT_IO_OPT_INVALID);
> if (gerr) {
> @@ -4269,6 +4304,23 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> }
> }
>
> +bool device_rebonding_complete(struct btd_device *device, uint8_t bdaddr_type)
> +{
> + bool ret = true;
> +
> + DBG("");
> +
> + device_set_rebonding(device, bdaddr_type, false);
> +
> + if (bdaddr_type != BDADDR_BREDR && device->att_rebond_io) {
> + ret = device_attach_attrib(device, device->att_rebond_io);
> + g_io_channel_unref(device->att_rebond_io);
> + device->att_rebond_io = NULL;
> + }
> +
> + return ret;
> +}
> +
> static gboolean svc_idle_cb(gpointer user_data)
> {
> struct svc_callback *cb = user_data;
> diff --git a/src/device.h b/src/device.h
> index 2e0473e..65b1018 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -94,6 +94,10 @@ bool device_is_retrying(struct btd_device *device);
> void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> uint8_t status);
> gboolean device_is_bonding(struct btd_device *device, const char *sender);
> +bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type);
> +void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
> + bool rebonding);
> +bool device_complete_rebonding(struct btd_device *device, uint8_t bdaddr_type);
> void device_bonding_attempt_failed(struct btd_device *device, uint8_t status);
> void device_bonding_failed(struct btd_device *device, uint8_t status);
> struct btd_adapter_pin_cb_iter *device_bonding_iter(struct btd_device *device);
> --
> 1.9.1
>
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v2 2/2] core: Add subscription API for Manufacturer Specific Data
From: Alfonso Acosta @ 2014-10-13 11:45 UTC (permalink / raw)
To: Alfonso Acosta, BlueZ development
In-Reply-To: <20141013080138.GA5588@t440s.lan>
Hi Johan,
> On Fri, Oct 10, 2014, Alfonso Acosta wrote:
>> --- a/src/adapter.h
>> +++ b/src/adapter.h
>> @@ -30,6 +30,8 @@
>> #include <glib.h>
>> #include <stdbool.h>
>>
>> +#include "eir.h"
>> +
>> #define MAX_NAME_LENGTH 248
>>
>> /* Invalid SSP passkey value used to indicate negative replies */
>> @@ -138,6 +140,14 @@ struct btd_adapter_pin_cb_iter *btd_adapter_pin_cb_iter_new(
>> void btd_adapter_pin_cb_iter_free(struct btd_adapter_pin_cb_iter *iter);
>> bool btd_adapter_pin_cb_iter_end(struct btd_adapter_pin_cb_iter *iter);
>>
>> +typedef void (*btd_msd_cb_t) (struct btd_adapter *adapter,
>> + struct btd_device *dev,
>> + const struct eir_msd *msd);
>> +void btd_adapter_register_msd_cb(struct btd_adapter *adapter,
>> + btd_msd_cb_t cb);
>
> In our user space code we try to follow the following principles for
> internal header files:
>
> 1) The c-file that includes them should also include the prerequisites
> 2) We don't use multi-include guards
>
> The general idea is that we don't want hidden and implicit dependencies
> but prefer having them explicitly spelled out. This practice also helps
> detect circular dependencies. For public header files or those of
> library-like modules we don't follow this practice (e.g. gdbus/gdbus.h).
>
> As for your patch, I'd suggest to spell out each of the three variables
> in your bt_msd_cb_t instead of using the "struct eir_msd" in adapter.h.
> That way you don't have a dependency to eir.h from adapter.h.
Thanks for the explanation. v3 corrects this.
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* [PATCH v3 2/2] core: Add subscription API for Manufacturer Specific Data
From: Alfonso Acosta @ 2014-10-13 11:43 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1413200623-31278-1-git-send-email-fons@spotify.com>
This patch allows plugins to be notified whenever an adapter receives
Manufacturer Specific Data in the advertising reports from a device.
This can happen when new device is discovered or when we autoconnect
to it.
---
src/adapter.c | 36 ++++++++++++++++++++++++++++++++++++
src/adapter.h | 10 ++++++++++
2 files changed, 46 insertions(+)
diff --git a/src/adapter.c b/src/adapter.c
index 92ee1a0..d04ea83 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -206,6 +206,7 @@ struct btd_adapter {
gboolean initialized;
GSList *pin_callbacks;
+ GSList *msd_callbacks;
GSList *drivers;
GSList *profiles;
@@ -4549,6 +4550,9 @@ static void adapter_remove(struct btd_adapter *adapter)
g_slist_free(adapter->pin_callbacks);
adapter->pin_callbacks = NULL;
+
+ g_slist_free(adapter->msd_callbacks);
+ adapter->msd_callbacks = NULL;
}
const char *adapter_get_path(struct btd_adapter *adapter)
@@ -4647,6 +4651,20 @@ static void confirm_name(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
confirm_name_timeout, adapter);
}
+void adapter_msd_notify(struct btd_adapter *adapter, struct btd_device *dev,
+ const struct eir_msd *msd)
+{
+ GSList *l, *next;
+
+ for (l = adapter->msd_callbacks; l != NULL; l = next) {
+ btd_msd_cb_t cb = l->data;
+
+ next = g_slist_next(l);
+
+ cb(adapter, dev, msd->company, msd->data, msd->data_len);
+ }
+}
+
static void update_found_devices(struct btd_adapter *adapter,
const bdaddr_t *bdaddr,
uint8_t bdaddr_type, int8_t rssi,
@@ -4738,6 +4756,9 @@ static void update_found_devices(struct btd_adapter *adapter,
device_add_eir_uuids(dev, eir_data.services);
+ if (eir_data.msd)
+ adapter_msd_notify(adapter, dev, eir_data.msd);
+
eir_data_free(&eir_data);
/*
@@ -5173,6 +5194,18 @@ void btd_adapter_unregister_pin_cb(struct btd_adapter *adapter,
adapter->pin_callbacks = g_slist_remove(adapter->pin_callbacks, cb);
}
+void btd_adapter_unregister_msd_cb(struct btd_adapter *adapter,
+ btd_msd_cb_t cb)
+{
+ adapter->msd_callbacks = g_slist_remove(adapter->msd_callbacks, cb);
+}
+
+void btd_adapter_register_msd_cb(struct btd_adapter *adapter,
+ btd_msd_cb_t cb)
+{
+ adapter->msd_callbacks = g_slist_prepend(adapter->msd_callbacks, cb);
+}
+
int btd_adapter_set_fast_connectable(struct btd_adapter *adapter,
gboolean enable)
{
@@ -6663,6 +6696,9 @@ static void connected_callback(uint16_t index, uint16_t length,
btd_device_device_set_name(device, eir_data.name);
}
+ if (eir_data.msd)
+ adapter_msd_notify(adapter, device, eir_data.msd);
+
eir_data_free(&eir_data);
}
diff --git a/src/adapter.h b/src/adapter.h
index 6801fee..8f4098a 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -138,6 +138,16 @@ struct btd_adapter_pin_cb_iter *btd_adapter_pin_cb_iter_new(
void btd_adapter_pin_cb_iter_free(struct btd_adapter_pin_cb_iter *iter);
bool btd_adapter_pin_cb_iter_end(struct btd_adapter_pin_cb_iter *iter);
+typedef void (*btd_msd_cb_t) (struct btd_adapter *adapter,
+ struct btd_device *dev,
+ uint16_t company,
+ const uint8_t *data,
+ uint8_t data_len);
+void btd_adapter_register_msd_cb(struct btd_adapter *adapter,
+ btd_msd_cb_t cb);
+void btd_adapter_unregister_msd_cb(struct btd_adapter *adapter,
+ btd_msd_cb_t cb);
+
/* If TRUE, enables fast connectabe, i.e. reduces page scan interval and changes
* type. If FALSE, disables fast connectable, i.e. sets page scan interval and
* type to default values. Valid for both connectable and discoverable modes. */
--
1.9.1
^ permalink raw reply related
* [PATCH v3 1/2] core: Add Manufacturer Specific Data EIR field
From: Alfonso Acosta @ 2014-10-13 11:43 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1413200623-31278-1-git-send-email-fons@spotify.com>
Add data structure and parsing support.
---
src/eir.c | 11 +++++++++++
src/eir.h | 8 ++++++++
2 files changed, 19 insertions(+)
diff --git a/src/eir.c b/src/eir.c
index d22ad91..8ebefaa 100644
--- a/src/eir.c
+++ b/src/eir.c
@@ -53,6 +53,8 @@ void eir_data_free(struct eir_data *eir)
eir->hash = NULL;
g_free(eir->randomizer);
eir->randomizer = NULL;
+ g_free(eir->msd);
+ eir->msd = NULL;
}
static void eir_parse_uuid16(struct eir_data *eir, const void *data,
@@ -240,6 +242,15 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
eir->did_product = data[4] | (data[5] << 8);
eir->did_version = data[6] | (data[7] << 8);
break;
+
+ case EIR_MANUFACTURER_DATA:
+ if (data_len < 2 || data_len > 2 + sizeof(eir->msd->data))
+ break;
+ eir->msd = g_malloc(sizeof(*eir->msd));
+ eir->msd->company = get_le16(data);
+ eir->msd->data_len = data_len - 2;
+ memcpy(&eir->msd->data, data + 2, eir->msd->data_len);
+ break;
}
eir_data += field_len + 1;
diff --git a/src/eir.h b/src/eir.h
index e486fa2..4cc9dbf 100644
--- a/src/eir.h
+++ b/src/eir.h
@@ -37,6 +37,7 @@
#define EIR_SSP_RANDOMIZER 0x0F /* SSP Randomizer */
#define EIR_DEVICE_ID 0x10 /* device ID */
#define EIR_GAP_APPEARANCE 0x19 /* GAP appearance */
+#define EIR_MANUFACTURER_DATA 0xFF /* Manufacturer Specific Data */
/* Flags Descriptions */
#define EIR_LIM_DISC 0x01 /* LE Limited Discoverable Mode */
@@ -47,6 +48,12 @@
#define EIR_SIM_HOST 0x10 /* Simultaneous LE and BR/EDR to Same
Device Capable (Host) */
+struct eir_msd {
+ uint16_t company;
+ uint8_t data[HCI_MAX_EIR_LENGTH];
+ uint8_t data_len;
+};
+
struct eir_data {
GSList *services;
unsigned int flags;
@@ -62,6 +69,7 @@ struct eir_data {
uint16_t did_product;
uint16_t did_version;
uint16_t did_source;
+ struct eir_msd *msd;
};
void eir_data_free(struct eir_data *eir);
--
1.9.1
^ permalink raw reply related
* [PATCH v3 0/2] core: Add plugin-support for Manufacturer Specific Data EIR
From: Alfonso Acosta @ 2014-10-13 11:43 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1413200623-31278-1-git-send-email-fons@spotify.com>
This version adds:
* Corrections suggested by Johan on the handling of eir.h
* Parser sanity check on the size of the MSD payload.
* Minor typo correction on commit message.
Alfonso Acosta (2):
core: Add Manufacturer Specific Data EIR field
core: Add subscription API for Manufacturer Specific Data
src/adapter.c | 36 ++++++++++++++++++++++++++++++++++++
src/adapter.h | 10 ++++++++++
src/eir.c | 11 +++++++++++
src/eir.h | 8 ++++++++
4 files changed, 65 insertions(+)
--
1.9.1
^ permalink raw reply
* [PATCH] core: Add btd_adapter_recreate_bonding()
From: Alfonso Acosta @ 2014-10-13 11:43 UTC (permalink / raw)
To: linux-bluetooth
This patch adds btd_adapter_recreate_bonding() which rebonds a device,
i.e. it performs an unbonding operation inmediately followed by a
bonding operation.
Although there is no internal use for btd_adapter_recreate_bonding()
yet, it is useful for plugins dealing with devices which require
renegotiaing their keys. For instance, when dealing with devices
without persistent storage which lose their keys on reset.
Certain caveats have been taken into account when rebonding with a LE
device:
* If the device to rebond is already connected, the rebonding is done
without disconnecting to avoid the extra latency of reestablishing
the connection.
* If no connection exists, we connect before unbonding anyways to
avoid losing the device's autoconnection and connection parameters,
which are removed by the kernel when unpairing if no connection
exists.
* Not closing the connection requires defering attribute discovery
until the rebonding is done. Otherwise, the security level could be
elavated with the old LTK, which may be invalid since we are
rebonding. When rebonding, attribute discovery is suspended and the
ATT socket is saved to allow resuming it once bonding is finished.
---
src/adapter.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
src/adapter.h | 7 ++++++-
src/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
src/device.h | 4 ++++
4 files changed, 119 insertions(+), 7 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 92ee1a0..81e8f22 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -5195,14 +5195,15 @@ int btd_adapter_read_clock(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
}
int btd_adapter_remove_bonding(struct btd_adapter *adapter,
- const bdaddr_t *bdaddr, uint8_t bdaddr_type)
+ const bdaddr_t *bdaddr, uint8_t bdaddr_type,
+ uint8_t disconnect)
{
struct mgmt_cp_unpair_device cp;
memset(&cp, 0, sizeof(cp));
bacpy(&cp.addr.bdaddr, bdaddr);
cp.addr.type = bdaddr_type;
- cp.disconnect = 1;
+ cp.disconnect = disconnect;
if (mgmt_send(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE,
adapter->dev_id, sizeof(cp), &cp,
@@ -5212,6 +5213,53 @@ int btd_adapter_remove_bonding(struct btd_adapter *adapter,
return -EIO;
}
+int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
+ const bdaddr_t *bdaddr,
+ uint8_t bdaddr_type,
+ uint8_t io_cap)
+{
+ struct btd_device *device;
+ int err;
+
+ device = btd_adapter_get_device(adapter, bdaddr, bdaddr_type);
+
+ if (!device || !device_is_bonded(device, bdaddr_type))
+ return -EINVAL;
+
+ device_set_rebonding(device, bdaddr_type, true);
+
+ /* Make sure the device is connected before unbonding to avoid
+ * losing the device's autoconnection and connection
+ * parameters, which are removed by the kernel when unpairing
+ * if no connection exists. We would had anyways implicitly
+ * connected when bonding later on, so we might as well just
+ * do it explicitly now.
+ */
+ if (bdaddr_type != BDADDR_BREDR && !btd_device_is_connected(device)) {
+ err = device_connect_le(device);
+ if (err < 0)
+ goto failed;
+ }
+
+ /* Unbond without disconnecting to avoid the connection
+ * re-establishment latency
+ */
+ err = btd_adapter_remove_bonding(adapter, bdaddr, bdaddr_type, 0);
+ if (err < 0)
+ goto failed;
+
+ err = adapter_create_bonding(adapter, bdaddr, bdaddr_type, io_cap);
+ if (err < 0)
+ goto failed;
+
+ return 0;
+
+failed:
+ error("failed: %s", strerror(-err));
+ device_set_rebonding(device, bdaddr_type, false);
+ return err;
+}
+
static void pincode_reply_complete(uint8_t status, uint16_t length,
const void *param, void *user_data)
{
@@ -5594,8 +5642,11 @@ static void bonding_complete(struct btd_adapter *adapter,
else
device = btd_adapter_find_device(adapter, bdaddr, addr_type);
- if (device != NULL)
+ if (device != NULL) {
device_bonding_complete(device, addr_type, status);
+ if (device_is_rebonding(device, addr_type))
+ device_rebonding_complete(device, addr_type);
+ }
resume_discovery(adapter);
diff --git a/src/adapter.h b/src/adapter.h
index 6801fee..bd00d3e 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -158,7 +158,12 @@ int btd_adapter_disconnect_device(struct btd_adapter *adapter,
uint8_t bdaddr_type);
int btd_adapter_remove_bonding(struct btd_adapter *adapter,
- const bdaddr_t *bdaddr, uint8_t bdaddr_type);
+ const bdaddr_t *bdaddr, uint8_t bdaddr_type,
+ uint8_t disconnect);
+int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
+ const bdaddr_t *bdaddr,
+ uint8_t bdaddr_type,
+ uint8_t io_cap);
int btd_adapter_pincode_reply(struct btd_adapter *adapter,
const bdaddr_t *bdaddr,
diff --git a/src/device.c b/src/device.c
index 875a5c5..4aab349 100644
--- a/src/device.c
+++ b/src/device.c
@@ -158,6 +158,7 @@ struct att_callbacks {
struct bearer_state {
bool paired;
bool bonded;
+ bool rebonding;
bool connected;
bool svc_resolved;
};
@@ -221,6 +222,8 @@ struct btd_device {
int8_t rssi;
GIOChannel *att_io;
+ GIOChannel *att_rebond_io;
+
guint cleanup_id;
guint store_id;
};
@@ -522,6 +525,9 @@ static void device_free(gpointer user_data)
attio_cleanup(device);
+ if (device->att_rebond_io)
+ g_io_channel_unref(device->att_rebond_io);
+
if (device->tmp_records)
sdp_list_free(device->tmp_records,
(sdp_free_func_t) sdp_record_free);
@@ -1739,6 +1745,23 @@ long device_bonding_last_duration(struct btd_device *device)
return bonding->last_attempt_duration_ms;
}
+bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type)
+{
+ struct bearer_state *state = get_state(device, bdaddr_type);
+
+ return state->rebonding;
+}
+
+void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
+ bool rebonding)
+{
+ struct bearer_state *state = get_state(device, bdaddr_type);
+
+ state->rebonding = rebonding;
+
+ DBG("rebonding = %d", rebonding);
+}
+
static void create_bond_req_exit(DBusConnection *conn, void *user_data)
{
struct btd_device *device = user_data;
@@ -2029,7 +2052,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
if (state->paired && !state->bonded)
btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
- bdaddr_type);
+ bdaddr_type, 1);
if (device->bredr_state.connected || device->le_state.connected)
return;
@@ -2639,13 +2662,13 @@ static void device_remove_stored(struct btd_device *device)
if (device->bredr_state.bonded) {
device->bredr_state.bonded = false;
btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
- BDADDR_BREDR);
+ BDADDR_BREDR, 1);
}
if (device->le_state.bonded) {
device->le_state.bonded = false;
btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
- device->bdaddr_type);
+ device->bdaddr_type, 1);
}
device->bredr_state.paired = false;
@@ -3628,6 +3651,18 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
GAttrib *attrib;
BtIOSecLevel sec_level;
+ DBG("");
+
+ if (dev->le_state.rebonding) {
+ DBG("postponing due to rebonding");
+ /* Keep the att socket, and defer attaching the attributes
+ * until rebonding is done.
+ */
+ if (!dev->att_rebond_io)
+ dev->att_rebond_io = g_io_channel_ref(io);
+ return false;
+ }
+
bt_io_get(io, &gerr, BT_IO_OPT_SEC_LEVEL, &sec_level,
BT_IO_OPT_INVALID);
if (gerr) {
@@ -4269,6 +4304,23 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
}
}
+bool device_rebonding_complete(struct btd_device *device, uint8_t bdaddr_type)
+{
+ bool ret = true;
+
+ DBG("");
+
+ device_set_rebonding(device, bdaddr_type, false);
+
+ if (bdaddr_type != BDADDR_BREDR && device->att_rebond_io) {
+ ret = device_attach_attrib(device, device->att_rebond_io);
+ g_io_channel_unref(device->att_rebond_io);
+ device->att_rebond_io = NULL;
+ }
+
+ return ret;
+}
+
static gboolean svc_idle_cb(gpointer user_data)
{
struct svc_callback *cb = user_data;
diff --git a/src/device.h b/src/device.h
index 2e0473e..65b1018 100644
--- a/src/device.h
+++ b/src/device.h
@@ -94,6 +94,10 @@ bool device_is_retrying(struct btd_device *device);
void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
uint8_t status);
gboolean device_is_bonding(struct btd_device *device, const char *sender);
+bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type);
+void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
+ bool rebonding);
+bool device_complete_rebonding(struct btd_device *device, uint8_t bdaddr_type);
void device_bonding_attempt_failed(struct btd_device *device, uint8_t status);
void device_bonding_failed(struct btd_device *device, uint8_t status);
struct btd_adapter_pin_cb_iter *device_bonding_iter(struct btd_device *device);
--
1.9.1
^ permalink raw reply related
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