Linux bluetooth development
 help / color / mirror / Atom feed
* Re: [PATCH] Allow disable park state in main.conf
From: Johan Hedberg @ 2010-02-26 14:38 UTC (permalink / raw)
  To: Liang Bao, marcel; +Cc: linux-bluetooth
In-Reply-To: <6aeb672b1002260532v6e209d7dr8370003463334b3@mail.gmail.com>

Hi,

On Fri, Feb 26, 2010, Liang Bao wrote:
> Currently BlueZ is hard-coding the default link policy to include
> hold mode, role switch, sniff mode and park state. However, some
> device will have problem to maintain the connection or setup SCO
> if they are in park state. Making the park state configurable in
> main.conf improves flexibility so it can be disabled when needed.
> 
> Signed-off-by: Tim Bao <tim.bao@gmail.com>
> ---
>  src/main.c    |   13 +++++++++----
>  src/main.conf |    5 +++++
>  2 files changed, 14 insertions(+), 4 deletions(-)

In principle the patch looks ok to me, however we might want to discuss
a little bit about the naming of the parameter in the config file and
its possible values. E.g. would it make sense to be able to define the
link policy more flexibly than just toggling park mode on and off.
Marcel, do you have any thoughts on this?

Johan

^ permalink raw reply

* Re: [PATCH] Allow disable park state in main.conf
From: Bao Liang @ 2010-02-26 15:11 UTC (permalink / raw)
  To: Johan Hedberg, marcel; +Cc: linux-bluetooth

Thanks for your comment . The reason I do this is that only park mode makes=
 trouble when used  by certain models as far as I know . But I'm open to  m=
ake this parameter an hex value if it make more sense .

----- =E5=8E=9F=E5=A7=8B=E9=82=AE=E4=BB=B6 -----
=E5=8F=91=E4=BB=B6=E4=BA=BA: Johan Hedberg <johan.hedberg@gmail.com>
=E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2010=E5=B9=B42=E6=9C=8826=E6=97=A5 22=
:38
=E6=94=B6=E4=BB=B6=E4=BA=BA: Liang Bao <tim.bao@gmail.com>; marcel@holtmann=
.org
=E6=8A=84=E9=80=81: linux-bluetooth@vger.kernel.org
=E4=B8=BB=E9=A2=98: Re: [PATCH] Allow disable park state in main.conf

Hi,

On Fri, Feb 26, 2010, Liang Bao wrote:
> Currently BlueZ is hard-coding the default link policy to include
> hold mode, role switch, sniff mode and park state. However, some
> device will have problem to maintain the connection or setup SCO
> if they are in park state. Making the park state configurable in
> main.conf improves flexibility so it can be disabled when needed.
>=20
> Signed-off-by: Tim Bao <tim.bao@gmail.com>
> ---
>  src/main.c    |   13 +++++++++----
>  src/main.conf |    5 +++++
>  2 files changed, 14 insertions(+), 4 deletions(-)

In principle the patch looks ok to me, however we might want to discuss
a little bit about the naming of the parameter in the config file and
its possible values. E.g. would it make sense to be able to define the
link policy more flexibly than just toggling park mode on and off.
Marcel, do you have any thoughts on this?

Johan

^ permalink raw reply

* Re: What's the idea of security_cfm is called for both hci_proto_auth_cfm and hci_proto_encrypt_cfm? -resend
From: Liejun Tao @ 2010-02-27  0:19 UTC (permalink / raw)
  To: Bluettooth Linux
In-Reply-To: <8a7a8d7b1002171330m7d1549deye643aecad10c5822@mail.gmail.com>

On Wed, Feb 17, 2010 at 3:30 PM, Liejun Tao <liejuntao001@gmail.com> wrote:
> Hi Marcel,
> Beside the l2cap issue with Blackberry 8900, I meet another l2cap
> connection failure issue when testing
> PTS AVRCP_TC_TG_PTT_BV_01_I.
>
> For 8900, the sequence is:
> l2cap_do_start(): send L2CAP_INFO_REQ
> security_cfm(): send L2CAP_CONN_REQ
> l2cap_information_rsp(): call l2cap_do_start(), send L2CAP_CONN_REQ again
>
> For PTS AVRCP_TC_TG_PTT_BV_01_I, the sequence is:
> l2cap_do_start(): send L2CAP_CONN_REQ
> security_cfm():send L2CAP_CONN_REQ again
> l2cap_conn_rsp(): receive L2CAP_CR_PEND
> l2cap_conn_rsp(): receive L2CAP_CR_NO_MEM
>
> In both cases, security_cfm() is called from hci_proto_encrypt_cfm(),
> and interrupt the normal sequence of l2cap connection.
>
> security_cfm() is also called in hci_proto_auth_cfm. I feel auth_cfm
> and encryption_cfm are for different events, why they call the same
> function?
>
> What's your mind to avoid 2 connection request?
>
> Aside, should we set a state bit after send l2CAP_CONN_REQ?
>

Ping.

Hi Marcel,
Any idea how to resolve these 2 issues?

^ permalink raw reply

* Making serial (rfcomm0) work, need magic incantation, chicken foot voodoo or what?
From: Gene Heskett @ 2010-02-27 17:04 UTC (permalink / raw)
  To: Bluettooth Linux

Greetings all;

How does one go about, on linux, making /dev/rfcomm0 available to the likes 
of minicom?

Its there, blueman-manager says I have a good signal most of the time, that 
it is trusted and bonded.  lsof discloses no open paths to the device, but 
minicom claims it is in use and exits.  There is a shell running on the other 
end that should give me a system prompt from that system.

I have restarted /etc/init.d/bluetooth several times to no avail.

It may become usable if I reboot this machine, but that isn't how its 
supposed to work I don't think.

So what sort of a magic spell do I need here?

-- 
Cheers, Gene
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)

Of course it doesn't work. We've performed a software upgrade.

^ permalink raw reply

* Re: A2DP multiple sink support
From: Anil Sasidharan @ 2010-02-27 19:42 UTC (permalink / raw)
  To: Iain Hibbert; +Cc: linux-bluetooth
In-Reply-To: <1265396410.615373.5159.nullmailer@galant.ukfsn.org>

On Sat, Feb 6, 2010 at 12:30 AM, Iain Hibbert <plunky@rya-online.net> wrote:
> On Fri, 5 Feb 2010, Anil Sasidharan wrote:
>
>> Thanks a lot for the inputs. Based on this I've tried some experiments.
>> 1. A2DP src has been paired and connected to the A2DP sink
>> 2. While the connection with first SINK is active, I've tried to pair
>> and connect to another A2DP sink
>> 3. Pairing with the second SINK gets done, however while trying to
>> connect I'm encountering an error from D-Bus "Stream Setup Failed".
>> What could be wrong
>>
>> It would be really great if you could throw some light on this. BTW
>> I'm using Bluez v3.36
>
> Anil I can throw no more light on that as I don't use BlueZ at all. And, I
> think the only advice you will get on that list is to try the latest BlueZ
> version..  they don't really provide support for outdated versions.
>
> regards,
> iain
>
>
>

Hi Iain,

          Thanks for the inputs. I was able to resolve the "Stream
Setup" error and got the connection established with 2 SNKs. However
while trying stream audio to the second SNK, I'm not getting any
audio-output though the debug messages shows that the AVDTP sends out
"audio chunks". Any thoughts?

Warm Regards,
Anil

^ permalink raw reply

* Phonebook functions for BlueZ
From: Felix Huber @ 2010-02-28 10:01 UTC (permalink / raw)
  To: linux-bluetooth

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

Hello,

I have analyzed the data traffic of my car handsfree set and implemented
the functions for the phonebook retrieval via the CPBR and CPBS
commands. In addition, I added a telephony driver for the FSO middleware
with which I tested the new functions using a Parrot CK3100 car kit and
a Jabra ear plug using version 4.61 of BlueZ. The other telephony
drivers just have empty functions that reject the new commands in order
not to break the APIs.

Attached is the patch file for the git repository.

Felix Huber

[-- Attachment #2: 0001-Added-support-for-HFP-phonebook-functions-and-FSO-mi.patch --]
[-- Type: text/x-patch, Size: 45623 bytes --]

>From a1e5b0d81b36242ac1a4c7362dd5146395bd7f1f Mon Sep 17 00:00:00 2001
From: Felix Huber <felix.huber@schyf.de>
Date: Fri, 26 Feb 2010 12:15:31 +0100
Subject: [PATCH] Added support for HFP phonebook functions and FSO middleware

---
 audio/headset.c         |   80 +++
 audio/telephony-dummy.c |   11 +
 audio/telephony-fso.c   | 1411 +++++++++++++++++++++++++++++++++++++++++++++++
 audio/telephony-maemo.c |   10 +
 audio/telephony-ofono.c |   11 +
 audio/telephony.h       |   12 +
 6 files changed, 1535 insertions(+), 0 deletions(-)
 create mode 100644 audio/telephony-fso.c

diff --git a/audio/headset.c b/audio/headset.c
index 7002a3a..442a73d 100644
--- a/audio/headset.c
+++ b/audio/headset.c
@@ -1154,6 +1154,84 @@ static int voice_dial(struct audio_device *device, const char *buf)
 	return 0;
 }
 
+int get_ATtype(const char *buf, int *offset)
+{
+	const char *ATquery = "=?", *ATcheck ="?", *ATset = "=";
+
+	if (!strncmp(buf, ATquery, 2)) {
+		*offset = 2;
+		return (ATQUERY);
+	} else if (!strncmp(buf, ATcheck, 1)) {
+		*offset = 1;
+		return (ATCHECK);
+	} else if (!strncmp(buf, ATset, 1)) {
+		*offset = 1;
+		return (ATSET);
+	} else {
+		*offset = 0;
+		return (ATNONE);
+	}
+}
+
+
+int telephony_phonebook_storage_rsp(void *telephony_device, cme_error_t err)
+{
+	return telephony_generic_rsp(telephony_device, err);
+}
+
+int telephony_phonebook_storage_ind(void *telephony_device, const char *storagelist)
+{
+	struct audio_device *device = telephony_device;
+	struct headset *hs = device->headset;
+
+	headset_send(hs, "\r\n+CPBS: %s \r\n", storagelist);
+
+	return 0;
+}
+
+static int phonebook_storage(struct audio_device *device, const char *buf)
+{
+	int ATtype, offset;
+
+	if (strlen(buf) < 8) {
+		return -EINVAL;
+	}
+
+	ATtype = get_ATtype(&buf[7], &offset);
+	telephony_phonebook_storage_req(device, &buf[7+offset], ATtype);
+
+	return 0;
+}
+
+int telephony_phonebook_read_rsp(void *telephony_device, cme_error_t err)
+{
+	return telephony_generic_rsp(telephony_device, err);
+}
+
+int telephony_phonebook_read_ind(void *telephony_device, const char *entrylist)
+{
+	struct audio_device *device = telephony_device;
+	struct headset *hs = device->headset;
+
+	headset_send(hs, "\r\n+CPBR: %s\r\n", entrylist);
+
+	return 0;
+}
+
+static int phonebook_read(struct audio_device *device, const char *buf)
+{
+	int ATtype, offset;
+
+	if (strlen(buf) < 8) {
+		return -EINVAL;
+	}
+	ATtype = get_ATtype(&buf[7], &offset);
+	telephony_phonebook_read_req(device, &buf[7+offset], ATtype);
+
+	return 0;
+}
+
+
 static struct event event_callbacks[] = {
 	{ "ATA", answer_call },
 	{ "ATD", dial_number },
@@ -1175,6 +1253,8 @@ static struct event event_callbacks[] = {
 	{ "AT+COPS", operator_selection },
 	{ "AT+NREC", nr_and_ec },
 	{ "AT+BVRA", voice_dial },
+	{ "AT+CPBS", phonebook_storage },
+	{ "AT+CPBR", phonebook_read },
 	{ 0 }
 };
 
diff --git a/audio/telephony-dummy.c b/audio/telephony-dummy.c
index 2409a49..71d1d2b 100644
--- a/audio/telephony-dummy.c
+++ b/audio/telephony-dummy.c
@@ -225,6 +225,17 @@ void telephony_key_press_req(void *telephony_device, const char *keys)
 	telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+
 /* D-Bus method handlers */
 static DBusMessage *outgoing_call(DBusConnection *conn, DBusMessage *msg,
 					void *data)
diff --git a/audio/telephony-fso.c b/audio/telephony-fso.c
new file mode 100644
index 0000000..ec5a75f
--- /dev/null
+++ b/audio/telephony-fso.c
@@ -0,0 +1,1411 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *  telephony interface for Freesmartphone.org stack
+ *
+ *  Copyright (C) 2009-2010  Intel Corporation
+ *  Copyright (C) 2006-2009  Nokia Corporation
+ *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
+ *  Copyright (C) 2010       Felix Huber
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <glib.h>
+#include <dbus/dbus.h>
+#include <gdbus.h>
+
+#include "logging.h"
+#include "telephony.h"
+
+/* Mask bits for supported services */
+#define NETWORK_MASK_GPRS_SUPPORT       0x01
+#define NETWORK_MASK_COMPACT_GSM        0x02
+#define NETWORK_MASK_UMTS               0x04
+#define NETWORK_MASK_EGDGE              0x08
+#define NETWORK_MASK_HSDPA_AVAIL        0x10
+#define NETWORK_MASK_HSUPA_AVAIL        0x20
+
+/* network get cell info: cell type */
+#define NETWORK_UNKNOWN_CELL            0
+#define NETWORK_GSM_CELL                1
+#define NETWORK_WCDMA_CELL              2
+
+enum net_registration_status {
+	NETWORK_REG_STATUS_UNREGISTERED = 0x00,
+	NETWORK_REG_STATUS_HOME,
+	NETWORK_REG_STATUS_BUSY,
+	NETWORK_REG_STATUS_DENIED,
+	NETWORK_REG_STATUS_UNKOWN,
+	NETWORK_REG_STATUS_ROAM,
+};
+
+enum network_types {
+	NETWORK_GSM = 0,
+	NETWORK_COMPACT_GSM,
+	NETWORK_UMTS,
+	NETWORK_EDGE,
+	NETWORK_HSDPA,
+	NETWORK_PSUPA,
+	NETWORK_HSDPAHSUPA
+};
+
+
+struct voice_call {
+	dbus_int32_t call_index;
+	int status;
+	gboolean originating;
+	char *number;
+};
+
+static struct {
+	void *telephony_device;
+	int first;
+	int last;
+	int category;
+} phonebook_info = {
+	.telephony_device = NULL,
+	.first = -1,
+	.last = -1,
+	.category = 0
+};
+
+#define NUM_CATEGORIES 7
+char *fso_categories[NUM_CATEGORIES] = {"contacts", "emergency", "fixed", "dialed", "received", "missed", "own"};
+char *gsm_categories[NUM_CATEGORIES] = {"\"SM\"",   "\"EN\"",    "\"FD\"","\"DC\"", "\"RC\"",   "\"MC\"", "\"ON\""};
+#define OWN_CATEGORY 6
+#define SIM_CATEGORY 0
+
+/* OM sends:
++CPBS: ("EN","BD","FD","DC","LD","RC","LR","MT","AD","SM","SD","MC","LM","AF","ON","UD")
+"EN" SIM (or ME) emergency number
+"FD" SIM fixed-dialing-phonebook
+"LD" SIM last-dialing-phonebook
+"BD" SIM barred-dialing phonebook
+"SD" SIM service numbers
+"DC" MT dialled calls list
+"RC" MT received calls list
+"LR" Last received numbers (nonstandard)
+"MT" combined MT and SIM/UICC phonebook
+"AD" Abbreviated dialing numbers (nonstandard)
+"LM" Last missed numbers (nonstandard)
+"AF" comb. of fixed and abbrev. dialing phonebook (nonstandard)
+"MC" MT missed (unanswered received) calls list
+"ON" active application in the UICC (GSM or USIM) or SIM card (or MT) own numbers (MSISDNs) list
+"UD" User defined
+*/
+
+static DBusConnection *connection = NULL;
+static char *last_dialed_number = NULL;
+static GSList *calls = NULL;
+
+#define FSO_BUS_NAME "org.freesmartphone.ogsmd"
+#define FSO_MODEM_OBJ_PATH "/org/freesmartphone/GSM/Device"
+#define FSO_NETWORKREG_INTERFACE "org.freesmartphone.GSM.Network"
+#define FSO_GSMC_INTERFACE "org.freesmartphone.GSM.Call"
+#define FSO_SIM_INTERFACE "org.freesmartphone.GSM.SIM"
+
+static guint registration_watch = 0;
+static guint voice_watch = 0;
+static guint device_watch = 0;
+
+/* HAL battery namespace key values */
+static int battchg_cur = -1;    /* "battery.charge_level.current" */
+static int battchg_last = -1;   /* "battery.charge_level.last_full" */
+static int battchg_design = -1; /* "battery.charge_level.design" */
+
+static struct {
+	uint8_t status;         // act  'GSM'
+	uint32_t cell_id;       // cid  '51FD'
+	uint32_t operator_code; // code '26203'
+	uint16_t lac;           // lac  '233b' 
+	char *mode;             // mode 'automatic'
+	char *operator_name;    // provider  'debitel'
+	char *registration;     // registration 'home'
+	uint16_t signals_bar;   // strength  '87'
+} net = {
+	.status = NETWORK_REG_STATUS_UNREGISTERED,
+	.cell_id = 0,
+	.operator_code = 0,
+	.lac = 0,
+	.mode = NULL,
+	.operator_name = NULL,
+	.registration = NULL,
+	.signals_bar = 0,
+};
+
+
+static const char *chld_str = "0,1,1x,2,2x,3,4";
+static char *subscriber_number = NULL;
+
+static gboolean events_enabled = FALSE;
+
+/* Response and hold state
+ * -1 = none
+ *  0 = incoming call is put on hold in the AG
+ *  1 = held incoming call is accepted in the AG
+ *  2 = held incoming call is rejected in the AG
+ */
+static int response_and_hold = -1;
+
+static struct indicator fso_indicators[] =
+{
+	{ "battchg",	"0-5",	5,	TRUE },
+	{ "signal",	"0-5",	5,	TRUE },
+	{ "service",	"0,1",	1,	TRUE },
+	{ "call",	"0,1",	0,	TRUE },
+	{ "callsetup",	"0-3",	0,	TRUE },
+	{ "callheld",	"0-2",	0,	FALSE },
+	{ "roam",	"0,1",	0,	TRUE },
+	{ NULL }
+};
+
+
+static void vc_free(struct voice_call *vc)
+{
+	if (!vc)
+		return;
+	g_free(vc->number);
+	g_free(vc);
+}
+
+
+static struct voice_call *find_vc(dbus_int32_t call_index)
+{
+	GSList *l;
+
+	for (l = calls; l != NULL; l = l->next) {
+		struct voice_call *vc = l->data;
+
+		if (vc->call_index == call_index)
+			return vc;
+	}
+
+	return NULL;
+}
+
+static struct voice_call *find_vc_with_status(int status)
+{
+	GSList *l;
+
+	for (l = calls; l != NULL; l = l->next) {
+		struct voice_call *vc = l->data;
+
+		if (vc->status == status)
+			return vc;
+	}
+
+	return NULL;
+}
+
+static gboolean iter_get_basic_args(DBusMessageIter *iter,
+                                        int first_arg_type, ...)
+{
+	int type;
+	va_list ap;
+
+	va_start(ap, first_arg_type);
+
+	for (type = first_arg_type; type != DBUS_TYPE_INVALID;
+		type = va_arg(ap, int)) {
+		void *value = va_arg(ap, void *);
+		int real_type = dbus_message_iter_get_arg_type(iter);
+
+		if (real_type != type) {
+			error("iter_get_basic_args: expected %c but got %c",
+				(char) type, (char) real_type);
+			break;
+		}
+
+		dbus_message_iter_get_basic(iter, value);
+		dbus_message_iter_next(iter);
+	}
+
+	va_end(ap);
+
+	return type == DBUS_TYPE_INVALID ? TRUE : FALSE;
+}
+
+static int reply_check_error(DBusError *err, DBusMessage *reply)
+{
+	dbus_error_init(err);
+	if (dbus_set_error_from_message(err, reply)) {
+		error("fso replied with an error: %s, %s",
+			err->name, err->message);
+		dbus_error_free(err);
+		return -1;
+	}
+	return 0;
+}
+
+static int find_category(char **phonebooks, const char *category)
+{
+	int i;
+	for (i=0; i < NUM_CATEGORIES; i++) {
+		if (!strcmp(phonebooks[i], category))
+			return i;
+	}
+	return -1;
+
+}
+
+
+void telephony_device_connected(void *telephony_device)
+{
+	debug("telephony-fso: device %p connected", telephony_device);
+}
+
+void telephony_device_disconnected(void *telephony_device)
+{
+	debug("telephony-fso: device %p disconnected", telephony_device);
+	events_enabled = FALSE;
+}
+
+void telephony_event_reporting_req(void *telephony_device, int ind)
+{
+	events_enabled = ind == 1 ? TRUE : FALSE;
+
+	telephony_event_reporting_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_response_and_hold_req(void *telephony_device, int rh)
+{
+	response_and_hold = rh;
+
+	telephony_response_and_hold_ind(response_and_hold);
+
+	telephony_response_and_hold_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_last_dialed_number_req(void *telephony_device)
+{
+	debug("telephony-fso: last dialed number request");
+
+	if (last_dialed_number)
+		telephony_dial_number_req(telephony_device, last_dialed_number);
+	else
+		telephony_last_dialed_number_rsp(telephony_device,
+				CME_ERROR_NOT_ALLOWED);
+}
+
+static int send_method_call(const char *dest, const char *path,
+				const char *interface, const char *method,
+				DBusPendingCallNotifyFunction cb,
+				void *user_data, int type, ...)
+{
+	DBusMessage *msg;
+	DBusPendingCall *call;
+	va_list args;
+
+	msg = dbus_message_new_method_call(dest, path, interface, method);
+	if (!msg) {
+		error("Unable to allocate new D-Bus %s message", method);
+		return -ENOMEM;
+	}
+
+	va_start(args, type);
+
+	if (!dbus_message_append_args_valist(msg, type, args)) {
+		dbus_message_unref(msg);
+		va_end(args);
+		return -EIO;
+	}
+
+	va_end(args);
+
+	if (!cb) {
+		g_dbus_send_message(connection, msg);
+		return 0;
+	}
+
+	if (!dbus_connection_send_with_reply(connection, msg, &call, -1)) {
+		error("Sending %s failed", method);
+		dbus_message_unref(msg);
+		return -EIO;
+	}
+
+	dbus_pending_call_set_notify(call, cb, user_data, NULL);
+	dbus_pending_call_unref(call);
+	dbus_message_unref(msg);
+
+	return 0;
+}
+
+void telephony_terminate_call_req(void *telephony_device)
+{
+	struct voice_call *vc;
+	int ret;
+
+	if ((vc = find_vc_with_status(CALL_STATUS_ACTIVE))) {
+	} else if ((vc = find_vc_with_status(CALL_STATUS_DIALING))) {
+	} else if ((vc = find_vc_with_status(CALL_STATUS_INCOMING))) {
+	}
+
+	if (!vc) {
+		error("in telephony_terminate_call_req, no active call");
+		telephony_terminate_call_rsp(telephony_device,
+					CME_ERROR_NOT_ALLOWED);
+		return;
+	}
+
+	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+					FSO_GSMC_INTERFACE,
+					"Release", NULL, NULL,
+					DBUS_TYPE_INT32, &vc->call_index,
+					DBUS_TYPE_INVALID);
+
+
+	if (ret < 0) {
+		telephony_terminate_call_rsp(telephony_device,
+					CME_ERROR_AG_FAILURE);
+	} else
+		telephony_terminate_call_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_answer_call_req(void *telephony_device)
+{
+	int ret;
+	struct voice_call *vc = find_vc_with_status(CALL_STATUS_INCOMING);
+
+	if (!vc) {
+		telephony_answer_call_rsp(telephony_device,
+					CME_ERROR_NOT_ALLOWED);
+		return;
+	}
+
+	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+                                        FSO_GSMC_INTERFACE,
+					"Activate", NULL, NULL,
+					DBUS_TYPE_INT32, &vc->call_index,
+					DBUS_TYPE_INVALID);
+
+	if (ret < 0) {
+		telephony_answer_call_rsp(telephony_device,
+					CME_ERROR_AG_FAILURE);
+	} else
+		telephony_answer_call_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_dial_number_req(void *telephony_device, const char *number)
+{
+	const char *clir, *callType = "voice";
+	int ret;
+
+	debug("telephony-fso: dial request to %s", number);
+
+#if 0
+	if (!strncmp(number, "*31#", 4)) {
+		number += 4;
+		clir = "enabled";
+	} else if (!strncmp(number, "#31#", 4)) {
+		number += 4;
+		clir =  "disabled";
+	} else
+		clir = "default";
+#endif
+	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+			FSO_GSMC_INTERFACE,
+			"Initiate", NULL, NULL,
+			DBUS_TYPE_STRING, &number,
+			DBUS_TYPE_STRING, &callType,
+			DBUS_TYPE_INVALID);
+
+	if (ret < 0)
+		telephony_dial_number_rsp(telephony_device,
+			CME_ERROR_AG_FAILURE);
+	else
+		telephony_dial_number_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_transmit_dtmf_req(void *telephony_device, char tone)
+{
+	char *tone_string;
+	int ret;
+
+	debug("telephony-fso: transmit dtmf: %c", tone);
+
+	tone_string = g_strdup_printf("%c", tone);
+	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+                        FSO_GSMC_INTERFACE,
+			"SendDtmf", NULL, NULL,
+			DBUS_TYPE_STRING, &tone_string,
+			DBUS_TYPE_INVALID);
+	g_free(tone_string);
+
+	if (ret < 0)
+		telephony_transmit_dtmf_rsp(telephony_device,
+			CME_ERROR_AG_FAILURE);
+	else
+		telephony_transmit_dtmf_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_subscriber_number_req(void *telephony_device)
+{
+	debug("telephony-fso: subscriber number request");
+
+	if (subscriber_number)
+		telephony_subscriber_number_ind(subscriber_number,
+						NUMBER_TYPE_TELEPHONY,
+						SUBSCRIBER_SERVICE_VOICE);
+	telephony_subscriber_number_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_list_current_calls_req(void *telephony_device)
+{
+	GSList *l;
+	int i;
+
+	debug("telephony-fso: list current calls request");
+
+	for (l = calls, i = 1; l != NULL; l = l->next, i++) {
+		struct voice_call *vc = l->data;
+		int direction;
+
+		direction = vc->originating ?
+				CALL_DIR_OUTGOING : CALL_DIR_INCOMING;
+
+		telephony_list_current_call_ind(i, direction, vc->status,
+					CALL_MODE_VOICE, CALL_MULTIPARTY_NO,
+					vc->number, NUMBER_TYPE_TELEPHONY);
+	}
+	telephony_list_current_calls_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_operator_selection_req(void *telephony_device)
+{
+	debug("telephony-fso: operator selection request");
+
+	telephony_operator_selection_ind(OPERATOR_MODE_AUTO,
+				net.operator_name ? net.operator_name : "");
+	telephony_operator_selection_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_call_hold_req(void *telephony_device, const char *cmd)
+{
+	debug("telephony-fso: got call hold request %s", cmd);
+	telephony_call_hold_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_nr_and_ec_req(void *telephony_device, gboolean enable)
+{
+	debug("telephony-fso: got %s NR and EC request",
+			enable ? "enable" : "disable");
+
+	telephony_nr_and_ec_rsp(telephony_device, CME_ERROR_NONE);
+}
+
+void telephony_key_press_req(void *telephony_device, const char *keys)
+{
+	struct voice_call *vc;
+	int err=0;
+	char *cmd = NULL, *act="Activate", *rel="Release";
+
+	debug("telephony-fso: got key press request for %s", keys);
+
+	if (vc = find_vc_with_status(CALL_STATUS_INCOMING)) {
+		cmd = act;
+	} else if (vc = find_vc_with_status(CALL_STATUS_ACTIVE)) {
+		cmd = rel;
+	} else if (vc = find_vc_with_status(CALL_STATUS_DIALING)) {
+		cmd = rel;
+	}
+	if (cmd) {
+		err = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+					FSO_GSMC_INTERFACE,
+					cmd, NULL, NULL,
+					DBUS_TYPE_INT32, &vc->call_index,
+					DBUS_TYPE_INVALID);
+	}
+	if (err < 0)
+		telephony_key_press_rsp(telephony_device, CME_ERROR_AG_FAILURE);
+        else
+		telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);
+
+}
+
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+
+void telephony_voice_dial_req(void *telephony_device, gboolean enable)
+{
+	debug("telephony-fso: got %s voice dial request",
+			enable ? "enable" : "disable");
+
+	telephony_voice_dial_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+static void retrieve_entry_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	const char *name, *number;
+	char **number_type = user_data;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (reply_check_error(&err, reply))
+		goto done;
+
+	if (!dbus_message_get_args(reply, NULL,
+				DBUS_TYPE_STRING, &name,
+				DBUS_TYPE_STRING, &number,
+				DBUS_TYPE_INVALID)) 
+		goto done;
+
+	if (number_type == &subscriber_number) {
+		g_free(subscriber_number);
+		subscriber_number = g_strdup(number);
+	}
+
+done:
+	dbus_message_unref(reply);
+}
+
+
+static void retrieve_phonebook_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	DBusMessageIter iter, array;
+	int ret = 0;
+	char * info;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (reply_check_error(&err, reply)) {
+		ret = -1;
+		goto done;
+	}
+
+	dbus_message_iter_init(reply, &iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY) {
+		error("Unexpected signature in retrieve phonebook reply");
+		ret = -1;
+		goto done;
+	}
+
+	dbus_message_iter_recurse(&iter, &array);
+
+	while (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_INVALID) {
+		DBusMessageIter prop;
+		const char *name, *number;
+		int index;
+
+		dbus_message_iter_recurse(&array, &prop);
+
+		if (!iter_get_basic_args(&prop,
+					DBUS_TYPE_INT32, &index,
+					DBUS_TYPE_STRING, &name,
+					DBUS_TYPE_STRING, &number,
+					DBUS_TYPE_INVALID)) {
+			error("Invalid phonebook entry data");
+			break;
+		}
+		if ((index >= phonebook_info.first) && (index <= phonebook_info.last)) {
+			info = g_strdup_printf("%d,\"%s\",,\"%s\"", index, number, name);
+			telephony_phonebook_read_ind(phonebook_info.telephony_device, info);
+			g_free(info);
+		}
+		dbus_message_iter_next(&array);
+	}
+
+done:
+	dbus_message_unref(reply);
+	if (ret)
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
+	else
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);
+}
+
+
+
+static void get_phonebook_info_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	DBusMessageIter iter, iter_entry, array;
+	int ret = 0;
+	int min_index=-1, max_index =-1, number_length = -1, name_length = -1;
+	char * info;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (reply_check_error(&err, reply)) {
+		ret = -1;
+		goto done;
+	}
+
+	dbus_message_iter_init(reply, &iter);
+
+	/* ARRAY -> ENTRY -> VARIANT*/
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY) {
+		error("**** Unexpected signature in get phonebook info reply");
+		ret = -1;
+		goto done;
+	}
+
+	dbus_message_iter_recurse(&iter, &iter_entry);
+
+	if (dbus_message_iter_get_arg_type(&iter_entry)
+					!= DBUS_TYPE_DICT_ENTRY) {
+		error("**** Unexpected signature in get phonebook info reply");
+		ret = -1;
+		goto done;
+	}
+
+	while (dbus_message_iter_get_arg_type(&iter_entry)
+						!= DBUS_TYPE_INVALID) {
+		DBusMessageIter iter_property, sub;
+		char *property;
+
+		dbus_message_iter_recurse(&iter_entry, &iter_property);
+		if (dbus_message_iter_get_arg_type(&iter_property)
+					!= DBUS_TYPE_STRING) {
+			error("Unexpected signature in get phonebook info reply");
+			ret = -1;
+			goto done;
+		}
+
+		dbus_message_iter_get_basic(&iter_property, &property);
+
+		dbus_message_iter_next(&iter_property);
+		dbus_message_iter_recurse(&iter_property, &sub);
+
+		if (g_str_equal(property, "min_index")) {
+			dbus_message_iter_get_basic(&sub, &min_index);
+		} else if (g_str_equal(property, "max_index")) {
+			dbus_message_iter_get_basic(&sub, &max_index);
+		} else if (g_str_equal(property, "number_length")) {
+			dbus_message_iter_get_basic(&sub, &number_length);
+		} else if (g_str_equal(property, "name_length")) {
+			dbus_message_iter_get_basic(&sub, &name_length);
+		}
+
+		dbus_message_iter_next(&iter_entry);
+	}
+
+	info = g_strdup_printf("\"(%d-%d)\",%d,%d", min_index,max_index,
+		(number_length != -1) ? number_length : 0,
+		(name_length != -1) ? name_length : 0);
+
+	telephony_phonebook_read_ind(phonebook_info.telephony_device, info);
+	g_free(info);
+
+done:
+	dbus_message_unref(reply);
+	if (ret)
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
+	else
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);
+
+}
+
+static void get_phonebook_storage_info_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	int used, total;
+	GString *gstr;
+	char *str;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (reply_check_error(&err, reply))
+		goto done;
+
+	if (!dbus_message_get_args(reply, NULL,
+				DBUS_TYPE_INT32, &used,
+				DBUS_TYPE_INT32, &total,
+				DBUS_TYPE_INVALID))
+		goto done;
+
+	gstr = g_string_new("");
+	g_string_append_printf(gstr, "%s,%d,%d", gsm_categories[phonebook_info.category], used, total);
+	str = g_string_free(gstr, FALSE);
+	telephony_phonebook_storage_ind(phonebook_info.telephony_device, str);
+	g_free(str);
+	telephony_phonebook_storage_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);
+
+done:
+	dbus_message_unref(reply);
+
+}
+
+
+
+static void list_phonebooks_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	DBusMessageIter iter, iter_entry, array;
+	int ret = 0;
+	char **s, *str;
+	int i, n_s, index;
+	GString *gstr;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (reply_check_error(&err, reply)) {
+		ret = -1;
+		goto done;
+	}
+
+	dbus_message_iter_init(reply, &iter);
+
+	dbus_error_init(&err);
+
+	if (!dbus_message_get_args(reply, &err,
+		DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &s, &n_s,
+		DBUS_TYPE_INVALID)) {
+		error("dbus_message_get_args replied with an error: %s, %s",
+			err.name, err.message);
+		dbus_error_free(&err);
+		ret = -1;
+		goto done;
+	}
+
+	gstr = g_string_new("(");
+	for (i=0; i< n_s; i++) {
+		index = find_category(fso_categories, s[i]);
+		if (index != -1) {
+			debug("GSM %d: %s", index, gsm_categories[index]);
+			if (i == 0)
+				g_string_append_printf(gstr, "%s", gsm_categories[index]);
+			else
+				g_string_append_printf(gstr, ",%s", gsm_categories[index]);
+		}
+	}
+	g_string_append(gstr, ")");
+	str = g_string_free(gstr, FALSE);
+	telephony_phonebook_storage_ind(phonebook_info.telephony_device, str);
+	g_free(str);
+	dbus_free_string_array(s);
+
+done:
+	dbus_message_unref(reply);
+	if (ret)
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
+	else
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);
+}
+
+
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *storage, int ATtype)
+{
+	int ret=0, index;
+
+	debug("telephony-fso: got phonebook storage request %d", ATtype);
+
+	phonebook_info.telephony_device = telephony_device;
+
+	switch (ATtype) {
+	case ATQUERY:      // =?
+		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+			FSO_SIM_INTERFACE,
+			"ListPhonebooks",
+			list_phonebooks_reply, NULL,
+			DBUS_TYPE_INVALID);
+		break;
+	case ATCHECK:       // ?
+		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+			FSO_SIM_INTERFACE,
+			"GetPhonebookStorageInfo",
+			get_phonebook_storage_info_reply, NULL,
+			DBUS_TYPE_STRING, &fso_categories[phonebook_info.category],
+			DBUS_TYPE_INVALID);
+		break;
+	case ATSET:         // =
+		debug("Phonebook request to be %s", storage);
+		index = find_category(gsm_categories, storage);
+		debug ("Phonebook found at %d", index);
+		if (index == -1)
+			ret = -1;
+		else {
+			phonebook_info.category = index;
+			telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NONE);
+		}
+		break;
+	default:
+		ret = -1;
+	}
+	
+	if (ret < 0)
+		telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_AG_FAILURE);
+}
+
+
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	int size, ret=0;
+	debug("telephony-fso: got pbook read request: %s", readindex);
+
+	phonebook_info.telephony_device = telephony_device;
+
+	switch (ATtype) {
+	case ATQUERY:          // =?
+		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+		FSO_SIM_INTERFACE,
+			"GetPhonebookInfo",
+			get_phonebook_info_reply, NULL,
+			DBUS_TYPE_STRING, &fso_categories[phonebook_info.category],
+			DBUS_TYPE_INVALID);
+		break;
+
+	case ATCHECK:          // ?
+		ret = -1;
+		break;
+
+	case ATSET:            // =
+		phonebook_info.first=-1, phonebook_info.last=-1;
+		sscanf(readindex, "%d,%d", &phonebook_info.first, &phonebook_info.last);
+		if (phonebook_info.first == -1)
+			break;
+
+		if (phonebook_info.last == -1) phonebook_info.last = phonebook_info.first;
+
+		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+			FSO_SIM_INTERFACE,
+			"RetrievePhonebook",
+			retrieve_phonebook_reply, NULL,
+			DBUS_TYPE_STRING, &fso_categories[phonebook_info.category],
+			DBUS_TYPE_INT32, &phonebook_info.first,
+			DBUS_TYPE_INT32, &phonebook_info.last,
+			DBUS_TYPE_INVALID);
+		break;
+
+	default:
+		ret = -1;
+	}
+
+	if (ret < 0)
+		telephony_phonebook_read_rsp(telephony_device, CME_ERROR_AG_FAILURE);
+
+}
+
+
+
+static void parse_gsmcall_property(const char *property, DBusMessageIter sub, struct voice_call *vc)
+{
+	const char *direction, *peer, *reason, *auxstatus, *line;
+
+	if (g_str_equal(property, "direction")) {
+		dbus_message_iter_get_basic(&sub, &direction);
+	} else if (g_str_equal(property, "peer")) {
+		dbus_message_iter_get_basic(&sub, &peer);
+		vc->number = g_strdup(peer);
+	} else if (g_str_equal(property, "reason")) {
+		dbus_message_iter_get_basic(&sub, &reason);
+	} else if (g_str_equal(property, "auxstatus")) {
+		dbus_message_iter_get_basic(&sub, &auxstatus);
+	} else if (g_str_equal(property, "line")) {
+		dbus_message_iter_get_basic(&sub, &line);
+	}
+}
+
+
+static gboolean handle_gsmcall_property_changed(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	DBusMessageIter iter, iter_entry, array;
+	const char *status;
+	struct voice_call *vc;
+	dbus_int32_t call_index;
+
+	dbus_message_iter_init(msg, &iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_INT32) {
+		error("Unexpected signature in hal PropertyChanged signal");
+		return TRUE;
+	}
+
+	dbus_message_iter_get_basic(&iter, &call_index);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING) {
+	error("Unexpected signature in gsmcall PropertyChanged signal");
+		return TRUE;
+	}
+
+	dbus_message_iter_get_basic(&iter, &status);
+
+	debug("**** gsmProp Changed id:%d status: %s", call_index, status);
+
+
+	vc = find_vc(call_index);
+	if (!vc) {
+		vc = g_new0(struct voice_call, 1);
+	if (!vc)
+		return TRUE;
+	vc->call_index = call_index;
+	calls = g_slist_append(calls, vc);
+	}
+
+	dbus_message_iter_next(&iter);
+
+	/* array -> entry -> sv */
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY) {
+		error("Unexpected signature in gsmcallPropertyChanged signal");
+		return TRUE;
+	}
+
+	dbus_message_iter_recurse(&iter, &iter_entry);
+
+	if (dbus_message_iter_get_arg_type(&iter_entry)
+					!= DBUS_TYPE_DICT_ENTRY) {
+		error("Unexpected signature in gsmcallPropertyChanged signal");
+		return TRUE;
+	}
+
+	while (dbus_message_iter_get_arg_type(&iter_entry)
+					!= DBUS_TYPE_INVALID) {
+		DBusMessageIter iter_property, sub;
+		char *property;
+
+		dbus_message_iter_recurse(&iter_entry, &iter_property);
+		if (dbus_message_iter_get_arg_type(&iter_property)
+			!= DBUS_TYPE_STRING) {
+			error("Unexpected signature in gsmcallPropertyChanged signal");
+			return TRUE;
+		}
+
+		dbus_message_iter_get_basic(&iter_property, &property);
+
+		dbus_message_iter_next(&iter_property);
+		dbus_message_iter_recurse(&iter_property, &sub);
+
+		parse_gsmcall_property(property, sub, vc);
+
+		dbus_message_iter_next(&iter_entry);
+	}
+
+	if (g_str_equal(status, "incoming")) {
+		/* state change from waiting to incoming */
+		vc->status = CALL_STATUS_INCOMING;
+		vc->originating = FALSE;
+		telephony_update_indicator(fso_indicators, "callsetup",
+				EV_CALLSETUP_INCOMING);
+		telephony_incoming_call_ind(vc->number, NUMBER_TYPE_TELEPHONY);
+		debug("vc status is CALL_STATUS_INCOMING");
+	} else if (g_str_equal(status, "outgoing")) {
+		vc->status = CALL_STATUS_DIALING;
+		vc->originating = TRUE;
+		g_free(last_dialed_number);
+		last_dialed_number = g_strdup(vc->number);
+		telephony_update_indicator(fso_indicators, "callsetup",
+						EV_CALLSETUP_OUTGOING);
+	} else if (g_str_equal(status, "active")) {
+		telephony_update_indicator(fso_indicators,
+					"call", EV_CALL_ACTIVE);
+		telephony_update_indicator(fso_indicators,
+					"callsetup", EV_CALLSETUP_INACTIVE);
+		if (vc->status == CALL_STATUS_INCOMING) {
+			telephony_calling_stopped_ind();
+		}
+		vc->status = CALL_STATUS_ACTIVE;
+		debug("vc status is CALL_STATUS_ACTIVE");
+	} else if (g_str_equal(status, "held")) {
+		vc->status = CALL_STATUS_HELD;
+	} else if (g_str_equal(status, "release")) {
+		printf("in disconnected case\n");
+		if (vc->status == CALL_STATUS_ACTIVE)
+			telephony_update_indicator(fso_indicators,
+				"call", EV_CALL_INACTIVE);
+		else
+			telephony_update_indicator(fso_indicators,
+				"callsetup", EV_CALLSETUP_INACTIVE);
+		if (vc->status == CALL_STATUS_INCOMING)
+			telephony_calling_stopped_ind();
+		calls = g_slist_remove(calls, vc);
+		vc_free(vc);
+	}
+
+	return TRUE;
+}
+
+
+
+
+static void parse_registration_property(const char *property, DBusMessageIter sub)
+{
+	const char *status, *operator;
+	unsigned int signals_bar;
+
+	if (g_str_equal(property, "registration")) {
+		dbus_message_iter_get_basic(&sub, &status);
+		debug("registration is %s", status);
+		if (g_str_equal(status, "home")) {
+			net.status = NETWORK_REG_STATUS_HOME;
+			telephony_update_indicator(fso_indicators,
+						"roam", EV_ROAM_INACTIVE);
+			telephony_update_indicator(fso_indicators,
+						"service", EV_SERVICE_PRESENT);
+		} else if (g_str_equal(status, "roaming")) {
+			net.status = NETWORK_REG_STATUS_ROAM;
+			telephony_update_indicator(fso_indicators,
+						"roam", EV_ROAM_ACTIVE);
+			telephony_update_indicator(fso_indicators,
+						"service", EV_SERVICE_PRESENT);
+		} else {
+			net.status = NETWORK_REG_STATUS_UNREGISTERED;
+			telephony_update_indicator(fso_indicators,
+						"roam", EV_ROAM_INACTIVE);
+			telephony_update_indicator(fso_indicators,
+						"service", EV_SERVICE_NONE);
+		}
+	} else if (g_str_equal(property, "provider")) {
+		dbus_message_iter_get_basic(&sub, &operator);
+		debug("Operator is %s", operator);
+		g_free(net.operator_name);
+		net.operator_name = g_strdup(operator);
+	} else if (g_str_equal(property, "strength")) {
+		dbus_message_iter_get_basic(&sub, &signals_bar);
+		debug("SignalStrength is %d", signals_bar);
+		net.signals_bar = signals_bar;
+		telephony_update_indicator(fso_indicators, "signal",
+						(signals_bar + 20) / 21);
+	}
+}
+
+
+static gboolean handle_registration_property(DBusMessage *msg)
+{
+	DBusMessageIter iter, iter_entry;
+	const char *property;
+
+	dbus_message_iter_init(msg, &iter);
+
+	/* ARRAY -> ENTRY -> VARIANT*/
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY) {
+		error("**** Unexpected signature in GetProperties return");
+		goto done;
+	}
+
+	dbus_message_iter_recurse(&iter, &iter_entry);
+
+	if (dbus_message_iter_get_arg_type(&iter_entry)
+					!= DBUS_TYPE_DICT_ENTRY) {
+		error("**** Unexpected signature in GetProperties return");
+		goto done;
+	}
+
+	while (dbus_message_iter_get_arg_type(&iter_entry)
+					!= DBUS_TYPE_INVALID) {
+		DBusMessageIter iter_property, sub;
+		char *property;
+
+		dbus_message_iter_recurse(&iter_entry, &iter_property);
+		if (dbus_message_iter_get_arg_type(&iter_property)
+					!= DBUS_TYPE_STRING) {
+			error("Unexpected signature in GetProperties return");
+			goto done;
+		}
+
+		dbus_message_iter_get_basic(&iter_property, &property);
+
+		dbus_message_iter_next(&iter_property);
+		dbus_message_iter_recurse(&iter_property, &sub);
+
+		parse_registration_property(property, sub);
+
+                dbus_message_iter_next(&iter_entry);
+	}
+	
+done:
+	return TRUE;
+}
+
+
+static void get_registration_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	DBusMessageIter iter, iter_entry;
+	uint32_t features = AG_FEATURE_EC_ANDOR_NR |
+				AG_FEATURE_REJECT_A_CALL |
+				AG_FEATURE_ENHANCED_CALL_STATUS |
+				AG_FEATURE_EXTENDED_ERROR_RESULT_CODES;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (reply_check_error(&err, reply)) {
+		goto done;
+	}
+
+	handle_registration_property(reply);
+
+	telephony_ready_ind(features, fso_indicators,
+				response_and_hold, chld_str);
+
+done:
+	dbus_message_unref(reply);
+}
+
+
+static gboolean handle_registration_property_changed(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	return (handle_registration_property(msg));
+}
+
+
+static void hal_battery_level_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply;
+	DBusError err;
+	dbus_int32_t level;
+	int *value = user_data;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (reply_check_error(&err, reply)) {
+		goto done;
+	}
+
+	dbus_message_get_args(reply, NULL,
+				DBUS_TYPE_INT32, &level,
+				DBUS_TYPE_INVALID);
+
+	*value = (int) level;
+
+	if (value == &battchg_last)
+		debug("telephony-fso: battery.charge_level.last_full"
+					" is %d", *value);
+	else if (value == &battchg_design)
+		debug("telephony-fso: battery.charge_level.design"
+					" is %d", *value);
+	else
+		debug("telephony-fso: battery.charge_level.current"
+					" is %d", *value);
+
+	if ((battchg_design > 0 || battchg_last > 0) && battchg_cur >= 0) {
+		int new, max;
+
+		if (battchg_last > 0)
+			max = battchg_last;
+		else
+			max = battchg_design;
+
+		new = battchg_cur * 5 / max;
+
+		telephony_update_indicator(fso_indicators, "battchg", new);
+	}
+done:
+	dbus_message_unref(reply);
+}
+
+static void hal_get_integer(const char *path, const char *key, void *user_data)
+{
+	send_method_call("org.freedesktop.Hal", path,
+			"org.freedesktop.Hal.Device",
+			"GetPropertyInteger",
+			hal_battery_level_reply, user_data,
+			DBUS_TYPE_STRING, &key,
+			DBUS_TYPE_INVALID);
+}
+
+static gboolean handle_hal_property_modified(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	const char *path;
+	DBusMessageIter iter, array;
+	dbus_int32_t num_changes;
+
+	path = dbus_message_get_path(msg);
+
+	dbus_message_iter_init(msg, &iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_INT32) {
+		error("Unexpected signature in hal PropertyModified signal");
+		return TRUE;
+	}
+
+	dbus_message_iter_get_basic(&iter, &num_changes);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY) {
+		error("Unexpected signature in hal PropertyModified signal");
+		return TRUE;
+	}
+
+	dbus_message_iter_recurse(&iter, &array);
+
+	while (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_INVALID) {
+		DBusMessageIter prop;
+		const char *name;
+		dbus_bool_t added, removed;
+
+		dbus_message_iter_recurse(&array, &prop);
+
+		if (!iter_get_basic_args(&prop,
+					DBUS_TYPE_STRING, &name,
+					DBUS_TYPE_BOOLEAN, &added,
+					DBUS_TYPE_BOOLEAN, &removed,
+					DBUS_TYPE_INVALID)) {
+			error("Invalid hal PropertyModified parameters");
+			break;
+		}
+
+		if (g_str_equal(name, "battery.charge_level.last_full"))
+			hal_get_integer(path, name, &battchg_last);
+		else if (g_str_equal(name, "battery.charge_level.current"))
+			hal_get_integer(path, name, &battchg_cur);
+		else if (g_str_equal(name, "battery.charge_level.design"))
+			hal_get_integer(path, name, &battchg_design);
+
+		dbus_message_iter_next(&array);
+	}
+
+	return TRUE;
+}
+
+static void hal_find_device_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply;
+	DBusError err;
+	DBusMessageIter iter, sub;
+	int type;
+	const char *path;
+
+	debug("begin of hal_find_device_reply()");
+	reply = dbus_pending_call_steal_reply(call);
+
+
+	if (reply_check_error(&err, reply)) {
+		goto done;
+	}
+
+	dbus_message_iter_init(reply, &iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY) {
+		error("Unexpected signature in hal_find_device_reply()");
+		goto done;
+	}
+
+	dbus_message_iter_recurse(&iter, &sub);
+
+	type = dbus_message_iter_get_arg_type(&sub);
+
+	if (type != DBUS_TYPE_OBJECT_PATH && type != DBUS_TYPE_STRING) {
+		error("No hal device with battery capability found");
+		goto done;
+	}
+
+	dbus_message_iter_get_basic(&sub, &path);
+
+	debug("telephony-fso: found battery device at %s", path);
+
+	device_watch = g_dbus_add_signal_watch(connection, NULL, path,
+					"org.freedesktop.Hal.Device",
+					"PropertyModified",
+					handle_hal_property_modified,
+					NULL, NULL);
+
+	hal_get_integer(path, "battery.charge_level.last_full", &battchg_last);
+	hal_get_integer(path, "battery.charge_level.current", &battchg_cur);
+	hal_get_integer(path, "battery.charge_level.design", &battchg_design);
+done:
+	dbus_message_unref(reply);
+}
+
+int telephony_init(void)
+{
+	const char *battery_cap = "battery";
+	dbus_uint32_t first_index = 1;
+	int ret;
+
+	connection = dbus_bus_get(DBUS_BUS_SYSTEM, NULL);
+
+	registration_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
+					FSO_NETWORKREG_INTERFACE,
+					"Status",
+					handle_registration_property_changed,
+					NULL, NULL);
+
+	voice_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
+					FSO_GSMC_INTERFACE,
+					"CallStatus",
+					handle_gsmcall_property_changed,
+					NULL, NULL);
+
+	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+			FSO_NETWORKREG_INTERFACE,
+			"GetStatus", get_registration_reply,
+			NULL, DBUS_TYPE_INVALID);
+
+	if (ret < 0)
+		return ret;
+
+	ret = send_method_call("org.freedesktop.Hal",
+				"/org/freedesktop/Hal/Manager",
+				"org.freedesktop.Hal.Manager",
+				"FindDeviceByCapability",
+				hal_find_device_reply, NULL,
+				DBUS_TYPE_STRING, &battery_cap,
+				DBUS_TYPE_INVALID);
+	if (ret < 0)
+		return ret;
+
+	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+				FSO_SIM_INTERFACE, "RetrieveEntry",
+				retrieve_entry_reply, &subscriber_number,
+				DBUS_TYPE_STRING, &fso_categories[OWN_CATEGORY],
+				DBUS_TYPE_INT32, &first_index,
+                                DBUS_TYPE_INVALID);
+	if (ret < 0)
+		return ret;
+
+	debug("telephony_init() successfully");
+
+	return ret;
+}
+
+void telephony_exit(void)
+{
+	g_free(net.operator_name);
+	g_free(last_dialed_number);
+	g_free(subscriber_number);
+
+	g_slist_foreach(calls, (GFunc) vc_free, NULL);
+	g_slist_free(calls);
+	calls = NULL;
+
+	g_dbus_remove_watch(connection, registration_watch);
+	g_dbus_remove_watch(connection, voice_watch);
+	g_dbus_remove_watch(connection, device_watch);
+
+	dbus_connection_unref(connection);
+	connection = NULL;
+}
+
diff --git a/audio/telephony-maemo.c b/audio/telephony-maemo.c
index f7531f3..0d990d1 100644
--- a/audio/telephony-maemo.c
+++ b/audio/telephony-maemo.c
@@ -922,6 +922,16 @@ void telephony_voice_dial_req(void *telephony_device, gboolean enable)
 	telephony_voice_dial_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+        telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+        telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
 static void handle_incoming_call(DBusMessage *msg)
 {
 	const char *number, *call_path;
diff --git a/audio/telephony-ofono.c b/audio/telephony-ofono.c
index 3df76c3..6aedf12 100644
--- a/audio/telephony-ofono.c
+++ b/audio/telephony-ofono.c
@@ -387,6 +387,17 @@ void telephony_key_press_req(void *telephony_device, const char *keys)
 	telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+
 void telephony_voice_dial_req(void *telephony_device, gboolean enable)
 {
 	debug("telephony-ofono: got %s voice dial request",
diff --git a/audio/telephony.h b/audio/telephony.h
index 0bc4769..9e4c3d3 100644
--- a/audio/telephony.h
+++ b/audio/telephony.h
@@ -127,6 +127,12 @@ typedef enum {
 	CME_ERROR_NETWORK_NOT_ALLOWED	= 32,
 } cme_error_t;
 
+/* AT command types */
+#define ATNONE  0
+#define ATQUERY 1
+#define ATCHECK 2
+#define ATSET   3
+
 struct indicator {
 	const char *desc;
 	const char *range;
@@ -157,6 +163,8 @@ void telephony_call_hold_req(void *telephony_device, const char *cmd);
 void telephony_nr_and_ec_req(void *telephony_device, gboolean enable);
 void telephony_voice_dial_req(void *telephony_device, gboolean enable);
 void telephony_key_press_req(void *telephony_device, const char *keys);
+void telephony_phonebook_storage_req(void *telephony_device, const char *storage, int ATtype);
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype);
 
 /* AG responses to HF requests. These are implemented by headset.c */
 int telephony_event_reporting_rsp(void *telephony_device, cme_error_t err);
@@ -173,6 +181,8 @@ int telephony_call_hold_rsp(void *telephony_device, cme_error_t err);
 int telephony_nr_and_ec_rsp(void *telephony_device, cme_error_t err);
 int telephony_voice_dial_rsp(void *telephony_device, cme_error_t err);
 int telephony_key_press_rsp(void *telephony_device, cme_error_t err);
+int telephony_phonebook_storage_rsp(void *telephony_device, cme_error_t err);
+int telephony_phonebook_read_rsp(void *telephony_device, cme_error_t err);
 
 /* Event indications by AG. These are implemented by headset.c */
 int telephony_event_ind(int index);
@@ -188,6 +198,8 @@ int telephony_subscriber_number_ind(const char *number, int type,
 					int service);
 int telephony_call_waiting_ind(const char *number, int type);
 int telephony_operator_selection_ind(int mode, const char *oper);
+int telephony_phonebook_storage_ind(void *device, const char *storagelist);
+int telephony_phonebook_read_ind(void *device, const char *entrylist);
 
 /* Helper function for quick indicator updates */
 static inline int telephony_update_indicator(struct indicator *indicators,
-- 
1.6.4.2


^ permalink raw reply related

* [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
From: Przemo Firszt @ 2010-02-28 20:39 UTC (permalink / raw)
  To: Bastien Nocera, linux-bluetooth, marcel, Jiri Kosina,
	Peter Hutterer; +Cc: Ping

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

Hi,
I need your opinion if reporting battery condition/changing reporting
speed of a bluetooth device through sysfs is an acceptable practice.

[PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet

The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery
/sys/class/bluetooth/hci*:*/{btaddr}/battery
/sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.

The speed attribute allows to poke reporting speed of wacom tablet.
This attribute is RW, valid values are: 5 for low speed, 6 is high
speed. High speed is the default value. Using low speed is
a workaround if you experience big delay between move of stylus on
the tablet and move of cursor on screen.

--
Kind regards,
Przemo

[-- Attachment #2: 0001-Add-sysfs-battery-speed-attributes-for-wacom-bluetoo.patch --]
[-- Type: text/x-patch, Size: 5240 bytes --]

>From d61bb3eb0fd8109c235ade904a8d7184fc96b68a Mon Sep 17 00:00:00 2001
From: Przemo Firszt <przemo@firszt.eu>
Date: Sat, 20 Feb 2010 12:58:41 +0000
Subject: [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet

The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery
/sys/class/bluetooth/hci*:*/{btaddr}/battery
/sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.

The speed attribute allows to poke reporting speed of wacom tablet.
This attribute is RW, valid values are: 5 for low speed, 6 is high
speed. High speed is the default value. Using low speed is
a workaround if you experience big delay between move of stylus on
the tablet and move of cursor on screen.

Signed-off-by: Przemo Firszt <przemo@firszt.eu>
Acked-by: Ping Cheng <pingc@wacom.com>
---
 drivers/hid/hid-wacom.c |  113 +++++++++++++++++++++++++++++++++++++----------
 1 files changed, 89 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 8d3b46f..ce2782c 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -24,11 +24,86 @@
 
 #include "hid-ids.h"
 
+#define WACOM_SPEED_LO	0X05
+#define WACOM_SPEED_HI	0x06
+
 struct wacom_data {
 	__u16 tool;
 	unsigned char butstate;
+	unsigned char battery;
 };
 
+/* Battery capacity, 0 value means AC connected */
+static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 };
+
+static void wacom_poke(struct hid_device *hdev, u8 data)
+{
+	struct wacom_data *wdata = hid_get_drvdata(hdev);
+	int limit;
+	int ret;
+	char rep_data[2];
+
+	/*
+	 * Note that if the raw queries fail, it's not a hard failure and it
+	 * is safe to continue
+	 */
+	rep_data[0] = 0x03 ; rep_data[1] = 0x00;
+	limit = 3;
+	do {
+		ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
+				HID_FEATURE_REPORT);
+	} while (ret < 0 && limit-- > 0);
+	if (ret >= 0)
+		rep_data[0] = data ; rep_data[1] = 0x00;
+		limit = 3;
+		do {
+			ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
+					HID_FEATURE_REPORT);
+		} while (ret < 0 && limit-- > 0);
+	if (ret >= 0) {
+		wdata->speed = data;
+		return;
+	}
+
+	dev_warn(&hdev->dev, "failed to poke device, command %d, err %d\n",
+				data, ret);
+}
+
+static ssize_t wacom_show_battery(struct device *dev, struct device_attribute
+				*attr, char *buf)
+{
+	struct wacom_data *wdata = dev_get_drvdata(dev);
+	return snprintf(buf, PAGE_SIZE, "%i\n", batcap[wdata->battery]);
+}
+
+static DEVICE_ATTR(battery, S_IRUGO, wacom_show_battery, NULL);
+
+static ssize_t wacom_show_speed(struct device *dev,
+				struct device_attribute
+				*attr, char *buf)
+{
+	struct wacom_data *wdata = dev_get_drvdata(dev);
+	return snprintf(buf, PAGE_SIZE, "%i\n", wdata->speed);
+}
+
+static ssize_t wacom_store_speed(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	int new_speed;
+
+	sscanf(buf, "%1d", &new_speed);
+	if (new_speed == WACOM_SPEED_HI || new_speed == WACOM_SPEED_LO) {
+		wacom_poke(hdev, new_speed);
+		return strnlen(buf, PAGE_SIZE);
+	} else
+		return -EINVAL;
+}
+
+static DEVICE_ATTR(speed, S_IRUGO | S_IWUGO,
+		wacom_show_speed, wacom_store_speed);
+
 static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *raw_data, int size)
 {
@@ -147,6 +222,11 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		input_sync(input);
 	}
 
+	/* Store current battery capacity */
+	rw = (data[7] >> 2 & 0x07);
+	if (rw != wdata->battery)
+		wdata->battery = rw;
+
 	return 1;
 }
 
@@ -156,9 +236,7 @@ static int wacom_probe(struct hid_device *hdev,
 	struct hid_input *hidinput;
 	struct input_dev *input;
 	struct wacom_data *wdata;
-	char rep_data[2];
 	int ret;
-	int limit;
 
 	wdata = kzalloc(sizeof(*wdata), GFP_KERNEL);
 	if (wdata == NULL) {
@@ -181,30 +259,17 @@ static int wacom_probe(struct hid_device *hdev,
 		goto err_free;
 	}
 
-	/*
-	 * Note that if the raw queries fail, it's not a hard failure and it
-	 * is safe to continue
-	 */
+	ret = device_create_file(&hdev->dev, &dev_attr_battery);
+	if (ret)
+		dev_warn(&hdev->dev,
+			"can't create sysfs battery attribute err: %d\n", ret);
 
-	/* Set Wacom mode2 */
-	rep_data[0] = 0x03; rep_data[1] = 0x00;
-	limit = 3;
-	do {
-		ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
-				HID_FEATURE_REPORT);
-	} while (ret < 0 && limit-- > 0);
-	if (ret < 0)
-		dev_warn(&hdev->dev, "failed to poke device #1, %d\n", ret);
+	ret = device_create_file(&hdev->dev, &dev_attr_speed);
+	if (ret)
+		dev_warn(&hdev->dev,
+			"can't create sysfs speed attribute err: %d\n", ret);
 
-	/* 0x06 - high reporting speed, 0x05 - low speed */
-	rep_data[0] = 0x06; rep_data[1] = 0x00;
-	limit = 3;
-	do {
-		ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
-				HID_FEATURE_REPORT);
-	} while (ret < 0 && limit-- > 0);
-	if (ret < 0)
-		dev_warn(&hdev->dev, "failed to poke device #2, %d\n", ret);
+	wacom_poke(hdev, WACOM_SPEED_HI);
 
 	hidinput = list_entry(hdev->inputs.next, struct hid_input, list);
 	input = hidinput->input;
-- 
1.6.6.1


^ permalink raw reply related

* Re: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip
From: Suraj Sumangala @ 2010-03-01  9:53 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Vikram Kandukuri, linux-bluetooth@vger.kernel.org,
	Suraj Sumangala, Luis Rodriguez, Jothikumar Mothilal
In-Reply-To: <4B836ABE.70505@atheros.com>

Suraj Sumangala wrote:
> 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
>>
>>
>>
>>     
>
>
>   
Hi Marcel,

can you please let me know your comments regarding my answers. I can 
then have a second look at our implementation based on your comments.

Regards
Suraj

^ permalink raw reply

* Re: Kernel panic when handing Motorola S305 headset
From: Liang Bao @ 2010-03-01  9:54 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <6aeb672b1001272156m678aa8fepec2498386947936a@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

I'd like to continue the previous thread on that Motorola S305 causes
kernel panic because I did find some clue here. Sorry for misleading
guess one month ago if any.

Recap the problem here so that you don't to read the first long post.
The pattern to reproduce the issue is:
1. Pair the S305 headset from the phone or the PC( I am using a Ubuntu)
2. Remove pairing on the phone or PC
3. Power off and then power on S305.
4. The S305 will try to connect and since link key removed on this
side it will try to pair. Input 0000.
5. Kernel panic happens. This can be observed on kernel version
2.6.29(on the Droid phone, yes, it's a modified version),
2.6.31-19-generic on a Ubuntu and a pretty latest 2.6.33-020633rc8
from Ubuntu official RC release.

The exact kernel crash point is
             if (l2cap_check_security(sk)) {
                  if (bt_sk(sk)->defer_setup) {
                      struct sock *parent = bt_sk(sk)->parent;
                      rsp.result = cpu_to_le16(L2CAP_CR_PEND);
                      rsp.status = cpu_to_le16(L2CAP_CS_AUTHOR_PEND);
>>>                   parent->sk_data_ready(parent, 0)
                  } else {

After tracing the issue for a couple of weeks, I find the difference
between a normal flow and the panic one. If the user space process
accepts the L2CAP connection request before L2CAP_INFO_RSP received,
the following calls will be carried out:

l2cap_sock_accept-> bt_accept_dequeue->bt_accept_unlink(in the branch
bt_sk(parent)->defer_setup)-> set bt_sk(sk)->parent = NULL. Later when
L2CAP_INFO_RSP arrives, the l2cap_conn_start() will try to call the
marked line above and de-referring NULL happen.

To fix this, shall we consider checking if a pending socket can be
accepted in bt_accept_dequeue() prior to a pending L2CAP_INFO_REQ
responded? For example,  adding a check to BT_CONNECT2 in
af_bluetooth.c.

215         if (sk->sk_state == BT_CONNECTED || !newsock ||
216                         ( bt_sk(parent)->defer_setup &&
(sk->sk_state != BT_CONNECT2))) {

Again, I am not sure if this will bring a side-effect. Please advise
the most appropriate way. Thanks.

p.s: I attached partial trace files for those who're interested to the traces.

[-- Attachment #2: s305-good-bad-trace.txt --]
[-- Type: text/plain, Size: 1891 bytes --]

Trace when panic happended:
<3>[  356.758483] l2cap_connect_req: psm 0x19 scid 0x0041
<3>[  356.763854] l2cap_connect_req: parent c7e8f600
<3>[  356.768829] __l2cap_chan_add: conn c7fa37c0, psm 0x19, dcid 0x0041
<3>[  356.775482] l2cap_connect_req: before return, sk c5d66c00 parent c7e8f600
<3>[  356.782897] l2cap_connect_req: psm 0x01 scid 0x0042
<3>[  356.788177] l2cap_connect_req: parent c7e8fe00
<3>[  356.793121] __l2cap_chan_add: conn c7fa37c0, psm 0x01, dcid 0x0042
<3>[  356.799774] l2cap_connect_req: before return, sk c5d66a00 parent c7e8fe00
<3>[  356.837158] l2cap_sock_accept: new socket c5d66c00
<3>[  356.842590] l2cap_conn_start: BT_CONNECTE2, sk c5d66c00, parent (null)

Trace when everything goes on well.
<3>[  641.393463] l2cap_connect_req: psm 0x19 scid 0x0041
<3>[  641.398956] l2cap_connect_req: parent c7e91200
<3>[  641.403778] __l2cap_chan_add: conn c9519440, psm 0x19, dcid 0x0041
<3>[  641.410461] l2cap_connect_req: before return, sk c97e3a00 parent c7e91200
<3>[  641.417877] l2cap_connect_req: psm 0x01 scid 0x0042
<3>[  641.423156] l2cap_connect_req: parent c64b0c00
<3>[  641.428131] __l2cap_chan_add: conn c9519440, psm 0x01, dcid 0x0042
<3>[  641.434753] l2cap_connect_req: before return, sk c97e3400 parent c64b0c00
<3>[  641.476165] l2cap_conn_start: BT_CONNECTE2, sk c97e3a00, parent c7e91200
<3>[  641.483856] l2cap_sock_accept: new socket c97e3a00
<3>[  641.551513] l2cap_sock_accept: new socket c97e3400
<3>[  641.592742] l2cap_sock_shutdown: sock cea7a840, sk c97e3a00, how 2
<3>[  641.607727] l2cap_sock_shutdown: sock cea7a0c0, sk c97e3400, how 2
<3>[  644.055419] __l2cap_chan_add: conn c9519440, psm 0x01, dcid 0x0000
<3>[  650.055816] l2cap_sock_shutdown: sock cea7a0c0, sk c97e3400, how 2
<3>[  650.063018] l2cap_sock_shutdown: sk-shutdown = true
<3>[  650.069427] __l2cap_sock_close: sk c97e3400 state 1 socket cea7a0c0 reason 0


^ permalink raw reply

* [PATCH 1/4] Do not automatically remove watches for service names
From: Luiz Augusto von Dentz @ 2010-03-01 16:07 UTC (permalink / raw)
  To: linux-bluetooth, ofono

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);
 	}

-- 
Luiz Augusto von Dentz
Computer Engineer

^ permalink raw reply related

* [PATCH 2/4] Fix signal watch when a service name is given
From: Luiz Augusto von Dentz @ 2010-03-01 16:08 UTC (permalink / raw)
  To: linux-bluetooth, ofono

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


-- 
Luiz Augusto von Dentz
Computer Engineer

^ permalink raw reply related

* [PATCH 3/4] Fix calling watch callbacks after it has been removed
From: Luiz Augusto von Dentz @ 2010-03-01 16:09 UTC (permalink / raw)
  To: linux-bluetooth, ofono

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

-- 
Luiz Augusto von Dentz
Computer Engineer

^ permalink raw reply related

* Re: Phonebook functions for BlueZ
From: Johan Hedberg @ 2010-03-01 19:20 UTC (permalink / raw)
  To: Felix Huber; +Cc: linux-bluetooth
In-Reply-To: <4B8A3EEA.3030901@schyf.de>

Hi Felix,

On Sun, Feb 28, 2010, Felix Huber wrote:
> I have analyzed the data traffic of my car handsfree set and implemented
> the functions for the phonebook retrieval via the CPBR and CPBS
> commands. In addition, I added a telephony driver for the FSO middleware
> with which I tested the new functions using a Parrot CK3100 car kit and
> a Jabra ear plug using version 4.61 of BlueZ. The other telephony
> drivers just have empty functions that reject the new commands in order
> not to break the APIs.
> 
> Attached is the patch file for the git repository.

Thanks for this contribution! In general the patch looks quite ok,
however before pushing this upstream there are a some coding style
issues that'd need to be fixed:

> +int get_ATtype(const char *buf, int *offset)
> +{
> +	const char *ATquery = "=?", *ATcheck ="?", *ATset = "=";

We usually don't use capital letters in any variable or function names.
Pre-processor defines are an exception. So use something like
get_at_type, at_query, etc. Also, since get_ATtype is only used within
headset.c you have to declare it static.

> +	if (!strncmp(buf, ATquery, 2)) {
> +		*offset = 2;
> +		return (ATQUERY);

No () around the value given to return;

> +	} else if (!strncmp(buf, ATcheck, 1)) {
> +		*offset = 1;
> +		return (ATCHECK);
> +	} else if (!strncmp(buf, ATset, 1)) {
> +		*offset = 1;
> +		return (ATSET);
> +	} else {
> +		*offset = 0;
> +		return (ATNONE);
> +	}
> +}
> +
> +

There shouldn't at any point be the need to have more than one empty
line in the code, so please remove the other.

> +int telephony_phonebook_storage_ind(void *telephony_device, const char *storagelist)

This line looks like it's longer than 80 columns. Please split it to
two.

> +	headset_send(hs, "\r\n+CPBS: %s \r\n", storagelist);

Why is there a space before the "\r\n"?

> +	int ATtype, offset;

Again the capital letter thing.

> +	if (strlen(buf) < 8) {
> +		return -EINVAL;
> +	}

No braces for single-line scopes.

> +	ATtype = get_ATtype(&buf[7], &offset);

No capital letters.

> +	if (strlen(buf) < 8) {
> +		return -EINVAL;
> +	}

No braces.

> +	ATtype = get_ATtype(&buf[7], &offset);
> +	telephony_phonebook_read_req(device, &buf[7+offset], ATtype);
> +
> +	return 0;
> +}
> +
> +

Unnecessary extra empty line.

> +void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)

Looks like a > 80 column line.

> +void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)

Same issue.

> +{
> +	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
> +}
> +
> +

Extra empty line again.

> +char *fso_categories[NUM_CATEGORIES] = {"contacts", "emergency", "fixed", "dialed", "received", "missed", "own"};
> +char *gsm_categories[NUM_CATEGORIES] = {"\"SM\"",   "\"EN\"",    "\"FD\"","\"DC\"", "\"RC\"",   "\"MC\"", "\"ON\""};

Probably you could split these to fit within 80 columns.

> +static struct {
> +	uint8_t status;         // act  'GSM'
> +	uint32_t cell_id;       // cid  '51FD'
> +	uint32_t operator_code; // code '26203'
> +	uint16_t lac;           // lac  '233b' 

We don't use C++ style comments. So please change to /* .. */. Also,
there's an extra space at the end of the "lac" line.

> +} net = {
> +	.status = NETWORK_REG_STATUS_UNREGISTERED,
> +	.cell_id = 0,
> +	.operator_code = 0,
> +	.lac = 0,
> +	.mode = NULL,
> +	.operator_name = NULL,
> +	.registration = NULL,
> +	.signals_bar = 0,
> +};
> +
> +

Extra empty line.

> +static void vc_free(struct voice_call *vc)
> +{
> +	if (!vc)
> +		return;
> +	g_free(vc->number);
> +	g_free(vc);
> +}
> +
> +

Extra empty line.

> +static int find_category(char **phonebooks, const char *category)
> +{
> +	int i;
> +	for (i=0; i < NUM_CATEGORIES; i++) {

Space before and after '='.

> +		if (!strcmp(phonebooks[i], category))

The convention has usually been to use == 0 in the case of strcmp for
readability.

> +			return i;
> +	}
> +	return -1;
> +
> +}

Why the empty line after the return statement? I'd put it after the
for-loop for consistency with the rest of the code base.

> +
> +

Extra empty line.

> +static int send_method_call(const char *dest, const char *path,
> +				const char *interface, const char *method,
> +				DBusPendingCallNotifyFunction cb,
> +				void *user_data, int type, ...)

Since this and many other functions are shared with (or actually copied
from) telephony-maemo.c would it make sense to put them to some shared
c-file (it's fine if this is a separate commit later).

> +	if ((vc = find_vc_with_status(CALL_STATUS_ACTIVE))) {
> +	} else if ((vc = find_vc_with_status(CALL_STATUS_DIALING))) {
> +	} else if ((vc = find_vc_with_status(CALL_STATUS_INCOMING))) {
> +	}

The purpose of this construction isn't imediately clear imo. Wouldn't
something like doing specific NULL checks after each find() call be more
readable? I.e.

vc = find(...);
if (vc == NULL)
	vc = find(...);
if (vc == NULL)
	vc = find(...);

Alternatively creating something like find_active_call() which would
match any of those states could also make the code a bit more readable.

> +	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
> +					FSO_GSMC_INTERFACE,
> +					"Release", NULL, NULL,
> +					DBUS_TYPE_INT32, &vc->call_index,
> +					DBUS_TYPE_INVALID);
> +
> +

Extra empty line.

> +void telephony_dial_number_req(void *telephony_device, const char *number)
> +{
> +	const char *clir, *callType = "voice";

No capital letters in variable names.

> +#if 0
> +	if (!strncmp(number, "*31#", 4)) {
> +		number += 4;
> +		clir = "enabled";
> +	} else if (!strncmp(number, "#31#", 4)) {
> +		number += 4;
> +		clir =  "disabled";
> +	} else
> +		clir = "default";
> +#endif

Is this really code that you think can be enabled later? If not I'd just
remove it instead of having it commented out.

> +	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
> +                        FSO_GSMC_INTERFACE,
> +			"SendDtmf", NULL, NULL,
> +			DBUS_TYPE_STRING, &tone_string,
> +			DBUS_TYPE_INVALID);

Looks like the indentation is somehow screwed up in the first
continuation line. In general the rule for indenting continuation lines
is: same indentation for all continuation lines, at least two tabs more
than the first line and as much as possible as long as the lines stay
less than 80 columns.

> +	if (vc = find_vc_with_status(CALL_STATUS_INCOMING)) {
> +		cmd = act;
> +	} else if (vc = find_vc_with_status(CALL_STATUS_ACTIVE)) {
> +		cmd = rel;
> +	} else if (vc = find_vc_with_status(CALL_STATUS_DIALING)) {
> +		cmd = rel;
> +	}

No braces for one-line scopes.

> +	if (cmd) {
> +		err = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
> +					FSO_GSMC_INTERFACE,
> +					cmd, NULL, NULL,
> +					DBUS_TYPE_INT32, &vc->call_index,
> +					DBUS_TYPE_INVALID);
> +	}
> +	if (err < 0)
> +		telephony_key_press_rsp(telephony_device, CME_ERROR_AG_FAILURE);
> +        else
> +		telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);

Shouldn't it be an error if cmd is NULL? In general doing
initializations upon declaration, especially for error variables, should
be avoided. Would e.g. having a final "else err = -EINVAL" at the end of
the else/else if statement make sense (which would allow removing the
initialzation of err to 0?

Also the indentation of the second if-statments else line is with spaces
instead of tabs.

> +void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)

Looks like a possibly grater than 80 columns line.

> +void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
> +{
> +	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
> +}
> +
> +

Extra empty line.

> +	if (!dbus_message_get_args(reply, NULL,
> +				DBUS_TYPE_STRING, &name,
> +				DBUS_TYPE_STRING, &number,
> +				DBUS_TYPE_INVALID)) 

Extra space at the end of the last line.

> +	if (number_type == &subscriber_number) {
> +		g_free(subscriber_number);
> +		subscriber_number = g_strdup(number);
> +	}
> +
> +done:
> +	dbus_message_unref(reply);
> +}
> +
> +

Extra empty line.

> +static void retrieve_phonebook_reply(DBusPendingCall *call, void *user_data)
> +{
> +	DBusError err;
> +	DBusMessage *reply;
> +	DBusMessageIter iter, array;
> +	int ret = 0;

Instead of initializing ret upon declaration (and btw, we use "err"
instead of "ret" usually) you could set it to 0 right before the done
label.
point:

> +		if ((index >= phonebook_info.first) && (index <= phonebook_info.last)) {
> +			info = g_strdup_printf("%d,\"%s\",,\"%s\"", index, number, name);
> +			telephony_phonebook_read_ind(phonebook_info.telephony_device, info);

Looks like possibly grater than 80 columns lines.

> +	if (ret)
> +		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
> +	else
> +		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);

Same here.


> +}
> +
> +
> +

Two extra empty lines.

> +static void get_phonebook_info_reply(DBusPendingCall *call, void *user_data)
> +{
> +	DBusError err;
> +	DBusMessage *reply;
> +	DBusMessageIter iter, iter_entry, array;
> +	int ret = 0;

s/ret/err/ and try to rethink how the initialization upon declaration
could be avoided.

> +	int min_index=-1, max_index =-1, number_length = -1, name_length = -1;

Missing spaces before and after '='.

> +		if (g_str_equal(property, "min_index")) {
> +			dbus_message_iter_get_basic(&sub, &min_index);
> +		} else if (g_str_equal(property, "max_index")) {
> +			dbus_message_iter_get_basic(&sub, &max_index);
> +		} else if (g_str_equal(property, "number_length")) {
> +			dbus_message_iter_get_basic(&sub, &number_length);
> +		} else if (g_str_equal(property, "name_length")) {
> +			dbus_message_iter_get_basic(&sub, &name_length);
> +		}

No braces for one-line scopes.

> +	if (ret)
> +		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
> +	else
> +		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);

Possibly longer than 80-column lines.

 +	g_free(str);
> +	telephony_phonebook_storage_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);
> +
> +done:
> +	dbus_message_unref(reply);
> +
> +}
> +
> +
> +

Two unnecessary extra empty lines.

> +	int ret = 0;

Same thing as already mentioned.

> +	gstr = g_string_new("(");
> +	for (i=0; i< n_s; i++) {

Missing spaces before and after '=', before '<'. Can you come up with a
more descriptive name for the n_s variable please?

> +		index = find_category(fso_categories, s[i]);
> +		if (index != -1) {
> +			debug("GSM %d: %s", index, gsm_categories[index]);
> +			if (i == 0)
> +				g_string_append_printf(gstr, "%s", gsm_categories[index]);
> +			else
> +				g_string_append_printf(gstr, ",%s", gsm_categories[index]);
> +		}
> +	}

Looks like some lines go beyone 80-colums. You can avoid this by doing

if (index == -1)
	continue;

which also (imho) makes the code more readable.

> +done:
> +	dbus_message_unref(reply);
> +	if (ret)
> +		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
> +	else
> +		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);

Looks like greater than 80-column lines again.

> +}
> +
> +
> +

Two extra empty lines.

> +void telephony_phonebook_storage_req(void *telephony_device, const char *storage, int ATtype)

Greater than 80-column line. No capital letters in variable names.

> +	int ret=0, index;

s/ret/err/, rethink how the initialization upon declaration could be
avoided (not to mention the missing spaces before and after '=').

> +	switch (ATtype) {
> +	case ATQUERY:      // =?
> +		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
> +			FSO_SIM_INTERFACE,
> +			"ListPhonebooks",
> +			list_phonebooks_reply, NULL,
> +			DBUS_TYPE_INVALID);

No C++ comments, the spit continuation lines need more indentation (at
least two more than the original line).

> +	case ATSET:         // =
> +		debug("Phonebook request to be %s", storage);
> +		index = find_category(gsm_categories, storage);
> +		debug ("Phonebook found at %d", index);

No space between debug and (

> +		if (index == -1)
> +			ret = -1;
> +		else {
> +			phonebook_info.category = index;
> +			telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NONE);

Looks like greater than 80-column line.

> +		}
> +		break;
> +	default:
> +		ret = -1;
> +	}
> +	

Redundant tab-character on this empty line.

> +	if (ret < 0)
> +		telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_AG_FAILURE);

Greater than 80-column line.

> +}
> +
> +

Unnecessary empty line.

> +void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)

Greater than 80 column line.

> +{
> +	int size, ret=0;
> +	debug("telephony-fso: got pbook read request: %s", readindex);

Empty line after variable declarations. s/ret/err/. Rethink
initialization upon declaration. Missing space before and after '='

> +	switch (ATtype) {
> +	case ATQUERY:          // =?

No C++ comments.

> +		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
> +		FSO_SIM_INTERFACE,
> +			"GetPhonebookInfo",
> +			get_phonebook_info_reply, NULL,
> +			DBUS_TYPE_STRING, &fso_categories[phonebook_info.category],
> +			DBUS_TYPE_INVALID);

Incorrect indentation (e.g. first continuation line seems not to be
indented at all from the original line)

> +		phonebook_info.first=-1, phonebook_info.last=-1;

Missing spaces before and after '='.

> +		sscanf(readindex, "%d,%d", &phonebook_info.first, &phonebook_info.last);
> +		if (phonebook_info.first == -1)
> +			break;

Probably you'd also want to check for sscanf return value (i.e. == 2).
Maybe that's the only thing you should check for and not try to
initialize these variables here since you already do that in the
beginning of telephony-fso.c where you define the phonebook_info struct?

> +
> +		if (phonebook_info.last == -1) phonebook_info.last = phonebook_info.first;

Break this into two lines.

> +
> +		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
> +			FSO_SIM_INTERFACE,
> +			"RetrievePhonebook",
> +			retrieve_phonebook_reply, NULL,
> +			DBUS_TYPE_STRING, &fso_categories[phonebook_info.category],
> +			DBUS_TYPE_INT32, &phonebook_info.first,
> +			DBUS_TYPE_INT32, &phonebook_info.last,
> +			DBUS_TYPE_INVALID);
> +		break;
> +
> +	default:
> +		ret = -1;
> +	}
> +
> +	if (ret < 0)
> +		telephony_phonebook_read_rsp(telephony_device, CME_ERROR_AG_FAILURE);
> +
> +}
> +
> +
> +

Unnecessary empty line at the end of the function and two unnecessary
empty lines after it.

> +static void parse_gsmcall_property(const char *property, DBusMessageIter sub, struct voice_call *vc)

Too long line.

> +	if (g_str_equal(property, "direction")) {
> +		dbus_message_iter_get_basic(&sub, &direction);
> +	} else if (g_str_equal(property, "peer")) {
> +		dbus_message_iter_get_basic(&sub, &peer);
> +		vc->number = g_strdup(peer);
> +	} else if (g_str_equal(property, "reason")) {
> +		dbus_message_iter_get_basic(&sub, &reason);
> +	} else if (g_str_equal(property, "auxstatus")) {
> +		dbus_message_iter_get_basic(&sub, &auxstatus);
> +	} else if (g_str_equal(property, "line")) {
> +		dbus_message_iter_get_basic(&sub, &line);
> +	}

No braces for one-line scopes.

> +}
> +
> +

Unnecessary empty line.

> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING) {
> +	error("Unexpected signature in gsmcall PropertyChanged signal");
> +		return TRUE;
> +	}

Incorrect indentation and no braces needed for one-line scope.

> +	dbus_message_iter_get_basic(&iter, &status);
> +
> +	debug("**** gsmProp Changed id:%d status: %s", call_index, status);
> +
> +

Unnecessary empty line.

> +	vc = find_vc(call_index);
> +	if (!vc) {
> +		vc = g_new0(struct voice_call, 1);
> +	if (!vc)
> +		return TRUE;

g_new0 is guaranteed to succeed or else it will call abort() so the NULL
check is redundant.

> +	vc->call_index = call_index;
> +	calls = g_slist_append(calls, vc);
> +	}

Looks like some incorrect indentation is going on here (since the ending
brace has the same indentation as the preceeding lines.

> +		if (dbus_message_iter_get_arg_type(&iter_property)
> +			!= DBUS_TYPE_STRING) {
> +			error("Unexpected signature in gsmcallPropertyChanged signal");
> +			return TRUE;
> +		}

Incorrect indentation for the split if-line. The continuation line
should be indented by at least two tabs to clearly separate it from the
actual branch code.

> +	return TRUE;
> +}
> +
> +
> +
> +

Three unnecessary empty lines.

> +}
> +
> +

Unnecessary empty line.

> +	/* ARRAY -> ENTRY -> VARIANT*/

Space before */

> +		dbus_message_iter_recurse(&iter_property, &sub);
> +
> +		parse_registration_property(property, sub);
> +
> +                dbus_message_iter_next(&iter_entry);

Incorrect indentation for the last line.

> +	}
> +	
> +done:

Redundant tab character on the empty line.

> +	return TRUE;
> +}
> +
> +

Unnecessary empty line.

> +done:
> +	dbus_message_unref(reply);
> +}
> +
> +

Unnecessary empty line.

> +static gboolean handle_registration_property_changed(DBusConnection *conn,
> +						DBusMessage *msg, void *data)
> +{
> +	return (handle_registration_property(msg));
> +}

No () needed around the value given to return.


> +	if (reply_check_error(&err, reply)) {
> +		goto done;
> +	}

No braces for one-line scope (though I realize this one is probably
inherited from telephony-maemo.c which I'm responsible for :)

> --- a/audio/telephony.h
> +++ b/audio/telephony.h
> @@ -127,6 +127,12 @@ typedef enum {
>  	CME_ERROR_NETWORK_NOT_ALLOWED	= 32,
>  } cme_error_t;
>  
> +/* AT command types */
> +#define ATNONE  0
> +#define ATQUERY 1
> +#define ATCHECK 2
> +#define ATSET   3

These should probably be an enum and have some namespacing (e.g.
AT_TYPE_). To be consistent with the AT command spec terminology (I've
been looking at 3GPP TS 07.07) it should be s/QUERY/TEST/ and
s/CHECK/READ/.

There were probably other issues in the patch that I missed but it'd be
nice if you could fix the already mentioned problems first and then I'll
take a second look.

Johan

^ permalink raw reply

* [PATCH] drivers: bluetooth: hci_ldisc: Fixed multiple coding style issues
From: Romain Guillebert @ 2010-03-01 20:33 UTC (permalink / raw)
  To: alan; +Cc: linux-bluetooth, Romain Guillebert

Fixes many coding style issues in this file

Signed-off-by: Romain Guillebert <romain.py@gmail.com>
---
 drivers/bluetooth/hci_ldisc.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index aa09193..6aa1c25 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -48,7 +48,7 @@
 
 #define VERSION "2.2"
 
-static int reset = 0;
+static int reset;
 
 static struct hci_uart_proto *hup[HCI_UART_MAX_PROTO];
 
@@ -244,12 +244,12 @@ static void hci_uart_destruct(struct hci_dev *hdev)
 
 /* ------ LDISC part ------ */
 /* hci_uart_tty_open
- * 
+ *
  *     Called when line discipline changed to HCI_UART.
  *
  * Arguments:
  *     tty    pointer to tty info structure
- * Return Value:    
+ * Return Value:
  *     0 if success, otherwise error code
  */
 static int hci_uart_tty_open(struct tty_struct *tty)
@@ -261,7 +261,8 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	if (hu)
 		return -EEXIST;
 
-	if (!(hu = kzalloc(sizeof(struct hci_uart), GFP_KERNEL))) {
+	hu = kzalloc(sizeof(struct hci_uart), GFP_KERNEL);
+	if (!hu) {
 		BT_ERR("Can't allocate control structure");
 		return -ENFILE;
 	}
@@ -339,15 +340,15 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
 }
 
 /* hci_uart_tty_receive()
- * 
+ *
  *     Called by tty low level driver when receive data is
  *     available.
- *     
+ *
  * Arguments:  tty          pointer to tty isntance data
  *             data         pointer to received data
  *             flags        pointer to flags for data
  *             count        count of received data in bytes
- *     
+ *
  * Return Value:    None
  */
 static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data, char *flags, int count)
@@ -515,7 +516,7 @@ static int __init hci_uart_init(void)
 
 	/* Register the tty discipline */
 
-	memset(&hci_uart_ldisc, 0, sizeof (hci_uart_ldisc));
+	memset(&hci_uart_ldisc, 0, sizeof(hci_uart_ldisc));
 	hci_uart_ldisc.magic		= TTY_LDISC_MAGIC;
 	hci_uart_ldisc.name		= "n_hci";
 	hci_uart_ldisc.open		= hci_uart_tty_open;
@@ -528,7 +529,8 @@ static int __init hci_uart_init(void)
 	hci_uart_ldisc.write_wakeup	= hci_uart_tty_wakeup;
 	hci_uart_ldisc.owner		= THIS_MODULE;
 
-	if ((err = tty_register_ldisc(N_HCI, &hci_uart_ldisc))) {
+	err = tty_register_ldisc(N_HCI, &hci_uart_ldisc);
+	if (err) {
 		BT_ERR("HCI line discipline registration failed. (%d)", err);
 		return err;
 	}
@@ -561,7 +563,8 @@ static void __exit hci_uart_exit(void)
 #endif
 
 	/* Release tty registration of line discipline */
-	if ((err = tty_unregister_ldisc(N_HCI)))
+	err = tty_unregister_ldisc(N_HCI);
+	if (err)
 		BT_ERR("Can't unregister HCI line discipline (%d)", err);
 }
 
-- 
1.7.0


^ permalink raw reply related

* Re: linux-next: Tree for March 1 (bluetooth/hci_sysfs)
From: Randy Dunlap @ 2010-03-01 21:08 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: linux-next, LKML, Marcel Holtmann, linux-bluetooth, Netdev
In-Reply-To: <20100301210210.c6f465e6.sfr@canb.auug.org.au>

On 03/01/10 02:02, Stephen Rothwell wrote:
> Hi all,
> 
> Changes since 20100226:
> 
> We are seeing conflicts migrate from being between 2 trees in linux-next
> to be between a tree and Linus' tree as things start to get merged.
> 
> 
> The net tree gained a conflict against Linus' tree.

static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
						size_t count, loff_t *ppos)
{
	struct hci_dev *hdev = file->private_data;
	struct inquiry_cache *cache = &hdev->inq_cache;
	struct inquiry_entry *e;
	char buf[4096]; // <<<<<<<<<<<<<<<<<<<<<<<<<<< huh? don't do that on stack.
	int n = 0;


-- 
~Randy

^ permalink raw reply

* Re: linux-next: Tree for March 1 (bluetooth/hci_sysfs)
From: Stephen Rothwell @ 2010-03-02  1:15 UTC (permalink / raw)
  To: Randy Dunlap; +Cc: linux-next, LKML, Marcel Holtmann, linux-bluetooth, Netdev
In-Reply-To: <4B8C2CC5.8000603@oracle.com>

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]

Hi Randy,

On Mon, 01 Mar 2010 13:08:21 -0800 Randy Dunlap <randy.dunlap@oracle.com> wrote:
>
> static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
> 						size_t count, loff_t *ppos)
> {
> 	struct hci_dev *hdev = file->private_data;
> 	struct inquiry_cache *cache = &hdev->inq_cache;
> 	struct inquiry_entry *e;
> 	char buf[4096]; // <<<<<<<<<<<<<<<<<<<<<<<<<<< huh? don't do that on stack.
> 	int n = 0;

Dave Miller is following up on that.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply

* Re: linux-next: Tree for March 1 (bluetooth/hci_sysfs)
From: David Miller @ 2010-03-02  2:14 UTC (permalink / raw)
  To: sfr; +Cc: randy.dunlap, linux-next, linux-kernel, marcel, linux-bluetooth,
	netdev
In-Reply-To: <20100302121526.36a1ea88.sfr@canb.auug.org.au>

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 2 Mar 2010 12:15:26 +1100

> Hi Randy,
> 
> On Mon, 01 Mar 2010 13:08:21 -0800 Randy Dunlap <randy.dunlap@oracle.com> wrote:
>>
>> static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
>> 						size_t count, loff_t *ppos)
>> {
>> 	struct hci_dev *hdev = file->private_data;
>> 	struct inquiry_cache *cache = &hdev->inq_cache;
>> 	struct inquiry_entry *e;
>> 	char buf[4096]; // <<<<<<<<<<<<<<<<<<<<<<<<<<< huh? don't do that on stack.
>> 	int n = 0;
> 
> Dave Miller is following up on that.

This looks like a job for.... SEQ FILE! :-)

I'm testing the following fix.

diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 1a79a6c..f02c2ff 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -3,6 +3,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -405,44 +406,85 @@ static struct device_type bt_host = {
 	.release = bt_host_release,
 };
 
-static int inquiry_cache_open(struct inode *inode, struct file *file)
+static int inquiry_cache_show(struct seq_file *m, void *v)
 {
-	file->private_data = inode->i_private;
+	struct inquiry_entry *e = v;
+	struct inquiry_data *data;
+	bdaddr_t bdaddr;
+
+	data = &e->data;
+	baswap(&bdaddr, &data->bdaddr);
+
+	seq_printf(m, "%s %d %d %d 0x%.2x%.2x%.2x 0x%.4x %d %d %u\n",
+		   batostr(&bdaddr),
+		   data->pscan_rep_mode, data->pscan_period_mode,
+		   data->pscan_mode, data->dev_class[2],
+		   data->dev_class[1], data->dev_class[0],
+		   __le16_to_cpu(data->clock_offset),
+		   data->rssi, data->ssp_mode, e->timestamp);
+
 	return 0;
 }
 
-static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
-						size_t count, loff_t *ppos)
+static void *inquiry_cache_get_idx(struct hci_dev *hdev, loff_t pos)
 {
-	struct hci_dev *hdev = file->private_data;
 	struct inquiry_cache *cache = &hdev->inq_cache;
-	struct inquiry_entry *e;
-	char buf[4096];
-	int n = 0;
+	struct inquiry_entry *e = cache->list;
+
+	while (e && pos) {
+		e = e->next;
+		pos--;
+	}
+	return e;
+}
+
+static void *inquiry_cache_start(struct seq_file *m, loff_t *pos)
+{
+	struct hci_dev *hdev = m->private;
 
 	hci_dev_lock_bh(hdev);
 
-	for (e = cache->list; e; e = e->next) {
-		struct inquiry_data *data = &e->data;
-		bdaddr_t bdaddr;
-		baswap(&bdaddr, &data->bdaddr);
-		n += sprintf(buf + n, "%s %d %d %d 0x%.2x%.2x%.2x 0x%.4x %d %d %u\n",
-				batostr(&bdaddr),
-				data->pscan_rep_mode, data->pscan_period_mode,
-				data->pscan_mode, data->dev_class[2],
-				data->dev_class[1], data->dev_class[0],
-				__le16_to_cpu(data->clock_offset),
-				data->rssi, data->ssp_mode, e->timestamp);
-	}
+	return inquiry_cache_get_idx(hdev, *pos);
+}
+
+static void *inquiry_cache_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	struct inquiry_entry *e = v;
+
+	++*pos;
+	return e->next;
+}
+
+static void inquiry_cache_stop(struct seq_file *m, void *v)
+{
+	struct hci_dev *hdev = m->private;
 
 	hci_dev_unlock_bh(hdev);
+}
 
-	return simple_read_from_buffer(userbuf, count, ppos, buf, n);
+static const struct seq_operations inquiry_cache_ops = {
+	.start	= inquiry_cache_start,
+	.next	= inquiry_cache_next,
+	.stop	= inquiry_cache_stop,
+	.show	= inquiry_cache_show,
+};
+
+static int inquiry_cache_open(struct inode *inode, struct file *file)
+{
+	int rc = seq_open(file, &inquiry_cache_ops);
+
+	if (rc >= 0) {
+		struct seq_file *m = file->private_data;
+		m->private = inode->i_private;
+	}
+	return rc;
 }
 
 static const struct file_operations inquiry_cache_fops = {
-	.open = inquiry_cache_open,
-	.read = inquiry_cache_read,
+	.open		= inquiry_cache_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
 };
 
 int hci_register_sysfs(struct hci_dev *hdev)

^ permalink raw reply related

* Re: linux-next: Tree for March 1 (bluetooth/hci_sysfs)
From: Marcel Holtmann @ 2010-03-02  7:06 UTC (permalink / raw)
  To: David Miller
  Cc: sfr, randy.dunlap, linux-next, linux-kernel, linux-bluetooth,
	netdev
In-Reply-To: <20100301.181415.266447389.davem@davemloft.net>

Hi Dave,

> >> static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
> >> 						size_t count, loff_t *ppos)
> >> {
> >> 	struct hci_dev *hdev = file->private_data;
> >> 	struct inquiry_cache *cache = &hdev->inq_cache;
> >> 	struct inquiry_entry *e;
> >> 	char buf[4096]; // <<<<<<<<<<<<<<<<<<<<<<<<<<< huh? don't do that on stack.
> >> 	int n = 0;
> > 
> > Dave Miller is following up on that.
> 
> This looks like a job for.... SEQ FILE! :-)
> 
> I'm testing the following fix.

the patch looks good, but it doesn't do the job. I have to look into why
that is. Will try to come up with a proper patch soon.

Regards

Marcel



^ permalink raw reply

* Re: [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
From: Bastien Nocera @ 2010-03-02 12:02 UTC (permalink / raw)
  To: Przemo Firszt; +Cc: linux-bluetooth, marcel, Jiri Kosina, Peter Hutterer, Ping
In-Reply-To: <1267389553.24791.22.camel@pldmachine>

Hey Przemo,

On Sun, 2010-02-28 at 20:39 +0000, Przemo Firszt wrote:
> Hi,
> I need your opinion if reporting battery condition/changing reporting
> speed of a bluetooth device through sysfs is an acceptable practice.
> 
> [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
> 
> The patch creates 2 sysfs attributes:
> The battery attribute is read-only and it appears in:
> /sys/bus/hid/devices/{btaddr}/battery
> /sys/class/bluetooth/hci*:*/{btaddr}/battery
> /sys/class/hidraw/hidraw*/device/battery
> Capacity values are in %, zero value means AC plug is connected.

A couple of comments:
- battery status and adapter status should probably be 2 separate
values. I don't know what the hardware actually exports, but I hope to
have access to the specs of the device soon, which should answer that
question.
- isn't there a more kernel-y way to export that data, so that it's
automatically picked up by things like upower (né DeviceKit-power)?

> The speed attribute allows to poke reporting speed of wacom tablet.
> This attribute is RW, valid values are: 5 for low speed, 6 is high
> speed. High speed is the default value. Using low speed is
> a workaround if you experience big delay between move of stylus on
> the tablet and move of cursor on screen.

5 and 6 are magic values. I'd rather have a boolean "high_speed"
attribute which would switch between high-speed and low-speed.

Finally, the patch should be split in at least 2 parts, one to add the
sysfs infrastructure and one of the properties, and another one adding
the property itself (maybe even 3, if we were to split the sysfs
enablement from the properties themselves).

But that's a minor problem, and if the maintainers are happy taking a
single patch for the features, then I'm happy as well :)

Cheers

^ permalink raw reply

* Re: [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
From: Bastien Nocera @ 2010-03-02 12:11 UTC (permalink / raw)
  To: Przemo Firszt; +Cc: linux-bluetooth, marcel, Jiri Kosina, Peter Hutterer, Ping
In-Reply-To: <1267531337.23521.14275.camel@localhost.localdomain>

On Tue, 2010-03-02 at 12:02 +0000, Bastien Nocera wrote:
> Hey Przemo,
> 
> On Sun, 2010-02-28 at 20:39 +0000, Przemo Firszt wrote:
> > Hi,
> > I need your opinion if reporting battery condition/changing reporting
> > speed of a bluetooth device through sysfs is an acceptable practice.
> > 
> > [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
> > 
> > The patch creates 2 sysfs attributes:
> > The battery attribute is read-only and it appears in:
> > /sys/bus/hid/devices/{btaddr}/battery
> > /sys/class/bluetooth/hci*:*/{btaddr}/battery
> > /sys/class/hidraw/hidraw*/device/battery
> > Capacity values are in %, zero value means AC plug is connected.
> 
> A couple of comments:
<snip>
> - isn't there a more kernel-y way to export that data, so that it's
> automatically picked up by things like upower (né DeviceKit-power)?

I've been told it should use the power_supply class, so it would work
pretty much out-of-the-box with things like upower and
gnome-power-manager.

Cheers

^ permalink raw reply

* question about 'read' system call of sco socket
From: liu yang @ 2010-03-02 14:28 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Tan Miaoqing, Valerio Valerio

Hi experts,
I have one question about 'read' system call of sco socket. I'm using
an open source  library which implements bluetooth handfree profile on
linux platform. About our use case, cell phone works as audio gateway
that all audio received on cell phone will be streamed to linux PC,
and vice versa. Everything works just fine of audio streaming above
bluetooth channel. But our special application is very sensitive on
timing that we need send/receive bluetooth audio frame every 20ms.
Regarding sending direction, I can tweak MTU of bluetooth donglel
installed on my linux PC. Let's say, MTU is set to 320 bytes, so every
20ms 'send' system call only needs be called one time. ( the audio
frame conveyed aboved bluetooth is of signed linear 16bit per sample,
8K sampling-rate )  But this does not affect receiving direction that
every 'read' system call on sco socket returns exact 48 bytes. Also I
find such 'read' event on linux 'poll' is triggered every 10ms. Every
10ms, such 'read' action is invoked, but it takes 3 or 4 times
invokation ( sometimes 3 , sometimes 4) of  'read' to consume this
'read' event, which results in 'EAGAIN'. So every 20ms, I get 288
(48*6) or 336 (48*7)  bytes, not exact 320 I expect.

Shortly, my question is how to receive ( 'read' system call ) exact
320 bytes every 20ms.

Thanks

Kandy

^ permalink raw reply

* Bluetooth SPP on Nokia N900 (Maemo 5)
From: Henry Owens @ 2010-03-02 16:26 UTC (permalink / raw)
  To: linux-bluetooth

Hi All,

My first mail to this list. I am currently trying to get a bluetooth
SPP connection going on my Nokia N900, but I'm having some
difficulties using the examples on the HOWTO Wiki.

Specifically, I get an error related to the ActivateService call in
the example - I have seen reference to this being obselete in Bluez
4.x, is this the problem in the example code?

If so, how could the code be rewritten to allow a connection (I have
no prior experience with BT programming, but am comfortable in
Python).

The code at the moment is:

import dbus
import time
import sys
bus = dbus.SystemBus()
bmgr = dbus.Interface(bus.get_object('org.bluez', '/org/bluez'),
'org.bluez.Manager')
bus_id = bmgr.ActivateService('serial')
serial = dbus.Interface(bus.get_object(bus_id, '/org/bluez/serial'),
'org.bluez.serial.Manager')
# Service connection, read the serial API to check the available patterns
if (len(sys.argv) < 2):
        print "Usage: %s <address> [service]" % (sys.argv[0])
        sys.exit(1)
address = sys.argv[1]
if (len(sys.argv) < 3):
        service = "spp"
else:
        service = sys.argv[2]
# Bind to the default local adapter
device = serial.ConnectService(address, service)
print "Connected %s to %s" % (device, address)
print "Press CTRL-C to disconnect"
try:
        time.sleep(1000)
        print "Terminating connection"
except:
        pass
serial.DisconnectService(device)


Regards,
Henry.

^ permalink raw reply

* Re: Bluetooth SPP on Nokia N900 (Maemo 5)
From: Vinicius Gomes @ 2010-03-02 17:14 UTC (permalink / raw)
  To: Henry Owens; +Cc: linux-bluetooth
In-Reply-To: <aa6ee971003020826m7e6db9b2jd5b9456a1aa1d368@mail.gmail.com>

Hi Henry,

On Tue, Mar 2, 2010 at 1:26 PM, Henry Owens <owens.henry@googlemail.com> wrote:
> Hi All,
>
> My first mail to this list. I am currently trying to get a bluetooth
> SPP connection going on my Nokia N900, but I'm having some
> difficulties using the examples on the HOWTO Wiki.
>
> Specifically, I get an error related to the ActivateService call in
> the example - I have seen reference to this being obselete in Bluez
> 4.x, is this the problem in the example code?
>

Yeah, the example code in the wiki is outdated, take a look at the
example in the sources [1],
for a working one.

Also, in that same directory there are a few more examples that could be useful.


Cheers,
-- 
Vinicius Gomes
INdT - Instituto Nokia de Tecnologia

[1] http://git.kernel.org/?p=bluetooth/bluez.git;a=blob;f=test/test-serial;hb=HEAD

^ permalink raw reply

* [PATCH] Fix using invalid data from previous headset connection
From: Luiz Augusto von Dentz @ 2010-03-02 21:09 UTC (permalink / raw)
  To: linux-bluetooth

Data notiated in a session should be reset once disconnected, to fix this
a new structure is introduced called headset_session is introduced to hold
the session data which is freed when disconnected.
---
 audio/headset.c |  187 +++++++++++++++++++++++++++++++------------------------
 1 files changed, 106 insertions(+), 81 deletions(-)

diff --git a/audio/headset.c b/audio/headset.c
index 7002a3a..4decc18 100644
--- a/audio/headset.c
+++ b/audio/headset.c
@@ -128,6 +128,25 @@ struct pending_connect {
 	uint16_t svclass;
 };

+struct headset_session {
+	char buf[BUF_SIZE];
+	int data_start;
+	int data_length;
+
+	gboolean cli_active;
+	gboolean cme_enabled;
+	gboolean cwa_enabled;
+	gboolean pending_ring;
+	gboolean inband_ring;
+	gboolean nrec;
+	gboolean nrec_req;
+
+	int sp_gain;
+	int mic_gain;
+
+	unsigned int hf_features;
+};
+
 struct headset {
 	uint32_t hsp_handle;
 	uint32_t hfp_handle;
@@ -144,28 +163,14 @@ struct headset {

 	guint dc_timer;

-	char buf[BUF_SIZE];
-	int data_start;
-	int data_length;
-
 	gboolean hfp_active;
 	gboolean search_hfp;
-	gboolean cli_active;
-	gboolean cme_enabled;
-	gboolean cwa_enabled;
-	gboolean pending_ring;
-	gboolean inband_ring;
-	gboolean nrec;
-	gboolean nrec_req;

 	headset_state_t state;
 	struct pending_connect *pending;

-	int sp_gain;
-	int mic_gain;
-
-	unsigned int hf_features;
 	headset_lock_t lock;
+	struct headset_session *session;
 };

 struct event {
@@ -338,14 +343,15 @@ static int headset_send(struct headset *hs, char
*format, ...)
 static int supported_features(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;
 	int err;

 	if (strlen(buf) < 9)
 		return -EINVAL;

-	hs->hf_features = strtoul(&buf[8], NULL, 10);
+	session->hf_features = strtoul(&buf[8], NULL, 10);

-	print_hf_features(hs->hf_features);
+	print_hf_features(session->hf_features);

 	err = headset_send(hs, "\r\n+BRSF: %u\r\n", ag.features);
 	if (err < 0)
@@ -534,10 +540,12 @@ static void send_foreach_headset(GSList *devices,

 static int cli_cmp(struct headset *hs)
 {
+	struct headset_session *session = hs->session;
+
 	if (!hs->hfp_active)
 		return -1;

-	if (hs->cli_active)
+	if (session->cli_active)
 		return 0;
 	else
 		return -1;
@@ -560,6 +568,7 @@ static void sco_connect_cb(GIOChannel *chan,
GError *err, gpointer user_data)
 	int sk;
 	struct audio_device *dev = user_data;
 	struct headset *hs = dev->headset;
+	struct headset_session *session = hs->session;
 	struct pending_connect *p = hs->pending;

 	if (err) {
@@ -599,12 +608,12 @@ static void sco_connect_cb(GIOChannel *chan,
GError *err, gpointer user_data)

 	headset_set_state(dev, HEADSET_STATE_PLAYING);

-	if (hs->pending_ring) {
+	if (session->pending_ring) {
 		ring_timer_cb(NULL);
 		ag.ring_timer = g_timeout_add_seconds(RING_INTERVAL,
 						ring_timer_cb,
 						NULL);
-		hs->pending_ring = FALSE;
+		session->pending_ring = FALSE;
 	}
 }

@@ -684,9 +693,10 @@ static void hfp_slc_complete(struct audio_device *dev)
 static int telephony_generic_rsp(struct audio_device *device, cme_error_t err)
 {
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;

 	if (err != CME_ERROR_NONE) {
-		if (hs->cme_enabled)
+		if (session->cme_enabled)
 			return headset_send(hs, "\r\n+CME ERROR: %d\r\n", err);
 		else
 			return headset_send(hs, "\r\nERROR\r\n");
@@ -699,6 +709,7 @@ int telephony_event_reporting_rsp(void
*telephony_device, cme_error_t err)
 {
 	struct audio_device *device = telephony_device;
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;
 	int ret;

 	if (err != CME_ERROR_NONE)
@@ -711,7 +722,7 @@ int telephony_event_reporting_rsp(void
*telephony_device, cme_error_t err)
 	if (hs->state != HEADSET_STATE_CONNECTING)
 		return 0;

-	if (hs->hf_features & HF_FEATURE_CALL_WAITING_AND_3WAY &&
+	if (session->hf_features & HF_FEATURE_CALL_WAITING_AND_3WAY &&
 			ag.features & AG_FEATURE_THREE_WAY_CALLING)
 		return 0;

@@ -865,11 +876,12 @@ static int terminate_call(struct audio_device
*device, const char *buf)
 static int cli_notification(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;

 	if (strlen(buf) < 9)
 		return -EINVAL;

-	hs->cli_active = buf[8] == '1' ? TRUE : FALSE;
+	session->cli_active = buf[8] == '1' ? TRUE : FALSE;

 	return headset_send(hs, "\r\nOK\r\n");
 }
@@ -937,6 +949,7 @@ static int dial_number(struct audio_device
*device, const char *buf)
 static int signal_gain_setting(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;
 	const char *property;
 	const char *name;
 	dbus_uint16_t gain;
@@ -955,18 +968,18 @@ static int signal_gain_setting(struct
audio_device *device, const char *buf)

 	switch (buf[5]) {
 	case HEADSET_GAIN_SPEAKER:
-		if (hs->sp_gain == gain)
+		if (session->sp_gain == gain)
 			goto ok;
 		name = "SpeakerGainChanged";
 		property = "SpeakerGain";
-		hs->sp_gain = gain;
+		session->sp_gain = gain;
 		break;
 	case HEADSET_GAIN_MICROPHONE:
-		if (hs->mic_gain == gain)
+		if (session->mic_gain == gain)
 			goto ok;
 		name = "MicrophoneGainChanged";
 		property = "MicrophoneGain";
-		hs->mic_gain = gain;
+		session->mic_gain = gain;
 		break;
 	default:
 		error("Unknown gain setting");
@@ -1030,15 +1043,16 @@ static int list_current_calls(struct
audio_device *device, const char *buf)
 static int extended_errors(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;

 	if (strlen(buf) < 9)
 		return -EINVAL;

 	if (buf[8] == '1') {
-		hs->cme_enabled = TRUE;
+		session->cme_enabled = TRUE;
 		debug("CME errors enabled for headset %p", hs);
 	} else {
-		hs->cme_enabled = FALSE;
+		session->cme_enabled = FALSE;
 		debug("CME errors disabled for headset %p", hs);
 	}

@@ -1048,15 +1062,16 @@ static int extended_errors(struct audio_device
*device, const char *buf)
 static int call_waiting_notify(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;

 	if (strlen(buf) < 9)
 		return -EINVAL;

 	if (buf[8] == '1') {
-		hs->cwa_enabled = TRUE;
+		session->cwa_enabled = TRUE;
 		debug("Call waiting notification enabled for headset %p", hs);
 	} else {
-		hs->cwa_enabled = FALSE;
+		session->cwa_enabled = FALSE;
 		debug("Call waiting notification disabled for headset %p", hs);
 	}

@@ -1077,9 +1092,10 @@ int telephony_nr_and_ec_rsp(void
*telephony_device, cme_error_t err)
 {
 	struct audio_device *device = telephony_device;
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;

 	if (err == CME_ERROR_NONE)
-		hs->nrec = hs->nrec_req;
+		session->nrec = hs->session->nrec_req;

 	return telephony_generic_rsp(telephony_device, err);
 }
@@ -1123,16 +1139,17 @@ static int operator_selection(struct
audio_device *device, const char *buf)
 static int nr_and_ec(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;

 	if (strlen(buf) < 9)
 		return -EINVAL;

 	if (buf[8] == '0')
-		hs->nrec_req = FALSE;
+		session->nrec_req = FALSE;
 	else
-		hs->nrec_req = TRUE;
+		session->nrec_req = TRUE;

-	telephony_nr_and_ec_req(device, hs->nrec_req);
+	telephony_nr_and_ec_req(device, session->nrec_req);

 	return 0;
 }
@@ -1214,6 +1231,7 @@ static gboolean rfcomm_io_cb(GIOChannel *chan,
GIOCondition cond,
 				struct audio_device *device)
 {
 	struct headset *hs;
+	struct headset_session *session;
 	unsigned char buf[BUF_SIZE];
 	gsize bytes_read = 0;
 	gsize free_space;
@@ -1222,6 +1240,7 @@ static gboolean rfcomm_io_cb(GIOChannel *chan,
GIOCondition cond,
 		return FALSE;

 	hs = device->headset;
+	session = hs->session;

 	if (cond & (G_IO_ERR | G_IO_HUP)) {
 		debug("ERR or HUP on RFCOMM socket");
@@ -1232,7 +1251,8 @@ static gboolean rfcomm_io_cb(GIOChannel *chan,
GIOCondition cond,
 				&bytes_read) != G_IO_ERROR_NONE)
 		return TRUE;

-	free_space = sizeof(hs->buf) - hs->data_start - hs->data_length - 1;
+	free_space = sizeof(session->buf) - session->data_start -
+			session->data_length - 1;

 	if (free_space < bytes_read) {
 		/* Very likely that the HS is sending us garbage so
@@ -1241,45 +1261,45 @@ static gboolean rfcomm_io_cb(GIOChannel *chan,
GIOCondition cond,
 		goto failed;
 	}

-	memcpy(&hs->buf[hs->data_start], buf, bytes_read);
-	hs->data_length += bytes_read;
+	memcpy(&session->buf[session->data_start], buf, bytes_read);
+	session->data_length += bytes_read;

 	/* Make sure the data is null terminated so we can use string
 	 * functions */
-	hs->buf[hs->data_start + hs->data_length] = '\0';
+	session->buf[session->data_start + session->data_length] = '\0';

-	while (hs->data_length > 0) {
+	while (session->data_length > 0) {
 		char *cr;
 		int err;
 		off_t cmd_len;

-		cr = strchr(&hs->buf[hs->data_start], '\r');
+		cr = strchr(&session->buf[session->data_start], '\r');
 		if (!cr)
 			break;

-		cmd_len = 1 + (off_t) cr - (off_t) &hs->buf[hs->data_start];
+		cmd_len = 1 + (off_t) cr - (off_t) &session->buf[session->data_start];
 		*cr = '\0';

 		if (cmd_len > 1)
-			err = handle_event(device, &hs->buf[hs->data_start]);
+			err = handle_event(device, &session->buf[session->data_start]);
 		else
 			/* Silently skip empty commands */
 			err = 0;

 		if (err == -EINVAL) {
 			error("Badly formated or unrecognized command: %s",
-					&hs->buf[hs->data_start]);
+					&session->buf[session->data_start]);
 			err = headset_send(hs, "\r\nERROR\r\n");
 		} else if (err < 0)
 			error("Error handling command %s: %s (%d)",
-						&hs->buf[hs->data_start],
+						&session->buf[session->data_start],
 						strerror(-err), -err);

-		hs->data_start += cmd_len;
-		hs->data_length -= cmd_len;
+		session->data_start += cmd_len;
+		session->data_length -= cmd_len;

-		if (!hs->data_length)
-			hs->data_start = 0;
+		if (!session->data_length)
+			session->data_start = 0;
 	}

 	return TRUE;
@@ -1337,6 +1357,9 @@ void headset_connect_cb(GIOChannel *chan, GError
*err, gpointer user_data)

 	debug("%s: Connected to %s", dev->path, hs_address);

+	hs->session = g_new0(struct headset_session, 1);
+	hs->session->nrec = TRUE;
+
 	/* In HFP mode wait for Service Level Connection */
 	if (hs->hfp_active)
 		return;
@@ -1811,10 +1834,11 @@ static DBusMessage
*hs_get_speaker_gain(DBusConnection *conn,
 {
 	struct audio_device *device = data;
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;
 	DBusMessage *reply;
 	dbus_uint16_t gain;

-	if (hs->state < HEADSET_STATE_CONNECTED || hs->sp_gain < 0)
+	if (hs->state < HEADSET_STATE_CONNECTED || session == NULL)
 		return g_dbus_create_error(msg, ERROR_INTERFACE ".NotAvailable",
 						"Operation not Available");

@@ -1822,7 +1846,7 @@ static DBusMessage
*hs_get_speaker_gain(DBusConnection *conn,
 	if (!reply)
 		return NULL;

-	gain = (dbus_uint16_t) hs->sp_gain;
+	gain = (dbus_uint16_t) session->sp_gain;

 	dbus_message_append_args(reply, DBUS_TYPE_UINT16, &gain,
 					DBUS_TYPE_INVALID);
@@ -1836,10 +1860,11 @@ static DBusMessage
*hs_get_mic_gain(DBusConnection *conn,
 {
 	struct audio_device *device = data;
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;
 	DBusMessage *reply;
 	dbus_uint16_t gain;

-	if (hs->state < HEADSET_STATE_CONNECTED || hs->mic_gain < 0)
+	if (hs->state < HEADSET_STATE_CONNECTED || session == NULL)
 		return g_dbus_create_error(msg, ERROR_INTERFACE ".NotAvailable",
 						"Operation not Available");

@@ -1847,7 +1872,7 @@ static DBusMessage *hs_get_mic_gain(DBusConnection *conn,
 	if (!reply)
 		return NULL;

-	gain = (dbus_uint16_t) hs->mic_gain;
+	gain = (dbus_uint16_t) session->mic_gain;

 	dbus_message_append_args(reply, DBUS_TYPE_UINT16, &gain,
 					DBUS_TYPE_INVALID);
@@ -1862,6 +1887,7 @@ static DBusMessage *hs_set_gain(DBusConnection *conn,
 {
 	struct audio_device *device = data;
 	struct headset *hs = device->headset;
+	struct headset_session *session = hs->session;
 	DBusMessage *reply;
 	int err;

@@ -1891,14 +1917,14 @@ static DBusMessage *hs_set_gain(DBusConnection *conn,

 done:
 	if (type == HEADSET_GAIN_SPEAKER) {
-		hs->sp_gain = gain;
+		session->sp_gain = gain;
 		g_dbus_emit_signal(conn, device->path,
 					AUDIO_HEADSET_INTERFACE,
 					"SpeakerGainChanged",
 					DBUS_TYPE_UINT16, &gain,
 					DBUS_TYPE_INVALID);
 	} else {
-		hs->mic_gain = gain;
+		session->mic_gain = gain;
 		g_dbus_emit_signal(conn, device->path,
 					AUDIO_HEADSET_INTERFACE,
 					"MicrophoneGainChanged",
@@ -1975,11 +2001,13 @@ static DBusMessage
*hs_get_properties(DBusConnection *conn,

 	/* SpeakerGain */
 	dict_append_entry(&dict, "SpeakerGain",
-				DBUS_TYPE_UINT16, &device->headset->sp_gain);
+				DBUS_TYPE_UINT16,
+				&device->headset->session->sp_gain);

 	/* MicrophoneGain */
 	dict_append_entry(&dict, "MicrophoneGain",
-				DBUS_TYPE_UINT16, &device->headset->mic_gain);
+				DBUS_TYPE_UINT16,
+				&device->headset->session->mic_gain);

 done:
 	dbus_message_iter_close_container(&iter, &dict);
@@ -2118,10 +2146,8 @@ static int headset_close_rfcomm(struct audio_device *dev)
 		hs->rfcomm = NULL;
 	}

-	hs->data_start = 0;
-	hs->data_length = 0;
-
-	hs->nrec = TRUE;
+	g_free(hs->session);
+	hs->session = NULL;

 	return 0;
 }
@@ -2176,12 +2202,7 @@ struct headset *headset_init(struct
audio_device *dev, uint16_t svc,

 	hs = g_new0(struct headset, 1);
 	hs->rfcomm_ch = -1;
-	hs->sp_gain = -1;
-	hs->mic_gain = -1;
 	hs->search_hfp = server_is_enabled(&dev->src, HANDSFREE_SVCLASS_ID);
-	hs->hfp_active = FALSE;
-	hs->cli_active = FALSE;
-	hs->nrec = TRUE;

 	record = btd_device_get_record(dev->btd_dev, uuidstr);
 	if (!record)
@@ -2424,18 +2445,19 @@ int headset_connect_rfcomm(struct audio_device
*dev, GIOChannel *io)
 int headset_connect_sco(struct audio_device *dev, GIOChannel *io)
 {
 	struct headset *hs = dev->headset;
+	struct headset_session *session = hs->session;

 	if (hs->sco)
 		return -EISCONN;

 	hs->sco = g_io_channel_ref(io);

-	if (hs->pending_ring) {
+	if (session->pending_ring) {
 		ring_timer_cb(NULL);
 		ag.ring_timer = g_timeout_add_seconds(RING_INTERVAL,
 						ring_timer_cb,
 						NULL);
-		hs->pending_ring = FALSE;
+		session->pending_ring = FALSE;
 	}

 	return 0;
@@ -2454,6 +2476,7 @@ static void disconnect_cb(struct btd_device
*btd_dev, gboolean removal,
 void headset_set_state(struct audio_device *dev, headset_state_t state)
 {
 	struct headset *hs = dev->headset;
+	struct headset_session *session = hs->session;
 	gboolean value;
 	const char *state_str;
 	headset_state_t old_state = hs->state;
@@ -2499,9 +2522,9 @@ void headset_set_state(struct audio_device *dev,
headset_state_t state)
 					DBUS_TYPE_STRING, &state_str);
 		if (hs->state < state) {
 			if (ag.features & AG_FEATURE_INBAND_RINGTONE)
-				hs->inband_ring = TRUE;
+				session->inband_ring = TRUE;
 			else
-				hs->inband_ring = FALSE;
+				session->inband_ring = FALSE;
 			g_dbus_emit_signal(dev->conn, dev->path,
 						AUDIO_HEADSET_INTERFACE,
 						"Connected",
@@ -2546,10 +2569,10 @@ void headset_set_state(struct audio_device
*dev, headset_state_t state)
 					AUDIO_HEADSET_INTERFACE, "Playing",
 					DBUS_TYPE_BOOLEAN, &value);

-		if (hs->sp_gain >= 0)
-			headset_send(hs, "\r\n+VGS=%u\r\n", hs->sp_gain);
-		if (hs->mic_gain >= 0)
-			headset_send(hs, "\r\n+VGM=%u\r\n", hs->mic_gain);
+		if (session->sp_gain >= 0)
+			headset_send(hs, "\r\n+VGS=%u\r\n", session->sp_gain);
+		if (session->mic_gain >= 0)
+			headset_send(hs, "\r\n+VGM=%u\r\n", session->mic_gain);
 		break;
 	}

@@ -2658,7 +2681,7 @@ gboolean headset_get_nrec(struct audio_device *dev)
 {
 	struct headset *hs = dev->headset;

-	return hs->nrec;
+	return hs->session->nrec;
 }

 gboolean headset_get_sco_hci(struct audio_device *dev)
@@ -2715,6 +2738,7 @@ int telephony_incoming_call_ind(const char
*number, int type)
 {
 	struct audio_device *dev;
 	struct headset *hs;
+	struct headset_session *session;

 	if (!active_devices)
 		return -ENODEV;
@@ -2722,6 +2746,7 @@ int telephony_incoming_call_ind(const char
*number, int type)
 	/* Get the latest connected device */
 	dev = active_devices->data;
 	hs = dev->headset;
+	session = hs->session;

 	if (ag.ring_timer) {
 		debug("telephony_incoming_call_ind: already calling");
@@ -2730,16 +2755,16 @@ int telephony_incoming_call_ind(const char
*number, int type)

 	/* With HSP 1.2 the RING messages should *not* be sent if inband
 	 * ringtone is being used */
-	if (!hs->hfp_active && hs->inband_ring)
+	if (!hs->hfp_active && session->inband_ring)
 		return 0;

 	g_free(ag.number);
 	ag.number = g_strdup(number);
 	ag.number_type = type;

-	if (hs->inband_ring && hs->hfp_active &&
+	if (session->inband_ring && hs->hfp_active &&
 					hs->state != HEADSET_STATE_PLAYING) {
-		hs->pending_ring = TRUE;
+		session->pending_ring = TRUE;
 		return 0;
 	}

@@ -2765,10 +2790,10 @@ int telephony_calling_stopped_ind(void)
 	/* In case SCO isn't fully up yet */
 	dev = active_devices->data;

-	if (!dev->headset->pending_ring && !ag.ring_timer)
+	if (!dev->headset->session->pending_ring && !ag.ring_timer)
 		return -EINVAL;

-	dev->headset->pending_ring = FALSE;
+	dev->headset->session->pending_ring = FALSE;

 	return 0;
 }
@@ -2826,7 +2851,7 @@ static int cwa_cmp(struct headset *hs)
 	if (!hs->hfp_active)
 		return -1;

-	if (hs->cwa_enabled)
+	if (hs->session->cwa_enabled)
 		return 0;
 	else
 		return -1;
-- 
1.6.3.3

-- 
Luiz Augusto von Dentz
Computer Engineer

^ permalink raw reply related

* Re: linux-next: Tree for March 1 (bluetooth/hci_sysfs)
From: Marcel Holtmann @ 2010-03-03  1:23 UTC (permalink / raw)
  To: David Miller
  Cc: sfr, randy.dunlap, linux-next, linux-kernel, linux-bluetooth,
	netdev
In-Reply-To: <20100301.181415.266447389.davem@davemloft.net>

Hi Dave,

> >> static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
> >> 						size_t count, loff_t *ppos)
> >> {
> >> 	struct hci_dev *hdev = file->private_data;
> >> 	struct inquiry_cache *cache = &hdev->inq_cache;
> >> 	struct inquiry_entry *e;
> >> 	char buf[4096]; // <<<<<<<<<<<<<<<<<<<<<<<<<<< huh? don't do that on stack.
> >> 	int n = 0;
> > 
> > Dave Miller is following up on that.
> 
> This looks like a job for.... SEQ FILE! :-)
> 
> I'm testing the following fix.

I have a working and tested patch that uses single_open(). Will send a
patch with commit message and everything in a bit so you can apply it.

Regards

Marcel



^ permalink raw reply


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