Linux bluetooth development
 help / color / mirror / Atom feed
* Re: Problem with an Apple bluetooth keyboard
From: Daniel O'Connor @ 2011-03-05  2:00 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20110305005651.GC9005@joana>


On 05/03/2011, at 11:26, Gustavo F. Padovan wrote:
>> Is there some command I can run to re-probe it as a work around? Or perhaps some debugging I can enable to find out exactly what is going wrong.
> 
> Send to us the output of "hciconfig -a" before you unplug the dongle.

Output is as follows:

hci0:	Type: USB
	BD Address: 00:10:DC:57:76:BE ACL MTU: 192:8 SCO MTU: 64:8
	UP RUNNING PSCAN ISCAN 
	RX bytes:689 acl:0 sco:0 events:22 errors:0
	TX bytes:86 acl:0 sco:0 commands:22 errors:0
	Features: 0xff 0xff 0x0f 0x00 0x00 0x00 0x00 0x00
	Packet type: DM1 DM3 DM5 DH1 DH3 DH5 HV1 HV2 HV3 
	Link policy: 
	Link mode: SLAVE ACCEPT 
	Name: 'MythTV'
	Class: 0x4a0000
	Service Classes: Networking, Capturing, Telephony
	Device Class: Miscellaneous, 
	HCI Ver: 1.1 (0x1) HCI Rev: 0x110 LMP Ver: 1.1 (0x1) LMP Subver: 0x110
	Manufacturer: Cambridge Silicon Radio (10)

Thanks.

--
Daniel O'Connor software and network engineer
for Genesis Software - http://www.gsoft.com.au
"The nice thing about standards is that there
are so many of them to choose from."
  -- Andrew Tanenbaum
GPG Fingerprint - 5596 B766 97C0 0E94 4347 295E E593 DC20 7B3F CE8C







^ permalink raw reply

* Re: Problem with an Apple bluetooth keyboard
From: Gustavo F. Padovan @ 2011-03-05  0:56 UTC (permalink / raw)
  To: Daniel O'Connor; +Cc: linux-bluetooth
In-Reply-To: <AA50FBAC-6B8B-4D61-83A0-0B8D4B24B967@gsoft.com.au>

Hi Daniel,

* Daniel O'Connor <doconnor@gsoft.com.au> [2011-02-22 00:28:17 +1030]:

> Hi,
> I have an Apple BT keyboard talking to Ubuntu 10.04 and it only works if I unplug and replug the dongle.
> 
> dmesg shows the following :-
> 
> [22723.580234] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> [22723.697163] apple 0005:05AC:0239.0002: parse failed
> [22723.697172] apple: probe of 0005:05AC:0239.0002 failed with error -14
> 
> <unplug/replug USB BT dongle.
> 
> [22727.040345] usb 3-1: USB disconnect, address 2
> [22727.041256] btusb_intr_complete: hci0 urb ffff880037286840 failed to resubmit (19)
> [22727.041272] btusb_bulk_complete: hci0 urb ffff8800372860c0 failed to resubmit (19)
> [22727.042257] btusb_bulk_complete: hci0 urb ffff880037286900 failed to resubmit (19)
> [22727.042574] btusb_send_frame: hci0 urb ffff88002bf97a80 submission failed
> [22728.784680] usb 3-1: new full speed USB device using uhci_hcd and address 3
> [22729.170802] usb 3-1: configuration #1 chosen from 1 choice
> [22733.044904] input: darius’s keyboard as /devices/pci0000:00/0000:00:1a.0/usb3/3-1/3-1:1.0/bluetooth/hci0/hci0:40/input8
> [22733.045006] apple 0005:05AC:0239.0003: input,hidraw1: BLUETOOTH HID v0.50 Keyboard [darius’s keyboard] on 00:10:DC:57:76:BE
> 
> Is there some command I can run to re-probe it as a work around? Or perhaps some debugging I can enable to find out exactly what is going wrong.

Send to us the output of "hciconfig -a" before you unplug the dongle.

-- 
Gustavo F. Padovan
http://profusion.mobi

^ permalink raw reply

* Re: updating unacked_frames counter during retransmission
From: Gustavo F. Padovan @ 2011-03-05  0:53 UTC (permalink / raw)
  To: Suraj Sumangala; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <4D6F358B.9020603@Atheros.com>

Hi Suraj,

* Suraj Sumangala <suraj@Atheros.com> [2011-03-03 12:00:35 +0530]:

> Hi Gustavo,
> 
> I have a question regarding the ERTM implementation.
> 
> Should we be incrementing the "l2cap_pinfo.unacked_frames" variable if 
> we are retransmitting a frame?

No, if we are retransmitting a frame that means L2CAP didn't received an ack
for it and we are already accounting it in unacked_frames and there is no need
to increment unacked_frames in this case.

> 
> Won't this cause the same frame to be accounted twice?
> 
> If there are too many retransmissions, will there be a chance that 
> "unacked_frames" could cross 0xFF and over flow.

No, unacked_frames number is limited by the remote transmission window size.

-- 
Gustavo F. Padovan
http://profusion.mobi

^ permalink raw reply

* Re: [PATCH v2] linux-firmware: Initial release for Bluetooth init script
From: Ohad Ben-Cohen @ 2011-03-04 23:34 UTC (permalink / raw)
  To: Pavan Savoy
  Cc: Gery Kahn, David Woodhouse, linux-wireless, linux-bluetooth,
	Luciano Coelho
In-Reply-To: <AANLkTik5Z_eAA_qWGB_rwoK3P1g4jMnM6hj26fje6JHf@mail.gmail.com>

Hi Pavan,

On Thu, Mar 3, 2011 at 3:44 PM, Pavan Savoy <pavan_savoy@ti.com> wrote:
> On Wed, Mar 2, 2011 at 8:01 PM, Gery Kahn <gery.kahn@gmail.com> wrote:
...
> Can we name this in all smaller case?

How is this going to work with smaller case ?

AFAICS, Gery used the exact name format that st_kim is looking for:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/misc/ti-st/st_kim.c;h=73b6c8b0e8696db9dbe8a71590bf7b62fb3b5532;hb=HEAD#l261

Do you plan to change st_kim ?

If so, please first change it, and then update the firmware file.

Thanks,
Ohad.

^ permalink raw reply

* Re: [RFC 0/6] LE advertising cache
From: Brian Gix @ 2011-03-04 20:49 UTC (permalink / raw)
  To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1299270913-8436-1-git-send-email-andre.guedes@openbossa.org>

On 11-03-04 12:35 PM, Andre Guedes wrote:
> During a LE connection establishment, the host should be able to infer the
> bdaddr type from a given bdaddr.
>
> To achieve that, during the LE scanning, the host stores the bdaddr and the
> bdaddr type gathered from advertising reports. The host keeps a list of
> advertising entry (bdaddr and bdaddr_type) for later lookup. This list will
> be called Advertising Cache.

My biggest problem with this is testing purposes.

While it is true that the bdaddr_type can be extracted from the LE Scan 
data, when you are at an event like UPF, there can be an awful lot of 
devices very close to you.

It would be nice to be able to explicitly specify both the bdaddr and 
bdaddr_type during these things.

But I agree that as a deployed device, caching from an LE scan makes the 
most sense.

Will this also work for (future) private addressing, where the address 
being connected to may not be the one initially seen in the scan?


>
> Since the penality to connect to an unreachable device is relatively high,
> we must keep only fresh advertising entries on the advertising cache. So,
> before each LE scanning the advertising cache is cleared. Also, after the LE
> scanning, a timer is set to clear the cache.
>
> Next steps include removing all advertising cache from userspace and
> implementing a mechanism to sync kernel and userspace advertising cache.
>
> Patches are rebased using Vinicius SMP patches, repo:
> git://git.infradead.org/users/vcgomes/linux-2.6.git for-next
>
> Anderson Briglia (1):
>    Bluetooth: Implement advertising report meta event
>
> Andre Guedes (5):
>    Bluetooth: LE advertising info caching
>    Bluetooth: Protect adv_entries with a RW semaphore
>    Bluetooth: Check advertising cache in hci_connect()
>    Bluetooth: Clear advertising cache before scanning
>    Bluetooth: Add a timer to clear the advertising cache
>
>   include/net/bluetooth/hci.h      |   20 ++++++++
>   include/net/bluetooth/hci_core.h |   16 +++++++
>   net/bluetooth/hci_conn.c         |   12 ++++-
>   net/bluetooth/hci_core.c         |   92 ++++++++++++++++++++++++++++++++++++++
>   net/bluetooth/hci_event.c        |   48 ++++++++++++++++++++
>   5 files changed, 185 insertions(+), 3 deletions(-)
>
> --
> 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


-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

^ permalink raw reply

* [RFC 6/6] Bluetooth: Add a timer to clear the advertising cache
From: Andre Guedes @ 2011-03-04 20:35 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Andre Guedes
In-Reply-To: <1299270913-8436-1-git-send-email-andre.guedes@openbossa.org>

This patch adds a timer to clear adv_entries list after three minutes.

After some amount of time, the advertising entries cached during the
last LE scan should be considered expired and they should be removed
from the adv_entries list.

It was chosen a three minutes timeout as an initial attempt. This value
might change in future.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    2 ++
 net/bluetooth/hci_core.c         |    9 +++++++++
 net/bluetooth/hci_event.c        |    6 +++++-
 3 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3ead365..790f839 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -179,6 +179,7 @@ struct hci_dev {
 
 	struct list_head	adv_entries;
 	struct rw_semaphore	adv_entries_rwsem;
+	struct timer_list	adv_timer;
 
 	struct hci_dev_stats	stat;
 
@@ -517,6 +518,7 @@ int hci_add_link_key(struct hci_dev *hdev, int new_key, bdaddr_t *bdaddr,
 						u8 *key, u8 type, u8 pin_len);
 int hci_remove_link_key(struct hci_dev *hdev, bdaddr_t *bdaddr);
 
+#define ADV_CLEAR_TIMEOUT (3*60*HZ) /* Three minutes */
 int hci_adv_entries_clear(struct hci_dev *hdev);
 struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr);
 int hci_add_adv_entry(struct hci_dev *hdev,
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8f9b3a5..ea8b0c2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1081,6 +1081,13 @@ static void hci_cmd_timer(unsigned long arg)
 	tasklet_schedule(&hdev->cmd_task);
 }
 
+static void hci_adv_clear(unsigned long arg)
+{
+	struct hci_dev *hdev = (void *) arg;
+
+	hci_adv_entries_clear(hdev);
+}
+
 int hci_adv_entries_clear(struct hci_dev *hdev)
 {
 	struct list_head *p, *n;
@@ -1233,6 +1240,7 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	INIT_LIST_HEAD(&hdev->adv_entries);
 	init_rwsem(&hdev->adv_entries_rwsem);
+	setup_timer(&hdev->adv_timer, hci_adv_clear, (unsigned long) hdev);
 
 	INIT_WORK(&hdev->power_on, hci_power_on);
 	INIT_WORK(&hdev->power_off, hci_power_off);
@@ -1314,6 +1322,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
 	hci_unregister_sysfs(hdev);
 
 	hci_del_off_timer(hdev);
+	del_timer(&hdev->adv_timer);
 
 	destroy_workqueue(hdev->workqueue);
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a07ed86..a1fe9cc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -858,8 +858,12 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		return;
 
 	param_scan_enable = *((__u8 *) sent);
-	if (param_scan_enable == 0x01)
+	if (param_scan_enable == 0x01) {
+		del_timer(&hdev->adv_timer);
 		hci_adv_entries_clear(hdev);
+	} else if (param_scan_enable == 0x00) {
+		mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
+	}
 }
 
 static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
-- 
1.7.1


^ permalink raw reply related

* [RFC 5/6] Bluetooth: Clear advertising cache before scanning
From: Andre Guedes @ 2011-03-04 20:35 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Andre Guedes
In-Reply-To: <1299270913-8436-1-git-send-email-andre.guedes@openbossa.org>

The LE advertising list should be cleared when a LE scanning is
performed. This will force the list to contain only fresh advertising
entries.

In order to achieve that, it was added a handler to the Command
Complete Event generated by the LE Set Scan Enable Command.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci.h      |    2 ++
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |    2 +-
 net/bluetooth/hci_event.c        |   23 +++++++++++++++++++++++
 4 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index fdcc494..1786794 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -670,6 +670,8 @@ struct hci_rp_le_read_buffer_size {
 	__u8     le_max_pkt;
 } __packed;
 
+#define HCI_OP_LE_SET_SCAN_ENABLE	0x200c
+
 #define HCI_OP_LE_CREATE_CONN		0x200d
 struct hci_cp_le_create_conn {
 	__le16   scan_interval;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index bf7b5df..3ead365 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -517,6 +517,7 @@ int hci_add_link_key(struct hci_dev *hdev, int new_key, bdaddr_t *bdaddr,
 						u8 *key, u8 type, u8 pin_len);
 int hci_remove_link_key(struct hci_dev *hdev, bdaddr_t *bdaddr);
 
+int hci_adv_entries_clear(struct hci_dev *hdev);
 struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr);
 int hci_add_adv_entry(struct hci_dev *hdev,
 					struct hci_ev_le_advertising_info *ev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 71f8788..8f9b3a5 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1081,7 +1081,7 @@ static void hci_cmd_timer(unsigned long arg)
 	tasklet_schedule(&hdev->cmd_task);
 }
 
-static int hci_adv_entries_clear(struct hci_dev *hdev)
+int hci_adv_entries_clear(struct hci_dev *hdev)
 {
 	struct list_head *p, *n;
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 825de59..a07ed86 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -843,6 +843,25 @@ static void hci_cc_le_ltk_neg_reply(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_req_complete(hdev, HCI_OP_LE_LTK_NEG_REPLY, rp->status);
 }
 
+static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
+					struct sk_buff *skb)
+{
+	void *sent;
+	__u8 param_scan_enable;
+	__u8 status = *((__u8 *) skb->data);
+
+	if (status)
+		return;
+
+	sent = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
+	if (!sent)
+		return;
+
+	param_scan_enable = *((__u8 *) sent);
+	if (param_scan_enable == 0x01)
+		hci_adv_entries_clear(hdev);
+}
+
 static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 {
 	BT_DBG("%s status 0x%x", hdev->name, status);
@@ -1799,6 +1818,10 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
 		hci_cc_le_ltk_neg_reply(hdev, skb);
 		break;
 
+	case HCI_OP_LE_SET_SCAN_ENABLE:
+		hci_cc_le_set_scan_enable(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%x", hdev->name, opcode);
 		break;
-- 
1.7.1


^ permalink raw reply related

* [RFC 4/6] Bluetooth: Check advertising cache in hci_connect()
From: Andre Guedes @ 2011-03-04 20:35 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Andre Guedes
In-Reply-To: <1299270913-8436-1-git-send-email-andre.guedes@openbossa.org>

When connecting to a LE device, we need to check the advertising cache
in order to know the bdaddr_type of that device. Additionally,
hci_le_connect() was changed to handle the bdaddr_type info.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 9a4a769..2d3197a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -45,7 +45,7 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
-static void hci_le_connect(struct hci_conn *conn)
+static void hci_le_connect(struct hci_conn *conn, u8 bdaddr_type)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_le_create_conn cp;
@@ -59,6 +59,7 @@ static void hci_le_connect(struct hci_conn *conn)
 	cp.scan_interval = cpu_to_le16(0x0004);
 	cp.scan_window = cpu_to_le16(0x0004);
 	bacpy(&cp.peer_addr, &conn->dst);
+	cp.peer_addr_type = bdaddr_type;
 	cp.conn_interval_min = cpu_to_le16(0x0008);
 	cp.conn_interval_max = cpu_to_le16(0x0100);
 	cp.supervision_timeout = cpu_to_le16(0x0064);
@@ -482,8 +483,13 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
 		le = hci_conn_add(hdev, LE_LINK, dst);
 		if (!le)
 			return ERR_PTR(-ENOMEM);
-		if (le->state == BT_OPEN)
-			hci_le_connect(le);
+		if (le->state == BT_OPEN) {
+			struct adv_entry *entry = hci_find_adv_entry(hdev, dst);
+			if (!entry)
+				return ERR_PTR(-EHOSTUNREACH);
+
+			hci_le_connect(le, entry->bdaddr_type);
+		}
 
 		hci_conn_hold(le);
 
-- 
1.7.1


^ permalink raw reply related

* [RFC 3/6] Bluetooth: Protect adv_entries with a RW semaphore
From: Andre Guedes @ 2011-03-04 20:35 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Andre Guedes
In-Reply-To: <1299270913-8436-1-git-send-email-andre.guedes@openbossa.org>

This patch adds a RW semaphore to protect concurrent operations on
adv_entries list.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |   22 ++++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 970711d..bf7b5df 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -178,6 +178,7 @@ struct hci_dev {
 	struct list_head	link_keys;
 
 	struct list_head	adv_entries;
+	struct rw_semaphore	adv_entries_rwsem;
 
 	struct hci_dev_stats	stat;
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cbff329..71f8788 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -44,6 +44,7 @@
 #include <linux/timer.h>
 #include <linux/crypto.h>
 #include <net/sock.h>
+#include <linux/rwsem.h>
 
 #include <asm/system.h>
 #include <linux/uaccess.h>
@@ -1084,6 +1085,8 @@ static int hci_adv_entries_clear(struct hci_dev *hdev)
 {
 	struct list_head *p, *n;
 
+	down_write(&hdev->adv_entries_rwsem);
+
 	list_for_each_safe(p, n, &hdev->adv_entries) {
 		struct adv_entry *entry;
 
@@ -1093,23 +1096,31 @@ static int hci_adv_entries_clear(struct hci_dev *hdev)
 		kfree(entry);
 	}
 
+	up_write(&hdev->adv_entries_rwsem);
+
 	return 0;
 }
 
 struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr)
 {
 	struct list_head *p;
+	struct adv_entry *res = NULL;
+
+	down_read(&hdev->adv_entries_rwsem);
 
 	list_for_each(p, &hdev->adv_entries) {
 		struct adv_entry *entry;
 
 		entry = list_entry(p, struct adv_entry, list);
 
-		if (bacmp(bdaddr, &entry->bdaddr) == 0)
-			return entry;
+		if (bacmp(bdaddr, &entry->bdaddr) == 0) {
+			res = entry;
+			goto out;
+		}
 	}
-
-	return NULL;
+out:
+	up_read(&hdev->adv_entries_rwsem);
+	return res;
 }
 
 static inline int is_connectable_adv(u8 evt_type)
@@ -1141,7 +1152,9 @@ int hci_add_adv_entry(struct hci_dev *hdev,
 	bacpy(&entry->bdaddr, &ev->bdaddr);
 	entry->bdaddr_type = ev->bdaddr_type;
 
+	down_write(&hdev->adv_entries_rwsem);
 	list_add(&entry->list, &hdev->adv_entries);
+	up_write(&hdev->adv_entries_rwsem);
 
 	return 0;
 }
@@ -1219,6 +1232,7 @@ int hci_register_dev(struct hci_dev *hdev)
 	INIT_LIST_HEAD(&hdev->link_keys);
 
 	INIT_LIST_HEAD(&hdev->adv_entries);
+	init_rwsem(&hdev->adv_entries_rwsem);
 
 	INIT_WORK(&hdev->power_on, hci_power_on);
 	INIT_WORK(&hdev->power_off, hci_power_off);
-- 
1.7.1


^ permalink raw reply related

* [RFC 2/6] Bluetooth: LE advertising info caching
From: Andre Guedes @ 2011-03-04 20:35 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Andre Guedes
In-Reply-To: <1299270913-8436-1-git-send-email-andre.guedes@openbossa.org>

This patch adds a list of 'advertising entry' to the struct hci_dev.

The advertising entry (struct adv_entry) stores sensitive information
(bdaddr and bdaddr_type so far) gathered from LE advertising report
events.

A double-linked list (list_head adv_entries) is used to store those
advertising entries. Only advertising entries from connectables devices
are inserted into the list.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |   12 ++++++
 net/bluetooth/hci_core.c         |   69 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |    5 +--
 3 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c59e857..970711d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -82,6 +82,12 @@ struct link_key {
 	u8 pin_len;
 };
 
+struct adv_entry {
+	struct list_head list;
+	bdaddr_t bdaddr;
+	u8 bdaddr_type;
+};
+
 #define NUM_REASSEMBLY 4
 struct hci_dev {
 	struct list_head list;
@@ -171,6 +177,8 @@ struct hci_dev {
 
 	struct list_head	link_keys;
 
+	struct list_head	adv_entries;
+
 	struct hci_dev_stats	stat;
 
 	struct sk_buff_head	driver_init;
@@ -508,6 +516,10 @@ int hci_add_link_key(struct hci_dev *hdev, int new_key, bdaddr_t *bdaddr,
 						u8 *key, u8 type, u8 pin_len);
 int hci_remove_link_key(struct hci_dev *hdev, bdaddr_t *bdaddr);
 
+struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr);
+int hci_add_adv_entry(struct hci_dev *hdev,
+					struct hci_ev_le_advertising_info *ev);
+
 void hci_del_off_timer(struct hci_dev *hdev);
 
 void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ff67843..cbff329 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1080,6 +1080,72 @@ static void hci_cmd_timer(unsigned long arg)
 	tasklet_schedule(&hdev->cmd_task);
 }
 
+static int hci_adv_entries_clear(struct hci_dev *hdev)
+{
+	struct list_head *p, *n;
+
+	list_for_each_safe(p, n, &hdev->adv_entries) {
+		struct adv_entry *entry;
+
+		entry = list_entry(p, struct adv_entry, list);
+
+		list_del(p);
+		kfree(entry);
+	}
+
+	return 0;
+}
+
+struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr)
+{
+	struct list_head *p;
+
+	list_for_each(p, &hdev->adv_entries) {
+		struct adv_entry *entry;
+
+		entry = list_entry(p, struct adv_entry, list);
+
+		if (bacmp(bdaddr, &entry->bdaddr) == 0)
+			return entry;
+	}
+
+	return NULL;
+}
+
+static inline int is_connectable_adv(u8 evt_type)
+{
+	if (evt_type == ADV_IND || evt_type == ADV_DIRECT_IND)
+		return 1;
+
+	return 0;
+}
+
+int hci_add_adv_entry(struct hci_dev *hdev,
+					struct hci_ev_le_advertising_info *ev)
+{
+	struct adv_entry *entry;
+
+	if (!is_connectable_adv(ev->evt_type))
+		return -EINVAL;
+
+	entry = hci_find_adv_entry(hdev, &ev->bdaddr);
+	/* Only new entries should be added to adv_entries. So, if
+	 * bdaddr was found, don't add it. */
+	if (entry)
+		return 0;
+
+	entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
+	if (!entry)
+		return -ENOMEM;
+
+	bacpy(&entry->bdaddr, &ev->bdaddr);
+	entry->bdaddr_type = ev->bdaddr_type;
+
+	list_add(&entry->list, &hdev->adv_entries);
+
+	return 0;
+}
+
 static struct crypto_blkcipher *alloc_cypher(void)
 {
 	if (enable_smp)
@@ -1152,6 +1218,8 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	INIT_LIST_HEAD(&hdev->link_keys);
 
+	INIT_LIST_HEAD(&hdev->adv_entries);
+
 	INIT_WORK(&hdev->power_on, hci_power_on);
 	INIT_WORK(&hdev->power_off, hci_power_off);
 	setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
@@ -1239,6 +1307,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
 	hci_blacklist_clear(hdev);
 	hci_uuids_clear(hdev);
 	hci_link_keys_clear(hdev);
+	hci_adv_entries_clear(hdev);
 	hci_dev_unlock_bh(hdev);
 
 	__hci_dev_put(hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 8acc4a5..825de59 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2565,12 +2565,11 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
 	num_reports = skb->data[0];
 
 	ev = (void *) &skb->data[1];
-
-	BT_DBG("adv from: %s", batostr(&ev->bdaddr));
+	hci_add_adv_entry(hdev, ev);
 
 	for (i = 1; i < num_reports; i++) {
 		ev = (void *) (ev->data + ev->length + 1);
-		BT_DBG("adv from: %s", batostr(&ev->bdaddr));
+		hci_add_adv_entry(hdev, ev);
 	}
 }
 
-- 
1.7.1


^ permalink raw reply related

* [RFC 1/6] Bluetooth: Implement advertising report meta event
From: Andre Guedes @ 2011-03-04 20:35 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Anderson Briglia
In-Reply-To: <1299270913-8436-1-git-send-email-andre.guedes@openbossa.org>

From: Anderson Briglia <anderson.briglia@openbossa.org>

This patch implements new LE meta event in order to handle advertising
reports.

Signed-off-by: Anderson Briglia <anderson.briglia@openbossa.org>
---
 include/net/bluetooth/hci.h |   18 ++++++++++++++++++
 net/bluetooth/hci_event.c   |   22 ++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index d851f8b..fdcc494 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1011,6 +1011,24 @@ struct hci_ev_le_conn_complete {
 	__u8     clk_accurancy;
 } __packed;
 
+#define ADV_IND		0x00
+#define ADV_DIRECT_IND	0x01
+#define ADV_SCAN_IND	0x02
+#define ADV_NONCONN_IND	0x03
+#define SCAN_RSP	0x04
+
+#define ADDR_DEV_PUBLIC	0x00
+#define ADDR_DEV_RANDOM	0x01
+
+#define HCI_EV_LE_ADVERTISING_REPORT	0x02
+struct hci_ev_le_advertising_info {
+	__u8	 evt_type;
+	__u8	 bdaddr_type;
+	bdaddr_t bdaddr;
+	__u8	 length;
+	__u8	 data[0];
+} __packed;
+
 #define HCI_EV_LE_LTK_REQ		0x05
 struct hci_ev_le_ltk_req {
 	__le16	handle;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 5a2ab2c..8acc4a5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2556,6 +2556,24 @@ static inline void hci_le_ltk_request_evt(struct hci_dev *hdev,
 	hci_dev_unlock(hdev);
 }
 
+static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
+						struct sk_buff *skb)
+{
+	struct hci_ev_le_advertising_info *ev;
+	u8 num_reports, i;
+
+	num_reports = skb->data[0];
+
+	ev = (void *) &skb->data[1];
+
+	BT_DBG("adv from: %s", batostr(&ev->bdaddr));
+
+	for (i = 1; i < num_reports; i++) {
+		ev = (void *) (ev->data + ev->length + 1);
+		BT_DBG("adv from: %s", batostr(&ev->bdaddr));
+	}
+}
+
 static inline void hci_le_meta_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_le_meta *le_ev = (void *) skb->data;
@@ -2571,6 +2589,10 @@ static inline void hci_le_meta_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_le_ltk_request_evt(hdev, skb);
 		break;
 
+	case HCI_EV_LE_ADVERTISING_REPORT:
+		hci_le_adv_report_evt(hdev, skb);
+		break;
+
 	default:
 		break;
 	}
-- 
1.7.1


^ permalink raw reply related

* [RFC 0/6] LE advertising cache
From: Andre Guedes @ 2011-03-04 20:35 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Andre Guedes

During a LE connection establishment, the host should be able to infer the
bdaddr type from a given bdaddr.

To achieve that, during the LE scanning, the host stores the bdaddr and the
bdaddr type gathered from advertising reports. The host keeps a list of
advertising entry (bdaddr and bdaddr_type) for later lookup. This list will
be called Advertising Cache.

Since the penality to connect to an unreachable device is relatively high,
we must keep only fresh advertising entries on the advertising cache. So,
before each LE scanning the advertising cache is cleared. Also, after the LE
scanning, a timer is set to clear the cache.

Next steps include removing all advertising cache from userspace and
implementing a mechanism to sync kernel and userspace advertising cache.

Patches are rebased using Vinicius SMP patches, repo:
git://git.infradead.org/users/vcgomes/linux-2.6.git for-next

Anderson Briglia (1):
  Bluetooth: Implement advertising report meta event

Andre Guedes (5):
  Bluetooth: LE advertising info caching
  Bluetooth: Protect adv_entries with a RW semaphore
  Bluetooth: Check advertising cache in hci_connect()
  Bluetooth: Clear advertising cache before scanning
  Bluetooth: Add a timer to clear the advertising cache

 include/net/bluetooth/hci.h      |   20 ++++++++
 include/net/bluetooth/hci_core.h |   16 +++++++
 net/bluetooth/hci_conn.c         |   12 ++++-
 net/bluetooth/hci_core.c         |   92 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |   48 ++++++++++++++++++++
 5 files changed, 185 insertions(+), 3 deletions(-)


^ permalink raw reply

* [PATCHv3 2/2] Fix MTU value used on MTU exchange response
From: Bruna Moreira @ 2011-03-04 20:17 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1299083684-5062-2-git-send-email-bruna.moreira@openbossa.org>

ATT_MTU_RESP shall send the original MTU value prior to the exchange. If
client sends an invalid value, the server shall use the ATT_DEFAULT_LE_MTU
value. This operation is not supported in BR/EDR, so if any client sends
this request it will be ignored by server and it will reply with
"Request not supported".

For BR/EDR, the MTU is limited to ATT_MAX_MTU (currently 256). This
avoids allocating a big buffer when most ATT PDUs are small.

Note: the kernel currently limits the minimum MTU to 48, regardless of
transport type. This limit is valid only for BR/EDR, for LE the minimum
is 23. This bug will be fixed, but it does not affect the outgoing MTU
for new created LE sockets, which are correctly set to 23.
---
 src/attrib-server.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index 61db851..8464d2a 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -59,6 +59,7 @@ struct gatt_channel {
 	GSList *indicate;
 	GAttrib *attrib;
 	guint mtu;
+	gboolean le;
 	guint id;
 	gboolean encrypted;
 };
@@ -759,9 +760,18 @@ static uint16_t write_value(struct gatt_channel *channel, uint16_t handle,
 static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
 		uint8_t *pdu, int len)
 {
-	channel->mtu = MIN(mtu, channel->mtu);
+	guint old_mtu = channel->mtu;
 
-	return enc_mtu_resp(channel->mtu, pdu, len);
+	if (mtu < ATT_DEFAULT_LE_MTU)
+		channel->mtu = ATT_DEFAULT_LE_MTU;
+	else
+		channel->mtu = MIN(mtu, channel->mtu);
+
+	bt_io_set(le_io, BT_IO_L2CAP, NULL,
+			BT_IO_OPT_OMTU, channel->mtu,
+			BT_IO_OPT_INVALID);
+
+	return enc_mtu_resp(old_mtu, pdu, len);
 }
 
 static void channel_disconnect(void *user_data)
@@ -831,6 +841,11 @@ static void channel_handler(const uint8_t *ipdu, uint16_t len,
 		length = read_blob(channel, start, offset, opdu, channel->mtu);
 		break;
 	case ATT_OP_MTU_REQ:
+		if (!channel->le) {
+			status = ATT_ECODE_REQ_NOT_SUPP;
+			goto done;
+		}
+
 		length = dec_mtu_req(ipdu, len, &mtu);
 		if (length == 0) {
 			status = ATT_ECODE_INVALID_PDU;
@@ -913,6 +928,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 			BT_IO_OPT_SOURCE_BDADDR, &channel->src,
 			BT_IO_OPT_DEST_BDADDR, &channel->dst,
 			BT_IO_OPT_CID, &cid,
+			BT_IO_OPT_OMTU, &channel->mtu,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
 		error("bt_io_get: %s", gerr->message);
@@ -922,10 +938,13 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 		return;
 	}
 
+	if (channel->mtu > ATT_MAX_MTU)
+		channel->mtu = ATT_MAX_MTU;
+
 	if (cid != GATT_CID)
-		channel->mtu = ATT_DEFAULT_L2CAP_MTU;
+		channel->le = FALSE;
 	else
-		channel->mtu = ATT_DEFAULT_LE_MTU;
+		channel->le = TRUE;
 
 	channel->attrib = g_attrib_new(io);
 	g_io_channel_unref(io);
-- 
1.7.0.4


^ permalink raw reply related

* [PATCHv3 1/2] Use CID to infer transport type
From: Bruna Moreira @ 2011-03-04 20:17 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1299083684-5062-2-git-send-email-bruna.moreira@openbossa.org>

Instead of comparing GIOChannel pointers, use the CID returned by
bt_io_get() to infer the transport type. LE uses the fixed CID for GATT.
---
 src/attrib-server.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index b980b28..61db851 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -899,7 +899,7 @@ done:
 static void connect_event(GIOChannel *io, GError *err, void *user_data)
 {
 	struct gatt_channel *channel;
-	GIOChannel **server_io = user_data;
+	uint16_t cid;
 	GError *gerr = NULL;
 
 	if (err) {
@@ -912,6 +912,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 	bt_io_get(io, BT_IO_L2CAP, &gerr,
 			BT_IO_OPT_SOURCE_BDADDR, &channel->src,
 			BT_IO_OPT_DEST_BDADDR, &channel->dst,
+			BT_IO_OPT_CID, &cid,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
 		error("bt_io_get: %s", gerr->message);
@@ -921,7 +922,7 @@ static void connect_event(GIOChannel *io, GError *err, void *user_data)
 		return;
 	}
 
-	if (server_io == &l2cap_io)
+	if (cid != GATT_CID)
 		channel->mtu = ATT_DEFAULT_L2CAP_MTU;
 	else
 		channel->mtu = ATT_DEFAULT_LE_MTU;
@@ -1046,7 +1047,7 @@ int attrib_server_init(void)
 
 	/* BR/EDR socket */
 	l2cap_io = bt_io_listen(BT_IO_L2CAP, NULL, confirm_event,
-					&l2cap_io, NULL, &gerr,
+					NULL, NULL, &gerr,
 					BT_IO_OPT_SOURCE_BDADDR, BDADDR_ANY,
 					BT_IO_OPT_PSM, GATT_PSM,
 					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
-- 
1.7.0.4


^ permalink raw reply related

* Re: [PATCH 1/5] Add new UUID utility functions
From: Brian Gix @ 2011-03-04 19:54 UTC (permalink / raw)
  To: Anderson Lizardo
  Cc: Elvis Pfützenreuter, linux-bluetooth, Claudio Takahasi
In-Reply-To: <AANLkTi=a0Wy-qxiRUVYH7TmaYe9yoBHc0U2JDPBs47B=@mail.gmail.com>

Hi Lizardo,

On 11-03-04 11:17 AM, Anderson Lizardo wrote:
> Hi Brian,
>
> On Fri, Mar 4, 2011 at 2:57 PM, Brian Gix<bgix@codeaurora.org>  wrote:
>> On 11-03-04 10:32 AM, Elvis Pfützenreuter wrote:
>>>> I don't believe this line to be always portable between platforms. This
>>>> line makes the assumption that a uint16_t is two native units (not always
>>>> true) and can therefore be directly memcopy'd into into an array of uint8_t
>>>> array members. This is not always true. memcpy should never be used in
>>>> portable code when packing data for network datagram usage models (anything
>>>> that treats data as an octet stream).
>>>
>>> If we had used "unsigned char" and "unsigned short int", I'd agree, but
>>> uint8_t and uint16_t are *explicitly* 8-bit and 16-bit types, and thus there
>>> is a guaranteed 2:1 size relationship.
>>>
>>
>> Unfortunately, this is only part true. There are already CPU architectures
>> in the marketplace which define the smallest addressable unit as being 16
>> bits, which means that even if you have defined a uint8_t that has a valid
>> range of only 0-255, the sizeof(uint8_t) will == 1, and the sizeof(uint16_t)
>> will also == 1, and so a uint16_t of 0x1234 memcpy'd into a uint8_t array
>> will not appear as {0x12, 0x34} but rather as {0x34, 0x00} regardless of
>> your endian-ness (the Most Sig 8 bits hidden by the allowed range). Memcpy
>> is only portability-safe when used to copy between two identical data types.
>> It is also the reason you see things like sizeof(uint16_t) being passed as
>> arguments to the mem* functions.
>>
>> That axiom is also true when copying data between unions and/or stucts with
>> different underlying definitions. The layout of raw bits can never be
>> assumed.
>
> While your explanation looks really interesting (I always thought that
> sizeof() returned the number of bytes), keep in mind that there are
> tons of memcpy() calls beween different datatypes, at least on BlueZ
> and on Linux kernel. I'm not arguing that we should keep it as is, but
> I suppose there will be a lot of changes to be done even before BlueZ
> (or even the kernel) can run on these CPUs.
>
> If I understand your explanation, even memcpy(..., ...,
> sizeof(uint16_t)) would not work on these CPUs, as it copies the
> specified number of *bytes*.
>
> Simply grep for "memcpy" on BlueZ sources to get an idea.
>
> My two cents,


I suppose you are correct that for Linux and BlueZ, the damage is 
already done as far as mem* function usage, and that therefore all of my 
other objections are moot, and withdraw my fundamental memcpy objection.


However:
You are also correct that sizeof() returns the number of bytes, but 
bytes are not in fact the same thing as octets.  An octet is used in 
networking, and other inter-architecture communication, and is defined 
as being exactly eight bits of data.

A byte on the other hand is architecturally defined and is neither 
limited to, nor gaurenteed to be at least, 8 bits.  It is rather "the 
smallest addressable unit" of the target architecture. That is why if 
you read networking documents, you are more likely to hear the 
underlying data referred to as "octets" instead of bytes. Octets are 
precise, and bytes are imprecise.

byte != octet

So memcpy(a1, a2, sizeof(uint16_t)) would work as expected, assuming a1 
and a2 were both pointers to objects with underlying 16 bit definitions.

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

^ permalink raw reply

* Re: [PATCH 1/3] hcitool: use defined constants for address type
From: Johan Hedberg @ 2011-03-04 19:46 UTC (permalink / raw)
  To: Claudio Takahasi; +Cc: linux-bluetooth
In-Reply-To: <1299097768-27237-1-git-send-email-claudio.takahasi@openbossa.org>

Hi Claudio,

On Wed, Mar 02, 2011, Claudio Takahasi wrote:
> ---
>  tools/hcitool.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

All three patches have been pushed upstream. Thanks.

Johan

^ permalink raw reply

* Re: [PATCH] Fix support for shells that do not have "+="
From: Johan Hedberg @ 2011-03-04 19:43 UTC (permalink / raw)
  To: lkslawek; +Cc: linux-bluetooth
In-Reply-To: <1298637272-21031-1-git-send-email-lkslawek@gmail.com>

Hi Slawek,

On Fri, Feb 25, 2011, lkslawek@gmail.com wrote:
> From: Slawomir Bochenski <lkslawek@gmail.com>
> 
> This will make maintainer mode work on less advanced shells, the ones
> that do not support appending.
> ---
>  acinclude.m4 |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)

Pushed upstream. Thanks. I also went ahead and pushed a similar patch
for BlueZ.

Johan

^ permalink raw reply

* Re: [PATCHv2 1/2] Use CID to infer transport type
From: Bruna Moreira @ 2011-03-04 19:32 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1299083684-5062-1-git-send-email-bruna.moreira@openbossa.org>

On 3/2/11, Bruna Moreira <bruna.moreira@openbossa.org> wrote:
> Instead of comparing GIOChannel pointers, use the CID returned by
> bt_io_get() to infer the transport type. LE uses the fixed CID for GATT.
> ---
>  src/attrib-server.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>

Please ignore this series, I will send a fixed one (I found a bug
after running more tests).

BR,
-- 
Bruna Moreira
Instituto Nokia de Tecnologia (INdT)
Manaus - Brazil

^ permalink raw reply

* Re: [PATCH 1/5] Add new UUID utility functions
From: Anderson Lizardo @ 2011-03-04 19:17 UTC (permalink / raw)
  To: Brian Gix; +Cc: Elvis Pfützenreuter, linux-bluetooth, Claudio Takahasi
In-Reply-To: <4D71361E.3020606@codeaurora.org>

Hi Brian,

On Fri, Mar 4, 2011 at 2:57 PM, Brian Gix <bgix@codeaurora.org> wrote:
> On 11-03-04 10:32 AM, Elvis Pfützenreuter wrote:
>>> I don't believe this line to be always portable between platforms. This
>>> line makes the assumption that a uint16_t is two native units (not always
>>> true) and can therefore be directly memcopy'd into into an array of uint8_t
>>> array members. This is not always true. memcpy should never be used in
>>> portable code when packing data for network datagram usage models (anything
>>> that treats data as an octet stream).
>>
>> If we had used "unsigned char" and "unsigned short int", I'd agree, but
>> uint8_t and uint16_t are *explicitly* 8-bit and 16-bit types, and thus there
>> is a guaranteed 2:1 size relationship.
>>
>
> Unfortunately, this is only part true. There are already CPU architectures
> in the marketplace which define the smallest addressable unit as being 16
> bits, which means that even if you have defined a uint8_t that has a valid
> range of only 0-255, the sizeof(uint8_t) will == 1, and the sizeof(uint16_t)
> will also == 1, and so a uint16_t of 0x1234 memcpy'd into a uint8_t array
> will not appear as {0x12, 0x34} but rather as {0x34, 0x00} regardless of
> your endian-ness (the Most Sig 8 bits hidden by the allowed range). Memcpy
> is only portability-safe when used to copy between two identical data types.
> It is also the reason you see things like sizeof(uint16_t) being passed as
> arguments to the mem* functions.
>
> That axiom is also true when copying data between unions and/or stucts with
> different underlying definitions. The layout of raw bits can never be
> assumed.

While your explanation looks really interesting (I always thought that
sizeof() returned the number of bytes), keep in mind that there are
tons of memcpy() calls beween different datatypes, at least on BlueZ
and on Linux kernel. I'm not arguing that we should keep it as is, but
I suppose there will be a lot of changes to be done even before BlueZ
(or even the kernel) can run on these CPUs.

If I understand your explanation, even memcpy(..., ...,
sizeof(uint16_t)) would not work on these CPUs, as it copies the
specified number of *bytes*.

Simply grep for "memcpy" on BlueZ sources to get an idea.

My two cents,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

^ permalink raw reply

* Re: [PATCH] Fix adapter reference count in input plugin
From: Szymon Janc @ 2011-03-04 19:02 UTC (permalink / raw)
  To: Johan Hedberg
  Cc: linux-bluetooth@vger.kernel.org,
	par-gunnar.p.hjalmdahl@stericsson.com,
	henrik.possung@stericsson.com
In-Reply-To: <20110304183826.GB21452@jh-x301>

> This one looks like it's working around the core issue instead of fixing
> it. The _remove function shouldn't get called if _probe failed. I.e. the
> issue seems to be somewhere in the core daemon.

Every btd_adapter_driver do that so I just followed.

But I agree that proper fix should be in core daemon i.e. flag in
btd_adapter_driver structure and set/test in load/unload_driver in adapter.c
or a separate list of activated drivers.

-- 
BR
Szymon Janc

^ permalink raw reply

* Re: [PATCH] Remove duplicated NULL check and debug message
From: Johan Hedberg @ 2011-03-04 18:58 UTC (permalink / raw)
  To: Szymon Janc; +Cc: linux-bluetooth, par-gunnar.p.hjalmdahl, henrik.possung
In-Reply-To: <1299263587-25273-1-git-send-email-szymon.janc@tieto.com>

Hi Szymon,

On Fri, Mar 04, 2011, Szymon Janc wrote:
> ---
>  src/sdpd-request.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)

This one has also been applied. Thanks.

Johan

^ permalink raw reply

* Re: [PATCH] Fix small memleak in compat/pand.c
From: Johan Hedberg @ 2011-03-04 18:57 UTC (permalink / raw)
  To: Szymon Janc; +Cc: linux-bluetooth, par-gunnar.p.hjalmdahl, henrik.possung
In-Reply-To: <1299263520-25191-1-git-send-email-szymon.janc@tieto.com>

Hi Szymon,

On Fri, Mar 04, 2011, Szymon Janc wrote:
> ---
>  compat/pand.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)

Both memleak patches have been applied. Thanks.

Johan

^ permalink raw reply

* Re: [PATCH 1/5] Add new UUID utility functions
From: Brian Gix @ 2011-03-04 18:57 UTC (permalink / raw)
  To: Elvis Pfützenreuter; +Cc: linux-bluetooth, Claudio Takahasi
In-Reply-To: <0881010B-89C3-40B7-A48E-A85E1BDD0E6C@signove.com>

Hi Elvis,

On 11-03-04 10:32 AM, Elvis Pfützenreuter wrote:
> Hi Brian,
>
> On 4 Mar 2011, at 15:06 , Brian Gix wrote:
>
>> Hi Elvis,
>>
>> I'm trying to understand the problem being addressed by these patches. I am guessing that there is a difference in behaviour between little endian and big endian architectures, but I still question some of the changes.
>>
>> One example below.
>>
>> On 11-03-04 08:20 AM, Elvis Pfützenreuter wrote:
>> [...]
>>> +
>>> +	memcpy(&uuid128->value.u128.data[BASE_UUID16_OFFSET],&data, 2);
>>
>> I don't believe this line to be always portable between platforms. This line makes the assumption that a uint16_t is two native units (not always true) and can therefore be directly memcopy'd into into an array of uint8_t array members. This is not always true. memcpy should never be used in portable code when packing data for network datagram usage models (anything that treats data as an octet stream).
>
> If we had used "unsigned char" and "unsigned short int", I'd agree, but uint8_t and uint16_t are *explicitly* 8-bit and 16-bit types, and thus there is a guaranteed 2:1 size relationship.
>

Unfortunately, this is only part true. There are already CPU 
architectures in the marketplace which define the smallest addressable 
unit as being 16 bits, which means that even if you have defined a 
uint8_t that has a valid range of only 0-255, the sizeof(uint8_t) will 
== 1, and the sizeof(uint16_t) will also == 1, and so a uint16_t of 
0x1234 memcpy'd into a uint8_t array will not appear as {0x12, 0x34} but 
rather as {0x34, 0x00} regardless of your endian-ness (the Most Sig 8 
bits hidden by the allowed range). Memcpy is only portability-safe when 
used to copy between two identical data types. It is also the reason you 
see things like sizeof(uint16_t) being passed as arguments to the mem* 
functions.

That axiom is also true when copying data between unions and/or stucts 
with different underlying definitions. The layout of raw bits can never 
be assumed.

>>
>> I am not saying I agree with the need to store UUIDs in anything but network order, however if I were to write a portable function that will always work in this instance, I would eliminate all usage of memcpy here and do something like:
>>
>> uuid128->value.u128.data[SHORT_UUID_LE_0] = (uint8_t) data;
>> uuid128->value.u128.data[SHORT_UUID_LE_1] = (uint8_t) (data>>  8);
>>
>> And likewise for uuid32 to uui128:
>> uuid128->value.u128.data[SHORT_UUID_LE_2] = (uint8_t) (data>>  16);
>> uuid128->value.u128.data[SHORT_UUID_LE_3] = (uint8_t) (data>>  24);
>
> I don't feel it's any better. This is Claudio's style, I'd prefer something like
>
> *((uint16_t* )&uuid128->value.u128.data[BASE_UUID16_OFFSET]) = data;

I personally think this slightly preferable to memcpy, but still not 
100% portable unless targeted only for 8-bit addressable CPUs.


-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

^ permalink raw reply

* Re: [PATCH] Adjust timeout LE and API
From: Johan Hedberg @ 2011-03-04 18:54 UTC (permalink / raw)
  To: Marco Sinigaglia; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <4D6FBAB3.20304@csr.com>

Hi Marco,

On Thu, Mar 03, 2011, Marco Sinigaglia wrote:
> The hci_send_req 100ms timeout is not enough when the chip is in deep
> sleep.  The timeout value has been hardcoded on hci_send_req call and,
> instead, it should be passed from above (for ex from
> hci_le_set_scan_enable).  This patch removes the hardcoded value from
> hci_send_req and it increases the timeout to 1000ms.
> 
> From: Marco Sinigaglia, marco.sinigaglia@csr.com
> 
> ---
>  lib/hci.c         |   12 ++++++------
>  lib/hci_lib.h     |    6 +++---
>  tools/hciconfig.c |    2 +-
>  tools/hcitool.c   |    6 +++---
>  4 files changed, 13 insertions(+), 13 deletions(-)

The patch looks good but doesn't apply with git am. Could you please
recreate it with git format-patch and resend with git send-email?
Thanks.

Johan

^ permalink raw reply

* Re: [PATCH] Fix hal plugin compilation error
From: Johan Hedberg @ 2011-03-04 18:52 UTC (permalink / raw)
  To: Szymon Janc; +Cc: linux-bluetooth, par-gunnar.p.hjalmdahl, henrik.possung
In-Reply-To: <1299144254-5405-1-git-send-email-szymon.janc@tieto.com>

Hi Szymon,

On Thu, Mar 03, 2011, Szymon Janc wrote:
> ---
>  plugins/hal.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)

Pushed upstream. Thanks.

Johan

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox