* Re: [PATCH v2] Fix leak of EIR data if RSSI does not change
From: Anderson Lizardo @ 2010-12-28 18:16 UTC (permalink / raw)
To: Anderson Lizardo, linux-bluetooth
In-Reply-To: <AANLkTi=dsEwvjAOGM7eFdDDZSXUrPuA5wjQw8VcWH8N5@mail.gmail.com>
Hi Johan,
On Mon, Dec 27, 2010 at 6:24 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> On Mon, Dec 27, 2010 at 5:48 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> Beware of words like "also" and "and" in commit messages. Often they are
>> a good indication that the patch shold be split into multiple ones.
>> Though in this case I can't really make up my mind if it's fine since
>> the changes are so strongly related. I'll leave it up to you to think
>> about it and decide if you can cleanly split this into two patches or
>> not.
>
> I can split into two patches without problem.
I just sent a v3 of this patch, which I hope fix all issues. It is
bigger than previous versions, but I assure it is solely to fix the
leak of EIR data. The other benefits are just "side effects":
* That if() clause which handles EIR name has been simplified, by
removing free()/g_free()
* I avoid passing ownership of memory allocated in src/event.c to
src/adapter.c, which would potentially become memory leaks if not
managed carefully.
I hope these side effects are not of a big issue to be kept in a single patch.
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* [PATCH v3] Fix leak of EIR data if RSSI does not change
From: Anderson Lizardo @ 2010-12-28 18:10 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1293473827-26418-1-git-send-email-anderson.lizardo@openbossa.org>
If RSSI value does not change, memory used by parsed EIR data would leak
because it would not be assigned to the remote_dev_info structure.
---
src/adapter.c | 20 ++++++++++++++------
src/adapter.h | 6 +++---
src/event.c | 49 ++++++++++++++++++++++++++++---------------------
3 files changed, 45 insertions(+), 30 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 4af11bc..b7a65dc 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2923,10 +2923,18 @@ static void remove_same_uuid(gpointer data, gpointer user_data)
}
}
+static void dev_prepend_uuid(gpointer data, gpointer user_data)
+{
+ struct remote_dev_info *dev = user_data;
+ char *new_uuid = data;
+
+ dev->services = g_slist_prepend(dev->services, g_strdup(new_uuid));
+}
+
void adapter_update_device_from_info(struct btd_adapter *adapter,
- bdaddr_t bdaddr, int8_t rssi,
- uint8_t evt_type, char *name,
- GSList *services, uint8_t flags)
+ bdaddr_t bdaddr, int8_t rssi,
+ uint8_t evt_type, const char *name,
+ GSList *services, uint8_t flags)
{
struct remote_dev_info *dev;
gboolean new_dev;
@@ -2945,13 +2953,13 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
(GCompareFunc) dev_rssi_cmp);
g_slist_foreach(services, remove_same_uuid, dev);
- dev->services = g_slist_concat(dev->services, services);
+ g_slist_foreach(services, dev_prepend_uuid, dev);
dev->flags = flags;
if (name) {
g_free(dev->name);
- dev->name = name;
+ dev->name = g_strdup(name);
}
/* FIXME: check if other information was changed before emitting the
@@ -2989,7 +2997,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
(GCompareFunc) dev_rssi_cmp);
g_slist_foreach(services, remove_same_uuid, dev);
- dev->services = g_slist_concat(dev->services, services);
+ g_slist_foreach(services, dev_prepend_uuid, dev);
adapter_emit_device_found(adapter, dev);
}
diff --git a/src/adapter.h b/src/adapter.h
index 1fbc221..a655cdb 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -135,9 +135,9 @@ int adapter_get_discover_type(struct btd_adapter *adapter);
struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter,
struct remote_dev_info *match);
void adapter_update_device_from_info(struct btd_adapter *adapter,
- bdaddr_t bdaddr, int8_t rssi,
- uint8_t evt_type, char *name,
- GSList *services, uint8_t flags);
+ bdaddr_t bdaddr, int8_t rssi,
+ uint8_t evt_type, const char *name,
+ GSList *services, uint8_t flags);
void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
int8_t rssi, uint32_t class, const char *name,
const char *alias, gboolean legacy,
diff --git a/src/event.c b/src/event.c
index 178cea8..b1054d7 100644
--- a/src/event.c
+++ b/src/event.c
@@ -431,6 +431,13 @@ static int parse_eir_data(struct eir_data *eir, uint8_t *eir_data,
return 0;
}
+static void free_eir_data(struct eir_data *eir)
+{
+ g_slist_foreach(eir->services, (GFunc) g_free, NULL);
+ g_slist_free(eir->services);
+ g_free(eir->name);
+}
+
void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
{
struct btd_adapter *adapter;
@@ -455,6 +462,8 @@ void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
adapter_update_device_from_info(adapter, info->bdaddr, rssi,
info->evt_type, eir_data.name,
eir_data.services, eir_data.flags);
+
+ free_eir_data(&eir_data);
}
static void update_lastseen(bdaddr_t *sba, bdaddr_t *dba)
@@ -491,6 +500,7 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
int state, err;
dbus_bool_t legacy;
unsigned char features[8];
+ const char *dev_name;
ba2str(local, local_addr);
ba2str(peer, peer_addr);
@@ -517,11 +527,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
adapter_set_state(adapter, state);
}
- memset(&eir_data, 0, sizeof(eir_data));
- err = parse_eir_data(&eir_data, data, EIR_DATA_LENGTH);
- if (err < 0)
- error("Error parsing EIR data: %s (%d)", strerror(-err), -err);
-
/* the inquiry result can be triggered by NON D-Bus client */
if (adapter_get_discover_type(adapter) & DISC_RESOLVNAME &&
adapter_has_discov_sessions(adapter))
@@ -547,28 +552,30 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
} else
legacy = TRUE;
- if (eir_data.name) {
+ memset(&eir_data, 0, sizeof(eir_data));
+ err = parse_eir_data(&eir_data, data, EIR_DATA_LENGTH);
+ if (err < 0)
+ error("Error parsing EIR data: %s (%d)", strerror(-err), -err);
+
+ /* Complete EIR names are always used. Shortened EIR names are only
+ * used if there is no name already in storage. */
+ dev_name = name;
+ if (eir_data.name != NULL) {
if (eir_data.name_complete) {
write_device_name(local, peer, eir_data.name);
name_status = NAME_NOT_REQUIRED;
-
- if (name)
- g_free(name);
-
- name = eir_data.name;
- } else {
- if (name)
- free(eir_data.name);
- else
- name = eir_data.name;
- }
+ dev_name = eir_data.name;
+ } else if (name == NULL)
+ dev_name = eir_data.name;
}
- adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
- legacy, eir_data.services, name_status);
+ adapter_update_found_devices(adapter, peer, rssi, class, dev_name,
+ alias, legacy, eir_data.services,
+ name_status);
- g_free(name);
- g_free(alias);
+ free_eir_data(&eir_data);
+ free(name);
+ free(alias);
}
void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer,
--
1.7.0.4
^ permalink raw reply related
* [PATCH] Fix crash due to misplaced parameter in emit_device_found() call
From: Anderson Lizardo @ 2010-12-28 16:52 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
To avoid related mistakes, dev->uuid_count was shortened to uuid_count
and kept on the same line.
---
Note that there is a 82-column line on this patch. I chose to put it
on the same line to avoid a similar mistake in future.
---
src/adapter.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index bfc2b8b..816d842 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2841,9 +2841,9 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
"RSSI", DBUS_TYPE_INT16, &rssi,
"Name", DBUS_TYPE_STRING, &dev->name,
"Paired", DBUS_TYPE_BOOLEAN, &paired,
- "UUIDs", DBUS_TYPE_ARRAY, &dev->uuids,
"Broadcaster", DBUS_TYPE_BOOLEAN, &broadcaster,
- dev->uuid_count, NULL);
+ "UUIDs", DBUS_TYPE_ARRAY, &dev->uuids, uuid_count,
+ NULL);
return;
}
@@ -2867,7 +2867,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
"Alias", DBUS_TYPE_STRING, &alias,
"LegacyPairing", DBUS_TYPE_BOOLEAN, &dev->legacy,
"Paired", DBUS_TYPE_BOOLEAN, &paired,
- "UUIDs", DBUS_TYPE_ARRAY, &dev->uuids, dev->uuid_count,
+ "UUIDs", DBUS_TYPE_ARRAY, &dev->uuids, uuid_count,
NULL);
g_free(alias);
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCHv3] Bluetooth: Use non-flushable by default L2CAP data packets
From: Andrei Emeltchenko @ 2010-12-28 15:31 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1293549367-2516-2-git-send-email-Andrei.Emeltchenko.news@gmail.com>
On Tue, Dec 28, 2010 at 5:16 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>
> Modification of Nick Pelly <npelly@google.com> patch.
>
> With Bluetooth 2.1 ACL packets can be flushable or non-flushable. This commit
> makes ACL data packets non-flushable by default on compatible chipsets, and
> adds the BT_FLUSHABLE socket option to explicitly request flushable ACL
> data packets for a given L2CAP socket. This is useful for A2DP data which can
> be safely discarded if it can not be delivered within a short time (while
> other ACL data should not be discarded).
>
> Note that making ACL data flushable has no effect unless the automatic flush
> timeout for that ACL link is changed from its default of 0 (infinite).
>
> Default packet types (for compatible chipsets):
> Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
> Bluetooth HCI H4
> Bluetooth HCI ACL Packet
> .... 0000 0000 0010 = Connection Handle: 0x0002
> ..00 .... .... .... = PB Flag: First Non-automatically Flushable Packet (0)
> 00.. .... .... .... = BC Flag: Point-To-Point (0)
> Data Total Length: 8
> Bluetooth L2CAP Packet
>
> After setting BT_FLUSHABLE
> (sock.setsockopt(274 /*SOL_BLUETOOTH*/, 8 /* BT_FLUSHABLE */, 1 /* flush */))
> Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
> Bluetooth HCI H4
> Bluetooth HCI ACL Packet
> .... 0000 0000 0010 = Connection Handle: 0x0002
> ..10 .... .... .... = PB Flag: First Automatically Flushable Packet (2)
> 00.. .... .... .... = BC Flag: Point-To-Point (0)
> Data Total Length: 8
> Bluetooth L2CAP Packet
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
> include/net/bluetooth/bluetooth.h | 5 +++
> include/net/bluetooth/hci.h | 2 +
> include/net/bluetooth/hci_core.h | 1 +
> include/net/bluetooth/l2cap.h | 1 +
> net/bluetooth/hci_core.c | 7 +++-
> net/bluetooth/l2cap.c | 57 ++++++++++++++++++++++++++++++++++--
> 6 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 0c5e725..ed7d775 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -64,6 +64,11 @@ struct bt_security {
>
> #define BT_DEFER_SETUP 7
>
> +#define BT_FLUSHABLE 8
> +
> +#define BT_FLUSHABLE_OFF 0
> +#define BT_FLUSHABLE_ON 1
> +
> #define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" , ## arg)
> #define BT_ERR(fmt, arg...) printk(KERN_ERR "%s: " fmt "\n" , __func__ , ## arg)
> #define BT_DBG(fmt, arg...) pr_debug("%s: " fmt "\n" , __func__ , ## arg)
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 29a7a8c..5d033dc 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -150,6 +150,7 @@ enum {
> #define EDR_ESCO_MASK (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
>
> /* ACL flags */
> +#define ACL_START_NO_FLUSH 0x00
> #define ACL_CONT 0x01
> #define ACL_START 0x02
> #define ACL_ACTIVE_BCAST 0x04
> @@ -194,6 +195,7 @@ enum {
> #define LMP_EDR_3S_ESCO 0x80
>
> #define LMP_SIMPLE_PAIR 0x08
> +#define LMP_NO_FLUSH 0x40
>
> /* Connection modes */
> #define HCI_CM_ACTIVE 0x0000
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a29feb0..59135a6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -457,6 +457,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> #define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
> #define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
> +#define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
>
> /* ----- HCI protocols ----- */
> struct hci_proto {
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 7ad25ca..7f88a87 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -327,6 +327,7 @@ struct l2cap_pinfo {
> __u8 sec_level;
> __u8 role_switch;
> __u8 force_reliable;
> + __u8 flushable;
>
> __u8 conf_req[64];
> __u8 conf_len;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 8b602d8..3c0990d 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1391,7 +1391,7 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>
> skb->dev = (void *) hdev;
> bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
> - hci_add_acl_hdr(skb, conn->handle, flags | ACL_START);
> + hci_add_acl_hdr(skb, conn->handle, flags);
>
> list = skb_shinfo(skb)->frag_list;
> if (!list) {
> @@ -1409,12 +1409,15 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
> spin_lock_bh(&conn->data_q.lock);
>
> __skb_queue_tail(&conn->data_q, skb);
> +
> + flags &= ~ACL_START;
> + flags |= ACL_CONT;
> do {
> skb = list; list = list->next;
>
> skb->dev = (void *) hdev;
> bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
> - hci_add_acl_hdr(skb, conn->handle, flags | ACL_CONT);
> + hci_add_acl_hdr(skb, conn->handle, flags);
>
> BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index c791fcd..bedbb5f 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
> static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
> {
> struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
> + u8 flags;
>
> BT_DBG("code 0x%2.2x", code);
>
> if (!skb)
> return;
>
> - hci_send_acl(conn->hcon, skb, 0);
> + if (lmp_no_flush_capable(conn->hcon->hdev))
> + flags = ACL_START_NO_FLUSH;
> + else
> + flags = ACL_START;
> +
> + hci_send_acl(conn->hcon, skb, flags);
> }
>
> static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
> @@ -378,6 +384,7 @@ static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
> struct l2cap_conn *conn = pi->conn;
> struct sock *sk = (struct sock *)pi;
> int count, hlen = L2CAP_HDR_SIZE + 2;
> + u8 flags;
>
> if (sk->sk_state != BT_CONNECTED)
> return;
> @@ -414,7 +421,12 @@ static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
> put_unaligned_le16(fcs, skb_put(skb, 2));
> }
>
> - hci_send_acl(pi->conn->hcon, skb, 0);
> + if (lmp_no_flush_capable(conn->hcon->hdev))
> + flags = ACL_START_NO_FLUSH;
> + else
> + flags = ACL_START;
> +
> + hci_send_acl(pi->conn->hcon, skb, flags);
> }
>
> static inline void l2cap_send_rr_or_rnr(struct l2cap_pinfo *pi, u16 control)
> @@ -900,6 +912,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
> pi->sec_level = l2cap_pi(parent)->sec_level;
> pi->role_switch = l2cap_pi(parent)->role_switch;
> pi->force_reliable = l2cap_pi(parent)->force_reliable;
> + pi->flushable = l2cap_pi(parent)->flushable;
> } else {
> pi->imtu = L2CAP_DEFAULT_MTU;
> pi->omtu = 0;
> @@ -915,6 +928,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
> pi->sec_level = BT_SECURITY_LOW;
> pi->role_switch = 0;
> pi->force_reliable = 0;
> + pi->flushable = BT_FLUSHABLE_OFF;
> }
>
> /* Default config options */
> @@ -1450,10 +1464,17 @@ static void l2cap_drop_acked_frames(struct sock *sk)
> static inline void l2cap_do_send(struct sock *sk, struct sk_buff *skb)
> {
> struct l2cap_pinfo *pi = l2cap_pi(sk);
> + struct hci_conn *hcon = pi->conn->hcon;
> + u16 flags;
>
> BT_DBG("sk %p, skb %p len %d", sk, skb, skb->len);
>
> - hci_send_acl(pi->conn->hcon, skb, 0);
> + if (!pi->flushable && lmp_no_flush_capable(hcon->hdev))
> + flags = ACL_START_NO_FLUSH;
> + else
> + flags = ACL_START;
> +
> + hci_send_acl(hcon, skb, flags);
> }
>
> static void l2cap_streaming_send(struct sock *sk)
> @@ -2098,6 +2119,28 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
> bt_sk(sk)->defer_setup = opt;
> break;
>
> + case BT_FLUSHABLE:
> + if (get_user(opt, (u32 __user *) optval)) {
> + err = -EFAULT;
> + break;
> + }
> +
> + if (opt > BT_FLUSHABLE_ON) {
> + err = -EINVAL;
> + break;
> + }
> +
> + if (opt == BT_FLUSHABLE_OFF) {
> + struct l2cap_conn *conn = l2cap_pi(sk)->conn;
> + if (conn && !lmp_no_flush_capable(conn->hcon->hdev)) {
or maybe:
if (!conn ||
!lmp_no_flush_capable(conn->hcon->hdev)) {
> + err = -EINVAL;
> + break;
> + }
> + }
> +
> + l2cap_pi(sk)->flushable = opt;
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> break;
> @@ -2237,6 +2280,12 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
>
> break;
>
> + case BT_FLUSHABLE:
> + if (put_user(l2cap_pi(sk)->flushable, (u32 __user *) optval))
> + err = -EFAULT;
> +
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> break;
> @@ -4697,7 +4746,7 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
>
> BT_DBG("conn %p len %d flags 0x%x", conn, skb->len, flags);
>
> - if (flags & ACL_START) {
> + if (!(flags & ACL_CONT)) {
> struct l2cap_hdr *hdr;
> struct sock *sk;
> u16 cid;
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [RFCv2] Bluetooth: Use non-flushable by default L2CAP data packets
From: Andrei Emeltchenko @ 2010-12-28 15:22 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20101223020615.GA26284@vigoh>
Hi Gustavo,
On Thu, Dec 23, 2010 at 4:06 AM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Andrei,
>
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-12-16 16:54:26 +0200]:
>
>> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>
>> Modification of Nick Pelly <npelly@google.com> patch.
>>
>> With Bluetooth 2.1 ACL packets can be flushable or non-flushable. This commit
>> makes ACL data packets non-flushable by default on compatible chipsets, and
>> adds the BT_FLUSHABLE socket option to explicitly request flushable ACL
>> data packets for a given L2CAP socket. This is useful for A2DP data which can
>> be safely discarded if it can not be delivered within a short time (while
>> other ACL data should not be discarded).
>>
>> Note that making ACL data flushable has no effect unless the automatic flush
>> timeout for that ACL link is changed from its default of 0 (infinite).
>>
>> Default packet types (for compatible chipsets):
>> Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
>> Bluetooth HCI H4
>> Bluetooth HCI ACL Packet
>> .... 0000 0000 0010 = Connection Handle: 0x0002
>> ..00 .... .... .... = PB Flag: First Non-automatically Flushable Packet (0)
>> 00.. .... .... .... = BC Flag: Point-To-Point (0)
>> Data Total Length: 8
>> Bluetooth L2CAP Packet
>>
>> After setting BT_FLUSHABLE
>> (sock.setsockopt(274 /*SOL_BLUETOOTH*/, 8 /* BT_FLUSHABLE */, 1 /* flush */))
>> Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
>> Bluetooth HCI H4
>> Bluetooth HCI ACL Packet
>> .... 0000 0000 0010 = Connection Handle: 0x0002
>> ..10 .... .... .... = PB Flag: First Automatically Flushable Packet (2)
>> 00.. .... .... .... = BC Flag: Point-To-Point (0)
>> Data Total Length: 8
>> Bluetooth L2CAP Packet
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> ---
>> include/net/bluetooth/bluetooth.h | 5 ++++
>> include/net/bluetooth/hci.h | 2 +
>> include/net/bluetooth/hci_core.h | 1 +
>> include/net/bluetooth/l2cap.h | 2 +
>> net/bluetooth/hci_core.c | 6 +++-
>> net/bluetooth/l2cap.c | 48 ++++++++++++++++++++++++++++++++++--
>> 6 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 0c5e725..ed7d775 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -64,6 +64,11 @@ struct bt_security {
>>
>> #define BT_DEFER_SETUP 7
>>
>> +#define BT_FLUSHABLE 8
>> +
>> +#define BT_FLUSHABLE_OFF 0
>> +#define BT_FLUSHABLE_ON 1
>> +
>> #define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" , ## arg)
>> #define BT_ERR(fmt, arg...) printk(KERN_ERR "%s: " fmt "\n" , __func__ , ## arg)
>> #define BT_DBG(fmt, arg...) pr_debug("%s: " fmt "\n" , __func__ , ## arg)
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index 29a7a8c..333d5cb 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -150,6 +150,7 @@ enum {
>> #define EDR_ESCO_MASK (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
>>
>> /* ACL flags */
>> +#define ACL_START_NO_FLUSH 0x00
>> #define ACL_CONT 0x01
>> #define ACL_START 0x02
>> #define ACL_ACTIVE_BCAST 0x04
>> @@ -193,6 +194,7 @@ enum {
>> #define LMP_EDR_ESCO_3M 0x40
>> #define LMP_EDR_3S_ESCO 0x80
>>
>> +#define LMP_NO_FLUSH 0x01
>
> Isn't this 0x40? As stated on Core v4.0 (Volume 2, part C, 3.3 Feature Mask
> Definition)
>
>> #define LMP_SIMPLE_PAIR 0x08
>>
>> /* Connection modes */
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 1992fac..9778bc8 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -456,6 +456,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>> #define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
>> #define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
>> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
>> +#define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
>>
>> /* ----- HCI protocols ----- */
>> struct hci_proto {
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 7ad25ca..af35711 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -75,6 +75,7 @@ struct l2cap_conninfo {
>> #define L2CAP_LM_TRUSTED 0x0008
>> #define L2CAP_LM_RELIABLE 0x0010
>> #define L2CAP_LM_SECURE 0x0020
>> +#define L2CAP_LM_FLUSHABLE 0x0040
>
> Not using this flag in the code.
>
>>
>> /* L2CAP command codes */
>> #define L2CAP_COMMAND_REJ 0x01
>> @@ -327,6 +328,7 @@ struct l2cap_pinfo {
>> __u8 sec_level;
>> __u8 role_switch;
>> __u8 force_reliable;
>> + __u8 flushable;
>>
>> __u8 conf_req[64];
>> __u8 conf_len;
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 51c61f7..c0d776b 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1380,7 +1380,7 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>>
>> skb->dev = (void *) hdev;
>> bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> - hci_add_acl_hdr(skb, conn->handle, flags | ACL_START);
>> + hci_add_acl_hdr(skb, conn->handle, flags);
>>
>> list = skb_shinfo(skb)->frag_list;
>> if (!list) {
>> @@ -1398,12 +1398,14 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>> spin_lock_bh(&conn->data_q.lock);
>>
>> __skb_queue_tail(&conn->data_q, skb);
>
> add an empty line here.
>
>> + flags &= ~ACL_START;
>> + flags |= ACL_CONT;
>> do {
>> skb = list; list = list->next;
>>
>> skb->dev = (void *) hdev;
>> bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> - hci_add_acl_hdr(skb, conn->handle, flags | ACL_CONT);
>> + hci_add_acl_hdr(skb, conn->handle, flags);
>>
>> BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index c791fcd..efa60eb 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
>> static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
>> {
>> struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
>> + u8 flags;
>>
>> BT_DBG("code 0x%2.2x", code);
>>
>> if (!skb)
>> return;
>>
>> - hci_send_acl(conn->hcon, skb, 0);
>> + if (lmp_no_flush_capable(conn->hcon->hdev))
>> + flags = ACL_START_NO_FLUSH;
>> + else
>> + flags = ACL_START;
>> +
>> + hci_send_acl(conn->hcon, skb, flags);
>> }
>>
>> static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
>> @@ -900,6 +906,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
>> pi->sec_level = l2cap_pi(parent)->sec_level;
>> pi->role_switch = l2cap_pi(parent)->role_switch;
>> pi->force_reliable = l2cap_pi(parent)->force_reliable;
>> + pi->flushable = l2cap_pi(parent)->flushable;
>> } else {
>> pi->imtu = L2CAP_DEFAULT_MTU;
>> pi->omtu = 0;
>> @@ -915,6 +922,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
>> pi->sec_level = BT_SECURITY_LOW;
>> pi->role_switch = 0;
>> pi->force_reliable = 0;
>> + pi->flushable = BT_FLUSHABLE_OFF;
>
> You need to check for the no_flush feature here, right?
I think at this stage we do not have hci and l2cap connection handlers
added to the socket.
We assign it in l2cap_chan_add later when making connect. Do you think
it makes sense to add the check there?
I am sending the latest patch leaving this unchanged for now.
Regards,
Andrei
>
>> }
>>
>> /* Default config options */
>> @@ -1450,10 +1458,17 @@ static void l2cap_drop_acked_frames(struct sock *sk)
>> static inline void l2cap_do_send(struct sock *sk, struct sk_buff *skb)
>> {
>> struct l2cap_pinfo *pi = l2cap_pi(sk);
>> + struct hci_conn *hcon = pi->conn->hcon;
>> + u16 flags;
>>
>> BT_DBG("sk %p, skb %p len %d", sk, skb, skb->len);
>>
>> - hci_send_acl(pi->conn->hcon, skb, 0);
>> + if (lmp_no_flush_capable(hcon->hdev) && !l2cap_pi(sk)->flushable)
>> + flags = ACL_START_NO_FLUSH;
>> + else
>> + flags = ACL_START;
>> +
>> + hci_send_acl(hcon, skb, flags);
>
> There is another call to hci_send_acl() in l2cap. It is in
> l2cap_send_sframe(), but I'm still wondering if we shall flush those packets
> or not in the case flushable was set. Now I'm thinking that don't.
>
> Then you need to add the same check in l2cap_send_cmd to l2cap_send_sframe().
>
>> }
>>
>> static void l2cap_streaming_send(struct sock *sk)
>> @@ -2045,6 +2060,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
>> static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, char __user *optval, unsigned int optlen)
>> {
>> struct sock *sk = sock->sk;
>> + struct hci_conn *hcon = l2cap_pi(sk)->conn->hcon;
>> struct bt_security sec;
>> int len, err = 0;
>> u32 opt;
>> @@ -2098,6 +2114,26 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
>> bt_sk(sk)->defer_setup = opt;
>> break;
>>
>> + case BT_FLUSHABLE:
>> + if (get_user(opt, (u32 __user *) optval)) {
>> + err = -EFAULT;
>> + break;
>> + }
>> +
>> + if (opt > BT_FLUSHABLE_ON) {
>> + err = -EINVAL;
>> + break;
>> + }
>> +
>> + if (opt == BT_FLUSHABLE_OFF &&
>> + !lmp_no_flush_capable(hcon->hdev)) {
>
> I'm not really happy with !lmp_no_flush... 2 negatives in the same place, but
> that is the feature name. So we can't do too much here. Btw, the "No" in the
> feature name is making my brain hurt. ;)
>
> regards,
>
> --
> Gustavo F. Padovan
> http://profusion.mobi
>
^ permalink raw reply
* [PATCHv3] Bluetooth: Use non-flushable by default L2CAP data packets
From: Emeltchenko Andrei @ 2010-12-28 15:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1293549367-2516-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Modification of Nick Pelly <npelly@google.com> patch.
With Bluetooth 2.1 ACL packets can be flushable or non-flushable. This commit
makes ACL data packets non-flushable by default on compatible chipsets, and
adds the BT_FLUSHABLE socket option to explicitly request flushable ACL
data packets for a given L2CAP socket. This is useful for A2DP data which can
be safely discarded if it can not be delivered within a short time (while
other ACL data should not be discarded).
Note that making ACL data flushable has no effect unless the automatic flush
timeout for that ACL link is changed from its default of 0 (infinite).
Default packet types (for compatible chipsets):
Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
Bluetooth HCI H4
Bluetooth HCI ACL Packet
.... 0000 0000 0010 = Connection Handle: 0x0002
..00 .... .... .... = PB Flag: First Non-automatically Flushable Packet (0)
00.. .... .... .... = BC Flag: Point-To-Point (0)
Data Total Length: 8
Bluetooth L2CAP Packet
After setting BT_FLUSHABLE
(sock.setsockopt(274 /*SOL_BLUETOOTH*/, 8 /* BT_FLUSHABLE */, 1 /* flush */))
Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
Bluetooth HCI H4
Bluetooth HCI ACL Packet
.... 0000 0000 0010 = Connection Handle: 0x0002
..10 .... .... .... = PB Flag: First Automatically Flushable Packet (2)
00.. .... .... .... = BC Flag: Point-To-Point (0)
Data Total Length: 8
Bluetooth L2CAP Packet
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
include/net/bluetooth/bluetooth.h | 5 +++
include/net/bluetooth/hci.h | 2 +
include/net/bluetooth/hci_core.h | 1 +
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/hci_core.c | 7 +++-
net/bluetooth/l2cap.c | 57 ++++++++++++++++++++++++++++++++++--
6 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 0c5e725..ed7d775 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -64,6 +64,11 @@ struct bt_security {
#define BT_DEFER_SETUP 7
+#define BT_FLUSHABLE 8
+
+#define BT_FLUSHABLE_OFF 0
+#define BT_FLUSHABLE_ON 1
+
#define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" , ## arg)
#define BT_ERR(fmt, arg...) printk(KERN_ERR "%s: " fmt "\n" , __func__ , ## arg)
#define BT_DBG(fmt, arg...) pr_debug("%s: " fmt "\n" , __func__ , ## arg)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 29a7a8c..5d033dc 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -150,6 +150,7 @@ enum {
#define EDR_ESCO_MASK (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
/* ACL flags */
+#define ACL_START_NO_FLUSH 0x00
#define ACL_CONT 0x01
#define ACL_START 0x02
#define ACL_ACTIVE_BCAST 0x04
@@ -194,6 +195,7 @@ enum {
#define LMP_EDR_3S_ESCO 0x80
#define LMP_SIMPLE_PAIR 0x08
+#define LMP_NO_FLUSH 0x40
/* Connection modes */
#define HCI_CM_ACTIVE 0x0000
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a29feb0..59135a6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -457,6 +457,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
#define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
#define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
#define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
+#define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
/* ----- HCI protocols ----- */
struct hci_proto {
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 7ad25ca..7f88a87 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -327,6 +327,7 @@ struct l2cap_pinfo {
__u8 sec_level;
__u8 role_switch;
__u8 force_reliable;
+ __u8 flushable;
__u8 conf_req[64];
__u8 conf_len;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8b602d8..3c0990d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1391,7 +1391,7 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
skb->dev = (void *) hdev;
bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
- hci_add_acl_hdr(skb, conn->handle, flags | ACL_START);
+ hci_add_acl_hdr(skb, conn->handle, flags);
list = skb_shinfo(skb)->frag_list;
if (!list) {
@@ -1409,12 +1409,15 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
spin_lock_bh(&conn->data_q.lock);
__skb_queue_tail(&conn->data_q, skb);
+
+ flags &= ~ACL_START;
+ flags |= ACL_CONT;
do {
skb = list; list = list->next;
skb->dev = (void *) hdev;
bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
- hci_add_acl_hdr(skb, conn->handle, flags | ACL_CONT);
+ hci_add_acl_hdr(skb, conn->handle, flags);
BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index c791fcd..bedbb5f 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
{
struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
+ u8 flags;
BT_DBG("code 0x%2.2x", code);
if (!skb)
return;
- hci_send_acl(conn->hcon, skb, 0);
+ if (lmp_no_flush_capable(conn->hcon->hdev))
+ flags = ACL_START_NO_FLUSH;
+ else
+ flags = ACL_START;
+
+ hci_send_acl(conn->hcon, skb, flags);
}
static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
@@ -378,6 +384,7 @@ static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
struct l2cap_conn *conn = pi->conn;
struct sock *sk = (struct sock *)pi;
int count, hlen = L2CAP_HDR_SIZE + 2;
+ u8 flags;
if (sk->sk_state != BT_CONNECTED)
return;
@@ -414,7 +421,12 @@ static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
put_unaligned_le16(fcs, skb_put(skb, 2));
}
- hci_send_acl(pi->conn->hcon, skb, 0);
+ if (lmp_no_flush_capable(conn->hcon->hdev))
+ flags = ACL_START_NO_FLUSH;
+ else
+ flags = ACL_START;
+
+ hci_send_acl(pi->conn->hcon, skb, flags);
}
static inline void l2cap_send_rr_or_rnr(struct l2cap_pinfo *pi, u16 control)
@@ -900,6 +912,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
pi->sec_level = l2cap_pi(parent)->sec_level;
pi->role_switch = l2cap_pi(parent)->role_switch;
pi->force_reliable = l2cap_pi(parent)->force_reliable;
+ pi->flushable = l2cap_pi(parent)->flushable;
} else {
pi->imtu = L2CAP_DEFAULT_MTU;
pi->omtu = 0;
@@ -915,6 +928,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
pi->sec_level = BT_SECURITY_LOW;
pi->role_switch = 0;
pi->force_reliable = 0;
+ pi->flushable = BT_FLUSHABLE_OFF;
}
/* Default config options */
@@ -1450,10 +1464,17 @@ static void l2cap_drop_acked_frames(struct sock *sk)
static inline void l2cap_do_send(struct sock *sk, struct sk_buff *skb)
{
struct l2cap_pinfo *pi = l2cap_pi(sk);
+ struct hci_conn *hcon = pi->conn->hcon;
+ u16 flags;
BT_DBG("sk %p, skb %p len %d", sk, skb, skb->len);
- hci_send_acl(pi->conn->hcon, skb, 0);
+ if (!pi->flushable && lmp_no_flush_capable(hcon->hdev))
+ flags = ACL_START_NO_FLUSH;
+ else
+ flags = ACL_START;
+
+ hci_send_acl(hcon, skb, flags);
}
static void l2cap_streaming_send(struct sock *sk)
@@ -2098,6 +2119,28 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
bt_sk(sk)->defer_setup = opt;
break;
+ case BT_FLUSHABLE:
+ if (get_user(opt, (u32 __user *) optval)) {
+ err = -EFAULT;
+ break;
+ }
+
+ if (opt > BT_FLUSHABLE_ON) {
+ err = -EINVAL;
+ break;
+ }
+
+ if (opt == BT_FLUSHABLE_OFF) {
+ struct l2cap_conn *conn = l2cap_pi(sk)->conn;
+ if (conn && !lmp_no_flush_capable(conn->hcon->hdev)) {
+ err = -EINVAL;
+ break;
+ }
+ }
+
+ l2cap_pi(sk)->flushable = opt;
+ break;
+
default:
err = -ENOPROTOOPT;
break;
@@ -2237,6 +2280,12 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
break;
+ case BT_FLUSHABLE:
+ if (put_user(l2cap_pi(sk)->flushable, (u32 __user *) optval))
+ err = -EFAULT;
+
+ break;
+
default:
err = -ENOPROTOOPT;
break;
@@ -4697,7 +4746,7 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
BT_DBG("conn %p len %d flags 0x%x", conn, skb->len, flags);
- if (flags & ACL_START) {
+ if (!(flags & ACL_CONT)) {
struct l2cap_hdr *hdr;
struct sock *sk;
u16 cid;
--
1.7.1
^ permalink raw reply related
* [PATCHv3] Bluetooth: Use non-flushable by default L2CAP data packets
From: Emeltchenko Andrei @ 2010-12-28 15:16 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Modification of Nick Pelly <npelly@google.com> patch.
Changes to previous releases:
- rebasing, simplifying logic, using SOL_BLUETOOTH instead of SOL_L2CAP.
- LMP_NO_FLUSH feature mask corrected, added check to l2cap_send_sframe
Andrei Emeltchenko (1):
Bluetooth: Use non-flushable by default L2CAP data packets
include/net/bluetooth/bluetooth.h | 5 +++
include/net/bluetooth/hci.h | 2 +
include/net/bluetooth/hci_core.h | 1 +
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/hci_core.c | 7 +++-
net/bluetooth/l2cap.c | 57 ++++++++++++++++++++++++++++++++++--
6 files changed, 67 insertions(+), 6 deletions(-)
^ permalink raw reply
* Re: [PATCH] ath3k-2.fw is obsolete and is not used by Atheros chipsets.
From: Bala Shanmugam @ 2010-12-28 11:40 UTC (permalink / raw)
To: dwmw2@infradead.org; +Cc: linux-bluetooth@vger.kernel.org, Luis Rodriguez
In-Reply-To: <1292922606-32255-1-git-send-email-sbalashanmugam@atheros.com>
Hi David,
Can you please upstream this patch.
Regards,
Bala.
^ permalink raw reply
* Re: [PATCH 3/4] Fix memory leaks after using strtoba, batostr
From: Michal.Labedzki @ 2010-12-28 9:34 UTC (permalink / raw)
To: anderson.lizardo; +Cc: linux-bluetooth
In-Reply-To: <AANLkTikD4S-rD+NCXs7URSYv3Ah=-uToEw4YyCP4cA5x@mail.gmail.com>
>Hi,
>
>This patch would not be necessary if you keep only str2ba() and
>ba2str() (like I commented on patch 2/4).
>
>Regards,
Cannot keep only *2*() functions, because for two reason. Firstly, these functions are different (see discussion at patch 2/4).
Secondly, for example, "ba2str" use external buffer, while "batostr" create it internal. So simpler is using "batostr",
because there is no need to create temporary buffer.
So I think patch is (quite) useful.
Regards
^ permalink raw reply
* [PATCH v2 2/4] Refactoring common code in bluetooth.c
From: Michal Labedzki @ 2010-12-28 9:17 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michal Labedzki
In-Reply-To: <AANLkTi==TysBDUUXrDYy4bizr+3Nz20nn2fyc_g2g__U@mail.gmail.com>
in lib/bluetooth.c make "str2ba" using "strtoba", "ba2str" using "batostr"
add comments describe differences between these functions
---
lib/bluetooth.c | 34 +++++++++++++++++-----------------
1 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/lib/bluetooth.c b/lib/bluetooth.c
index 4af2ef6..3a4e3f4 100644
--- a/lib/bluetooth.c
+++ b/lib/bluetooth.c
@@ -55,8 +55,7 @@ char *batostr(const bdaddr_t *ba)
return NULL;
sprintf(str, "%2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X",
- ba->b[0], ba->b[1], ba->b[2],
- ba->b[3], ba->b[4], ba->b[5]);
+ ba->b[0], ba->b[1], ba->b[2], ba->b[3], ba->b[4], ba->b[5]);
return str;
}
@@ -80,29 +79,30 @@ bdaddr_t *strtoba(const char *str)
return (bdaddr_t *) ba;
}
+/* reverse bdaddr and do batostr */
int ba2str(const bdaddr_t *ba, char *str)
{
- uint8_t b[6];
-
- baswap((bdaddr_t *) b, ba);
- return sprintf(str, "%2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X",
- b[0], b[1], b[2], b[3], b[4], b[5]);
+ bdaddr_t b;
+ char *bastr;
+
+ baswap(&b, ba);
+ bastr = batostr(&b);
+ strcpy(str, bastr);
+ bt_free(bastr);
+ return *str && 1;
}
+/* do strtoba and return reverse bdaddr */
int str2ba(const char *str, bdaddr_t *ba)
{
- uint8_t b[6];
- const char *ptr = str;
+ bdaddr_t *b;
int i;
- for (i = 0; i < 6; i++) {
- b[i] = (uint8_t) strtol(ptr, NULL, 16);
- if (i != 5 && !(ptr = strchr(ptr, ':')))
- ptr = ":00:00:00:00:00";
- ptr++;
- }
-
- baswap(ba, (bdaddr_t *) b);
+ b = strtoba(str);
+ if (b == NULL)
+ return 0;
+ baswap(ba, b);
+ bt_free(b);
return 0;
}
--
1.7.0.4
^ permalink raw reply related
* [PATCH v2 4/4] Better filtering input string contain bdaddr
From: Michal Labedzki @ 2010-12-28 9:15 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michal Labedzki
In-Reply-To: <AANLkTinXwbHZt4SSaz+tUUH+7QnQo7L4FKFKetmQm9Qx@mail.gmail.com>
---
lib/bluetooth.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 46 insertions(+), 6 deletions(-)
diff --git a/lib/bluetooth.c b/lib/bluetooth.c
index 3a4e3f4..4e32a1d 100644
--- a/lib/bluetooth.c
+++ b/lib/bluetooth.c
@@ -60,23 +60,63 @@ char *batostr(const bdaddr_t *ba)
return str;
}
+/* convert to bdaddr_t string describes with regular expression "(XX:){0,5}XX"
+ * where X is "[0-9A-Fa-f]"
+ * shorter address is filling with ":00" at the end
+ */
bdaddr_t *strtoba(const char *str)
{
const char *ptr = str;
int i;
+ int parts;
+ int length;
+ bdaddr_t *ba;
+
+ length = strlen(str);
+ if (length <= 1 || length > 17)
+ return NULL;
- uint8_t *ba = bt_malloc(sizeof(bdaddr_t));
+ ba = bt_malloc(sizeof(bdaddr_t));
if (!ba)
return NULL;
- for (i = 0; i < 6; i++) {
- ba[i] = (uint8_t) strtol(ptr, NULL, 16);
- if (i != 5 && !(ptr = strchr(ptr,':')))
- ptr = ":00:00:00:00:00";
+ i = 0;
+ parts = 0;
+ while (*ptr) {
+ if (i == 2) {
+ if (*ptr == ':') {
+ i = 0;
+ ba->b[parts] = strtol(ptr - 2, NULL, 16);
+ parts++;
+ ptr++;
+ } else {
+ bt_free(ba);
+ return NULL;
+ }
+ }
+
+ if ((*ptr < 'A' || *ptr > 'F') &&
+ (*ptr < 'a' || *ptr > 'f') &&
+ (*ptr < '0' || *ptr > '9')) {
+ bt_free(ba);
+ return NULL;
+ }
+
+ i++;
ptr++;
}
- return (bdaddr_t *) ba;
+ if (*(ptr - 1) == ':' || i == 1) {
+ bt_free(ba);
+ return NULL;
+ }
+
+ ba->b[parts] = strtol(ptr - 2, NULL, 16);
+ for (i = parts + 1 ; i < 6 ; i++) {
+ ba->b[i] = 0;
+ }
+
+ return ba;
}
/* reverse bdaddr and do batostr */
--
1.7.0.4
^ permalink raw reply related
* [PATCH v2 1/4] Filtering interface name from program arguments
From: Michal Labedzki @ 2010-12-28 9:06 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michal Labedzki
In-Reply-To: <AANLkTi%3dzo%3dpgCCgJ%3dsL9bUftf6Vv7nO7xac92zU6gECC%40mail.gmail.com>
tools: Name must be "hciX", where X is 0..HCI_MAX_DEV. Any other like "tty1,
"hci001" or "hci1x" will no longer work as "hci1".
tools: Add info that "bdaddr" can be put for parameter "interface".
---
attrib/gatttool.c | 9 ++++++---
compat/pand.c | 2 +-
lib/hci.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------
lib/hci_lib.h | 1 +
test/hciemu.c | 9 +++++----
test/l2test.c | 8 +++++---
test/rctest.c | 10 ++++++----
tools/ciptool.c | 9 +++++----
tools/hciconfig.c | 8 +++++++-
tools/hcitool.c | 4 ++--
tools/l2ping.c | 11 +++++++----
tools/main.c | 7 ++++---
tools/sdptool.c | 9 +++++----
13 files changed, 98 insertions(+), 41 deletions(-)
diff --git a/attrib/gatttool.c b/attrib/gatttool.c
index a234e36..43b4f57 100644
--- a/attrib/gatttool.c
+++ b/attrib/gatttool.c
@@ -105,10 +105,13 @@ static GIOChannel *do_connect(gboolean le)
/* Local adapter */
if (opt_src != NULL) {
- if (!strncmp(opt_src, "hci", 3))
- hci_devba(atoi(opt_src + 3), &sba);
- else
+ int devid;
+
+ devid = hci_filter_devname(opt_src);
+ if (devid < 0)
str2ba(opt_src, &sba);
+ else
+ hci_devba(devid, &sba);
} else
bacpy(&sba, BDADDR_ANY);
diff --git a/compat/pand.c b/compat/pand.c
index c3860fa..7331fad 100644
--- a/compat/pand.c
+++ b/compat/pand.c
@@ -586,7 +586,7 @@ static const char *main_help =
"\t--role -r <role> Local PAN role (PANU, NAP, GN)\n"
"\t--service -d <role> Remote PAN service (PANU, NAP, GN)\n"
"\t--ethernet -e <name> Network interface name\n"
- "\t--device -i <bdaddr> Source bdaddr\n"
+ "\t--device -i <hciX|bdaddr> Source interface or bdaddr\n"
"\t--nosdp -D Disable SDP\n"
"\t--auth -A Enable authentication\n"
"\t--encrypt -E Enable encryption\n"
diff --git a/lib/hci.c b/lib/hci.c
index 048fda4..fb29f87 100644
--- a/lib/hci.c
+++ b/lib/hci.c
@@ -887,22 +887,56 @@ int hci_get_route(bdaddr_t *bdaddr)
(long) (bdaddr ? bdaddr : BDADDR_ANY));
}
+/* return 1 if string is decimal number without leading zeros
+ * return 0 if not */
+static int is_devnumber(const char *c)
+{
+ if (c[0] == '0' && c[1] != 0)
+ return 0;
+
+ while (*c) {
+ if (*c < '0' || *c > '9')
+ return 0;
+ ++c;
+ }
+ return 1;
+}
+
+/* return HCI dev_id from string like "hciX", where X is dev_id
+ * return -1 if str has invalid format */
+int hci_filter_devname(const char *str)
+{
+ int dev_id;
+
+ if ((strlen(str) >= 4) && (!strncmp(str, "hci", 3)) &&
+ (is_devnumber(str + 3)))
+ dev_id = atoi(str + 3);
+ else
+ dev_id = -1;
+
+ if (dev_id >= HCI_MAX_DEV)
+ dev_id = -1;
+
+ return dev_id;
+}
+
+/* return dev_id, which is on UP state, from 'hciX' or 'bdaddr'
+ * otherwise return -1 */
int hci_devid(const char *str)
{
bdaddr_t ba;
- int id = -1;
+ int dev_id;
- if (!strncmp(str, "hci", 3) && strlen(str) >= 4) {
- id = atoi(str + 3);
- if (hci_devba(id, &ba) < 0)
- return -1;
- } else {
+ dev_id = hci_filter_devname(str);
+ if (dev_id < 0) {
errno = ENODEV;
str2ba(str, &ba);
- id = hci_for_each_dev(HCI_UP, __same_bdaddr, (long) &ba);
+ dev_id = hci_for_each_dev(HCI_UP, __same_bdaddr, (long) &ba);
+ } else if (hci_devba(dev_id, &ba) < 0) {
+ return -1;
}
- return id;
+ return dev_id;
}
int hci_devinfo(int dev_id, struct hci_dev_info *di)
@@ -925,6 +959,8 @@ int hci_devinfo(int dev_id, struct hci_dev_info *di)
return ret;
}
+/* return status 0 and bdaddr assigned to dev_id
+ * otherwise status -1 */
int hci_devba(int dev_id, bdaddr_t *bdaddr)
{
struct hci_dev_info di;
diff --git a/lib/hci_lib.h b/lib/hci_lib.h
index b63a2a4..ef00f99 100644
--- a/lib/hci_lib.h
+++ b/lib/hci_lib.h
@@ -60,6 +60,7 @@ int hci_inquiry(int dev_id, int len, int num_rsp, const uint8_t *lap, inquiry_in
int hci_devinfo(int dev_id, struct hci_dev_info *di);
int hci_devba(int dev_id, bdaddr_t *bdaddr);
int hci_devid(const char *str);
+int hci_filter_devname(const char *str);
int hci_read_local_name(int dd, int len, char *name, int to);
int hci_write_local_name(int dd, const char *name, int to);
diff --git a/test/hciemu.c b/test/hciemu.c
index a20374f..ae33d72 100644
--- a/test/hciemu.c
+++ b/test/hciemu.c
@@ -1234,15 +1234,16 @@ int main(int argc, char *argv[])
exit(1);
}
- if (strlen(argv[0]) > 3 && !strncasecmp(argv[0], "hci", 3)) {
+ dev = hci_filter_devname(argv[0]);
+ if (dev < 0) {
+ if (getbdaddrbyname(argv[0], &vdev.bdaddr) < 0)
+ exit(1);
+ } else {
dev = hci_devid(argv[0]);
if (dev < 0) {
perror("Invalid device");
exit(1);
}
- } else {
- if (getbdaddrbyname(argv[0], &vdev.bdaddr) < 0)
- exit(1);
}
if (detach) {
diff --git a/test/l2test.c b/test/l2test.c
index 17883a9..438ba39 100644
--- a/test/l2test.c
+++ b/test/l2test.c
@@ -1101,6 +1101,7 @@ int main(int argc, char *argv[])
{
struct sigaction sa;
int opt, sk, mode = RECV, need_addr = 0;
+ int devid;
bacpy(&bdaddr, BDADDR_ANY);
@@ -1175,10 +1176,11 @@ int main(int argc, char *argv[])
break;
case 'i':
- if (!strncasecmp(optarg, "hci", 3))
- hci_devba(atoi(optarg + 3), &bdaddr);
- else
+ devid = hci_filter_devname(optarg);
+ if (devid < 0)
str2ba(optarg, &bdaddr);
+ else
+ hci_devba(devid, &bdaddr);
break;
case 'P':
diff --git a/test/rctest.c b/test/rctest.c
index b3804f5..a828ad9 100644
--- a/test/rctest.c
+++ b/test/rctest.c
@@ -579,7 +579,7 @@ static void usage(void)
"\t-m multiple connects\n");
printf("Options:\n"
- "\t[-b bytes] [-i device] [-P channel] [-U uuid]\n"
+ "\t[-b bytes] [-i hciX|bdaddr] [-P channel] [-U uuid]\n"
"\t[-L seconds] enabled SO_LINGER option\n"
"\t[-W seconds] enable deferred setup\n"
"\t[-B filename] use data packets from file\n"
@@ -597,6 +597,7 @@ int main(int argc, char *argv[])
{
struct sigaction sa;
int opt, sk, mode = RECV, need_addr = 0;
+ int devid;
bacpy(&bdaddr, BDADDR_ANY);
@@ -644,10 +645,11 @@ int main(int argc, char *argv[])
break;
case 'i':
- if (!strncasecmp(optarg, "hci", 3))
- hci_devba(atoi(optarg + 3), &bdaddr);
- else
+ devid = hci_filter_devname(optarg);
+ if (devid < 0)
str2ba(optarg, &bdaddr);
+ else
+ hci_devba(devid, &bdaddr);
break;
case 'P':
diff --git a/tools/ciptool.c b/tools/ciptool.c
index edce9da..ec602ef 100644
--- a/tools/ciptool.c
+++ b/tools/ciptool.c
@@ -427,7 +427,7 @@ static void usage(void)
"\n");
printf("Options:\n"
- "\t-i [hciX|bdaddr] Local HCI device or BD Address\n"
+ "\t-i <hciX|bdaddr> Local HCI device or BD Address\n"
"\t-h, --help Display help\n"
"\n");
@@ -455,10 +455,11 @@ int main(int argc, char *argv[])
while ((opt = getopt_long(argc, argv, "+i:h", main_options, NULL)) != -1) {
switch(opt) {
case 'i':
- if (!strncmp(optarg, "hci", 3))
- hci_devba(atoi(optarg + 3), &bdaddr);
- else
+ i = hci_filter_devname(optarg);
+ if (i < 0)
str2ba(optarg, &bdaddr);
+ else
+ hci_devba(i, &bdaddr);
break;
case 'h':
usage();
diff --git a/tools/hciconfig.c b/tools/hciconfig.c
index f0ae11c..e20963d 100644
--- a/tools/hciconfig.c
+++ b/tools/hciconfig.c
@@ -1849,7 +1849,13 @@ int main(int argc, char *argv[])
exit(0);
}
- di.dev_id = atoi(argv[0] + 3);
+ i = hci_filter_devname(argv[0]);
+ if (i < 0) {
+ fprintf(stderr, "No valid device name.\n");
+ exit(1);
+ }
+ di.dev_id = i;
+
argc--; argv++;
if (ioctl(ctl, HCIGETDEVINFO, (void *) &di)) {
diff --git a/tools/hcitool.c b/tools/hcitool.c
index d50adaf..dc70e63 100644
--- a/tools/hcitool.c
+++ b/tools/hcitool.c
@@ -2560,8 +2560,8 @@ static void usage(void)
printf("Usage:\n"
"\thcitool [options] <command> [command parameters]\n");
printf("Options:\n"
- "\t--help\tDisplay help\n"
- "\t-i dev\tHCI device\n");
+ "\t--help Display help\n"
+ "\t-i <hciX|bdaddr> Local HCI device or BD Address\n");
printf("Commands:\n");
for (i = 0; command[i].cmd; i++)
printf("\t%-4s\t%s\n", command[i].cmd,
diff --git a/tools/l2ping.c b/tools/l2ping.c
index 29fb3d0..58c5faf 100644
--- a/tools/l2ping.c
+++ b/tools/l2ping.c
@@ -255,7 +255,8 @@ static void usage(void)
{
printf("l2ping - L2CAP ping\n");
printf("Usage:\n");
- printf("\tl2ping [-i device] [-s size] [-c count] [-t timeout] [-d delay] [-f] [-r] [-v] <bdaddr>\n");
+ printf("\tl2ping [-i <hciX|bdaddr>] [-s size] [-c count] [-t timeout] "
+ "[-d delay] [-f] [-r] [-v] <bdaddr>\n");
printf("\t-f Flood ping (delay = 0)\n");
printf("\t-r Reverse ping\n");
printf("\t-v Verify request and response payload\n");
@@ -264,6 +265,7 @@ static void usage(void)
int main(int argc, char *argv[])
{
int opt;
+ int devid;
/* Default options */
bacpy(&bdaddr, BDADDR_ANY);
@@ -271,10 +273,11 @@ int main(int argc, char *argv[])
while ((opt=getopt(argc,argv,"i:d:s:c:t:frv")) != EOF) {
switch(opt) {
case 'i':
- if (!strncasecmp(optarg, "hci", 3))
- hci_devba(atoi(optarg + 3), &bdaddr);
- else
+ devid = hci_filter_devname(optarg);
+ if (devid < 0)
str2ba(optarg, &bdaddr);
+ else
+ hci_devba(devid, &bdaddr);
break;
case 'd':
diff --git a/tools/main.c b/tools/main.c
index 6800445..c000fba 100644
--- a/tools/main.c
+++ b/tools/main.c
@@ -753,10 +753,11 @@ int main(int argc, char *argv[])
while ((opt = getopt_long(argc, argv, "+i:f:rahAESML:", main_options, NULL)) != -1) {
switch(opt) {
case 'i':
- if (strncmp(optarg, "hci", 3) == 0)
- hci_devba(atoi(optarg + 3), &bdaddr);
- else
+ dev_id = hci_filter_devname(optarg);
+ if (dev_id < 0)
str2ba(optarg, &bdaddr);
+ else
+ hci_devba(dev_id, &bdaddr);
break;
case 'f':
diff --git a/tools/sdptool.c b/tools/sdptool.c
index 140a46a..c782e62 100644
--- a/tools/sdptool.c
+++ b/tools/sdptool.c
@@ -4209,7 +4209,7 @@ static void usage(void)
"\tsdptool [options] <command> [command parameters]\n");
printf("Options:\n"
"\t-h\t\tDisplay help\n"
- "\t-i\t\tSpecify source interface\n");
+ "\t-i\t\tSpecify source interface or bdaddr\n");
printf("Commands:\n");
for (i = 0; command[i].cmd; i++)
@@ -4242,10 +4242,11 @@ int main(int argc, char *argv[])
while ((opt=getopt_long(argc, argv, "+i:h", main_options, NULL)) != -1) {
switch(opt) {
case 'i':
- if (!strncmp(optarg, "hci", 3))
- hci_devba(atoi(optarg + 3), &interface);
- else
+ i = hci_filter_devname(optarg);
+ if (i < 0)
str2ba(optarg, &interface);
+ else
+ hci_devba(i, &interface);
break;
case 'h':
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH v2] Fix leak of EIR data if RSSI does not change
From: Anderson Lizardo @ 2010-12-27 22:24 UTC (permalink / raw)
To: Anderson Lizardo, linux-bluetooth
In-Reply-To: <20101227214841.GA22777@jh-x301>
Hi Johan,
On Mon, Dec 27, 2010 at 5:48 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Beware of words like "also" and "and" in commit messages. Often they are
> a good indication that the patch shold be split into multiple ones.
> Though in this case I can't really make up my mind if it's fine since
> the changes are so strongly related. I'll leave it up to you to think
> about it and decide if you can cleanly split this into two patches or
> not.
I can split into two patches without problem.
> It looks to me like shortened names are completely ignored after this
> patch, even to the extent that if there's a shortened name the memory
> will be leaked if adapter_update_found_devices returns TRUE.
>
> If we don't have any name in storage and all we have is a shortened name
> then I think it should be included in the DeviceFound signal (which is
> what the existing code does).
You are right. Somehow I forgot about the shortened name case while
trying to refactor this part of the code.
I'll resend the patch with just the memory leak fix (taking care to
not introduce another one!) and if I still have any idea to simplify
this code (which I don't believe anymore) a second patch will come.
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH v2] Fix leak of EIR data if RSSI does not change
From: Johan Hedberg @ 2010-12-27 21:48 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293473827-26418-1-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> If RSSI value does not change, memory used by parsed EIR data would leak
> because it would not be assigned to the remote_dev_info structure.
>
> Also simplify related code and replace a couple of g_free()'s with
> free() (simply because they were allocated with malloc()).
Beware of words like "also" and "and" in commit messages. Often they are
a good indication that the patch shold be split into multiple ones.
Though in this case I can't really make up my mind if it's fine since
the changes are so strongly related. I'll leave it up to you to think
about it and decide if you can cleanly split this into two patches or
not.
> @@ -529,6 +537,14 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
> else
> name_status = NAME_NOT_REQUIRED;
>
> + /* Update storage if EIR contains the complete device name */
> + if (eir_data.name && eir_data.name_complete) {
> + write_device_name(local, peer, eir_data.name);
> + name_status = NAME_NOT_REQUIRED;
> + g_free(eir_data.name);
> + eir_data.name = NULL;
> + }
> +
> create_name(filename, PATH_MAX, STORAGEDIR, local_addr, "aliases");
> alias = textfile_get(filename, peer_addr);
>
> @@ -547,28 +563,13 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
> } else
> legacy = TRUE;
>
> - if (eir_data.name) {
> - if (eir_data.name_complete) {
> - write_device_name(local, peer, eir_data.name);
> - name_status = NAME_NOT_REQUIRED;
> -
> - if (name)
> - g_free(name);
> -
> - name = eir_data.name;
> - } else {
> - if (name)
> - free(eir_data.name);
> - else
> - name = eir_data.name;
> - }
> - }
> -
> - adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
> - legacy, eir_data.services, name_status);
> + if (!adapter_update_found_devices(adapter, peer, rssi, class, name,
> + alias, legacy, eir_data.services,
> + name_status))
> + free_eir_data(&eir_data);
>
> - g_free(name);
> - g_free(alias);
> + free(name);
> + free(alias);
> }
>
> void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer,
It looks to me like shortened names are completely ignored after this
patch, even to the extent that if there's a shortened name the memory
will be leaked if adapter_update_found_devices returns TRUE.
If we don't have any name in storage and all we have is a shortened name
then I think it should be included in the DeviceFound signal (which is
what the existing code does).
Johan
^ permalink raw reply
* Re: [PATCH 4/4] Fix leak of EIR data if RSSI does not change
From: Anderson Lizardo @ 2010-12-27 18:23 UTC (permalink / raw)
To: Anderson Lizardo, linux-bluetooth
In-Reply-To: <20101227151122.GD17203@jh-x301>
Hi Johan,
On Mon, Dec 27, 2010 at 11:11 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> In general this seems fine, but to avoid future memory leaks in case you
> go ahead and change the eir_data struct wouldn't it be better to have a
> separate free_eir_data() function which does the g_slist_foreach,
> g_slist_free and g_free?
I followed your suggestion and created a free_eir_data(). I also did a
simple code reordering that simplified the complete EIR name storage.
See v2 of this patch. (2/4 should be ignored).
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Anderson Lizardo @ 2010-12-27 18:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1293456068-11615-2-git-send-email-anderson.lizardo@openbossa.org>
This patch should be ignored. Instead, I incorporated a small
refactoring on v2 of patch 4/4.
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* [PATCH v2] Fix leak of EIR data if RSSI does not change
From: Anderson Lizardo @ 2010-12-27 18:17 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1293456068-11615-4-git-send-email-anderson.lizardo@openbossa.org>
If RSSI value does not change, memory used by parsed EIR data would leak
because it would not be assigned to the remote_dev_info structure.
Also simplify related code and replace a couple of g_free()'s with
free() (simply because they were allocated with malloc()).
---
src/adapter.c | 19 ++++++++++++-------
src/adapter.h | 11 ++++++-----
src/event.c | 47 ++++++++++++++++++++++++-----------------------
3 files changed, 42 insertions(+), 35 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index bfc2b8b..4b10189 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2917,7 +2917,7 @@ static void remove_same_uuid(gpointer data, gpointer user_data)
}
}
-void adapter_update_device_from_info(struct btd_adapter *adapter,
+gboolean adapter_update_device_from_info(struct btd_adapter *adapter,
bdaddr_t bdaddr, int8_t rssi,
uint8_t evt_type, char *name,
GSList *services, uint8_t flags)
@@ -2931,7 +2931,7 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
dev->le = TRUE;
dev->evt_type = evt_type;
} else if (dev->rssi == rssi)
- return;
+ return FALSE;
dev->rssi = rssi;
@@ -2951,12 +2951,15 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
/* FIXME: check if other information was changed before emitting the
* signal */
adapter_emit_device_found(adapter, dev);
+
+ return TRUE;
}
-void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
- int8_t rssi, uint32_t class, const char *name,
- const char *alias, gboolean legacy,
- GSList *services, name_status_t name_status)
+gboolean adapter_update_found_devices(struct btd_adapter *adapter,
+ bdaddr_t *bdaddr, int8_t rssi, uint32_t class,
+ const char *name, const char *alias,
+ gboolean legacy, GSList *services,
+ name_status_t name_status)
{
struct remote_dev_info *dev;
gboolean new_dev;
@@ -2975,7 +2978,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
dev->legacy = legacy;
dev->name_status = name_status;
} else if (dev->rssi == rssi)
- return;
+ return FALSE;
dev->rssi = rssi;
@@ -2986,6 +2989,8 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
dev->services = g_slist_concat(dev->services, services);
adapter_emit_device_found(adapter, dev);
+
+ return TRUE;
}
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr)
diff --git a/src/adapter.h b/src/adapter.h
index 857eec8..cee0bd4 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -134,14 +134,15 @@ int adapter_get_state(struct btd_adapter *adapter);
int adapter_get_discover_type(struct btd_adapter *adapter);
struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter,
struct remote_dev_info *match);
-void adapter_update_device_from_info(struct btd_adapter *adapter,
+gboolean adapter_update_device_from_info(struct btd_adapter *adapter,
bdaddr_t bdaddr, int8_t rssi,
uint8_t evt_type, char *name,
GSList *services, uint8_t flags);
-void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
- int8_t rssi, uint32_t class, const char *name,
- const char *alias, gboolean legacy,
- GSList *services, name_status_t name_status);
+gboolean adapter_update_found_devices(struct btd_adapter *adapter,
+ bdaddr_t *bdaddr, int8_t rssi, uint32_t class,
+ const char *name, const char *alias,
+ gboolean legacy, GSList *services,
+ name_status_t name_status);
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr);
void adapter_emit_device_found(struct btd_adapter *adapter,
struct remote_dev_info *dev);
diff --git a/src/event.c b/src/event.c
index 178cea8..5beedb8 100644
--- a/src/event.c
+++ b/src/event.c
@@ -431,6 +431,13 @@ static int parse_eir_data(struct eir_data *eir, uint8_t *eir_data,
return 0;
}
+static void free_eir_data(struct eir_data *eir)
+{
+ g_slist_foreach(eir->services, (GFunc) g_free, NULL);
+ g_slist_free(eir->services);
+ g_free(eir->name);
+}
+
void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
{
struct btd_adapter *adapter;
@@ -452,9 +459,10 @@ void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
rssi = *(info->data + info->length);
- adapter_update_device_from_info(adapter, info->bdaddr, rssi,
+ if (!adapter_update_device_from_info(adapter, info->bdaddr, rssi,
info->evt_type, eir_data.name,
- eir_data.services, eir_data.flags);
+ eir_data.services, eir_data.flags))
+ free_eir_data(&eir_data);
}
static void update_lastseen(bdaddr_t *sba, bdaddr_t *dba)
@@ -529,6 +537,14 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
else
name_status = NAME_NOT_REQUIRED;
+ /* Update storage if EIR contains the complete device name */
+ if (eir_data.name && eir_data.name_complete) {
+ write_device_name(local, peer, eir_data.name);
+ name_status = NAME_NOT_REQUIRED;
+ g_free(eir_data.name);
+ eir_data.name = NULL;
+ }
+
create_name(filename, PATH_MAX, STORAGEDIR, local_addr, "aliases");
alias = textfile_get(filename, peer_addr);
@@ -547,28 +563,13 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
} else
legacy = TRUE;
- if (eir_data.name) {
- if (eir_data.name_complete) {
- write_device_name(local, peer, eir_data.name);
- name_status = NAME_NOT_REQUIRED;
-
- if (name)
- g_free(name);
-
- name = eir_data.name;
- } else {
- if (name)
- free(eir_data.name);
- else
- name = eir_data.name;
- }
- }
-
- adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
- legacy, eir_data.services, name_status);
+ if (!adapter_update_found_devices(adapter, peer, rssi, class, name,
+ alias, legacy, eir_data.services,
+ name_status))
+ free_eir_data(&eir_data);
- g_free(name);
- g_free(alias);
+ free(name);
+ free(alias);
}
void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer,
--
1.7.0.4
^ permalink raw reply related
* [PATCH] Bluetooth: l2cap: fix misuse of logical operation in place of bitop
From: David Sterba @ 2010-12-27 18:00 UTC (permalink / raw)
To: padovan
Cc: linux-bluetooth, netdev, linux-kernel, David Sterba,
Marcel Holtmann, João Paulo Rechi Vita
CC: Marcel Holtmann <marcel@holtmann.org>
CC: "Gustavo F. Padovan" <padovan@profusion.mobi>
CC: João Paulo Rechi Vita <jprvita@profusion.mobi>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
net/bluetooth/l2cap.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index cd8f6ea..bdfdfdc 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1893,8 +1893,8 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms
if (pi->mode == L2CAP_MODE_STREAMING) {
l2cap_streaming_send(sk);
} else {
- if (pi->conn_state & L2CAP_CONN_REMOTE_BUSY &&
- pi->conn_state && L2CAP_CONN_WAIT_F) {
+ if ((pi->conn_state & L2CAP_CONN_REMOTE_BUSY) &&
+ (pi->conn_state & L2CAP_CONN_WAIT_F)) {
err = len;
break;
}
--
1.7.3.4.626.g73e7b
^ permalink raw reply related
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Anderson Lizardo @ 2010-12-27 15:33 UTC (permalink / raw)
To: Anderson Lizardo, Luiz Augusto von Dentz, linux-bluetooth
In-Reply-To: <20101227145322.GA16529@jh-x301>
Hi,
On Mon, Dec 27, 2010 at 10:53 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Mon, Dec 27, 2010, Anderson Lizardo wrote:
>> > if the variable name is set
>> > than your could would skip eir name completely while the code did
>> > actually use the eir name, probably the eir name is updated more
>> > frequently so it is probably the most updated one and we should in
>> > fact update the storage if they don't match.
>>
>> I could not find on spec any mention that the "complete" EIR name may
>> change.
>
> It also doesn't say that it will not change. So the safe thing would be
> not to make any assumption about its permanence, right?
Agreed. I'll adapt this patch in a attempt to at least simplify the
current logic.
> Naturally the user of the remote device might change is name (if the
> remote device has such a feature) and then the EIR data would change
> too. Note that even if there's a name in storage it doesn't mean that
> it's from the same discovery session. It might be from a previous
> discovery session a long time ago and the user of the remote device
> might have changed its name since then. So imho a complete name in an
> EIR should always override whatever is in storage.
Ok, I'll keep this semantics but try to simplify the current logic.
> For the case of shortened names I don't think there's an indisputably
> right answer. The question is what is better: that which is "complete"
> or that which is more recent. I've got the feeling that the complete
> version would be better for the user, but someone might of course
> disagree with this.
For now, I'll keep the current (from master) behavior of only storing
complete EIR names and using the stored name in favor of the shortened
one. My intent is not to change behavior unless there is some need to
do so.
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Anderson Lizardo @ 2010-12-27 15:21 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <AANLkTi=06P4ZpGO-sp7czZtn9FGLkBwPnrxNqDdGspfy@mail.gmail.com>
Hi,
On Mon, Dec 27, 2010 at 10:35 AM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> I've not found any references to the spec saying the shortened name
> has to be a prefix. I believe it is left to the implementer how to
> display such shortened names. bluez so far has made no distinction on
> the display of shortened names vs. complete names.
Interestingly, on Volume 3, Part C, 11.1.2, it is mentioned that for
Advertising Data (i.e. LE devices):
"A shortened name shall only contain contiguous
characters from the beginning of the full name. For example, if the device name
is ‘BT_Device_Name’ then the shortened name over BR/EDR could be
‘BT_Device’ while the shortened name on LE could be ‘BT_Dev’."
Nothing is said on the EIR format, though (see Vol 3, Part C, 8.1.2).
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [RFCv2] Bluetooth: Use non-flushable by default L2CAP data packets
From: Andrei Emeltchenko @ 2010-12-27 15:13 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20101223020615.GA26284@vigoh>
Hi Gustavo,
On Thu, Dec 23, 2010 at 4:06 AM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Andrei,
>
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-12-16 16:54:26 +0200]:
>
>> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>
>> Modification of Nick Pelly <npelly@google.com> patch.
>>
>> With Bluetooth 2.1 ACL packets can be flushable or non-flushable. This commit
>> makes ACL data packets non-flushable by default on compatible chipsets, and
>> adds the BT_FLUSHABLE socket option to explicitly request flushable ACL
>> data packets for a given L2CAP socket. This is useful for A2DP data which can
>> be safely discarded if it can not be delivered within a short time (while
>> other ACL data should not be discarded).
>>
>> Note that making ACL data flushable has no effect unless the automatic flush
>> timeout for that ACL link is changed from its default of 0 (infinite).
>>
>> Default packet types (for compatible chipsets):
>> Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
>> Bluetooth HCI H4
>> Bluetooth HCI ACL Packet
>> .... 0000 0000 0010 = Connection Handle: 0x0002
>> ..00 .... .... .... = PB Flag: First Non-automatically Flushable Packet (0)
>> 00.. .... .... .... = BC Flag: Point-To-Point (0)
>> Data Total Length: 8
>> Bluetooth L2CAP Packet
>>
>> After setting BT_FLUSHABLE
>> (sock.setsockopt(274 /*SOL_BLUETOOTH*/, 8 /* BT_FLUSHABLE */, 1 /* flush */))
>> Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
>> Bluetooth HCI H4
>> Bluetooth HCI ACL Packet
>> .... 0000 0000 0010 = Connection Handle: 0x0002
>> ..10 .... .... .... = PB Flag: First Automatically Flushable Packet (2)
>> 00.. .... .... .... = BC Flag: Point-To-Point (0)
>> Data Total Length: 8
>> Bluetooth L2CAP Packet
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> ---
>> include/net/bluetooth/bluetooth.h | 5 ++++
>> include/net/bluetooth/hci.h | 2 +
>> include/net/bluetooth/hci_core.h | 1 +
>> include/net/bluetooth/l2cap.h | 2 +
>> net/bluetooth/hci_core.c | 6 +++-
>> net/bluetooth/l2cap.c | 48 ++++++++++++++++++++++++++++++++++--
>> 6 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 0c5e725..ed7d775 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -64,6 +64,11 @@ struct bt_security {
>>
>> #define BT_DEFER_SETUP 7
>>
>> +#define BT_FLUSHABLE 8
>> +
>> +#define BT_FLUSHABLE_OFF 0
>> +#define BT_FLUSHABLE_ON 1
>> +
>> #define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" , ## arg)
>> #define BT_ERR(fmt, arg...) printk(KERN_ERR "%s: " fmt "\n" , __func__ , ## arg)
>> #define BT_DBG(fmt, arg...) pr_debug("%s: " fmt "\n" , __func__ , ## arg)
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index 29a7a8c..333d5cb 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -150,6 +150,7 @@ enum {
>> #define EDR_ESCO_MASK (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
>>
>> /* ACL flags */
>> +#define ACL_START_NO_FLUSH 0x00
>> #define ACL_CONT 0x01
>> #define ACL_START 0x02
>> #define ACL_ACTIVE_BCAST 0x04
>> @@ -193,6 +194,7 @@ enum {
>> #define LMP_EDR_ESCO_3M 0x40
>> #define LMP_EDR_3S_ESCO 0x80
>>
>> +#define LMP_NO_FLUSH 0x01
>
> Isn't this 0x40? As stated on Core v4.0 (Volume 2, part C, 3.3 Feature Mask
> Definition)
>
>> #define LMP_SIMPLE_PAIR 0x08
>>
>> /* Connection modes */
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 1992fac..9778bc8 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -456,6 +456,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>> #define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
>> #define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
>> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
>> +#define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
>>
>> /* ----- HCI protocols ----- */
>> struct hci_proto {
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 7ad25ca..af35711 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -75,6 +75,7 @@ struct l2cap_conninfo {
>> #define L2CAP_LM_TRUSTED 0x0008
>> #define L2CAP_LM_RELIABLE 0x0010
>> #define L2CAP_LM_SECURE 0x0020
>> +#define L2CAP_LM_FLUSHABLE 0x0040
>
> Not using this flag in the code.
>
>>
>> /* L2CAP command codes */
>> #define L2CAP_COMMAND_REJ 0x01
>> @@ -327,6 +328,7 @@ struct l2cap_pinfo {
>> __u8 sec_level;
>> __u8 role_switch;
>> __u8 force_reliable;
>> + __u8 flushable;
>>
>> __u8 conf_req[64];
>> __u8 conf_len;
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 51c61f7..c0d776b 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1380,7 +1380,7 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>>
>> skb->dev = (void *) hdev;
>> bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> - hci_add_acl_hdr(skb, conn->handle, flags | ACL_START);
>> + hci_add_acl_hdr(skb, conn->handle, flags);
>>
>> list = skb_shinfo(skb)->frag_list;
>> if (!list) {
>> @@ -1398,12 +1398,14 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>> spin_lock_bh(&conn->data_q.lock);
>>
>> __skb_queue_tail(&conn->data_q, skb);
>
> add an empty line here.
>
>> + flags &= ~ACL_START;
>> + flags |= ACL_CONT;
>> do {
>> skb = list; list = list->next;
>>
>> skb->dev = (void *) hdev;
>> bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> - hci_add_acl_hdr(skb, conn->handle, flags | ACL_CONT);
>> + hci_add_acl_hdr(skb, conn->handle, flags);
>>
>> BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index c791fcd..efa60eb 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
>> static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
>> {
>> struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
>> + u8 flags;
>>
>> BT_DBG("code 0x%2.2x", code);
>>
>> if (!skb)
>> return;
>>
>> - hci_send_acl(conn->hcon, skb, 0);
>> + if (lmp_no_flush_capable(conn->hcon->hdev))
>> + flags = ACL_START_NO_FLUSH;
>> + else
>> + flags = ACL_START;
>> +
>> + hci_send_acl(conn->hcon, skb, flags);
>> }
>>
>> static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
>> @@ -900,6 +906,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
>> pi->sec_level = l2cap_pi(parent)->sec_level;
>> pi->role_switch = l2cap_pi(parent)->role_switch;
>> pi->force_reliable = l2cap_pi(parent)->force_reliable;
>> + pi->flushable = l2cap_pi(parent)->flushable;
>> } else {
>> pi->imtu = L2CAP_DEFAULT_MTU;
>> pi->omtu = 0;
>> @@ -915,6 +922,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
>> pi->sec_level = BT_SECURITY_LOW;
>> pi->role_switch = 0;
>> pi->force_reliable = 0;
>> + pi->flushable = BT_FLUSHABLE_OFF;
>
> You need to check for the no_flush feature here, right?
we have here uninitialized socket with no access to conn->hcon->hdev.
I am thinking about adding check below:
if (parent) {
...
if (lmp_no_flush_capable(hcon->hdev))
pi->flushable = BT_FLUSHABLE_OFF;
else
pi->flushable = BT_FLUSHABLE_ON;
else {
...
pi->flushable = BT_FLUSHABLE_OFF;
}
then we could skip checking for lmp_no_flush_capable in l2cap_do_send
Regards,
Andrei
>
>> }
>>
>> /* Default config options */
>> @@ -1450,10 +1458,17 @@ static void l2cap_drop_acked_frames(struct sock *sk)
>> static inline void l2cap_do_send(struct sock *sk, struct sk_buff *skb)
>> {
>> struct l2cap_pinfo *pi = l2cap_pi(sk);
>> + struct hci_conn *hcon = pi->conn->hcon;
>> + u16 flags;
>>
>> BT_DBG("sk %p, skb %p len %d", sk, skb, skb->len);
>>
>> - hci_send_acl(pi->conn->hcon, skb, 0);
>> + if (lmp_no_flush_capable(hcon->hdev) && !l2cap_pi(sk)->flushable)
>> + flags = ACL_START_NO_FLUSH;
>> + else
>> + flags = ACL_START;
>> +
>> + hci_send_acl(hcon, skb, flags);
>
> There is another call to hci_send_acl() in l2cap. It is in
> l2cap_send_sframe(), but I'm still wondering if we shall flush those packets
> or not in the case flushable was set. Now I'm thinking that don't.
>
> Then you need to add the same check in l2cap_send_cmd to l2cap_send_sframe().
>
>> }
>>
>> static void l2cap_streaming_send(struct sock *sk)
>> @@ -2045,6 +2060,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
>> static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, char __user *optval, unsigned int optlen)
>> {
>> struct sock *sk = sock->sk;
>> + struct hci_conn *hcon = l2cap_pi(sk)->conn->hcon;
>> struct bt_security sec;
>> int len, err = 0;
>> u32 opt;
>> @@ -2098,6 +2114,26 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
>> bt_sk(sk)->defer_setup = opt;
>> break;
>>
>> + case BT_FLUSHABLE:
>> + if (get_user(opt, (u32 __user *) optval)) {
>> + err = -EFAULT;
>> + break;
>> + }
>> +
>> + if (opt > BT_FLUSHABLE_ON) {
>> + err = -EINVAL;
>> + break;
>> + }
>> +
>> + if (opt == BT_FLUSHABLE_OFF &&
>> + !lmp_no_flush_capable(hcon->hdev)) {
>
> I'm not really happy with !lmp_no_flush... 2 negatives in the same place, but
> that is the feature name. So we can't do too much here. Btw, the "No" in the
> feature name is making my brain hurt. ;)
>
> regards,
>
> --
> Gustavo F. Padovan
> http://profusion.mobi
>
^ permalink raw reply
* Re: [PATCH 4/4] Fix leak of EIR data if RSSI does not change
From: Johan Hedberg @ 2010-12-27 15:11 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293456068-11615-4-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> --- a/src/event.c
> +++ b/src/event.c
> @@ -452,9 +452,13 @@ void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
>
> rssi = *(info->data + info->length);
>
> - adapter_update_device_from_info(adapter, info->bdaddr, rssi,
> + if (!adapter_update_device_from_info(adapter, info->bdaddr, rssi,
> info->evt_type, eir_data.name,
> - eir_data.services, eir_data.flags);
> + eir_data.services, eir_data.flags)) {
> + g_slist_foreach(eir_data.services, (GFunc) g_free, NULL);
> + g_slist_free(eir_data.services);
> + g_free(eir_data.name);
> + }
> }
>
> static void update_lastseen(bdaddr_t *sba, bdaddr_t *dba)
> @@ -556,8 +560,12 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
> } else if (eir_data.name != NULL)
> g_free(eir_data.name);
>
> - adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
> - legacy, eir_data.services, name_status);
> + if (!adapter_update_found_devices(adapter, peer, rssi, class, name,
> + alias, legacy, eir_data.services,
> + name_status)) {
> + g_slist_foreach(eir_data.services, (GFunc) g_free, NULL);
> + g_slist_free(eir_data.services);
> + }
>
> g_free(name);
> g_free(alias);
In general this seems fine, but to avoid future memory leaks in case you
go ahead and change the eir_data struct wouldn't it be better to have a
separate free_eir_data() function which does the g_slist_foreach,
g_slist_free and g_free?
Johan
^ permalink raw reply
* Re: [PATCH 3/4] Remove outdated comments
From: Johan Hedberg @ 2010-12-27 15:09 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293456068-11615-3-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> ---
> src/event.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
The patch has been pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 1/4] Remove ancient NAME_SENT name resolution status
From: Johan Hedberg @ 2010-12-27 15:08 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293456068-11615-1-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> The NAME_SENT status was introduced on commit
> d6a16516a9f6deae8342f00e8186b02d0019a1e1, when there was a
> "RemoteNameUpdate" D-Bus signal. Nowadays, there is no such signal, and
> the device name (if any) is always sent on "DeviceFound" signal.
> ---
> src/adapter.h | 1 -
> src/event.c | 19 -------------------
> 2 files changed, 0 insertions(+), 20 deletions(-)
Thanks. The patch has been pushed upstream.
Johan
^ permalink raw reply
* Re: [PATCH] Fix printing D-Bus errors when headset record could not be found
From: Johan Hedberg @ 2010-12-27 15:07 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1293452841-7141-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Mon, Dec 27, 2010, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>
> When connection is started via headset_config_stream there is no D-Bus
> message to reply to.
> ---
> audio/headset.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox