* Re: pull request: bluetooth-next-2.6 2010-10-13
From: Luis R. Rodriguez @ 2010-10-13 21:41 UTC (permalink / raw)
To: Gustavo F. Padovan
Cc: linville, marcel, linux-wireless, linux-bluetooth, linux-kernel
In-Reply-To: <20101013034808.GA3205@vigoh>
On Tue, Oct 12, 2010 at 8:48 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> One notable change is in the MAINTAINERS file, From now and on I'm going
> to maintain the Bluetooth trees as well as send the pull requests to you.
I noted here you mentioned "as well", does this mean there may be
updates from two people now instead of just one?
> Despite this maintenance change, Marcel will remain helping with patch
> review, and ACK/NAK in the Bluetooth subsystem as he always did. Not a
> big change in the end. ;)
Luis
^ permalink raw reply
* Re: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900
From: Marcel Holtmann @ 2010-10-13 21:48 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl
Cc: linux-bluetooth, linux-kernel, linus.walleij, Pavan Savoy
In-Reply-To: <AANLkTinCUscmm=1t7iEAz2LQ0O_TM5-G7zANhLcuA-d9@mail.gmail.com>
Hi Par-Gunnar,
> Sorry for not answering to your mail earlier. I've been on a business
> trip whole last week where I just did not have the possibility to
> answer.
>
> We are currently working on a new version of our driver, both the MFD
> and the Bluetooth part, where we address the comments that we have so
> far received. Hopefully I will able to send it later this week.
>
> As answer to last mail: yes, we will change our debug system to be
> reuse existing functionality in the Kernel.
sounds good to me. dynamic debug is actually pretty nice :)
>
> /P-G
>
> 2010/10/5 Marcel Holtmann <marcel@holtmann.org>:
> > Hi Par-Gunnar,
> >
> >> This patch adds support for using the ST-Ericsson CG2900
> >> connectivity controller as a driver for the BlueZ Bluetooth
> >> stack.
> >> This patch registers as a driver into the BlueZ framework and, when
> >> opened by BlueZ, it registers as user for bt_cmd, bt_acl, and bt_evt
> >> channels.
> >>
> >> Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
> >> ---
> >> drivers/bluetooth/Kconfig | 7 +
> >> drivers/bluetooth/Makefile | 2 +
> >> drivers/bluetooth/cg2900_hci.c | 896 ++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 905 insertions(+), 0 deletions(-)
> >> create mode 100644 drivers/bluetooth/cg2900_hci.c
> >>
> >> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> >> index 02deef4..9ca8d69 100644
> >> --- a/drivers/bluetooth/Kconfig
> >> +++ b/drivers/bluetooth/Kconfig
> >> @@ -219,4 +219,11 @@ config BT_ATH3K
> >> Say Y here to compile support for "Atheros firmware download driver"
> >> into the kernel or say M to compile it as module (ath3k).
> >>
> >> +config BT_CG2900
> >> + tristate "ST-Ericsson CG2900 driver"
> >> + depends on MFD_CG2900 && BT
> >> + help
> >> + Select if ST-Ericsson CG2900 Connectivity controller shall be used as
> >> + Bluetooth controller for BlueZ.
> >> +
> >> endmenu
> >> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> >> index 71bdf13..a479c16 100644
> >> --- a/drivers/bluetooth/Makefile
> >> +++ b/drivers/bluetooth/Makefile
> >> @@ -19,6 +19,8 @@ obj-$(CONFIG_BT_ATH3K) += ath3k.o
> >> obj-$(CONFIG_BT_MRVL) += btmrvl.o
> >> obj-$(CONFIG_BT_MRVL_SDIO) += btmrvl_sdio.o
> >>
> >> +obj-$(CONFIG_BT_CG2900) += cg2900_hci.o
> >> +
> >
> > Please sort this after ath3k and before btmvrl config.
> >
> > And for the name either just use cg2900 if it uniquely identifies the
> > Bluetooth chip used in the combo or use btcg2900.o as module name if it
> > is the name of the combo module.
> >
> > We clearly moved into the direction of either unique hardware names
> > without bt prefix or we have the bt prefix in front of it. The fully
> > generic term hci will be phased out of the driver names.
> >
>
> I can change the name to btcg2900.o since the whole chip is called
> cg2900 and this file is just for the BT part of it.
> I will also sort it correctly into the file.
Actually btcg2900.ko sounds good to me.
> >> btmrvl-y := btmrvl_main.o
> >> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
> >>
> >> diff --git a/drivers/bluetooth/cg2900_hci.c b/drivers/bluetooth/cg2900_hci.c
> >> new file mode 100644
> >> index 0000000..de1ada8
> >> --- /dev/null
> >> +++ b/drivers/bluetooth/cg2900_hci.c
> >> @@ -0,0 +1,896 @@
> >> +/*
> >> + * drivers/bluetooth/cg2900_hci.c
> >> + *
> >> + * Copyright (C) ST-Ericsson SA 2010
> >> + * Authors:
> >> + * Par-Gunnar Hjalmdahl (par-gunnar.p.hjalmdahl@stericsson.com) for
> >> ST-Ericsson.
> >> + * Henrik Possung (henrik.possung@stericsson.com) for ST-Ericsson.
> >> + * Josef Kindberg (josef.kindberg@stericsson.com) for ST-Ericsson.
> >> + * Dariusz Szymszak (dariusz.xd.szymczak@stericsson.com) for ST-Ericsson.
> >> + * Kjell Andersson (kjell.k.andersson@stericsson.com) for ST-Ericsson.
> >> + * License terms: GNU General Public License (GPL), version 2
> >> + *
> >> + * Linux Bluetooth HCI H:4 Driver for ST-Ericsson CG2900 connectivity
> >> controller
> >> + * towards the BlueZ Bluetooth stack.
> >> + */
> >
> > Drop the filename in this header and don't bother with the description
> > towards BlueZ or Bluetooth subsystem. That is clear from the location of
> > the file.
> >
> > Also what is up with the "for ST-Ericsson"?
> >
>
> OK regarding the header comments. We have been instructed to use "for
> ST-Ericsson" when writing author name. Is that a problem?
I find this fully pointless since a) you have an stericsson.com email
address and b) the code is copyright by ST-Ericsson. So my take is, yes,
we get it ST-Ericsson has written this code.
> >> +
> >> +#include <linux/module.h>
> >> +#include <linux/types.h>
> >> +#include <linux/skbuff.h>
> >> +#include <asm/byteorder.h>
> >> +#include <net/bluetooth/bluetooth.h>
> >> +#include <net/bluetooth/hci.h>
> >> +#include <net/bluetooth/hci_core.h>
> >> +
> >> +#include <linux/workqueue.h>
> >> +#include <linux/wait.h>
> >> +#include <linux/time.h>
> >> +#include <linux/jiffies.h>
> >> +#include <linux/sched.h>
> >> +#include <linux/timer.h>
> >> +
> >> +#include <linux/mfd/cg2900.h>
> >> +#include <mach/cg2900_devices.h>
> >> +
> >> +/* module_param declared in cg2900_core.c */
> >> +extern int cg2900_debug_level;
> >> +
> >> +#define NAME "CG2900 HCI"
> >> +
> >> +/* Debug defines */
> >> +#define CG2900_DBG_DATA(fmt, arg...) \
> >> +do { \
> >> + if (cg2900_debug_level >= 25) \
> >> + printk(KERN_DEBUG NAME " %s: " fmt "\n" , __func__ , ## arg); \
> >> +} while (0)
> >> +
> >> +#define CG2900_DBG(fmt, arg...) \
> >> +do { \
> >> + if (cg2900_debug_level >= 20) \
> >> + printk(KERN_DEBUG NAME " %s: " fmt "\n" , __func__ , ## arg); \
> >> +} while (0)
> >> +
> >> +#define CG2900_INFO(fmt, arg...) \
> >> +do { \
> >> + if (cg2900_debug_level >= 10) \
> >> + printk(KERN_INFO NAME ": " fmt "\n" , ## arg); \
> >> +} while (0)
> >> +
> >> +#define CG2900_ERR(fmt, arg...) \
> >> +do { \
> >> + if (cg2900_debug_level >= 1) \
> >> + printk(KERN_ERR NAME " %s: " fmt "\n" , __func__ , ## arg); \
> >> +} while (0)
> >
> > Remove all this debug stuff. Use BT_DBG, BT_ERR etc.
>
> OK
>
> >
> >> +#define CG2900_SET_STATE(__name, __var, __new_state) \
> >> +do { \
> >> + CG2900_DBG("New %s: 0x%X", __name, (uint32_t)__new_state); \
> >> + __var = __new_state; \
> >> +} while (0)
> >
> > What is this for? Please don't do that. It just obfuscates the code.
> >
>
> Is it OK to use inline functions instead of defines? It's pretty
> useful to be able to printout when changing state.
> So something like:
>
> static inline void set_reset_state(enum reset_state new_state)
> {
> BT_DBG("New reset state: %x", new_state);
> hci_info->reset_state = new_state;
> }
Not really. It just clutters the code. Do the debugging with dynamic
debug and keep the code readable.
I do see where you are coming from and it might be useful during initial
development. For longterm maintainability this is not helping.
> >> +/* HCI device type */
> >> +#define HCI_CG2900 HCI_VIRTUAL
> >
> > This is the wrong type. Don't do that. And don't create a new define for
> > it. Use the standard types. I assume that HCI_UART is most likely what
> > you want here.
> >
>
> The problem here is that the physical transport used is not shown for
> this module. It is handled with the CG2900 MFD driver. But I guess I
> could expose the type through an API function.
Yes, please do that instead, but essentially HCI_UART is a way better
fit than HCI_VIRTUAL if you are in doubt.
> >> +/* Wait for 5 seconds for a response to our requests */
> >> +#define RESP_TIMEOUT 5000
> >> +
> >> +/* State-setting defines */
> >> +#define SET_RESET_STATE(__hci_reset_new_state) \
> >> + CG2900_SET_STATE("reset_state", hci_info->reset_state, \
> >> + __hci_reset_new_state)
> >> +#define SET_ENABLE_STATE(__hci_enable_new_state) \
> >> + CG2900_SET_STATE("enable_state", hci_info->enable_state, \
> >> + __hci_enable_new_state)
> >
> > Don't do this. It is just highly obfuscating the code flow.
> >
>
> As stated above, is it OK to use static inline functions instead?
No. Just fix the code to do it properly there. You will see that it is
just fine.
> >> +/* Bluetooth error codes */
> >> +#define HCI_ERR_NO_ERROR 0x00
> >> +#define HCI_ERR_CMD_DISALLOWED 0x0C
> >> +
> >> +/**
> >> + * enum reset_state - RESET-states of the HCI driver.
> >> + *
> >> + * @RESET_IDLE: No reset in progress.
> >> + * @RESET_ACTIVATED: Reset in progress.
> >> + * @RESET_UNREGISTERED: hdev is unregistered.
> >> + */
> >> +
> >> +enum reset_state {
> >> + RESET_IDLE,
> >> + RESET_ACTIVATED,
> >> + RESET_UNREGISTERED
> >> +};
> >> +
> >> +/**
> >> + * enum enable_state - ENABLE-states of the HCI driver.
> >> + *
> >> + * @ENABLE_IDLE: The HCI driver is loaded but not opened.
> >> + * @ENABLE_WAITING_BT_ENABLED_CC: The HCI driver is waiting for a command
> >> + * complete event from the BT chip as a
> >> + * response to a BT Enable (true) command.
> >> + * @ENABLE_BT_ENABLED: The BT chip is enabled.
> >> + * @ENABLE_WAITING_BT_DISABLED_CC: The HCI driver is waiting for a command
> >> + * complete event from the BT chip as a
> >> + * response to a BT Enable (false) command.
> >> + * @ENABLE_BT_DISABLED: The BT chip is disabled.
> >> + * @ENABLE_BT_ERROR: The HCI driver is in a bad state, some
> >> + * thing has failed and is not expected to
> >> + * work properly.
> >> + */
> >> +enum enable_state {
> >> + ENABLE_IDLE,
> >> + ENABLE_WAITING_BT_ENABLED_CC,
> >> + ENABLE_BT_ENABLED,
> >> + ENABLE_WAITING_BT_DISABLED_CC,
> >> + ENABLE_BT_DISABLED,
> >> + ENABLE_BT_ERROR
> >> +};
> >> +
> >> +/**
> >> + * struct hci_info - Specifies HCI driver private data.
> >> + *
> >> + * This type specifies CG2900 HCI driver private data.
> >> + *
> >> + * @bt_cmd: Device structure for BT command channel.
> >> + * @bt_evt: Device structure for BT event channel.
> >> + * @bt_acl: Device structure for BT ACL channel.
> >> + * @hdev: Device structure for HCI device.
> >> + * @reset_state: Device enum for HCI driver reset state.
> >> + * @enable_state: Device enum for HCI driver BT enable state.
> >> + */
> >> +struct hci_info {
> >> + struct cg2900_device *bt_cmd;
> >> + struct cg2900_device *bt_evt;
> >> + struct cg2900_device *bt_acl;
> >> + struct hci_dev *hdev;
> >> + enum reset_state reset_state;
> >> + enum enable_state enable_state;
> >> +};
> >> +
> >> +/**
> >> + * struct dev_info - Specifies private data used when receiving
> >> callbacks from CPD.
> >> + *
> >> + * @hdev: Device structure for HCI device.
> >> + * @hci_data_type: Type of data according to BlueZ.
> >> + */
> >> +struct dev_info {
> >> + struct hci_dev *hdev;
> >> + u8 hci_data_type;
> >> +};
> >> +
> >> +/* Variables where LSB and MSB for bt_enable command is stored */
> >> +static u16 bt_enable_cmd;
> >> +
> >> +static struct hci_info *hci_info;
> >> +
> >> +/*
> >> + * hci_wait_queue - Main Wait Queue in HCI driver.
> >> + */
> >> +static DECLARE_WAIT_QUEUE_HEAD(hci_wait_queue);
> >> +
> >> +/* Internal function declarations */
> >> +static int register_to_bluez(void);
> >
> > Don't use bluez in kernel code. Just use bluetooth or bt.
> >
>
> OK
>
> >> +/* Internal functions */
> >> +
> >> +/**
> >> + * remove_bt_users() - Unregister and remove any existing BT users.
> >> + * @info: HCI driver info structure.
> >> + */
> >> +static void remove_bt_users(struct hci_info *info)
> >> +{
> >> + if (info->bt_cmd) {
> >> + kfree(info->bt_cmd->user_data);
> >> + info->bt_cmd->user_data = NULL;
> >> + cg2900_deregister_user(info->bt_cmd);
> >> + info->bt_cmd = NULL;
> >> + }
> >> +
> >> + if (info->bt_evt) {
> >> + kfree(info->bt_evt->user_data);
> >> + info->bt_evt->user_data = NULL;
> >> + cg2900_deregister_user(info->bt_evt);
> >> + info->bt_evt = NULL;
> >> + }
> >> +
> >> + if (info->bt_acl) {
> >> + kfree(info->bt_acl->user_data);
> >> + info->bt_acl->user_data = NULL;
> >> + cg2900_deregister_user(info->bt_acl);
> >> + info->bt_acl = NULL;
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * hci_read_cb() - Callback for handling data received from CG2900 driver.
> >> + * @dev: Device receiving data.
> >> + * @skb: Buffer with data coming from device.
> >> + */
> >> +static void hci_read_cb(struct cg2900_device *dev, struct sk_buff *skb)
> >> +{
> >> + int err = 0;
> >> + struct dev_info *dev_info;
> >> + struct hci_event_hdr *evt;
> >> + struct hci_ev_cmd_complete *cmd_complete;
> >> + struct hci_ev_cmd_status *cmd_status;
> >> + u8 status;
> >> +
> >> + if (!skb) {
> >> + CG2900_ERR("NULL supplied for skb");
> >> + return;
> >> + }
> >> +
> >> + if (!dev) {
> >> + CG2900_ERR("dev == NULL");
> >> + goto fin_free_skb;
> >> + }
> >> +
> >> + dev_info = (struct dev_info *)dev->user_data;
> >> +
> >> + if (!dev_info) {
> >> + CG2900_ERR("dev_info == NULL");
> >> + goto fin_free_skb;
> >> + }
> >> +
> >> + evt = (struct hci_event_hdr *)skb->data;
> >> + cmd_complete = (struct hci_ev_cmd_complete *)(skb->data + sizeof(*evt));
> >> + cmd_status = (struct hci_ev_cmd_status *)(skb->data + sizeof(*evt));
> >> +
> >> + /*
> >> + * Check if HCI Driver it self is expecting a Command Complete packet
> >> + * from the chip after a BT Enable command.
> >> + */
> >> + if ((hci_info->enable_state == ENABLE_WAITING_BT_ENABLED_CC ||
> >> + hci_info->enable_state == ENABLE_WAITING_BT_DISABLED_CC) &&
> >> + hci_info->bt_evt->h4_channel == dev->h4_channel &&
> >> + evt->evt == HCI_EV_CMD_COMPLETE &&
> >> + le16_to_cpu(cmd_complete->opcode) == bt_enable_cmd) {
> >> + /*
> >> + * This is the command complete event for
> >> + * the HCI_Cmd_VS_Bluetooth_Enable.
> >> + * Check result and update state.
> >> + *
> >> + * The BT chip is enabled/disabled. Either it was enabled/
> >> + * disabled now (status NO_ERROR) or it was already enabled/
> >> + * disabled (assuming status CMD_DISALLOWED is already enabled/
> >> + * disabled).
> >> + */
> >> + status = *(skb->data + sizeof(*evt) + sizeof(*cmd_complete));
> >> + if (status != HCI_ERR_NO_ERROR &&
> >> + status != HCI_ERR_CMD_DISALLOWED) {
> >> + CG2900_ERR("Could not enable/disable BT core (0x%X)",
> >> + status);
> >> + SET_ENABLE_STATE(ENABLE_BT_ERROR);
> >> + goto fin_free_skb;
> >> + }
> >> +
> >> + if (hci_info->enable_state == ENABLE_WAITING_BT_ENABLED_CC) {
> >> + SET_ENABLE_STATE(ENABLE_BT_ENABLED);
> >> + CG2900_INFO("BT core is enabled");
> >> + } else {
> >> + SET_ENABLE_STATE(ENABLE_BT_DISABLED);
> >> + CG2900_INFO("BT core is disabled");
> >> + }
> >> +
> >> + /* Wake up whom ever is waiting for this result. */
> >> + wake_up_interruptible(&hci_wait_queue);
> >> + goto fin_free_skb;
> >> + } else if ((hci_info->enable_state == ENABLE_WAITING_BT_DISABLED_CC ||
> >> + hci_info->enable_state == ENABLE_WAITING_BT_ENABLED_CC) &&
> >> + hci_info->bt_evt->h4_channel == dev->h4_channel &&
> >> + evt->evt == HCI_EV_CMD_STATUS &&
> >> + le16_to_cpu(cmd_status->opcode) == bt_enable_cmd) {
> >> + /*
> >> + * Clear the status events since the Bluez is not expecting
> >> + * them.
> >> + */
> >> + CG2900_DBG("HCI Driver received Command Status(for "
> >> + "BT enable): 0x%X", cmd_status->status);
> >> + /*
> >> + * This is the command status event for
> >> + * the HCI_Cmd_VS_Bluetooth_Enable.
> >> + * Just free the packet.
> >> + */
> >> + goto fin_free_skb;
> >> + } else {
> >> + bt_cb(skb)->pkt_type = dev_info->hci_data_type;
> >> + skb->dev = (struct net_device *)dev_info->hdev;
> >> + /* Update BlueZ stats */
> >> + dev_info->hdev->stat.byte_rx += skb->len;
> >> + if (bt_cb(skb)->pkt_type == HCI_ACLDATA_PKT)
> >> + dev_info->hdev->stat.acl_rx++;
> >> + else
> >> + dev_info->hdev->stat.evt_rx++;
> >> +
> >> + CG2900_DBG_DATA("Data receive %d bytes", skb->len);
> >> +
> >> + /* Provide BlueZ with received frame*/
> >> + err = hci_recv_frame(skb);
> >> + /* If err, skb have been freed in hci_recv_frame() */
> >> + if (err)
> >> + CG2900_ERR("Failed in supplying packet to BlueZ (%d)",
> >> + err);
> >> + }
> >> +
> >> + return;
> >> +
> >> +fin_free_skb:
> >> + kfree_skb(skb);
> >> +}
> >> +
> >> +/**
> >> + * hci_reset_cb() - Callback for handling reset from CG2900 driver.
> >> + * @dev: CPD device resetting.
> >> + */
> >> +static void hci_reset_cb(struct cg2900_device *dev)
> >> +{
> >> + int err;
> >> + struct hci_dev *hdev;
> >> + struct dev_info *dev_info;
> >> + struct hci_info *info;
> >> +
> >> + CG2900_INFO("BluezDriver: hci_reset_cb");
> >> +
> >> + if (!dev) {
> >> + CG2900_ERR("NULL supplied for dev");
> >> + return;
> >> + }
> >> +
> >> + dev_info = (struct dev_info *)dev->user_data;
> >> + if (!dev_info) {
> >> + CG2900_ERR("NULL supplied for dev_info");
> >> + return;
> >> + }
> >> +
> >> + hdev = dev_info->hdev;
> >> + if (!hdev) {
> >> + CG2900_ERR("NULL supplied for hdev");
> >> + return;
> >> + }
> >> +
> >> + info = (struct hci_info *)hdev->driver_data;
> >> + if (!info) {
> >> + CG2900_ERR("NULL supplied for driver_data");
> >> + return;
> >> + }
> >> +
> >> + switch (dev_info->hci_data_type) {
> >> +
> >> + case HCI_EVENT_PKT:
> >> + info->bt_evt = NULL;
> >> + break;
> >> +
> >> + case HCI_COMMAND_PKT:
> >> + info->bt_cmd = NULL;
> >> + break;
> >> +
> >> + case HCI_ACLDATA_PKT:
> >> + info->bt_acl = NULL;
> >> + break;
> >> +
> >> + default:
> >> + CG2900_ERR("Unknown HCI data type:%d",
> >> + dev_info->hci_data_type);
> >> + return;
> >> + }
> >> +
> >> + SET_RESET_STATE(RESET_ACTIVATED);
> >> +
> >> + /* Free userdata as device info structure will be freed by CG2900
> >> + * when this callback returns */
> >> + kfree(dev->user_data);
> >> + dev->user_data = NULL;
> >> +
> >> + /*
> >> + * Continue to deregister hdev if all channels has been reset else
> >> + * return.
> >> + */
> >> + if (info->bt_evt || info->bt_cmd || info->bt_acl)
> >> + return;
> >> +
> >> + /*
> >> + * Deregister HCI device. Close and Destruct functions should
> >> + * in turn be called by BlueZ.
> >> + */
> >> + CG2900_INFO("Deregister HCI device");
> >> + err = hci_unregister_dev(hdev);
> >> + if (err)
> >> + CG2900_ERR("Can not deregister HCI device! (%d)", err);
> >> + /*
> >> + * Now we are in trouble. Try to register a new hdev
> >> + * anyway even though this will cost some memory.
> >> + */
> >> +
> >> + wait_event_interruptible_timeout(hci_wait_queue,
> >> + (RESET_UNREGISTERED == hci_info->reset_state),
> >> + msecs_to_jiffies(RESP_TIMEOUT));
> >> + if (RESET_UNREGISTERED != hci_info->reset_state)
> >> + /*
> >> + * Now we are in trouble. Try to register a new hdev
> >> + * anyway even though this will cost some memory.
> >> + */
> >> + CG2900_ERR("Timeout expired. Could not deregister HCI device");
> >> +
> >> + /* Init and register hdev */
> >> + CG2900_INFO("Register HCI device");
> >> + err = register_to_bluez();
> >> + if (err)
> >> + CG2900_ERR("HCI Device registration error (%d).", err);
> >> +}
> >> +
> >> +/*
> >> + * struct cg2900_cb - Specifies callback structure for CG2900 user.
> >> + *
> >> + * @read_cb: Callback function called when data is received from
> >> + * the controller.
> >> + * @reset_cb: Callback function called when the controller has been reset.
> >> + */
> >> +static struct cg2900_callbacks cg2900_cb = {
> >> + .read_cb = hci_read_cb,
> >> + .reset_cb = hci_reset_cb
> >> +};
> >> +
> >> +/**
> >> + * open_from_hci() - Open HCI interface.
> >> + * @hdev: HCI device being opened.
> >> + *
> >> + * BlueZ callback function for opening HCI interface to device.
> >> + *
> >> + * Returns:
> >> + * 0 if there is no error.
> >> + * -EINVAL if NULL pointer is supplied.
> >> + * -EOPNOTSUPP if supplied packet type is not supported.
> >> + * -EBUSY if device is already opened.
> >> + * -EACCES if opening of channels failed.
> >> + */
> >> +static int open_from_hci(struct hci_dev *hdev)
> >> +{
> >> + struct hci_info *info;
> >> + struct dev_info *dev_info;
> >> + struct sk_buff *enable_cmd;
> >> + int err;
> >> +
> >> + CG2900_INFO("Open ST-Ericsson connectivity HCI driver");
> >> +
> >> + if (!hdev) {
> >> + CG2900_ERR("NULL supplied for hdev");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + info = (struct hci_info *)hdev->driver_data;
> >> + if (!info) {
> >> + CG2900_ERR("NULL supplied for driver_data");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (test_and_set_bit(HCI_RUNNING, &(hdev->flags))) {
> >> + CG2900_ERR("Device already opened!");
> >> + return -EBUSY;
> >> + }
> >> +
> >> + if (!(info->bt_cmd)) {
> >> + info->bt_cmd = cg2900_register_user(CG2900_BT_CMD,
> >> + &cg2900_cb);
> >> + if (info->bt_cmd) {
> >> + dev_info = kmalloc(sizeof(*dev_info), GFP_KERNEL);
> >> + if (dev_info) {
> >> + dev_info->hdev = hdev;
> >> + dev_info->hci_data_type = HCI_COMMAND_PKT;
> >> + }
> >> + info->bt_cmd->user_data = dev_info;
> >> + } else {
> >> + CG2900_ERR("Couldn't register CG2900_BT_CMD to CG2900");
> >> + err = -EACCES;
> >> + goto handle_error;
> >> + }
> >> + }
> >> +
> >> + if (!(info->bt_evt)) {
> >> + info->bt_evt = cg2900_register_user(CG2900_BT_EVT,
> >> + &cg2900_cb);
> >> + if (info->bt_evt) {
> >> + dev_info = kmalloc(sizeof(*dev_info), GFP_KERNEL);
> >> + if (dev_info) {
> >> + dev_info->hdev = hdev;
> >> + dev_info->hci_data_type = HCI_EVENT_PKT;
> >> + }
> >> + info->bt_evt->user_data = dev_info;
> >> + } else {
> >> + CG2900_ERR("Couldn't register CG2900_BT_EVT to CG2900");
> >> + err = -EACCES;
> >> + goto handle_error;
> >> + }
> >> + }
> >> +
> >> + if (!(info->bt_acl)) {
> >> + info->bt_acl = cg2900_register_user(CG2900_BT_ACL,
> >> + &cg2900_cb);
> >> + if (info->bt_acl) {
> >> + dev_info = kmalloc(sizeof(*dev_info), GFP_KERNEL);
> >> + if (dev_info) {
> >> + dev_info->hdev = hdev;
> >> + dev_info->hci_data_type = HCI_ACLDATA_PKT;
> >> + }
> >> + info->bt_acl->user_data = dev_info;
> >> + } else {
> >> + CG2900_ERR("Couldn't register CG2900_BT_ACL to CG2900");
> >> + err = -EACCES;
> >> + goto handle_error;
> >> + }
> >> + }
> >> +
> >> + if (info->reset_state == RESET_ACTIVATED)
> >> + SET_RESET_STATE(RESET_IDLE);
> >> +
> >> + /*
> >> + * Call platform specific function that returns the chip dependent
> >> + * bt enable(true) HCI command.
> >> + * If NULL is returned, then no bt_enable command should be sent to the
> >> + * chip.
> >> + */
> >> + enable_cmd = cg2900_devices_get_bt_enable_cmd(&bt_enable_cmd, true);
> >> + if (!enable_cmd) {
> >> + /* The chip is enabled by default */
> >> + SET_ENABLE_STATE(ENABLE_BT_ENABLED);
> >> + return 0;
> >> + }
> >> +
> >> + /* Set the HCI state before sending command to chip. */
> >> + SET_ENABLE_STATE(ENABLE_WAITING_BT_ENABLED_CC);
> >> +
> >> + /* Send command to chip */
> >> + cg2900_write(info->bt_cmd, enable_cmd);
> >> +
> >> + /*
> >> + * Wait for callback to receive command complete and then wake us up
> >> + * again.
> >> + */
> >> + wait_event_interruptible_timeout(hci_wait_queue,
> >> + (info->enable_state == ENABLE_BT_ENABLED),
> >> + msecs_to_jiffies(RESP_TIMEOUT));
> >> + /* Check the current state to se that it worked. */
> >> + if (info->enable_state != ENABLE_BT_ENABLED) {
> >> + CG2900_ERR("Could not enable BT core (%d)", info->enable_state);
> >> + err = -EACCES;
> >> + SET_ENABLE_STATE(ENABLE_BT_DISABLED);
> >> + goto handle_error;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +handle_error:
> >> + remove_bt_users(info);
> >> + clear_bit(HCI_RUNNING, &(hdev->flags));
> >> + return err;
> >> +
> >> +}
> >> +
> >> +/**
> >> + * flush_from_hci() - Flush HCI interface.
> >> + * @hdev: HCI device being flushed.
> >> + *
> >> + * Returns:
> >> + * 0 if there is no error.
> >> + * -EINVAL if NULL pointer is supplied.
> >> + */
> >> +static int flush_from_hci(struct hci_dev *hdev)
> >> +{
> >> + CG2900_INFO("flush_from_hci");
> >> +
> >> + if (!hdev) {
> >> + CG2900_ERR("NULL supplied for hdev");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * close_from_hci() - Close HCI interface.
> >> + * @hdev: HCI device being closed.
> >> + *
> >> + * BlueZ callback function for closing HCI interface.
> >> + * It flushes the interface first.
> >> + *
> >> + * Returns:
> >> + * 0 if there is no error.
> >> + * -EINVAL if NULL pointer is supplied.
> >> + * -EOPNOTSUPP if supplied packet type is not supported.
> >> + * -EBUSY if device is not opened.
> >> + */
> >> +static int close_from_hci(struct hci_dev *hdev)
> >> +{
> >> + struct hci_info *info = NULL;
> >> + struct sk_buff *enable_cmd;
> >> +
> >> + CG2900_INFO("close_from_hci");
> >> +
> >> + if (!hdev) {
> >> + CG2900_ERR("NULL supplied for hdev");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + info = (struct hci_info *)hdev->driver_data;
> >> + if (!info) {
> >> + CG2900_ERR("NULL supplied for driver_data");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (!test_and_clear_bit(HCI_RUNNING, &(hdev->flags))) {
> >> + CG2900_ERR("Device already closed!");
> >> + return -EBUSY;
> >> + }
> >> +
> >> + flush_from_hci(hdev);
> >> +
> >> + /* Do not do this if there is an reset ongoing */
> >> + if (hci_info->reset_state == RESET_ACTIVATED)
> >> + goto remove_users;
> >> +
> >> + /*
> >> + * Get the chip dependent BT Enable HCI command. The command parameter
> >> + * shall be set to false to disable the BT core.
> >> + * If NULL is returned, then no BT Enable command should be sent to the
> >> + * chip.
> >> + */
> >> + enable_cmd = cg2900_devices_get_bt_enable_cmd(&bt_enable_cmd, false);
> >> + if (!enable_cmd) {
> >> + /*
> >> + * The chip is enabled by default and we should not disable it.
> >> + */
> >> + SET_ENABLE_STATE(ENABLE_BT_ENABLED);
> >> + goto remove_users;
> >> + }
> >> +
> >> + /* Set the HCI state before sending command to chip */
> >> + SET_ENABLE_STATE(ENABLE_WAITING_BT_DISABLED_CC);
> >> +
> >> + /* Send command to chip */
> >> + cg2900_write(info->bt_cmd, enable_cmd);
> >> +
> >> + /*
> >> + * Wait for callback to receive command complete and then wake us up
> >> + * again.
> >> + */
> >> + wait_event_interruptible_timeout(hci_wait_queue,
> >> + (info->enable_state == ENABLE_BT_DISABLED),
> >> + msecs_to_jiffies(RESP_TIMEOUT));
> >> + /* Check the current state to se that it worked. */
> >> + if (info->enable_state != ENABLE_BT_DISABLED) {
> >> + CG2900_ERR("Could not disable BT core.");
> >> + SET_ENABLE_STATE(ENABLE_BT_ENABLED);
> >> + }
> >> +
> >> +remove_users:
> >> + /* Finally deregister all users and free allocated data */
> >> + remove_bt_users(info);
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * send_from_hci() - Send packet to device.
> >> + * @skb: sk buffer to be sent.
> >> + *
> >> + * BlueZ callback function for sending sk buffer.
> >> + *
> >> + * Returns:
> >> + * 0 if there is no error.
> >> + * -EINVAL if NULL pointer is supplied.
> >> + * -EOPNOTSUPP if supplied packet type is not supported.
> >> + * Error codes from cg2900_write.
> >> + */
> >> +static int send_from_hci(struct sk_buff *skb)
> >> +{
> >> + struct hci_dev *hdev;
> >> + struct hci_info *info;
> >> + int err = 0;
> >> +
> >> + if (!skb) {
> >> + CG2900_ERR("NULL supplied for skb");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + hdev = (struct hci_dev *)(skb->dev);
> >> + if (!hdev) {
> >> + CG2900_ERR("NULL supplied for hdev");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + info = (struct hci_info *)hdev->driver_data;
> >> + if (!info) {
> >> + CG2900_ERR("NULL supplied for info");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Update BlueZ stats */
> >> + hdev->stat.byte_tx += skb->len;
> >> +
> >> + CG2900_DBG_DATA("Data transmit %d bytes", skb->len);
> >> +
> >> + switch (bt_cb(skb)->pkt_type) {
> >> + case HCI_COMMAND_PKT:
> >> + CG2900_DBG("Sending HCI_COMMAND_PKT");
> >> + err = cg2900_write(info->bt_cmd, skb);
> >> + hdev->stat.cmd_tx++;
> >> + break;
> >> + case HCI_ACLDATA_PKT:
> >> + CG2900_DBG("Sending HCI_ACLDATA_PKT");
> >> + err = cg2900_write(info->bt_acl, skb);
> >> + hdev->stat.acl_tx++;
> >> + break;
> >> + default:
> >> + CG2900_ERR("Trying to transmit unsupported packet type "
> >> + "(0x%.2X)", bt_cb(skb)->pkt_type);
> >> + err = -EOPNOTSUPP;
> >> + break;
> >> + };
> >> +
> >> + return err;
> >> +}
> >> +
> >> +/**
> >> + * destruct_from_hci() - Destruct HCI interface.
> >> + * @hdev: HCI device being destructed.
> >> + */
> >> +static void destruct_from_hci(struct hci_dev *hdev)
> >> +{
> >> + CG2900_INFO("destruct_from_hci");
> >> +
> >> + if (!hci_info)
> >> + return;
> >> +
> >> + /* When being reset, register a new hdev when hdev is deregistered */
> >> + if (hci_info->reset_state == RESET_ACTIVATED) {
> >> + if (hci_info->hdev)
> >> + hci_free_dev(hci_info->hdev);
> >> + SET_RESET_STATE(RESET_UNREGISTERED);
> >> + }
> >> +}
> >
> > Please name thse cg2900_destruct or btcg2900_destruct. And not from_hci.
> > Follow how the other Bluetooth drivers do the naming.
> >
> > And the destruct callback is for freeing memory. I am also not sure how
> > reset and unregister/re-register HCI is a good idea.
> >
>
> Regarding naming: no problem, we'll change that. We will see how we
> solve the reset. It's a bit tricky to get it working correctly, but
> we'll see what we can do. We have to be able in some way to handle if
> another stack, such as GPS or FM, suddenly reset the chip.
It sounds wrong to me, but please enlighten me.
> >> +/**
> >> + * notify_from_hci() - Notification from the HCI interface.
> >> + * @hdev: HCI device notifying.
> >> + * @evt: Notification event.
> >> + */
> >> +static void notify_from_hci(struct hci_dev *hdev, unsigned int evt)
> >> +{
> >> + CG2900_INFO("notify_from_hci - evt = 0x%.2X", evt);
> >> +}
> >
> > If you don't use it, then just leave it out.
> >
>
> OK
>
> >> +/**
> >> + * ioctl_from_hci() - Handle IOCTL call to the HCI interface.
> >> + * @hdev: HCI device.
> >> + * @cmd: IOCTL command.
> >> + * @arg: IOCTL argument.
> >> + *
> >> + * Returns:
> >> + * -EINVAL if NULL is supplied for hdev.
> >> + * -EPERM otherwise since current driver supports no IOCTL.
> >> + */
> >> +static int ioctl_from_hci(struct hci_dev *hdev, unsigned int cmd,
> >> + unsigned long arg)
> >> +{
> >> + CG2900_INFO("ioctl_from_hci - cmd 0x%X", cmd);
> >> + CG2900_DBG("DIR: %d, TYPE: %d, NR: %d, SIZE: %d",
> >> + _IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd),
> >> + _IOC_SIZE(cmd));
> >> +
> >> + if (!hdev) {
> >> + CG2900_ERR("NULL supplied for hdev");
> >> + return -EINVAL;;
> >> + }
> >> +
> >> + return -EPERM;
> >> +}
> >
> > Since you are not using anything with the ioctl, just leave it out. The
> > Bluetooth subsystem will do the right thing.
> >
>
> OK
>
> >> +/**
> >> + * register_to_bluez() - Initialize module.
> >> + *
> >> + * Alloc, init, and register HCI device to BlueZ.
> >> + *
> >> + * Returns:
> >> + * 0 if there is no error.
> >> + * -ENOMEM if allocation fails.
> >> + * Error codes from hci_register_dev.
> >> + */
> >> +static int register_to_bluez(void)
> >> +{
> >> + int err;
> >> +
> >> + hci_info->hdev = hci_alloc_dev();
> >> + if (!hci_info->hdev) {
> >> + CG2900_ERR("Could not allocate mem for HCI driver");
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + hci_info->hdev->bus = HCI_CG2900;
> >> + hci_info->hdev->driver_data = hci_info;
> >> + hci_info->hdev->owner = THIS_MODULE;
> >> + hci_info->hdev->open = open_from_hci;
> >> + hci_info->hdev->close = close_from_hci;
> >> + hci_info->hdev->flush = flush_from_hci;
> >> + hci_info->hdev->send = send_from_hci;
> >> + hci_info->hdev->destruct = destruct_from_hci;
> >> + hci_info->hdev->notify = notify_from_hci;
> >> + hci_info->hdev->ioctl = ioctl_from_hci;
> >> +
> >> + err = hci_register_dev(hci_info->hdev);
> >> + if (err) {
> >> + CG2900_ERR("Can not register HCI device (%d)", err);
> >> + hci_free_dev(hci_info->hdev);
> >> + }
> >> +
> >> + SET_ENABLE_STATE(ENABLE_IDLE);
> >> + SET_RESET_STATE(RESET_IDLE);
> >> +
> >> + return err;
> >> +}
> >
> > So whenever the module is loaded it registers the HCI device. That is
> > not really useful here. This driver needs to be able to be compiled into
> > the kernel (or as module) and run on hardware that doesn't have this
> > chip built in.
> >
>
> OK. We will think this through and solve it in a better way.
The TI guys seem to favoring a platform device. So either you have to do
that or create your own bus.
Regards
Marcel
^ permalink raw reply
* Re: pull request: bluetooth-next-2.6 2010-10-13
From: Gustavo F. Padovan @ 2010-10-13 22:01 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: linville, marcel, linux-wireless, linux-bluetooth, linux-kernel
In-Reply-To: <AANLkTinH38ut+Uj1d+MJXjUQygdpUOVw=7rMKTk-Y-8d@mail.gmail.com>
Hi Luis,
* Luis R. Rodriguez <mcgrof@gmail.com> [2010-10-13 14:41:24 -0700]:
> On Tue, Oct 12, 2010 at 8:48 PM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > One notable change is in the MAINTAINERS file, From now and on I'm going
> > to maintain the Bluetooth trees as well as send the pull requests to you.
>
> I noted here you mentioned "as well", does this mean there may be
> updates from two people now instead of just one?
No, updates are coming only from me. You misunderstood it due to my poor
English. I'm going to maitain the trees and send the pull requests.
That's what I said. Sorry, for the bad English. ;)
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* [PATCH -next] bluetooth: fix hidp kconfig dependency warning
From: Randy Dunlap @ 2010-10-14 1:16 UTC (permalink / raw)
To: lkml; +Cc: akpm, Marcel Holtmann, linux-bluetooth
From: Randy Dunlap <randy.dunlap@oracle.com>
Fix kconfig dependency warning to satisfy dependencies:
warning: (BT_HIDP && NET && BT && BT_L2CAP && INPUT || USB_HID && HID_SUPPORT && USB && INPUT) selects HID which has unmet direct dependencies (HID_SUPPORT && INPUT)
Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
net/bluetooth/hidp/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-next-20101013.orig/net/bluetooth/hidp/Kconfig
+++ linux-next-20101013/net/bluetooth/hidp/Kconfig
@@ -1,6 +1,6 @@
config BT_HIDP
tristate "HIDP protocol support"
- depends on BT && BT_L2CAP && INPUT
+ depends on BT && BT_L2CAP && INPUT && HID_SUPPORT
select HID
help
HIDP (Human Interface Device Protocol) is a transport layer
^ permalink raw reply
* Re: [PATCH -next] bluetooth: fix hidp kconfig dependency warning
From: Marcel Holtmann @ 2010-10-14 4:22 UTC (permalink / raw)
To: Randy Dunlap; +Cc: lkml, akpm, linux-bluetooth
In-Reply-To: <20101013181652.7ddf481e.randy.dunlap@oracle.com>
Hi Randy,
> Fix kconfig dependency warning to satisfy dependencies:
>
> warning: (BT_HIDP && NET && BT && BT_L2CAP && INPUT || USB_HID && HID_SUPPORT && USB && INPUT) selects HID which has unmet direct dependencies (HID_SUPPORT && INPUT)
>
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
looks fine to me.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: Firmware versioning best practices: ath3k-2.fw rename or replace ath3k-1.fw ?
From: Suraj Sumangala @ 2010-10-14 4:23 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Kevin Hayes, Luis Rodriguez, Marcel Holtmann, Henry Ptasinski,
Suraj Sumangala, David Woodhouse, linux-bluetooth,
linux-kernel@vger.kernel.org, linux-wireless
In-Reply-To: <AANLkTik9oYwspVG4KFbMMq63SpBp++iZe=vxOSQhxRki@mail.gmail.com>
Hi Luis,
On 10/13/2010 11:39 PM, Luis R. Rodriguez wrote:
> Does HCI support uploading firmware? Can't we resolve this blacklist
> crap issue if devices just used HCI to upload firmware?
HCI does not support uploading firmware. But HCI does provide options
for vendor specific commands that can be used for uploading firmware as
long as your device has enough intelligence to understand the command
when it comes.
This is what we do for AR300x serial devices. We do not download
firmware, but we do download all configuration entries as VS HCI commands.
>
> Luis
Regards
Suraj
^ permalink raw reply
* Query regarding Virtual simulators
From: steven bluez @ 2010-10-14 5:27 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo F. Padovan", Suraj Sumangala; +Cc: linux-bluetooth
HI MARCEL,GUSTAVO,Suraj
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0If I am developing a profile thats not y=
et been
implemented in bluez and if i dont have a Hardware that supports the
profile.How can i validate it to fix a situation that this profile
really works.Is there any virutal dummy simulators?
Do help me out by giving certain ideas on how to perform the same? .
Thanks,
Steven
^ permalink raw reply
* Re: Query regarding Virtual simulators
From: Suraj Sumangala @ 2010-10-14 6:03 UTC (permalink / raw)
To: steven bluez
Cc: Marcel Holtmann, gustavo, Suraj Sumangala,
linux-bluetooth@vger.kernel.org
In-Reply-To: <AANLkTincuwLZnBhjj=vrmghDEcmZhL=dy1VP=SH6vK_Y@mail.gmail.com>
Hi Steven,
On 10/14/2010 10:57 AM, steven bluez wrote:
> HI MARCEL,GUSTAVO,Suraj
>
> If I am developing a profile thats not yet been
> implemented in bluez and if i dont have a Hardware that supports the
> profile.How can i validate it to fix a situation that this profile
> really works.Is there any virutal dummy simulators?
> Do help me out by giving certain ideas on how to perform the same? .
>
> Thanks,
> Steven
I am not sure about any virtual dummy simulator.
But, you should be able to write a dummy agent and test the profile
against PTS if the profile is supported by it.
PTS would be able to give you a fair idea of what is missing.
It would be great if you can send a mail to the community regarding the
profile and how you intent to implement it. There would be people with
access to the hardware and they would be able to guide you accordingly.
Regards
Suraj
^ permalink raw reply
* Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
From: Jose Antonio Santos Cadenas @ 2010-10-14 10:57 UTC (permalink / raw)
To: linux-bluetooth
The following changes since commit 6b9deca15d0824ad5b3c38f8c0dd3a3c482572ea:
TODO: Device Name Characteristic for Low Energy (2010-10-13 22:26:59 +0300)
are available in the git repository at:
git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
José Antonio Santos Cadenas (7):
Connect echo channel
Initial steps to send echo data
Generate random echo packet
Create a function for closing mcl correctly
Notify channel deletion correctly
Separate private echo data from data channels
Add initial support for deleting an echo channel
Santiago Carot-Nemesio (8):
Add support to initiate echo
Remove unnecessary variable
Set watcher to check echo response
Check echo response
Add timeout to wait for echo reply
Abort echo channel if connection was not successful
Remove MDL when delete operation fails with INVALID_MDLID error
Remove unnecessary function
health/hdp.c | 431 +++++++++++++++++++++++++++++++++++++++++-----------
health/hdp_types.h | 5 +-
health/mcap.c | 14 ++-
3 files changed, 361 insertions(+), 89 deletions(-)
^ permalink raw reply
* Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
From: Johan Hedberg @ 2010-10-14 11:15 UTC (permalink / raw)
To: Jose Antonio Santos Cadenas; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimBnzJzW_YLjW+NJq-MJ_aEMtSFNy_eKS5v70N4@mail.gmail.com>
Hi Jose,
On Thu, Oct 14, 2010, Jose Antonio Santos Cadenas wrote:
> The following changes since commit 6b9deca15d0824ad5b3c38f8c0dd3a3c482572ea:
>
> TODO: Device Name Characteristic for Low Energy (2010-10-13 22:26:59 +0300)
>
> are available in the git repository at:
> git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
>
> José Antonio Santos Cadenas (7):
> Connect echo channel
> Initial steps to send echo data
> Generate random echo packet
> Create a function for closing mcl correctly
> Notify channel deletion correctly
> Separate private echo data from data channels
> Add initial support for deleting an echo channel
>
> Santiago Carot-Nemesio (8):
> Add support to initiate echo
> Remove unnecessary variable
> Set watcher to check echo response
> Check echo response
> Add timeout to wait for echo reply
> Abort echo channel if connection was not successful
> Remove MDL when delete operation fails with INVALID_MDLID error
> Remove unnecessary function
>
> health/hdp.c | 431 +++++++++++++++++++++++++++++++++++++++++-----------
> health/hdp_types.h | 5 +-
> health/mcap.c | 14 ++-
> 3 files changed, 361 insertions(+), 89 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH -next] bluetooth: fix hidp kconfig dependency warning
From: Gustavo F. Padovan @ 2010-10-14 12:18 UTC (permalink / raw)
To: Randy Dunlap; +Cc: lkml, akpm, Marcel Holtmann, linux-bluetooth
In-Reply-To: <20101013181652.7ddf481e.randy.dunlap@oracle.com>
Hi Randy,
* Randy Dunlap <randy.dunlap@oracle.com> [2010-10-13 18:16:52 -0700]:
> From: Randy Dunlap <randy.dunlap@oracle.com>
>
> Fix kconfig dependency warning to satisfy dependencies:
>
> warning: (BT_HIDP && NET && BT && BT_L2CAP && INPUT || USB_HID && HID_SUPPORT && USB && INPUT) selects HID which has unmet direct dependencies (HID_SUPPORT && INPUT)
>
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> ---
> net/bluetooth/hidp/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* [PATCH 0/2] Fix regression on suspend on opening
From: Dmitriy Paliy @ 2010-10-14 12:35 UTC (permalink / raw)
To: linux-bluetooth
Hi,
This fixes regression when doing suspend on opening obex stream. If
obex_write_stream returns length of buffer, it is treated as error
reponse, which is not correct.
A typo is fixed in comments as well.
Br,
Dmitriy
^ permalink raw reply
* [PATCH 1/2] Fix regression on suspend on opening
From: Dmitriy Paliy @ 2010-10-14 12:35 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
In-Reply-To: <1287059730-10988-1-git-send-email-dmitriy.paliy@nokia.com>
This fixes regression on suspend on opening when obex_write_stream
returns length of buffer, which is treated as error response
afterwards.
---
src/obex.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/obex.c b/src/obex.c
index d1ac339..4e1db9c 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -683,7 +683,7 @@ add_header:
os->offset += len;
- return len;
+ return 0;
}
static gboolean handle_async_io(void *object, int flags, int err,
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/2] Code cleanup: immidiately to immediately in comments
From: Dmitriy Paliy @ 2010-10-14 12:35 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
In-Reply-To: <1287059730-10988-1-git-send-email-dmitriy.paliy@nokia.com>
---
src/obex.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/obex.c b/src/obex.c
index 4e1db9c..5f6ad47 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -818,7 +818,7 @@ static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
OBEX_ObjectAddHeader (obex, obj, OBEX_HDR_BODY,
hd, 0, OBEX_FL_STREAM_START);
- /* Try to write to stream and suspend the stream immidiately
+ /* Try to write to stream and suspend the stream immediately
* if no data available to send. */
err = obex_write_stream(os, obex, obj);
if (err == -EAGAIN) {
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH] Bluetooth: TI_ST bluetooth driver
From: Gustavo F. Padovan @ 2010-10-14 13:31 UTC (permalink / raw)
To: pavan_savoy; +Cc: linux-bluetooth, marcel, greg, linux-kernel
In-Reply-To: <1286814803-6740-1-git-send-email-pavan_savoy@ti.com>
Hi Pavan,
* pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-10-11 12:33:23 -0400]:
> From: Pavan Savoy <pavan_savoy@ti.com>
>
> Gustavo, Marcel,
>
> I have taken care of most of the comments which you provided.
> Couple of them like usage of goto's, I didn't handle since
> I am not doing much error handling repeatedly there.
>
> I have also removed the 2 states like _REGISTERED, _RUNNING which
> were not required and the flags associated with it.
> I have taken care of other style comments.
>
> Please review and provide your comments,
>
> -- patch description --
>
> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> Texas Instrument's WiLink chipsets combine wireless technologies
> like BT, FM, GPS and WLAN onto a single chip.
>
> This Bluetooth driver works on top of the TI_ST shared transport
> line discipline driver which also allows other drivers like
> FM V4L2 and GPS character driver to make use of the same UART interface.
>
> Kconfig and Makefile modifications to enable the Bluetooth
> driver for Texas Instrument's WiLink 7 chipset.
>
> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> ---
> drivers/bluetooth/Kconfig | 10 +
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/btwilink.c | 455 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 466 insertions(+), 0 deletions(-)
> create mode 100644 drivers/bluetooth/btwilink.c
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 02deef4..e0d67dd 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -219,4 +219,14 @@ config BT_ATH3K
> Say Y here to compile support for "Atheros firmware download driver"
> into the kernel or say M to compile it as module (ath3k).
>
> +config BT_WILINK
> + tristate "BlueZ bluetooth driver for TI ST"
> + depends on TI_ST
> + help
> + This enables the Bluetooth driver for Texas Instrument's BT/FM/GPS
> + combo devices. This makes use of shared transport line discipline
> + core driver to communicate with the BT core of the combo chip.
> +
> + Say Y here to compile support for Texas Instrument's WiLink7 driver
> + into the kernel or say M to compile it as module.
> endmenu
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 71bdf13..5351b62 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -28,3 +28,4 @@ hci_uart-$(CONFIG_BT_HCIUART_BCSP) += hci_bcsp.o
> hci_uart-$(CONFIG_BT_HCIUART_LL) += hci_ll.o
> hci_uart-$(CONFIG_BT_HCIUART_ATH3K) += hci_ath.o
> hci_uart-objs := $(hci_uart-y)
> +obj-$(CONFIG_BT_WILINK) := btwilink.o
sort this after:
obj-$(CONFIG_BT_MRVL_SDIO) += btmrvl_sdio.o
> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
> new file mode 100644
> index 0000000..e0bde79
> --- /dev/null
> +++ b/drivers/bluetooth/btwilink.c
> @@ -0,0 +1,455 @@
> +/*
> + * Texas Instrument's Bluetooth Driver For Shared Transport.
> + *
> + * Bluetooth Driver acts as interface between HCI CORE and
> + * TI Shared Transport Layer.
> + *
> + * Copyright (C) 2009-2010 Texas Instruments
> + * Author: Raja Mani <raja_mani@ti.com>
> + * Pavan Savoy <pavan_savoy@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include <linux/ti_wilink_st.h>
> +
> +/* Bluetooth Driver Version */
> +#define VERSION "1.0"
> +
> +/* Defines number of seconds to wait for reg completion
> + * callback getting called from ST (in case,registration
> + * with ST returns PENDING status)
> + */
> +#define BT_REGISTER_TIMEOUT 6000 /* 6 sec */
> +
> +/**
> + * struct ti_st - BT driver operation structure
> + * @hdev: hci device pointer which binds to bt driver
> + * @flags: used locally,to maintain various BT driver status
> + * @streg_cbdata: to hold ST registration callback status
> + * @st_write: write function pointer of ST driver
> + * @wait_for_btdrv_reg_completion - completion sync between ti_st_open
> + * and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> + struct hci_dev *hdev;
> + char streg_cbdata;
> + long (*st_write) (struct sk_buff *);
> + struct completion wait_for_btdrv_reg_completion;
wait_for_btdrv_reg_completion is too big.
> +};
> +
> +static int reset;
> +
> +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> +static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
> +{
> + struct hci_dev *hdev;
> + hdev = hst->hdev;
> +
> + /* Update HCI stat counters */
> + switch (pkt_type) {
> + case HCI_COMMAND_PKT:
> + hdev->stat.cmd_tx++;
> + break;
> +
> + case HCI_ACLDATA_PKT:
> + hdev->stat.acl_tx++;
> + break;
> +
> + case HCI_SCODATA_PKT:
> + hdev->stat.sco_tx++;
> + break;
> + }
> +}
> +
> +/* ------- Interfaces to Shared Transport ------ */
> +
> +/* Called by ST layer to indicate protocol registration completion
> + * status.ti_st_open() function will wait for signal from this
> + * API when st_register() function returns ST_PENDING.
> + */
> +static void st_registration_completion_cb(void *priv_data, char data)
> +{
> + struct ti_st *lhst = (struct ti_st *)priv_data;
> + /* ti_st_open() function needs value of 'data' to know
> + * the registration status(success/fail),So have a back
> + * up of it.
> + */
> + lhst->streg_cbdata = data;
> +
> + /* Got a feedback from ST for BT driver registration
> + * request.Wackup ti_st_open() function to continue
> + * it's open operation.
> + */
> + complete(&lhst->wait_for_btdrv_reg_completion);
> +}
> +
> +/* Called by Shared Transport layer when receive data is
> + * available */
> +static long st_receive(void *priv_data, struct sk_buff *skb)
> +{
> + int err;
> + struct ti_st *lhst = (struct ti_st *)priv_data;
> +
> + if (skb == NULL) {
> + BT_ERR("Invalid SKB received from ST");
I don't think this error message is worth.
> + return -EFAULT;
> + }
Make this (!skb) or the check below (lhst == NULL). Try to use only one
pattern in your code.
> +
> + if (!lhst) {
> + kfree_skb(skb);
> + BT_ERR("Invalid ti_st memory,freeing SKB");
I don't think this error message is worth.
> + return -EFAULT;
> + }
> +
> + skb->dev = (struct net_device *)lhst->hdev;
Blank line here.
> + /* Forward skb to HCI CORE layer */
> + err = hci_recv_frame(skb);
> + if (err) {
> + kfree_skb(skb);
> + BT_ERR("Unable to push skb to HCI CORE(%d),freeing SKB",
Turn this message into:
"Unable to push skb to HCI core(%d)"
> + err);
> + return err;
> + }
> +
> + lhst->hdev->stat.byte_rx += skb->len;
> + return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> + static struct st_proto_s ti_st_proto;
Usually we declare this outside of the function like this:
static struct usb_driver ath3k_driver = {
.name = "ath3k",
.probe = ath3k_probe,
.disconnect = ath3k_disconnect,
.id_table = ath3k_table,
};
> + unsigned long timeleft;
> + struct ti_st *hst;
> + int err;
> + err = 0;
No need to set err to 0 here.
> +
> + BT_DBG("%s %p", hdev->name, hdev);
Skip a line here.
> + hst = hdev->driver_data;
> +
> + /* Populate BT driver info required by ST */
> + memset(&ti_st_proto, 0, sizeof(ti_st_proto));
> +
> + /* BT driver ID */
> + ti_st_proto.type = ST_BT;
> +
> + /* Receive function which called from ST */
> + ti_st_proto.recv = st_receive;
> +
> + /* Callback to be called when registration is pending */
> + ti_st_proto.reg_complete_cb = st_registration_completion_cb;
> +
> + /* send in the hst to be received at registration complete callback
> + * and during st's receive
> + */
> + ti_st_proto.priv_data = hst;
> +
> + /* Register with ST layer */
No need for this comment, your function name already set that.
> + err = st_register(&ti_st_proto);
> + if (err == -EINPROGRESS) {
> + /* Prepare wait-for-completion handler data structures.
> + * Needed to syncronize this and st_registration_completion_cb()
> + * functions.
> + */
> + init_completion(&hst->wait_for_btdrv_reg_completion);
> +
> + /* Reset ST registration callback status flag , this value
> + * will be updated in ti_st_registration_completion_cb()
> + * function whenever it called from ST driver.
> + */
> + hst->streg_cbdata = -EINPROGRESS;
> +
> + /* ST is busy with other protocol registration(may be busy with
> + * firmware download).So,Wait till the registration callback
> + * (passed as a argument to st_register() function) getting
> + * called from ST.
> + */
> + BT_DBG(" %s waiting for reg completion signal from ST",
> + __func__);
BT_DBG already have __func__.
> +
> + timeleft =
> + wait_for_completion_timeout
this could be:
timeleft = wait_for_completion_timeout
> + (&hst->wait_for_btdrv_reg_completion,
> + msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> + if (!timeleft) {
> + BT_ERR("Timeout(%d sec),didn't get reg"
> + "completion signal from ST",
> + BT_REGISTER_TIMEOUT / 1000);
> + return -ETIMEDOUT;
> + }
> +
> + /* Is ST registration callback called with ERROR value? */
> + if (hst->streg_cbdata != 0) {
> + BT_ERR("ST reg completion CB called with invalid"
> + "status %d", hst->streg_cbdata);
> + return -EAGAIN;
> + }
> + err = 0;
> + } else if (err == -1) {
Use the error macro instead of -1
> + BT_ERR("st_register failed %d", err);
> + return -EAGAIN;
> + }
> +
> + /* Do we have proper ST write function? */
> + if (ti_st_proto.write != NULL) {
> + /* We need this pointer for sending any Bluetooth pkts */
> + hst->st_write = ti_st_proto.write;
Didn't get this. Who sets ti_st_proto.write()?
> + } else {
> + BT_ERR("failed to get ST write func pointer");
> +
> + /* Undo registration with ST */
> + err = st_unregister(ST_BT);
> + if (err < 0)
> + BT_ERR("st_unregister failed %d", err);
> +
> + hst->st_write = NULL;
> + return -EAGAIN;
> + }
> +
> + /* Registration with ST layer is completed successfully,
> + * now chip is ready to accept commands from HCI CORE.
> + * Mark HCI Device flag as RUNNING
> + */
I don't think this comment is worth, your code already says that.
But one can argue here that comments are always good. ;)
> + set_bit(HCI_RUNNING, &hdev->flags);
> + return err;
> +}
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> + int err = 0;
> + struct ti_st *hst;
> +
> + hst = hdev->driver_data;
> +
> + /* Clear HCI device RUNNING flag */
> + if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
> + BT_ERR("HCI not RUNNING?");
That's is not a error, just return 0.
> +
> + /* continue to unregister from transport */
> + err = st_unregister(ST_BT);
> + if (err != 0) {
> + hst->st_write = NULL;
> + BT_ERR("st_unregister failed %d", err);
> + return -EBUSY;
> + }
> +
> + hst->st_write = NULL;
> + return err;
This should be better:
if (err)
BT_ERR("st_unregister failed %d", err);
hst->st_write = NULL;
return err;
> +}
> +
> +/* Called from HCI CORE , Sends frames to Shared Transport */
> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> + struct hci_dev *hdev;
> + struct ti_st *hst;
> + long len;
> +
> + if (skb == NULL) {
> + BT_ERR("Invalid skb received from HCI CORE");
> + return -ENOMEM;
> + }
> +
> + hdev = (struct hci_dev *)skb->dev;
> + if (!hdev) {
> + BT_ERR("SKB received for invalid HCI Device (hdev=NULL)");
> + return -ENODEV;
> + }
> +
> + if (!test_bit(HCI_RUNNING, &hdev->flags)) {
> + BT_ERR("Device is not running");
Remove the last 3 BT_ERR
> + return -EBUSY;
> + }
> +
> + hst = (struct ti_st *)hdev->driver_data;
> +
> + /* Prepend skb with frame type */
> + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> + BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
> + skb->len);
> +
> + /* Insert skb to shared transport layer's transmit queue.
> + * Freeing skb memory is taken care in shared transport layer,
> + * so don't free skb memory here.
> + */
> + if (!hst->st_write) {
> + kfree_skb(skb);
> + BT_ERR(" Can't write to ST, st_write null?");
> + return -EAGAIN;
> + }
> +
> + len = hst->st_write(skb);
> + if (len < 0) {
> + kfree_skb(skb);
> + BT_ERR(" ST write failed (%ld)", len);
> + return -EAGAIN;
> + }
> +
> + /* ST accepted our skb. So, Go ahead and do rest */
> + hdev->stat.byte_tx += len;
> + ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> +
> + return 0;
> +}
> +
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> + if (!hdev) {
> + BT_ERR("Destruct called with invalid HCI Device"
> + "(hdev=NULL)");
> + return;
> + }
> +
> + BT_DBG("%s", hdev->name);
> +
> + /* free ti_st memory */
> + if (hdev->driver_data != NULL)
> + kfree(hdev->driver_data);
> +
> + return;
No return; here
> +}
> +
> +/* Creates new HCI device */
> +static int ti_st_register_dev(struct ti_st *hst)
> +{
> + struct hci_dev *hdev;
> +
> + /* Initialize and register HCI device */
> + hdev = hci_alloc_dev();
> + if (!hdev) {
> + BT_ERR("Can't allocate HCI device");
> + return -ENOMEM;
> + }
> + BT_DBG(" HCI device allocated. hdev= %p", hdev);
> +
> + hst->hdev = hdev;
> + hdev->bus = HCI_UART;
> + hdev->driver_data = hst;
> + hdev->open = ti_st_open;
> + hdev->close = ti_st_close;
> + hdev->flush = NULL;
> + hdev->send = ti_st_send_frame;
> + hdev->destruct = ti_st_destruct;
> + hdev->owner = THIS_MODULE;
> +
> + if (reset)
> + set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
> +
> + if (hci_register_dev(hdev) < 0) {
> + BT_ERR("Can't register HCI device");
> + hci_free_dev(hdev);
> + return -ENODEV;
> + }
> +
> + BT_DBG(" HCI device registered. hdev= %p", hdev);
> + return 0;
> +}
> +
> +
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> + int err;
> + static struct ti_st *hst;
> + err = 0;
> +
> + BT_DBG(" Bluetooth Driver Version %s", VERSION);
> +
> + /* Allocate local resource memory */
I don't think this comment is needed.
> + hst = kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> + if (!hst) {
> + BT_ERR("Can't allocate control structure");
remove the BT_ERR
> + return -ENFILE;
-ENOMEM
> + }
> +
> + /* Expose "hciX" device to user space */
> + err = ti_st_register_dev(hst);
> + if (err) {
> + /* Release local resource memory */
I don't think this comment is needed.
> + kfree(hst);
> +
> + BT_ERR("Unable to expose hci0 device(%d)", err);
Are you sure that this is hci0?
> + return err;
> + }
> +
> + dev_set_drvdata(&pdev->dev, hst);
> + return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> + struct ti_st *hst;
> +
> + hst = dev_get_drvdata(&pdev->dev);
> + /* Deallocate local resource's memory */
> + if (hst) {
> + struct hci_dev *hdev = hst->hdev;
> + if (hdev == NULL) {
> + BT_ERR("Invalid hdev memory");
> + kfree(hst);
> + } else {
> + ti_st_close(hdev);
> + hci_unregister_dev(hdev);
> + /* Free HCI device memory */
> + hci_free_dev(hdev);
> + }
> + }
> + return 0;
> +}
> +
> +static struct platform_driver bt_ti_driver = {
> + .probe = bt_ti_probe,
> + .remove = bt_ti_remove,
> + .driver = {
> + .name = "ti_st_bluetooth",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init bt_drv_init(void)
> +{
> +long ret = 0;
Skip a line and don't set ret to 0.
> + ret = platform_driver_register(&bt_ti_driver);
> + if (ret != 0) {
> + BT_ERR("bt_ti platform drv registration failed");
> + return -1;
Why -1?
> + }
> + return 0;
> +}
> +
> +static void __exit bt_drv_exit(void)
> +{
> + platform_driver_unregister(&bt_ti_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 http://vger.kernel.org/majordomo-info.html
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* [PATCH] Fix not responding Not Found when no entry is found
From: Luiz Augusto von Dentz @ 2010-10-14 13:34 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
Accourding to PBAP spec PSE must reply Not Found when:
"The function was recognized and all the parameters are proper, but the
vCard handle or the phone book object could not be found."
---
plugins/pbap.c | 15 +++++++++++++++
plugins/phonebook-tracker.c | 12 +++++++++---
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/plugins/pbap.c b/plugins/pbap.c
index 948f9df..13742da 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -228,6 +228,9 @@ static void phonebook_size_result(const char *buffer, size_t bufsize,
struct aparam_header *hdr = (struct aparam_header *) aparam;
uint16_t phonebooksize;
+ if (vcards < 0)
+ vcards = 0;
+
DBG("vcards %d", vcards);
phonebooksize = htons(vcards);
@@ -248,6 +251,11 @@ static void query_result(const char *buffer, size_t bufsize, int vcards,
DBG("");
+ if (vcards < 0) {
+ obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
+ return;
+ }
+
if (!pbap->buffer)
pbap->buffer = g_string_new_len(buffer, bufsize);
else
@@ -389,6 +397,13 @@ static void cache_ready_notify(void *user_data)
pbap->buffer = g_string_new_len(aparam, sizeof(aparam));
goto done;
}
+
+ if (pbap->cache.entries == NULL) {
+ pbap->cache.valid = TRUE;
+ obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
+ return;
+ }
+
/*
* Don't free the sorted list content: this list contains
* only the reference for the "real" cache entry.
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 39e4c81..2da825b 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -924,6 +924,11 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
gboolean cdata_present = FALSE;
char *home_addr, *work_addr;
+ if (num_fields < 0) {
+ data->cb(NULL, 0, num_fields, 0, data->user_data);
+ goto fail;
+ }
+
DBG("reply %p", reply);
if (reply == NULL)
@@ -1031,8 +1036,9 @@ done:
g_slist_length(data->contacts), 0,
data->user_data);
- g_slist_free(data->contacts);
g_string_free(vcards, TRUE);
+fail:
+ g_slist_free(data->contacts);
g_free(data);
}
@@ -1042,7 +1048,7 @@ static void add_to_cache(char **reply, int num_fields, void *user_data)
char *formatted;
int i;
- if (reply == NULL)
+ if (reply == NULL || num_fields < 0)
goto done;
/* the first element is the URI, always not empty */
@@ -1075,7 +1081,7 @@ static void add_to_cache(char **reply, int num_fields, void *user_data)
return;
done:
- if (num_fields == 0)
+ if (num_fields <= 0)
cache->ready_cb(cache->user_data);
g_free(cache);
--
1.7.1
^ permalink raw reply related
* [PATCH] Unref pending call in fallback scenario.
From: Lukasz Rymanowski @ 2010-10-14 15:13 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Par-Gunnar.p.HJALMDAHL, Lukasz Rymanowski
Drop the reference to pending call in fallback scenario.
Otherwise, dbus will assert once timer expires.
---
src/agent.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/agent.c b/src/agent.c
index c7fdbd4..b65a550 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -706,6 +706,7 @@ static int request_fallback(struct agent_request *req,
return -EINVAL;
dbus_pending_call_cancel(req->call);
+ dbus_pending_call_unref(req->call);
msg = dbus_message_copy(req->msg);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] Unref pending call in fallback scenario.
From: Johan Hedberg @ 2010-10-14 20:34 UTC (permalink / raw)
To: Lukasz Rymanowski; +Cc: linux-bluetooth, Par-Gunnar.p.HJALMDAHL
In-Reply-To: <1287069227-4076-1-git-send-email-lukasz.rymanowski@tieto.com>
Hi Lukasz,
On Thu, Oct 14, 2010, Lukasz Rymanowski wrote:
> Drop the reference to pending call in fallback scenario.
> Otherwise, dbus will assert once timer expires.
> ---
> src/agent.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/agent.c b/src/agent.c
> index c7fdbd4..b65a550 100644
> --- a/src/agent.c
> +++ b/src/agent.c
> @@ -706,6 +706,7 @@ static int request_fallback(struct agent_request *req,
> return -EINVAL;
>
> dbus_pending_call_cancel(req->call);
> + dbus_pending_call_unref(req->call);
>
> msg = dbus_message_copy(req->msg);
>
Good catch. The patch has been pushed upstream. Over the last few years
I've bumped into this libdbus issue several times but it seems this
particular code path (request_fallback) gets triggered seldom enough for
it to not have surfaced earlier. I also vaguely remember that this got
fixed in newer D-Bus versions so those might not assert in this case
anymore.
Johan
^ permalink raw reply
* Re: [PATCH] Fix not responding Not Found when no entry is found
From: Johan Hedberg @ 2010-10-14 20:35 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1287063243-23296-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Thu, Oct 14, 2010, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>
> Accourding to PBAP spec PSE must reply Not Found when:
>
> "The function was recognized and all the parameters are proper, but the
> vCard handle or the phone book object could not be found."
> ---
> plugins/pbap.c | 15 +++++++++++++++
> plugins/phonebook-tracker.c | 12 +++++++++---
> 2 files changed, 24 insertions(+), 3 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] fix oops in l2cap_connect_req
From: Gustavo F. Padovan @ 2010-10-14 21:37 UTC (permalink / raw)
To: Nathan Holstein; +Cc: linux-kernel, linux-bluetooth
In-Reply-To: <AANLkTi=CNW1pu-YdvmViqHxfw-_TDzvLkj15Ai_aANWo@mail.gmail.com>
Hi Nathan,
* Nathan Holstein <nathan.holstein@gmail.com> [2010-10-14 18:37:53 -0400]:
> (Please keep me in the CC list, I'm not subscribed to lkml)
>
> [1] L2CAP module dereferences an uninitialized pointer within l2cap_connect_req.
>
> [2] I'm currently testing a 2.6.35 kernel on a Nexus One with backported
> patches from bluetooth-2.6. When testing against certain BT devices, I'm seeing
> a null-pointer deref. The crash is caused by this portion of commit e9aeb2dd:
>
> @@ -2966,6 +2991,15 @@ sendresp:
> L2CAP_INFO_REQ, sizeof(info), &info);
> }
>
> + if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_REQ_SENT) &&
> + result == L2CAP_CR_SUCCESS) {
> + u8 buf[128];
> + l2cap_pi(sk)->conf_state |= L2CAP_CONF_REQ_SENT;
> + l2cap_send_cmd(conn, l2cap_get_ident(conn), L2CAP_CONF_REQ,
> + l2cap_build_conf_req(sk, buf), buf);
> + l2cap_pi(sk)->num_conf_req++;
> + }
> +
> return 0;
> }
>
> Multiple error cases jump to the response & sendresp labels prior to
> initializing
> the "sk" variable. In the case I'm currently seeing, the remote BT
> device fails to
> properly secure the ACL, making this crash 100% reproducible.
>
> [3] Bluetooth, L2CAP
>
> [4] This bug appears to be in the mainline 2.6.36-rc? kernel, in addition to
> multiple Bluetooth development trees
>
> The following patch fixes the crash.
>
>
> --nathan
>
> ---
> In error cases when the ACL is insecure or we fail to allocate a new
> struct sock, we jump to the "response" label. If so, "sk" will be
> uninitialized and the kernel crashes.
>
> Signed-off-by: Nathan Holstein <nathan.holstein@gmail.com>
> ---
> net/bluetooth/l2cap.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index d527b10..10ae0af 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -2911,7 +2911,7 @@ static inline int l2cap_connect_req(struct
> l2cap_conn *conn, struct l2cap_cmd_hd
> struct l2cap_chan_list *list = &conn->chan_list;
> struct l2cap_conn_req *req = (struct l2cap_conn_req *) data;
> struct l2cap_conn_rsp rsp;
> - struct sock *parent, *uninitialized_var(sk);
> + struct sock *parent, *sk = 0;
Your fix is right, but please make *sk = NULL here.
When I wrote that code I thought is was a false positive, but no, it's
bug. :(
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* [PATCH] fix oops in l2cap_connect_req
From: Nathan Holstein @ 2010-10-14 22:37 UTC (permalink / raw)
To: linux-kernel, linux-bluetooth
(Please keep me in the CC list, I'm not subscribed to lkml)
[1] L2CAP module dereferences an uninitialized pointer within l2cap_connect_req.
[2] I'm currently testing a 2.6.35 kernel on a Nexus One with backported
patches from bluetooth-2.6. When testing against certain BT devices, I'm seeing
a null-pointer deref. The crash is caused by this portion of commit e9aeb2dd:
@@ -2966,6 +2991,15 @@ sendresp:
L2CAP_INFO_REQ, sizeof(info), &info);
}
+ if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_REQ_SENT) &&
+ result == L2CAP_CR_SUCCESS) {
+ u8 buf[128];
+ l2cap_pi(sk)->conf_state |= L2CAP_CONF_REQ_SENT;
+ l2cap_send_cmd(conn, l2cap_get_ident(conn), L2CAP_CONF_REQ,
+ l2cap_build_conf_req(sk, buf), buf);
+ l2cap_pi(sk)->num_conf_req++;
+ }
+
return 0;
}
Multiple error cases jump to the response & sendresp labels prior to
initializing
the "sk" variable. In the case I'm currently seeing, the remote BT
device fails to
properly secure the ACL, making this crash 100% reproducible.
[3] Bluetooth, L2CAP
[4] This bug appears to be in the mainline 2.6.36-rc? kernel, in addition to
multiple Bluetooth development trees
The following patch fixes the crash.
--nathan
---
In error cases when the ACL is insecure or we fail to allocate a new
struct sock, we jump to the "response" label. If so, "sk" will be
uninitialized and the kernel crashes.
Signed-off-by: Nathan Holstein <nathan.holstein@gmail.com>
---
net/bluetooth/l2cap.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index d527b10..10ae0af 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2911,7 +2911,7 @@ static inline int l2cap_connect_req(struct
l2cap_conn *conn, struct l2cap_cmd_hd
struct l2cap_chan_list *list = &conn->chan_list;
struct l2cap_conn_req *req = (struct l2cap_conn_req *) data;
struct l2cap_conn_rsp rsp;
- struct sock *parent, *uninitialized_var(sk);
+ struct sock *parent, *sk = 0;
int result, status = L2CAP_CS_NO_INFO;
u16 dcid = 0, scid = __le16_to_cpu(req->scid);
@@ -3020,7 +3020,7 @@ sendresp:
L2CAP_INFO_REQ, sizeof(info), &info);
}
- if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_REQ_SENT) &&
+ if (sk && !(l2cap_pi(sk)->conf_state & L2CAP_CONF_REQ_SENT) &&
result == L2CAP_CR_SUCCESS) {
u8 buf[128];
l2cap_pi(sk)->conf_state |= L2CAP_CONF_REQ_SENT;
--
1.7.2.3
^ permalink raw reply related
* [PATCH] Update Gustavo's email in AUTHORS
From: Gustavo F. Padovan @ 2010-10-14 23:26 UTC (permalink / raw)
To: linux-bluetooth
---
AUTHORS | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index 8e0a012..e063569 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -35,7 +35,7 @@ Pekka Pessi <pekka.pessi@nokia.com>
Siarhei Siamashka <siarhei.siamashka@nokia.com>
Nick Pelly <npelly@google.com>
Lennart Poettering <lennart@poettering.net>
-Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
+Gustavo F. Padovan <padovan@profusion.mobi>
Marc-Andre Lureau <marc-andre.lureau@nokia.com>
Bea Lam <bea.lam@nokia.com>
Zygo Blaxell <zygo.blaxell@xandros.com>
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH] Update Gustavo's email in AUTHORS
From: Johan Hedberg @ 2010-10-15 5:59 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <1287098803-22806-1-git-send-email-padovan@profusion.mobi>
Hi Gustavo,
On Thu, Oct 14, 2010, Gustavo F. Padovan wrote:
> ---
> AUTHORS | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 8e0a012..e063569 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -35,7 +35,7 @@ Pekka Pessi <pekka.pessi@nokia.com>
> Siarhei Siamashka <siarhei.siamashka@nokia.com>
> Nick Pelly <npelly@google.com>
> Lennart Poettering <lennart@poettering.net>
> -Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
> +Gustavo F. Padovan <padovan@profusion.mobi>
> Marc-Andre Lureau <marc-andre.lureau@nokia.com>
> Bea Lam <bea.lam@nokia.com>
> Zygo Blaxell <zygo.blaxell@xandros.com>
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* [PATCH 0/2] Fix regression on suspend on opening
From: Dmitriy Paliy @ 2010-10-15 6:57 UTC (permalink / raw)
To: linux-bluetooth
Hi,
This fixes regression when doing suspend on opening obex stream. If
obex_write_stream returns length of buffer, it is treated as an error
response later on, which is not correct.
Negative values returned by obex_write_stream are error codes, while
positive ones mean length of buffer. Positive values are never used
afterwards in the code. Therefore, due to this reason, and for being
compliant with obex_read_stream, which also returns 0 only, it was
decided to remove return of positive value representing length at all.
Result of such is also some code cleanup that removes unnecessary return
len when it is zero.
A typo is fixed in comments as well.
Br,
Dmitriy
^ permalink raw reply
* [PATCH 1/2] Fix regression on suspend on opening
From: Dmitriy Paliy @ 2010-10-15 6:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
In-Reply-To: <1287125828-5479-1-git-send-email-dmitriy.paliy@nokia.com>
This fixes regression on suspend on opening when obex_write_stream
returns length of buffer, which is treated as error response
afterwards.
---
src/obex.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/obex.c b/src/obex.c
index d1ac339..4e1db9c 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -683,7 +683,7 @@ add_header:
os->offset += len;
- return len;
+ return 0;
}
static gboolean handle_async_io(void *object, int flags, int err,
--
1.7.0.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox