* Re: [PATCH] TODO: ATT protocol transactions timeout
From: Johan Hedberg @ 2010-10-19 21:53 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth
In-Reply-To: <1287523632-5966-1-git-send-email-claudio.takahasi@openbossa.org>
Hi Claudio,
On Tue, Oct 19, 2010, Claudio Takahasi wrote:
> ---
> TODO | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
This one and the two other TODO patches have been pushed upstream.
Thanks.
Johan
^ permalink raw reply
* [PATCH] TODO: Define Auto Connection Establishment Procedure
From: Claudio Takahasi @ 2010-10-19 21:27 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1287523632-5966-1-git-send-email-claudio.takahasi@openbossa.org>
---
TODO | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/TODO b/TODO
index 948d4d0..c0a25f1 100644
--- a/TODO
+++ b/TODO
@@ -40,6 +40,20 @@ Low Energy
Priority: Medium
Complexity: C2
+- Define Auto Connection Establishment Procedure. Some profiles such as
+ Proximity requires an active link to identify path lost situation. It is
+ necessary to define how to manage connections, it seems that White List
+ is appropriated to address auto connections, however is not clear if the
+ this procedure shall be a profile specific detail or if the remote device
+ object can expose a property "WhiteList", maybe "Trusted" property can be
+ also used for this purpose. Another alternative is to define a method to
+ allow application to request/register the wanted scanning/connection
+ parameters. Before start this task, a RFC/PATCH shall be sent to the ML.
+ See Volume 3, Part C, section 9.3.5 for more information.
+
+ Priority: Medium
+ Complexity: C2
+
- Device Name Characteristic is a GAP characteristic for Low Energy. This
characteristic shall be integrated/used in the discovery procedure. The
ideia is to report the value of this characteristic using DeviceFound signals.
--
1.7.3.1
^ permalink raw reply related
* [PATCH] TODO: Advertising management
From: Claudio Takahasi @ 2010-10-19 21:27 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1287523632-5966-1-git-send-email-claudio.takahasi@openbossa.org>
---
TODO | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/TODO b/TODO
index d95932b..948d4d0 100644
--- a/TODO
+++ b/TODO
@@ -26,6 +26,20 @@ Low Energy
Priority: Medium
Complexity: C2
+- Advertising management. Adapter interface needs to be changed to manage
+ connection modes, adapter type and advertising policy. See Volume 3,
+ Part C, section 9.3. If Attribute Server is enabled the LE capable
+ adapter shall to start advertising. Further investigation is necessary
+ to define which connectable mode needs to be supported: Non-connectable,
+ directed connectable and undirected connectable. Basically, two connectable
+ scenarios shall be addressed:
+ 1. GATT client is disconnected, but intends to become a Peripheral to
+ receive indications/notifications.
+ 2. GATT server intends to accept connections.
+
+ Priority: Medium
+ Complexity: C2
+
- Device Name Characteristic is a GAP characteristic for Low Energy. This
characteristic shall be integrated/used in the discovery procedure. The
ideia is to report the value of this characteristic using DeviceFound signals.
--
1.7.3.1
^ permalink raw reply related
* [PATCH] TODO: ATT protocol transactions timeout
From: Claudio Takahasi @ 2010-10-19 21:27 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
---
TODO | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/TODO b/TODO
index b4682e8..d95932b 100644
--- a/TODO
+++ b/TODO
@@ -137,3 +137,10 @@ ATT/GATT
Priority: Low
Complexity: C2
+
+- gattrib needs to be extended to handle Attribute Protocol Transactions
+ timeout. See Volume 3, Part F, section 3.3.3 and Part G, section 4.14
+ for more information.
+
+ Priority: Low
+ Complexity: C2
--
1.7.3.1
^ permalink raw reply related
* [PATCH v3] Bluetooth: btwilink driver
From: pavan_savoy @ 2010-10-19 20:57 UTC (permalink / raw)
To: padovan, marcel; +Cc: linux-bluetooth, linux-kernel, Pavan Savoy
From: Pavan Savoy <pavan_savoy@ti.com>
v3 comments
Marcel, Gustavo, & list,
Please review this version of patch.
Anderson,
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 | 411 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 422 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..930e3d3
--- /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;
+
+ /* 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 = (struct ti_st *)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 = (struct ti_st *)priv_data;
+
+ if (!skb)
+ return -EFAULT;
+
+ if (!lhst) {
+ kfree_skb(skb);
+ return -EFAULT;
+ }
+
+ skb->dev = (struct net_device *)lhst->hdev;
+
+ /* 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;
+ }
+
+ 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,
+};
+
+/* 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;
+}
+
+/* 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;
+}
+
+static int ti_st_send_frame(struct sk_buff *skb)
+{
+ struct hci_dev *hdev;
+ struct ti_st *hst;
+ long len;
+
+ if (!skb)
+ return -ENOMEM;
+
+ hdev = (struct hci_dev *)skb->dev;
+ if (!hdev)
+ return -ENODEV;
+
+ if (!test_bit(HCI_RUNNING, &hdev->flags))
+ return -EBUSY;
+
+ hst = (struct ti_st *)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;
+ }
+
+ 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)
+{
+ if (!hdev)
+ return;
+
+ BT_DBG("%s", hdev->name);
+
+ /* free ti_st memory */
+ kfree(hdev->driver_data);
+
+ return;
+}
+
+/* Creates new HCI device */
+static int ti_st_register_dev(struct ti_st *hst)
+{
+ int err;
+ struct hci_dev *hdev;
+
+ /* 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;
+ 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);
+
+ 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;
+}
+
+
+static int bt_ti_probe(struct platform_device *pdev)
+{
+ int err;
+ static struct ti_st *hst;
+
+ BT_DBG(" Bluetooth Driver Version %s", VERSION);
+
+ 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;
+ }
+
+ 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);
+
+ if (!hst)
+ return -EFAULT;
+
+ /* Deallocate local resource's memory */
+ hdev = hst->hdev;
+
+ if (!hdev) {
+ BT_ERR("Invalid hdev memory");
+ kfree(hst);
+ return -EFAULT;
+ }
+
+ 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 -EPERM;
+ }
+ return 0;
+}
+
+static void __exit bt_drv_exit(void)
+{
+ platform_driver_unregister(&btwilink_driver);
+}
+
+module_init(bt_drv_init);
+module_exit(bt_drv_exit);
+
+/* ------ Module Info ------ */
+
+module_param(reset, bool, 0644);
+MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
+MODULE_AUTHOR("Raja Mani <raja_mani@ti.com>");
+MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
--
1.6.5
^ permalink raw reply related
* RE: [PATCH v3] Bluetooth: btwilink driver
From: Savoy, Pavan @ 2010-10-19 20:54 UTC (permalink / raw)
To: Gustavo F. Padovan
Cc: marcel@holtmann.org, linux-bluetooth@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20101019205403.GA2331@vigoh>
> -----Original Message-----
> From: Gustavo F. Padovan [mailto:pao@profusion.mobi] On Behalf Of Gustavo=
F.
> Padovan
> Sent: Tuesday, October 19, 2010 3:54 PM
> To: Savoy, Pavan
> Cc: marcel@holtmann.org; linux-bluetooth@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v3] Bluetooth: btwilink driver
>=20
> * Savoy, Pavan <pavan_savoy@ti.com> [2010-10-20 02:15:29 +0530]:
>=20
> >
> >
> >
> > > -----Original Message-----
> > > From: Gustavo F. Padovan [mailto:pao@profusion.mobi] On Behalf Of Gus=
tavo
> F.
> > > Padovan
> > > Sent: Tuesday, October 19, 2010 3:39 PM
> > > To: Savoy, Pavan
> > > Cc: marcel@holtmann.org; linux-bluetooth@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH v3] Bluetooth: btwilink driver
> > >
> > > * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-10-19 16:57:38 -0400]=
:
> > >
> > > > From: Pavan Savoy <pavan_savoy@ti.com>
> > > >
> > > > v3 comments
> > > >
> > > > Marcel, Gustavo, & list,
> > > > Please review this version of patch.
> > > >
> > > > Anderson,
> > > > 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 ke=
ep
> > > > it similar to other drivers
> > >
> > > Which drivers returns -EPERM to any kind of error? The are many reaso=
ns
> > > why the funcion can fail, and you want to give the best error report =
to
> the
> > > user. Use EPERM to all of them is just wrong.
> >
> > Yes, it can fail for plenty of reasons.
> > So I'll just return whatever I get from platform_driver_register.
> > Is this OK?
>=20
> Yes.
>=20
> >
> > > >type casting is maintained just to feel safe
> > > > and have style similar to other drivers.
> > >
> > > We don't need to feel safe here. Type cast actually can hide errors,
> > > only use them when you really need to cast, in many case here you don=
't.
> >
> > Ok, I can remove type casting.
> > I am not really for or against it...
>=20
> Yes, do that please. ;)
Done :)
> --
> Gustavo F. Padovan
> ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: btwilink driver
From: Gustavo F. Padovan @ 2010-10-19 20:54 UTC (permalink / raw)
To: Savoy, Pavan
Cc: marcel@holtmann.org, linux-bluetooth@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <19F8576C6E063C45BE387C64729E739404AA4E790D@dbde02.ent.ti.com>
* Savoy, Pavan <pavan_savoy@ti.com> [2010-10-20 02:15:29 +0530]:
>
>
>
> > -----Original Message-----
> > From: Gustavo F. Padovan [mailto:pao@profusion.mobi] On Behalf Of Gustavo F.
> > Padovan
> > Sent: Tuesday, October 19, 2010 3:39 PM
> > To: Savoy, Pavan
> > Cc: marcel@holtmann.org; linux-bluetooth@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH v3] Bluetooth: btwilink driver
> >
> > * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-10-19 16:57:38 -0400]:
> >
> > > From: Pavan Savoy <pavan_savoy@ti.com>
> > >
> > > v3 comments
> > >
> > > Marcel, Gustavo, & list,
> > > Please review this version of patch.
> > >
> > > Anderson,
> > > 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
> >
> > Which drivers returns -EPERM to any kind of error? The are many reasons
> > why the funcion can fail, and you want to give the best error report to the
> > user. Use EPERM to all of them is just wrong.
>
> Yes, it can fail for plenty of reasons.
> So I'll just return whatever I get from platform_driver_register.
> Is this OK?
Yes.
>
> > >type casting is maintained just to feel safe
> > > and have style similar to other drivers.
> >
> > We don't need to feel safe here. Type cast actually can hide errors,
> > only use them when you really need to cast, in many case here you don't.
>
> Ok, I can remove type casting.
> I am not really for or against it...
Yes, do that please. ;)
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* RE: [PATCH v3] Bluetooth: btwilink driver
From: Savoy, Pavan @ 2010-10-19 20:45 UTC (permalink / raw)
To: Gustavo F. Padovan
Cc: marcel@holtmann.org, linux-bluetooth@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20101019203913.GA2322@vigoh>
> -----Original Message-----
> From: Gustavo F. Padovan [mailto:pao@profusion.mobi] On Behalf Of Gustavo=
F.
> Padovan
> Sent: Tuesday, October 19, 2010 3:39 PM
> To: Savoy, Pavan
> Cc: marcel@holtmann.org; linux-bluetooth@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v3] Bluetooth: btwilink driver
>=20
> * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-10-19 16:57:38 -0400]:
>=20
> > From: Pavan Savoy <pavan_savoy@ti.com>
> >
> > v3 comments
> >
> > Marcel, Gustavo, & list,
> > Please review this version of patch.
> >
> > Anderson,
> > 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
>=20
> Which drivers returns -EPERM to any kind of error? The are many reasons
> why the funcion can fail, and you want to give the best error report to t=
he
> user. Use EPERM to all of them is just wrong.
Yes, it can fail for plenty of reasons.
So I'll just return whatever I get from platform_driver_register.
Is this OK?
> >type casting is maintained just to feel safe
> > and have style similar to other drivers.
>=20
> We don't need to feel safe here. Type cast actually can hide errors,
> only use them when you really need to cast, in many case here you don't.
Ok, I can remove type casting.
I am not really for or against it...
> --
> Gustavo F. Padovan
> ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: btwilink driver
From: Gustavo F. Padovan @ 2010-10-19 20:39 UTC (permalink / raw)
To: pavan_savoy; +Cc: marcel, linux-bluetooth, linux-kernel
In-Reply-To: <1287521858-15190-1-git-send-email-pavan_savoy@ti.com>
* pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-10-19 16:57:38 -0400]:
> From: Pavan Savoy <pavan_savoy@ti.com>
>
> v3 comments
>
> Marcel, Gustavo, & list,
> Please review this version of patch.
>
> Anderson,
> 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
Which drivers returns -EPERM to any kind of error? The are many reasons
why the funcion can fail, and you want to give the best error report to the
user. Use EPERM to all of them is just wrong.
>type casting is maintained just to feel safe
> and have style similar to other drivers.
We don't need to feel safe here. Type cast actually can hide errors,
only use them when you really need to cast, in many case here you don't.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: AVRCP 1.4 : Future on Target Role
From: Luiz Augusto von Dentz @ 2010-10-19 16:14 UTC (permalink / raw)
To: Sander van Grieken; +Cc: linux-bluetooth
In-Reply-To: <201010191147.34241.sander@outrightsolutions.nl>
Hi,
On Tue, Oct 19, 2010 at 12:47 PM, Sander van Grieken
<sander@outrightsolutions.nl> wrote:
> I am very much in favor of not directly depending on MPRIS, but instead letting
> applications registering themselves as a target. For two reasons:
>
> - AVRCP seems to be a superset of MPRIS (which is very limited IMO), and might have
> different semantics, especially w.r.t. event notifications. So we would limit ourselves to
> the intersecting subset of both technologies.
> - A separate AVRCP/TG <-> MPRIS bridge agent would still allow controlling all MPRIS-
> enabled players, so we can have both full implementation of the AVRCP spec, AND generic
> MPRIS support.
Sounds good to me, we can use Media API to register players as well.
>> > I am keen to receive your feedback with some ideas and thoughts to put
>> > my effort in right direction.
>> >
>> > Question:
>> > Is anyone working on AVRCP 1.4 Target role profile development?
>>
>> Joao Paulo (http://jprvita.wordpress.com/2010/07/22/avrcp-metadata/)
>
> Actually, the metadata work is not part of v1.4 of the spec, but 1.3
>
> Second, I have already added some boilerplate stuff (like a DBUS Connect method for RCP and
> some fixes for CT commands), but I've based on Joao's branch, so I have to wait until his
> stuff gets merged. Alternatively, I could rebase that stuff on HEAD, but that would overlap
> and conflict with Joao's stuff, so I'm hesitant to go there.
I hope to see this code being push upstream soon, I will try to
contact Joao Paulo to get an idea if the code is ready to be
reviewed/pushed so we get the things rolling.
> Shivendra, before you start, let's sync so we don't duplicate efforts
>
> grtz,
> Sander
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* Re: [PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections
From: Matthew Garrett @ 2010-10-19 16:03 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, linux-usb
In-Reply-To: <1286962345.3316.1.camel@aeonflux>
On Wed, Oct 13, 2010 at 12:32:25PM +0300, Marcel Holtmann wrote:
> Hi Matthew,
>
> > Bluetooth runtime PM interacts badly with input devices - the connection
> > will be dropped if the device becomes idle, resulting in noticable lag when
> > the user interacts with the input device again. Bump the pm runtime count
> > when the device is associated and release it when it's disassociated in
> > order to avoid this.
>
> we already have hci_conn_hold_device() and hci_conn_put_device() calls
> for HID to hold reference of the ACL connection. Why do we need more?
Because doing so doesn't do anything to influence runtime PM behaviour.
Putting the calls in these functions might be reasonable though - would
you prefer that?
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [PATCH 2/3] bluetooth: Remove some unnecessary error messages
From: Matthew Garrett @ 2010-10-19 16:01 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, linux-usb
In-Reply-To: <1287402917.3316.128.camel@aeonflux>
On Mon, Oct 18, 2010 at 01:55:17PM +0200, Marcel Holtmann wrote:
> Hi Matthew,
>
> > The main reason for these urbs to error out on submission is that runtime
> > pm has kicked in, which is unnecessary noise. Let's just drop them.
>
> do we wanna remove them or just turn into BT_DBG which is under dynamic
> debug control. Or do we have a set of error results that we do wanna
> print and others that we wanna ignore?
BT_DBG doesn't seem like an obvious problem. Are there any cases where
this will actually trigger in a problematic way?
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [PATCH v2] Bluetooth: btwilink driver
From: Anderson Lizardo @ 2010-10-19 14:56 UTC (permalink / raw)
To: pavan_savoy; +Cc: padovan, marcel, linux-bluetooth, linux-kernel
In-Reply-To: <1287441278-11284-1-git-send-email-pavan_savoy@ti.com>
On Mon, Oct 18, 2010 at 6:34 PM, <pavan_savoy@ti.com> wrote:
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 02deef4..e0d67dd 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -219,4 +219,14 @@ config BT_ATH3K
> =A0 =A0 =A0 =A0 =A0Say Y here to compile support for "Atheros firmware do=
wnload driver"
> =A0 =A0 =A0 =A0 =A0into the kernel or say M to compile it as module (ath3=
k).
>
> +config BT_WILINK
> + =A0 =A0 =A0 tristate "BlueZ bluetooth driver for TI ST"
I think this has been mentioned before: "BlueZ" is not used in this
context on the kernel. It is also not consistent with the config name
"BT_WILINK". You can follow the pattern from the other entries and
use:
tristate "Texas Instruments WinLink7 driver"
> + =A0 =A0 =A0 depends on TI_ST
> + =A0 =A0 =A0 help
> + =A0 =A0 =A0 =A0 This enables the Bluetooth driver for Texas Instrument'=
s BT/FM/GPS
> + =A0 =A0 =A0 =A0 combo devices. This makes use of shared transport line =
discipline
> + =A0 =A0 =A0 =A0 core driver to communicate with the BT core of the comb=
o chip.
> +
> + =A0 =A0 =A0 =A0 Say Y here to compile support for Texas Instrument's Wi=
Link7 driver
> + =A0 =A0 =A0 =A0 into the kernel or say M to compile it as module.
> =A0endmenu
> [...]
> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
> new file mode 100644
> index 0000000..e6e2e64
> --- /dev/null
> +++ b/drivers/bluetooth/btwilink.c
> [...]
> +/* Defines number of seconds to wait for reg completion
> + * callback getting called from ST (in case,registration
> + * with ST returns PENDING status)
> + */
I suggest rewriting this comment into:
"Number of seconds to wait for registration completion when ST returns
PENDING status"
> +#define BT_REGISTER_TIMEOUT =A0 6000 =A0 =A0 /* 6 sec */
> +
> +/**
> + * struct ti_st - BT driver operation structure
> + * @hdev: hci device pointer which binds to bt driver
Just drop "BT" and "bt" here. I think it is clear you are referring to
the Bluetooth driver.
> + * @flags: used locally,to maintain various BT driver status
Suggestion: @flags: driver status flags
(if you can be more specific to which kind of status, it would be even bett=
er)
> + * @streg_cbdata: to hold ST registration callback status
You can drop "to hold" here.
> + * @st_write: write function pointer of ST driver
IMHO this description does not add anything to what st_write is for.
> + * @wait_reg_completion - completion sync between ti_st_open
> + * =A0 =A0 and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> + =A0 =A0 =A0 struct hci_dev *hdev;
> + =A0 =A0 =A0 char streg_cbdata;
> + =A0 =A0 =A0 long (*st_write) (struct sk_buff *);
> + =A0 =A0 =A0 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)
> +{
> + =A0 =A0 =A0 struct hci_dev *hdev;
> + =A0 =A0 =A0 hdev =3D hst->hdev;
> +
> + =A0 =A0 =A0 /* Update HCI stat counters */
> + =A0 =A0 =A0 switch (pkt_type) {
> + =A0 =A0 =A0 case HCI_COMMAND_PKT:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hdev->stat.cmd_tx++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> +
> + =A0 =A0 =A0 case HCI_ACLDATA_PKT:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hdev->stat.acl_tx++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> +
> + =A0 =A0 =A0 case HCI_SCODATA_PKT:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hdev->stat.sco_tx++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 }
> +}
> +
> +/* ------- 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)
> +{
> + =A0 =A0 =A0 struct ti_st *lhst =3D (struct ti_st *)priv_data;
Is this explicit cast necessary?
> +
> + =A0 =A0 =A0 /* ti_st_open() function needs value of 'data' to know
> + =A0 =A0 =A0 =A0* the registration status(success/fail),So have a back
> + =A0 =A0 =A0 =A0* up of it.
> + =A0 =A0 =A0 =A0*/
Suggestion: /* Save registration status for use in ti_st_open() */
> + =A0 =A0 =A0 lhst->streg_cbdata =3D data;
> +
> + =A0 =A0 =A0 /* Got a feedback from ST for BT driver registration
> + =A0 =A0 =A0 =A0* request.Wackup ti_st_open() function to continue
> + =A0 =A0 =A0 =A0* it's open operation.
> + =A0 =A0 =A0 =A0*/
Too much BT here. If it means "Bluetooth", you don't need to use it
every time. Additionally, I don't get what the above comment means.
> + =A0 =A0 =A0 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)
Can this function return "int" instead? It is more common for
functions which return just a error value.
> +{
> + =A0 =A0 =A0 int err;
> + =A0 =A0 =A0 struct ti_st *lhst =3D (struct ti_st *)priv_data;
Again, is this cast necessary?
> +
> + =A0 =A0 =A0 if (!skb)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EFAULT;
> +
> + =A0 =A0 =A0 if (!lhst) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree_skb(skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EFAULT;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 skb->dev =3D (struct net_device *)lhst->hdev;
> +
> + =A0 =A0 =A0 /* Forward skb to HCI core layer */
> + =A0 =A0 =A0 err =3D hci_recv_frame(skb);
> + =A0 =A0 =A0 if (err) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree_skb(skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Unable to push skb to HCI core(%d)"=
, err);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 lhst->hdev->stat.byte_rx +=3D skb->len;
Add a blank like here.
> + =A0 =A0 =A0 return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +/* protocol structure registered with shared transport */
> +static struct st_proto_s ti_st_proto =3D {
> + =A0 =A0 =A0 .type =3D ST_BT,
> + =A0 =A0 =A0 .recv =3D st_receive,
> + =A0 =A0 =A0 .reg_complete_cb =3D st_registration_completion_cb,
> + =A0 =A0 =A0 .priv_data =3D NULL,
> +};
> +
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> + =A0 =A0 =A0 unsigned long timeleft;
> + =A0 =A0 =A0 struct ti_st *hst;
> + =A0 =A0 =A0 int err;
> +
> + =A0 =A0 =A0 BT_DBG("%s %p", hdev->name, hdev);
> +
> + =A0 =A0 =A0 /* provide contexts for callbacks from ST */
> + =A0 =A0 =A0 hst =3D hdev->driver_data;
> + =A0 =A0 =A0 ti_st_proto.priv_data =3D hst;
> +
> + =A0 =A0 =A0 err =3D st_register(&ti_st_proto);
> + =A0 =A0 =A0 if (err =3D=3D -EINPROGRESS) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Prepare wait-for-completion handler data=
structures.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* Needed to syncronize this and st_regis=
tration_completion_cb()
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* functions.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
syncronize -> synchronize
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 init_completion(&hst->wait_reg_completion);
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Reset ST registration callback status fl=
ag , this value
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* will be updated in ti_st_registration_=
completion_cb()
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* function whenever it called from ST dr=
iver.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hst->streg_cbdata =3D -EINPROGRESS;
If this field is used solely for holding status values, why not call
it "reg_status" or something like that? "cbdata" is more for opaque
parameters IMHO.
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* ST is busy with other protocol registrat=
ion(may be busy with
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* firmware download).So,Wait till the re=
gistration callback
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* (passed as a argument to st_register()=
function) getting
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* called from ST.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
"ST is busy with either protocol registration or firmware download.
Wait until the registration callback is called."
Is it clearer?
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_DBG(" waiting for reg completion signal =
from ST");
BT_DBG("Waiting for registration completion signal from ST");
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 timeleft =3D wait_for_completion_timeout
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (&hst->wait_reg_completion,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msecs_to_jiffies(BT_REGI=
STER_TIMEOUT));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!timeleft) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Timeout(%d sec),did=
n't get reg "
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 "completion signal from ST",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 BT_REGISTER_TIMEOUT / 1000);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ETIMEDOUT;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Is ST registration callback called with =
ERROR value? */
ERROR value -> "error status"
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (hst->streg_cbdata !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("ST reg completion C=
B called with invalid"
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 "status %d", hst->streg_cbdata);
Too much abbreviations here:
reg -> registration
CB -> callback
Also you are truncating the C string wrong here (and maybe in other
places). It will be print as "invalidstatus". Use either
"...invalid<space>" (1st line) or "<space>status..." (2nd line).
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EAGAIN;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D 0;
> + =A0 =A0 =A0 } else if (err =3D=3D -EPERM) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("st_register failed %d", err);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
You could assign hst->st_write right here:
hst->st_write =3D ti_st_proto.write;
And simplify the if/else below to:
if (!hst->st_write) {
BT_ERR(....);
...
}
(i.e. invert the check logic and drop the "!=3D NULL" case)
> + =A0 =A0 =A0 /* Do we have proper ST write function? */
> + =A0 =A0 =A0 if (ti_st_proto.write !=3D NULL) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* We need this pointer for sending any Blu=
etooth pkts */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hst->st_write =3D ti_st_proto.write;
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("failed to get ST write func pointer=
");
This error message could be:
BT_ERR("undefined ST write function");
*Although* I think the whole check is in the wrong place. I think this
check should be inside the function which sets ti_st_proto.write.
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Undo registration with ST */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D st_unregister(ST_BT);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (err)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("st_unregister faile=
d %d", err);
Suggestion:
BT_ERR("st_unregister() failed with error %d", err);
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hst->st_write =3D NULL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 /* Registration with ST layer is completed successfully,
> + =A0 =A0 =A0 =A0* now chip is ready to accept commands from HCI core.
> + =A0 =A0 =A0 =A0* Mark HCI Device flag as RUNNING
> + =A0 =A0 =A0 =A0*/
The comment above could be summed as:
/* Registration with ST layer was successful and hardware is ready to
accept commands from HCI core. */
> + =A0 =A0 =A0 set_bit(HCI_RUNNING, &hdev->flags);
Add a blank line here.
> + =A0 =A0 =A0 return err;
> +}
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> + =A0 =A0 =A0 int err;
> + =A0 =A0 =A0 struct ti_st *hst =3D hdev->driver_data;
> +
> + =A0 =A0 =A0 /* continue to unregister from transport */
> + =A0 =A0 =A0 err =3D st_unregister(ST_BT);
> + =A0 =A0 =A0 if (err)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("st_unregister failed %d", err);
BT_ERR("st_unregister() failed with error %d", err);
> +
> + =A0 =A0 =A0 hst->st_write =3D NULL;
Add a blank line here.
> + =A0 =A0 =A0 return err;
> +}
> +
> +/* Called from HCI core, Sends frames to Shared Transport */
IMHO the comment above can be dropped (it is too obvious).
> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> + =A0 =A0 =A0 struct hci_dev *hdev;
> + =A0 =A0 =A0 struct ti_st *hst;
> + =A0 =A0 =A0 long len;
> +
> + =A0 =A0 =A0 if (!skb)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 hdev =3D (struct hci_dev *)skb->dev;
> + =A0 =A0 =A0 if (!hdev)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENODEV;
> +
> + =A0 =A0 =A0 if (!test_bit(HCI_RUNNING, &hdev->flags))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EBUSY;
> +
> + =A0 =A0 =A0 hst =3D (struct ti_st *)hdev->driver_data;
> +
> + =A0 =A0 =A0 /* Prepend skb with frame type */
> + =A0 =A0 =A0 memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> + =A0 =A0 =A0 BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_t=
ype,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb->len);
> +
> + =A0 =A0 =A0 /* Insert skb to shared transport layer's transmit queue.
> + =A0 =A0 =A0 =A0* Freeing skb memory is taken care in shared transport l=
ayer,
> + =A0 =A0 =A0 =A0* so don't free skb memory here.
> + =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 if (!hst->st_write) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree_skb(skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR(" Can't write to ST, st_write null?"=
);
Suggestion: BT_ERR("Could not write to ST (st_write is NULL)");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EAGAIN;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 len =3D hst->st_write(skb);
> + =A0 =A0 =A0 if (len < 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree_skb(skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR(" ST write failed (%ld)", len);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EAGAIN;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 /* ST accepted our skb. So, Go ahead and do rest */
> + =A0 =A0 =A0 hdev->stat.byte_tx +=3D len;
> + =A0 =A0 =A0 ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> +
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> + =A0 =A0 =A0 if (!hdev)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Destruct called with invalid HCI De=
vice"
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "(hdev=3DNU=
LL)");
There is a "return" missing here. Also I don't think this error
message is necessary at all. You could have just:
if (!hdev)
return;
> +
> + =A0 =A0 =A0 BT_DBG("%s", hdev->name);
> +
> + =A0 =A0 =A0 /* free ti_st memory */
> + =A0 =A0 =A0 kfree(hdev->driver_data);
add a blank line here.
> + =A0 =A0 =A0 return;
> +}
> +
> +/* Creates new HCI device */
> +static int ti_st_register_dev(struct ti_st *hst)
> +{
> + =A0 =A0 =A0 int err;
> + =A0 =A0 =A0 struct hci_dev *hdev;
> +
> + =A0 =A0 =A0 /* Initialize and register HCI device */
> + =A0 =A0 =A0 hdev =3D hci_alloc_dev();
> + =A0 =A0 =A0 if (!hdev)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 BT_DBG("hdev=3D %p", hdev);
BT_DBG("hdev %p", hdev);
> +
> + =A0 =A0 =A0 hst->hdev =3D hdev;
> + =A0 =A0 =A0 hdev->bus =3D HCI_UART;
> + =A0 =A0 =A0 hdev->driver_data =3D hst;
> + =A0 =A0 =A0 hdev->open =3D ti_st_open;
> + =A0 =A0 =A0 hdev->close =3D ti_st_close;
> + =A0 =A0 =A0 hdev->flush =3D NULL;
> + =A0 =A0 =A0 hdev->send =3D ti_st_send_frame;
> + =A0 =A0 =A0 hdev->destruct =3D ti_st_destruct;
> + =A0 =A0 =A0 hdev->owner =3D THIS_MODULE;
> +
> + =A0 =A0 =A0 if (reset)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
> +
> + =A0 =A0 =A0 err =3D hci_register_dev(hdev);
> + =A0 =A0 =A0 if (err < 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Can't register HCI device");
Print the err value on the message above.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_free_dev(hdev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 BT_DBG(" HCI device registered. hdev=3D %p", hdev);
Suggestion: BT_DBG("HCI device registered (hdev %p)", hdev);
> + =A0 =A0 =A0 return 0;
> +}
> +
> +
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> + =A0 =A0 =A0 int err;
> + =A0 =A0 =A0 static struct ti_st *hst;
> +
> + =A0 =A0 =A0 BT_DBG(" Bluetooth Driver Version %s", VERSION);
> +
> + =A0 =A0 =A0 hst =3D kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> + =A0 =A0 =A0 if (!hst)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 /* Expose "hciX" device to user space */
> + =A0 =A0 =A0 err =3D ti_st_register_dev(hst);
> + =A0 =A0 =A0 if (err) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(hst);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 dev_set_drvdata(&pdev->dev, hst);
> + =A0 =A0 =A0 return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> + =A0 =A0 =A0 struct ti_st *hst;
> +
> + =A0 =A0 =A0 hst =3D dev_get_drvdata(&pdev->dev);
> + =A0 =A0 =A0 /* Deallocate local resource's memory =A0*/
You can invert the hst check:
if (!hst)
return <some_error_code>;
And reduce put the code below outside the if(). Also note that the
current code always returns 0. It should return error for hst =3D=3D NULL
and hdev =3D=3D NULL.
> + =A0 =A0 =A0 if (hst) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct hci_dev *hdev =3D hst->hdev;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!hdev) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Invalid hdev memory=
");
The error message above is not informative.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(hst);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ti_st_close(hdev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_unregister_dev(hdev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Free HCI device memory *=
/
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_free_dev(hdev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static struct platform_driver btwilink_driver =3D {
> + =A0 =A0 =A0 .probe =3D bt_ti_probe,
> + =A0 =A0 =A0 .remove =3D bt_ti_remove,
> + =A0 =A0 =A0 .driver =3D {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .name =3D "btwilink",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .owner =3D THIS_MODULE,
> + =A0 =A0 =A0 },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init bt_drv_init(void)
> +{
> + =A0 =A0 =A0 long ret;
> +
> + =A0 =A0 =A0 ret =3D platform_driver_register(&btwilink_driver);
> + =A0 =A0 =A0 if (ret !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("btwilink platform driver registrati=
on failed");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EPERM;
-EPERM for a registration failure? Looks strange to me.
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static void __exit bt_drv_exit(void)
> +{
> + =A0 =A0 =A0 platform_driver_unregister(&btwilink_driver);
> +}
> +
> +module_init(bt_drv_init);
> +module_exit(bt_drv_exit);
> +
> +/* ------ Module Info ------ */
> +
> +module_param(reset, bool, 0644);
> +MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> +MODULE_AUTHOR("Raja Mani <raja_mani@ti.com>");
> +MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
> --
> 1.6.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>
Regards,
--=20
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH] hidd shows different names for same device when connected from GUI and CLI.
From: Johan Hedberg @ 2010-10-19 14:09 UTC (permalink / raw)
To: Balamurugan Mahalingam; +Cc: marcel, linux-bluetooth, charubala
In-Reply-To: <1287496282-16170-1-git-send-email-mbalamurugan@atheros.com>
Hi,
On Tue, Oct 19, 2010, Balamurugan Mahalingam wrote:
> Blueman and other GUIs use the string from ServiceName attribute as
> device name in the SDP response, but hidd tool uses strings from
> Service Description and Provider Name atrribute.
>
> SDP response from Microsoft Bluetooth device has extended ASCII
> character 174(registered symbol) in its ServiceDescription attribute.
> Android platform expects the device name to have only printable
> characters and therefore GUI Crashes while connecting Microsoft
> Bluetooth Mouse using hidd
>
> Using the ServiceName attribute from the SDP response
> instead of ProviderName+ServiceDescription for device name in the
> hidd tool solves both the issues.
> ---
> compat/sdp.c | 33 +++++++++++++++++----------------
> 1 files changed, 17 insertions(+), 16 deletions(-)
Same issues as with the previous commit message. Lines should be max 74
characters long and please redo the summary line to be of the format
"Fix ..." and shorter than it is now.
Btw, why are you using hidd and not the bluetoothd input plugin? hidd is
legacy stuff which will disappear from BlueZ in the future.
Johan
^ permalink raw reply
* [PATCH] hidd shows different names for same device when connected from GUI and CLI.
From: Balamurugan Mahalingam @ 2010-10-19 13:51 UTC (permalink / raw)
To: marcel; +Cc: linux-bluetooth, charubala, Balamurugan Mahalingam
Blueman and other GUIs use the string from ServiceName
attribute as device name in the SDP response, but hidd tool
uses strings from Service Description and Provider Name atrribute.
SDP response from Microsoft Bluetooth device has extended ASCII
character 174(registered symbol) in its ServiceDescription attribute.
Android platform expects the device name to have only printable characters and
therefore GUI Crashes while connecting Microsoft Bluetooth Mouse using hidd
Using the ServiceName attribute from the SDP response
instead of ProviderName+ServiceDescription for device name in the
hidd tool solves both the issues.
---
compat/sdp.c | 33 +++++++++++++++++----------------
1 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/compat/sdp.c b/compat/sdp.c
index ff2e39f..f384844 100644
--- a/compat/sdp.c
+++ b/compat/sdp.c
@@ -248,22 +248,23 @@ int get_sdp_device_info(const bdaddr_t *src, const bdaddr_t *dst, struct hidp_co
rec = (sdp_record_t *) hid_rsp->data;
- pdlist = sdp_data_get(rec, 0x0101);
- pdlist2 = sdp_data_get(rec, 0x0102);
- if (pdlist) {
- if (pdlist2) {
- if (strncmp(pdlist->val.str, pdlist2->val.str, 5)) {
- strncpy(req->name, pdlist2->val.str, sizeof(req->name) - 1);
- strcat(req->name, " ");
- }
- strncat(req->name, pdlist->val.str,
- sizeof(req->name) - strlen(req->name));
- } else
- strncpy(req->name, pdlist->val.str, sizeof(req->name) - 1);
- } else {
- pdlist2 = sdp_data_get(rec, 0x0100);
- if (pdlist2)
- strncpy(req->name, pdlist2->val.str, sizeof(req->name) - 1);
+ pdlist2 = sdp_data_get(rec, 0x0100);
+ if (pdlist2)
+ strncpy(req->name, pdlist2->val.str, sizeof(req->name) - 1);
+ else {
+ pdlist = sdp_data_get(rec, 0x0101);
+ pdlist2 = sdp_data_get(rec, 0x0102);
+ if (pdlist) {
+ if (pdlist2) {
+ if (strncmp(pdlist->val.str, pdlist2->val.str, 5)) {
+ strncpy(req->name, pdlist2->val.str, sizeof(req->name) - 1);
+ strcat(req->name, " ");
+ }
+ strncat(req->name, pdlist->val.str,
+ sizeof(req->name) - strlen(req->name));
+ } else
+ strncpy(req->name, pdlist->val.str, sizeof(req->name) - 1);
+ }
}
pdlist = sdp_data_get(rec, 0x0201);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] Fix problem with handling CHLD=0 command
From: Johan Hedberg @ 2010-10-19 13:44 UTC (permalink / raw)
To: Lukasz Pawlik; +Cc: linux-bluetooth
In-Reply-To: <1287491524-8384-1-git-send-email-lucas.pawlik@gmail.com>
Hi Lukasz,
On Tue, Oct 19, 2010, Lukasz Pawlik wrote:
> This patch change handling of CHLD=0 command. Previously both calls
> (held and waiting) were terminated when HS sent CHLD=0. Now all held
> calls will be released or only incoming call will be rejected without
> affecting any held calls.
> ---
> audio/telephony-maemo6.c | 7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
Thanks. Pushed upstream.
Johan
^ permalink raw reply
* Re: [PATCH] Fix accessing freed memory
From: Johan Hedberg @ 2010-10-19 13:43 UTC (permalink / raw)
To: Balamurugan Mahalingam; +Cc: marcel, linux-bluetooth, charubala
In-Reply-To: <1287491414-14015-1-git-send-email-mbalamurugan@atheros.com>
Hi,
On Tue, Oct 19, 2010, Balamurugan Mahalingam wrote:
> Crash is due to some junk characters in the device name.
> The device name is copied from a string variable after freeing the memory
> it points to, so there were some junk characters in it which was the reason
> for the crash.
>
> Fixing the issue by freeing the memory in string variable after
> device name is copied.
>
> ---
> compat/sdp.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
Thanks. The patch has been pushed upstream.
Johan
^ permalink raw reply
* Re: [PATCH] Fix regression when formatting timestamps from tracker
From: Johan Hedberg @ 2010-10-19 13:42 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1287489236-4558-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Tue, Oct 19, 2010, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>
> Tracker format user delimiters like '-' and ':' which need to be keep in
> order to sscanf to work.
> ---
> plugins/phonebook-tracker.c | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* [PATCH] Fix problem with handling CHLD=0 command
From: Lukasz Pawlik @ 2010-10-19 12:32 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lukasz Pawlik
This patch change handling of CHLD=0 command. Previously both calls
(held and waiting) were terminated when HS sent CHLD=0. Now all held
calls will be released or only incoming call will be rejected without
affecting any held calls.
---
audio/telephony-maemo6.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/audio/telephony-maemo6.c b/audio/telephony-maemo6.c
index f4dfbde..33fd13f 100644
--- a/audio/telephony-maemo6.c
+++ b/audio/telephony-maemo6.c
@@ -784,8 +784,11 @@ void telephony_call_hold_req(void *telephony_device, const char *cmd)
switch (cmd[0]) {
case '0':
- foreach_call_with_status(CSD_CALL_STATUS_HOLD, release_call);
- foreach_call_with_status(CSD_CALL_STATUS_WAITING,
+ if (find_call_with_status(CSD_CALL_STATUS_WAITING))
+ foreach_call_with_status(CSD_CALL_STATUS_WAITING,
+ release_call);
+ else
+ foreach_call_with_status(CSD_CALL_STATUS_HOLD,
release_call);
break;
case '1':
--
1.7.0.4
^ permalink raw reply related
* [PATCH] Fix accessing freed memory
From: Balamurugan Mahalingam @ 2010-10-19 12:30 UTC (permalink / raw)
To: marcel; +Cc: linux-bluetooth, charubala, Balamurugan Mahalingam
Crash is due to some junk characters in the device name.
The device name is copied from a string variable after freeing the memory
it points to, so there were some junk characters in it which was the reason
for the crash.
Fixing the issue by freeing the memory in string variable after
device name is copied.
---
compat/sdp.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/compat/sdp.c b/compat/sdp.c
index 8898136..ff2e39f 100644
--- a/compat/sdp.c
+++ b/compat/sdp.c
@@ -152,7 +152,6 @@ int get_stored_device_info(const bdaddr_t *src, const bdaddr_t *dst, struct hidp
&vendor, &product, &version, &subclass, &country,
&parser, desc, &req->flags, &pos);
- free(str);
req->vendor = vendor;
req->product = product;
@@ -163,6 +162,7 @@ int get_stored_device_info(const bdaddr_t *src, const bdaddr_t *dst, struct hidp
snprintf(req->name, 128, "%s", str + pos);
+ free(str);
req->rd_size = strlen(desc) / 2;
req->rd_data = malloc(req->rd_size);
if (!req->rd_data) {
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] Fix GUI crash while connecting Interlink Bluetooth Mouse in android platform.
From: Balamurugan Mahalingam @ 2010-10-19 12:01 UTC (permalink / raw)
To: Balamurugan Mahalingam, marcel, linux-bluetooth, charubala
In-Reply-To: <20101019112328.GA12554@jh-x301>
Johan Hedberg wrote:
> Hi,
>
> On Tue, Oct 19, 2010, Balamurugan Mahalingam wrote:
>
>> Crash is due to some junk characters in the device name.
>> The device name is copied from a string variable after freeing the memory it points to, so there were some junk characters in it which was the reason for the crash.
>> Fixing the issue by freeing the memory in string variable after device name is copied.
>>
>> Signed-off-by: Balamurugan Mahalingam <mbalamurugan@atheros.com>
>> ---
>> compat/sdp.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>
> Looks like a good fix, but could you please fix your commit message and
> resend:
>
> 1. The commit message doesn't so much describe the change as it does
> some symptom in a downstream system (android). E.g. the summary line
> should be something like "Fix accessing freed memory". You can still
> talk about the background to this issue wrt. android but the main point
> should be in a bluez-only context.
>
> 2. Keep the commit message lines below 74 characters so that it's
> properly viewable on a 80-character wide terminal when running "git log"
>
> 3. Remove the signed-off-by line. We don't use that in userspace.
>
> Johan
> .
>
>
Hi Johan,
Thanks for your comments. I will do the changes and send the patch once
again
Thanks
Balamurugan Mahalingam
^ permalink raw reply
* [PATCH] Fix regression when formatting timestamps from tracker
From: Luiz Augusto von Dentz @ 2010-10-19 11:53 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
Tracker format user delimiters like '-' and ':' which need to be keep in
order to sscanf to work.
---
plugins/phonebook-tracker.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 2da825b..f0e00b8 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -705,18 +705,21 @@ static char *iso8601_utc_to_localtime(const char *datetime)
memset(&tm, 0, sizeof(tm));
- nr = sscanf(datetime, "%04u%02u%02uT%02u%02u%02u%c",
+ nr = sscanf(datetime, "%04u-%02u-%02uT%02u:%02u:%02u%c",
&tm.tm_year, &tm.tm_mon, &tm.tm_mday,
&tm.tm_hour, &tm.tm_min, &tm.tm_sec,
&tz);
if (nr < 6) {
/* Invalid time format */
+ error("sscanf(): %s (%d)", strerror(errno), errno);
return g_strdup("");
}
/* Time already in localtime */
- if (nr == 6)
- return g_strdup(datetime);
+ if (nr == 6) {
+ strftime(localdate, sizeof(localdate), "%Y%m%dT%H%M%S", &tm);
+ return g_strdup(localdate);
+ }
tm.tm_year -= 1900; /* Year since 1900 */
tm.tm_mon--; /* Months since January, values 0-11 */
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] Fix GUI crash while connecting Interlink Bluetooth Mouse in android platform.
From: Johan Hedberg @ 2010-10-19 11:23 UTC (permalink / raw)
To: Balamurugan Mahalingam; +Cc: marcel, linux-bluetooth, charubala
In-Reply-To: <1287482080-10093-1-git-send-email-mbalamurugan@atheros.com>
Hi,
On Tue, Oct 19, 2010, Balamurugan Mahalingam wrote:
> Crash is due to some junk characters in the device name.
> The device name is copied from a string variable after freeing the memory it points to, so there were some junk characters in it which was the reason for the crash.
> Fixing the issue by freeing the memory in string variable after device name is copied.
>
> Signed-off-by: Balamurugan Mahalingam <mbalamurugan@atheros.com>
> ---
> compat/sdp.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
Looks like a good fix, but could you please fix your commit message and
resend:
1. The commit message doesn't so much describe the change as it does
some symptom in a downstream system (android). E.g. the summary line
should be something like "Fix accessing freed memory". You can still
talk about the background to this issue wrt. android but the main point
should be in a bluez-only context.
2. Keep the commit message lines below 74 characters so that it's
properly viewable on a 80-character wide terminal when running "git log"
3. Remove the signed-off-by line. We don't use that in userspace.
Johan
^ permalink raw reply
* [PATCH] Fix GUI crash while connecting Interlink Bluetooth Mouse in android platform.
From: Balamurugan Mahalingam @ 2010-10-19 9:54 UTC (permalink / raw)
To: marcel; +Cc: linux-bluetooth, charubala, Balamurugan Mahalingam
Crash is due to some junk characters in the device name.
The device name is copied from a string variable after freeing the memory it points to, so there were some junk characters in it which was the reason for the crash.
Fixing the issue by freeing the memory in string variable after device name is copied.
Signed-off-by: Balamurugan Mahalingam <mbalamurugan@atheros.com>
---
compat/sdp.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/compat/sdp.c b/compat/sdp.c
index 8898136..ff2e39f 100644
--- a/compat/sdp.c
+++ b/compat/sdp.c
@@ -152,7 +152,6 @@ int get_stored_device_info(const bdaddr_t *src, const bdaddr_t *dst, struct hidp
&vendor, &product, &version, &subclass, &country,
&parser, desc, &req->flags, &pos);
- free(str);
req->vendor = vendor;
req->product = product;
@@ -163,6 +162,7 @@ int get_stored_device_info(const bdaddr_t *src, const bdaddr_t *dst, struct hidp
snprintf(req->name, 128, "%s", str + pos);
+ free(str);
req->rd_size = strlen(desc) / 2;
req->rd_data = malloc(req->rd_size);
if (!req->rd_data) {
--
1.6.3.3
^ permalink raw reply related
* Re: AVRCP 1.4 : Future on Target Role
From: Sander van Grieken @ 2010-10-19 9:47 UTC (permalink / raw)
To: linux-bluetooth
On Tuesday 19 October 2010 10:31:33 Luiz Augusto von Dentz wrote:
> Hi,
>
> On Tue, Oct 19, 2010 at 9:16 AM, Shivendra Agrawal
> <ag.shivendra@gmail.com> wrote:
> > Hi All,
> >
> > I have been looking at AVRCP 1.4 in BlueZ and intending to
> > enhance/develop this profile for the target role. I have been
> > following the discussion initiated by Sander van Grieken earlier last
> > month, and as I understand, they are premitive and has scope for
> > further enhancements.
>
> Joao Paulo also did some work in this area using MPRIS, I guess he
> even publish his tree somewhere.
Yes, it's at git://git.profusion.mobi/users/jprvita/bluez.git
> > In the current BlueZ implementation, control-api.txt mentions few
> > methods e.g. SendVendoeDependent, ChangePlayback..., that are not
> > referred/implemented in the code, or I may be unable to find at right
> > place.
Yes, the document is more like a proposal, than a description of the actual
implementation.
> > Further, there are some more AVRCP 1.4 TG specific Notify and Browsing
> > commands that can be added.
>
> I would suggest you to take a look at media-api.txt, this is what we
> area planning to use for streams and we should probably add support
> for metadata and browsing (those 2 seems to be the most asked features
> from avrcp nowadays). Actually metadata seems to fit nicely there, we
> just need another interface to receive stream metadata, now browsing
> is probably not so easy.
>
> > I am willing to define some preliminary interface APIs for Target
> > Media Applications to register itself with BlueZ, and would come back
> > with some idea proposal for your suggestions on improvements.
>
> If media players all agree on using MPRIS than we probably don't need
> to have them registering to us, in the other hand Im not sure if MPRIS
> API do cover everything in terms of avrcp, maybe you can figure this
> out.
I am very much in favor of not directly depending on MPRIS, but instead letting
applications registering themselves as a target. For two reasons:
- AVRCP seems to be a superset of MPRIS (which is very limited IMO), and might have
different semantics, especially w.r.t. event notifications. So we would limit ourselves to
the intersecting subset of both technologies.
- A separate AVRCP/TG <-> MPRIS bridge agent would still allow controlling all MPRIS-
enabled players, so we can have both full implementation of the AVRCP spec, AND generic
MPRIS support.
> > I am keen to receive your feedback with some ideas and thoughts to put
> > my effort in right direction.
> >
> > Question:
> > Is anyone working on AVRCP 1.4 Target role profile development?
>
> Joao Paulo (http://jprvita.wordpress.com/2010/07/22/avrcp-metadata/)
Actually, the metadata work is not part of v1.4 of the spec, but 1.3
Second, I have already added some boilerplate stuff (like a DBUS Connect method for RCP and
some fixes for CT commands), but I've based on Joao's branch, so I have to wait until his
stuff gets merged. Alternatively, I could rebase that stuff on HEAD, but that would overlap
and conflict with Joao's stuff, so I'm hesitant to go there.
Shivendra, before you start, let's sync so we don't duplicate efforts
grtz,
Sander
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox