* [PATCH v2 0/2] core: Add plugin-support for Manufacturer Specific Data EIR
From: Alfonso Acosta @ 2014-10-10 15:23 UTC (permalink / raw)
To: linux-bluetooth
Athough the Manufacturer Specific Data (MSD) field is not used
internally, it's useful for external plugins, e.g. iBeacon.
For instance we are dealing with a device which, when losing its LTK
on a reset, notifies the master about the need of rebonding by broadcasting
specific content in the MSD field of ADV_IND reports.
The first patch simply adds the field and parsing support.
The second one adds a subscription mechanism for plugins so that they can
be notified about new Manufacturer Specific Data being received.
Alfonso Acosta (2):
core: Add Manufacturer Specific Data EIR field
core: Add subscription API for Manufacturer Specific Data
plugins/neard.c | 1 -
src/adapter.c | 38 +++++++++++++++++++++++++++++++++++++-
src/adapter.h | 10 ++++++++++
src/eir.c | 11 +++++++++++
src/eir.h | 8 ++++++++
5 files changed, 66 insertions(+), 2 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH v2 1/2] core: Add Manufacturer Specific Data EIR field
From: Alfonso Acosta @ 2014-10-10 15:23 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1412954626-30226-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..4a92efd 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)
+ 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 v2 2/2] core: Add subscription API for Manufacturer Specific Data
From: Alfonso Acosta @ 2014-10-10 15:23 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1412954626-30226-1-git-send-email-fons@spotify.com>
This patch allows plugins to be notified whenever an adapter receives
Manufacturer Specific Data inside from a device.
This can happen when new device is discovered or when we autoconnect
to it.
---
plugins/neard.c | 1 -
src/adapter.c | 38 +++++++++++++++++++++++++++++++++++++-
src/adapter.h | 10 ++++++++++
3 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/plugins/neard.c b/plugins/neard.c
index 137d601..a975417 100644
--- a/plugins/neard.c
+++ b/plugins/neard.c
@@ -38,7 +38,6 @@
#include "src/dbus-common.h"
#include "src/adapter.h"
#include "src/device.h"
-#include "src/eir.h"
#include "src/agent.h"
#include "src/hcid.h"
diff --git a/src/adapter.c b/src/adapter.c
index 92ee1a0..8302cb4 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -68,7 +68,6 @@
#include "attrib/att.h"
#include "attrib/gatt.h"
#include "attrib-server.h"
-#include "eir.h"
#define ADAPTER_INTERFACE "org.bluez.Adapter1"
@@ -206,6 +205,7 @@ struct btd_adapter {
gboolean initialized;
GSList *pin_callbacks;
+ GSList *msd_callbacks;
GSList *drivers;
GSList *profiles;
@@ -4549,6 +4549,10 @@ 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);
+ }
+}
+
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..e72af89 100644
--- 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);
+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
* Re: [PATCH] core: Add Manufacturer Specific Data EIR field
From: Alfonso Acosta @ 2014-10-10 15:27 UTC (permalink / raw)
To: BlueZ development
In-Reply-To: <1412941053-11835-1-git-send-email-fons@spotify.com>
Please ignore this first patch. I sent a new bundle "[PATCH v2 0/2] core:
Add plugin-support for Manufacturer Specific Data EIR" with more
context and an extra patch for subscribing to MSD from plugins.
On Fri, Oct 10, 2014 at 1:37 PM, Alfonso Acosta <fons@spotify.com> wrote:
> Athough the Manufacturer Specific Data field is not used internally,
> it's useful for external plugins, e.g. iBeacon.
> ---
> src/eir.c | 8 ++++++++
> src/eir.h | 4 ++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/src/eir.c b/src/eir.c
> index d22ad91..f130855 100644
> --- a/src/eir.c
> +++ b/src/eir.c
> @@ -240,6 +240,14 @@ 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)
> + break;
> + 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..88a5bf0 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 */
> @@ -62,6 +63,9 @@ struct eir_data {
> uint16_t did_product;
> uint16_t did_version;
> uint16_t did_source;
> + uint16_t msd_company;
> + uint8_t msd_data[HCI_MAX_EIR_LENGTH];
> + uint8_t msd_data_len;
> };
>
> void eir_data_free(struct eir_data *eir);
> --
> 1.9.1
>
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting
From: Alfonso Acosta @ 2014-10-10 15:40 UTC (permalink / raw)
To: Marcel Holtmann, johan.hedberg; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4p3Rn=Q3pdW3ML7=H00pBcGUSjr3SfAsm6v+Da8_LPcCQ@mail.gmail.com>
Hi Jhoan and Marcel,
I sent v2 which I hope addresses all your remarks.
Thanks,
Fons
On Fri, Oct 10, 2014 at 10:14 AM, Alfonso Acosta <fons@spotify.com> wrote:
> Hi Marcel,
>
>
>> don't we also need to clear the flag when the new pairing succeed?
>
> Yep, in fact that's the whole point of the flag .... which I missed.
> Thanks for your patience :)
>
>> If we destroy hci_conn anyway, there is pretty much no point in test_and_clear_bit and we could just use test_bit here.
>
> Fair point, I will change them to test_bit.
>
>
>
>
> --
> Alfonso Acosta
>
> Embedded Systems Engineer at Spotify
> Birger Jarlsgatan 61, Stockholm, Sweden
> http://www.spotify.com
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: Bluez dual-mode support
From: Marcel Holtmann @ 2014-10-10 15:40 UTC (permalink / raw)
To: Boulais, Marc-Andre; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <29A2697B0516A946B1023D5E798DFCF65B5C257D@mail-yang>
Hi Marc,
> Does BlueZ support a "dual mode" where both Bluetooth "classic" and "low-energy" are enabled? I need to support classic and LE Bluetooth devices in my network.
>
> Furthermore, is there an recent list of Bluetooth chipsets supported by BlueZ for Low Energy? any chipset recommendations would be greatly appreciated.
yes, BlueZ 5.x fully supports dual-mode operation. Almost all recent chipsets should be supported. So any USB based Bluetooth controller should actually work out of the box.
Regards
Marcel
^ permalink raw reply
* Re: [RFC] need for new scan api for bluetooth low energy.
From: Jakub Pawlowski @ 2014-10-10 15:49 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Arman Uguray, BlueZ development, Scott James Remnant
In-Reply-To: <8A0217F1-C062-466F-8C04-38E324BDD55D@holtmann.org>
Thanks for info!
I'll look into that.
On Fri, Oct 10, 2014 at 3:52 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Jakub,
>
>>>>> Ok, new updated version - we do only passive scan, and we send Device
>>>>> found event:
>>>>>
>>>>>
>>>>> diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
>>>>> index 11e2490..3d03617 100644
>>>>> --- a/doc/mgmt-api.txt
>>>>> +++ b/doc/mgmt-api.txt
>>>>> @@ -2135,6 +2135,29 @@ Set Public Address Command
>>>>> Invalid Index
>>>>>
>>>>>
>>>>> +Start Passive LE Filtered Device Scan
>>>>> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D
>>>>> +
>>>>> + Command Code: 0x003A
>>>>> + Controller Index: <controller id>
>>>>> + Command Parameters: UUID (16 Octets)
>>>>> + max_pathloss (1 octet)
>>>>> +
>>>>> + Return Parameters:
>>>>> +
>>>>> + This command starts passive LE scan, and filter received
>>>>> advertisements: if AD contains both TX power level and Service
>>>>> Solicitation, and UUID is contained in Service Solicitation and
>>>>> computed pathloss is smaller than max_pathloss parameter, then a
>>>>> Device Found event will be sent.
>>>>
>>>> generally we tried to avoid to specific LE only commands. Our attempts=
with the kernel APIs where to make them as generic as possible. So I think=
something along the lines of adding and removing UUID filters should be so=
mething we should target at.
>>>>
>>>> Add UUID Notification Filter
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
>>>>
>>>> Command Parameters: Address_Type (1 Octet)
>>>> UUID (16 Octets)
>>>> Max_Pathloss (1 Octet)
>>>> Return Parameters: Address_Type (1 Octet)
>>>>
>>>>
>>>> Remove UUID Notification Filter
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D
>>>>
>>>> Command Parameters: Address_Type (1 Octet)
>>>> UUID (16 Octets)
>>>> Return Parameters: Address_Type (1 Octet)
>>>>
>>>> The Address_Type would be defined the same way as for Start Discovery =
command.
>>>>
>>>> Now we can discuss if on the mgmt level we want to use pathloss or rat=
her a range of RSSI. I have seen that RSSI is preferred and that the transl=
ation from RSSI and advertising data with TX power level is done in the dae=
mon. So for kernel API it might be better to expose RSSI range.
>>>>
>>>> Besides a RSSI range, we might also want to allow giving a RSSI thresh=
old that defines how much the RSSI to change between events to be reported =
again.
>>>
>>> Some mobile devices that I'm working can change power level for
>>> advertising, that makes raw RSSI filter useless. It must be pathloss,
>>> that's why I require TX power in advertisement.
>>
>> you will still get it, but just have to do the math in the daemon. The o=
nly difference is that the daemon gets woken up for RSSI ranges that in the=
end you might filter out.
>>
>>>>
>>>> That said, the danger with a generic notification filter like this is =
that we can not implement it efficiently with current hardware without vend=
or commands. If you use duplicate filtering, then Broadcom and CSR chips be=
have fully different. Broadcom only filters on BD_ADDR and report devices o=
nce no matter what the RSSI, while CSR will report the same device again if=
the RSSI changes.
>>>>
>>>> With that in mind, it might be safer to introduce a one-shot mgmt API =
that needs to be repeatedly called to keep scanning for new devices.
>>>>
>>>> Start Service Discovery
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>>
>>>> Command Parameters: Address_Type (1 Octet)
>>>> Min_RSSI (1 Octet)
>>>> Max_RSSI (1 Octet)
>>>> Num_UUID (1 Octet)
>>>> UUID[i] (16 Octets)
>>>> Return Parameters: Address_Type (1 Octet)
>>>>
>>>> Stop Service Discovery
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>>
>>>> Command Parameters: Address_Type (1 Octet)
>>>> Return Parameters: Address_Type (1 Octet)
>>>>
>>>> Without having dedicated controller support for filtering, having such=
a dedicated discovery for services with a specific UUID seems to be more a=
ppropriate and more safe. Since we could always add controller based filter=
ing for background scanning once there is controller support.
>>>
>>> I preffer the "Add UUID Notification Filter" and "Remove UUID
>>> Notification Filter". For controllers like Broadcom that reports only
>>> one advertisement can we restart scan internally in kernel ? Why
>>> propagate that event higher ?
>>
>> Personally I would prefer just adding another filter as well. However it=
might be better to start with something like a one-shot handling and see w=
here that is taking us. It is a little bit less intrusive and you could als=
o use active scanning. At the same time you could also easily make it work =
for BR/EDR. I am thinking here along the lines of something wanting to pair=
a mouse. You just give the HID UUID and start interleaved discovery for th=
at service on both transports.
>
> I send a proposal for doing an one-shot service discovery. So you can pro=
pose a RSSI Threshold and a list of possible UUIDs to look for.
>
> If RSSI Threshold is 127 or the Num_UUID is 0, then it will behave the sa=
me way as Start Discovery. I think this is how we should get started to tac=
kle this problem.
>
> Regards
>
> Marcel
>
^ permalink raw reply
* Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting
From: Marcel Holtmann @ 2014-10-10 15:57 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4oTZjk6k0YeBA6xEpksZRWU1TTtkZfB+YESWmmRxNs0GA@mail.gmail.com>
Hi Alfonso,
> As an alternative to this patch, I am thinking that it may be worth
> considering two other options:
>
> 1. Adding an additional repairing operation: MGMT_OP_REPAIR_DEVICE.
> Making the repairing semantics explicit would allow us to keep the connection
> parameters even if we choose to disconnect when unpairing. On the
> other hand, one could argue that it clutters the API with an extra
> operation which is the composite of two already existing operations.
>
> 2. Make MGMT_OP_PAIR_DEVICE behave like the repairing operation in (1)
> if the device was already paired. If I recall properly, Johan suggested
> this on IRC.
I am not in favor of making Pair Device just do re-pairing. I think that is a dangerous behavior since want bondings in the kernel only in two ways. Either they are loaded via Load Long Term Keys or via Pair Device. Doing something magic is something that I consider dangerous and can lead into some flaws. If we come with a clear error than we at least know that something went wrong.
Repairing command is possible, but I am not 100% convinced that it is a good idea. The normal workflow is that you pair and unpair a device. If you get stuck in a weird state, you unpair first which ensures that everything is cleaned out and then pair again. For the connection parameters we can just be smarter.
I bet there is still a lot of improvement for our connection parameter handling. For example we could just keep all connection parameters around in cache and just expire them after 1 day or so. That would improve general handling with devices with do not pair in the first. So I would focus on being smart when dropping connection parameters and not trying to bind it too much to mgmt API visible states.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Alexander Aring @ 2014-10-10 19:41 UTC (permalink / raw)
To: Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen,
Martin Townsend, werner
In-Reply-To: <1412840794-17283-2-git-send-email-martin.townsend@xsilon.com>
Hi Martin,
I reconsider these steps which we do here now and saw some new
improvements/issues.
On Thu, Oct 09, 2014 at 08:46:34AM +0100, Martin Townsend wrote:
> Currently there are potentially 2 skb_copy_expand calls in IPHC
> decompression. This patch replaces this with one call to
> pskb_expand_head. It also checks to see if there is enough headroom
> first to ensure it's only done if necessary.
> As pskb_expand_head must only have one reference the calling code
> now ensures this.
>
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
> net/bluetooth/6lowpan.c | 7 +++++++
> 2 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 142eef5..853b4b8 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
> static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
> struct net_device *dev, skb_delivery_cb deliver_skb)
> {
> - struct sk_buff *new;
> int stat;
>
> - new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
> - GFP_ATOMIC);
> - kfree_skb(skb);
> -
> - if (!new)
> - return -ENOMEM;
> -
> - skb_push(new, sizeof(struct ipv6hdr));
> - skb_reset_network_header(new);
> - skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
> + skb_push(skb, sizeof(struct ipv6hdr));
> + skb_reset_network_header(skb);
> + skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
>
> - new->protocol = htons(ETH_P_IPV6);
> - new->pkt_type = PACKET_HOST;
> - new->dev = dev;
> + skb->protocol = htons(ETH_P_IPV6);
> + skb->pkt_type = PACKET_HOST;
> + skb->dev = dev;
>
> raw_dump_table(__func__, "raw skb data dump before receiving",
> - new->data, new->len);
> + skb->data, skb->len);
>
> - stat = deliver_skb(new, dev);
> + stat = deliver_skb(skb, dev);
>
> - kfree_skb(new);
> + consume_skb(skb);
>
> return stat;
> }
> @@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> /* UDP data uncompression */
> if (iphc0 & LOWPAN_IPHC_NH_C) {
> struct udphdr uh;
> - struct sk_buff *new;
> + const int needed = sizeof(struct udphdr) + sizeof(hdr);
>
> if (uncompress_udp_header(skb, &uh))
> goto drop;
> @@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> /* replace the compressed UDP head by the uncompressed UDP
> * header
> */
> - new = skb_copy_expand(skb, sizeof(struct udphdr),
> - skb_tailroom(skb), GFP_ATOMIC);
> - kfree_skb(skb);
> -
> - if (!new)
> - return -ENOMEM;
> -
> - skb = new;
> + if (skb_headroom(skb) < needed) {
> + err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
> + if (unlikely(err)) {
> + kfree_skb(skb);
> + return err;
> + }
> + }
>
> skb_push(skb, sizeof(struct udphdr));
> skb_reset_transport_header(skb);
> @@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> (u8 *)&uh, sizeof(uh));
>
> hdr.nexthdr = UIP_PROTO_UDP;
> + } else {
> + if (skb_headroom(skb) < sizeof(hdr)) {
> + err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
> + if (unlikely(err)) {
> + kfree_skb(skb);
> + return err;
> + }
> + }
What we here try to do is a usual sk_buff principle.
There exist a function skb_cow [0]:
<qoute>
static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom,
int cloned)
{
int delta = 0;
if (headroom > skb_headroom(skb))
delta = headroom - skb_headroom(skb);
if (delta || cloned)
return pskb_expand_head(skb, ALIGN(delta, NET_SKB_PAD), 0,
GFP_ATOMIC);
return 0;
}
</qoute>
This is the wrapper call which is called by skb_cow. I see here much
similarity and also more performance. It calculates a delta size and do
also a NET_SKB_PAD which is a align to the cache_lines and I think they
try do some "false sharing" here. [1]
We should use this function, if you agree here.
There exist also some skb_cow_head function [2].
code commentar:
<qoute>
It should be used when you only need to push on some header and do not
need to modify the data.
</qoute>
I am not sure about if we can use skb_cow_head here, because we modify
the data. I mean we run skb_pull and then skb_cow_head and run skb_push
with different data buffer. (IPv6 header).
Currently I am not sure if this works.
> }
>
> hdr.payload_len = htons(skb->len);
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index c2e0d14..6643a7c 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> kfree_skb(local_skb);
> kfree_skb(skb);
> } else {
> + /* Decompression may use pskb_expand_head so no shared skb's */
> + skb = skb_share_check(skb, GFP_ATOMIC);
> + if (!skb) {
> + dev->stats.rx_dropped++;
> + return NET_RX_DROP;
> + }
> +
> switch (skb->data[0] & 0xe0) {
> case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
> local_skb = skb_clone(skb, GFP_ATOMIC);
Now we come to this function.
There exist two global rules here:
1.
If we change only the skb pointers head/tail with e.g. skb_push then we
need a cloned skb only. We don't change the skb data buffer here.
We need this in LOWPAN_DISPATCH_IPV6, here we run only one skb_pull for
one byte, and the FRAGN and FRAG1 dispatches in 802.15.4 6LoWPAN.
skb_share_check do this, we have surely a clone afterwards and the
sk_buff attributes are not changed elsewhere, which means we are
allowed to manipulate the skb attributes.
2.
If we touch the data buffer, then we need a skb_copy. We need this at
LOWPAN_DISPATCH_IPHC because here we replacing the data buffer.
skb_unshare do this for us.
If we can use skb_cow_head like above, I think then we don't need a
skb_unshare here. But I am not 100% sure if we really can use
skb_cow_head here. We don't need skb_unshare then because, skb_cow_head
makes the head writeable only, this should something similar like a
private copy of headroom then.
Summary:
We need always a skb_share_check at begin of evaluate the dispatch, for
the LOWPAN_DISPATCH_IPHC we need a skb_unshare before. That's fine for
now, maybe later we can do some tricks with skb_cow_head.
skb_share_check:
We always have a cloned skb and we can manipulate sk_buff attributes.
skb_unshare:
We have a private copy of data buffer and can replace 6LoWPAN header
with IPv6 header.
Do you agree here?
- Alex
[0] http://lxr.free-electrons.com/source/include/linux/skbuff.h#L2316
[1] http://en.wikipedia.org/wiki/False_sharing
[2] http://lxr.free-electrons.com/source/include/linux/skbuff.h#L2331
^ permalink raw reply
* [PATCH] core: Add btd_adapter_recreate_bonding()
From: Alfonso Acosta @ 2014-10-10 23:53 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
* Re: [PATCH] core: Add btd_adapter_recreate_bonding()
From: Alfonso Acosta @ 2014-10-10 23:56 UTC (permalink / raw)
To: BlueZ development
In-Reply-To: <1412985213-5447-1-git-send-email-fons@spotify.com>
Please note that this patch depends on a previously submitted kernel
patch [1] to ensure that connection parameters are not lost when
rebonding.
[1] [PATCH] Bluetooth: Defer connection-parameter removal when
unpairing without disconnecting
On Sat, Oct 11, 2014 at 1:53 AM, 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] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting
From: Alfonso Acosta @ 2014-10-11 0:14 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <872F1D49-E810-4D33-9B05-ED3DDBA64336@holtmann.org>
Hi Marcel,
> I am not in favor of making Pair Device just do re-pairing. I think that =
is a dangerous behavior since want bondings in the kernel only in two ways.=
Either they are loaded via Load Long Term Keys or via Pair Device. Doing s=
omething magic is something that I consider dangerous and can lead into som=
e flaws. If we come with a clear error than we at least know that something=
went wrong.
>
> Repairing command is possible, but I am not 100% convinced that it is a g=
ood idea. The normal workflow is that you pair and unpair a device. If you =
get stuck in a weird state, you unpair first which ensures that everything =
is cleaned out and then pair again. For the connection parameters we can ju=
st be smarter.
Fair enough. Does this mean that the current patch is still of
interest? I hope so :)
> I bet there is still a lot of improvement for our connection parameter ha=
ndling. For example we could just keep all connection parameters around in =
cache and just expire them after 1 day or so. That would improve general ha=
ndling with devices with do not pair in the first. So I would focus on bein=
g smart when dropping connection parameters and not trying to bind it too m=
uch to mgmt API visible states.
I actually have a patch lying around which keeps track of
conn_parameters within bluetoothd. I think that it would be a good
start (I would like to complete the submissions I sent this week
getting to it though).
--=20
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 1:33 UTC (permalink / raw)
To: linux-bluetooth
Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.
This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.
Signed-off-by: Alfonso Acosta <fons@spotify.com>
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
HCI_CONN_STK_ENCRYPT,
HCI_CONN_AUTH_INITIATOR,
HCI_CONN_DROP,
+ HCI_CONN_PARAM_REMOVAL_PEND,
};
static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..eb9988f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
conn->state = BT_CLOSED;
break;
}
+
+ if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
}
/* Enter sniff mode */
@@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
hci_conn_del_sysfs(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
hci_dev_put(hdev);
hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..0af579d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2699,7 +2699,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
struct mgmt_rp_unpair_device rp;
struct hci_cp_disconnect dc;
struct pending_cmd *cmd;
- struct hci_conn *conn;
+ struct hci_conn *uninitialized_var(conn);
int err;
memset(&rp, 0, sizeof(rp));
@@ -2736,7 +2736,17 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
- hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+ &cp->addr.bdaddr);
+
+ /* If the BLE connection is being used, defer clearing up
+ * the connection parameters until closing to give a
+ * chance of keeping them if a repairing happens.
+ */
+ if (conn && !cp->disconnect)
+ set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
+ else
+ hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
@@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (cp->disconnect) {
+ /* Only lookup the connection in the BR/EDR since the
+ * LE connection was already looked up earlier.
+ */
if (cp->addr.type == BDADDR_BREDR)
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
&cp->addr.bdaddr);
- else
- conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
- &cp->addr.bdaddr);
} else {
conn = NULL;
}
@@ -3062,6 +3072,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
hci_conn_put(conn);
mgmt_pending_remove(cmd);
+
+ /* The device is paired so there is no need to remove
+ * its connection parameters anymore.
+ */
+ clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
}
void mgmt_smp_complete(struct hci_conn *conn, bool complete)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 1:37 UTC (permalink / raw)
To: BlueZ development
In-Reply-To: <1412991237-20847-1-git-send-email-fons@spotify.com>
I shortened the commit message header and moved the
clear_bit(HCI_CONN_PARAM_REMOVAL_PEND) to cover all the pairing
completion cases.
On Sat, Oct 11, 2014 at 3:33 AM, Alfonso Acosta <fons@spotify.com> wrote:
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> HCI_CONN_STK_ENCRYPT,
> HCI_CONN_AUTH_INITIATOR,
> HCI_CONN_DROP,
> + HCI_CONN_PARAM_REMOVAL_PEND,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..eb9988f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
> conn->state = BT_CLOSED;
> break;
> }
> +
> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> }
>
> /* Enter sniff mode */
> @@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
>
> hci_conn_del_sysfs(conn);
>
> + if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> hci_dev_put(hdev);
>
> hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..0af579d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2699,7 +2699,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> struct mgmt_rp_unpair_device rp;
> struct hci_cp_disconnect dc;
> struct pending_cmd *cmd;
> - struct hci_conn *conn;
> + struct hci_conn *uninitialized_var(conn);
> int err;
>
> memset(&rp, 0, sizeof(rp));
> @@ -2736,7 +2736,17 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> - hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> + &cp->addr.bdaddr);
> +
> + /* If the BLE connection is being used, defer clearing up
> + * the connection parameters until closing to give a
> + * chance of keeping them if a repairing happens.
> + */
> + if (conn && !cp->disconnect)
> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> + else
> + hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
>
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
> @@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> if (cp->disconnect) {
> + /* Only lookup the connection in the BR/EDR since the
> + * LE connection was already looked up earlier.
> + */
> if (cp->addr.type == BDADDR_BREDR)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> - else
> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> - &cp->addr.bdaddr);
> } else {
> conn = NULL;
> }
> @@ -3062,6 +3072,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
> hci_conn_put(conn);
>
> mgmt_pending_remove(cmd);
> +
> + /* The device is paired so there is no need to remove
> + * its connection parameters anymore.
> + */
> + clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> }
>
> void mgmt_smp_complete(struct hci_conn *conn, bool complete)
> --
> 1.9.1
>
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Alexander Aring @ 2014-10-11 6:55 UTC (permalink / raw)
To: Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen,
Martin Townsend, werner
In-Reply-To: <20141010194116.GA31843@omega>
Hi Martin,
On Fri, Oct 10, 2014 at 09:41:16PM +0200, Alexander Aring wrote:
...
> > hdr.payload_len = htons(skb->len);
> > diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> > index c2e0d14..6643a7c 100644
> > --- a/net/bluetooth/6lowpan.c
> > +++ b/net/bluetooth/6lowpan.c
> > @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> > kfree_skb(local_skb);
> > kfree_skb(skb);
> > } else {
> > + /* Decompression may use pskb_expand_head so no shared skb's */
> > + skb = skb_share_check(skb, GFP_ATOMIC);
> > + if (!skb) {
> > + dev->stats.rx_dropped++;
> > + return NET_RX_DROP;
> > + }
> > +
> > switch (skb->data[0] & 0xe0) {
> > case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
> > local_skb = skb_clone(skb, GFP_ATOMIC);
>
>
> Now we come to this function.
>
> There exist two global rules here:
>
> 1.
>
> If we change only the skb pointers head/tail with e.g. skb_push then we
> need a cloned skb only. We don't change the skb data buffer here.
>
> We need this in LOWPAN_DISPATCH_IPV6, here we run only one skb_pull for
> one byte, and the FRAGN and FRAG1 dispatches in 802.15.4 6LoWPAN.
>
> skb_share_check do this, we have surely a clone afterwards and the
> sk_buff attributes are not changed elsewhere, which means we are
> allowed to manipulate the skb attributes.
>
> 2.
>
> If we touch the data buffer, then we need a skb_copy. We need this at
> LOWPAN_DISPATCH_IPHC because here we replacing the data buffer.
>
> skb_unshare do this for us.
>
This is not correct. I mean the chain of thought is correct. But a
private copy of data buffer is already done by pskb_expand_head, which
is used before modify the data buffer. If we should not run it, then it
would be correct. This handling is done by decompression IPHC.
So we don't need skb_unshare. Now it's very interesting if we can use
skb_cow_head, when replacing transport/network header, because we only
need to modify the headroom. Otherwise we should use skb_cow.
- Alex
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Alexander Aring @ 2014-10-11 7:36 UTC (permalink / raw)
To: Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen,
Martin Townsend
In-Reply-To: <1412840794-17283-2-git-send-email-martin.townsend@xsilon.com>
Hi Martin,
This is only a summarize what we should now to try to do here for next
version.
I know... this patch ends in a global debate on principles how we deal
with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
more a hack.
On Thu, Oct 09, 2014 at 08:46:34AM +0100, Martin Townsend wrote:
> Currently there are potentially 2 skb_copy_expand calls in IPHC
> decompression. This patch replaces this with one call to
> pskb_expand_head. It also checks to see if there is enough headroom
> first to ensure it's only done if necessary.
> As pskb_expand_head must only have one reference the calling code
> now ensures this.
>
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
> net/bluetooth/6lowpan.c | 7 +++++++
> 2 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 142eef5..853b4b8 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
> static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
> struct net_device *dev, skb_delivery_cb deliver_skb)
> {
> - struct sk_buff *new;
> int stat;
>
> - new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
> - GFP_ATOMIC);
> - kfree_skb(skb);
> -
> - if (!new)
> - return -ENOMEM;
> -
> - skb_push(new, sizeof(struct ipv6hdr));
> - skb_reset_network_header(new);
> - skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
> + skb_push(skb, sizeof(struct ipv6hdr));
> + skb_reset_network_header(skb);
> + skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
>
> - new->protocol = htons(ETH_P_IPV6);
> - new->pkt_type = PACKET_HOST;
> - new->dev = dev;
> + skb->protocol = htons(ETH_P_IPV6);
> + skb->pkt_type = PACKET_HOST;
> + skb->dev = dev;
>
> raw_dump_table(__func__, "raw skb data dump before receiving",
> - new->data, new->len);
> + skb->data, skb->len);
>
> - stat = deliver_skb(new, dev);
> + stat = deliver_skb(skb, dev);
>
> - kfree_skb(new);
> + consume_skb(skb);
>
> return stat;
> }
> @@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> /* UDP data uncompression */
> if (iphc0 & LOWPAN_IPHC_NH_C) {
> struct udphdr uh;
> - struct sk_buff *new;
> + const int needed = sizeof(struct udphdr) + sizeof(hdr);
>
> if (uncompress_udp_header(skb, &uh))
> goto drop;
> @@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> /* replace the compressed UDP head by the uncompressed UDP
> * header
> */
> - new = skb_copy_expand(skb, sizeof(struct udphdr),
> - skb_tailroom(skb), GFP_ATOMIC);
> - kfree_skb(skb);
> -
> - if (!new)
> - return -ENOMEM;
> -
> - skb = new;
> + if (skb_headroom(skb) < needed) {
> + err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
> + if (unlikely(err)) {
> + kfree_skb(skb);
> + return err;
> + }
> + }
use skb_cow_head here.
>
> skb_push(skb, sizeof(struct udphdr));
> skb_reset_transport_header(skb);
> @@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> (u8 *)&uh, sizeof(uh));
>
> hdr.nexthdr = UIP_PROTO_UDP;
> + } else {
> + if (skb_headroom(skb) < sizeof(hdr)) {
> + err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
> + if (unlikely(err)) {
> + kfree_skb(skb);
> + return err;
> + }
> + }
same here.
> }
>
> hdr.payload_len = htons(skb->len);
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index c2e0d14..6643a7c 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> kfree_skb(local_skb);
> kfree_skb(skb);
> } else {
> + /* Decompression may use pskb_expand_head so no shared skb's */
> + skb = skb_share_check(skb, GFP_ATOMIC);
> + if (!skb) {
> + dev->stats.rx_dropped++;
> + return NET_RX_DROP;
> + }
> +
I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
is the share_check really needed? In my opinion there are several things
wrong.
What bluetooth should do here is:
Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
- only run a skb_pull (doesn't change the data buffer, only skb attribute)
for the one byte dispatch.
- run other things like skb_reset_network_header, etc...
-> Currently this works because we run a complete skb_copy_expand here.
But this is not needed, we need a clone because we don't modify the
data buffer.
-> To Jukka: I don't see a skb_pull for the one byte dispatch value?
What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
for this here.
In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
this is already handled by "skb_share_check" then.
- Alex
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 8:32 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1412991237-20847-1-git-send-email-fons@spotify.com>
Hi Alfonso,
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> HCI_CONN_STK_ENCRYPT,
> HCI_CONN_AUTH_INITIATOR,
> HCI_CONN_DROP,
> + HCI_CONN_PARAM_REMOVAL_PEND,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..eb9988f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
> conn->state = BT_CLOSED;
> break;
> }
> +
> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
do we really need this one? The hci_conn_timeout should trigger when the connection establishment has a timeout and that is really non of the concerns here. And in case we hit an idle disconnect timeout, we are still ending up in hci_conn_del eventually, right? If not, I am having the slight feeling that you might have uncovered a bug that we should fix.
> }
>
> /* Enter sniff mode */
> @@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
>
> hci_conn_del_sysfs(conn);
>
> + if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> hci_dev_put(hdev);
>
> hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..0af579d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2699,7 +2699,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> struct mgmt_rp_unpair_device rp;
> struct hci_cp_disconnect dc;
> struct pending_cmd *cmd;
> - struct hci_conn *conn;
> + struct hci_conn *uninitialized_var(conn);
> int err;
>
> memset(&rp, 0, sizeof(rp));
> @@ -2736,7 +2736,17 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> - hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> + &cp->addr.bdaddr);
> +
> + /* If the BLE connection is being used, defer clearing up
> + * the connection parameters until closing to give a
> + * chance of keeping them if a repairing happens.
> + */
> + if (conn && !cp->disconnect)
> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> + else
> + hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
Coming to think about this a bit in detail, why are we making a difference on if we disconnect here or not. We could make the code a lot simpler. Just always set the PARAM_REMOVAL flag when unpairing and clear it when pairing. If we disconnect right away or not should not affect the usage of this flag.
As long as the connection is up, we can happily keep the flags around. Only when the connection goes away, we really need to ensure that we know if we want to remove them or not. Honestly, we should have done it that way in the first place and not spread hci_conn_params_del in different places. The central logic to delete or keep connection parameters should be run when hci_conn goes away.
>
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
> @@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> if (cp->disconnect) {
> + /* Only lookup the connection in the BR/EDR since the
> + * LE connection was already looked up earlier.
> + */
> if (cp->addr.type == BDADDR_BREDR)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> - else
> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> - &cp->addr.bdaddr);
> } else {
> conn = NULL;
> }
I know that Johan told you to do it this way, but seeing the patch now makes me a bit uneasy. The usage of uninitilized_var in this case can lead to errors in the future. The logic that the compiler is wrong here is not obvious. Which means a week from now all of us have forgotten why we did it.
So here is what I would do to make this easier.
if (cp->addr.type == BDADDR_BREDR) {
if (cp->disconnect) {
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
&cp->addr.bdaddr);
else
conn = NULL;
err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
} else {
conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
&cp->addr.bdaddr);
if (conn) {
set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
if (!cp->disconnect)
conn = NULL;
}
...
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
Add some minor comments on why we set conn = NULL and this should be a simpler block. Mainly because the difference between BR/EDR and LE are contained in a single spot and not spread out over two places.
This already takes into account my comment from above to just set the PARAM_REMOVAL flag no matter if we actually be asked to disconnect or not.
> @@ -3062,6 +3072,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
> hci_conn_put(conn);
>
> mgmt_pending_remove(cmd);
> +
> + /* The device is paired so there is no need to remove
> + * its connection parameters anymore.
> + */
> + clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> }
>
> void mgmt_smp_complete(struct hci_conn *conn, bool complete)
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting
From: Marcel Holtmann @ 2014-10-11 8:44 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4q2ii0jGLXuBVu1ZOOqmQQ+m+zCne-Jizzjr6UkfcgWXA@mail.gmail.com>
Hi Alfonso,
>> I am not in favor of making Pair Device just do re-pairing. I think that is a dangerous behavior since want bondings in the kernel only in two ways. Either they are loaded via Load Long Term Keys or via Pair Device. Doing something magic is something that I consider dangerous and can lead into some flaws. If we come with a clear error than we at least know that something went wrong.
>>
>> Repairing command is possible, but I am not 100% convinced that it is a good idea. The normal workflow is that you pair and unpair a device. If you get stuck in a weird state, you unpair first which ensures that everything is cleaned out and then pair again. For the connection parameters we can just be smarter.
>
> Fair enough. Does this mean that the current patch is still of
> interest? I hope so :)
the kernel side for delaying the removal of the connection parameters is the right thing to do. We should have done in the first place actually.
>> I bet there is still a lot of improvement for our connection parameter handling. For example we could just keep all connection parameters around in cache and just expire them after 1 day or so. That would improve general handling with devices with do not pair in the first. So I would focus on being smart when dropping connection parameters and not trying to bind it too much to mgmt API visible states.
>
> I actually have a patch lying around which keeps track of
> conn_parameters within bluetoothd. I think that it would be a good
> start (I would like to complete the submissions I sent this week
> getting to it though).
I am not in favor of that. bluetoothd is supposed to be stupid when it comes to these values. I mean stupid in a sense that when the kernel tells us to store them (conn params, IRK, LTK etc.), the daemon will store them. And on reboot just reload the whole thing back into the kernel. On unpairing/removing a device, the only other thing the daemon is suppose to be doing is clear all we know about that device. Meaning removing all stored keys and params. That is all it is suppose to be doing. Plain and simple.
This basic concept has gotten us pretty far and allowed us to put the kernel in control of handling the nasty details when it comes to pairing and connection handling.
But as I said before, we can be smarter inside the kernel. So the advertising data can actually contain proposed connection interval values from the peripheral. A peripheral can you tell the central its preferred values from the start. We are currently not parsing the advertising data and pre-filling the connection request with these values from the advertising report. That is something we could do. That will clearly improve our handling of peripherals that do not create a bond with us.
And for peripherals that do not keep a bond with us, we can actually just keep the connection parameters for x amount of time and then expire them like we expire the inquiry cache. That is exactly how the inquiry cache in BR/EDR works to speed up the connection creation. We will try to re-read the clock offset before terminating a connection so that when you do a re-connection right away, we use the right clock offset.
There are tons of improvements that we can do. And most of them can be done without changing any behavior in bluetoothd or changing the mgmt API.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 11:14 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <F4236E26-CB50-4334-9AD9-0D930B2B9955@holtmann.org>
Hi Marcel,
>> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *wor=
k)
>> conn->state =3D BT_CLOSED;
>> break;
>> }
>> +
>> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type=
);
>
> do we really need this one? The hci_conn_timeout should trigger when the =
connection establishment has a timeout and that is really non of the concer=
ns here. And in case we hit an idle disconnect timeout, we are still ending=
up in hci_conn_del eventually, right? If not, I am having the slight feeli=
ng that you might have uncovered a bug that we should fix.
It's probably just lack of familiarity with the code, but it wasn't
all that clear to me that hci_conn_del is called after
hci_conn_timeout kicks in. I removed it.
>
>>
>> err =3D hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
>> }
>> @@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct=
hci_dev *hdev, void *data,
>> }
>>
>> if (cp->disconnect) {
>> + /* Only lookup the connection in the BR/EDR since the
>> + * LE connection was already looked up earlier.
>> + */
>> if (cp->addr.type =3D=3D BDADDR_BREDR)
>> conn =3D hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>> &cp->addr.bdaddr);
>> - else
>> - conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK,
>> - &cp->addr.bdaddr);
>> } else {
>> conn =3D NULL;
>> }
>
> I know that Johan told you to do it this way, but seeing the patch now ma=
kes me a bit uneasy. The usage of uninitilized_var in this case can lead to=
errors in the future. The logic that the compiler is wrong here is not obv=
ious. Which means a week from now all of us have forgotten why we did it.
>
> So here is what I would do to make this easier.
>
> if (cp->addr.type =3D=3D BDADDR_BREDR) {
> if (cp->disconnect) {
> conn =3D hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> else
> conn =3D NULL;
>
> err =3D hci_remove_link_key(hdev, &cp->addr.bdaddr);
> } else {
> conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK,
> &cp->addr.bdaddr);
> if (conn) {
> set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags=
);
>
> if (!cp->disconnect)
> conn =3D NULL;
> }
>
> ...
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> err =3D hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type)=
;
> }
>
> Add some minor comments on why we set conn =3D NULL and this should be a =
simpler block. Mainly because the difference between BR/EDR and LE are cont=
ained in a single spot and not spread out over two places.
>
> This already takes into account my comment from above to just set the PAR=
AM_REMOVAL flag no matter if we actually be asked to disconnect or not.
Yep, I agree, what you propose is way cleaner. Thanks.
V4 takes care of your remarks.
--=20
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 11:14 UTC (permalink / raw)
To: linux-bluetooth
Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.
This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.
Signed-off-by: Alfonso Acosta <fons@spotify.com>
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
HCI_CONN_STK_ENCRYPT,
HCI_CONN_AUTH_INITIATOR,
HCI_CONN_DROP,
+ HCI_CONN_PARAM_REMOVAL_PEND,
};
static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..11aac06 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
hci_conn_del_sysfs(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
hci_dev_put(hdev);
hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..2911a5e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2725,10 +2725,29 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (cp->addr.type == BDADDR_BREDR) {
+ if (cp->disconnect)
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
+ &cp->addr.bdaddr);
+ else
+ conn = NULL; /* Avoid disconnecting later on*/
+
err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
} else {
u8 addr_type;
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+ &cp->addr.bdaddr);
+ if (conn) {
+ /* Defer clearing up the connection parameters
+ * until closing to give a chance of keeping
+ * them if a repairing happens.
+ */
+ set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
+
+ if (!cp->disconnect)
+ conn = NULL; /* Avoid disconnecting later on*/
+ }
+
if (cp->addr.type == BDADDR_LE_PUBLIC)
addr_type = ADDR_LE_DEV_PUBLIC;
else
@@ -2736,8 +2755,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
- hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
-
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
@@ -2747,17 +2764,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}
- if (cp->disconnect) {
- if (cp->addr.type == BDADDR_BREDR)
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
- &cp->addr.bdaddr);
- else
- conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
- &cp->addr.bdaddr);
- } else {
- conn = NULL;
- }
-
if (!conn) {
err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
&rp, sizeof(rp));
@@ -3062,6 +3068,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
hci_conn_put(conn);
mgmt_pending_remove(cmd);
+
+ /* The device is paired so there is no need to remove
+ * its connection parameters anymore.
+ */
+ clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
}
void mgmt_smp_complete(struct hci_conn *conn, bool complete)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 11:53 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1413026054-5913-1-git-send-email-fons@spotify.com>
Hi Alfonso,
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> HCI_CONN_STK_ENCRYPT,
> HCI_CONN_AUTH_INITIATOR,
> HCI_CONN_DROP,
> + HCI_CONN_PARAM_REMOVAL_PEND,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..11aac06 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
>
> hci_conn_del_sysfs(conn);
>
> + if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> hci_dev_put(hdev);
>
> hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..2911a5e 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2725,10 +2725,29 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> if (cp->addr.type == BDADDR_BREDR) {
> + if (cp->disconnect)
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> + &cp->addr.bdaddr);
> + else
> + conn = NULL; /* Avoid disconnecting later on*/
> +
I do not think this is a comment style that is valid in the network subsystem coding style and even if it is valid, it would be pretty unusual.
Generally I would do comment on the whole code. So something like this:
/* When disconnect of the the connection is requested,
* then look up the connection. If the remote device is
* connected, it will be later used to terminate the
* link.
*
* Setting it to NULL explicitly will cause no
* termination of the link.
*/
if (cp->disconnect)
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
&cp->addr.bdaddr);
else
conn = NULL;
> err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
> } else {
> u8 addr_type;
>
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> + &cp->addr.bdaddr);
> + if (conn) {
> + /* Defer clearing up the connection parameters
> + * until closing to give a chance of keeping
> + * them if a repairing happens.
> + */
> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> +
> + if (!cp->disconnect)
> + conn = NULL; /* Avoid disconnecting later on*/
And here I would do this:
/* If disconnection is not requested, then clear the
* connection variable so that that the link is not
* terminated.
*/
if (!cp->disconnect)
conn = NULL;
> + }
> +
> if (cp->addr.type == BDADDR_LE_PUBLIC)
> addr_type = ADDR_LE_DEV_PUBLIC;
> else
> @@ -2736,8 +2755,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> - hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> -
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
>
> @@ -2747,17 +2764,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> goto unlock;
> }
>
> - if (cp->disconnect) {
> - if (cp->addr.type == BDADDR_BREDR)
> - conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> - &cp->addr.bdaddr);
> - else
> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> - &cp->addr.bdaddr);
> - } else {
> - conn = NULL;
> - }
> -
And here I would just add a minor comment to remind us why this is correct:
/* If the connection variable is set, then termination of the
* link is requested.
*/
> if (!conn) {
> err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
> &rp, sizeof(rp));
> @@ -3062,6 +3068,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
> hci_conn_put(conn);
>
> mgmt_pending_remove(cmd);
> +
> + /* The device is paired so there is no need to remove
> + * its connection parameters anymore.
> + */
> + clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> }
>
> void mgmt_smp_complete(struct hci_conn *conn, bool complete)
>From a logic point of view the patch looks good to me. This is just style changes to make it easier to remember why things are done. Johan might should have a second look at it before applying.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 11:56 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4o7BE5hVzNgyXR0_jGPqNLnfjj1MgzuTU0iB-tvvgKa3A@mail.gmail.com>
Hi Alfonso,
>>> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
>>> conn->state = BT_CLOSED;
>>> break;
>>> }
>>> +
>>> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
>>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>>
>> do we really need this one? The hci_conn_timeout should trigger when the connection establishment has a timeout and that is really non of the concerns here. And in case we hit an idle disconnect timeout, we are still ending up in hci_conn_del eventually, right? If not, I am having the slight feeling that you might have uncovered a bug that we should fix.
>
> It's probably just lack of familiarity with the code, but it wasn't
> all that clear to me that hci_conn_del is called after
> hci_conn_timeout kicks in. I removed it.
there is always a possibility that you found an actual bug that we trigger, but has not done any harm. At least not harm that anybody has noticed so far. So if this happens, then I like to fix the actual bug.
Regards
Marcel
^ permalink raw reply
* [PATCH v5] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 14:27 UTC (permalink / raw)
To: linux-bluetooth
Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.
This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.
Signed-off-by: Alfonso Acosta <fons@spotify.com>
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
HCI_CONN_STK_ENCRYPT,
HCI_CONN_AUTH_INITIATOR,
HCI_CONN_DROP,
+ HCI_CONN_PARAM_REMOVAL_PEND,
};
static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..11aac06 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
hci_conn_del_sysfs(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
hci_dev_put(hdev);
hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..063b0a7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2725,10 +2725,40 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (cp->addr.type == BDADDR_BREDR) {
+ /* If disconnection is requested, then look up the
+ * connection. If the remote device is connected, it
+ * will be later used to terminate the link.
+ *
+ * Setting it to NULL explicitly will cause no
+ * termination of the link.
+ */
+ if (cp->disconnect)
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
+ &cp->addr.bdaddr);
+ else
+ conn = NULL;
+
err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
} else {
u8 addr_type;
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+ &cp->addr.bdaddr);
+ if (conn) {
+ /* Defer clearing up the connection parameters
+ * until closing to give a chance of keeping
+ * them if a repairing happens.
+ */
+ set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
+
+ /* If disconnection is not requested, then
+ * clear the connection variable so that the
+ * link is not terminated.
+ */
+ if (!cp->disconnect)
+ conn = NULL;
+ }
+
if (cp->addr.type == BDADDR_LE_PUBLIC)
addr_type = ADDR_LE_DEV_PUBLIC;
else
@@ -2736,8 +2766,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
- hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
-
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
@@ -2747,17 +2775,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}
- if (cp->disconnect) {
- if (cp->addr.type == BDADDR_BREDR)
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
- &cp->addr.bdaddr);
- else
- conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
- &cp->addr.bdaddr);
- } else {
- conn = NULL;
- }
-
if (!conn) {
err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
&rp, sizeof(rp));
@@ -3062,6 +3079,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
hci_conn_put(conn);
mgmt_pending_remove(cmd);
+
+ /* The device is paired so there is no need to remove
+ * its connection parameters anymore.
+ */
+ clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
}
void mgmt_smp_complete(struct hci_conn *conn, bool complete)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 14:30 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <0025B704-1B18-4B1E-B27D-DF84AA2F9201@holtmann.org>
Hi Marcel,
>> if (cp->addr.type == BDADDR_BREDR) {
>> + if (cp->disconnect)
>> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>> + &cp->addr.bdaddr);
>> + else
>> + conn = NULL; /* Avoid disconnecting later on*/
>> +
>
> I do not think this is a comment style that is valid in the network subsystem coding style and even if it is valid, it would be pretty unusual.
Excuse my ignorance, but Where is the network subsystem coding style
documented? I haven't been able to find it.
> Generally I would do comment on the whole code. So something like this:
> /* When disconnect of the the connection is requested,
> * then look up the connection. If the remote device is
> * connected, it will be later used to terminate the
> * link.
> *
> * Setting it to NULL explicitly will cause no
> * termination of the link.
> */
> if (cp->disconnect)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> else
> conn = NULL;
>
>> err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
>> } else {
>> u8 addr_type;
>>
>> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
>> + &cp->addr.bdaddr);
>> + if (conn) {
>> + /* Defer clearing up the connection parameters
>> + * until closing to give a chance of keeping
>> + * them if a repairing happens.
>> + */
>> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
>> +
>> + if (!cp->disconnect)
>> + conn = NULL; /* Avoid disconnecting later on*/
>
> And here I would do this:
>
> /* If disconnection is not requested, then clear the
> * connection variable so that that the link is not
> * terminated.
> */
> if (!cp->disconnect)
> conn = NULL;
Thanks, I submitted V5 to correct this.
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 16:10 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4oqcDKpPySM-qgscD8-=oTrm01Y_KnezTZt4irJ8C1FPg@mail.gmail.com>
Hi Alfonso,
>>> if (cp->addr.type == BDADDR_BREDR) {
>>> + if (cp->disconnect)
>>> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>>> + &cp->addr.bdaddr);
>>> + else
>>> + conn = NULL; /* Avoid disconnecting later on*/
>>> +
>>
>> I do not think this is a comment style that is valid in the network subsystem coding style and even if it is valid, it would be pretty unusual.
>
> Excuse my ignorance, but Where is the network subsystem coding style
> documented? I haven't been able to find it.
that is actually a good question and I am not sure I have a really good answer for that.
It is essentially Documentation/CodingStyle plus a few extra special cases. Some of them I think are only documented in scripts/checkpatch.pl --strict. The --strict switch is also called --subjective so some of the rules really only apply to the net/ and drivers/net/. I think the term subjective gives it away pretty clearly ;)
We just end up following the rules of the network subsystem here. So there is a difference between the coding style that we use in BlueZ userspace and the one that we use in the kernel. Not a major difference, but it comes to comment styles and multi-line indentations you see the difference.
Regards
Marcel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox