* [PATCH 1/7] Fix proper type handling in contacts_query_all
From: Bartosz Szatkowski @ 2010-11-10 13:15 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bartosz Szatkowski
Previously all phone numbers, addresses and emails was considered to be "work".
Now there are three working types for emails and addresses: "work", "home",
"other" and four for phone numbers - these three as well as "cell".
---
plugins/phonebook-tracker.c | 61 +++++++++++++++++++++++++++++++------------
1 files changed, 44 insertions(+), 17 deletions(-)
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 58f52ab..83327e0 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -43,8 +43,8 @@
#define TRACKER_RESOURCES_INTERFACE "org.freedesktop.Tracker1.Resources"
#define TRACKER_DEFAULT_CONTACT_ME "http://www.semanticdesktop.org/ontologies/2007/03/22/nco#default-contact-me"
-#define CONTACTS_ID_COL 38
-#define PULL_QUERY_COL_AMOUNT 39
+#define CONTACTS_ID_COL 47
+#define PULL_QUERY_COL_AMOUNT 48
#define COUNT_QUERY_COL_AMOUNT 1
#define COL_HOME_NUMBER 0
#define COL_HOME_EMAIL 7
@@ -52,14 +52,16 @@
#define COL_FAX_NUMBER 16
#define COL_WORK_EMAIL 17
#define COL_OTHER_NUMBER 34
-#define COL_DATE 35
-#define COL_SENT 36
-#define COL_ANSWERED 37
+#define COL_OTHER_EMAIL 42
+#define COL_CELL_NUMBER 43
+#define COL_DATE 44
+#define COL_SENT 45
+#define COL_ANSWERED 46
#define ADDR_FIELD_AMOUNT 7
#define CONTACT_ID_PREFIX "contact:"
#define CONTACTS_QUERY_ALL \
- "SELECT ?v nco:fullname(?c) " \
+ "SELECT nco:phoneNumber(?v) nco:fullname(?c) " \
"nco:nameFamily(?c) nco:nameGiven(?c) " \
"nco:nameAdditional(?c) nco:nameHonorificPrefix(?c) " \
"nco:nameHonorificSuffix(?c) nco:emailAddress(?e) " \
@@ -71,29 +73,43 @@
"nco:role(?a) nco:pobox(?pw) nco:extendedAddress(?pw) " \
"nco:streetAddress(?pw) nco:locality(?pw) nco:region(?pw) " \
"nco:postalcode(?pw) nco:country(?pw) nco:contactUID(?c) " \
- "nco:title(?a) nco:phoneNumber(?t) " \
+ "nco:title(?a) ?t nco:pobox(?po) nco:extendedAddress(?po) " \
+ "nco:streetAddress(?po) nco:locality(?po) nco:region(?po) " \
+ "nco:postalcode(?po) nco:country(?po) nco:emailAddress(?eo) " \
+ "?vc " \
"\"NOTACALL\" \"false\" \"false\" ?c " \
"WHERE { " \
"?c a nco:PersonContact . " \
- "OPTIONAL { ?c nco:hasPhoneNumber ?h . \
- OPTIONAL {" \
+ "OPTIONAL { ?c nco:hasPhoneNumber ?h . " \
+ "OPTIONAL {" \
"?h a nco:FaxNumber ; " \
"nco:phoneNumber ?f . " \
"}" \
"OPTIONAL {" \
+ "?h a nco:CellPhoneNumber ; " \
+ "nco:phoneNumber ?vc" \
+ "}" \
+ "OPTIONAL {" \
"?h a nco:VoicePhoneNumber ; " \
- "nco:phoneNumber ?v" \
+ "nco:phoneNumber ?t" \
"}" \
"}" \
- "OPTIONAL { ?c nco:hasEmailAddress ?e . } " \
- "OPTIONAL { ?c nco:hasPostalAddress ?p . } " \
"OPTIONAL { " \
"?c nco:hasAffiliation ?a . " \
- "OPTIONAL { ?a nco:hasPhoneNumber ?w . } " \
- "OPTIONAL { ?a nco:hasEmailAddress ?ew . } " \
- "OPTIONAL { ?a nco:hasPostalAddress ?pw . } " \
+ "OPTIONAL { ?a rdfs:label \"Work\" . " \
+ "OPTIONAL { ?a nco:hasEmailAddress ?ew . } " \
+ "OPTIONAL { ?a nco:hasPostalAddress ?pw . } " \
+ "OPTIONAL { ?a nco:hasPhoneNumber ?w . } " \
+ "}" \
+ "OPTIONAL { ?a rdfs:label \"Home\" . " \
+ "OPTIONAL { ?a nco:hasEmailAddress ?e . } " \
+ "OPTIONAL { ?a nco:hasPostalAddress ?p . } " \
+ "OPTIONAL { ?a nco:hasPhoneNumber ?v . } " \
+ "}" \
"OPTIONAL { ?a nco:org ?o . } " \
"} " \
+ "OPTIONAL { ?c nco:hasPostalAddress ?po . } " \
+ "OPTIONAL { ?c nco:hasEmailAddress ?eo . } " \
"}"
#define CONTACTS_QUERY_ALL_LIST \
@@ -1107,7 +1123,7 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
GString *vcards;
int last_index, i;
gboolean cdata_present = FALSE;
- char *home_addr, *work_addr;
+ char *home_addr, *work_addr, *other_addr;
if (num_fields < 0) {
data->cb(NULL, 0, num_fields, 0, data->user_data);
@@ -1178,11 +1194,16 @@ add_numbers:
add_phone_number(contact, reply[COL_HOME_NUMBER], TEL_TYPE_HOME);
add_phone_number(contact, reply[COL_WORK_NUMBER], TEL_TYPE_WORK);
add_phone_number(contact, reply[COL_FAX_NUMBER], TEL_TYPE_FAX);
- add_phone_number(contact, reply[COL_OTHER_NUMBER], TEL_TYPE_OTHER);
+ add_phone_number(contact, reply[COL_CELL_NUMBER], TEL_TYPE_MOBILE);
+ if ((g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_CELL_NUMBER]) != 0) &&
+ (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_WORK_NUMBER]) != 0) &&
+ (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_HOME_NUMBER]) != 0))
+ add_phone_number(contact, reply[COL_OTHER_NUMBER], TEL_TYPE_OTHER);
/* Adding emails */
add_email(contact, reply[COL_HOME_EMAIL], EMAIL_TYPE_HOME);
add_email(contact, reply[COL_WORK_EMAIL], EMAIL_TYPE_WORK);
+ add_email(contact, reply[COL_OTHER_EMAIL], EMAIL_TYPE_OTHER);
/* Adding addresses */
home_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
@@ -1193,11 +1214,17 @@ add_numbers:
reply[25], reply[26], reply[27], reply[28],
reply[29], reply[30], reply[31]);
+ other_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
+ reply[35], reply[36], reply[37], reply[38],
+ reply[39], reply[40], reply[41]);
+
add_address(contact, home_addr, ADDR_TYPE_HOME);
add_address(contact, work_addr, ADDR_TYPE_WORK);
+ add_address(contact, other_addr, ADDR_TYPE_OTHER);
g_free(home_addr);
g_free(work_addr);
+ g_free(other_addr);
/* Adding fields connected by nco:hasAffiliation - they may be in
* separate replies */
--
1.7.0.4
^ permalink raw reply related
* [PATCH v5] Bluetooth: btwilink driver
From: pavan_savoy @ 2010-11-10 13:07 UTC (permalink / raw)
To: marcel, padovan; +Cc: linux-bluetooth, linux-kernel, Pavan Savoy
From: Pavan Savoy <pavan_savoy@ti.com>
Marcel,
Thanks for the comments...
This patch contains,
v5 comments :-
declaration and assiging of variables and private data fixed up.
proper casting.
removed redundant un-necessary checks in send_frame.
HCI_RUNNING fixes in terms of test_and_set/clear bit instead of set and
clear.
removed redundant checks for hdev, skb being NULL.
removed module_param of reset, also WiLink don't need HCI_RESET anyways.
removed ti_st_register_dev function and functionality moved to _probe.
module_init/exit function names fixed up.
stat byte counter increments and tx_complete is similar to hci_ldisc.
Also I have not implemented the flush routine, since the functionality
which needs to be done in flush routine is done in the underlying driver
which is the shared transport driver and moreover the btwilink driver by
itself doesn't maintains queue or data relevant to transport, so nothing
to do.
And Yes, I have verified this driver with multiple up/down reset on
hci0.
Also I generally test a2dp/ftp to verify large data transfers.
Please review and comment.
Thanks,
Pavan
v4 comments :-
module init now returns what platform_driver_register returns.
type casting of void* private data has been removed
v3 comments :-
Lizardo,
I have taken care of most of the comments you had.
Have re-wrote some of the code commenting you've mentioned.
Thanks for the comments,
The other few like -EPERM for platform driver registration is to keep
it similar to other drivers, type casting is maintained just to feel safe
and have style similar to other drivers.
BT_WILINK in Kconfig is similar to BT_MRVL.
I hope those aren't too critical.
-- patch description --
This is the bluetooth protocol driver for the TI WiLink7 chipsets.
Texas Instrument's WiLink chipsets combine wireless technologies
like BT, FM, GPS and WLAN onto a single chip.
This Bluetooth driver works on top of the TI_ST shared transport
line discipline driver which also allows other drivers like
FM V4L2 and GPS character driver to make use of the same UART interface.
Kconfig and Makefile modifications to enable the Bluetooth
driver for Texas Instrument's WiLink 7 chipset.
Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
---
drivers/bluetooth/Kconfig | 10 +
drivers/bluetooth/Makefile | 1 +
drivers/bluetooth/btwilink.c | 379 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 390 insertions(+), 0 deletions(-)
create mode 100644 drivers/bluetooth/btwilink.c
diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 02deef4..8e0de9a 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -219,4 +219,14 @@ config BT_ATH3K
Say Y here to compile support for "Atheros firmware download driver"
into the kernel or say M to compile it as module (ath3k).
+config BT_WILINK
+ tristate "Texas Instruments WiLink7 driver"
+ depends on TI_ST
+ help
+ This enables the Bluetooth driver for Texas Instrument's BT/FM/GPS
+ combo devices. This makes use of shared transport line discipline
+ core driver to communicate with the BT core of the combo chip.
+
+ Say Y here to compile support for Texas Instrument's WiLink7 driver
+ into the kernel or say M to compile it as module.
endmenu
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 71bdf13..f4460f4 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_BT_HCIBTSDIO) += btsdio.o
obj-$(CONFIG_BT_ATH3K) += ath3k.o
obj-$(CONFIG_BT_MRVL) += btmrvl.o
obj-$(CONFIG_BT_MRVL_SDIO) += btmrvl_sdio.o
+obj-$(CONFIG_BT_WILINK) += btwilink.o
btmrvl-y := btmrvl_main.o
btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
new file mode 100644
index 0000000..1b1c4bc
--- /dev/null
+++ b/drivers/bluetooth/btwilink.c
@@ -0,0 +1,379 @@
+/*
+ * Texas Instrument's Bluetooth Driver For Shared Transport.
+ *
+ * Bluetooth Driver acts as interface between HCI core and
+ * TI Shared Transport Layer.
+ *
+ * Copyright (C) 2009-2010 Texas Instruments
+ * Author: Raja Mani <raja_mani@ti.com>
+ * Pavan Savoy <pavan_savoy@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include <linux/ti_wilink_st.h>
+
+/* Bluetooth Driver Version */
+#define VERSION "1.0"
+
+/* Number of seconds to wait for registration completion
+ * when ST returns PENDING status.
+ */
+#define BT_REGISTER_TIMEOUT 6000 /* 6 sec */
+
+/**
+ * struct ti_st - driver operation structure
+ * @hdev: hci device pointer which binds to bt driver
+ * @reg_status: ST registration callback status
+ * @st_write: write function provided by the ST driver
+ * to be used by the driver during send_frame.
+ * @wait_reg_completion - completion sync between ti_st_open
+ * and ti_st_registration_completion_cb.
+ */
+struct ti_st {
+ struct hci_dev *hdev;
+ char reg_status;
+ long (*st_write) (struct sk_buff *);
+ struct completion wait_reg_completion;
+};
+
+/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
+static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
+{
+ struct hci_dev *hdev = hst->hdev;
+
+ /* Update HCI stat counters */
+ switch (pkt_type) {
+ case HCI_COMMAND_PKT:
+ hdev->stat.cmd_tx++;
+ break;
+
+ case HCI_ACLDATA_PKT:
+ hdev->stat.acl_tx++;
+ break;
+
+ case HCI_SCODATA_PKT:
+ hdev->stat.sco_tx++;
+ break;
+ }
+}
+
+/* ------- Interfaces to Shared Transport ------ */
+
+/* Called by ST layer to indicate protocol registration completion
+ * status.ti_st_open() function will wait for signal from this
+ * API when st_register() function returns ST_PENDING.
+ */
+static void st_registration_completion_cb(void *priv_data, char data)
+{
+ struct ti_st *lhst = priv_data;
+
+ /* Save registration status for use in ti_st_open() */
+ lhst->reg_status = data;
+ /* complete the wait in ti_st_open() */
+ complete(&lhst->wait_reg_completion);
+}
+
+/* Called by Shared Transport layer when receive data is
+ * available */
+static long st_receive(void *priv_data, struct sk_buff *skb)
+{
+ struct ti_st *lhst = priv_data;
+ int err;
+
+ if (!skb)
+ return -EFAULT;
+
+ if (!lhst) {
+ kfree_skb(skb);
+ return -EFAULT;
+ }
+
+ skb->dev = (void *) lhst->hdev;
+
+ /* Forward skb to HCI core layer */
+ err = hci_recv_frame(skb);
+ if (err < 0) {
+ BT_ERR("Unable to push skb to HCI core(%d)", err);
+ return err;
+ }
+
+ lhst->hdev->stat.byte_rx += skb->len;
+
+ return 0;
+}
+
+/* ------- Interfaces to HCI layer ------ */
+/* protocol structure registered with shared transport */
+static struct st_proto_s ti_st_proto = {
+ .type = ST_BT,
+ .recv = st_receive,
+ .reg_complete_cb = st_registration_completion_cb,
+};
+
+/* Called from HCI core to initialize the device */
+static int ti_st_open(struct hci_dev *hdev)
+{
+ unsigned long timeleft;
+ struct ti_st *hst;
+ int err;
+
+ BT_DBG("%s %p", hdev->name, hdev);
+
+ /* provide contexts for callbacks from ST */
+ hst = hdev->driver_data;
+ ti_st_proto.priv_data = hst;
+
+ err = st_register(&ti_st_proto);
+ if (err == -EINPROGRESS) {
+ /* Prepare wait-for-completion handler data structures.
+ * Needed to synchronize this and
+ * st_registration_completion_cb() functions.
+ */
+ init_completion(&hst->wait_reg_completion);
+
+ /* Reset ST registration callback status flag , this value
+ * will be updated in ti_st_registration_completion_cb()
+ * function whenever it called from ST driver.
+ */
+ hst->reg_status = -EINPROGRESS;
+
+ /* ST is busy with either protocol registration or firmware
+ * download. Wait until the registration callback is called
+ */
+ BT_DBG(" waiting for registration completion signal from ST");
+
+ timeleft = wait_for_completion_timeout
+ (&hst->wait_reg_completion,
+ msecs_to_jiffies(BT_REGISTER_TIMEOUT));
+ if (!timeleft) {
+ BT_ERR("Timeout(%d sec),didn't get reg "
+ "completion signal from ST",
+ BT_REGISTER_TIMEOUT / 1000);
+ return -ETIMEDOUT;
+ }
+
+ /* Is ST registration callback called with ERROR status? */
+ if (hst->reg_status != 0) {
+ BT_ERR("ST registration completed with invalid "
+ "status %d", hst->reg_status);
+ return -EAGAIN;
+ }
+ err = 0;
+ } else if (err == -EPERM) {
+ BT_ERR("st_register failed %d", err);
+ return err;
+ }
+
+ /* ti_st_proto.write is filled up by the underlying shared
+ * transport driver upon registration
+ */
+ hst->st_write = ti_st_proto.write;
+ if (!hst->st_write) {
+ BT_ERR("undefined ST write function");
+
+ /* Undo registration with ST */
+ err = st_unregister(ST_BT);
+ if (err)
+ BT_ERR("st_unregister() failed with error %d", err);
+
+ hst->st_write = NULL;
+ return err;
+ }
+
+ /* Registration with ST layer is successful,
+ * hardware is ready to accept commands from HCI core.
+ */
+ if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
+ clear_bit(HCI_RUNNING, &hdev->flags);
+ err = st_unregister(ST_BT);
+ if (err)
+ BT_ERR("st_unregister() failed with error %d", err);
+ hst->st_write = NULL;
+ }
+
+ return err;
+}
+
+/* Close device */
+static int ti_st_close(struct hci_dev *hdev)
+{
+ int err;
+ struct ti_st *hst = hdev->driver_data;
+
+ /* continue to unregister from transport */
+ err = st_unregister(ST_BT);
+ if (err)
+ BT_ERR("st_unregister() failed with error %d", err);
+
+ hst->st_write = NULL;
+
+ if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
+ return 0;
+
+ return err;
+}
+
+static int ti_st_send_frame(struct sk_buff *skb)
+{
+ struct hci_dev *hdev;
+ struct ti_st *hst;
+ long len;
+
+ hdev = (struct hci_dev *)skb->dev;
+
+ if (!test_bit(HCI_RUNNING, &hdev->flags))
+ return -EBUSY;
+
+ hst = hdev->driver_data;
+
+ /* Prepend skb with frame type */
+ memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
+
+ BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
+ skb->len);
+
+ /* Insert skb to shared transport layer's transmit queue.
+ * Freeing skb memory is taken care in shared transport layer,
+ * so don't free skb memory here.
+ */
+ len = hst->st_write(skb);
+ if (len < 0) {
+ kfree_skb(skb);
+ BT_ERR(" ST write failed (%ld)", len);
+ return -EAGAIN;
+ }
+
+ /* ST accepted our skb. So, Go ahead and do rest */
+ hdev->stat.byte_tx += len;
+ ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
+
+ return 0;
+}
+
+static void ti_st_destruct(struct hci_dev *hdev)
+{
+ BT_DBG("%s", hdev->name);
+
+ /* free ti_st memory */
+ kfree(hdev->driver_data);
+
+ return;
+}
+
+static int bt_ti_probe(struct platform_device *pdev)
+{
+ static struct ti_st *hst;
+ struct hci_dev *hdev;
+ int err;
+
+ hst = kzalloc(sizeof(struct ti_st), GFP_KERNEL);
+ if (!hst)
+ return -ENOMEM;
+
+ /* Expose "hciX" device to user space */
+ hdev = hci_alloc_dev();
+ if (!hdev)
+ return -ENOMEM;
+
+ BT_DBG("hdev %p", hdev);
+
+ hst->hdev = hdev;
+ hdev->bus = HCI_UART;
+ hdev->driver_data = hst;
+ hdev->open = ti_st_open;
+ hdev->close = ti_st_close;
+ hdev->flush = NULL;
+ hdev->send = ti_st_send_frame;
+ hdev->destruct = ti_st_destruct;
+ hdev->owner = THIS_MODULE;
+
+ err = hci_register_dev(hdev);
+ if (err < 0) {
+ BT_ERR("Can't register HCI device error %d", err);
+ hci_free_dev(hdev);
+ return err;
+ }
+
+ BT_DBG(" HCI device registered (hdev %p)", hdev);
+
+ dev_set_drvdata(&pdev->dev, hst);
+ return err;
+}
+
+static int bt_ti_remove(struct platform_device *pdev)
+{
+ struct hci_dev *hdev;
+ struct ti_st *hst = dev_get_drvdata(&pdev->dev);
+
+ if (!hst)
+ return -EFAULT;
+
+ hdev = hst->hdev;
+ ti_st_close(hdev);
+ hci_unregister_dev(hdev);
+
+ /* Free HCI device memory */
+ hci_free_dev(hdev);
+
+ /* Free driver data memory */
+ kfree(hst);
+
+ dev_set_drvdata(&pdev->dev, NULL);
+ return 0;
+}
+
+static struct platform_driver btwilink_driver = {
+ .probe = bt_ti_probe,
+ .remove = bt_ti_remove,
+ .driver = {
+ .name = "btwilink",
+ .owner = THIS_MODULE,
+ },
+};
+
+/* ------- Module Init/Exit interfaces ------ */
+static int __init btwilink_init(void)
+{
+ long ret;
+
+ BT_INFO(" Bluetooth Driver for TI WiLink - Version %s", VERSION);
+
+ ret = platform_driver_register(&btwilink_driver);
+ if (ret != 0) {
+ BT_ERR("btwilink platform driver registration failed");
+ return ret;
+ }
+ return 0;
+}
+
+static void __exit btwilink_exit(void)
+{
+ platform_driver_unregister(&btwilink_driver);
+}
+
+module_init(btwilink_init);
+module_exit(btwilink_exit);
+
+/* ------ Module Info ------ */
+
+MODULE_AUTHOR("Raja Mani <raja_mani@ti.com>");
+MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
--
1.5.6.3
^ permalink raw reply related
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Jose Antonio Santos Cadenas @ 2010-11-10 13:02 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
In-Reply-To: <BCBAC715-C7A9-415F-BE62-F843DE5A72D1@signove.com>
Hi Elvis,
2010/11/10 Elvis Pfützenreuter <epx@signove.com>:
>> Hi Elvis,
>>
>> 2010/11/10 Elvis Pfützenreuter <epx@signove.com>:
>>>>> ---
>>>>> health/hdp.c | 1 +
>>>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/health/hdp.c b/health/hdp.c
>>>>> index 1eba8e1..d361b27 100644
>>>>> --- a/health/hdp.c
>>>>> +++ b/health/hdp.c
>>>>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>>>>> if (!hdp_device->mcl)
>>>>> return;
>>>>>
>>>>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>>>>> mcap_mcl_unref(hdp_device->mcl);
>>>>> hdp_device->mcl = NULL;
>>>>> hdp_device->mcl_conn = FALSE;
>>>>> --
>>>>> 1.7.1
>>>>>
>>>>>
>>>>
>>>> Please Elvis, try this solution and tell us if it fix the segfault problem.
>>>
>>> Yes, it seems to have fixed the problem. And far cleaner :)
>>>
>>> I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
>>
>> This function doesn't disable the caching in general, it just closes
>> this MCL without catching it, but caching is still active for all the
>> other MCL's. Additionally, nothing happens if mcap_close_mcl is called
>> more than once, so this patch takes care of this too.
>
> Indeed, MCL disconnection and hdp_device destruction are different moments. Still fulfilling the caffeine quota for this morning :P
Of course they are, but when a device is removed, is completely normal
that you (HDP) would like to delete the MCAP cache because you (HDP)
are forgetting this device so you won't have any structures for the
data channels nor other structs in HDP needed to manage the
reconnections of the data channels that could be cached. The way to
uncache an mcl in MCAP is to close it indicating that you don't want
to cache it. This is just what this patch to.
>
>>>
>>> So, perhaps it would be better to get rid of caching code in mcap.c?
>>
>> I can't understand this, can you explain this more?. As I see, MCAP
>> should do the catching because it holds all the information about the
>> channels that are connected and can cache it, HDP could not do this
>> without MCAP. More over, if in the future more profiles use MCAP they
>> may want to cache too.
>
> I do have the opinion that caching in mcap.c is more complicated than it could be.
My opinion is that caching in MCAP is the correct way because MCAP is
dealing with sending and receiving commands and need to retain certain
structures that are necessary for it, this structures do not concern
to HDP, only concerns to MCAP and should be MCAP who stores (caches)
them.
> I'd have used a single list with a CLOSED flag. Also, there is some confusion in nomenclature (mcap_uncache_mcl() recovers from the cache, while mcl_uncached_cb callback means that MCL has been invalidated). This is source of bugs like this one.
This is a different issue. Probably the nomenclature is not as clear
as it should be but I think that this is not the point here,
nevertheless we can change it too.
>
> At the very least, cached MCLs should have mcl->cb zeroed. if MCAP level caches MCLs, very well, but HDP level should be asked to set callbacks and user data again, as if it were a new MCL. (Actually, mcl_reconnected in hdp.c seems to do that; not sure why it did not avoid the particular bug that is under discussion.)
It is supposed that if HDP (or any other profile using MCAP) wants to
use cache, it should keep its variables all the time that it is
interested in caching them. If the profile using MCAP don't want to
keep its state should tell MCAP that it is not interested in caching
(as the patch in this thread does) because it is deleting its own
state.
Regards.
Jose
^ permalink raw reply
* Re: [RFC] Bluetooth Low energy support
From: Ville Tervo @ 2010-11-10 12:51 UTC (permalink / raw)
To: ext Anderson Lizardo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <AANLkTi=iCXfycMzHbgwS3jbvCxJvzVh97iSDRf4WhQeQ@mail.gmail.com>
On Wed, Nov 10, 2010 at 11:46:47AM +0100, ext Anderson Lizardo wrote:
> On Wed, Nov 10, 2010 at 2:20 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> > On Tue, Nov 09, 2010 at 05:25:10PM +0100, ext Anderson Lizardo wrote:
> >> While doing tests with your most recent trees (using devel HW from
> >> TI), I'm getting consistent panic on the following test:
> >>
> >> (dev1) hciconfig hciX leadv
> >> (dev2) hcitool -i hciX lecc <dev2_addr>
> >
> > Did you have server listening on dev 2 on ATT socket?
>
> I had no bluetoothd running on any side (not sure if that was your question).
That was actually my question :) Thanks.
--
VIlle
^ permalink raw reply
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Elvis Pfützenreuter @ 2010-11-10 12:40 UTC (permalink / raw)
To: Jose Antonio Santos Cadenas; +Cc: linux-bluetooth
In-Reply-To: <AANLkTim=3tZWao0Kzqq8nr4Z=dOrXL_aK76Ernx=UqQS@mail.gmail.com>
> Hi Elvis,
>
> 2010/11/10 Elvis Pfützenreuter <epx@signove.com>:
>>>> ---
>>>> health/hdp.c | 1 +
>>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/health/hdp.c b/health/hdp.c
>>>> index 1eba8e1..d361b27 100644
>>>> --- a/health/hdp.c
>>>> +++ b/health/hdp.c
>>>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>>>> if (!hdp_device->mcl)
>>>> return;
>>>>
>>>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>>>> mcap_mcl_unref(hdp_device->mcl);
>>>> hdp_device->mcl = NULL;
>>>> hdp_device->mcl_conn = FALSE;
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>
>>> Please Elvis, try this solution and tell us if it fix the segfault problem.
>>
>> Yes, it seems to have fixed the problem. And far cleaner :)
>>
>> I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
>
> This function doesn't disable the caching in general, it just closes
> this MCL without catching it, but caching is still active for all the
> other MCL's. Additionally, nothing happens if mcap_close_mcl is called
> more than once, so this patch takes care of this too.
Indeed, MCL disconnection and hdp_device destruction are different moments. Still fulfilling the caffeine quota for this morning :P
>>
>> So, perhaps it would be better to get rid of caching code in mcap.c?
>
> I can't understand this, can you explain this more?. As I see, MCAP
> should do the catching because it holds all the information about the
> channels that are connected and can cache it, HDP could not do this
> without MCAP. More over, if in the future more profiles use MCAP they
> may want to cache too.
I do have the opinion that caching in mcap.c is more complicated than it could be. I'd have used a single list with a CLOSED flag. Also, there is some confusion in nomenclature (mcap_uncache_mcl() recovers from the cache, while mcl_uncached_cb callback means that MCL has been invalidated). This is source of bugs like this one.
At the very least, cached MCLs should have mcl->cb zeroed. if MCAP level caches MCLs, very well, but HDP level should be asked to set callbacks and user data again, as if it were a new MCL. (Actually, mcl_reconnected in hdp.c seems to do that; not sure why it did not avoid the particular bug that is under discussion.)
^ permalink raw reply
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Jose Antonio Santos Cadenas @ 2010-11-10 11:49 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
In-Reply-To: <126814B4-25DA-4BA4-A0B8-0E5D57001EF7@signove.com>
Hi Elvis,
2010/11/10 Elvis Pfützenreuter <epx@signove.com>:
>>> ---
>>> health/hdp.c | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/health/hdp.c b/health/hdp.c
>>> index 1eba8e1..d361b27 100644
>>> --- a/health/hdp.c
>>> +++ b/health/hdp.c
>>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>>> if (!hdp_device->mcl)
>>> return;
>>>
>>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>>> mcap_mcl_unref(hdp_device->mcl);
>>> hdp_device->mcl = NULL;
>>> hdp_device->mcl_conn = FALSE;
>>> --
>>> 1.7.1
>>>
>>>
>>
>> Please Elvis, try this solution and tell us if it fix the segfault problem.
>
> Yes, it seems to have fixed the problem. And far cleaner :)
>
> I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
This function doesn't disable the caching in general, it just closes
this MCL without catching it, but caching is still active for all the
other MCL's. Additionally, nothing happens if mcap_close_mcl is called
more than once, so this patch takes care of this too.
>
> The only place hdp.c calls mcap_close_mcl(cache=TRUE) is when mcap_mcl_set_cb() fails, which seems "impossible", because it only depends on valid parameters to succeed.
This is because HDP wants to cache as long as possible.
>
> So, perhaps it would be better to get rid of caching code in mcap.c?
I can't understand this, can you explain this more?. As I see, MCAP
should do the catching because it holds all the information about the
channels that are connected and can cache it, HDP could not do this
without MCAP. More over, if in the future more profiles use MCAP they
may want to cache too.
Regards
^ permalink raw reply
* [PATCH] Fix pull phonebook reply if filter not set
From: Lukasz Pawlik @ 2010-11-10 11:43 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lukasz Pawlik
According to the PBAP specification if filter is not set or is set to
0x00000000 in the application parameters header all attributes of the vCard
should be returned. Previously only mandatory attributes were returned in
phonebook pull reply. This patch fix this and now all currently supported
vCards attributes will be returned.
---
plugins/vcard.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/plugins/vcard.c b/plugins/vcard.c
index 41f9fbd..3f69189 100644
--- a/plugins/vcard.c
+++ b/plugins/vcard.c
@@ -457,10 +457,16 @@ static void vcard_printf_end(GString *vcards)
void phonebook_add_contact(GString *vcards, struct phonebook_contact *contact,
uint64_t filter, uint8_t format)
{
- if (format == FORMAT_VCARD30)
+ if (format == FORMAT_VCARD30 && filter)
filter |= (FILTER_VERSION | FILTER_FN | FILTER_N | FILTER_TEL);
- else if (format == FORMAT_VCARD21)
+ else if (format == FORMAT_VCARD21 && filter)
filter |= (FILTER_VERSION | FILTER_N | FILTER_TEL);
+ else
+ filter = (FILTER_VERSION | FILTER_UID | FILTER_N | FILTER_FN |
+ FILTER_TEL | FILTER_EMAIL | FILTER_ADR |
+ FILTER_BDAY | FILTER_NICKNAME | FILTER_URL |
+ FILTER_PHOTO | FILTER_ORG | FILTER_ROLE |
+ FILTER_TITLE | FILTER_X_IRMC_CALL_DATETIME);
vcard_printf_begin(vcards, format);
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Elvis Pfützenreuter @ 2010-11-10 11:36 UTC (permalink / raw)
To: Jose Antonio Santos Cadenas; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimoQRAHXJ65xa1-TMKGUfLujw8f9MhzxrxPXHmY@mail.gmail.com>
>> ---
>> health/hdp.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/health/hdp.c b/health/hdp.c
>> index 1eba8e1..d361b27 100644
>> --- a/health/hdp.c
>> +++ b/health/hdp.c
>> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
>> if (!hdp_device->mcl)
>> return;
>>
>> + mcap_close_mcl(hdp_device->mcl, FALSE);
>> mcap_mcl_unref(hdp_device->mcl);
>> hdp_device->mcl = NULL;
>> hdp_device->mcl_conn = FALSE;
>> --
>> 1.7.1
>>
>>
>
> Please Elvis, try this solution and tell us if it fix the segfault problem.
Yes, it seems to have fixed the problem. And far cleaner :)
I hadn't proposed a lookalike solution because this seems to disable MCL caching completely. HDP already calls mcap_close_mcl(cache=FALSE) when takes the initiative of closing the MCL; this patch takes care of remaining case.
The only place hdp.c calls mcap_close_mcl(cache=TRUE) is when mcap_mcl_set_cb() fails, which seems "impossible", because it only depends on valid parameters to succeed.
So, perhaps it would be better to get rid of caching code in mcap.c?
^ permalink raw reply
* RE: [PATCH 1/4] Sim Access Profile API
From: Waldemar.Rymarkiewicz @ 2010-11-10 11:12 UTC (permalink / raw)
To: marcel; +Cc: linux-bluetooth, suraj, johan.hedberg, joakim.xj.ceder
In-Reply-To: <1289367190.9615.235.camel@aeonflux>
Hi Marcel,=20
>> + void Disable()
>> +
>> + Shudown SAP server and remove the SDP record.
>> +
>> + Possible errors: org.bluez.Error.Failed
>
>I don't like this. If you have properties then just changing=20
>the property should be enough. So a SetProperty is more appropriate.
I see another option to get rid of 'Enabled' property and leave the methods=
. What would you say on that?
>> +
>> + void Disconnect(boolean type)
>> +
>> + Disconnect SAP client from the server.=20
>The 'type'
>> + parameter indicates disconnection type.
>> +
>> + True - gracefull disconnection
>> + False - immediate disconnection
>> +
>> + Possible errors: org.bluez.Error.Failed
>
>I don't like this style of method names at all. Using method=20
>names like GracefulDisconnect and ImmediateDisconnect would be better.
That's fine.
>However I am not sure we should differentiate here at all. We=20
>should always to the graceful disconnect. What will the=20
>immediate disconnect bring us?
That's actually intended for testing only. One of PTS test cases expects th=
e tester to trigger immediate disconnect.
In practce, it is only used when connection to sim card is lost, but this i=
s obviously done internally.
Thanks,
/Waldek=
^ permalink raw reply
* Re: [RFC] Bluetooth Low energy support
From: Anderson Lizardo @ 2010-11-10 10:46 UTC (permalink / raw)
To: Ville Tervo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101110062030.GE20384@null>
On Wed, Nov 10, 2010 at 2:20 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> On Tue, Nov 09, 2010 at 05:25:10PM +0100, ext Anderson Lizardo wrote:
>> While doing tests with your most recent trees (using devel HW from
>> TI), I'm getting consistent panic on the following test:
>>
>> (dev1) hciconfig hciX leadv
>> (dev2) hcitool -i hciX lecc <dev2_addr>
>
> Did you have server listening on dev 2 on ATT socket?
I had no bluetoothd running on any side (not sure if that was your question).
>> I attached two logs. One is from dev1 machine (which has the oops),
>> the other is from the dev2 machine.
>
> I'll try to fix this today.
Thanks,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH] Fix segfault in HDP during device re-creation
From: Jose Antonio Santos Cadenas @ 2010-11-10 9:49 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jose Antonio Santos Cadenas
In-Reply-To: <1289382461-10510-1-git-send-email-santoscadenas@gmail.com>
2010/11/10 Jose Antonio Santos Cadenas <santoscadenas@gmail.com>:
> ---
> health/hdp.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/health/hdp.c b/health/hdp.c
> index 1eba8e1..d361b27 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
> if (!hdp_device->mcl)
> return;
>
> + mcap_close_mcl(hdp_device->mcl, FALSE);
> mcap_mcl_unref(hdp_device->mcl);
> hdp_device->mcl = NULL;
> hdp_device->mcl_conn = FALSE;
> --
> 1.7.1
>
>
Please Elvis, try this solution and tell us if it fix the segfault problem.
Regards
^ permalink raw reply
* [PATCH] Fix segfault in HDP during device re-creation
From: Jose Antonio Santos Cadenas @ 2010-11-10 9:47 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jose Antonio Santos Cadenas
In-Reply-To: <1289358915-6612-1-git-send-email-epx@signove.com>
---
health/hdp.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/health/hdp.c b/health/hdp.c
index 1eba8e1..d361b27 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -259,6 +259,7 @@ static void device_unref_mcl(struct hdp_device *hdp_device)
if (!hdp_device->mcl)
return;
+ mcap_close_mcl(hdp_device->mcl, FALSE);
mcap_mcl_unref(hdp_device->mcl);
hdp_device->mcl = NULL;
hdp_device->mcl_conn = FALSE;
--
1.7.1
^ permalink raw reply related
* Re: Ubuntu 10.04 - BlueZ 4.60 - Console-only BlueZ setup
From: elroy @ 2010-11-10 8:11 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <930b6b5d0fcdfd10087260af984b94ac@libresoft.es>
José Antonio Santos Cadenas wrote:
> Hi Elroy,
>
>
>
> Did you try simple-agent? It is a python scrypt in the bluez
> test/folder that doesn't require any gui. I don't if Ubuntu installs it,
> but if you download the source code from the web page or the git repo
> you will find it. Probably with this agent you will be able to pair your
> cell phone.
>
>
>>
>>I have been documenting my progress so far, to try to aid others
>>following my path - this may be useful to elaborate on what I am doing
>>and have achieved so far.
>>
>>
>>Regards,
>>
>>Elroy Liddington.
Cheers Jose.
I'll add this gem to the page I'm documenting this on.
Looks like those other scripts would be useful too.
How would I be able to help in regards to knocking up a basic BlueZ MAN
page etc so that others could at least be pointed in the right direction
on where to go for help/docs?
Regards,
Elroy Liddington.
^ permalink raw reply
* Re: [PATCH] [RFC] Fix HDP-related segfault upon device recreation
From: Jose Antonio Santos Cadenas @ 2010-11-10 8:02 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
In-Reply-To: <1289358915-6612-1-git-send-email-epx@signove.com>
Hi Elvis,
2010/11/10 Elvis Pfützenreuter <epx@signove.com>:
> When device is removed and paired again, hdp_device is destroyed
> but a cached mcap_mcl may retain a pointer in mcl->cb->user_data.
> This patch ensures that such MCL is destroyed.
>
> There is probably a better solution to this e.g. changing cache
> strategy for accepted connections. A loop can be removed if 1:1
> relationship between hdp_device and MCL is guaranteed.
> ---
> health/hdp.c | 1 +
> health/mcap.c | 23 ++++++++++++++++++++++-
> health/mcap_lib.h | 2 ++
> 3 files changed, 25 insertions(+), 1 deletions(-)
>
> diff --git a/health/hdp.c b/health/hdp.c
> index b141fe7..44ebe75 100644
> --- a/health/hdp.c
> +++ b/health/hdp.c
> @@ -277,6 +277,7 @@ static void free_health_device(struct hdp_device *device)
> }
>
> device_unref_mcl(device);
> + mcap_invalidate_by_user_data(device->hdp_adapter->mi, device);
>
> g_free(device);
> }
> diff --git a/health/mcap.c b/health/mcap.c
> index cb7b74c..34ccdaa 100644
> --- a/health/mcap.c
> +++ b/health/mcap.c
> @@ -938,6 +938,27 @@ gboolean mcap_mcl_set_cb(struct mcap_mcl *mcl, gpointer user_data,
> return TRUE;
> }
>
> +static gint cmp_mcl_user_data(gconstpointer a, gconstpointer b)
> +{
> + const struct mcap_mcl *mcl = a;
> + gconstpointer user_data = b;
> +
> + if (mcl->cb)
> + if (mcl->cb->user_data == user_data)
> + return 0;
> +
> + return 1;
> +}
> +
> +void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data)
> +{
> + GSList *l;
> +
> + while ((l = g_slist_find_custom(mi->cached, user_data,
> + cmp_mcl_user_data)))
> + mcap_close_mcl(l->data, FALSE);
> +}
> +
> void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr)
> {
> bacpy(addr, &mcl->addr);
> @@ -2143,4 +2164,4 @@ gboolean mcap_set_data_chan_mode(struct mcap_instance *mi, uint8_t mode,
>
> return bt_io_set(mi->dcio, BT_IO_L2CAP, err, BT_IO_OPT_MODE, mode,
> BT_IO_OPT_INVALID);
> -}
> \ No newline at end of file
> +}
> diff --git a/health/mcap_lib.h b/health/mcap_lib.h
> index 7740623..cc10c17 100644
> --- a/health/mcap_lib.h
> +++ b/health/mcap_lib.h
> @@ -172,6 +172,8 @@ void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr);
> struct mcap_mcl *mcap_mcl_ref(struct mcap_mcl *mcl);
> void mcap_mcl_unref(struct mcap_mcl *mcl);
>
> +void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data);
> +
> /* CSP operations */
>
> void mcap_enable_csp(struct mcap_instance *ms);
I'm not sure if this is the best way to face this issue. It seems that
this solution is a workaround to avoid the real problem. Let me have a
look and search for a better solution.
Regards.
^ permalink raw reply
* Re: [RFC] Bluetooth Low energy support
From: Ville Tervo @ 2010-11-10 6:20 UTC (permalink / raw)
To: ext Anderson Lizardo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <AANLkTina_HUT3SGEUMemVLrD4N3+Ni+xCsaC57whHDZG@mail.gmail.com>
Hi,
On Tue, Nov 09, 2010 at 05:25:10PM +0100, ext Anderson Lizardo wrote:
> Hi Ville,
>
> On Mon, Oct 18, 2010 at 9:02 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> > Hi,
> >
> > Here is v2 of bluetooth low energy patch set.
> > Changes from previous version.
>
> While doing tests with your most recent trees (using devel HW from
> TI), I'm getting consistent panic on the following test:
>
> (dev1) hciconfig hciX leadv
> (dev2) hcitool -i hciX lecc <dev2_addr>
Did you have server listening on dev 2 on ATT socket?
>
> I attached two logs. One is from dev1 machine (which has the oops),
> the other is from the dev2 machine.
I'll try to fix this today.
--
Ville
^ permalink raw reply
* Re: [PATCH v4] Bluetooth: btwilink driver
From: Marcel Holtmann @ 2010-11-10 6:08 UTC (permalink / raw)
To: pavan_savoy; +Cc: padovan, linux-bluetooth, linux-kernel
In-Reply-To: <1287525975-17187-1-git-send-email-pavan_savoy@ti.com>
Hi Pavan,
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 02deef4..8e0de9a 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -219,4 +219,14 @@ config BT_ATH3K
> Say Y here to compile support for "Atheros firmware download driver"
> into the kernel or say M to compile it as module (ath3k).
>
> +config BT_WILINK
> + tristate "Texas Instruments WiLink7 driver"
> + depends on TI_ST
> + help
> + This enables the Bluetooth driver for Texas Instrument's BT/FM/GPS
> + combo devices. This makes use of shared transport line discipline
> + core driver to communicate with the BT core of the combo chip.
> +
> + Say Y here to compile support for Texas Instrument's WiLink7 driver
> + into the kernel or say M to compile it as module.
> endmenu
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 71bdf13..f4460f4 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_BT_HCIBTSDIO) += btsdio.o
> obj-$(CONFIG_BT_ATH3K) += ath3k.o
> obj-$(CONFIG_BT_MRVL) += btmrvl.o
> obj-$(CONFIG_BT_MRVL_SDIO) += btmrvl_sdio.o
> +obj-$(CONFIG_BT_WILINK) += btwilink.o
>
> btmrvl-y := btmrvl_main.o
> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
> new file mode 100644
> index 0000000..218efd6
> --- /dev/null
> +++ b/drivers/bluetooth/btwilink.c
> @@ -0,0 +1,411 @@
> +/*
> + * Texas Instrument's Bluetooth Driver For Shared Transport.
> + *
> + * Bluetooth Driver acts as interface between HCI core and
> + * TI Shared Transport Layer.
> + *
> + * Copyright (C) 2009-2010 Texas Instruments
> + * Author: Raja Mani <raja_mani@ti.com>
> + * Pavan Savoy <pavan_savoy@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include <linux/ti_wilink_st.h>
> +
> +/* Bluetooth Driver Version */
> +#define VERSION "1.0"
> +
> +/* Number of seconds to wait for registration completion
> + * when ST returns PENDING status.
> + */
> +#define BT_REGISTER_TIMEOUT 6000 /* 6 sec */
> +
> +/**
> + * struct ti_st - driver operation structure
> + * @hdev: hci device pointer which binds to bt driver
> + * @reg_status: ST registration callback status
> + * @st_write: write function provided by the ST driver
> + * to be used by the driver during send_frame.
> + * @wait_reg_completion - completion sync between ti_st_open
> + * and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> + struct hci_dev *hdev;
> + char reg_status;
> + long (*st_write) (struct sk_buff *);
> + struct completion wait_reg_completion;
> +};
> +
> +static int reset;
> +
> +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> +static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
> +{
> + struct hci_dev *hdev;
> + hdev = hst->hdev;
please do this properly. Just write it like this:
struct hci_dev *hdev = hst->hdev;
+
> + /* Update HCI stat counters */
> + switch (pkt_type) {
> + case HCI_COMMAND_PKT:
> + hdev->stat.cmd_tx++;
> + break;
> +
> + case HCI_ACLDATA_PKT:
> + hdev->stat.acl_tx++;
> + break;
> +
> + case HCI_SCODATA_PKT:
> + hdev->stat.sco_tx++;
> + break;
> + }
> +}
> +
> +/* ------- Interfaces to Shared Transport ------ */
> +
> +/* Called by ST layer to indicate protocol registration completion
> + * status.ti_st_open() function will wait for signal from this
> + * API when st_register() function returns ST_PENDING.
> + */
> +static void st_registration_completion_cb(void *priv_data, char data)
> +{
> + struct ti_st *lhst = priv_data;
> +
> + /* Save registration status for use in ti_st_open() */
> + lhst->reg_status = data;
> + /* complete the wait in ti_st_open() */
> + complete(&lhst->wait_reg_completion);
> +}
> +
> +/* Called by Shared Transport layer when receive data is
> + * available */
> +static long st_receive(void *priv_data, struct sk_buff *skb)
> +{
> + int err;
> + struct ti_st *lhst = priv_data;
I really prefer if the variable with the assignment comes first.
> + if (!skb)
> + return -EFAULT;
> +
> + if (!lhst) {
> + kfree_skb(skb);
> + return -EFAULT;
> + }
> +
> + skb->dev = (struct net_device *)lhst->hdev;
Don't do this cast. See the other drivers where we just use (void *)
cast.
> + /* Forward skb to HCI core layer */
> + err = hci_recv_frame(skb);
> + if (err) {
> + kfree_skb(skb);
> + BT_ERR("Unable to push skb to HCI core(%d)", err);
> + return err;
> + }
So first of all, I prefer if you check like this:
if (err < 0) {
And then second, you are double freeing the SKB here. The hci_recv_frame
will free the SKB in an error case.
> +
> + lhst->hdev->stat.byte_rx += skb->len;
> +
> + return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +/* protocol structure registered with shared transport */
> +static struct st_proto_s ti_st_proto = {
> + .type = ST_BT,
> + .recv = st_receive,
> + .reg_complete_cb = st_registration_completion_cb,
> + .priv_data = NULL,
> +};
Please don't bother with NULL assignment. It should not be needed.
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> + unsigned long timeleft;
> + struct ti_st *hst;
> + int err;
> +
> + BT_DBG("%s %p", hdev->name, hdev);
> +
> + /* provide contexts for callbacks from ST */
> + hst = hdev->driver_data;
> + ti_st_proto.priv_data = hst;
> +
> + err = st_register(&ti_st_proto);
> + if (err == -EINPROGRESS) {
> + /* Prepare wait-for-completion handler data structures.
> + * Needed to synchronize this and
> + * st_registration_completion_cb() functions.
> + */
> + init_completion(&hst->wait_reg_completion);
> +
> + /* Reset ST registration callback status flag , this value
> + * will be updated in ti_st_registration_completion_cb()
> + * function whenever it called from ST driver.
> + */
> + hst->reg_status = -EINPROGRESS;
> +
> + /* ST is busy with either protocol registration or firmware
> + * download. Wait until the registration callback is called
> + */
> + BT_DBG(" waiting for registration completion signal from ST");
> +
> + timeleft = wait_for_completion_timeout
> + (&hst->wait_reg_completion,
> + msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> + if (!timeleft) {
> + BT_ERR("Timeout(%d sec),didn't get reg "
> + "completion signal from ST",
> + BT_REGISTER_TIMEOUT / 1000);
> + return -ETIMEDOUT;
> + }
> +
> + /* Is ST registration callback called with ERROR status? */
> + if (hst->reg_status != 0) {
> + BT_ERR("ST registration completed with invalid "
> + "status %d", hst->reg_status);
> + return -EAGAIN;
> + }
> + err = 0;
> + } else if (err == -EPERM) {
> + BT_ERR("st_register failed %d", err);
> + return err;
> + }
> +
> + hst->st_write = ti_st_proto.write;
> + if (!hst->st_write) {
> + BT_ERR("undefined ST write function");
> +
> + /* Undo registration with ST */
> + err = st_unregister(ST_BT);
> + if (err)
> + BT_ERR("st_unregister() failed with error %d", err);
> +
> + hst->st_write = NULL;
> + return err;
> + }
> +
> + /* Registration with ST layer is successful,
> + * hardware is ready to accept commands from HCI core.
> + */
> + set_bit(HCI_RUNNING, &hdev->flags);
> +
> + return err;
> +}
I really don't like what you are doing here. So please use
test_and_set_bit and clear it in an error case.
Also you need to handle all error cases. Just not only two.
Where is the ti_st_proto.write coming from?
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> + int err;
> + struct ti_st *hst = hdev->driver_data;
> +
> + /* continue to unregister from transport */
> + err = st_unregister(ST_BT);
> + if (err)
> + BT_ERR("st_unregister() failed with error %d", err);
> +
> + hst->st_write = NULL;
> +
> + return err;
> +}
You need a test_and_clear_bit for HCI_RUNNING. There is a huge imbalance
here. Have you tested this with consecutive hciconfig hci0 up/down
executions actually?
> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> + struct hci_dev *hdev;
> + struct ti_st *hst;
> + long len;
> +
> + if (!skb)
> + return -ENOMEM;
Pointless check. The core will not call this function with a NULL
pointer SKB.
> +
> + hdev = (struct hci_dev *)skb->dev;
> + if (!hdev)
> + return -ENODEV;
Even this can't really happen. Have you seen such a case?
> + if (!test_bit(HCI_RUNNING, &hdev->flags))
> + return -EBUSY;
> +
> + hst = hdev->driver_data;
> +
> + /* Prepend skb with frame type */
> + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> + BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
> + skb->len);
> +
> + /* Insert skb to shared transport layer's transmit queue.
> + * Freeing skb memory is taken care in shared transport layer,
> + * so don't free skb memory here.
> + */
> + if (!hst->st_write) {
> + kfree_skb(skb);
> + BT_ERR(" Could not write to ST (st_write is NULL)");
> + return -EAGAIN;
> + }
I don't like these crappy checks on every packet. That is just stupid.
You have checked for st_write when open happens and you set the hdev to
HCI_RUNNING. Are you saying this could change during the lifetime of the
hdev? If so then you have a serious problem here.
> + len = hst->st_write(skb);
> + if (len < 0) {
> + kfree_skb(skb);
> + BT_ERR(" ST write failed (%ld)", len);
> + return -EAGAIN;
> + }
> +
> + /* ST accepted our skb. So, Go ahead and do rest */
> + hdev->stat.byte_tx += len;
> + ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> +
> + return 0;
> +}
What is the reason for this deferred stats update. That code looks
pretty much hackish to me.
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> + if (!hdev)
> + return;
> +
> + BT_DBG("%s", hdev->name);
> +
> + /* free ti_st memory */
> + kfree(hdev->driver_data);
> +
> + return;
> +}
What are you checking here for? Why do you think that hdev would not be
valid? This is what the btusb and btsdio drivers do:
static void btusb_destruct(struct hci_dev *hdev)
{
struct btusb_data *data = hdev->driver_data;
BT_DBG("%s", hdev->name);
kfree(data);
}
> +/* Creates new HCI device */
> +static int ti_st_register_dev(struct ti_st *hst)
> +{
> + int err;
> + struct hci_dev *hdev;
I prefer if err is last in the variable list.
> +
> + /* Initialize and register HCI device */
> + hdev = hci_alloc_dev();
> + if (!hdev)
> + return -ENOMEM;
> +
> + BT_DBG("hdev %p", hdev);
> +
> + hst->hdev = hdev;
> + hdev->bus = HCI_UART;
> + hdev->driver_data = hst;
> + hdev->open = ti_st_open;
> + hdev->close = ti_st_close;
> + hdev->flush = NULL;
Please implement a flush callback.
> + hdev->send = ti_st_send_frame;
> + hdev->destruct = ti_st_destruct;
> + hdev->owner = THIS_MODULE;
> +
> + if (reset)
> + set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
Why do you need this? This should only be crappy devices. Something like
Bluetooth 1.0b old devices.
> + err = hci_register_dev(hdev);
> + if (err < 0) {
> + BT_ERR("Can't register HCI device error %d", err);
> + hci_free_dev(hdev);
> + return err;
> + }
> +
> + BT_DBG(" HCI device registered (hdev %p)", hdev);
> + return 0;
> +}
> +
> +
No double empty lines please.
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> + int err;
> + static struct ti_st *hst;
See above.
> +
> + BT_DBG(" Bluetooth Driver Version %s", VERSION);
This should be in the module_init function. And should be a BT_INFO and
be precise what driver this actually this.
> +
> + hst = kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> + if (!hst)
> + return -ENOMEM;
> +
> + /* Expose "hciX" device to user space */
> + err = ti_st_register_dev(hst);
> + if (err) {
> + kfree(hst);
> + return err;
> + }
Is this ti_st_register device use anywhere else. Then please just
include that code in here to make this clear. All other drivers do all
the work in their probe() callback.
> +
> + dev_set_drvdata(&pdev->dev, hst);
> + return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> + struct ti_st *hst;
> + struct hci_dev *hdev;
> +
> + hst = dev_get_drvdata(&pdev->dev);
Here I would prefer this:
struct ti_st *hst = dev_get_drvdata(&pdev->dev);
> +
> + if (!hst)
> + return -EFAULT;
> +
> + /* Deallocate local resource's memory */
> + hdev = hst->hdev;
That comment doesn't match what you are doing here.
> +
> + if (!hdev) {
> + BT_ERR("Invalid hdev memory");
> + kfree(hst);
> + return -EFAULT;
> + }
No need to check for hdev here. If probe fails, then remove should never
be called, right?
And just to be safe you might wanna add this:
dev_set_drvdata(&pdev->dev, NULL);
> +
> + ti_st_close(hdev);
> + hci_unregister_dev(hdev);
> + /* Free HCI device memory */
> + hci_free_dev(hdev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver btwilink_driver = {
> + .probe = bt_ti_probe,
> + .remove = bt_ti_remove,
> + .driver = {
> + .name = "btwilink",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init bt_drv_init(void)
> +{
> + long ret;
> +
> + ret = platform_driver_register(&btwilink_driver);
> + if (ret != 0) {
> + BT_ERR("btwilink platform driver registration failed");
> + return ret;
> + }
> + return 0;
> +}
please just do like we do with all other drivers;
BT_INFO(...)
return platform_driver_register(&btwilink_driver);
> +
> +static void __exit bt_drv_exit(void)
> +{
> + platform_driver_unregister(&btwilink_driver);
> +}
> +
> +module_init(bt_drv_init);
> +module_exit(bt_drv_exit);
And this should be btwilink_init and btwilink_exit. Please don't try to
grab some generic namespace.
> +
> +/* ------ Module Info ------ */
> +
> +module_param(reset, bool, 0644);
> +MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
As mentioned above, that one seems wrong to me. You need to know what
your device supports. And by default it should allow sending HCI_Reset
at init. If not, then just that quirk. No need for module parameter
here.
> +MODULE_AUTHOR("Raja Mani <raja_mani@ti.com>");
> +MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2] Bluetooth: Automate remote name requests
From: Marcel Holtmann @ 2010-11-10 5:44 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth, Johan Hedberg
In-Reply-To: <1288282824-30736-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> In Bluetooth there are no automatic updates of remote device names when
> they get changed on the remote side. Instead, it is a good idea to do a
> manual name request when a new connection gets created (for whatever
> reason) since at this point it is very cheap (no costly baseband
> connection creation needed just for the sake of the name request).
>
> So far userspace has been responsible for this extra name request but
> tighter control is needed in order not to flood Bluetooth controllers
> with two many commands during connection creation. It has been shown
> that some controllers simply fail to function correctly if they get too
> many (almost) simultaneous commands during connection creation. The
> simplest way to acheive better control of these commands is to move
> their sending completely to the kernel side.
>
> This patch inserts name requests into the sequence of events that the
> kernel performs during connection creation. It does this after the
> remote features have been successfully requested and before any pending
> authentication requests are performed. The code will work sub-optimally
> with userspace versions that still do the name requesting themselves (it
> shouldn't break anything though) so it is recommended to combine this
> with a userspace software version that doesn't have automated name
> requests.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
> v2 makes sure that the name request gets always sent instead of just
> doing it for cases when we're also going to request authentication.
>
> net/bluetooth/hci_event.c | 151 ++++++++++++++++++++++++++++++++-------------
> 1 files changed, 107 insertions(+), 44 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 84093b0..6e770e8 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -677,9 +677,51 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
> hci_dev_unlock(hdev);
> }
>
> +static void request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> +{
> + struct hci_cp_auth_requested cp;
> + struct hci_conn *conn;
> +
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> + if (!conn)
> + return;
> +
> + if (conn->state != BT_CONFIG || !conn->out)
> + return;
> +
> + if (conn->sec_level == BT_SECURITY_SDP)
> + return;
> +
> + /* Only request authentication for SSP connections or non-SSP
> + * devices with sec_level HIGH */
> + if (!(hdev->ssp_mode > 0 && conn->ssp_mode > 0) &&
> + conn->sec_level != BT_SECURITY_HIGH)
> + return;
> +
> + cp.handle = __cpu_to_le16(conn->handle);
> + hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
> +}
please split this patch into two. One creating the common function for
the authentication requested and one for adding the name request.
My brain actually core dumps when following the logic and making sure
that it is still correct. I pretty sure it is correct, but the whole
patch is damn hard to review.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()
From: Marcel Holtmann @ 2010-11-10 5:39 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Ville Tervo, linux-bluetooth@vger.kernel.org
In-Reply-To: <20101105143711.GB9116@vigoh>
Hi Gustavo,
> > On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > > It also have to change the name of the function to
> > > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > >
> > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > > ---
> > > net/bluetooth/l2cap.c | 17 ++++++-----------
> > > 1 files changed, 6 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > > index 6f931cc..3d48867 100644
> > > --- a/net/bluetooth/l2cap.c
> > > +++ b/net/bluetooth/l2cap.c
> > > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> > > }
> > >
> > > /* ---- Socket interface ---- */
> > > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > {
> > > struct sock *sk;
> > > struct hlist_node *node;
> > > +
> > > + write_lock_bh(&l2cap_sk_list.lock);
> >
> > Code is only reading so read_lock_bh would be enough?
>
> Sure, I didn't looked to that, I just keept the same code that we were
> using before. I'll fix it.
we might also not just bother with read/write locks. Since they are not
always the right thing to do. In a lot of cases a pure spinlock is just
better. And in case of Bluetooth I think we would be just fine with
using a pure spinlock. You might run some tests with this.
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap
From: Marcel Holtmann @ 2010-11-10 5:36 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
In-Reply-To: <1288780365-32099-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
> Yet another version of patches fixing kernel crash in RFCOMM / L2CAP.
> *v4: taken Gustavo comments about timer HZ -> HZ/5
>
> Do not delete l2cap channel and socket sk when sk is owned by user.
> To delete l2cap channel standard timer is used.
>
> lock_sock and release_sock do not hold a normal spinlock directly but
> instead hold the owner field. This means bh_lock_sock can still execute
> even if the socket is "locked". More info can be found here:
> http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
>
> When sending following sequence:
> ...
> No. Time Source Destination Protocol Info
> 89 1.951202 RFCOMM Rcvd DISC DLCI=20
> 90 1.951324 RFCOMM Sent UA DLCI=20
> 91 1.959381 HCI_EVT Number of Completed Packets
> 92 1.966461 RFCOMM Rcvd DISC DLCI=0
> 93 1.966492 L2CAP Rcvd Disconnect Request
> 94 1.972595 L2CAP Sent Disconnect Response
>
> ...
>
> krfcommd kernel thread is preempted with l2cap tasklet which remove l2cap_conn
> (L2CAP connection handler structure). Then rfcomm thread tries to send RFCOMM
> UA which is reply to RFCOMM DISC and when de-referencing l2cap_conn crash
> happens.
so I assume you have tested this extensively with various RFCOMM corner
cases like incoming RFCOMM. Since a lot of profiles require proper
disconnects and we have to ensure that our reference counting is
correct.
Other then that it seems fine to me.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/4] Sim Access Profile API
From: Marcel Holtmann @ 2010-11-10 5:33 UTC (permalink / raw)
To: Waldemar Rymarkiewicz
Cc: linux-bluetooth, suraj, Johan Hedberg, joakim.xj.ceder
In-Reply-To: <1288791271-13857-2-git-send-email-waldemar.rymarkiewicz@tieto.com>
Hi Waldemar,
> New API for Sim Access Profile.
> ---
> Makefile.am | 2 +-
> doc/sap-api.txt | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 58 insertions(+), 1 deletions(-)
> create mode 100644 doc/sap-api.txt
>
> diff --git a/Makefile.am b/Makefile.am
> index 873f2df..e1183de 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -352,7 +352,7 @@ EXTRA_DIST += doc/manager-api.txt \
> doc/service-api.txt doc/agent-api.txt doc/attribute-api.txt \
> doc/serial-api.txt doc/network-api.txt \
> doc/input-api.txt doc/audio-api.txt doc/control-api.txt \
> - doc/hfp-api.txt doc/assigned-numbers.txt
> + doc/hfp-api.txt doc/assigned-numbers.txt doc/sap-api.txt
>
> AM_YFLAGS = -d
>
> diff --git a/doc/sap-api.txt b/doc/sap-api.txt
> new file mode 100644
> index 0000000..7951f56
> --- /dev/null
> +++ b/doc/sap-api.txt
> @@ -0,0 +1,57 @@
> +BlueZ D-Bus Sim Access Profile API description
> +***********************************
> +
> +Copyright (C) 2010 ST-Ericsson SA
> +
> +
> +Sim Access Profile hierarchy
> +============================
> +
> +Service org.bluez
> +Interface org.bluez.SimAccess
> +Object path [variable prefix]/{hci0,hci1,...}
> +
> +Methods void Enable()
> +
> + Start up SAP server and register SDP record for it.
> +
> + Possible errors: org.bluez.Error.Failed
> +
> + void Disable()
> +
> + Shudown SAP server and remove the SDP record.
> +
> + Possible errors: org.bluez.Error.Failed
I don't like this. If you have properties then just changing the
property should be enough. So a SetProperty is more appropriate.
> +
> + void Disconnect(boolean type)
> +
> + Disconnect SAP client from the server. The 'type'
> + parameter indicates disconnection type.
> +
> + True - gracefull disconnection
> + False - immediate disconnection
> +
> + Possible errors: org.bluez.Error.Failed
I don't like this style of method names at all. Using method names like
GracefulDisconnect and ImmediateDisconnect would be better.
However I am not sure we should differentiate here at all. We should
always to the graceful disconnect. What will the immediate disconnect
bring us?
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Move gattrib source files to src directory
From: Marcel Holtmann @ 2010-11-10 5:26 UTC (permalink / raw)
To: Johan Hedberg; +Cc: Claudio Takahasi, linux-bluetooth
In-Reply-To: <20101105050602.GD25270@jh-x301>
Hi Johan,
> > gattrib related functions will be required during the device creation
> > for GATT enabled devices(BR/EDR and LE). Primary service discovery is
> > a pre-condition to probe the GATT device driver.
> > ---
> > Makefile.am | 7 +-
> > attrib/att.c | 764 ------------------------------------------------------
> > attrib/att.h | 206 ---------------
> > attrib/gatt.c | 113 --------
> > attrib/gatt.h | 43 ---
> > attrib/gattrib.c | 535 --------------------------------------
> > attrib/gattrib.h | 72 -----
> > src/att.c | 764 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > src/att.h | 206 +++++++++++++++
> > src/gatt.c | 113 ++++++++
> > src/gatt.h | 43 +++
> > src/gattrib.c | 535 ++++++++++++++++++++++++++++++++++++++
> > src/gattrib.h | 72 +++++
> > 13 files changed, 1736 insertions(+), 1737 deletions(-)
> > delete mode 100644 attrib/att.c
> > delete mode 100644 attrib/att.h
> > delete mode 100644 attrib/gatt.c
> > delete mode 100644 attrib/gatt.h
> > delete mode 100644 attrib/gattrib.c
> > delete mode 100644 attrib/gattrib.h
> > create mode 100644 src/att.c
> > create mode 100644 src/att.h
> > create mode 100644 src/gatt.c
> > create mode 100644 src/gatt.h
> > create mode 100644 src/gattrib.c
> > create mode 100644 src/gattrib.h
>
> I'll wait a little bit with this one. I agree that the gattrib
> funcionality needs to be available within the core daemon, but does that
> necessarily mean that the source files for need to be in src? It'd be
> good to get some comment from Marcel about this too.
I think that this change is actually bad. Since we have non-recursive
build system, we are not bound to have code under the same directory.
So my advise would be to not do this and just link the attrib/* code
into the bluetoothd.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 0/9] Fixing DBus error system in BlueZ
From: Marcel Holtmann @ 2010-11-10 5:23 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20101108173133.GA18818@vigoh>
Hi Gustavo,
> > On Mon, Nov 08, 2010, Gustavo F. Padovan wrote:
> > > Here are some patches that try to fix the mess of reporting error to
> > > DBus inside BlueZ. It follows the oFono and ConnMan error system.
> > >
> > > The goal is to get ride of any directly call to g_dbus_create_error()
> > > inside bluez code, changing that to __btd_error_*. This patch set
> > > doesn't fix all of them yet, but is a very good start. Please review.
> > >
> > >
> > > Gustavo F. Padovan (9):
> > > Create __btd_error_invalid_args()
> > > Add __btd_error_already_exists()
> > > Add __btd_error_not_supported()
> > > Add __btd_error_not_connected()
> > > Add __btd_error_in_progress()
> > > Add __btd_error_not_available()
> > > Add __btd_error_busy()
> > > Add __btd_error_does_not_exist()
> > > Add __btd_error_not_authorized()
> >
> > The patches seem fine to me, but before pushing upstream I'd like to
> > understand the reason for prefixing these with with __btd instead of
> > btd. What's the criteria used to decide what to use and when and why is
> > __btd the correct choice for these new functions? My first guess would
> > have been that __btd is for things only accessible by the core-daemon
> > whereas btd is for functions exported to plugins, but that doesn't seem
> > to be the case with your patches since many of these __btd functions get
> > called from plugins.
>
> I just followed oFono and ConnMan on this. That is the reason and I
> didn't asked myself why have a __ in this case.. But I see your point.
> Do you think that change that to btd_error_* will fit better inside
> BlueZ? I can change that then.
so within ConnMan and oFono we make a difference between public symbols
that are reachable from within plugins and other which are not.
In general btd_ should be public symbols available to plugins and __btd_
for internal symbols that are no available to plugins.
For builtin plugins that makes no difference of course, but this is not
about internal builtin plugins. It is for protecting against external
plugins to not allow access to internal details.
That said, bluetoothd is not linked with the case to hide certain
symbols anyway so that right now there is no real difference here.
Regards
Marcel
^ permalink raw reply
* Re: I think we should add GoepL2capPsm attribute in sdp.h
From: Marcel Holtmann @ 2010-11-10 5:19 UTC (permalink / raw)
To: hui li; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimThap03_iYvxYyw39HOkMRAzzkscKjGMwmY7dX@mail.gmail.com>
Hi Hui,
> Currently there is no GoepL2capPsm attribute value in sdp.h, I think
> we should add this to supprot GOEP v2.0.
> Maybe we can add
> #define SDP_ATTR_GOEP_L2CAP_PSM 0x0200
> in sdp.h.
please send a proper patch for this and provide a clear reference to it
since this is not part of the assigned numbers document.
Regards
Marcel
^ permalink raw reply
* [PATCH] [RFC] Fix HDP-related segfault upon device recreation
From: Elvis Pfützenreuter @ 2010-11-10 3:15 UTC (permalink / raw)
To: linux-bluetooth; +Cc: epx
When device is removed and paired again, hdp_device is destroyed
but a cached mcap_mcl may retain a pointer in mcl->cb->user_data.
This patch ensures that such MCL is destroyed.
There is probably a better solution to this e.g. changing cache
strategy for accepted connections. A loop can be removed if 1:1
relationship between hdp_device and MCL is guaranteed.
---
health/hdp.c | 1 +
health/mcap.c | 23 ++++++++++++++++++++++-
health/mcap_lib.h | 2 ++
3 files changed, 25 insertions(+), 1 deletions(-)
diff --git a/health/hdp.c b/health/hdp.c
index b141fe7..44ebe75 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -277,6 +277,7 @@ static void free_health_device(struct hdp_device *device)
}
device_unref_mcl(device);
+ mcap_invalidate_by_user_data(device->hdp_adapter->mi, device);
g_free(device);
}
diff --git a/health/mcap.c b/health/mcap.c
index cb7b74c..34ccdaa 100644
--- a/health/mcap.c
+++ b/health/mcap.c
@@ -938,6 +938,27 @@ gboolean mcap_mcl_set_cb(struct mcap_mcl *mcl, gpointer user_data,
return TRUE;
}
+static gint cmp_mcl_user_data(gconstpointer a, gconstpointer b)
+{
+ const struct mcap_mcl *mcl = a;
+ gconstpointer user_data = b;
+
+ if (mcl->cb)
+ if (mcl->cb->user_data == user_data)
+ return 0;
+
+ return 1;
+}
+
+void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data)
+{
+ GSList *l;
+
+ while ((l = g_slist_find_custom(mi->cached, user_data,
+ cmp_mcl_user_data)))
+ mcap_close_mcl(l->data, FALSE);
+}
+
void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr)
{
bacpy(addr, &mcl->addr);
@@ -2143,4 +2164,4 @@ gboolean mcap_set_data_chan_mode(struct mcap_instance *mi, uint8_t mode,
return bt_io_set(mi->dcio, BT_IO_L2CAP, err, BT_IO_OPT_MODE, mode,
BT_IO_OPT_INVALID);
-}
\ No newline at end of file
+}
diff --git a/health/mcap_lib.h b/health/mcap_lib.h
index 7740623..cc10c17 100644
--- a/health/mcap_lib.h
+++ b/health/mcap_lib.h
@@ -172,6 +172,8 @@ void mcap_mcl_get_addr(struct mcap_mcl *mcl, bdaddr_t *addr);
struct mcap_mcl *mcap_mcl_ref(struct mcap_mcl *mcl);
void mcap_mcl_unref(struct mcap_mcl *mcl);
+void mcap_invalidate_by_user_data(struct mcap_instance *mi, gpointer user_data);
+
/* CSP operations */
void mcap_enable_csp(struct mcap_instance *ms);
--
1.7.0.4
^ permalink raw reply related
* [PATCH] Check HealthApplication path before trying to destroy it
From: Elvis Pfützenreuter @ 2010-11-10 2:11 UTC (permalink / raw)
To: linux-bluetooth; +Cc: epx
---
health/hdp.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/health/hdp.c b/health/hdp.c
index 1eba8e1..b141fe7 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -376,6 +376,12 @@ static DBusMessage *manager_destroy_application(DBusConnection *conn,
l = g_slist_find_custom(applications, path, cmp_app);
+ if (!l)
+ return g_dbus_create_error(msg,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call, "
+ "no such application");
+
app = l->data;
applications = g_slist_remove(applications, app);
--
1.7.0.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox