* (reminder) Re: [PATCH v4] Bluetooth: btwilink driver
From: Pavan Savoy @ 2010-10-28 4:04 UTC (permalink / raw)
To: padovan, marcel; +Cc: linux-bluetooth, linux-kernel, Pavan Savoy
On Tue, Oct 19, 2010 at 6:06 PM, <pavan_savoy@ti.com> wrote:
> From: Pavan Savoy <pavan_savoy@ti.com>
>
> v4 comments
>
> module init now returns what platform_driver_register returns.
> type casting of void* private data has been removed
>
Marcel, Gustavo,
Any comments on this?
Was really hoping this would get in for 2.6.37 ...
PS:
deliberately editing the subject..
regards,
Pavan Savoy
> 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,
>
> Marcel, Gustavo, & list,
> Please review this version of patch.
>
> 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>
> ---
> =C2=A0drivers/bluetooth/Kconfig =C2=A0 =C2=A0| =C2=A0 10 +
> =C2=A0drivers/bluetooth/Makefile =C2=A0 | =C2=A0 =C2=A01 +
> =C2=A0drivers/bluetooth/btwilink.c | =C2=A0411 ++++++++++++++++++++++++++=
++++++++++++++++
> =C2=A03 files changed, 422 insertions(+), 0 deletions(-)
> =C2=A0create 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
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Say Y here to compile support for "Athe=
ros firmware download driver"
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0into the kernel or say M to compile it =
as module (ath3k).
>
> +config BT_WILINK
> + =C2=A0 =C2=A0 =C2=A0 tristate "Texas Instruments WiLink7 driver"
> + =C2=A0 =C2=A0 =C2=A0 depends on TI_ST
> + =C2=A0 =C2=A0 =C2=A0 help
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 This enables the Bluetooth driver for Texas=
Instrument's BT/FM/GPS
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 combo devices. This makes use of shared tra=
nsport line discipline
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 core driver to communicate with the BT core=
of the combo chip.
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 Say Y here to compile support for Texas Ins=
trument's WiLink7 driver
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 into the kernel or say M to compile it as m=
odule.
> =C2=A0endmenu
> 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) =C2=A0 =C2=A0+=3D btsdio.o
> =C2=A0obj-$(CONFIG_BT_ATH3K) =C2=A0 =C2=A0 =C2=A0 =C2=A0 +=3D ath3k.o
> =C2=A0obj-$(CONFIG_BT_MRVL) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0+=3D btmrvl=
.o
> =C2=A0obj-$(CONFIG_BT_MRVL_SDIO) =C2=A0 =C2=A0 +=3D btmrvl_sdio.o
> +obj-$(CONFIG_BT_WILINK) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0+=3D btwilink.o
>
> =C2=A0btmrvl-y =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 :=3D btmrvl_main.o
> =C2=A0btmrvl-$(CONFIG_DEBUG_FS) =C2=A0 =C2=A0 =C2=A0+=3D 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 @@
> +/*
> + * =C2=A0Texas Instrument's Bluetooth Driver For Shared Transport.
> + *
> + * =C2=A0Bluetooth Driver acts as interface between HCI core and
> + * =C2=A0TI Shared Transport Layer.
> + *
> + * =C2=A0Copyright (C) 2009-2010 Texas Instruments
> + * =C2=A0Author: Raja Mani <raja_mani@ti.com>
> + * =C2=A0 =C2=A0 Pavan Savoy <pavan_savoy@ti.com>
> + *
> + * =C2=A0This program is free software; you can redistribute it and/or m=
odify
> + * =C2=A0it under the terms of the GNU General Public License version 2 =
as
> + * =C2=A0published by the Free Software Foundation.
> + *
> + * =C2=A0This program is distributed in the hope that it will be useful,
> + * =C2=A0but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * =C2=A0MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =C2=A0See =
the
> + * =C2=A0GNU General Public License for more details.
> + *
> + * =C2=A0You should have received a copy of the GNU General Public Licen=
se
> + * =C2=A0along with this program; if not, write to the Free Software
> + * =C2=A0Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA =C2=A0=
02111-1307 =C2=A0USA
> + *
> + */
> +
> +#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 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "1.0"
> +
> +/* Number of seconds to wait for registration completion
> + * when ST returns PENDING status.
> + */
> +#define BT_REGISTER_TIMEOUT =C2=A0 6000 =C2=A0 =C2=A0 /* 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
> + * =C2=A0 =C2=A0 to be used by the driver during send_frame.
> + * @wait_reg_completion - completion sync between ti_st_open
> + * =C2=A0 =C2=A0 and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> + =C2=A0 =C2=A0 =C2=A0 char reg_status;
> + =C2=A0 =C2=A0 =C2=A0 long (*st_write) (struct sk_buff *);
> + =C2=A0 =C2=A0 =C2=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)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> + =C2=A0 =C2=A0 =C2=A0 hdev =3D hst->hdev;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Update HCI stat counters */
> + =C2=A0 =C2=A0 =C2=A0 switch (pkt_type) {
> + =C2=A0 =C2=A0 =C2=A0 case HCI_COMMAND_PKT:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.cmd_tx++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +
> + =C2=A0 =C2=A0 =C2=A0 case HCI_ACLDATA_PKT:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.acl_tx++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +
> + =C2=A0 =C2=A0 =C2=A0 case HCI_SCODATA_PKT:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.sco_tx++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> + =C2=A0 =C2=A0 =C2=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)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *lhst =3D priv_data;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Save registration status for use in ti_st_open(=
) */
> + =C2=A0 =C2=A0 =C2=A0 lhst->reg_status =3D data;
> + =C2=A0 =C2=A0 =C2=A0 /* complete the wait in ti_st_open() */
> + =C2=A0 =C2=A0 =C2=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)
> +{
> + =C2=A0 =C2=A0 =C2=A0 int err;
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *lhst =3D priv_data;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!skb)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!lhst) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 skb->dev =3D (struct net_device *)lhst->hdev;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Forward skb to HCI core layer */
> + =C2=A0 =C2=A0 =C2=A0 err =3D hci_recv_frame(skb);
> + =C2=A0 =C2=A0 =C2=A0 if (err) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("Unable to push=
skb to HCI core(%d)", err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 lhst->hdev->stat.byte_rx +=3D skb->len;
> +
> + =C2=A0 =C2=A0 =C2=A0 return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +/* protocol structure registered with shared transport */
> +static struct st_proto_s ti_st_proto =3D {
> + =C2=A0 =C2=A0 =C2=A0 .type =3D ST_BT,
> + =C2=A0 =C2=A0 =C2=A0 .recv =3D st_receive,
> + =C2=A0 =C2=A0 =C2=A0 .reg_complete_cb =3D st_registration_completion_cb=
,
> + =C2=A0 =C2=A0 =C2=A0 .priv_data =3D NULL,
> +};
> +
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 unsigned long timeleft;
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *hst;
> + =C2=A0 =C2=A0 =C2=A0 int err;
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG("%s %p", hdev->name, hdev);
> +
> + =C2=A0 =C2=A0 =C2=A0 /* provide contexts for callbacks from ST */
> + =C2=A0 =C2=A0 =C2=A0 hst =3D hdev->driver_data;
> + =C2=A0 =C2=A0 =C2=A0 ti_st_proto.priv_data =3D hst;
> +
> + =C2=A0 =C2=A0 =C2=A0 err =3D st_register(&ti_st_proto);
> + =C2=A0 =C2=A0 =C2=A0 if (err =3D=3D -EINPROGRESS) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Prepare wait-for-co=
mpletion handler data structures.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Needed to sync=
hronize this and
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* st_registratio=
n_completion_cb() functions.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 init_completion(&hst->=
wait_reg_completion);
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Reset ST registrati=
on callback status flag , this value
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* will be update=
d in ti_st_registration_completion_cb()
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* function whene=
ver it called from ST driver.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->reg_status =3D -E=
INPROGRESS;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* ST is busy with eit=
her protocol registration or firmware
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* download. Wait=
until the registration callback is called
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_DBG(" waiting for r=
egistration completion signal from ST");
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timeleft =3D wait_for_=
completion_timeout
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 (&hst->wait_reg_completion,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!timeleft) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 BT_ERR("Timeout(%d sec),didn't get reg "
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "completion =
signal from ST",
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_REGISTER_=
TIMEOUT / 1000);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 return -ETIMEDOUT;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Is ST registration =
callback called with ERROR status? */
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (hst->reg_status !=
=3D 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 BT_ERR("ST registration completed with invalid "
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "status %d",=
hst->reg_status);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 return -EAGAIN;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D 0;
> + =C2=A0 =C2=A0 =C2=A0 } else if (err =3D=3D -EPERM) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("st_register fa=
iled %d", err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D ti_st_proto.write;
> + =C2=A0 =C2=A0 =C2=A0 if (!hst->st_write) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("undefined ST w=
rite function");
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Undo registration w=
ith ST */
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D st_unregister(=
ST_BT);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (err)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 BT_ERR("st_unregister() failed with error %d", err);
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D NULL=
;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Registration with ST layer is successful,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* hardware is ready to accept commands from =
HCI core.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 set_bit(HCI_RUNNING, &hdev->flags);
> +
> + =C2=A0 =C2=A0 =C2=A0 return err;
> +}
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 int err;
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *hst =3D hdev->driver_data;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* continue to unregister from transport */
> + =C2=A0 =C2=A0 =C2=A0 err =3D st_unregister(ST_BT);
> + =C2=A0 =C2=A0 =C2=A0 if (err)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("st_unregister(=
) failed with error %d", err);
> +
> + =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D NULL;
> +
> + =C2=A0 =C2=A0 =C2=A0 return err;
> +}
> +
> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *hst;
> + =C2=A0 =C2=A0 =C2=A0 long len;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!skb)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
> +
> + =C2=A0 =C2=A0 =C2=A0 hdev =3D (struct hci_dev *)skb->dev;
> + =C2=A0 =C2=A0 =C2=A0 if (!hdev)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENODEV;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!test_bit(HCI_RUNNING, &hdev->flags))
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EBUSY;
> +
> + =C2=A0 =C2=A0 =C2=A0 hst =3D hdev->driver_data;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Prepend skb with frame type */
> + =C2=A0 =C2=A0 =C2=A0 memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1)=
;
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(sk=
b)->pkt_type,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 skb->len);
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Insert skb to shared transport layer's transmit=
queue.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* Freeing skb memory is taken care in shared=
transport layer,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* so don't free skb memory here.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 if (!hst->st_write) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR(" Could not wri=
te to ST (st_write is NULL)");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EAGAIN;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 len =3D hst->st_write(skb);
> + =C2=A0 =C2=A0 =C2=A0 if (len < 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR(" ST write fail=
ed (%ld)", len);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EAGAIN;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 /* ST accepted our skb. So, Go ahead and do rest *=
/
> + =C2=A0 =C2=A0 =C2=A0 hdev->stat.byte_tx +=3D len;
> + =C2=A0 =C2=A0 =C2=A0 ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> +
> + =C2=A0 =C2=A0 =C2=A0 return 0;
> +}
> +
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 if (!hdev)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG("%s", hdev->name);
> +
> + =C2=A0 =C2=A0 =C2=A0 /* free ti_st memory */
> + =C2=A0 =C2=A0 =C2=A0 kfree(hdev->driver_data);
> +
> + =C2=A0 =C2=A0 =C2=A0 return;
> +}
> +
> +/* Creates new HCI device */
> +static int ti_st_register_dev(struct ti_st *hst)
> +{
> + =C2=A0 =C2=A0 =C2=A0 int err;
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Initialize and register HCI device */
> + =C2=A0 =C2=A0 =C2=A0 hdev =3D hci_alloc_dev();
> + =C2=A0 =C2=A0 =C2=A0 if (!hdev)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG("hdev %p", hdev);
> +
> + =C2=A0 =C2=A0 =C2=A0 hst->hdev =3D hdev;
> + =C2=A0 =C2=A0 =C2=A0 hdev->bus =3D HCI_UART;
> + =C2=A0 =C2=A0 =C2=A0 hdev->driver_data =3D hst;
> + =C2=A0 =C2=A0 =C2=A0 hdev->open =3D ti_st_open;
> + =C2=A0 =C2=A0 =C2=A0 hdev->close =3D ti_st_close;
> + =C2=A0 =C2=A0 =C2=A0 hdev->flush =3D NULL;
> + =C2=A0 =C2=A0 =C2=A0 hdev->send =3D ti_st_send_frame;
> + =C2=A0 =C2=A0 =C2=A0 hdev->destruct =3D ti_st_destruct;
> + =C2=A0 =C2=A0 =C2=A0 hdev->owner =3D THIS_MODULE;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (reset)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 set_bit(HCI_QUIRK_NO_R=
ESET, &hdev->quirks);
> +
> + =C2=A0 =C2=A0 =C2=A0 err =3D hci_register_dev(hdev);
> + =C2=A0 =C2=A0 =C2=A0 if (err < 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("Can't register=
HCI device error %d", err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hci_free_dev(hdev);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG(" HCI device registered (hdev %p)", hdev);
> + =C2=A0 =C2=A0 =C2=A0 return 0;
> +}
> +
> +
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 int err;
> + =C2=A0 =C2=A0 =C2=A0 static struct ti_st *hst;
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG(" Bluetooth Driver Version %s", VERSION);
> +
> + =C2=A0 =C2=A0 =C2=A0 hst =3D kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> + =C2=A0 =C2=A0 =C2=A0 if (!hst)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Expose "hciX" device to user space */
> + =C2=A0 =C2=A0 =C2=A0 err =3D ti_st_register_dev(hst);
> + =C2=A0 =C2=A0 =C2=A0 if (err) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree(hst);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 dev_set_drvdata(&pdev->dev, hst);
> + =C2=A0 =C2=A0 =C2=A0 return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *hst;
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> +
> + =C2=A0 =C2=A0 =C2=A0 hst =3D dev_get_drvdata(&pdev->dev);
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!hst)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Deallocate local resource's memory =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 hdev =3D hst->hdev;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!hdev) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("Invalid hdev m=
emory");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree(hst);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 ti_st_close(hdev);
> + =C2=A0 =C2=A0 =C2=A0 hci_unregister_dev(hdev);
> + =C2=A0 =C2=A0 =C2=A0 /* Free HCI device memory */
> + =C2=A0 =C2=A0 =C2=A0 hci_free_dev(hdev);
> +
> + =C2=A0 =C2=A0 =C2=A0 return 0;
> +}
> +
> +static struct platform_driver btwilink_driver =3D {
> + =C2=A0 =C2=A0 =C2=A0 .probe =3D bt_ti_probe,
> + =C2=A0 =C2=A0 =C2=A0 .remove =3D bt_ti_remove,
> + =C2=A0 =C2=A0 =C2=A0 .driver =3D {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .name =3D "btwilink",
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .owner =3D THIS_MODULE=
,
> + =C2=A0 =C2=A0 =C2=A0 },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init bt_drv_init(void)
> +{
> + =C2=A0 =C2=A0 =C2=A0 long ret;
> +
> + =C2=A0 =C2=A0 =C2=A0 ret =3D platform_driver_register(&btwilink_driver)=
;
> + =C2=A0 =C2=A0 =C2=A0 if (ret !=3D 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("btwilink platf=
orm driver registration failed");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;
> + =C2=A0 =C2=A0 =C2=A0 }
> + =C2=A0 =C2=A0 =C2=A0 return 0;
> +}
> +
> +static void __exit bt_drv_exit(void)
> +{
> + =C2=A0 =C2=A0 =C2=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 =C2=A0http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] Bluetooth: Automate remote name requests
From: Luiz Augusto von Dentz @ 2010-10-28 7:42 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth, Johan Hedberg
In-Reply-To: <1288196742-29386-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
On Wed, Oct 27, 2010 at 7:25 PM, <johan.hedberg@gmail.com> wrote:
> From: Johan Hedberg <johan.hedberg@nokia.com>
>
> 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>
> ---
> net/bluetooth/hci_event.c | 66 ++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 60 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 84093b0..05ad699 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);
> +}
> +
> static void hci_cs_remote_name_req(struct hci_dev *hdev, __u8 status)
> {
> + struct hci_cp_remote_name_req *cp;
> +
> BT_DBG("%s status 0x%x", hdev->name, status);
> +
> + /* If successful wait for the name req complete event before
> + * checking for the need to do authentication */
> + if (status == 0)
> + return;
> +
> + cp = hci_sent_cmd_data(hdev, HCI_OP_REMOTE_NAME_REQ);
> + if (!cp)
> + return;
> +
> + hci_dev_lock(hdev);
> +
> + request_outgoing_auth(hdev, &cp->bdaddr);
> +
> + hci_dev_unlock(hdev);
> }
>
> static void hci_cs_read_remote_features(struct hci_dev *hdev, __u8 status)
> @@ -1090,9 +1132,17 @@ static inline void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *s
>
> static inline void hci_remote_name_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> + struct hci_ev_remote_name *ev = (void *) skb->data;
> +
> BT_DBG("%s", hdev->name);
>
> hci_conn_check_pending(hdev);
> +
> + hci_dev_lock(hdev);
> +
> + request_outgoing_auth(hdev, &ev->bdaddr);
> +
> + hci_dev_unlock(hdev);
> }
>
> static inline void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
> @@ -1177,9 +1227,11 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
> sizeof(cp), &cp);
> } else if (!ev->status && conn->out &&
> conn->sec_level == BT_SECURITY_HIGH) {
> - struct hci_cp_auth_requested cp;
> - cp.handle = ev->handle;
> - hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED,
> + struct hci_cp_remote_name_req cp;
> + memset(&cp, 0, sizeof(cp));
> + bacpy(&cp.bdaddr, &conn->dst);
> + cp.pscan_rep_mode = 0x02;
> + hci_send_cmd(hdev, HCI_OP_REMOTE_NAME_REQ,
> sizeof(cp), &cp);
> } else {
> conn->state = BT_CONNECTED;
> @@ -1660,9 +1712,11 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
> if (!ev->status && hdev->ssp_mode > 0 &&
> conn->ssp_mode > 0 && conn->out &&
> conn->sec_level != BT_SECURITY_SDP) {
> - struct hci_cp_auth_requested cp;
> - cp.handle = ev->handle;
> - hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED,
> + struct hci_cp_remote_name_req cp;
> + memset(&cp, 0, sizeof(cp));
> + bacpy(&cp.bdaddr, &conn->dst);
> + cp.pscan_rep_mode = 0x02;
> + hci_send_cmd(hdev, HCI_OP_REMOTE_NAME_REQ,
> sizeof(cp), &cp);
> } else {
> conn->state = BT_CONNECTED;
> --
> 1.7.1
This seems to be done only for some connections which IMO is not the
correct, so for instance it wouldn't request name for SDP connections
nor for incoming connections, is there a reason for that?
Perhaps requesting the name regardless the security level on
hci_remote_features_evt and then latter, when we got the name
response, we continue with security level check and authentication
request if necessary. Wouldn't that work better?
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* [PATCH 1/5] Fix issue when setting limited discoverable mode
From: Luiz Augusto von Dentz @ 2010-10-28 8:00 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
When setting limited discoverable mode it will always switch to
discoverable on adapter_mode_changed which doesn't match the pending mode
requested.
---
src/adapter.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 4a9f34e..98d96e2 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3225,9 +3225,10 @@ void adapter_mode_changed(struct btd_adapter *adapter, uint8_t scan_mode)
DBUS_TYPE_BOOLEAN, &pairable);
if (discoverable && adapter->pairable && adapter->discov_timeout > 0 &&
- adapter->discov_timeout <= 60)
+ adapter->discov_timeout <= 60) {
+ adapter->mode = MODE_LIMITED;
adapter_set_limited_discoverable(adapter, TRUE);
- else if (!discoverable)
+ } else if (!discoverable)
adapter_set_limited_discoverable(adapter, FALSE);
emit_property_changed(connection, path,
--
1.7.1
^ permalink raw reply related
* [PATCH 2/5] Fix not waiting mode change when setting powered property
From: Luiz Augusto von Dentz @ 2010-10-28 8:00 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1288252859-10337-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
This may cause errors if the user application immediately set another
mode.
---
src/adapter.c | 84 +++++++++++++++++++++++++++++---------------------------
1 files changed, 43 insertions(+), 41 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 98d96e2..f4fc3c1 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -604,11 +604,11 @@ static DBusMessage *set_powered(DBusConnection *conn, DBusMessage *msg,
if (mode == adapter->mode)
return dbus_message_new_method_return(msg);
- err = set_mode(adapter, mode, NULL);
+ err = set_mode(adapter, mode, msg);
if (err < 0)
return failed_strerror(msg, -err);
- return dbus_message_new_method_return(msg);
+ return NULL;
}
static DBusMessage *set_discoverable(DBusConnection *conn, DBusMessage *msg,
@@ -2570,6 +2570,45 @@ static void unload_drivers(struct btd_adapter *adapter)
}
}
+static void set_mode_complete(struct btd_adapter *adapter)
+{
+ struct session_req *pending;
+ const char *modestr;
+ int err;
+
+ if (adapter->pending_mode == NULL)
+ return;
+
+ pending = adapter->pending_mode;
+ adapter->pending_mode = NULL;
+
+ err = (pending->mode != adapter->mode) ? -EINVAL : 0;
+
+ if (pending->msg != NULL) {
+ DBusMessage *msg = pending->msg;
+ DBusMessage *reply;
+
+ if (err < 0)
+ reply = failed_strerror(msg, -err);
+ else
+ reply = g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
+
+ g_dbus_send_message(connection, reply);
+ }
+
+ modestr = mode2str(adapter->mode);
+
+ DBG("%s", modestr);
+
+ /* Only store if the mode matches the pending */
+ if (err == 0)
+ write_device_mode(&adapter->bdaddr, modestr);
+ else
+ error("unable to set mode: %s", mode2str(pending->mode));
+
+ session_unref(pending);
+}
+
int adapter_stop(struct btd_adapter *adapter)
{
gboolean powered, discoverable, pairable;
@@ -2628,6 +2667,8 @@ int adapter_stop(struct btd_adapter *adapter)
info("Adapter %s has been disabled", adapter->path);
+ set_mode_complete(adapter);
+
return 0;
}
@@ -3137,45 +3178,6 @@ int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr)
return 0;
}
-static void set_mode_complete(struct btd_adapter *adapter)
-{
- struct session_req *pending;
- const char *modestr;
- int err;
-
- if (adapter->pending_mode == NULL)
- return;
-
- pending = adapter->pending_mode;
- adapter->pending_mode = NULL;
-
- err = (pending->mode != adapter->mode) ? -EINVAL : 0;
-
- if (pending->msg != NULL) {
- DBusMessage *msg = pending->msg;
- DBusMessage *reply;
-
- if (err < 0)
- reply = failed_strerror(msg, -err);
- else
- reply = g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
-
- g_dbus_send_message(connection, reply);
- }
-
- modestr = mode2str(adapter->mode);
-
- DBG("%s", modestr);
-
- /* Only store if the mode matches the pending */
- if (err == 0)
- write_device_mode(&adapter->bdaddr, modestr);
- else
- error("unable to set mode: %s", mode2str(pending->mode));
-
- session_unref(pending);
-}
-
void adapter_mode_changed(struct btd_adapter *adapter, uint8_t scan_mode)
{
const gchar *path = adapter_get_path(adapter);
--
1.7.1
^ permalink raw reply related
* [PATCH 3/5] Fix setting Discoverable when adapter is down
From: Luiz Augusto von Dentz @ 2010-10-28 8:00 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1288252859-10337-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
This cause an error because adapter_up will load the mode from storage
ignoring pending mode which cause modes to mismatch.
To fix this behavior now when attempting to change mode we first store
the new mode and wait DEVUP to complete set the mode stored.
---
src/adapter.c | 19 +++++++++++--------
1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index f4fc3c1..e5f7730 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -533,6 +533,7 @@ static int set_mode(struct btd_adapter *adapter, uint8_t new_mode,
DBusMessage *msg)
{
int err;
+ const char *modestr;
if (adapter->pending_mode != NULL)
return -EALREADY;
@@ -541,6 +542,8 @@ static int set_mode(struct btd_adapter *adapter, uint8_t new_mode,
err = adapter_ops->set_powered(adapter->dev_id, TRUE);
if (err < 0)
return err;
+
+ goto done;
}
if (adapter->up && new_mode == MODE_OFF) {
@@ -576,18 +579,18 @@ static int set_mode(struct btd_adapter *adapter, uint8_t new_mode,
}
done:
- DBG("%s", mode2str(new_mode));
+ modestr = mode2str(new_mode);
+ write_device_mode(&adapter->bdaddr, modestr);
+
+ DBG("%s", modestr);
if (msg != NULL)
/* Wait for mode change to reply */
adapter->pending_mode = create_session(adapter, connection,
msg, new_mode, NULL);
- else {
+ else
/* Nothing to reply just write the new mode */
- const char *modestr = mode2str(new_mode);
adapter->mode = new_mode;
- write_device_mode(&adapter->bdaddr, modestr);
- }
return 0;
}
@@ -2600,11 +2603,11 @@ static void set_mode_complete(struct btd_adapter *adapter)
DBG("%s", modestr);
- /* Only store if the mode matches the pending */
- if (err == 0)
+ /* restore if the mode doesn't matches the pending */
+ if (err != 0) {
write_device_mode(&adapter->bdaddr, modestr);
- else
error("unable to set mode: %s", mode2str(pending->mode));
+ }
session_unref(pending);
}
--
1.7.1
^ permalink raw reply related
* [PATCH 4/5] Fix not replying when mode is limited discoverable or discoverable
From: Luiz Augusto von Dentz @ 2010-10-28 8:00 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1288252859-10337-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
When changing from/to limited discoverable or discoverable it won't
change the scan mode thus causing set_mode_complete to not be called.
---
src/adapter.c | 49 +++++++++++++++++++++++++++++++++----------------
1 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index e5f7730..b5e73b7 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -584,25 +584,41 @@ done:
DBG("%s", modestr);
- if (msg != NULL)
- /* Wait for mode change to reply */
- adapter->pending_mode = create_session(adapter, connection,
- msg, new_mode, NULL);
- else
+ if (msg != NULL) {
+ /* Limited to Discoverable and vice-versa doesn't cause any
+ change to scan mode */
+ if (g_str_equal(modestr, mode2str(adapter->mode)) == TRUE) {
+ DBusMessage *reply;
+
+ reply = g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
+
+ g_dbus_send_message(connection, reply);
+ } else
+ /* Wait for mode change to reply */
+ adapter->pending_mode = create_session(adapter,
+ connection,
+ msg, new_mode,
+ NULL);
+ } else
/* Nothing to reply just write the new mode */
adapter->mode = new_mode;
return 0;
}
-static DBusMessage *set_powered(DBusConnection *conn, DBusMessage *msg,
- gboolean powered, void *data)
+static DBusMessage *set_discoverable(DBusConnection *conn, DBusMessage *msg,
+ gboolean discoverable, void *data)
{
struct btd_adapter *adapter = data;
uint8_t mode;
int err;
- mode = powered ? get_mode(&adapter->bdaddr, "on") : MODE_OFF;
+ mode = discoverable ? MODE_DISCOVERABLE : MODE_CONNECTABLE;
+
+ if (mode == MODE_DISCOVERABLE && adapter->pairable &&
+ adapter->discov_timeout > 0 &&
+ adapter->discov_timeout <= 60)
+ mode = MODE_LIMITED;
if (mode == adapter->mode)
return dbus_message_new_method_return(msg);
@@ -614,19 +630,20 @@ static DBusMessage *set_powered(DBusConnection *conn, DBusMessage *msg,
return NULL;
}
-static DBusMessage *set_discoverable(DBusConnection *conn, DBusMessage *msg,
- gboolean discoverable, void *data)
+static DBusMessage *set_powered(DBusConnection *conn, DBusMessage *msg,
+ gboolean powered, void *data)
{
struct btd_adapter *adapter = data;
uint8_t mode;
int err;
- mode = discoverable ? MODE_DISCOVERABLE : MODE_CONNECTABLE;
+ if (powered) {
+ mode = get_mode(&adapter->bdaddr, "on");
+ return set_discoverable(conn, msg, mode == MODE_DISCOVERABLE,
+ data);
+ }
- if (mode == MODE_DISCOVERABLE && adapter->pairable &&
- adapter->discov_timeout > 0 &&
- adapter->discov_timeout <= 60)
- mode = MODE_LIMITED;
+ mode = MODE_OFF;
if (mode == adapter->mode)
return dbus_message_new_method_return(msg);
@@ -635,7 +652,7 @@ static DBusMessage *set_discoverable(DBusConnection *conn, DBusMessage *msg,
if (err < 0)
return failed_strerror(msg, -err);
- return 0;
+ return NULL;
}
static DBusMessage *set_pairable(DBusConnection *conn, DBusMessage *msg,
--
1.7.1
^ permalink raw reply related
* [PATCH 5/5] Fix not being able to set discoverable when discoverable timeout is set
From: Luiz Augusto von Dentz @ 2010-10-28 8:00 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1288252859-10337-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
Since setting Discoverable property now has the ability to turn on the
adapter if off we should no longer ignore discoverable mode if a timeout
is set.
---
src/adapter.c | 51 +++++++++++++++++++++++++++++----------------------
1 files changed, 29 insertions(+), 22 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index b5e73b7..19fe5a8 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -529,6 +529,32 @@ static struct session_req *create_session(struct btd_adapter *adapter,
return session_ref(req);
}
+static int adapter_set_mode(struct btd_adapter *adapter, uint8_t mode)
+{
+ int err;
+
+ if (mode == MODE_CONNECTABLE)
+ err = adapter_ops->set_connectable(adapter->dev_id);
+ else
+ err = adapter_ops->set_discoverable(adapter->dev_id);
+
+ if (err < 0)
+ return err;
+
+ if (mode == MODE_CONNECTABLE)
+ return 0;
+
+ adapter_remove_discov_timeout(adapter);
+
+ if (adapter->discov_timeout)
+ adapter_set_discov_timeout(adapter, adapter->discov_timeout);
+
+ if (mode != MODE_LIMITED && adapter->mode == MODE_LIMITED)
+ adapter_set_limited_discoverable(adapter, FALSE);
+
+ return 0;
+}
+
static int set_mode(struct btd_adapter *adapter, uint8_t new_mode,
DBusMessage *msg)
{
@@ -559,25 +585,11 @@ static int set_mode(struct btd_adapter *adapter, uint8_t new_mode,
if (new_mode == adapter->mode)
return 0;
- if (new_mode == MODE_CONNECTABLE)
- err = adapter_ops->set_connectable(adapter->dev_id);
- else
- err = adapter_ops->set_discoverable(adapter->dev_id);
+ err = adapter_set_mode(adapter, new_mode);
if (err < 0)
return err;
- if (new_mode > MODE_CONNECTABLE) {
- adapter_remove_discov_timeout(adapter);
-
- if (adapter->discov_timeout)
- adapter_set_discov_timeout(adapter,
- adapter->discov_timeout);
-
- if (new_mode != MODE_LIMITED && adapter->mode == MODE_LIMITED)
- adapter_set_limited_discoverable(adapter, FALSE);
- }
-
done:
modestr = mode2str(new_mode);
write_device_mode(&adapter->bdaddr, modestr);
@@ -2429,18 +2441,13 @@ static int adapter_up(struct btd_adapter *adapter, const char *mode)
write_device_mode(&adapter->bdaddr, onmode);
return adapter_up(adapter, onmode);
- } else if (!g_str_equal(mode, "connectable") &&
- adapter->discov_timeout == 0) {
- /* Set discoverable only if timeout is 0 */
+ } else if (!g_str_equal(mode, "connectable")) {
adapter->mode = MODE_DISCOVERABLE;
scan_mode = SCAN_PAGE | SCAN_INQUIRY;
}
proceed:
- if (scan_mode == SCAN_PAGE)
- err = adapter_ops->set_connectable(adapter->dev_id);
- else
- err = adapter_ops->set_discoverable(adapter->dev_id);
+ err = adapter_set_mode(adapter, adapter->mode);
if (err < 0)
return err;
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v2] Fix obexd crash for empty listing invalid cache
From: Dmitriy Paliy @ 2010-10-28 8:07 UTC (permalink / raw)
To: ext Johan Hedberg; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101027190347.GA8025@jh-x301>
Hi Johan,
On Wed, 2010-10-27 at 21:03 +0200, ext Johan Hedberg wrote:
> Hi Dmitriy,
>
> On Wed, Oct 27, 2010, Dmitriy Paliy wrote:
> > This fixes obexd crash in 3-way calling scenario when listing response is
> > empty. Valid cache and empty pbap buffer mean that cache was already attempted
> > to be created within a single session, but no data was available. Hence, it
> > is not notified and no such file error returned. New cache is not created
> > within current obex session or unless path is changed. Such removes necessity
> > of querying and filtering contacts for each incoming call in the other case,
> > which is extensive for large phone books. On the other hand, if user updates
> > contacts, cache will not be renewed till obex session is closed or path is
> > changed. Therefore TODO: note is added that clear of cache should be defined
> > besides of end of session or change of path.
>
> The commit message seems to have a max width of 78 characters which
> means that it's not viewable with git log on a 80 character terminal
> (git log indents the output by 4 characters). Please keep the max commit
> message width to 74 characters or so. Also, if possible try to split it
> up into multiple paragraphs. Paragraphs longer than 6 lines tend to be a
> bit harder to follow.
>
> > + * TODO: Define clear cache besides end of session or change
> > + * of path.
>
> That doesn't sound like proper english to me and I'm not sure what
> you're trying to say. Should it be "Define a clear distinction between
> end of session and change of path"?
Thanks for comments. What I meant is following. Cache is created at the
moment when there is an incoming call in the middle of another ongoing
call. Clear cache and create a new one when the call is retried, which
is one of possible solutions to fix this bug, looks inefficient to me.
Therefore, already created cache is valid either till session is closed
or path is changed. However, if user updates his contacts, then changes
will not affect cache within these limits. What I was trying to say in
the todo note is that in order to improve logic when cache is created
and cleared, besides two facts written above (end of session and change
of path), another way to renew cache should be defined.
Br,
Dmitriy
^ permalink raw reply
* [PATCH 0/1 v3] Fix obexd crash for empty listing invalid cache
From: Dmitriy Paliy @ 2010-10-28 8:43 UTC (permalink / raw)
To: linux-bluetooth
Hi,
In this revision comments are improved to be more consitent and more
focused on the bug fixed. Todo note is removed since there is already
similar fixme note in pbap_setpath function.
BR,
Dmitriy
^ permalink raw reply
* [PATCH v3] Fix obexd crash for empty listing invalid cache
From: Dmitriy Paliy @ 2010-10-28 8:43 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
In-Reply-To: <1288255408-3967-1-git-send-email-dmitriy.paliy@nokia.com>
This fixes obexd crash in 3-way calling scenario when filtered listing
response is empty. Valid cache and empty pbap buffer mean that cache was
already attempted to be created within a single session, but no data was
available. Hence, it is not notified and no such file error returned.
Such avoids clearing and creating a new cache operations for each incoming
call, which is one of possible solution to fix this bug. It can be
extensive for large phone books. New cache is not created within current
obex session or unless path is changed.
---
plugins/pbap.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/plugins/pbap.c b/plugins/pbap.c
index 11cb678..3ea7d6b 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -751,6 +751,15 @@ static void *vobject_list_open(const char *name, int oflag, mode_t mode,
/* PullvCardListing always get the contacts from the cache */
if (pbap->cache.valid) {
+ /*
+ * Valid cache and empty buffer mean that cache was already
+ * created within a single session, but no data is available.
+ */
+ if (!pbap->buffer) {
+ ret = -ENOENT;
+ goto fail;
+ }
+
cache_ready_notify(pbap);
goto done;
}
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 1/2 v2] Bluetooth: Fix system crash caused by del_timer()
From: Gustavo F. Padovan @ 2010-10-28 8:49 UTC (permalink / raw)
To: haijun liu; +Cc: Haijun Liu, linux-bluetooth
In-Reply-To: <AANLkTin2RaOZbRbg4faCFUxMLVi-FoY2xnNqq0a8cHpN@mail.gmail.com>
Hi Haijun,
* haijun liu <liuhaijun.er@gmail.com> [2010-10-26 09:32:19 +0800]:
> Hi Gustavo,
>
> >> >> During test session with another vendor's bt stack, found that in
> >> >> l2cap_chan_del() using del_timer() caused l2cap_monitor_timeout()
> >> >> be called after the sock was freed, so it raised a system crash.
> >> >> So I just replaced del_timer() with del_timer_sync() to solve it.
> >> >
> >> > NAK on this. If you read the del_timer_sync() documentation you can
> >> > see that you can't call del_timer_sync() on interrupt context. The
> >> > possible solution here is to check in the beginning of
> >> > l2cap_monitor_timeout() if your sock is still valid.
> >> >
> >>
> >> You are right, I only considered close() interface, so missed the interrupt
> >> context.
> >>
> >> It's very difficult to check sock valid or not in timeout procedure, since it's
> >> an interrupt context, and only can get context from parameter pre-stored,
> >> except global variables.
> >
> > I think you can check for sk == null there.
> >
>
> It's a pre-stored parameter, it will not change by itself.
I looked a bit into this and a good solution seems to be to hold a
reference to the sock when we call a mod_timer() and then put the
reference when we call del_timer() and the timer is inactive or when
l2cap_monitor_timeout(). Look net/sctp/ for examples.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: HFP Pulseaudio Source destroyed "too quickly" at the end of a call
From: Thomas Wälti @ 2010-10-28 8:57 UTC (permalink / raw)
To: johan.hedberg, linux-bluetooth, Krisztian.Litkey, Jyri.Sarha
In-Reply-To: <20101027222602.GB24756@jh-x301>
2010/10/28 Johan Hedberg <johan.hedberg@gmail.com>:
> Probably this isn't really BlueZ related, but the audio policy module
> (on the PulseAudio side) in the N900 kicks in and requests these changes
> to the audio streams. Unfortunately I'm not really an expert with that
> code (and afaik the audio policy part is closed) so I can't really offer
> more insight on the issue. Just speculating, but you might be able to
> accomplish something by hacking in some delays into the pulse bluetooth
> modules (but again, I'm not really familiar with them so this might not
> be a feasible approach).
Thanks for the prompt feedback, Johan.
I had looked at the PulseAudio Bluetooth Modules source
(http://git.0pointer.de/?p=pulseaudio.git;a=tree;f=src/modules/bluetooth;hb=HEAD)
before, but didn't see anything (but then, it's a bit over my head :-)
It seems like Joao Paulo Rechi Vita is the author of the relevant
source, and as he also did the Bluez HFP implementation (IIRC), I hope
he might shed a light on this. I'm asking on this list because once
HFP "hangs up", it probably issues some teardown event to its
consumers.
(It would be nice if I could get this solved this before heading to
the MeeGo conf in Dublin - see you there :-)
Best regards
-Tom
^ permalink raw reply
* Re: [PATCH 3/6] Bluetooth: Implement the first SMP commands
From: Gustavo F. Padovan @ 2010-10-28 9:17 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Ville Tervo, Anderson Briglia, linux-bluetooth@vger.kernel.org,
Vinicius Costa Gomes
In-Reply-To: <AANLkTikVG-gjd1d7T6ETbfXCU0jp2GsR0miWJFO55hps@mail.gmail.com>
* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2010-10-26 08:22:16 -0700]:
> Hi,
>
> On Tue, Oct 26, 2010 at 2:26 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> > On Mon, Oct 25, 2010 at 03:03:56PM +0200, ext Gustavo F. Padovan wrote:
> >> Hi Vinicius,
> >>
> >> * Anderson Briglia <anderson.briglia@openbossa.org> [2010-10-22 19:56:57 -0400]:
> >>
> >> > From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> >> >
> >> > These simple commands will allow the SMP procedure to be started
> >> > and terminated with a not supported error. This is the first step
> >> > toward something useful.
> >> >
> >> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> >> > ---
> >> > net/bluetooth/l2cap.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> > 1 files changed, 117 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >> > index 1ac44f4..ba87c84 100644
> >> > --- a/net/bluetooth/l2cap.c
> >> > +++ b/net/bluetooth/l2cap.c
> >> > @@ -54,6 +54,7 @@
> >> > #include <net/bluetooth/bluetooth.h>
> >> > #include <net/bluetooth/hci_core.h>
> >> > #include <net/bluetooth/l2cap.h>
> >> > +#include <net/bluetooth/smp.h>
> >> >
> >> > #define VERSION "2.15"
> >> >
> >> > @@ -307,6 +308,85 @@ static void l2cap_chan_del(struct sock *sk, int err)
> >> > }
> >> > }
> >> >
> >> > +static struct sk_buff *smp_build_cmd(struct l2cap_conn *conn, u8 code,
> >> > + u16 dlen, void *data)
> >>
> >> Call this l2cap_smp_build_cmd()
> >
> > Should the whole smp stuff be in separate file (smp.c)? It's not a l2cap feature but a
> > protocol using l2cap. In that case smp_build_cmd would be good name.
>
> +1
>
> It is also much better for maintenance and development since there is
> less patches touching the l2cap.c so less chances of conflicts,
> rebases and regressions on l2cap.
Yep, we may need a new smp.c file.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* [PATCH] Remove old hcid.conf
From: Gustavo F. Padovan @ 2010-10-28 9:21 UTC (permalink / raw)
To: linux-bluetooth
---
src/hcid.conf | 57 -------------
src/hcid.conf.5.in | 227 ----------------------------------------------------
2 files changed, 0 insertions(+), 284 deletions(-)
delete mode 100644 src/hcid.conf
delete mode 100644 src/hcid.conf.5.in
diff --git a/src/hcid.conf b/src/hcid.conf
deleted file mode 100644
index b6ce3b4..0000000
--- a/src/hcid.conf
+++ /dev/null
@@ -1,57 +0,0 @@
-#
-# HCI daemon configuration file.
-#
-
-# HCId options
-options {
- # Automatically initialize new devices
- autoinit yes;
-
- # Security Manager mode
- # none - Security manager disabled
- # auto - Use local PIN for incoming connections
- # user - Always ask user for a PIN
- #
- security user;
-
- # Pairing mode
- # none - Pairing disabled
- # multi - Allow pairing with already paired devices
- # once - Pair once and deny successive attempts
- pairing multi;
-
- # Default PIN code for incoming connections
- passkey "BlueZ";
-}
-
-# Default settings for HCI devices
-device {
- # Local device name
- # %d - device id
- # %h - host name
- name "BlueZ (%d)";
-
- # Local device class
- class 0x000100;
-
- # Default packet type
- #pkt_type DH1,DM1,HV1;
-
- # Inquiry and Page scan
- iscan enable; pscan enable;
-
- # Default link mode
- # none - no specific policy
- # accept - always accept incoming connections
- # master - become master on incoming connections,
- # deny role switch on outgoing connections
- lm accept;
-
- # Default link policy
- # none - no specific policy
- # rswitch - allow role switch
- # hold - allow hold mode
- # sniff - allow sniff mode
- # park - allow park mode
- lp rswitch,hold,sniff,park;
-}
diff --git a/src/hcid.conf.5.in b/src/hcid.conf.5.in
deleted file mode 100644
index b771389..0000000
--- a/src/hcid.conf.5.in
+++ /dev/null
@@ -1,227 +0,0 @@
-.TH "HCID.CONF" "5" "March 2004" "hcid.conf - HCI daemon" "System management commands"
-.SH "NAME"
-@CONFIGDIR@/hcid.conf \- Configuration file for the hcid Bluetooth HCI daemon
-
-.SH "DESCRIPTION"
-@CONFIGDIR@/hcid.conf contains all the options needed by the Bluetooth Host Controller Interface daemon.
-
-It consists of sections and parameters. A section begins with
-the name of the section followed by optional specifiers and the
-parameters inside curly brackets. Sections contain parameters of
-the form:
-.TP
-\fIname\fP \fIvalue1\fP, \fIvalue2\fP ... ;
-
-.PP
-Any character after a hash ('#') character is ignored until newline.
-Whitespace is also ignored.
-
-
-The valid section names for
-.B hcid.conf
-are, at the moment:
-
-.TP
-.B options
-contains generic options for hcid and the pairing policy.
-.TP
-.B device
-contains lower\-level options for the hci devices connected to the computer.
-.SH "OPTIONS SECTION"
-The following parameters may be present in an option section:
-
-
-.TP
-\fBautoinit\fP yes|no
-
-Automatically initialize newly connected devices. The default is \fIno\fP.
-
-
-.TP
-\fBpairing\fP none|multi|once
-
-\fInone\fP means that pairing is disabled. \fImulti\fP allows pairing
-with already paired devices. \fIonce\fP allows pairing once and denies
-successive attempts. The default hcid configuration is shipped with \fBmulti\fP
-enabled
-
-.TP
-\fBoffmode\fP noscan|devdown
-
-\fInoscan\fP means that page and inquiry scans are disabled when you call
-SetMode("off"). \fIdevdown\fP sets the adapter into down state (same what
-\fIhciconfig hci0 down\fP does).
-
-.TP
-\fBdeviceid\fP <vendor>:<product>:<version>
-
-This option allows to specify the vendor and product information of the
-Bluetooth device ID service record.
-
-.TP
-\fBpasskey\fP "\fIpin\fP"
-
-The default PIN for incoming connections if \fBsecurity\fP has been
-set to \fIauto\fP.
-
-.TP
-\fBsecurity\fP none|auto|user
-
-\fInone\fP means the security manager is disabled. \fIauto\fP uses
-local PIN, by default from pin_code, for incoming
-connections. \fIuser\fP always asks the user for a PIN.
-
-.SH "DEVICE SECTION"
-Parameters within a device section with no specifier, the default
-device section, will be applied to all devices and device sections
-where these are unspecified. The following optional device specifiers
-are supported:
-
-.TP
-\fInn\fP\fB:\fP\fInn\fP\fB:\fP\fInn\fP\fB:\fP\fInn\fP\fB:\fP\fInn\fP\fB:\fP\fInn\fP
-
-Parameters specified within this section will be applied to the device
-with this \fIdevice bluetooth address\fP. All other parameters are applied from
-the default section.
-
-.TP
-\fBhci\fIn\fP
-
-Parameters specified within this section will be applied to the device
-with this \fIdevice interface\fP, unless that device is matched by a
-\fIdevice address\fP section. All other parameters are applied from
-the default section.
-
-
-.PP
-\fBNote\fP: Most of the options supported in the \fBdevice\fP section are described to some extent in the bluetooth specification version 1.2 Vol2, Part E section 6. Please refer to it for technical details.
-
-.PP
-The following parameters may be present in a device section:
-
-.TP
-\fBname\fP "\fIname\fP"
-
-The device name. \fI%d\fP inserts the device id. \fI%h\fP inserts
-the host name.
-
-
-.TP
-\fBclass\fP 0x\fISSDDdd\fP (three bytes)
-
-The Bluetooth Device Class is described in the Bluetooth Specification section 1.2 ("Assigned Numbers \- Bluetooth Baseband").
-
-The default shipped with hcid is 0x000100 which simply stands for "Computer".
-
-The Bluetooth device class is a high\-level description of the bluetooth device, composed of three bytes: the "Major Service Class" (byte "SS" above), the "Major Device Class" (byte "DD" above) and the "Minor Device Class" (byte "dd" above). These classes describe the high\-level capabilities of the device, such as "Networking Device", "Computer", etc. This information is often used by clients who are looking for a certain type of service around them.
-
-Where it becomes tricky is that another type of mechanism for service discovery exists: "SDP", as in "Service Discovery Protocol".
-
-In practice, most Bluetooth clients scan their surroundings in two successive steps: they first look for all bluetooth devices around them and find out their "class". You can do this on Linux with the \fBhcitool scan\fP command. Then, they use SDP in order to check if a device in a given class offers the type of service that they want.
-
-This means that the hcid.conf "class" parameter needs to be set up properly if particular services are running on the host, such as "PAN", or "OBEX Obect Push", etc: in general a device looking for a service such as "Network Access Point" will only scan for this service on devices containing "Networking" in their major service class.
-
-
-.IP
-Major service class byte allocation (from LSB to MSB):
-
-Bit 1: Positioning (Location identification)
-
-Bit 2: Networking (LAN, Ad hoc, ...)
-
-Bit 3: Rendering (Printing, Speaker, ...)
-
-Bit 4: Capturing (Scanner, Microphone, ...)
-
-Bit 5: Object Transfer (v\-Inbox, v\-Folder, ...)
-
-Bit 6: Audio (Speaker, Microphone, Headset service, ...)
-
-Bit 7: Telephony (Cordless telephony, Modem, Headset service, ...)
-
-Bit 8: Information (WEB\-server, WAP\-server, ...)
-
-.IP
-Example: class 0x02hhhh : the device offers networking service
-
-
-.IP
-Major device class allocation:
-
-0x00: Miscellaneous
-
-0x01: Computer (desktop,notebook, PDA, organizers, .... )
-
-0x02: Phone (cellular, cordless, payphone, modem, ...)
-
-0x03: LAN /Network Access point
-
-0x04: Audio/Video (headset,speaker,stereo, video display, vcr.....
-
-0x05: Peripheral (mouse, joystick, keyboards, ..... )
-
-0x06: Imaging (printing, scanner, camera, display, ...)
-
-Other values are not defined (refer to the Bluetooth specification for more details
-
-.IP
-Minor device class allocation: the meaning of this byte depends on the major class allocation, please refer to the Bluetooth specifications for more details).
-
-.IP
-.B Example:
-if PAND runs on your server, you need to set up at least \fBclass 0x020100\fP, which stands for "Service Class: Networking" and "Device Class: Computer, Uncategorized".
-
-
-.TP
-\fBiscan\fP enable|disable
-.TP
-\fBpscan\fP enable|disable
-
-Bluetooth devices discover and connect to each other through the use of two special Bluetooth channels, the Inquiry and Page channels (described in the Bluetooth Spec Volume 1, Part A, Section 3.3.3, page 35). These two options enable the channels on the bluetooth device.
-
-\fBiscan enable\fP: makes the bluetooth device "discoverable" by enabling it to answer "inquiries" from other nearby bluetooth devices.
-
-\fBpscan enable\fP: makes the bluetooth device "connectable to" by enabling the use of the "page scan" channel.
-
-.TP
-\fBlm\fP none|accept,master
-
-\fInone\fP means no specific policy. \fIaccept\fP means always accept
-incoming connections. \fImaster\fP means become master on incoming
-connections and deny role switch on outgoing connections.
-
-.TP
-\fBlp\fP none|rswitch,hold,sniff,park
-
-\fInone\fP means no specific policy. \fIrswitch\fP means allow role
-switch. \fIhold\fP means allow hold mode. \fIsniff\fP means allow
-sniff mode. \fIpark\fP means allow park mode. Several options can be
-combined.
-
-This option determines the various operational modes that are allowed for this device when it participates to a piconet. Normally hold and sniff should be enabled for standard operations.
-
-hold: this mode is related to synchronous communications (SCO voice channel for example).
-
-sniff: when in this mode, a device is only present on the piconet during determined slots of time, allowing it to do other things when it is "absent", for example to scan for other bluetooth devices.
-
-park: this is a mode where the device is put on standby on the piconet, for power\-saving purposes for example.
-
-rswitch: this is a mode that enables role\-switch (master <\-> slave) between two devices in a piconet. It is not clear whether this needs to be enabled in order to make the "lm master" setting work properly or not.
-
-.TP
-\fBpageto\fP \fIn\fP
-
-Page Timeout measured in number of baseband slots. Interval length = N * 0.625 msec (1 baseband slot)
-
-.TP
-\fBdiscovto\fP \fIn\fP
-
-The time in seconds that the device will stay in discoverable mode. 0 disables this feature and forces the device to be always discoverable.
-
-.SH "FILES"
-.TP
-.I @CONFIGDIR@/hcid.conf
-Default location of the global configuration file.
-
-.SH "AUTHOR"
-This manual page was written by Edouard Lafargue, Fredrik Noring, Maxim Krasnyansky and Marcel Holtmann.
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH] bluetooth: Fix NULL pointer dereference issue
From: Gustavo F. Padovan @ 2010-10-28 9:58 UTC (permalink / raw)
To: Yuri Ershov
Cc: marcel, davem, jprvita, linux-bluetooth, ville.tervo,
andrei.emeltchenko
In-Reply-To: <4CC576D3.2090304@nokia.com>
Hi Yuri,
Please no top posting in this mainling list. It's not allowed, thanks.
* Yuri Ershov <ext-yuri.ershov@nokia.com> [2010-10-25 16:23:47 +0400]:
> Hello Gustavo,
>
> The problem appears in case of multiple connect-transfer-disconnect
> sequence (e.g. by using l2test). The conditions are the following:
> There are 2 BT devices. The first one listens and receives (l2test -r),
> the second one makes "connect-disconnect-connect..." sequence (l2test -c
> -b 1000 -i hci0 -P 10 <addr>). After some time this will cause the race
> between functions bt_accept_dequeue and l2cap_chan_del. The fail sequence:
>
> struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
> {
> ...
> list_for_each_safe(p, n, &bt_sk(parent)->accept_q) {
> sk = (struct sock *) list_entry(p, struct bt_sock, accept_q);
>
> lock_sock(sk);
>
>
>
> In this time the function l2cap_chan_del sets the socket state to
> BT_CLOSED, unlinks and kills the socket.
>
>
>
> /* FIXME: Is this check still needed */
> if (sk->sk_state == BT_CLOSED) {
> release_sock(sk);
> bt_accept_unlink(sk);
> continue;
> }
>
> ...
>
> release_sock(sk);
> }
> return NULL;
> }
I agree with you, just add this info to your commit message and then
resend your patch so I can apply it.
> ext Gustavo F. Padovan wrote:
> > Hi Yuri,
> >
> > * Yuri Ershov <ext-yuri.ershov@nokia.com> [2010-10-21 20:08:58 +0400]:
> >
> >
> >> This patch fixes NULL pointer dereference at running test with
> >> connect-transfer-disconnect in loop. Sometimes sk_state is
> >> BT_CLOSED and sk_refcnt equal to 0, so there is oops in
> >> bt_accept_unlink. In normal case removed block is not used.
> >>
> >
> > Question here is: Why sk_refcnt is 0 at that point of the code? The
> > socket should be destroyed if it ref is 0, but it wasn't, so something
> > in another point of the code went is wrong. "Sometimes" is not a good
> > description of the problem, you have to show why that happened.
> >
> >
>
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH 1/9] mfd: Add support for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-10-28 10:32 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <201010261406.10996.arnd@arndb.de>
Thanks again for your comments Arnd.
Sorry for not answering your mail earlier but I've spent the last few
days changing a few hundred debug comments. :-)
See answers below.
/P-G
2010/10/26 Arnd Bergmann <arnd@arndb.de>:
> On Tuesday 26 October 2010, Par-Gunnar Hjalmdahl wrote:
>
>> 2010/10/22 Arnd Bergmann <arnd@arndb.de>:
>> > On Friday 22 October 2010 12:35:16 Par-Gunnar Hjalmdahl wrote:
>> >> This patch adds support for the ST-Ericsson CG2900 Connectivity
>> >> Combo controller.
>> >> This patch adds the central framework to be able to register CG2900 users,
>> >> transports, and chip handlers.
>> >>
>> >> Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
>> >
>> > Looks ok from a coding style perspective, but some important information is
>> > missing from the description:
>> >
>> > * What is a CG2900?
>> > * Why is it a MFD device rather than e.g. a bus or a subsystem?
>> >
>>
>> I will add some more info:
>> - CG2900 is a GPS, Bluetooth, FM controller
>> - I do not really know what makes a device qualify as a certain type,
>> but at least for us this is a multifunction device. But I can't really
>> say that it isn't really a bus (could be stated as multifunction HCI
>> bus). I guess it could be qualified as a subsystem as well, but I
>> cannot give you a reason to have it as either.
>
> It's not often completely clear, but I would draw the distinction like
> this:
>
> * A multi-function device is a single device that sits on a given bus
> with one host I/O interface and provides functionality to different
> logical subsystems like serial or alsa. A typical case for an MFD would
> be a to solve the problem of sharing resources between the child drivers
> because you cannot bind one device to two drivers. If CG2900 is a single
> piece of silicon that always looks the same way, it's probably an MFD.
>
> * A bus is a much more general abstraction which has multiple devices
> and multiple device types behind it. The bus has no idea what devices
> are attached to it at compile time, so you either probe the devices at
> boot time or define a set of devices in a board definition (platform
> devices). Driver modules then register a struct device_driver for the
> bus, which gives all matching devices to the bus. If you can have
> future CG2900 compatible devices that need to have other drivers, e.g.
> for HID or video, it should probably be a bus.
>
> * A subsystem is less clearly defined, I would call bluetooth a subsystem
> instead of just a bus because it not only matches devices with drivers
> but also has its own user-level interfaces for raw communication.
> If you want to be able to have drivers in the kernel and in user space,
> you might want to consider starting a new drivers/cg2900 subsystem, or
> integrating into the bluetooth subsystem if that makes sense.
>
Thanks for this explanation. I would like to see this as a MFD at the
moment since it is most definitely a silicon, as stated by Linus
Walleij in a separate answer.
In the future this could however be extended to be a bus, when we
could use more chips with this driver.
>> >> +config MFD_CG2900
>> >> + tristate "Support ST-Ericsson CG2900 main structure"
>> >> + depends on NET
>> >> + help
>> >> + Support for ST-Ericsson CG2900 Connectivity Combo controller main structure.
>> >> + Supports multiple functionalities muxed over a Bluetooth HCI H:4 interface.
>> >> + CG2900 support Bluetooth, FM radio, and GPS.
>> >> +
>> >
>> > Can you explain what it means to mux over a H:4 interface? Does this mean
>> > you use bluetooth infrastructure that is designed for wireless communication
>> > in order to connect on-board or on-chip devices?
>> >
>>
>> The H:4 protocol is defined in the Bluetooth Core specification. It
>> uses a single byte first in the data packet to determine an HCI
>> channel. In the Bluetooth Core specification there are 4 channels
>> specified, 0x01-0x04 (0x01 is BT command, etc). The rest of the
>> channels are reserved, but these have been used on a proprietary basis
>> to transport data to different IPs within the controller. In our API
>> we have chosen to have a channel separation on an API basis and the
>> H:4 byte is then added within the driver. So we have multiple channels
>> coming in from the users that we mux onto a single data transport.
>> That's what I mean in the text. I guess I could rewrite it a bit to
>> make it clearer.
>
> I still don't understand it, though that may be a problem on my side.
>
> Does that mean that cg2900 is to the host side a standard bluetooth HCI,
> but it uses extensions to the HCI in order to make the other functions
> available to the host?
>
> What chapter in the bluetooth core specification describes this?
>
Yes, exactly. Just for Bluetooth functionality you could more or less
use existing Bluetooth UART drivers (you still have to enable chip and
so on separately though), but in order to reach FM and GPS this
protocol has been extended. In order to have an energy efficient
solution there must also be a common driver that can shut off the chip
when not in use, hence this CG2900 driver.
You can read about the original protocol in the Bluetooth Core
specification Volume 4 Part A - UART Transport Layer.
This way to extend functionality upon the existing protocol is common
amongst chip manufacturers for combo chips including Bluetooth
functionality. Texas Instruments has already posted a driver for their
chip (in drivers/staging/ti-st) that uses a similar protocol.
>> >> +
>> >> +/**
>> >> + * add_h4_user() - Add H4 user to user storage based on supplied H4 channel.
>> >> + * @dev: Stored CG2900 device.
>> >> + * @name: Device name to identify different devices that are using
>> >> + * the same H4 channel.
>> >> + *
>> >> + * Returns:
>> >> + * 0 if there is no error.
>> >> + * -EINVAL if NULL pointer or bad channel is supplied.
>> >> + * -EBUSY if there already is a user for supplied channel.
>> >> + */
>> >> +static int add_h4_user(struct cg2900_device *dev, const char * const name)
>> >> +{
>> >
>> > Where does that name come from? Why not just use an enum if the name is
>> > only use for strncmp?
>> >
>>
>> At a point in time we used to have an enum, but from what I can see it
>> is easier to keep an API stable if you use name lookup instead. If we
>> want to have minimal changes to the API in the future this is quite a
>> flexible solution, but this code should be moved to each respective
>> chip handler.
>
> You haven't really answered the first question, where the name comes from
> (I guess the question was not too clear). Does this e.g. get passed by a
> user application, or does it come from a hardware description of some
> sort?
>
> It looks a bit like a handcoded version of the code we use for device/driver
> matching in drivers/core. If that's the case, it would be better to use
> the existing infrastructure and create a bus with devices that can be
> matched with drivers.
>
Oops, I misunderstood your question. The name is supplied by the user
calling cg2900_register_user. The names are defined for reference in
include/linux/mfd/cg2900.h.
We used to have an enum to define this, but since we thought that
using name lookup to be more future safe (and as well more "Kernel
like") we chose to use that instead. It can have been a bad choice.
Problem here is to some extent that we are beginners at Linux coding.
A straight minimal API seemed to be easiest way to solve it. As you
say we could probably use device registration as you state (we are
already using MFD cells so we could possibly extend that) but I don't
know if that would improve our code.
We also have a lot of internal dependencies (like our FM driver that
will be submitted to the Kernel community) so would like to avoid
changing APIs if possible. But if you make this an absolute demand I
guess we will have to follow.
>> >> +static ssize_t test_char_dev_read(struct file *filp, char __user *buf,
>> >> + size_t count, loff_t *f_pos)
>> >> +{
>> >> + struct sk_buff *skb;
>> >> + int bytes_to_copy;
>> >> + int err;
>> >> + struct sk_buff_head *rx_queue = &core_info->test_char_dev->rx_queue;
>> >
>> > What is this read/write interface used for?
>> >
>> > The name suggests that this is only for testing and not an essential
>> > part of your driver. Could this be made a separate driver?
>> >
>> > It also looks like you do socket communication here, can't you use
>> > a proper AF_BLUETOOTH socket to do the same?
>> >
>>
>> This interface can be used for module testing where you can have a
>> test engine in user space that acts as a chip. Yes, it could be made
>> into a separate driver but it would be a very small driver. Don't know
>> if it will be worth having it in a separate file...
>
> I don't think file size is a major argument here. If it's used for
> testing, I'd make it a separate module that defaults to not being
> built, in order to prevent people from relying on what you intended
> as a testing interface.
>
OK. We can do this.
>> We use the sk_buffer, which I guess is the reason that you mention
>> sockets. We could of course use a socket instead of a char dev, both
>> here and in the cg2900_char_dev file (which of course then would be
>> renamed). It was just a choice we made during development. We think
>> that the transport as such is more like a streaming interface, but I
>> see no real problem to have sockets. We already have several users
>> using char dev though so I would prefer to keep char dev rather than
>> switching to sockets unless you have a strong reason for this.
>> I see no reason to use AF_BLUETOOTH though, the transport is not only
>> for Bluetooth.
>
> The question here is what is appropriate. My guess was that the frame
> format was the one used in bluetooth sockets. If that's not the
> case, AF_BLUETOOTH would obviously be wrong.
>
> For the test interface, it doesn't matter that much what you use, but
> for the cg2900_char_dev we should make sure that you considered all
> the options and made a reasonable decision. What does the data format
> look like there? My impression is that the character device might be
> too low-level and it could be better to use some structured interface
> like a socket, but it really depends on what it actually does.
>
> In particular, if you can address multiple hardware units over one
> character device, it may be better to have separate devices for each
> of them.
>
What is transmitted over each char dev is individual for that channel.
For BT channels this will be BT data (normally char dev is not used
for Bluetooth since BT stack connection is done directly in Kernel
through btcg2900.c), for FM channel this will be FM data, etc. But
most of all it will not be the same format as the sockets provided by
the Bluetooth stack (at least I think so) since BT stack have
protocols below the sockets (such as L2CAP). What is sent over the
char dev is the same data as sent to the chip minus the H4 header (the
first byte).
The CG2900 driver is currently written only to support one CG2900
chip, but of course that could be extended in the future.
>> >> + CG2900_INFO("test_char_dev_read");
>> >> +
>> >> + if (skb_queue_empty(rx_queue))
>> >> + wait_event_interruptible(char_wait_queue,
>> >> + !(skb_queue_empty(rx_queue)));
>> >> +
>> >> + skb = skb_dequeue(rx_queue);
>> >> + if (!skb) {
>> >> + CG2900_INFO("skb queue is empty - return with zero bytes");
>> >> + bytes_to_copy = 0;
>> >> + goto finished;
>> >> + }
>> >> +
>> >> + bytes_to_copy = min(count, skb->len);
>> >> + err = copy_to_user(buf, skb->data, bytes_to_copy);
>> >> + if (err) {
>> >> + skb_queue_head(rx_queue, skb);
>> >> + return -EFAULT;
>> >> + }
>> >> +
>> >> + skb_pull(skb, bytes_to_copy);
>> >> +
>> >> + if (skb->len > 0)
>> >> + skb_queue_head(rx_queue, skb);
>> >> + else
>> >> + kfree_skb(skb);
>> >> +
>> >> +finished:
>> >> + return bytes_to_copy;
>> >> +}
>> >
>> > This does not handle short/continued reads.
>> >
>>
>> As I mentioned above this interface is used for testing and we
>> therefore have some restriction of usage. I don't think we need to
>> impose all things necessary for a full interface upon it.
>
> This is where it gets complicated. We don't normally add interfaces
> to the kernel unless we expect to fully support them for the forseeable
> future. It has happened more than enough in the past that somebody
> introduced a debugging interface which subsequently became a supported
> ABI because some application started relying on it.
>
> Is the code that uses the test interface even publically available?
> If not, I would recommend saving the trouble of arguing about it
> and leaving out the interface.
>
OK. Good idea. I will split out the test code to a separate file and
leave it out of next patch set.
>> >> + #define CG2900_ERR(fmt, arg...) \
>> >> + do { \
>> >> + if (cg2900_debug_level >= 1) \
>> >> + pr_err("CG2900 %s: " fmt "\n" , __func__ , ## arg); \
>> >> + } while (0)
>> >> +
>> >> +#endif /* NDEBUG */
>> >
>> > You'll feel relieved when all this is gone...
>> >
>>
>> The only thing is it's been pretty nice to have it, but I will remove it.
>> Is it OK to keep defines so we can have "CG2900" in front and "\n"
>> after the text?
>
> Ideally, you would use the dev_* functions instead of the pr_* functions,
> which print the name of the device before the message. I also wouldn't
> add the "\n" in the macro because all the regular printing functions don't
> do it. It will likely only confuse people and the binary remains the same.
>
> Arnd
>
OK. I've updated the code (will come in next patch set). I use dev_dbg
where possible.
/P-G
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-10-28 10:37 UTC (permalink / raw)
To: Alan Cox; +Cc: linus.walleij, linux-bluetooth, linux-kernel, Lukasz.Rymanowski
In-Reply-To: <20101022163359.5b22a61b@lxorguk.ukuu.org.uk>
Thanks again for your comments Alan.
Next patch set will contain resolution to all your comments. See below.
/P-G
2010/10/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> The existence of the callback is checked in the function
>> uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow
>> sleep and this code will never run.
>
> OK yes see this now.
>
>> >> + CG2900_ERR("Failed to alloc memory for uart_work_struct!");
>> >
>> > Please use the standard dev_dbg etc functionality - or pr_err etc when
>> > you have no device pointer. The newest kernel tty object has a device
>> > pointer added so you could use that.
>> >
>>
>> OK. I like the debug system we have now (using module parameter to set
>> debug level in runtime), but I've received comments regarding this
>> before so I assume we will have to switch to standard printouts.
>> Is it OK to use defines or inline functions to add "CG2900" before and
>> '\n' after (as BT_INFO in include/net/bluetooth/bluetooth.h)?
>
> The pr_fmt bit will do what you want for a non dev_dbg type thing. See
> include/linux/kernel.h
>
OK. Added. I'm however using dev_err, dev_dbg, etc where possible.
>> >> + * unset_cts_irq() - Disable interrupt on CTS.
>> >> + */
>> >> +static void unset_cts_irq(void)
>> >
>> > And no ability to support multiple devices
>> >
>>
>> OK. We will try to fix this.
>
> That may go away when you clean up the device side. The line discipline
> can be attached to any device so must be multi-device aware, the hardware
> driver can certainly be single device only if you can only ever have one
>
> [Although its a good idea to design it so it can be fixed because you
> never know when you'll find your sales team just sold someone a two
> device solution ;) ]
>
OK. Design still ongoing, but will be fixed in some way.
>> >> + set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> >> + len = tty->ops->write(tty, skb->data, skb->len);
>> >
>> > A tty isn't required to have a write function
>>
>> I don't quite understand your comment here. This particular code is
>> inspired of the Bluetooth ldisc code and there it is used like. It
>> seems to work fine so we do it the same way.
>
> You copied a security hole from the bluetooth driver which we found a
> couple of weeks ago
>
>> How would you prefer it to be?
>
> Check it is valid, in your case probably on open just refuse to attach to
> a read only port.
>
OK. Done.
>> OK. We will try to figure out a new design.
>> I'm not too happy about putting the ldisc part in Bluetooth though
>> since it is only partly Bluetooth, it is also GPS and FM. Better could
>> maybe be under char/?
>
> Works for me - there is an ongoing "we must move tty ldiscs and core tty
> code somewhere more sensible of their own" discussion, so if it is
> dropped into char, it'll move with them just fine.
>
> Alan
>
Again, thanks for the comments.
/P-G
^ permalink raw reply
* [PATCH] bluetooth: Fix NULL pointer dereference issue
From: Yuri Ershov @ 2010-10-28 10:52 UTC (permalink / raw)
To: marcel, davem, padovan, jprvita
Cc: linux-bluetooth, ville.tervo, andrei.emeltchenko, Yuri Ershov
This patch fixes NULL pointer dereference at running test with
connect-transfer-disconnect in loop. The problem conditions are the
following: there are 2 BT devices. The first one listens and
receives (l2test -r), the second one makes "connect-disconnect-
connect..." sequence (l2test -c -b 1000 -i hci0 -P 10 <addr>).
After some time this will cause the race between functions
bt_accept_dequeue and l2cap_chan_del. The function l2cap_chan_del
sets the socket state to BT_CLOSED, unlinks and kills the socket
in the middle of bt_accept_dequeue, then at running the removed code
kernel oops appears.
Signed-off-by: Yuri Ershov <ext-yuri.ershov@nokia.com>
---
net/bluetooth/af_bluetooth.c | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 421c45b..47c107e 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -204,13 +204,6 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
lock_sock(sk);
- /* FIXME: Is this check still needed */
- if (sk->sk_state == BT_CLOSED) {
- release_sock(sk);
- bt_accept_unlink(sk);
- continue;
- }
-
if (sk->sk_state == BT_CONNECTED || !newsock ||
bt_sk(parent)->defer_setup) {
bt_accept_unlink(sk);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH 2/9] mfd: Add char devices for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-10-28 11:06 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <201010221749.37928.arnd@arndb.de>
Hi Arnd,
Thanks for your comments. Everyone appreciates the time and effort
you've put into the review of this code.
See below for answer.
/P-G
2010/10/22 Arnd Bergmann <arnd@arndb.de>:
> On Friday 22 October 2010 12:36:09 Par-Gunnar Hjalmdahl wrote:
>> This patch adds char devices to the ST-Ericsson CG2900 driver.
>> The reason for this is to allow users of CG2900, such as GPS, to
>> be placed in user space.
>
> Can you be more specific how you expect this to be used?
>
> I guess you have hardware units that then each correspond to
> one character device and can be talked to over a pseudo socket
> interface, right?
>
> For most devices (radio, audio, bluetooth, gps, ...), we already
> have existing user interfaces, so how do they interact with this
> one?
>
Char dev exposes the interfaces to user space. Each char dev should
correspond to a channel in the cg2900.h API. The API allows one user
for each channel and this user can either be a Kernel user through a
direct call or a User space user through opening the corresponding
char dev.
Our reference system have the following users:
BT (cmd, acl, and evt) through Kernel BT stack (/net/bluetooth).
Connected by btcg2900 in this patch set
FM through V2L2 radio device (/drivers/media/radio). Connected by
CG2900 FM driver for V2L2 (under development, will be submitted to
community).
GPS through stack located in User space
We do not however want to block anyone from using a different
combination, e.g. using different BT or FM stack (placed in Kernel or
User space). The CG2900 driver is only intended to present a reliable
and power efficient transport to the different features within the
chip.
Normal flow for a user space stack is:
User space user opens char dev -> char dev registers corresponding
channel user using cg2900_register_user
User space user can now send and receive data to the chip using write,
read, and poll. Data is in raw format, cg2900 only adds first byte (H4
header) to packet.
When finished, User space user closes char dev -> char dev unregisters
channel using cg2900_deregister_user
>> +#define NAME "CharDev "
>
> ?
>
>> +/* Ioctls */
>> +#define CG2900_CHAR_DEV_IOCTL_RESET _IOW('U', 210, int)
>> +#define CG2900_CHAR_DEV_IOCTL_CHECK4RESET _IOR('U', 212, int)
>> +#define CG2900_CHAR_DEV_IOCTL_GET_REVISION _IOR('U', 213, int)
>> +#define CG2900_CHAR_DEV_IOCTL_GET_SUB_VER _IOR('U', 214, int)
>
> These definitions look wrong -- you never use the ioctl argument...
>
Yes, we return values in the error which is probably not a good
solution. We should probably use argument instead. But I don't see
what's wrong with the defines (but most likely I've misunderstood how
they should be used :-) )?
>> + *
>> + * Returns:
>> + * Bytes successfully read (could be 0).
>> + * -EBADF if NULL pointer was supplied in private data.
>> + * -EFAULT if copy_to_user fails.
>> + * Error codes from wait_event_interruptible.
>> + */
>> +static ssize_t char_dev_read(struct file *filp, char __user *buf, size_t count,
>> + loff_t *f_pos)
>
> The same comment applies here that I made for the test interface:
> Why is this not an AF_BLUETOOTH socket instead of a chardev?
>
> Unless I'm mistaken, you actually send bluetooth frames after all.
>
I've discussed this in another mail, but to repeat in short:
For the BT channels we send raw BT data, but it is not on the same
level as for example the L2CAP socket in the BT stack. So I think it
would not apply, It would only be confusing to have the same kind of
socket on different stack levels (plus it does not apply for FM and
GPS).
>> + case CG2900_CHAR_DEV_IOCTL_RESET:
>> + if (!dev) {
>> + err = -EBADF;
>> + goto error_handling;
>> + }
>> + CG2900_INFO("ioctl reset command for device %s", dev->name);
>> + err = cg2900_reset(dev->dev);
>> + break;
>> +
>> + case CG2900_CHAR_DEV_IOCTL_CHECK4RESET:
>> + if (!dev) {
>> + CG2900_INFO("ioctl check for reset command for device");
>> + /* Return positive value if closed */
>> + err = CG2900_CHAR_DEV_IOCTL_EVENT_CLOSED;
>> + } else if (dev->reset_state == CG2900_CHAR_RESET) {
>> + CG2900_INFO("ioctl check for reset command for device "
>> + "%s", dev->name);
>> + /* Return positive value if reset */
>> + err = CG2900_CHAR_DEV_IOCTL_EVENT_RESET;
>> + }
>> + break;
>
> Strange interface. Why do you need to check for the reset?
>
Any user (Kernel or User space) can reset the chip (this is a HW
reset, not a SW reset). If someone does this, all settings made to the
chip are lost and chip is turned off. We can however not signal up to
User space that it now has to do a close and re-open operation plus
restart its stack (possibly informing the end-user about the restart).
By using poll and ioctl we can do this without disturbing other
operations. If User space application tries to perform operation on a
reset device, e.g. write, this will fail, but using ioctl provides the
opportunity to do this without any fake read or write operations.
>> + case CG2900_CHAR_DEV_IOCTL_GET_REVISION:
>> + CG2900_INFO("ioctl check for local revision info");
>> + if (cg2900_get_local_revision(&rev_data)) {
>> + CG2900_DBG("Read revision data revision %d "
>> + "sub_version %d",
>> + rev_data.revision, rev_data.sub_version);
>> + err = rev_data.revision;
>> + } else {
>> + CG2900_DBG("No revision data available");
>> + err = -EIO;
>> + }
>> + break;
>> +
>> + case CG2900_CHAR_DEV_IOCTL_GET_SUB_VER:
>> + CG2900_INFO("ioctl check for local sub-version info");
>> + if (cg2900_get_local_revision(&rev_data)) {
>> + CG2900_DBG("Read revision data revision %d "
>> + "sub_version %d",
>> + rev_data.revision, rev_data.sub_version);
>> + err = rev_data.sub_version;
>> + } else {
>> + CG2900_DBG("No revision data available");
>> + err = -EIO;
>> + }
>> + break;
>
> These look like could better live in a sysfs attribute of the platform device.
>
> Arnd
>
I guess so. I thought it would be more simple to have it in the ioctl
since I already have other ioctl parameters, but I can probably fix it
quite easily.
/P-G
^ permalink raw reply
* Unable to get on D-Bus: bluez 4.77
From: Puneet Pant @ 2010-10-28 11:32 UTC (permalink / raw)
To: linux-bluetooth
Hello,
I have compiled Bluez 4.77. But I'm getting the following error when
trying to start Bluetooth Daemon:
root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src#
dbus-daemon --system
root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src# ./bluetoothd -nd
bluetoothd[11819]: Bluetooth deamon 4.77
bluetoothd[11819]: src/main.c:parse_config() parsing main.conf
bluetoothd[11819]: src/main.c:parse_config() discovto=0
bluetoothd[11819]: src/main.c:parse_config() pairto=0
bluetoothd[11819]: src/main.c:parse_config() pageto=8192
bluetoothd[11819]: src/main.c:parse_config() name=%h-%d
bluetoothd[11819]: src/main.c:parse_config() class=0x000100
bluetoothd[11819]: src/main.c:parse_config() discov_interval=0
bluetoothd[11819]: src/main.c:parse_config() Key file does not have
key 'DeviceID'
bluetoothd[11819]: Unable to get on D-Bus
I've built kernel version 2.6.36-rc7 with LE patches from Ville Tervo
and am running it on an Ubuntu 10.10 distribution. Installed Dbus
Library version is 1.2.24 (I have also tried with 1.4.0, but get same
error)
I have tried to google around on this but couldn't find anything
relevant that could fix this issue.
Can someone kindly suggest if this is related to my linux pc
configuration or more of Bluez issue?
Thanks in advance.
Puneet
^ permalink raw reply
* Re: Unable to get on D-Bus: bluez 4.77
From: Gustavo F. Padovan @ 2010-10-28 12:09 UTC (permalink / raw)
To: Puneet Pant; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimTzSMQW3cJQPphWgTBKgn0fbN7zui7tRA1enmi@mail.gmail.com>
Hi Puneet,
* Puneet Pant <pant.puneet464@gmail.com> [2010-10-28 17:02:48 +0530]:
> Hello,
>
> I have compiled Bluez 4.77. But I'm getting the following error when
> trying to start Bluetooth Daemon:
>
> root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src#
> dbus-daemon --system
> root@puneet-VirtualBox:/home/puneet/packages/bluez-4.77/src# ./bluetoothd -nd
> bluetoothd[11819]: Bluetooth deamon 4.77
> bluetoothd[11819]: src/main.c:parse_config() parsing main.conf
> bluetoothd[11819]: src/main.c:parse_config() discovto=0
> bluetoothd[11819]: src/main.c:parse_config() pairto=0
> bluetoothd[11819]: src/main.c:parse_config() pageto=8192
> bluetoothd[11819]: src/main.c:parse_config() name=%h-%d
> bluetoothd[11819]: src/main.c:parse_config() class=0x000100
> bluetoothd[11819]: src/main.c:parse_config() discov_interval=0
> bluetoothd[11819]: src/main.c:parse_config() Key file does not have
> key 'DeviceID'
> bluetoothd[11819]: Unable to get on D-Bus
One of the reasons could be that you already have the bluetoothd daemon
running.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH 1/9] mfd: Add support for the ST-Ericsson CG2900.
From: Arnd Bergmann @ 2010-10-28 12:18 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <AANLkTin72z7AmfkMMH01siwkNNNvHM+1+qXLodmaUx5q@mail.gmail.com>
On Thursday 28 October 2010, Par-Gunnar Hjalmdahl wrote:
> Thanks for this explanation. I would like to see this as a MFD at the
> moment since it is most definitely a silicon, as stated by Linus
> Walleij in a separate answer.
> In the future this could however be extended to be a bus, when we
> could use more chips with this driver.
Ok, fair enough. Time will tell if you need this as a bus then,
or alternatively use multiple multi-function drivers, perhaps
with some shared code in form of another "library" module.
The only important part is that you do not introduce user-visible
interfaces that can not be changed if you decide to rewrite the
implementation.
> > Does that mean that cg2900 is to the host side a standard bluetooth HCI,
> > but it uses extensions to the HCI in order to make the other functions
> > available to the host?
> >
> > What chapter in the bluetooth core specification describes this?
> >
>
> Yes, exactly. Just for Bluetooth functionality you could more or less
> use existing Bluetooth UART drivers (you still have to enable chip and
> so on separately though), but in order to reach FM and GPS this
> protocol has been extended. In order to have an energy efficient
> solution there must also be a common driver that can shut off the chip
> when not in use, hence this CG2900 driver.
> You can read about the original protocol in the Bluetooth Core
> specification Volume 4 Part A - UART Transport Layer.
> This way to extend functionality upon the existing protocol is common
> amongst chip manufacturers for combo chips including Bluetooth
> functionality. Texas Instruments has already posted a driver for their
> chip (in drivers/staging/ti-st) that uses a similar protocol.
Hmm, this sounds like it's at least one level below where I expected
it. So your cg2900 is connected through a UART and behaves like a serial
bluetooth host, right?
If that's the case, I think what you should have done is to make the
low-level interface available to Linux as a tty/serial driver,
which gets switched to the HCI line discipline.
The code for this is in drivers/bluetooth/hci_ldisc.c.
On top of this sits the hci_h4 protocol driver, drivers/bluetooth/hci_h4.c
and you additional functionality would have to hook into that.
Is this a setup you have considered? Any particular reasons why you
decided against it? Or are you actually doing exactly this and I'm
just too blind to see it?
> Oops, I misunderstood your question. The name is supplied by the user
> calling cg2900_register_user. The names are defined for reference in
> include/linux/mfd/cg2900.h.
> We used to have an enum to define this, but since we thought that
> using name lookup to be more future safe (and as well more "Kernel
> like") we chose to use that instead. It can have been a bad choice.
> Problem here is to some extent that we are beginners at Linux coding.
We generally use strings for stuff that comes from hardware or
user and needs to be extensible, like the driver names.
In your case, there is no such requirement, so an enum would probably
have been much simpler.
> A straight minimal API seemed to be easiest way to solve it. As you
> say we could probably use device registration as you state (we are
> already using MFD cells so we could possibly extend that) but I don't
> know if that would improve our code.
Using mfd cells is reasonable, but you really should not try to duplicate
infrastructure code by introducing another layer on top.
I'm not familiar with MFD, but it's clear that your code took a wrong
turn in some place when you had to do that.
>From what I can tell, the slave drivers should only need to register
a platform_driver that binds to the mfd cells created here, but
the base driver should not at all need to interact with the slaves
otherwise.
> We also have a lot of internal dependencies (like our FM driver that
> will be submitted to the Kernel community) so would like to avoid
> changing APIs if possible. But if you make this an absolute demand I
> guess we will have to follow.
Changing internal APIs is not an issue, we do that all the time and you
really need not be afraid of it.
The thing we don't do is to change the user-visible kernel ABI!
I'm not asking to specifically change the string into an enum, the
real question is if you use the right abstraction, and the fact that
you had to choose an identifier like this hints that you are not.
This issue probably disappears once the question of the MFD integration
is solved.
> What is transmitted over each char dev is individual for that channel.
> For BT channels this will be BT data (normally char dev is not used
> for Bluetooth since BT stack connection is done directly in Kernel
> through btcg2900.c), for FM channel this will be FM data, etc. But
> most of all it will not be the same format as the sockets provided by
> the Bluetooth stack (at least I think so) since BT stack have
> protocols below the sockets (such as L2CAP). What is sent over the
> char dev is the same data as sent to the chip minus the H4 header (the
> first byte).
I see.
One point where I think the abstraction went wrong is that you have
a matrix of drivers and channels: The chardev is registered as
an MFD cell and the driver binds to that cell, creating a device
node for each channel (BT_CMD, FM_RADIO, ...).
What you should have instead is a single MFD cell for each channel
and let each channel bind to one driver!
The channel is the primary differentiator between functionality
on the CG2900, so that is what becomes your cell. Since you added
another abstraction on top, you had to duplicate core functionality
of the kernel, which is what I initially saw as "this looks wrong,
no idea what's going on".
> The CG2900 driver is currently written only to support one CG2900
> chip, but of course that could be extended in the future.
Ok. So if you use the MFD framework correctly, future chips
will require a new base driver but the slaves that you already
have drivers for will be able to use their drivers without modification,
right?
Arnd
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Arnd Bergmann @ 2010-10-28 12:22 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl
Cc: Alan Cox, linus.walleij, linux-bluetooth, linux-kernel,
Lukasz.Rymanowski
In-Reply-To: <AANLkTik-S50ZX6mFG+vWhBv0x_WmipP=t+4VqmAgssRC@mail.gmail.com>
On Friday 22 October 2010, Par-Gunnar Hjalmdahl wrote:
> >
> > So - NAK this for the moment, it needs to be split cleanly into ldisc
> > (thing which speaks the protocol and eg sees "speed change required" and
> > acts on it) and device (thing which knows about the hardware).
>
> OK. We will try to figure out a new design.
> I'm not too happy about putting the ldisc part in Bluetooth though
> since it is only partly Bluetooth, it is also GPS and FM. Better could
> maybe be under char/?
After getting a better idea of what the base mfd driver does, my impression
is now that you should not register a second N_HCI line discipline at all,
but instead extend the existing line discipline with this number.
I'm not sure what happens if you need two modules that try to register
the same ldisc number, but I imagine it is not good.
Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
Arnd
^ permalink raw reply
* Re: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session
From: Gustavo F. Padovan @ 2010-10-28 13:07 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, ville.tervo
In-Reply-To: <1282215970-32758-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2010-08-19 14:06:10 +0300]:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>
> This cause 'No Bonding' to be used if userspace has not yet been paired
> with remote device since the l2cap socket used to create the rfcomm
> session does not have any security level set.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> ---
> net/bluetooth/rfcomm/core.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
Patch has been applied to the bluetooth-2.6 tree. Thanks.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH] Bluetooth: Automate remote name requests
From: Johan Hedberg @ 2010-10-28 13:56 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <AANLkTikKGDtybssKjoNUACAwzg1HA7VC4rWrnNFvJ-2u@mail.gmail.com>
Hi Luiz,
On Thu, Oct 28, 2010, Luiz Augusto von Dentz wrote:
> This seems to be done only for some connections which IMO is not the
> correct, so for instance it wouldn't request name for SDP connections
> nor for incoming connections, is there a reason for that?
>
> Perhaps requesting the name regardless the security level on
> hci_remote_features_evt and then latter, when we got the name
> response, we continue with security level check and authentication
> request if necessary. Wouldn't that work better?
Yeah, you're right. Basicly what the patch does is replace the
occurences of AUTH_REQ with NAME_REQ and then do the AUTH_REQ in the
name_req_complete callback. However that is not the same as userspace is
doing which is an unconditional name request for every single connect
complete event. FWIW, there seems to be a potential bug with the
existing authentication request too: the code doesn't trigger it if the
extended features request fails (hci_cs_read_remote_ext_features
function).
I'll try to come up with a new revision of the name request patch
shortly.
Johan
^ 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