* Re: [PATCHes] osso-gwobex patches for review
From: Johan Hedberg @ 2010-02-23 13:48 UTC (permalink / raw)
To: Bastien Nocera; +Cc: BlueZ development
In-Reply-To: <1266762470.25029.2195.camel@localhost.localdomain>
Hi Bastien,
On Sun, Feb 21, 2010, Bastien Nocera wrote:
> The first patch fixes a few typos.
Yep, looks good and I've pushed it upstream.
> The second one requires the openobex patches I posted to the list, but
> should fail gracefully if the support does not exist.
This patch has some issues:
> + /* Setup low-level USB */
> + if (!gw_obex_transport_setup_usb(&handle)) {
> + debug("gw_obex_transport_setup_usb() failed, no USB support?\n");
> + return NULL;
> + }
You forget to set *error before returning NULL here.
> + /* Do the transport connection */
> + if (OBEX_InterfaceConnect(handle, iface) < 0) {
> + debug("Could not connect to USB device '%s'\n", dev);
> + if (error)
> + *error = GW_OBEX_ERROR_NO_SERVICE;
> + return NULL;
> + }
This looks like a memory leak since you don't do the same cleanup of ctx->mutex
and ctx as you do in the other failure cases. Maybe the function could use a
"fail" label where you jump to for cleanup tasks?
Besides those issues the patch seems ok'ish.
Johan
^ permalink raw reply
* A2DP sink indications + autoclose after suspension
From: Jacek Szczech @ 2010-02-23 12:58 UTC (permalink / raw)
To: linux-bluetooth
Hi,
I succeeded writing a program that receives a2dp stream (from my mobile
phone), decodes and sends to stdout using unix/ipc interface. It'll be run on
old hw without X enviroment, that's why I didn't use already available
software (like pulseaudio).
I faced a few problems connected with suspension of stream:
1) Unix.c does not support indications (am I right?). How can I detect changes
of states (e.g. OPEN<->STREAMING)? I can detect suspension with EAGAIN on
stream_fd but it does not seem to be proper solution. Do you plan to support
indications in unix/ipc interface?
2) Will it be possible to get indications concerning OPEN<->STREAMING changes
on dbus Media interface?
3) After suspension of stream (SUSPEND sent from SRC) the stream
is CLOSEd after about 10 seconds. What part of code disconnects this? Is it a
way to avoid this?
regards,
Jacek
^ permalink raw reply
* [PATCH 2/2] Fix pcsuite WHO header
From: Luiz Augusto von Dentz @ 2010-02-23 10:07 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]
---
plugins/ftp.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/plugins/ftp.c b/plugins/ftp.c
index e0a3b97..c3a4193 100644
--- a/plugins/ftp.c
+++ b/plugins/ftp.c
@@ -93,6 +93,7 @@
</record>"
#define PCSUITE_CHANNEL 24
+#define PCSUITE_WHO_SIZE 8
#define PCSUITE_RECORD "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>
\
<record>
\
<attribute id=\"0x0001\">
\
@@ -140,9 +141,8 @@ static const guint8 FTP_TARGET[TARGET_SIZE] = {
0xF9, 0xEC, 0x7B, 0xC4, 0x95, 0x3C, 0x11, 0xD2,
0x98, 0x4E, 0x52, 0x54, 0x00, 0xDC, 0x9E, 0x09 };
-static const guint8 PCSUITE_TARGET[TARGET_SIZE] = {
- 0x00, 0x00, 0x50, 0x05, 0x00, 0x00, 0x10, 0x00,
- 0x80, 0x00, 0x00, 0x02, 0xEE, 0x00, 0x00, 0x01 };
+static const guint8 PCSUITE_WHO[PCSUITE_WHO_SIZE] = {
+ 'P','C',' ','S','u','i','t','e' };
static gint get_by_type(struct obex_session *os, gchar *type, size_t *size)
{
@@ -464,8 +464,8 @@ struct obex_service_driver pcsuite = {
.record = PCSUITE_RECORD,
.target = FTP_TARGET,
.target_size = TARGET_SIZE,
- .who = PCSUITE_TARGET,
- .who_size = TARGET_SIZE,
+ .who = PCSUITE_WHO,
+ .who_size = PCSUITE_WHO_SIZE,
.connect = ftp_connect,
.get = ftp_get,
.put = ftp_put,
--
1.6.3.3
--
Luiz Augusto von Dentz
Computer Engineer
[-- Attachment #2: 0002-Fix-pcsuite-WHO-header.patch --]
[-- Type: text/x-patch, Size: 1612 bytes --]
From 40334d063fb34ef6085897ad698d83d5a0803551 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Mon, 22 Feb 2010 17:36:51 +0200
Subject: [PATCH 2/2] Fix pcsuite WHO header
---
plugins/ftp.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/plugins/ftp.c b/plugins/ftp.c
index e0a3b97..c3a4193 100644
--- a/plugins/ftp.c
+++ b/plugins/ftp.c
@@ -93,6 +93,7 @@
</record>"
#define PCSUITE_CHANNEL 24
+#define PCSUITE_WHO_SIZE 8
#define PCSUITE_RECORD "<?xml version=\"1.0\" encoding=\"UTF-8\" ?> \
<record> \
<attribute id=\"0x0001\"> \
@@ -140,9 +141,8 @@ static const guint8 FTP_TARGET[TARGET_SIZE] = {
0xF9, 0xEC, 0x7B, 0xC4, 0x95, 0x3C, 0x11, 0xD2,
0x98, 0x4E, 0x52, 0x54, 0x00, 0xDC, 0x9E, 0x09 };
-static const guint8 PCSUITE_TARGET[TARGET_SIZE] = {
- 0x00, 0x00, 0x50, 0x05, 0x00, 0x00, 0x10, 0x00,
- 0x80, 0x00, 0x00, 0x02, 0xEE, 0x00, 0x00, 0x01 };
+static const guint8 PCSUITE_WHO[PCSUITE_WHO_SIZE] = {
+ 'P','C',' ','S','u','i','t','e' };
static gint get_by_type(struct obex_session *os, gchar *type, size_t *size)
{
@@ -464,8 +464,8 @@ struct obex_service_driver pcsuite = {
.record = PCSUITE_RECORD,
.target = FTP_TARGET,
.target_size = TARGET_SIZE,
- .who = PCSUITE_TARGET,
- .who_size = TARGET_SIZE,
+ .who = PCSUITE_WHO,
+ .who_size = PCSUITE_WHO_SIZE,
.connect = ftp_connect,
.get = ftp_get,
.put = ftp_put,
--
1.6.3.3
^ permalink raw reply related
* [PATCH 1/2] Fix service driver selection when WHO header is informed
From: Luiz Augusto von Dentz @ 2010-02-23 10:06 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]
Drivers which support WHO header should be checked first since they have
priority e.g. pcsuite over ftp.
---
src/service.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/service.c b/src/service.c
index 196a5c1..c6ce101 100644
--- a/src/service.c
+++ b/src/service.c
@@ -49,8 +49,8 @@ struct obex_service_driver
*obex_service_driver_find(GSList *list,
struct obex_service_driver *driver = l->data;
if (driver->who && who &&
- driver->who_size == who_size &&
- memcmp(driver->who, who, who_size) != 0)
+ (driver->who_size != who_size ||
+ memcmp(driver->who, who, who_size) != 0))
continue;
if (driver->target == NULL && target == NULL)
@@ -97,7 +97,11 @@ int obex_service_driver_register(struct
obex_service_driver *driver)
debug("driver %p service %s registered", driver, driver->name);
- drivers = g_slist_append(drivers, driver);
+ /* Drivers that support who has priority */
+ if (driver->who)
+ drivers = g_slist_prepend(drivers, driver);
+ else
+ drivers = g_slist_append(drivers, driver);
return 0;
}
--
1.6.3.3
--
Luiz Augusto von Dentz
Computer Engineer
[-- Attachment #2: 0001-Fix-service-driver-selection-when-WHO-header-is-info.patch --]
[-- Type: text/x-patch, Size: 1346 bytes --]
From cce63393c40e55ddbdbc8241bddaecdb5d7306ef Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Mon, 22 Feb 2010 17:26:26 +0200
Subject: [PATCH 1/2] Fix service driver selection when WHO header is informed
Drivers which support WHO header should be checked first since they have
priority e.g. pcsuite over ftp.
---
src/service.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/service.c b/src/service.c
index 196a5c1..c6ce101 100644
--- a/src/service.c
+++ b/src/service.c
@@ -49,8 +49,8 @@ struct obex_service_driver *obex_service_driver_find(GSList *list,
struct obex_service_driver *driver = l->data;
if (driver->who && who &&
- driver->who_size == who_size &&
- memcmp(driver->who, who, who_size) != 0)
+ (driver->who_size != who_size ||
+ memcmp(driver->who, who, who_size) != 0))
continue;
if (driver->target == NULL && target == NULL)
@@ -97,7 +97,11 @@ int obex_service_driver_register(struct obex_service_driver *driver)
debug("driver %p service %s registered", driver, driver->name);
- drivers = g_slist_append(drivers, driver);
+ /* Drivers that support who has priority */
+ if (driver->who)
+ drivers = g_slist_prepend(drivers, driver);
+ else
+ drivers = g_slist_append(drivers, driver);
return 0;
}
--
1.6.3.3
^ permalink raw reply related
* Re: [Bugme-new] [Bug 15360] New: Bluetooth dongle can't be put into UP state
From: cijoml @ 2010-02-23 7:06 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-bluetooth, linux-usb, bugzilla-daemon, bugme-daemon, cijoml
In-Reply-To: <20100222160711.50085d16.akpm@linux-foundation.org>
Hello,
it seems to be more Bluetooth driver's problem, because I see the dongle in
lsusb
Bus 005 Device 002: ID 0b05:b700 ASUSTek Computer, Inc. Broadcom Bluetooth
2.1
Michal
Andrew Morton writes:
>
> (switched to email. Please respond via emailed reply-to-all, not via the
> bugzilla web interface).
>
> On Sat, 20 Feb 2010 08:00:33 GMT bugzilla-daemon@bugzilla.kernel.org wrote:
>
>> http://bugzilla.kernel.org/show_bug.cgi?id=15360
>>
>> Summary: Bluetooth dongle can't be put into UP state
>> Product: Drivers
>> Version: 2.5
>> Kernel Version: 2.6.32.3
>> Platform: All
>> OS/Version: Linux
>> Tree: Mainline
>> Status: NEW
>> Severity: high
>> Priority: P1
>> Component: Bluetooth
>> AssignedTo: drivers_bluetooth@kernel-bugs.osdl.org
>> ReportedBy: cijoml@volny.cz
>> Regression: Yes
>>
>>
>> In 2.6.31:
>> hci0: Type: USB
>> BD Address: 00:15:AF:FA:36:04 ACL MTU: 1021:8 SCO MTU: 64:1
>> UP RUNNING PSCAN
>> RX bytes:1348156 acl:3392 sco:0 events:1253 errors:0
>> TX bytes:306309 acl:2175 sco:0 commands:42 errors:0
>>
>> In 2.6.32:
>> hci0: Type: USB
>> BD Address: 00:00:00:00:00:00 ACL MTU: 1021:8 SCO MTU: 64:1
>> UP RUNNING PSCAN
>> RX bytes:0 acl:0 sco:0 events:0 errors:0
>> TX bytes:0 acl:0 sco:0 commands:42 errors:0
>>
>> Eee901:/home/cijoml# lsusb
>> Bus 004 Device 002: ID 0b05:b700 ASUSTek Computer, Inc. Broadcom Bluetooth 2.1
>
> It's a 2.6.31 -> 2.6.32 regression. Is it likely to be in
> drivers/bluetooth, or in drivers/usb?
>
^ permalink raw reply
* Re: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip
From: Suraj Sumangala @ 2010-02-23 5:42 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Vikram Kandukuri, linux-bluetooth@vger.kernel.org,
Suraj Sumangala, Luis Rodriguez
In-Reply-To: <1266805799.18491.33.camel@violet>
Marcel Holtmann wrote:
> Hi Vikram,
>
>
>> drivers/bluetooth/Kconfig | 10 +
>> drivers/bluetooth/Makefile | 1 +
>> drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++
>> drivers/bluetooth/hci_ath.h | 72 ++++++
>> drivers/bluetooth/hci_ldisc.c | 6 +
>> drivers/bluetooth/hci_uart.h | 8 +-
>> 6 files changed, 641 insertions(+), 1 deletions(-)
>> create mode 100755 drivers/bluetooth/hci_ath.c
>> create mode 100755 drivers/bluetooth/hci_ath.h
>>
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 058fbcc..32b98a4 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -58,6 +58,16 @@ config BT_HCIUART_BCSP
>>
>> Say Y here to compile support for HCI BCSP protocol.
>>
>> +config BT_HCIUART_ATH
>> + bool "Atheros AR3001 Board support"
>> + depends on BT_HCIUART
>> + help
>> + HCIATH (HCI Atheros) is a serial protocol for communication
>> + between Bluetooth device and host. This protocol is required for
>> + serial Bluetooth devices that are based on Atheros AR3001 chips.
>> +
>> + Say Y here to compile support for HCIATH protocol.
>> +
>>
>
> it would be nice if you add a document explaining this special Atheros
> protocol that is used.
>
I will send the document with the required changes
>
>> +static void ath_context_switch(struct work_struct *work)
>> +{
>> + int status;
>> + struct ath_struct *ath;
>> + struct hci_uart *hu;
>> + struct tty_struct *tty;
>> + struct sk_buff *skbuf;
>> + ath = container_of(work, struct ath_struct, ctxtsw);
>> + hu = ath->hu;
>> + tty = hu->tty;
>> + status = ath_wakeup_ar3001(tty);
>> + if ((status & TIOCM_CTS)) {
>> + while ((skbuf = skb_dequeue(&ath->tx_wait_q)))
>> + skb_queue_tail(&ath->txq, skbuf);
>> +
>> + /* Ready to send Data */
>> + clear_bit(HCI_UART_SENDING, &hu->tx_state);
>> + hci_uart_tx_wakeup(hu);
>> + }
>> +}
>> +
>> +/* Initialize protocol */
>> +static int ath_open(struct hci_uart *hu)
>> +{
>> + struct ath_struct *ath;
>> + BT_DBG("hu %p", hu);
>> + ath = kzalloc(sizeof(*ath), GFP_ATOMIC);
>> + if (!ath)
>> + return -ENOMEM;
>> + skb_queue_head_init(&ath->txq);
>> + skb_queue_head_init(&ath->tx_wait_q);
>> + skb_queue_head_init(&ath->tx_cmd_wait_q);
>> + spin_lock_init(&ath->hciath_lock);
>> + ath->cur_sleep = 0;
>> + ath->num_cmds_complete = 1;
>> + hu->priv = ath;
>> + ath->hu = hu;
>> + init_waitqueue_head(&ath->wqevt);
>> + INIT_WORK(&ath->ctxtsw, ath_context_switch);
>> + return 0;
>> +}
>>
>
> In all the function, use at least some empty lines in between so that
> somebody can try to read these function. Cramping everything in less
> space doesn't make it simpler.
>
We will make the necessary changes
>
>> +/* Enqueue frame for transmittion (padding, crc, etc) */
>> +/* may be called from two simultaneous tasklets */
>>
>
> Why are you stealing the comments from hci_ll.c. Are you using actually
> padding and CRC?
>
Will do the cleanup along with the other changes mentioned
>
>> +static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>> +{
>> + struct ath_struct *ath;
>> + ath = hu->priv;
>>
>
> Direct assignment on declaration.
>
Will correct it along with other changes
>
>> + if (HCI_SCODATA_PKT == bt_cb(skb)->pkt_type) {
>>
>
> Always the other way around. Variable compares to constant. Not the
> constant to variable.
>
Will correct it along with other changes
>
>> +static struct sk_buff *ath_dequeue(struct hci_uart *hu)
>> +{
>> + struct ath_struct *ath;
>> + struct sk_buff *skbuff;
>> + ath = hu->priv;
>> + skbuff = skb_dequeue(&ath->txq);
>> + if (NULL != skbuff) {
>>
>
> Never ever use the variable name skbuff and also if (skbuff) here.
> However preferable this is if (!skb) return NULL;
>
I will take care of the variable name change
>
>> + ath_handle_host_data(ath, bt_cb(skbuff)->pkt_type,
>> + &skbuff->data[1], skbuff->len - 1);
>> + }
>> + return skbuff;
>> +}
>> +
>> +static inline int ath_check_data_len(struct ath_struct *ath, int len)
>> +{
>> + register int room = skb_tailroom(ath->rx_skb);
>> + BT_DBG("len %d room %d", len, room);
>> + if (!len) {
>> + hci_recv_frame(ath->rx_skb);
>> + } else if (len > room) {
>> + BT_ERR("Data length is too large");
>> + kfree_skb(ath->rx_skb);
>> + } else {
>> + ath->rx_state = HCIATH_W4_DATA;
>> + ath->rx_count = len;
>> + return len;
>> + }
>>
>
> How is this if statement even suppose to work?
>
Retained this code as this was part of the hci_ll implementation that we used as template. Will make the necessary changes.
>
>> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
>> + ath->rx_skb = NULL;
>> + ath->rx_count = 0;
>> + return 0;
>> +}
>> +
>> +/* Recv data */
>> +static int ath_recv(struct hci_uart *hu, void *data, int count)
>> +{
>> + struct ath_struct *ath = hu->priv;
>> + register char *ptr;
>> + struct hci_event_hdr *eh;
>> + struct hci_acl_hdr *ah;
>> + struct hci_sco_hdr *sh;
>> + struct sk_buff *skbuff;
>> + register int len, type, dlen;
>> + skbuff = NULL;
>> + BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count,
>> + ath->rx_state, ath->rx_count);
>> + ptr = data;
>> + while (count) {
>> + if (ath->rx_count) {
>> + len = min_t(unsigned int, ath->rx_count, count);
>> + memcpy(skb_put(ath->rx_skb, len), ptr, len);
>> + ath->rx_count -= len;
>> + count -= len;
>> + ptr += len;
>> + if (ath->rx_count)
>> + continue;
>> + switch (ath->rx_state) {
>> + case HCIATH_W4_DATA:
>> + ath_handle_data_from_controller(ath,
>> + bt_cb
>> + (ath->rx_skb)->
>> + pkt_type,
>> + ath->rx_skb->
>> + data,
>> + ath->rx_skb->
>> + len);
>> + if (HCI_EVENT_PKT ==
>> + bt_cb(ath->rx_skb)->pkt_type
>> + && ath->num_cmds_complete > 0) {
>> +
>> + skbuff =
>> + skb_dequeue(&ath->tx_cmd_wait_q);
>> + if (skbuff != NULL) {
>> + skb_queue_tail(&ath->txq,
>> + skbuff);
>> + schedule_work(&ath->ctxtsw);
>> + }
>> + }
>> + if (ath_verify_event_discardable
>> + (hu, bt_cb(ath->rx_skb)->pkt_type,
>> + ath->rx_skb->data)) {
>> + kfree(ath->rx_skb);
>> + ath->rx_skb = NULL;
>> + } else {
>> + hci_recv_frame(ath->rx_skb);
>> + }
>> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
>> + ath->rx_skb = NULL;
>> + ath->rx_count = 0;
>> + continue;
>> + case HCIATH_W4_EVENT_HDR:
>> + eh = (struct hci_event_hdr *)ath->rx_skb->data;
>> + BT_DBG("Event header: evt 0x%2.2x plen %d",
>> + eh->evt, eh->plen);
>> + ath_check_data_len(ath, eh->plen);
>> + continue;
>> + case HCIATH_W4_ACL_HDR:
>> + ah = (struct hci_acl_hdr *)ath->rx_skb->data;
>> + dlen = __le16_to_cpu(ah->dlen);
>> + BT_DBG("ACL header: dlen %d", dlen);
>> + ath_check_data_len(ath, dlen);
>> + continue;
>> + case HCIATH_W4_SCO_HDR:
>> + sh = (struct hci_sco_hdr *)ath->rx_skb->data;
>> + BT_DBG("SCO header: dlen %d", sh->dlen);
>> + ath_check_data_len(ath, sh->dlen);
>> + continue;
>> + }
>> + }
>> +
>> + /* HCIATH_W4_PACKET_TYPE */
>> + switch (*ptr) {
>> + case HCI_EVENT_PKT:
>> + BT_DBG("Event packet");
>> + ath->rx_state = HCIATH_W4_EVENT_HDR;
>> + ath->rx_count = HCI_EVENT_HDR_SIZE;
>> + type = HCI_EVENT_PKT;
>> + break;
>> + case HCI_ACLDATA_PKT:
>> + BT_DBG("ACL packet");
>> + ath->rx_state = HCIATH_W4_ACL_HDR;
>> + ath->rx_count = HCI_ACL_HDR_SIZE;
>> + type = HCI_ACLDATA_PKT;
>> + break;
>> + case HCI_SCODATA_PKT:
>> + BT_DBG("SCO packet");
>> + ath->rx_state = HCIATH_W4_SCO_HDR;
>> + ath->rx_count = HCI_SCO_HDR_SIZE;
>> + type = HCI_SCODATA_PKT;
>> + break;
>> + default:
>> + BT_ERR("Unknown HCI packet type %2.2x", (__u8) *ptr);
>> + hu->hdev->stat.err_rx++;
>> + ptr++;
>> + count--;
>> + continue;
>> + };
>> + ptr++;
>> + count--;
>> +
>> + /* Allocate packet */
>> + ath->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE, GFP_ATOMIC);
>> + if (!ath->rx_skb) {
>> + BT_ERR("Can't allocate mem for new packet");
>> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
>> + ath->rx_count = 0;
>> + return 0;
>> + }
>> + ath->rx_skb->dev = (void *)hu->hdev;
>> + bt_cb(ath->rx_skb)->pkt_type = type;
>> + } return count;
>> +}
>>
>
> We have a common exported function called hci_recv_fragment that should
> be doing exactly this.
>
I have provided the explanation at the bottom
>
>> +static void ath_controller_sleep_mode(struct hci_uart *hu, bool enable)
>> +{
>> + unsigned char sleepcmd[] = { 0x01, 0x04, 0xFC, 0x01, 0x00 };
>> + sleepcmd[4] = enable;
>> + ath_write_data_to_cntrlr(hu->hdev, sleepcmd, sizeof(sleepcmd));
>> +}
>>
>
> Special vendor commands need a comment. Explain the data structure of
> them and what are the options.
>
Will add the necessary comments
>
>> +int ath_wakeup_ar3001(struct tty_struct *tty)
>> +{
>> + struct termios settings;
>> + int status = 0x00;
>> + mm_segment_t oldfs;
>> + status = tty->driver->ops->tiocmget(tty, NULL);
>> +
>> + if ((status & TIOCM_CTS))
>> + return status;
>> +
>> + oldfs = get_fs();
>> + set_fs(KERNEL_DS);
>> + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
>> +
>> + settings.c_cflag &= ~CRTSCTS;
>> + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
>> + set_fs(oldfs);
>> + status = tty->driver->ops->tiocmget(tty, NULL);
>> +
>> + tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS);
>> + mdelay(20);
>> +
>> + status = tty->driver->ops->tiocmget(tty, NULL);
>> +
>> + tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00);
>> + mdelay(20);
>> +
>> + status = tty->driver->ops->tiocmget(tty, NULL);
>> + oldfs = get_fs();
>> + set_fs(KERNEL_DS);
>> + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
>> +
>> + settings.c_cflag |= CRTSCTS;
>> + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
>> + set_fs(oldfs);
>> + return status;
>> +}
>>
>
> Why is this magic in kernel space and not inside hciattach?
>
if there is no Bluetooth activity for a while,
so the driver will verify if the controller is sleeping and
try to wake up the controller using the RTS/CTS mechanism.
This has to be done before sending any packet to controller.
This cannot be done in hciattach as this is required to be called throughout the life of the driver and not just when the driver is loaded.
>
>> +int ath_fullboot_config(struct hci_uart *hu, int current_event)
>> +{
>> + struct sk_buff *skbuf;
>> + struct ath_struct *ath = hu->priv;
>> + static int autosleep;
>> + unsigned char rstevt[] = { 0x1, 0x3, 0xc, 0x0 };
>> + if (current_event == HCI_RESET) {
>> +
>> + if (ath->cur_sleep) {
>> + autosleep = 1;
>> + ath_controller_sleep_mode(hu, 1);
>> + return 1;
>> + } else {
>> + return 0;
>> + }
>> + }
>> +
>> + if (current_event == HCI_SET_SLEEP_MODE) {
>> +
>> + if (autosleep == 0)
>> + return 1;
>> +
>> + while ((skbuf = skb_dequeue(&ath->tx_wait_q)))
>> + skb_queue_tail(&ath->txq, skbuf);
>> +
>> + ath_write_data_to_host(hu->hdev, rstevt, sizeof(rstevt));
>> + autosleep = 0;
>> + return 1;
>> + }
>> + return 0;
>> +}
>>
>
> Why is this function not static? And all the others?
>
will make every function static
>
>> +int ath_write_data_to_host(void *dev, unsigned char *data, u8 length)
>> +{
>> + struct sk_buff *skbuf;
>> + struct hci_dev *hdev;
>> + hdev = (struct hci_dev *)dev;
>> + skbuf = bt_skb_alloc(length, GFP_ATOMIC);
>> + if (!skbuf) {
>> + BT_ERR("Memory allocation failed");
>> + return -1;
>> + }
>> + skb_orphan(skbuf);
>> +
>> + /* First byte will be added as packet type */
>> + bt_cb(skbuf)->pkt_type = data[0];
>> + skbuf->dev = (void *)hdev;
>> + memcpy(skb_put(skbuf, length - 1), &data[1], length - 1);
>> + hci_recv_frame(skbuf);
>> + return length;
>> +}
>>
>
> What is this exactly for? Do you have a secondary receive path?
>
Yes, we have situation where driver sends HCI events to depending on acknowledgement of sleep enable vendor specific command.
the reason is explained at the bottom
>
>> +int ath_write_data_to_cntrlr(void *dev, unsigned char *Data, u8 len)
>> +{
>> + struct sk_buff *skbuff;
>> + struct ath_struct *ath;
>> + struct hci_uart *hu;
>> + struct hci_dev *hdev;
>> + if (NULL == dev) {
>> + BT_DBG("NULL handle received %p \n", dev);
>> + return 0;
>> + }
>> + hdev = (struct hci_dev *)dev;
>> + hu = (struct hci_uart *)hdev->driver_data;
>> + if (hu == NULL) {
>> + BT_DBG("NULL handle received %p \n", hdev);
>> + return 0;
>> + }
>> + ath = hu->priv;
>> + if (ath == NULL) {
>> + BT_DBG("NULL handle received \n");
>> + return 0;
>> + }
>> + skbuff = bt_skb_alloc(len, GFP_ATOMIC);
>> + if (skbuff == NULL) {
>> + BT_DBG("Malloc Fail memory %p \n", skbuff);
>> + return 0;
>> + }
>> + skb_orphan(skbuff);
>> +
>> + if (len != 0)
>> + memcpy(skb_put(skbuff, len), Data, len);
>> +
>> + bt_cb(skbuff)->pkt_type = HCI_COMMAND_PKT;
>> + skbuff->dev = dev;
>> + if (ath->num_cmds_complete > 0) {
>> + skb_queue_tail(&ath->txq, skbuff);
>> + schedule_work(&ath->ctxtsw);
>> + } else {
>> + skb_queue_tail(&ath->tx_cmd_wait_q, skbuff);
>> + }
>> + return len;
>> +}
>>
>
> Messing with num_cmds_complete inside the driver is wrong. What are you
> doing here?
>
I have explained the reason at the bottom
>
>> +int ath_check_sleep_cmd(struct ath_struct *ath, unsigned char *packet)
>> +{
>> + if (packet[0] == HCI_EVENT_PKT && packet[1] == 0xFC)
>> + ath->cur_sleep = packet[3];
>>
>
> How is this working. Every vendor event is sleep event?
>
Will correct it. It should have been packet[0] == 0x04.
>
>> +int ath_verify_event_discardable(struct hci_uart *hu, unsigned char pkt_type,
>> + unsigned char *packet)
>> +{
>> + if (pkt_type != HCI_EVENT_PKT)
>> + return 0;
>> +
>> + switch (packet[0]) {
>> + case 0x0E: /* Command Complete Event */
>> + if (packet[3] == 0x03 && packet[4] == 0x0C) {
>> +
>> + /* Command complete for HCI Reset Received */
>> + return ath_fullboot_config(hu, HCI_RESET);
>> + } else if (packet[3] == 0x04 && packet[4] == 0xFC) {
>> + return ath_fullboot_config(hu, HCI_SET_SLEEP_MODE);
>> + }
>> + break;
>> + }
>> + return 0;
>> +}
>>
>
> What are you doing here?
>
Events for vendor specific commands sent from driver need not go to the host. They are discarded at the driver level
>
>> +void ath_handle_hci_event(struct ath_struct *ath, u8 * packet)
>> +{
>> + switch (packet[0]) {
>> + case 0x05: /* ACL Disconnection Complete Event */
>> + break;
>> + case 0x03: /* ACL Connection Complete Event */
>> + break;
>> + case 0x0E: /* Command Complete Event */
>> + spin_lock(&ath->hciath_lock);
>> + ath->num_cmds_complete = packet[2];
>> + spin_unlock(&ath->hciath_lock);
>> + break;
>> + case 0x0F: /* Command Status Event */
>> + spin_lock(&ath->hciath_lock);
>> + ath->num_cmds_complete = packet[3];
>> + spin_unlock(&ath->hciath_lock);
>> + break;
>> + }
>> +}
>>
>
> Something is really wrong in this driver? Are you really know how this
> is suppose to work if you have to interfere this much?
>
I have provided the reason for this implementation at the bottom
>
>> +void ath_handle_host_data(struct ath_struct *ath, u8 pktType, u8 * packet,
>> + unsigned int len)
>> +{
>> + switch (pktType) {
>> + case HCI_ACLDATA_PKT: /* ACL packets */
>> + break;
>> + case HCI_COMMAND_PKT: /* HCI Commands */
>> + ath_handle_hci_cmd(ath, packet);
>> + ath_check_sleep_cmd(ath, packet);
>> + break;
>> + }
>> +}
>> +
>> +void ath_handle_data_from_controller(struct ath_struct *ath, u8 pktType,
>> + u8 *packet, unsigned int len)
>> +{
>> + switch (pktType) {
>> + case HCI_ACLDATA_PKT: /* ACL packets */
>> + break;
>> + case HCI_EVENT_PKT: /* HCI Events */
>> + ath_handle_hci_event(ath, packet);
>> + break;
>> + }
>> +}
>>
>
> Seriously? are you implemented your own Bluetooth stack? Please explain
> what you really need. And then the host stack can provide it for you.
> This is not acceptable.
>
The reason for this implementation is.
The AR3001 supports power management feature that can be enabled from host during hciattach.
This will let the controller go to sleep if there is no data transfer for some time.
This feature is disabled automatically on receiving HCI RESET command and has to be enabled
again by sending Sleep enable Vendor specific command after every HCI Reset command.
This required the driver to keep track of HCI RESET command complete event and sent Sleep Enable command on receiving the event.
Host will be notified of the HCI Reset command complete event only after the Sleep enable command is acknowledged by controller.
This is the reason for the following implementation changes
1. We had to avoid using hci_recv_fragment as we need to verify and decide if the packet has to be sent to Host (In case of HCI RESET).
2. We had to add ath_write_data_to_host so that we can sent HCI RESET
command complete on receiving Sleep Enable command complete event from controller.
3. We had to add ath_write_data_to_cntrlr so that driver can send the Sleep enable command after every HCI RESET.
4. Have ath_verify_event_discardable which keep track of the HCI RESET and Sleep Enable Command complete event
and decide whether to send the HCI RESET event to host or send only after sleep enable.
Regarding the use of num_cmds_complete inside driver
Due to a controller specific optimization, we have to send vendor specific commands from the driver to configure the controller depending on the status of ACL connection.
This might cause driver to send a command to controller when it is already busy with command sent from host.
That is the reason why we had to go for streamlining the command flow in the driver.
If you can export hci_send_cmd API in hci_core.c, then we might be able to use it to send a command to controller without worrying about command streamlining.
Please do let me know your comments,
Regards
Suraj
> Regards
>
> Marcel
>
>
>
^ permalink raw reply
* Re: [Bugme-new] [Bug 15360] New: Bluetooth dongle can't be put into UP state
From: Andrew Morton @ 2010-02-23 0:07 UTC (permalink / raw)
To: linux-bluetooth, linux-usb; +Cc: bugzilla-daemon, bugme-daemon, cijoml
In-Reply-To: <bug-15360-10286@http.bugzilla.kernel.org/>
(switched to email. Please respond via emailed reply-to-all, not via the
bugzilla web interface).
On Sat, 20 Feb 2010 08:00:33 GMT bugzilla-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=15360
>
> Summary: Bluetooth dongle can't be put into UP state
> Product: Drivers
> Version: 2.5
> Kernel Version: 2.6.32.3
> Platform: All
> OS/Version: Linux
> Tree: Mainline
> Status: NEW
> Severity: high
> Priority: P1
> Component: Bluetooth
> AssignedTo: drivers_bluetooth@kernel-bugs.osdl.org
> ReportedBy: cijoml@volny.cz
> Regression: Yes
>
>
> In 2.6.31:
> hci0: Type: USB
> BD Address: 00:15:AF:FA:36:04 ACL MTU: 1021:8 SCO MTU: 64:1
> UP RUNNING PSCAN
> RX bytes:1348156 acl:3392 sco:0 events:1253 errors:0
> TX bytes:306309 acl:2175 sco:0 commands:42 errors:0
>
> In 2.6.32:
> hci0: Type: USB
> BD Address: 00:00:00:00:00:00 ACL MTU: 1021:8 SCO MTU: 64:1
> UP RUNNING PSCAN
> RX bytes:0 acl:0 sco:0 events:0 errors:0
> TX bytes:0 acl:0 sco:0 commands:42 errors:0
>
> Eee901:/home/cijoml# lsusb
> Bus 004 Device 002: ID 0b05:b700 ASUSTek Computer, Inc. Broadcom Bluetooth 2.1
It's a 2.6.31 -> 2.6.32 regression. Is it likely to be in
drivers/bluetooth, or in drivers/usb?
^ permalink raw reply
* Re: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip
From: Luis R. Rodriguez @ 2010-02-22 16:11 UTC (permalink / raw)
To: Suraj Sumangala
Cc: Marcel Holtmann, Vikram Kandukuri,
linux-bluetooth@vger.kernel.org, Luis Rodriguez
In-Reply-To: <44EE5C37ADC36343B0625A05DD408C48508B1F5BA1@CHEXMB-01.global.atheros.com>
On Mon, Feb 22, 2010 at 12:54:44AM -0800, Suraj Sumangala wrote:
Suraj,
please see:
http://en.wikipedia.org/wiki/Posting_style#Bottom-posting
This will be easier on you if you use a mail user agent where
bottom posting is easily done from, like Thunderbird, or mutt.
It was very hard to read your reply to Marcel.
Luis
^ permalink raw reply
* Automatically connect to Bluetooth devices?
From: Caleb Katz @ 2010-02-22 13:47 UTC (permalink / raw)
To: Linux-Bluetooth
[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]
Hey,
How do I make a Bluetooth keyboard and mouse connect automatically on system startup? If that is not possible (and I would understand security reasons why it would not be), how do I make Bluez/bluetoothd automatically accept their connections?
In a slightly longer form:
My Bluetooth keyboard and mouse are able to behave as a standard USB keyboard/mouse (where the Bluetooth connection is hidden from the hardware/software) but the extra features like the scroll wheel and media keys do not work. In order to use them, I have to configure both as Bluetooth devices.
It is easy enough to get them working. My distro (Slackware) already starts bluetoothd at boot, and the command "hidd --search --master" can find both after I press the "Connect" button on each. But that is the problem. I have to run a command after telling them to find the computer. When this combo is the only keyboard and mouse available, it is a little difficult to do that, especially when they automatically disconnect after a timeout in order to conserve battery power.
Last night I got frustrated and wrote a short script that is launched into the background by the local init script. I'm attaching it (rocketfish.sh) for critique but it does increase my system load slightly. Please tell me there is a better way.
I have the hardware address for the keyboard and mouse and have added this to my /etc/bluetooth/hcid.conf as someone elsewhere recommended (I have been unable to find documentation for it):
device 00:02:76:03:3A:EA {
name "RocketFish Keyboard";
auth enable;
encrypt enable;
}
device 00:02:76:0A:D2:2E {
name "RocketFish Mouse";
}
When I run "bluetoothd -n -d," there are messages that indicate it detects the keyboard and mouse -- even when a key is pressed -- but then it removes their temporary entry.
Obligatory system information:
Slackware 13/kernel 2.6.29.5 (kernel was from pre-release)
Bluez v4.47
Keyboard and Mouse are models RF-BTKB2 and RF-BTMSE2
My main.conf and hcid.conf are both attached.
Thanks,
Caleb
[-- Attachment #2: rocketfish.sh --]
[-- Type: application/x-shellscript, Size: 544 bytes --]
[-- Attachment #3: main.conf --]
[-- Type: application/octet-stream, Size: 1919 bytes --]
[General]
# List of plugins that should not be loaded on bluetoothd startup
#DisablePlugins = network,input
# Default adaper name
# %h - substituted for hostname
# %d - substituted for adapter id
Name = %h-%d
# Default device class. Only the major and minor device class bits are
# considered.
Class = 0x000100
# How long to stay in discoverable mode before going back to non-discoverable
# The value is in seconds. Default is 180, i.e. 3 minutes.
# 0 = disable timer, i.e. stay discoverable forever
DiscoverableTimeout = 0
# How long to stay in pairable mode before going back to non-discoverable
# The value is in seconds. Default is 0.
# 0 = disable timer, i.e. stay pairable forever
PairableTimeout = 0
# Use some other page timeout than the controller default one
# which is 16384 (10 seconds).
PageTimeout = 8192
# Discover scheduler interval used in Adapter.DiscoverDevices
# The value is in seconds. Defaults is 0 to use controller scheduler.
#DiscoverSchedulerInterval = 10
# What value should be assumed for the adapter Powered property when
# SetProperty(Powered, ...) hasn't been called yet. Defaults to true
InitiallyPowered = true
# Remember the previously stored Powered state when initializing adapters
RememberPowered = true
# Use vendor, product and version information for DID profile support.
# The values are separated by ":" and VID, PID and version.
#DeviceID = 1234:5678:abcd
# Do reverse service discovery for previously unknown devices that connect to
# us. This option is really only needed for qualification since the BITE tester
# doesn't like us doing reverse SDP for some test cases (though there could in
# theory be other useful purposes for this too). Defaults to true.
ReverseServiceDiscovery = true
# Enable name resolving after inquiry. Set it to 'false' if you don't need
# remote devices name and want shorter discovery cycle. Defaults to 'true'.
NameResolving = true
[-- Attachment #4: hcid.conf --]
[-- Type: application/octet-stream, Size: 1407 bytes --]
#
# HCI daemon configuration file.
#
# HCId options
options {
# Automatically initialize new devices
autoinit yes;
# Security Manager mode
# none - Security manager disabled
# auto - Use local PIN for incoming connections
# user - Always ask user for a PIN
#
security user;
# Pairing mode
# none - Pairing disabled
# multi - Allow pairing with already paired devices
# once - Pair once and deny successive attempts
pairing multi;
# Default PIN code for incoming connections
passkey "6421";
}
# Default settings for HCI devices
device {
# Local device name
# %d - device id
# %h - host name
name "%h-%d";
# Local device class
#class 0x000100;
class 0x520204;
# Default packet type
#pkt_type DH1,DM1,HV1;
# Inquiry and Page scan
iscan enable; pscan enable;
discovto 0;
# Default link mode
# none - no specific policy
# accept - always accept incoming connections
# master - become master on incoming connections,
# deny role switch on outgoing connections
lm accept;
# Default link policy
# none - no specific policy
# rswitch - allow role switch
# hold - allow hold mode
# sniff - allow sniff mode
# park - allow park mode
lp rswitch,hold,sniff,park;
}
device 00:02:76:03:3A:EA {
name "RocketFish Keyboard";
auth enable;
encrypt enable;
}
device 00:02:76:0A:D2:2E {
name "RocketFish Mouse";
}
^ permalink raw reply
* [bluetooth-next] bluetooth: hci_sysfs: use strict_strtoul instead of simple_strtoul
From: Tomas Winkler @ 2010-02-22 13:05 UTC (permalink / raw)
To: marcel, linux-bluetooth; +Cc: guy.cohen, ron.rindjunsky, Tomas Winkler
In-Reply-To: <1266843933-28802-1-git-send-email-tomas.winkler@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
net/bluetooth/hci_sysfs.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 2bc6f6a..e4bfce5 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -286,11 +286,11 @@ static ssize_t show_idle_timeout(struct device *dev, struct device_attribute *at
static ssize_t store_idle_timeout(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
{
struct hci_dev *hdev = dev_get_drvdata(dev);
- char *ptr;
- __u32 val;
+ unsigned long val;
+ int ret;
- val = simple_strtoul(buf, &ptr, 10);
- if (ptr == buf)
+ ret = strict_strtoul(buf, 0, &val);
+ if (ret)
return -EINVAL;
if (val != 0 && (val < 500 || val > 3600000))
@@ -310,11 +310,11 @@ static ssize_t show_sniff_max_interval(struct device *dev, struct device_attribu
static ssize_t store_sniff_max_interval(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
{
struct hci_dev *hdev = dev_get_drvdata(dev);
- char *ptr;
- __u16 val;
+ unsigned long val;
+ int ret;
- val = simple_strtoul(buf, &ptr, 10);
- if (ptr == buf)
+ ret = strict_strtoul(buf, 0, &val);
+ if (ret)
return -EINVAL;
if (val < 0x0002 || val > 0xFFFE || val % 2)
@@ -337,11 +337,11 @@ static ssize_t show_sniff_min_interval(struct device *dev, struct device_attribu
static ssize_t store_sniff_min_interval(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
{
struct hci_dev *hdev = dev_get_drvdata(dev);
- char *ptr;
- __u16 val;
+ unsigned long val;
+ int ret;
- val = simple_strtoul(buf, &ptr, 10);
- if (ptr == buf)
+ ret = strict_strtoul(buf, 0, &val);
+ if (ret)
return -EINVAL;
if (val < 0x0002 || val > 0xFFFE || val % 2)
--
1.6.0.6
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply related
* [bluetooth-next V2] Bluetooth: handle device reset event
From: Tomas Winkler @ 2010-02-22 13:05 UTC (permalink / raw)
To: marcel, linux-bluetooth; +Cc: guy.cohen, ron.rindjunsky, Gregory Paskar
From: Gregory Paskar <gregory.paskar@intel.com>
A Bluetooth device experiencing hardware failure may issue
a HARDWARE_ERROR hci event. The reaction to this event is device
reset flow implemented in following sequence.
1. Notify: HCI_DEV_DOWN
2. Reinitialize internal structures.
3. Call driver flush function
4. Send HCI reset request to the device.
5. Send HCI init sequence reset to the device.
6. Notify HCI_DEV_UP.
Signed-off-by: Gregory Paskar <gregory.paskar@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Reviewed-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
V2:
1. Fix email typo
2. Fix style issues
include/net/bluetooth/hci.h | 5 ++++
include/net/bluetooth/hci_core.h | 2 +
net/bluetooth/hci_core.c | 47 +++++++++++++++++++++++++++++++++++++-
net/bluetooth/hci_event.c | 3 ++
4 files changed, 56 insertions(+), 1 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index ed3aea1..cd23eb4 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -691,6 +691,11 @@ struct hci_ev_cmd_status {
__le16 opcode;
} __attribute__ ((packed));
+#define HCI_EV_HARDWARE_ERROR 0x10
+struct hci_ev_hw_error {
+ __u8 hw_code;
+} __attribute__ ((packed));
+
#define HCI_EV_ROLE_CHANGE 0x12
struct hci_ev_role_change {
__u8 status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 7b86094..d2b1cba 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -110,6 +110,8 @@ struct hci_dev {
struct tasklet_struct rx_task;
struct tasklet_struct tx_task;
+ struct work_struct hw_err_work;
+
struct sk_buff_head rx_q;
struct sk_buff_head raw_q;
struct sk_buff_head cmd_q;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 94ba349..b7c0fbf 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -834,6 +834,51 @@ static int hci_rfkill_set_block(void *data, bool blocked)
return 0;
}
+static void hci_device_hw_error(struct hci_dev *hdev)
+{
+ hci_req_lock(hdev);
+
+ /* notify: going down */
+ clear_bit(HCI_UP, &hdev->flags);
+ hci_notify(hdev, HCI_DEV_DOWN);
+
+ tasklet_disable(&hdev->tx_task);
+ skb_queue_purge(&hdev->rx_q);
+ skb_queue_purge(&hdev->cmd_q);
+
+ hci_dev_lock_bh(hdev);
+ inquiry_cache_flush(hdev);
+ hci_conn_hash_flush(hdev);
+ hci_dev_unlock_bh(hdev);
+
+ if (hdev->flush)
+ hdev->flush(hdev);
+
+ /* reset device */
+ atomic_set(&hdev->cmd_cnt, 1);
+ hdev->acl_cnt = 0;
+ hdev->sco_cnt = 0;
+ hci_reset_req(hdev, 0);
+
+ /* init device */
+ atomic_set(&hdev->cmd_cnt, 1);
+ hci_init_req(hdev, 0);
+
+ tasklet_enable(&hdev->tx_task);
+
+ /* notify: going back up */
+ set_bit(HCI_UP, &hdev->flags);
+ hci_notify(hdev, HCI_DEV_UP);
+
+ hci_req_unlock(hdev);
+}
+
+static void hci_hw_err_work(struct work_struct *ws)
+{
+ struct hci_dev *hdev = container_of(ws, struct hci_dev, hw_err_work);
+ hci_device_hw_error(hdev);
+}
+
static const struct rfkill_ops hci_rfkill_ops = {
.set_block = hci_rfkill_set_block,
};
@@ -903,7 +948,7 @@ int hci_register_dev(struct hci_dev *hdev)
tasklet_init(&hdev->cmd_task, hci_cmd_task,(unsigned long) hdev);
tasklet_init(&hdev->rx_task, hci_rx_task, (unsigned long) hdev);
tasklet_init(&hdev->tx_task, hci_tx_task, (unsigned long) hdev);
-
+ INIT_WORK(&hdev->hw_err_work, hci_hw_err_work);
skb_queue_head_init(&hdev->rx_q);
skb_queue_head_init(&hdev->cmd_q);
skb_queue_head_init(&hdev->raw_q);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 28517ba..e8f48cc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1953,6 +1953,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
case HCI_EV_REMOTE_HOST_FEATURES:
hci_remote_host_features_evt(hdev, skb);
break;
+ case HCI_EV_HARDWARE_ERROR:
+ schedule_work(&hdev->hw_err_work);
+ break;
default:
BT_DBG("%s event 0x%x", hdev->name, event);
--
1.6.0.6
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply related
* Doubt Regarding " SCO over PCM "
From: bluezprofile kernel @ 2010-02-22 11:04 UTC (permalink / raw)
To: linux-bluetooth, Marcel Holtmann
Hi All
I have the following doubts regarding "SCO ove PCM" with BlueZ ;
I configure configured SCORouting=PCM in /etc/blueooth/audion.conf and
connected to head set
1) if check "hcitool con" there is no SCO connection and sound is
routing to local PCM
Is this behavior is correct ?
2) With the same configuration if I use Skype ; there is a SCO
connection and audio is not routing to local PCM
Does any body tried SCO over PCM using Skype.
I am using pulse audio 0.9.21 . please help me
Thanks
^ permalink raw reply
* RE: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip
From: Suraj Sumangala @ 2010-02-22 8:54 UTC (permalink / raw)
To: Marcel Holtmann, Vikram Kandukuri
Cc: linux-bluetooth@vger.kernel.org, Suraj Sumangala, Luis Rodriguez
In-Reply-To: <1266805799.18491.33.camel@violet>
-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]
Sent: Monday, February 22, 2010 8:00 AM
To: Vikram Kandukuri
Cc: linux-bluetooth@vger.kernel.org; Suraj Sumangala; Luis Rodriguez
Subject: Re: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip
Hi Vikram,
> drivers/bluetooth/Kconfig | 10 +
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_ath.h | 72 ++++++
> drivers/bluetooth/hci_ldisc.c | 6 +
> drivers/bluetooth/hci_uart.h | 8 +-
> 6 files changed, 641 insertions(+), 1 deletions(-)
> create mode 100755 drivers/bluetooth/hci_ath.c
> create mode 100755 drivers/bluetooth/hci_ath.h
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 058fbcc..32b98a4 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -58,6 +58,16 @@ config BT_HCIUART_BCSP
>
> Say Y here to compile support for HCI BCSP protocol.
>
> +config BT_HCIUART_ATH
> + bool "Atheros AR3001 Board support"
> + depends on BT_HCIUART
> + help
> + HCIATH (HCI Atheros) is a serial protocol for communication
> + between Bluetooth device and host. This protocol is required for
> + serial Bluetooth devices that are based on Atheros AR3001 chips.
> +
> + Say Y here to compile support for HCIATH protocol.
> +
it would be nice if you add a document explaining this special Atheros
protocol that is used.
>> We will do the same
> +static void ath_context_switch(struct work_struct *work)
> +{
> + int status;
> + struct ath_struct *ath;
> + struct hci_uart *hu;
> + struct tty_struct *tty;
> + struct sk_buff *skbuf;
> + ath = container_of(work, struct ath_struct, ctxtsw);
> + hu = ath->hu;
> + tty = hu->tty;
> + status = ath_wakeup_ar3001(tty);
> + if ((status & TIOCM_CTS)) {
> + while ((skbuf = skb_dequeue(&ath->tx_wait_q)))
> + skb_queue_tail(&ath->txq, skbuf);
> +
> + /* Ready to send Data */
> + clear_bit(HCI_UART_SENDING, &hu->tx_state);
> + hci_uart_tx_wakeup(hu);
> + }
> +}
> +
> +/* Initialize protocol */
> +static int ath_open(struct hci_uart *hu)
> +{
> + struct ath_struct *ath;
> + BT_DBG("hu %p", hu);
> + ath = kzalloc(sizeof(*ath), GFP_ATOMIC);
> + if (!ath)
> + return -ENOMEM;
> + skb_queue_head_init(&ath->txq);
> + skb_queue_head_init(&ath->tx_wait_q);
> + skb_queue_head_init(&ath->tx_cmd_wait_q);
> + spin_lock_init(&ath->hciath_lock);
> + ath->cur_sleep = 0;
> + ath->num_cmds_complete = 1;
> + hu->priv = ath;
> + ath->hu = hu;
> + init_waitqueue_head(&ath->wqevt);
> + INIT_WORK(&ath->ctxtsw, ath_context_switch);
> + return 0;
> +}
In all the function, use at least some empty lines in between so that
somebody can try to read these function. Cramping everything in less
space doesn't make it simpler.
>> We will make the necessary changes
> +/* Enqueue frame for transmittion (padding, crc, etc) */
> +/* may be called from two simultaneous tasklets */
Why are you stealing the comments from hci_ll.c. Are you using actually
padding and CRC?
>> Cleanup will be done
> +static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> +{
> + struct ath_struct *ath;
> + ath = hu->priv;
Direct assignment on declaration.
>> Will be done
> + if (HCI_SCODATA_PKT == bt_cb(skb)->pkt_type) {
Always the other way around. Variable compares to constant. Not the
constant to variable.
>> Will change it accordingly
> +static struct sk_buff *ath_dequeue(struct hci_uart *hu)
> +{
> + struct ath_struct *ath;
> + struct sk_buff *skbuff;
> + ath = hu->priv;
> + skbuff = skb_dequeue(&ath->txq);
> + if (NULL != skbuff) {
Never ever use the variable name skbuff and also if (skbuff) here.
However preferable this is if (!skb) return NULL;
>> Will change the variable name and implementation as mentioned
> + ath_handle_host_data(ath, bt_cb(skbuff)->pkt_type,
> + &skbuff->data[1], skbuff->len - 1);
> + }
> + return skbuff;
> +}
> +
> +static inline int ath_check_data_len(struct ath_struct *ath, int len)
> +{
> + register int room = skb_tailroom(ath->rx_skb);
> + BT_DBG("len %d room %d", len, room);
> + if (!len) {
> + hci_recv_frame(ath->rx_skb);
> + } else if (len > room) {
> + BT_ERR("Data length is too large");
> + kfree_skb(ath->rx_skb);
> + } else {
> + ath->rx_state = HCIATH_W4_DATA;
> + ath->rx_count = len;
> + return len;
> + }
How is this if statement even suppose to work?
>> This implementation was there in previous hci_ll and other implementation which we have used as our template
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_skb = NULL;
> + ath->rx_count = 0;
> + return 0;
> +}
> +
> +/* Recv data */
> +static int ath_recv(struct hci_uart *hu, void *data, int count)
> +{
> + struct ath_struct *ath = hu->priv;
> + register char *ptr;
> + struct hci_event_hdr *eh;
> + struct hci_acl_hdr *ah;
> + struct hci_sco_hdr *sh;
> + struct sk_buff *skbuff;
> + register int len, type, dlen;
> + skbuff = NULL;
> + BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count,
> + ath->rx_state, ath->rx_count);
> + ptr = data;
> + while (count) {
> + if (ath->rx_count) {
> + len = min_t(unsigned int, ath->rx_count, count);
> + memcpy(skb_put(ath->rx_skb, len), ptr, len);
> + ath->rx_count -= len;
> + count -= len;
> + ptr += len;
> + if (ath->rx_count)
> + continue;
> + switch (ath->rx_state) {
> + case HCIATH_W4_DATA:
> + ath_handle_data_from_controller(ath,
> + bt_cb
> + (ath->rx_skb)->
> + pkt_type,
> + ath->rx_skb->
> + data,
> + ath->rx_skb->
> + len);
> + if (HCI_EVENT_PKT ==
> + bt_cb(ath->rx_skb)->pkt_type
> + && ath->num_cmds_complete > 0) {
> +
> + skbuff =
> + skb_dequeue(&ath->tx_cmd_wait_q);
> + if (skbuff != NULL) {
> + skb_queue_tail(&ath->txq,
> + skbuff);
> + schedule_work(&ath->ctxtsw);
> + }
> + }
> + if (ath_verify_event_discardable
> + (hu, bt_cb(ath->rx_skb)->pkt_type,
> + ath->rx_skb->data)) {
> + kfree(ath->rx_skb);
> + ath->rx_skb = NULL;
> + } else {
> + hci_recv_frame(ath->rx_skb);
> + }
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_skb = NULL;
> + ath->rx_count = 0;
> + continue;
> + case HCIATH_W4_EVENT_HDR:
> + eh = (struct hci_event_hdr *)ath->rx_skb->data;
> + BT_DBG("Event header: evt 0x%2.2x plen %d",
> + eh->evt, eh->plen);
> + ath_check_data_len(ath, eh->plen);
> + continue;
> + case HCIATH_W4_ACL_HDR:
> + ah = (struct hci_acl_hdr *)ath->rx_skb->data;
> + dlen = __le16_to_cpu(ah->dlen);
> + BT_DBG("ACL header: dlen %d", dlen);
> + ath_check_data_len(ath, dlen);
> + continue;
> + case HCIATH_W4_SCO_HDR:
> + sh = (struct hci_sco_hdr *)ath->rx_skb->data;
> + BT_DBG("SCO header: dlen %d", sh->dlen);
> + ath_check_data_len(ath, sh->dlen);
> + continue;
> + }
> + }
> +
> + /* HCIATH_W4_PACKET_TYPE */
> + switch (*ptr) {
> + case HCI_EVENT_PKT:
> + BT_DBG("Event packet");
> + ath->rx_state = HCIATH_W4_EVENT_HDR;
> + ath->rx_count = HCI_EVENT_HDR_SIZE;
> + type = HCI_EVENT_PKT;
> + break;
> + case HCI_ACLDATA_PKT:
> + BT_DBG("ACL packet");
> + ath->rx_state = HCIATH_W4_ACL_HDR;
> + ath->rx_count = HCI_ACL_HDR_SIZE;
> + type = HCI_ACLDATA_PKT;
> + break;
> + case HCI_SCODATA_PKT:
> + BT_DBG("SCO packet");
> + ath->rx_state = HCIATH_W4_SCO_HDR;
> + ath->rx_count = HCI_SCO_HDR_SIZE;
> + type = HCI_SCODATA_PKT;
> + break;
> + default:
> + BT_ERR("Unknown HCI packet type %2.2x", (__u8) *ptr);
> + hu->hdev->stat.err_rx++;
> + ptr++;
> + count--;
> + continue;
> + };
> + ptr++;
> + count--;
> +
> + /* Allocate packet */
> + ath->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE, GFP_ATOMIC);
> + if (!ath->rx_skb) {
> + BT_ERR("Can't allocate mem for new packet");
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_count = 0;
> + return 0;
> + }
> + ath->rx_skb->dev = (void *)hu->hdev;
> + bt_cb(ath->rx_skb)->pkt_type = type;
> + } return count;
> +}
We have a common exported function called hci_recv_fragment that should
be doing exactly this.
>> Explained at bottom
> +static void ath_controller_sleep_mode(struct hci_uart *hu, bool enable)
> +{
> + unsigned char sleepcmd[] = { 0x01, 0x04, 0xFC, 0x01, 0x00 };
> + sleepcmd[4] = enable;
> + ath_write_data_to_cntrlr(hu->hdev, sleepcmd, sizeof(sleepcmd));
> +}
Special vendor commands need a comment. Explain the data structure of
them and what are the options.
>> Will make the necessary changes
> +int ath_wakeup_ar3001(struct tty_struct *tty)
> +{
> + struct termios settings;
> + int status = 0x00;
> + mm_segment_t oldfs;
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + if ((status & TIOCM_CTS))
> + return status;
> +
> + oldfs = get_fs();
> + set_fs(KERNEL_DS);
> + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
> +
> + settings.c_cflag &= ~CRTSCTS;
> + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
> + set_fs(oldfs);
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS);
> + mdelay(20);
> +
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00);
> + mdelay(20);
> +
> + status = tty->driver->ops->tiocmget(tty, NULL);
> + oldfs = get_fs();
> + set_fs(KERNEL_DS);
> + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
> +
> + settings.c_cflag |= CRTSCTS;
> + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
> + set_fs(oldfs);
> + return status;
> +}
Why is this magic in kernel space and not inside hciattach?
>> We have a sleep feature which makes the controller go to deep sleep
if there is no Bluetooth activity for a while,
so the driver will verify if the controller is sleeping and
try to wake up the controller using the RTS/CTS mechanism.
This has to be done before sending any packet to controller.
This cannot be done in hciattach as this is required to be called throughout the life of the driver and not just when the driver is loaded.
> +int ath_fullboot_config(struct hci_uart *hu, int current_event)
> +{
> + struct sk_buff *skbuf;
> + struct ath_struct *ath = hu->priv;
> + static int autosleep;
> + unsigned char rstevt[] = { 0x1, 0x3, 0xc, 0x0 };
> + if (current_event == HCI_RESET) {
> +
> + if (ath->cur_sleep) {
> + autosleep = 1;
> + ath_controller_sleep_mode(hu, 1);
> + return 1;
> + } else {
> + return 0;
> + }
> + }
> +
> + if (current_event == HCI_SET_SLEEP_MODE) {
> +
> + if (autosleep == 0)
> + return 1;
> +
> + while ((skbuf = skb_dequeue(&ath->tx_wait_q)))
> + skb_queue_tail(&ath->txq, skbuf);
> +
> + ath_write_data_to_host(hu->hdev, rstevt, sizeof(rstevt));
> + autosleep = 0;
> + return 1;
> + }
> + return 0;
> +}
Why is this function not static? And all the others?
>> Will make all the functions static
> +int ath_write_data_to_host(void *dev, unsigned char *data, u8 length)
> +{
> + struct sk_buff *skbuf;
> + struct hci_dev *hdev;
> + hdev = (struct hci_dev *)dev;
> + skbuf = bt_skb_alloc(length, GFP_ATOMIC);
> + if (!skbuf) {
> + BT_ERR("Memory allocation failed");
> + return -1;
> + }
> + skb_orphan(skbuf);
> +
> + /* First byte will be added as packet type */
> + bt_cb(skbuf)->pkt_type = data[0];
> + skbuf->dev = (void *)hdev;
> + memcpy(skb_put(skbuf, length - 1), &data[1], length - 1);
> + hci_recv_frame(skbuf);
> + return length;
> +}
What is this exactly for? Do you have a secondary receive path?
>> Yes, we have situation where driver sends HCI events to depending on acknowledgement of sleep enable vendor specific command.
Explained at the bottom.
> +int ath_write_data_to_cntrlr(void *dev, unsigned char *Data, u8 len)
> +{
> + struct sk_buff *skbuff;
> + struct ath_struct *ath;
> + struct hci_uart *hu;
> + struct hci_dev *hdev;
> + if (NULL == dev) {
> + BT_DBG("NULL handle received %p \n", dev);
> + return 0;
> + }
> + hdev = (struct hci_dev *)dev;
> + hu = (struct hci_uart *)hdev->driver_data;
> + if (hu == NULL) {
> + BT_DBG("NULL handle received %p \n", hdev);
> + return 0;
> + }
> + ath = hu->priv;
> + if (ath == NULL) {
> + BT_DBG("NULL handle received \n");
> + return 0;
> + }
> + skbuff = bt_skb_alloc(len, GFP_ATOMIC);
> + if (skbuff == NULL) {
> + BT_DBG("Malloc Fail memory %p \n", skbuff);
> + return 0;
> + }
> + skb_orphan(skbuff);
> +
> + if (len != 0)
> + memcpy(skb_put(skbuff, len), Data, len);
> +
> + bt_cb(skbuff)->pkt_type = HCI_COMMAND_PKT;
> + skbuff->dev = dev;
> + if (ath->num_cmds_complete > 0) {
> + skb_queue_tail(&ath->txq, skbuff);
> + schedule_work(&ath->ctxtsw);
> + } else {
> + skb_queue_tail(&ath->tx_cmd_wait_q, skbuff);
> + }
> + return len;
> +}
Messing with num_cmds_complete inside the driver is wrong. What are you
doing here?
>> Explanation given at bottom
> +int ath_check_sleep_cmd(struct ath_struct *ath, unsigned char *packet)
> +{
> + if (packet[0] == HCI_EVENT_PKT && packet[1] == 0xFC)
> + ath->cur_sleep = packet[3];
How is this working. Every vendor event is sleep event?
>> Will correct it. It should have been packet[0] == 0x04.
> +int ath_verify_event_discardable(struct hci_uart *hu, unsigned char pkt_type,
> + unsigned char *packet)
> +{
> + if (pkt_type != HCI_EVENT_PKT)
> + return 0;
> +
> + switch (packet[0]) {
> + case 0x0E: /* Command Complete Event */
> + if (packet[3] == 0x03 && packet[4] == 0x0C) {
> +
> + /* Command complete for HCI Reset Received */
> + return ath_fullboot_config(hu, HCI_RESET);
> + } else if (packet[3] == 0x04 && packet[4] == 0xFC) {
> + return ath_fullboot_config(hu, HCI_SET_SLEEP_MODE);
> + }
> + break;
> + }
> + return 0;
> +}
What are you doing here?
>> Events for commands sent from driver need not go to the host. They are discarded at the driver level.
> +void ath_handle_hci_event(struct ath_struct *ath, u8 * packet)
> +{
> + switch (packet[0]) {
> + case 0x05: /* ACL Disconnection Complete Event */
> + break;
> + case 0x03: /* ACL Connection Complete Event */
> + break;
> + case 0x0E: /* Command Complete Event */
> + spin_lock(&ath->hciath_lock);
> + ath->num_cmds_complete = packet[2];
> + spin_unlock(&ath->hciath_lock);
> + break;
> + case 0x0F: /* Command Status Event */
> + spin_lock(&ath->hciath_lock);
> + ath->num_cmds_complete = packet[3];
> + spin_unlock(&ath->hciath_lock);
> + break;
> + }
> +}
Something is really wrong in this driver? Are you really know how this
is suppose to work if you have to interfere this much?
>> Explanation given at the bottom
> +void ath_handle_host_data(struct ath_struct *ath, u8 pktType, u8 * packet,
> + unsigned int len)
> +{
> + switch (pktType) {
> + case HCI_ACLDATA_PKT: /* ACL packets */
> + break;
> + case HCI_COMMAND_PKT: /* HCI Commands */
> + ath_handle_hci_cmd(ath, packet);
> + ath_check_sleep_cmd(ath, packet);
> + break;
> + }
> +}
> +
> +void ath_handle_data_from_controller(struct ath_struct *ath, u8 pktType,
> + u8 *packet, unsigned int len)
> +{
> + switch (pktType) {
> + case HCI_ACLDATA_PKT: /* ACL packets */
> + break;
> + case HCI_EVENT_PKT: /* HCI Events */
> + ath_handle_hci_event(ath, packet);
> + break;
> + }
> +}
Seriously? are you implemented your own Bluetooth stack? Please explain
what you really need. And then the host stack can provide it for you.
This is not acceptable.
>> The AR3001 supports power management feature that can be enabled from host during hciattach.
This will let the controller go to sleep if there is no data transfer for some time.
This feature will be disabled automatically on receiving HCI RESET command and has to be enabled
Again by sending Sleep enable Vendor specific command after every Reset.
This required the driver to keep track of HCI RESET command complete event and sent Sleep Enable command on receiving the event.
Host will be notified of the HCI Reset command complete event only after the Sleep enable command is acknowledged by controller.
This is the reason for the following implementation changes
1. We had to avoid using hci_recv_fragment as we need to verify and decide if the packet has to be sent to Host (In case of HCI RESET).
2. We had to add ath_write_data_to_host so that we can sent HCI RESET
command complete on receiving Sleep Enable command complete event from controller.
3. We had to add ath_write_data_to_cntrlr so that driver can send the Sleep enable command after every HCI RESET.
4. Have ath_verify_event_discardable which keep track of the HCI RESET and Sleep Enable Command complete event
and decide whether to send the HCI RESET event to host or send only after sleep enable.
>> Regarding the use of num_cmds_complete inside driver,
we have to send vendor specific commands from the driver to configure the controller depending on the status of ACL connection.
So, there is a chance that the this might cause driver to send a command to controller
when it is already busy with command sent from host.
That is the reason why we had to go for streamlining the command flow in the driver.
If you can export hci_send_cmd API in hci_core.c, then we might be able to use it to send a command to controller without worrying about command streamlining.
Please do let me know your comments,
Regards
Suraj
^ permalink raw reply
* Re: [PATCH] Bluetooth: Allow SCO/eSCO packet type selection for outgoing SCO connections.
From: Ville Tervo @ 2010-02-22 7:53 UTC (permalink / raw)
To: ext Nick Pelly; +Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org
In-Reply-To: <35c90d961002191323x50203ca1x8ed709b3f128e41d@mail.gmail.com>
Nick Pelly wrote:
>>> Attached is a new patch with the consistent bit logic.
>>>
>>> Comments?
>> In order to keep backwards compatibility 1 should mean "don't allow this
>> packet type" for all packets. Other wise old application with new kernel
>> would not allow any packet types.
>
> The current patch achieves this backwards compatibility by assuming
> that all packet types are allowed if the old sockaddr_sco struct is
> used (it checks the size), or if sco_pkt_type is 0.
OK. Then my concerns are gone.
--
Ville
^ permalink raw reply
* Re: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip
From: Marcel Holtmann @ 2010-02-22 2:29 UTC (permalink / raw)
To: Vikram Kandukuri; +Cc: linux-bluetooth, suraj, lrodriguez
In-Reply-To: <20100209114231.GA6587@ATH-LT-538>
Hi Vikram,
> drivers/bluetooth/Kconfig | 10 +
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_ath.h | 72 ++++++
> drivers/bluetooth/hci_ldisc.c | 6 +
> drivers/bluetooth/hci_uart.h | 8 +-
> 6 files changed, 641 insertions(+), 1 deletions(-)
> create mode 100755 drivers/bluetooth/hci_ath.c
> create mode 100755 drivers/bluetooth/hci_ath.h
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 058fbcc..32b98a4 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -58,6 +58,16 @@ config BT_HCIUART_BCSP
>
> Say Y here to compile support for HCI BCSP protocol.
>
> +config BT_HCIUART_ATH
> + bool "Atheros AR3001 Board support"
> + depends on BT_HCIUART
> + help
> + HCIATH (HCI Atheros) is a serial protocol for communication
> + between Bluetooth device and host. This protocol is required for
> + serial Bluetooth devices that are based on Atheros AR3001 chips.
> +
> + Say Y here to compile support for HCIATH protocol.
> +
it would be nice if you add a document explaining this special Atheros
protocol that is used.
> +static void ath_context_switch(struct work_struct *work)
> +{
> + int status;
> + struct ath_struct *ath;
> + struct hci_uart *hu;
> + struct tty_struct *tty;
> + struct sk_buff *skbuf;
> + ath = container_of(work, struct ath_struct, ctxtsw);
> + hu = ath->hu;
> + tty = hu->tty;
> + status = ath_wakeup_ar3001(tty);
> + if ((status & TIOCM_CTS)) {
> + while ((skbuf = skb_dequeue(&ath->tx_wait_q)))
> + skb_queue_tail(&ath->txq, skbuf);
> +
> + /* Ready to send Data */
> + clear_bit(HCI_UART_SENDING, &hu->tx_state);
> + hci_uart_tx_wakeup(hu);
> + }
> +}
> +
> +/* Initialize protocol */
> +static int ath_open(struct hci_uart *hu)
> +{
> + struct ath_struct *ath;
> + BT_DBG("hu %p", hu);
> + ath = kzalloc(sizeof(*ath), GFP_ATOMIC);
> + if (!ath)
> + return -ENOMEM;
> + skb_queue_head_init(&ath->txq);
> + skb_queue_head_init(&ath->tx_wait_q);
> + skb_queue_head_init(&ath->tx_cmd_wait_q);
> + spin_lock_init(&ath->hciath_lock);
> + ath->cur_sleep = 0;
> + ath->num_cmds_complete = 1;
> + hu->priv = ath;
> + ath->hu = hu;
> + init_waitqueue_head(&ath->wqevt);
> + INIT_WORK(&ath->ctxtsw, ath_context_switch);
> + return 0;
> +}
In all the function, use at least some empty lines in between so that
somebody can try to read these function. Cramping everything in less
space doesn't make it simpler.
> +/* Enqueue frame for transmittion (padding, crc, etc) */
> +/* may be called from two simultaneous tasklets */
Why are you stealing the comments from hci_ll.c. Are you using actually
padding and CRC?
> +static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> +{
> + struct ath_struct *ath;
> + ath = hu->priv;
Direct assignment on declaration.
> + if (HCI_SCODATA_PKT == bt_cb(skb)->pkt_type) {
Always the other way around. Variable compares to constant. Not the
constant to variable.
> +static struct sk_buff *ath_dequeue(struct hci_uart *hu)
> +{
> + struct ath_struct *ath;
> + struct sk_buff *skbuff;
> + ath = hu->priv;
> + skbuff = skb_dequeue(&ath->txq);
> + if (NULL != skbuff) {
Never ever use the variable name skbuff and also if (skbuff) here.
However preferable this is if (!skb) return NULL;
> + ath_handle_host_data(ath, bt_cb(skbuff)->pkt_type,
> + &skbuff->data[1], skbuff->len - 1);
> + }
> + return skbuff;
> +}
> +
> +static inline int ath_check_data_len(struct ath_struct *ath, int len)
> +{
> + register int room = skb_tailroom(ath->rx_skb);
> + BT_DBG("len %d room %d", len, room);
> + if (!len) {
> + hci_recv_frame(ath->rx_skb);
> + } else if (len > room) {
> + BT_ERR("Data length is too large");
> + kfree_skb(ath->rx_skb);
> + } else {
> + ath->rx_state = HCIATH_W4_DATA;
> + ath->rx_count = len;
> + return len;
> + }
How is this if statement even suppose to work?
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_skb = NULL;
> + ath->rx_count = 0;
> + return 0;
> +}
> +
> +/* Recv data */
> +static int ath_recv(struct hci_uart *hu, void *data, int count)
> +{
> + struct ath_struct *ath = hu->priv;
> + register char *ptr;
> + struct hci_event_hdr *eh;
> + struct hci_acl_hdr *ah;
> + struct hci_sco_hdr *sh;
> + struct sk_buff *skbuff;
> + register int len, type, dlen;
> + skbuff = NULL;
> + BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count,
> + ath->rx_state, ath->rx_count);
> + ptr = data;
> + while (count) {
> + if (ath->rx_count) {
> + len = min_t(unsigned int, ath->rx_count, count);
> + memcpy(skb_put(ath->rx_skb, len), ptr, len);
> + ath->rx_count -= len;
> + count -= len;
> + ptr += len;
> + if (ath->rx_count)
> + continue;
> + switch (ath->rx_state) {
> + case HCIATH_W4_DATA:
> + ath_handle_data_from_controller(ath,
> + bt_cb
> + (ath->rx_skb)->
> + pkt_type,
> + ath->rx_skb->
> + data,
> + ath->rx_skb->
> + len);
> + if (HCI_EVENT_PKT ==
> + bt_cb(ath->rx_skb)->pkt_type
> + && ath->num_cmds_complete > 0) {
> +
> + skbuff =
> + skb_dequeue(&ath->tx_cmd_wait_q);
> + if (skbuff != NULL) {
> + skb_queue_tail(&ath->txq,
> + skbuff);
> + schedule_work(&ath->ctxtsw);
> + }
> + }
> + if (ath_verify_event_discardable
> + (hu, bt_cb(ath->rx_skb)->pkt_type,
> + ath->rx_skb->data)) {
> + kfree(ath->rx_skb);
> + ath->rx_skb = NULL;
> + } else {
> + hci_recv_frame(ath->rx_skb);
> + }
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_skb = NULL;
> + ath->rx_count = 0;
> + continue;
> + case HCIATH_W4_EVENT_HDR:
> + eh = (struct hci_event_hdr *)ath->rx_skb->data;
> + BT_DBG("Event header: evt 0x%2.2x plen %d",
> + eh->evt, eh->plen);
> + ath_check_data_len(ath, eh->plen);
> + continue;
> + case HCIATH_W4_ACL_HDR:
> + ah = (struct hci_acl_hdr *)ath->rx_skb->data;
> + dlen = __le16_to_cpu(ah->dlen);
> + BT_DBG("ACL header: dlen %d", dlen);
> + ath_check_data_len(ath, dlen);
> + continue;
> + case HCIATH_W4_SCO_HDR:
> + sh = (struct hci_sco_hdr *)ath->rx_skb->data;
> + BT_DBG("SCO header: dlen %d", sh->dlen);
> + ath_check_data_len(ath, sh->dlen);
> + continue;
> + }
> + }
> +
> + /* HCIATH_W4_PACKET_TYPE */
> + switch (*ptr) {
> + case HCI_EVENT_PKT:
> + BT_DBG("Event packet");
> + ath->rx_state = HCIATH_W4_EVENT_HDR;
> + ath->rx_count = HCI_EVENT_HDR_SIZE;
> + type = HCI_EVENT_PKT;
> + break;
> + case HCI_ACLDATA_PKT:
> + BT_DBG("ACL packet");
> + ath->rx_state = HCIATH_W4_ACL_HDR;
> + ath->rx_count = HCI_ACL_HDR_SIZE;
> + type = HCI_ACLDATA_PKT;
> + break;
> + case HCI_SCODATA_PKT:
> + BT_DBG("SCO packet");
> + ath->rx_state = HCIATH_W4_SCO_HDR;
> + ath->rx_count = HCI_SCO_HDR_SIZE;
> + type = HCI_SCODATA_PKT;
> + break;
> + default:
> + BT_ERR("Unknown HCI packet type %2.2x", (__u8) *ptr);
> + hu->hdev->stat.err_rx++;
> + ptr++;
> + count--;
> + continue;
> + };
> + ptr++;
> + count--;
> +
> + /* Allocate packet */
> + ath->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE, GFP_ATOMIC);
> + if (!ath->rx_skb) {
> + BT_ERR("Can't allocate mem for new packet");
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_count = 0;
> + return 0;
> + }
> + ath->rx_skb->dev = (void *)hu->hdev;
> + bt_cb(ath->rx_skb)->pkt_type = type;
> + } return count;
> +}
We have a common exported function called hci_recv_fragment that should
be doing exactly this.
> +static void ath_controller_sleep_mode(struct hci_uart *hu, bool enable)
> +{
> + unsigned char sleepcmd[] = { 0x01, 0x04, 0xFC, 0x01, 0x00 };
> + sleepcmd[4] = enable;
> + ath_write_data_to_cntrlr(hu->hdev, sleepcmd, sizeof(sleepcmd));
> +}
Special vendor commands need a comment. Explain the data structure of
them and what are the options.
> +int ath_wakeup_ar3001(struct tty_struct *tty)
> +{
> + struct termios settings;
> + int status = 0x00;
> + mm_segment_t oldfs;
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + if ((status & TIOCM_CTS))
> + return status;
> +
> + oldfs = get_fs();
> + set_fs(KERNEL_DS);
> + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
> +
> + settings.c_cflag &= ~CRTSCTS;
> + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
> + set_fs(oldfs);
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS);
> + mdelay(20);
> +
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00);
> + mdelay(20);
> +
> + status = tty->driver->ops->tiocmget(tty, NULL);
> + oldfs = get_fs();
> + set_fs(KERNEL_DS);
> + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
> +
> + settings.c_cflag |= CRTSCTS;
> + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
> + set_fs(oldfs);
> + return status;
> +}
Why is this magic in kernel space and not inside hciattach?
> +int ath_fullboot_config(struct hci_uart *hu, int current_event)
> +{
> + struct sk_buff *skbuf;
> + struct ath_struct *ath = hu->priv;
> + static int autosleep;
> + unsigned char rstevt[] = { 0x1, 0x3, 0xc, 0x0 };
> + if (current_event == HCI_RESET) {
> +
> + if (ath->cur_sleep) {
> + autosleep = 1;
> + ath_controller_sleep_mode(hu, 1);
> + return 1;
> + } else {
> + return 0;
> + }
> + }
> +
> + if (current_event == HCI_SET_SLEEP_MODE) {
> +
> + if (autosleep == 0)
> + return 1;
> +
> + while ((skbuf = skb_dequeue(&ath->tx_wait_q)))
> + skb_queue_tail(&ath->txq, skbuf);
> +
> + ath_write_data_to_host(hu->hdev, rstevt, sizeof(rstevt));
> + autosleep = 0;
> + return 1;
> + }
> + return 0;
> +}
Why is this function not static? And all the others?
> +int ath_write_data_to_host(void *dev, unsigned char *data, u8 length)
> +{
> + struct sk_buff *skbuf;
> + struct hci_dev *hdev;
> + hdev = (struct hci_dev *)dev;
> + skbuf = bt_skb_alloc(length, GFP_ATOMIC);
> + if (!skbuf) {
> + BT_ERR("Memory allocation failed");
> + return -1;
> + }
> + skb_orphan(skbuf);
> +
> + /* First byte will be added as packet type */
> + bt_cb(skbuf)->pkt_type = data[0];
> + skbuf->dev = (void *)hdev;
> + memcpy(skb_put(skbuf, length - 1), &data[1], length - 1);
> + hci_recv_frame(skbuf);
> + return length;
> +}
What is this exactly for? Do you have a secondary receive path?
> +int ath_write_data_to_cntrlr(void *dev, unsigned char *Data, u8 len)
> +{
> + struct sk_buff *skbuff;
> + struct ath_struct *ath;
> + struct hci_uart *hu;
> + struct hci_dev *hdev;
> + if (NULL == dev) {
> + BT_DBG("NULL handle received %p \n", dev);
> + return 0;
> + }
> + hdev = (struct hci_dev *)dev;
> + hu = (struct hci_uart *)hdev->driver_data;
> + if (hu == NULL) {
> + BT_DBG("NULL handle received %p \n", hdev);
> + return 0;
> + }
> + ath = hu->priv;
> + if (ath == NULL) {
> + BT_DBG("NULL handle received \n");
> + return 0;
> + }
> + skbuff = bt_skb_alloc(len, GFP_ATOMIC);
> + if (skbuff == NULL) {
> + BT_DBG("Malloc Fail memory %p \n", skbuff);
> + return 0;
> + }
> + skb_orphan(skbuff);
> +
> + if (len != 0)
> + memcpy(skb_put(skbuff, len), Data, len);
> +
> + bt_cb(skbuff)->pkt_type = HCI_COMMAND_PKT;
> + skbuff->dev = dev;
> + if (ath->num_cmds_complete > 0) {
> + skb_queue_tail(&ath->txq, skbuff);
> + schedule_work(&ath->ctxtsw);
> + } else {
> + skb_queue_tail(&ath->tx_cmd_wait_q, skbuff);
> + }
> + return len;
> +}
Messing with num_cmds_complete inside the driver is wrong. What are you
doing here?
> +int ath_check_sleep_cmd(struct ath_struct *ath, unsigned char *packet)
> +{
> + if (packet[0] == HCI_EVENT_PKT && packet[1] == 0xFC)
> + ath->cur_sleep = packet[3];
How is this working. Every vendor event is sleep event?
> +int ath_verify_event_discardable(struct hci_uart *hu, unsigned char pkt_type,
> + unsigned char *packet)
> +{
> + if (pkt_type != HCI_EVENT_PKT)
> + return 0;
> +
> + switch (packet[0]) {
> + case 0x0E: /* Command Complete Event */
> + if (packet[3] == 0x03 && packet[4] == 0x0C) {
> +
> + /* Command complete for HCI Reset Received */
> + return ath_fullboot_config(hu, HCI_RESET);
> + } else if (packet[3] == 0x04 && packet[4] == 0xFC) {
> + return ath_fullboot_config(hu, HCI_SET_SLEEP_MODE);
> + }
> + break;
> + }
> + return 0;
> +}
What are you doing here?
> +void ath_handle_hci_event(struct ath_struct *ath, u8 * packet)
> +{
> + switch (packet[0]) {
> + case 0x05: /* ACL Disconnection Complete Event */
> + break;
> + case 0x03: /* ACL Connection Complete Event */
> + break;
> + case 0x0E: /* Command Complete Event */
> + spin_lock(&ath->hciath_lock);
> + ath->num_cmds_complete = packet[2];
> + spin_unlock(&ath->hciath_lock);
> + break;
> + case 0x0F: /* Command Status Event */
> + spin_lock(&ath->hciath_lock);
> + ath->num_cmds_complete = packet[3];
> + spin_unlock(&ath->hciath_lock);
> + break;
> + }
> +}
Something is really wrong in this driver? Are you really know how this
is suppose to work if you have to interfere this much?
> +void ath_handle_host_data(struct ath_struct *ath, u8 pktType, u8 * packet,
> + unsigned int len)
> +{
> + switch (pktType) {
> + case HCI_ACLDATA_PKT: /* ACL packets */
> + break;
> + case HCI_COMMAND_PKT: /* HCI Commands */
> + ath_handle_hci_cmd(ath, packet);
> + ath_check_sleep_cmd(ath, packet);
> + break;
> + }
> +}
> +
> +void ath_handle_data_from_controller(struct ath_struct *ath, u8 pktType,
> + u8 *packet, unsigned int len)
> +{
> + switch (pktType) {
> + case HCI_ACLDATA_PKT: /* ACL packets */
> + break;
> + case HCI_EVENT_PKT: /* HCI Events */
> + ath_handle_hci_event(ath, packet);
> + break;
> + }
> +}
Seriously? are you implemented your own Bluetooth stack? Please explain
what you really need. And then the host stack can provide it for you.
This is not acceptable.
Regards
Marcel
^ permalink raw reply
* Re: Kernel panic in rfcomm_run - unbalanced refcount on rfcomm_session
From: Nick Pelly @ 2010-02-21 21:00 UTC (permalink / raw)
To: Dave Young; +Cc: Bluettooth Linux
In-Reply-To: <a8e1da1002200017t6e856c7cu1300ac5b3b3a43b@mail.gmail.com>
On Sat, Feb 20, 2010 at 12:17 AM, Dave Young <hidave.darkstar@gmail.com> wrote:
> On Thu, Feb 18, 2010 at 1:04 PM, Nick Pelly <npelly@google.com> wrote:
>> Since 2.6.32 we are seeing kernel panics like:
>>
>> [10651.110229] Unable to handle kernel paging request at virtual
>> address 6b6b6b6b
>> [10651.111968] Internal error: Oops: 5 [#1] PREEMPT
>> [10651.113952] CPU: 0 Tainted: G W (2.6.32-59979-gd0c97db #1)
>> [10651.114624] PC is at rfcomm_run+0xa04/0xdbc
>> <...>
>> [10651.406188] [<c031ad24>] (rfcomm_run+0xa04/0xdbc) from [<c006ce30>]
>> (kthread+0x78/0x80)
>> [10651.406585] [<c006ce30>] (kthread+0x78/0x80) from [<c002793c>]
>> (kernel_thread_exit+0x0/0x8)
>>
>> (rfcomm_run() is all inlined so theres not much of a stack trace))
>
> Could you make rfcomm_process_sessions to be not inlined, and get new
> kernel logs?
I'm not using a stock kernel, so i'm not sure how the kernel trace
will help, but the un-inlined stack that I decoded against my vmlinux
is:
>> This is a use-after-free on struct rfcomm_session s in the call chain
>> rfcomm_run() -> rfcomm_process_sessions() -> rfcomm_process_dlcs() ->
>> list_for_each_safe(p, n, &s->dlcs)
PS - 9e726b17422b is definitely not the root cause, we've now seen the
same crash with this patch reverted (but it is much harder to
reproduce with 9e726b17422b reverted).
Nick
^ permalink raw reply
* Re: [PATCH] Fix signal watch when a service name is given
From: Luiz Augusto von Dentz @ 2010-02-21 20:41 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Vinicius Gomes, linux-bluetooth
In-Reply-To: <2d5a2c101002170808r2c583b4fuf24b38a71d4c06d3@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 679 bytes --]
Hi,
On Wed, Feb 17, 2010 at 6:08 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Marcel,
>
> On Wed, Feb 17, 2010 at 12:00 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Luiz,
>>
>>> @Marcel: So before you apply this you should really apply Vinicius
>>> patch to the rest of projects: bluez, ofono, connman...
>>
>> has been applied to all projects now. Please resend your other patch
>> based on the latest tree.
>
> Resending the patch and also add another one to make sure we cancel
> any pending operation during watch removal.
Resending with the latests fixes to make hfp ofono plugin to work properly.
--
Luiz Augusto von Dentz
Computer Engineer
[-- Attachment #2: 0001-Do-not-automatically-remove-watches-for-service-name.patch --]
[-- Type: text/x-patch, Size: 1207 bytes --]
From 84a594eee3b8c3b793526e65936022e0655b99a1 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Sat, 20 Feb 2010 12:16:32 +0200
Subject: [PATCH 1/3] Do not automatically remove watches for service names
Services can be owned again so it is perfectly fine to keep the watch.
---
gdbus/watch.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/gdbus/watch.c b/gdbus/watch.c
index 1d479fa..e85f288 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -386,18 +386,19 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
cb->conn_func(connection, cb->user_data);
}
+ /* Only auto remove if it is a bus name watch */
+ if (data->argument[0] == ':' &&
+ (!cb->conn_func || !cb->disc_func)) {
+ filter_data_remove_callback(data, cb);
+ continue;
+ }
+
/* Check if the watch was removed/freed by the callback
* function */
if (!g_slist_find(data->callbacks, cb))
continue;
data->callbacks = g_slist_remove(data->callbacks, cb);
-
- if (!cb->conn_func || !cb->disc_func) {
- g_free(cb);
- continue;
- }
-
data->processed = g_slist_append(data->processed, cb);
}
--
1.6.3.3
[-- Attachment #3: 0002-Fix-signal-watch-when-a-service-name-is-given.patch --]
[-- Type: text/x-patch, Size: 9393 bytes --]
From f3c194ec729d4fa754cb8a0aa87d0e098d3ff420 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Wed, 17 Feb 2010 17:11:02 +0200
Subject: [PATCH 2/3] Fix signal watch when a service name is given
The bus name should be resolved when adding a watch by service name since
messages do always come with sender set to owner's bus name, also it
should listen to owner updates since it can change without invalidating
the watch.
---
gdbus/watch.c | 161 +++++++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 128 insertions(+), 33 deletions(-)
diff --git a/gdbus/watch.c b/gdbus/watch.c
index e85f288..a1acb72 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -55,19 +55,22 @@ struct filter_callback {
struct filter_data {
DBusConnection *connection;
DBusHandleMessageFunction handle_func;
- char *sender;
+ char *name;
+ char *owner;
char *path;
char *interface;
char *member;
char *argument;
GSList *callbacks;
GSList *processed;
+ guint name_watch;
gboolean lock;
gboolean registered;
};
static struct filter_data *filter_data_find(DBusConnection *connection,
- const char *sender,
+ const char *name,
+ const char *owner,
const char *path,
const char *interface,
const char *member,
@@ -82,8 +85,12 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
if (connection != data->connection)
continue;
- if (sender && data->sender &&
- g_str_equal(sender, data->sender) == FALSE)
+ if (name && data->name &&
+ g_str_equal(name, data->name) == FALSE)
+ continue;
+
+ if (owner && data->owner &&
+ g_str_equal(owner, data->owner) == FALSE)
continue;
if (path && data->path &&
@@ -110,13 +117,15 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
static void format_rule(struct filter_data *data, char *rule, size_t size)
{
+ const char *sender;
int offset;
offset = snprintf(rule, size, "type='signal'");
+ sender = data->name ? : data->owner;
- if (data->sender)
+ if (sender)
offset += snprintf(rule + offset, size - offset,
- ",sender='%s'", data->sender);
+ ",sender='%s'", sender);
if (data->path)
offset += snprintf(rule + offset, size - offset,
",path='%s'", data->path);
@@ -183,8 +192,10 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
const char *argument)
{
struct filter_data *data;
+ const char *name = NULL, *owner = NULL;
- if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL)) {
+ if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+ NULL)) {
if (!dbus_connection_add_filter(connection,
message_filter, NULL, NULL)) {
error("dbus_connection_add_filter() failed");
@@ -192,15 +203,25 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
}
}
- data = filter_data_find(connection, sender, path, interface, member,
- argument);
+ if (sender == NULL)
+ goto proceed;
+
+ if (sender[0] == ':')
+ owner = sender;
+ else
+ name = sender;
+
+proceed:
+ data = filter_data_find(connection, name, owner, path, interface,
+ member, argument);
if (data)
return data;
data = g_new0(struct filter_data, 1);
data->connection = dbus_connection_ref(connection);
- data->sender = g_strdup(sender);
+ data->name = name ? g_strdup(name) : NULL;
+ data->owner = owner ? g_strdup(owner) : NULL;
data->path = g_strdup(path);
data->interface = g_strdup(interface);
data->member = g_strdup(member);
@@ -244,7 +265,9 @@ static void filter_data_free(struct filter_data *data)
g_free(l->data);
g_slist_free(data->callbacks);
- g_free(data->sender);
+ g_dbus_remove_watch(data->connection, data->name_watch);
+ g_free(data->name);
+ g_free(data->owner);
g_free(data->path);
g_free(data->interface);
g_free(data->member);
@@ -322,7 +345,8 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
filter_data_free(data);
/* Remove filter if there are no listeners left for the connection */
- data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+ data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+ NULL);
if (!data)
dbus_connection_remove_filter(connection, message_filter,
NULL);
@@ -359,6 +383,37 @@ static DBusHandlerResult signal_filter(DBusConnection *connection,
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
+static void update_name_cache(const char *name, const char *owner)
+{
+ GSList *l;
+
+ for (l = listeners; l != NULL; l = l->next) {
+ struct filter_data *data = l->data;
+
+ if (g_strcmp0(data->name, name) != 0)
+ continue;
+
+ g_free(data->owner);
+ data->owner = g_strdup(owner);
+ }
+}
+
+static const char *check_name_cache(const char *name)
+{
+ GSList *l;
+
+ for (l = listeners; l != NULL; l = l->next) {
+ struct filter_data *data = l->data;
+
+ if (g_strcmp0(data->name, name) != 0)
+ continue;
+
+ return data->owner;
+ }
+
+ return NULL;
+}
+
static DBusHandlerResult service_filter(DBusConnection *connection,
DBusMessage *message, void *user_data)
{
@@ -375,6 +430,8 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
+ update_name_cache(name, new);
+
while (data->callbacks) {
cb = data->callbacks->data;
@@ -422,7 +479,9 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
member = dbus_message_get_member(message);
dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &arg, DBUS_TYPE_INVALID);
- data = filter_data_find(connection, sender, path, iface, member, arg);
+ /* Sender is always bus name */
+ data = filter_data_find(connection, NULL, sender, path, iface, member,
+ arg);
if (!data) {
error("Got %s.%s signal which has no listeners", iface, member);
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
@@ -447,7 +506,8 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
filter_data_free(data);
/* Remove filter if there no listener left for the connection */
- data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+ data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+ NULL);
if (!data)
dbus_connection_remove_filter(connection, message_filter,
NULL);
@@ -457,38 +517,60 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
struct service_data {
DBusConnection *conn;
+ char *name;
+ const char *owner;
GDBusWatchFunction conn_func;
void *user_data;
};
+static void service_data_free(struct service_data *data)
+{
+ dbus_connection_unref(data->conn);
+ g_free(data->name);
+ g_free(data);
+}
+
+static gboolean update_service(void *user_data)
+{
+ struct service_data *data = user_data;
+
+ update_name_cache(data->name, data->owner);
+ if (data->conn_func)
+ data->conn_func(data->conn, data->user_data);
+
+ service_data_free(data);
+
+ return FALSE;
+}
+
static void service_reply(DBusPendingCall *call, void *user_data)
{
struct service_data *data = user_data;
DBusMessage *reply;
- DBusError error;
- dbus_bool_t has_owner;
+ DBusError err;
reply = dbus_pending_call_steal_reply(call);
if (reply == NULL)
return;
- dbus_error_init(&error);
+ dbus_error_init(&err);
- if (dbus_message_get_args(reply, &error,
- DBUS_TYPE_BOOLEAN, &has_owner,
- DBUS_TYPE_INVALID) == FALSE) {
- if (dbus_error_is_set(&error) == TRUE) {
- error("%s", error.message);
- dbus_error_free(&error);
- } else {
- error("Wrong arguments for NameHasOwner reply");
- }
- goto done;
- }
+ if (dbus_set_error_from_message(&err, reply))
+ goto fail;
- if (has_owner && data->conn_func)
- data->conn_func(data->conn, data->user_data);
+ if (dbus_message_get_args(reply, &err,
+ DBUS_TYPE_STRING, &data->owner,
+ DBUS_TYPE_INVALID) == FALSE)
+ goto fail;
+ update_service(data);
+
+ goto done;
+
+fail:
+ error("%s", err.message);
+ dbus_error_free(&err);
+ service_data_free(data);
done:
dbus_message_unref(reply);
}
@@ -506,12 +588,19 @@ static void check_service(DBusConnection *connection, const char *name,
return;
}
- data->conn = connection;
+ data->conn = dbus_connection_ref(connection);
+ data->name = g_strdup(name);
data->conn_func = connect;
data->user_data = user_data;
+ data->owner = check_name_cache(name);
+ if (data->owner != NULL) {
+ g_idle_add(update_service, data);
+ return;
+ }
+
message = dbus_message_new_method_call(DBUS_SERVICE_DBUS,
- DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "NameHasOwner");
+ DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "GetNameOwner");
if (message == NULL) {
error("Can't allocate new message");
g_free(data);
@@ -597,6 +686,11 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
if (!cb)
return 0;
+ if (data->name != NULL && data->name_watch == 0)
+ data->name_watch = g_dbus_add_service_watch(connection,
+ data->name, NULL,
+ NULL, NULL, NULL);
+
return cb->id;
}
@@ -626,7 +720,8 @@ void g_dbus_remove_all_watches(DBusConnection *connection)
{
struct filter_data *data;
- while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL))) {
+ while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL,
+ NULL, NULL))) {
listeners = g_slist_remove(listeners, data);
filter_data_call_and_free(data);
}
--
1.6.3.3
[-- Attachment #4: 0003-Fix-calling-watch-callbacks-after-it-has-been-remove.patch --]
[-- Type: text/x-patch, Size: 5020 bytes --]
From 35d3c2168ac95967cd4e6b23c7e66e770e540395 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Wed, 17 Feb 2010 17:12:19 +0200
Subject: [PATCH 3/3] Fix calling watch callbacks after it has been removed
Pending call should be removed if the watch is removed since the
application no longer expect that to be reached and may already freed the
data associated with it.
---
gdbus/watch.c | 79 +++++++++++++++++++++++++++++++++++---------------------
1 files changed, 49 insertions(+), 30 deletions(-)
diff --git a/gdbus/watch.c b/gdbus/watch.c
index a1acb72..c0dcc93 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -43,11 +43,21 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
static guint listener_id = 0;
static GSList *listeners = NULL;
+struct service_data {
+ DBusConnection *conn;
+ DBusPendingCall *call;
+ char *name;
+ const char *owner;
+ guint id;
+ struct filter_callback *callback;
+};
+
struct filter_callback {
GDBusWatchFunction conn_func;
GDBusWatchFunction disc_func;
GDBusSignalFunction signal_func;
GDBusDestroyFunction destroy_func;
+ struct service_data *data;
void *user_data;
guint id;
};
@@ -302,7 +312,7 @@ static struct filter_callback *filter_data_add_callback(
{
struct filter_callback *cb = NULL;
- cb = g_new(struct filter_callback, 1);
+ cb = g_new0(struct filter_callback, 1);
cb->conn_func = connect;
cb->disc_func = disconnect;
@@ -319,6 +329,24 @@ static struct filter_callback *filter_data_add_callback(
return cb;
}
+static void service_data_free(struct service_data *data)
+{
+ struct filter_callback *callback = data->callback;
+
+ dbus_connection_unref(data->conn);
+
+ if (data->call)
+ dbus_pending_call_unref(data->call);
+
+ if (data->id)
+ g_source_remove(data->id);
+
+ g_free(data->name);
+ g_free(data);
+
+ callback->data = NULL;
+}
+
static gboolean filter_data_remove_callback(struct filter_data *data,
struct filter_callback *cb)
{
@@ -327,6 +355,13 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
data->callbacks = g_slist_remove(data->callbacks, cb);
data->processed = g_slist_remove(data->processed, cb);
+ /* Cancel pending operations */
+ if (cb->data) {
+ if (cb->data->call)
+ dbus_pending_call_cancel(cb->data->call);
+ service_data_free(cb->data);
+ }
+
if (cb->destroy_func)
cb->destroy_func(cb->user_data);
@@ -515,28 +550,14 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
-struct service_data {
- DBusConnection *conn;
- char *name;
- const char *owner;
- GDBusWatchFunction conn_func;
- void *user_data;
-};
-
-static void service_data_free(struct service_data *data)
-{
- dbus_connection_unref(data->conn);
- g_free(data->name);
- g_free(data);
-}
-
static gboolean update_service(void *user_data)
{
struct service_data *data = user_data;
+ struct filter_callback *cb = data->callback;
update_name_cache(data->name, data->owner);
- if (data->conn_func)
- data->conn_func(data->conn, data->user_data);
+ if (cb->conn_func)
+ cb->conn_func(data->conn, cb->user_data);
service_data_free(data);
@@ -575,11 +596,11 @@ done:
dbus_message_unref(reply);
}
-static void check_service(DBusConnection *connection, const char *name,
- GDBusWatchFunction connect, void *user_data)
+static void check_service(DBusConnection *connection,
+ const char *name,
+ struct filter_callback *callback)
{
DBusMessage *message;
- DBusPendingCall *call;
struct service_data *data;
data = g_try_malloc0(sizeof(*data));
@@ -590,12 +611,12 @@ static void check_service(DBusConnection *connection, const char *name,
data->conn = dbus_connection_ref(connection);
data->name = g_strdup(name);
- data->conn_func = connect;
- data->user_data = user_data;
+ data->callback = callback;
+ callback->data = data;
data->owner = check_name_cache(name);
if (data->owner != NULL) {
- g_idle_add(update_service, data);
+ data->id = g_idle_add(update_service, data);
return;
}
@@ -611,21 +632,19 @@ static void check_service(DBusConnection *connection, const char *name,
DBUS_TYPE_INVALID);
if (dbus_connection_send_with_reply(connection, message,
- &call, -1) == FALSE) {
+ &data->call, -1) == FALSE) {
error("Failed to execute method call");
g_free(data);
goto done;
}
- if (call == NULL) {
+ if (data->call == NULL) {
error("D-Bus connection not available");
g_free(data);
goto done;
}
- dbus_pending_call_set_notify(call, service_reply, data, NULL);
-
- dbus_pending_call_unref(call);
+ dbus_pending_call_set_notify(data->call, service_reply, data, NULL);
done:
dbus_message_unref(message);
@@ -654,7 +673,7 @@ guint g_dbus_add_service_watch(DBusConnection *connection, const char *name,
return 0;
if (connect)
- check_service(connection, name, connect, user_data);
+ check_service(connection, name, cb);
return cb->id;
}
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] Fix double free on AVDTP Abort response
From: Luiz Augusto von Dentz @ 2010-02-21 17:53 UTC (permalink / raw)
To: Daniel Örstadius, linux-bluetooth
In-Reply-To: <20100221134631.GA25639@jh-x301>
Hi Johan,
On Sun, Feb 21, 2010 at 3:46 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Daniel,
>
> On Sun, Feb 21, 2010, Daniel Örstadius wrote:
>> > Please try to at least do a compile check before you submit patches.
>> > This one gives the following error:
>> > audio/avdtp.c: In function ‘handle_unanswered_req’:
>> > audio/avdtp.c:908: error: comparison between pointer and integer
>> >
>> > What you probably want is session->req->signal_id == AVDTP_ABORT.
>> >
>>
>> Sorry. The attached patch compiles without warnings on "./configure && make".
>> (new dependency to a capabilities lib prevents ./bootstrap-configure atm)
>
> You can avoid that with "./bootstrap-configure --disable-capng"
>
>> The pending request might be freed twice when receiving an Abort
>> response, in handle_unanswered_req and session_cb. Avoid freeing
>> it in handle_unanswered_req.
>> ---
>> audio/avdtp.c | 7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> Thanks, the patch is now upstream.
While discussing this problem we find out that our timeout doesn't
really work as we imagine due to the possibility of abort being
rejected, so either we don't call any callback on timeout and do it on
abort response while changing to aborting state (which probably means
4-8 sec before we are really able to abort/cancel) or we disconnect as
the other end seems to not cooperate on aborting.
Btw, we probably should have a smaller timeout for avdtp_abort anyway
and make sure we call it upon RequestDisconnect not avdtp_close which
has no priority over the others commands.
Regards,
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* Re: obex-client: how to select a dongle from muliple dongles to send-files
From: Luiz Augusto von Dentz @ 2010-02-21 16:52 UTC (permalink / raw)
To: Ed Tsang; +Cc: Bluettooth Linux
In-Reply-To: <588634.45024.qm@web52606.mail.re2.yahoo.com>
Hi,
On Fri, Feb 19, 2010 at 9:47 PM, Ed Tsang <netdesign_98@yahoo.com> wrote:
> Hi,
> Need some hint from the expert.
> I am using test/send-files to send files. But when I have multiple dongles, I would like in run time to select which dongle to send out the file either using hci0/1 or the actual dongle address or other means. I tried trace it down but got lost. Anyone could provide some hint.
Currently send-files doesn't support choosing the source, but the dbus
API does, just set the bdaddr (string format) as Source in device
dictionary.
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* [PATCHes] osso-gwobex patches for review
From: Bastien Nocera @ 2010-02-21 14:27 UTC (permalink / raw)
To: BlueZ development
[-- Attachment #1: Type: text/plain, Size: 245 bytes --]
Heya,
The first patch fixes a few typos.
The second one requires the openobex patches I posted to the list, but
should fail gracefully if the support does not exist.
The dependency on openobex itself should probably be upped as well.
Cheers
[-- Attachment #2: 0001-Fix-a-few-typos.patch --]
[-- Type: text/x-patch, Size: 1197 bytes --]
>From f15d528af880b639567e8c2924be50cc6a6d0cd3 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Fri, 19 Feb 2010 15:49:27 +0000
Subject: [PATCH 1/2] Fix a few typos
---
src/obex-priv.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/obex-priv.c b/src/obex-priv.c
index f99740a..a43c56a 100644
--- a/src/obex-priv.c
+++ b/src/obex-priv.c
@@ -221,7 +221,7 @@ static void obex_connect_done(GwObex *ctx, obex_object_t *object, int obex_rsp)
#endif
case OBEX_HDR_CONNECTION:
ctx->conid = hv.bq4;
- debug("got Conection ID: %#x\n", hv.bq4);
+ debug("got Connection ID: %#x\n", hv.bq4);
break;
default:
debug("Skipped header %02x\n", hi);
@@ -372,7 +372,7 @@ static void obex_readstream(GwObex *ctx, obex_object_t *object) {
if (!xfer) {
debug("Incoming data even though no xfer active!\n");
- /* Flush incomming stream */
+ /* Flush incoming stream */
actual = OBEX_ObjectReadStream(ctx->handle, object, &buf);
if (actual > 0)
debug("Ignored %d bytes\n", actual);
--
1.6.6.1
[-- Attachment #3: 0002-Add-USB-support-to-osso-gwobex.patch --]
[-- Type: text/x-patch, Size: 7141 bytes --]
>From 841d913a1b31ada32cfeb7a2f36fd0ccc820c088 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Sun, 21 Feb 2010 14:18:37 +0000
Subject: [PATCH 2/2] Add USB support to osso-gwobex
Pass a "usb:busid,deviceid,interface" string to
gw_obex_setup_dev() to setup a USB Obex device.
Then you can access the FTP service as normal.
---
src/gw-obex.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/obex-priv.c | 20 +++++++++
src/obex-priv.h | 4 ++
src/obex-xfer.c | 2 +-
4 files changed, 142 insertions(+), 1 deletions(-)
diff --git a/src/gw-obex.c b/src/gw-obex.c
index 8352782..a51222a 100644
--- a/src/gw-obex.c
+++ b/src/gw-obex.c
@@ -370,11 +370,128 @@ GwObex *gw_obex_setup_fd(int fd, const gchar *uuid, gint uuid_len,
return ctx;
}
+static GwObex *gw_obex_setup_usb(const char *dev, const gchar *uuid, gint uuid_len,
+ GMainContext *context, gint *error) {
+ GwObex *ctx;
+ obex_t *handle;
+ obex_interface_t *iface;
+ int i, num;
+
+ /* Setup low-level USB */
+ if (!gw_obex_transport_setup_usb(&handle)) {
+ debug("gw_obex_transport_setup_usb() failed, no USB support?\n");
+ return NULL;
+ }
+
+ /* Look for the requested interface */
+ num = OBEX_EnumerateInterfaces(handle);
+ if (num == 0) {
+ debug("usbobex_find_interfaces() found no USB devices\n");
+ if (error)
+ *error = GW_OBEX_ERROR_NO_SERVICE;
+ return NULL;
+ }
+
+ iface = NULL;
+ for (i = 0; i < num; i++) {
+ gchar *path;
+ obex_interface_t *tmp;
+ tmp = OBEX_GetInterfaceByIndex(handle, i);
+
+ path = g_strdup_printf("usb:%d,%d,%d", tmp->usb.bus_number,
+ tmp->usb.device_address,
+ tmp->usb.interface_number);
+ debug("Checking requested '%s' against found '%s'\n", dev, path);
+ /* Is that the interface we're looking for? */
+ if (g_str_equal(path, dev)) {
+ iface = tmp;
+ g_free(path);
+ break;
+ }
+ g_free(path);
+ }
+
+ if (iface == NULL) {
+ debug("Could not find matching USB interface for %s\n", dev);
+ if (error)
+ *error = GW_OBEX_ERROR_NO_SERVICE;
+ return NULL;
+ }
+
+ /* Prepare the context */
+ ctx = make_context(handle);
+
+ if (!g_thread_supported())
+ g_thread_init(NULL);
+ ctx->mutex = g_mutex_new();
+
+ OBEX_SetCustomData(handle, ctx);
+
+ /* Do the transport connection */
+ if (OBEX_InterfaceConnect(handle, iface) < 0) {
+ debug("Could not connect to USB device '%s'\n", dev);
+ if (error)
+ *error = GW_OBEX_ERROR_NO_SERVICE;
+ return NULL;
+ }
+
+ debug("Transport connection opened.\n");
+
+ /* Set the file descriptor for listening in */
+ update_context(ctx);
+ if (ctx->conn_fd < 0) {
+ debug("Unable to get a file descriptor for the USB connection\n");
+ g_mutex_free(ctx->mutex);
+ ctx->mutex = NULL;
+
+ g_free(ctx);
+ OBEX_Cleanup(handle);
+ if (error)
+ *error = GW_OBEX_ERROR_NO_SERVICE;
+ return NULL;
+ }
+
+ debug("New FD is %d\n", ctx->conn_fd);
+
+ /* Do the service connection */
+ debug("Connecting to OBEX service\n");
+ if (!gw_obex_connect(ctx, uuid, uuid_len)) {
+ debug("Unable to connect to OBEX service\n");
+ g_mutex_free(ctx->mutex);
+ ctx->mutex = NULL;
+
+ g_free(ctx);
+ OBEX_Cleanup(handle);
+ if (error)
+ *error = GW_OBEX_ERROR_NO_SERVICE;
+ return NULL;
+ }
+
+ debug("Connected (Connection ID: %#x)\n", ctx->conid);
+
+ /* Setup the file descriptor to get events from */
+ ctx->gio = g_io_channel_unix_new(ctx->conn_fd);
+ ctx->gio_source = g_io_create_watch (ctx->gio,
+ G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL);
+ g_source_set_callback(ctx->gio_source, (GSourceFunc)gw_obex_cb, ctx, NULL);
+ (void) g_source_attach(ctx->gio_source, context);
+ g_source_unref(ctx->gio_source);
+
+ ctx->main_ctx = context;
+
+ return ctx;
+}
+
GwObex *gw_obex_setup_dev(const char *dev, const gchar *uuid, gint uuid_len,
GMainContext *context, gint *error) {
GwObex *ctx;
int fd;
+ if (strncmp(dev, "usb:", 4) == 0) {
+ debug("Trying to set up USB device '%s'\n", dev);
+ return gw_obex_setup_usb(dev, uuid, uuid_len, context, error);
+ }
+
fd = open(dev, O_RDWR | O_NOCTTY | O_SYNC);
if (fd < 0) {
debug("open(\"%s\"): %s\n", dev, strerror(errno));
diff --git a/src/obex-priv.c b/src/obex-priv.c
index a43c56a..278a0a7 100644
--- a/src/obex-priv.c
+++ b/src/obex-priv.c
@@ -515,6 +515,7 @@ static void obex_writestream(GwObex *ctx, obex_object_t *object) {
static void obex_event_handler(obex_t *handle, obex_object_t *object, int mode,
int event, int obex_cmd, int obex_rsp) {
GwObex *ctx = OBEX_GetCustomData(handle);
+
switch (event) {
case OBEX_EV_ABORT:
debug("OBEX_EV_ABORT\n");
@@ -630,6 +631,16 @@ gboolean gw_obex_transport_setup(int fd, obex_t **handle) {
return TRUE;
}
+gboolean gw_obex_transport_setup_usb(obex_t **handle) {
+ *handle = OBEX_Init(OBEX_TRANS_USB, obex_event_handler, 0);
+ if (*handle == NULL) {
+ debug("OBEX_Init() failed\n");
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
void gw_obex_get_error(GwObex *ctx, gint *error) {
if (error)
*error = ctx->error;
@@ -728,6 +739,15 @@ GwObex *make_context(obex_t *handle) {
return context;
}
+void update_context(GwObex *ctx)
+{
+ int fd;
+
+ fd = OBEX_GetFD(ctx->handle);
+ if (fd > 0)
+ ctx->conn_fd = fd;
+}
+
gboolean gw_obex_action_op(GwObex *ctx, const gchar *src, const gchar *dst,
uint8_t action) {
gboolean ret = FALSE;
diff --git a/src/obex-priv.h b/src/obex-priv.h
index ce7748c..4991d49 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -162,6 +162,8 @@ struct gw_obex {
GwObex *make_context(obex_t *handle);
+void update_context(GwObex *ctx);
+
gboolean gw_obex_set_error(GwObex *ctx);
void gw_obex_get_error(GwObex *ctx, gint *error);
@@ -176,6 +178,8 @@ gboolean gw_obex_disconnect(GwObex *ctx);
gboolean gw_obex_transport_setup(int fd, obex_t **handle);
+gboolean gw_obex_transport_setup_usb(obex_t **handle);
+
gboolean gw_obex_action_op(GwObex *ctx, const gchar *src, const gchar *dst,
uint8_t action);
diff --git a/src/obex-xfer.c b/src/obex-xfer.c
index 81ac5dc..8c7b645 100644
--- a/src/obex-xfer.c
+++ b/src/obex-xfer.c
@@ -48,7 +48,7 @@ static gboolean handle_input(GwObex *ctx, gint *err) {
r = OBEX_HandleInput(ctx->handle, 22);
if (r < 0) {
- debug("OBEX_HandleInput() failed\n");
+ debug("OBEX_HandleInput() failed (%d)\n", r);
obex_link_error(ctx);
if (err)
*err = GW_OBEX_ERROR_INTERNAL;
--
1.6.6.1
^ permalink raw reply related
* Re: [PATCH] Fix double free on AVDTP Abort response
From: Johan Hedberg @ 2010-02-21 13:46 UTC (permalink / raw)
To: Daniel Örstadius; +Cc: linux-bluetooth
In-Reply-To: <eb7933e21002210454r4b3805e1pec3f4f58440191bf@mail.gmail.com>
Hi Daniel,
On Sun, Feb 21, 2010, Daniel Örstadius wrote:
> > Please try to at least do a compile check before you submit patches.
> > This one gives the following error:
> > audio/avdtp.c: In function ‘handle_unanswered_req’:
> > audio/avdtp.c:908: error: comparison between pointer and integer
> >
> > What you probably want is session->req->signal_id == AVDTP_ABORT.
> >
>
> Sorry. The attached patch compiles without warnings on "./configure && make".
> (new dependency to a capabilities lib prevents ./bootstrap-configure atm)
You can avoid that with "./bootstrap-configure --disable-capng"
> The pending request might be freed twice when receiving an Abort
> response, in handle_unanswered_req and session_cb. Avoid freeing
> it in handle_unanswered_req.
> ---
> audio/avdtp.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
Thanks, the patch is now upstream.
Johan
^ permalink raw reply
* Re: [PATCH] Fix double free on AVDTP Abort response
From: Marcel Holtmann @ 2010-02-21 13:04 UTC (permalink / raw)
To: Daniel Örstadius; +Cc: linux-bluetooth
In-Reply-To: <eb7933e21002210454r4b3805e1pec3f4f58440191bf@mail.gmail.com>
Hi Daniel,
> > Please try to at least do a compile check before you submit patches.
> > This one gives the following error:
> > audio/avdtp.c: In function ‘handle_unanswered_req’:
> > audio/avdtp.c:908: error: comparison between pointer and integer
> >
> > What you probably want is session->req->signal_id == AVDTP_ABORT.
> >
>
> Sorry. The attached patch compiles without warnings on "./configure && make".
> (new dependency to a capabilities lib prevents ./bootstrap-configure atm)
you can use ./bootstrap-configure --disable-capng.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Fix double free on AVDTP Abort response
From: Daniel Örstadius @ 2010-02-21 12:54 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <20100219173257.GA12855@jh-x301>
[-- Attachment #1: Type: text/plain, Size: 563 bytes --]
On Fri, Feb 19, 2010 at 7:32 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Please try to at least do a compile check before you submit patches.
> This one gives the following error:
> audio/avdtp.c: In function ‘handle_unanswered_req’:
> audio/avdtp.c:908: error: comparison between pointer and integer
>
> What you probably want is session->req->signal_id == AVDTP_ABORT.
>
Sorry. The attached patch compiles without warnings on "./configure && make".
(new dependency to a capabilities lib prevents ./bootstrap-configure atm)
Br,
Daniel
[-- Attachment #2: 0001-Fix-double-free-on-AVDTP-Abort-response.patch --]
[-- Type: text/x-patch, Size: 974 bytes --]
From f45006ec75c23b55470e8088fe64e8f0b6ab6404 Mon Sep 17 00:00:00 2001
From: Daniel Orstadius <daniel.orstadius@gmail.com>
Date: Sun, 21 Feb 2010 14:39:49 +0200
Subject: [PATCH] Fix double free on AVDTP Abort response
The pending request might be freed twice when receiving an Abort
response, in handle_unanswered_req and session_cb. Avoid freeing
it in handle_unanswered_req.
---
audio/avdtp.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/audio/avdtp.c b/audio/avdtp.c
index 2591845..add08f1 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -905,6 +905,13 @@ static void handle_unanswered_req(struct avdtp *session,
struct avdtp_local_sep *lsep;
struct avdtp_error err;
+ if (session->req->signal_id == AVDTP_ABORT) {
+ /* Avoid freeing the Abort request here */
+ debug("handle_unanswered_req: Abort req, returning");
+ session->req->stream = NULL;
+ return;
+ }
+
req = session->req;
session->req = NULL;
--
1.6.0.4
^ permalink raw reply related
* Re: [bluetooth-next 1/1] bluetooth: handle device reset event
From: Marcel Holtmann @ 2010-02-21 10:27 UTC (permalink / raw)
To: Tomas Winkler; +Cc: linux-bluetooth, guy.cohen, ron.rindjunsky, Gregory Paskar
In-Reply-To: <1266486007-25015-1-git-send-email-tomas.winkler@intel.com>
Hi Tomas,
> From: Gregory Paskar <greogry.paskar@intel.com>
>
> A bluetooth device experiencing hardware failure may issue
> a HARDWARE_ERROR hci event. The reaction to this event is device
> reset flow implemented in following sequence.
since Bluetooth is a brand name, I prefer you write it properly inside
the commit message or comments. The B is uppercase, everything else is
lowercase.
> 1. Notify: HCI_DEV_DOWN
> 2. Reinitialize internal structures.
> 3. Call driver flush function
> 4. Send HCI reset request to the device.
> 5. Send HCI init sequence reset to the device.
> 6. Notify HCI_DEV_UP.
>
> Signed-off-by: Gregory Paskar <greogry.paskar@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Reviewed-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
> ---
> include/net/bluetooth/hci.h | 5 ++++
> include/net/bluetooth/hci_core.h | 2 +
> net/bluetooth/hci_core.c | 40 +++++++++++++++++++++++++++++++++++++-
> net/bluetooth/hci_event.c | 3 ++
> 4 files changed, 49 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index ed3aea1..cd23eb4 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -691,6 +691,11 @@ struct hci_ev_cmd_status {
> __le16 opcode;
> } __attribute__ ((packed));
>
> +#define HCI_EV_HARDWARE_ERROR 0x10
> +struct hci_ev_hw_error {
> + __u8 hw_code;
> +} __attribute__ ((packed));
> +
> #define HCI_EV_ROLE_CHANGE 0x12
> struct hci_ev_role_change {
> __u8 status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 7b86094..d2b1cba 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -110,6 +110,8 @@ struct hci_dev {
> struct tasklet_struct rx_task;
> struct tasklet_struct tx_task;
>
> + struct work_struct hw_err_work;
> +
> struct sk_buff_head rx_q;
> struct sk_buff_head raw_q;
> struct sk_buff_head cmd_q;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 94ba349..6002439 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -834,6 +834,44 @@ static int hci_rfkill_set_block(void *data, bool blocked)
> return 0;
> }
>
> +static void hci_device_hw_error(struct hci_dev *hdev)
> +{
> + hci_req_lock(hdev);
> + clear_bit(HCI_UP, &hdev->flags);
> + hci_notify(hdev, HCI_DEV_DOWN);
> + tasklet_disable(&hdev->tx_task);
> + skb_queue_purge(&hdev->rx_q);
> + skb_queue_purge(&hdev->cmd_q);
> + hci_dev_lock_bh(hdev);
> + inquiry_cache_flush(hdev);
> + hci_conn_hash_flush(hdev);
> + hci_dev_unlock_bh(hdev);
> + if (hdev->flush)
> + hdev->flush(hdev);
> + atomic_set(&hdev->cmd_cnt, 1);
> + hdev->acl_cnt = 0;
> + hdev->sco_cnt = 0;
> + hci_reset_req(hdev, 0);
> + atomic_set(&hdev->cmd_cnt, 1);
> + hci_init_req(hdev, 0);
> + tasklet_enable(&hdev->tx_task);
> + set_bit(HCI_UP, &hdev->flags);
> + hci_notify(hdev, HCI_DEV_UP);
> + hci_req_unlock(hdev);
> +}
This is a big block of commands. Almost impossible for anybody to follow
easily. You need to split them into small logical chunks and separate
them by empty lines.
Also I think there could be more unification with other init and reset
code inside hci_core.c.
> +
> +
I don't know where you get the double empty lines between functions
from, but I don't remember anything inside net/bluetooth/ doing it. And
even if, then that is an oversight.
> +static void hci_hw_err_work(struct work_struct *ws)
> +{
> + struct hci_dev *hdev;
> + hdev = container_of(ws, struct hci_dev, hw_err_work);
You could assign it directly at variable declaration.
> + if (!hdev) {
> + BT_DBG("%s: hci_hw_error_worker: hdev = NULL ", hdev->name);
> + return;
> + }
Can it really happen that hdev is NULL? Even if hdev goes away in
between, the pointer should be not NULL, it should be actually invalid
and cause more harm.
> + hci_device_hw_error(hdev);
> +}
> +
> static const struct rfkill_ops hci_rfkill_ops = {
> .set_block = hci_rfkill_set_block,
> };
> @@ -903,7 +941,7 @@ int hci_register_dev(struct hci_dev *hdev)
> tasklet_init(&hdev->cmd_task, hci_cmd_task,(unsigned long) hdev);
> tasklet_init(&hdev->rx_task, hci_rx_task, (unsigned long) hdev);
> tasklet_init(&hdev->tx_task, hci_tx_task, (unsigned long) hdev);
> -
> + INIT_WORK(&hdev->hw_err_work, hci_hw_err_work);
> skb_queue_head_init(&hdev->rx_q);
> skb_queue_head_init(&hdev->cmd_q);
> skb_queue_head_init(&hdev->raw_q);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 28517ba..e8f48cc 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1953,6 +1953,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
> case HCI_EV_REMOTE_HOST_FEATURES:
> hci_remote_host_features_evt(hdev, skb);
> break;
> + case HCI_EV_HARDWARE_ERROR:
> + schedule_work(&hdev->hw_err_work);
> + break;
The more I think about this, the more I feel like we should be
processing the HCI event in a work queue anyway. The sysfs integration
needs it and it is not a performance critical path anyway. The ACL and
SCO/eSCO packets are the only ones where processing in a tasklet really
matters.
Regards
Marcel
^ permalink raw reply
* Re: Kernel panic in rfcomm_run - unbalanced refcount on rfcomm_session
From: Dave Young @ 2010-02-20 8:17 UTC (permalink / raw)
To: Nick Pelly; +Cc: Bluettooth Linux
In-Reply-To: <35c90d961002172104q3af1ca8p850004f8b93e8af7@mail.gmail.com>
On Thu, Feb 18, 2010 at 1:04 PM, Nick Pelly <npelly@google.com> wrote:
> Since 2.6.32 we are seeing kernel panics like:
>
> [10651.110229] Unable to handle kernel paging request at virtual
> address 6b6b6b6b
> [10651.111968] Internal error: Oops: 5 [#1] PREEMPT
> [10651.113952] CPU: 0 Tainted: G W (2.6.32-59979-gd0c97db #1)
> [10651.114624] PC is at rfcomm_run+0xa04/0xdbc
> <...>
> [10651.406188] [<c031ad24>] (rfcomm_run+0xa04/0xdbc) from [<c006ce30>]
> (kthread+0x78/0x80)
> [10651.406585] [<c006ce30>] (kthread+0x78/0x80) from [<c002793c>]
> (kernel_thread_exit+0x0/0x8)
>
> (rfcomm_run() is all inlined so theres not much of a stack trace))
Could you make rfcomm_process_sessions to be not inlined, and get new
kernel logs?
>
> This is a use-after-free on struct rfcomm_session s in the call chain
> rfcomm_run() -> rfcomm_process_sessions() -> rfcomm_process_dlcs() ->
> list_for_each_safe(p, n, &s->dlcs). The only way this can happen is if
> there is an unbalanced refcount on the rfcomm session.
>
> We found that reverting the patch
> 9e726b17422bade75fba94e625cd35fd1353e682 "Bluetooth: Fix rejected
> connection not disconnecting ACL link" fixes the issue for us. The
> patch itself looks ok, I added some logging to check the new refcounts
> in the patch are balanced and they are. However if I remove the new
> calls to rfcomm_session_put() and rfcomm_session_hold() the crash is
> resolved. I also found that we can crash without hitting
> rfcomm_session_timeout(), so its not related to Marcel's recent patch
> to remove the scheduling-while-atomic warning.
>
> 9e726b17422bade75fba94e625cd35fd1353e682 does lead to a delay in
> calling rfcomm_session_del() due to the extra refcount while waiting
> for the new timeout. I believe that this delay has revealed some more
> subtle problem elsewhere that causes an unbalanced refcount and then
> the kernel panic.
>
> I have debug kernel logs and hci logs - they are too large to send to
> the list but I can send them directly to anyone interested in
> debugging.
>
> We see this crash frequently with a number of headsets since 2.6.32,
> but not reliably. I do have a 100% repro case with the Nuvi Garmin,
> with these exact steps:
> 1) Make sure Nuvi is unpaired, Bluez stack is unpaired, and kernel has
> been rebooted since unpairing.
> 2) Initiate device discovery, pairing, and handsfree connection from Nuvi
> 3) Observe HFP rfcomm connect briefly, then disconnect, and kernel panic
>
> Our short-term solution is unfortunately to revert
> 9e726b17422bade75fba94e625cd35fd1353e682.
>
> Nick
> --
> 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
>
--
Regards
dave
^ 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