[PATCH] Bluetooth: Add tty HCI driver

[PATCH] Bluetooth: Add tty HCI driver

[Eyal Reizer]

tty_hci driver exposes a /dev/hci_tty character device node, that intends
to emulate a generic /dev/ttyX device that would be used by the user-space
Bluetooth stacks to send/receive data to/from the WL combo-connectivity
chipsets.

The device driver has no internal logic of its own to intrepret data & all
such logic is handled by the user-space stack.

Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
 [Fixed checkpatch warnings]
Signed-off-by: Vishal Mahaveer <vishalm@ti.com>
 [Fixed checkpatch --strict warnings]
Signed-off-by: Eyal Reizer <eyalr@ti.com>
---
 drivers/misc/ti-st/Kconfig   |    8 +
 drivers/misc/ti-st/Makefile  |    1 +
 drivers/misc/ti-st/tty_hci.c |  538 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 547 insertions(+)
 create mode 100644 drivers/misc/ti-st/tty_hci.c

diff --git a/drivers/misc/ti-st/Kconfig b/drivers/misc/ti-st/Kconfig
index f34dcc5..f2df2c7 100644
--- a/drivers/misc/ti-st/Kconfig
+++ b/drivers/misc/ti-st/Kconfig
@@ -14,4 +14,12 @@ config TI_ST
 	  are returned to relevant protocol drivers based on their
 	  packet types.
 
+config ST_HCI
+	tristate "HCI TTY emulation driver for Bluetooth"
+	depends on TI_ST
+	help
+	  This enables the TTY device like emulation for HCI used by
+	  user-space Bluetooth stacks.
+	  It will provide a character device for user space Bluetooth stack to
+	  send/receive data over shared transport.
 endmenu
diff --git a/drivers/misc/ti-st/Makefile b/drivers/misc/ti-st/Makefile
index 78d7ebb..4546219 100644
--- a/drivers/misc/ti-st/Makefile
+++ b/drivers/misc/ti-st/Makefile
@@ -4,3 +4,4 @@
 #
 obj-$(CONFIG_TI_ST) 		+= st_drv.o
 st_drv-objs			:= st_core.o st_kim.o st_ll.o
+obj-$(CONFIG_ST_HCI)		+= tty_hci.o
diff --git a/drivers/misc/ti-st/tty_hci.c b/drivers/misc/ti-st/tty_hci.c
new file mode 100644
index 0000000..7a6669f
--- /dev/null
+++ b/drivers/misc/ti-st/tty_hci.c
@@ -0,0 +1,538 @@
+/*
+ *  TTY emulation for user-space Bluetooth stacks over HCI-H4
+ *  Copyright (C) 2011-2012 Texas Instruments
+ *  Author: 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.
+ */
+
+/** define one of the following for debugging
+#define DEBUG
+#define VERBOSE
+*/
+
+#define pr_fmt(fmt) "(hci_tty): " fmt
+#include <linux/module.h>
+#include <linux/cdev.h>
+#include <linux/fs.h>
+#include <linux/device.h>
+
+#include <linux/uaccess.h>
+#include <linux/tty.h>
+#include <linux/sched.h>
+
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/skbuff.h>
+#include <linux/interrupt.h>
+
+#include <linux/ti_wilink_st.h>
+
+/* Number of seconds to wait for registration completion
+ * when ST returns PENDING status.
+ */
+#define BT_REGISTER_TIMEOUT   6000	/* 6 sec */
+
+/**
+ * struct ti_st - driver operation structure
+ * @hdev: hci device pointer which binds to bt driver
+ * @reg_status: ST registration callback status
+ * @st_write: write function provided by the ST driver
+ *	to be used by the driver during send_frame.
+ * @wait_reg_completion - completion sync between ti_st_open
+ *	and st_reg_completion_cb.
+ */
+struct ti_st {
+	struct hci_dev *hdev;
+	char reg_status;
+	long (*st_write)(struct sk_buff *);
+	struct completion wait_reg_completion;
+	wait_queue_head_t data_q;
+	struct sk_buff_head rx_list;
+};
+
+#define DEVICE_NAME     "hci_tty"
+
+/***********Functions called from ST driver**********************************/
+/* Called by Shared Transport layer when receive data is
+ * available */
+static long st_receive(void *priv_data, struct sk_buff *skb)
+{
+	struct ti_st	*hst = (void *)priv_data;
+
+	pr_debug("@ %s", __func__);
+#ifdef VERBOSE
+	print_hex_dump(KERN_INFO, ">rx>", DUMP_PREFIX_NONE,
+		       16, 1, skb->data, skb->len, 0);
+#endif
+	skb_queue_tail(&hst->rx_list, skb);
+	wake_up_interruptible(&hst->data_q);
+	return 0;
+}
+
+/* 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_reg_completion_cb(void *priv_data, char data)
+{
+	struct ti_st	*lhst = (void *)priv_data;
+
+	pr_info("@ %s\n", __func__);
+	/* Save registration status for use in ti_st_open() */
+	lhst->reg_status = data;
+	/* complete the wait in ti_st_open() */
+	complete(&lhst->wait_reg_completion);
+}
+
+/* protocol structure registered with shared transport */
+#define MAX_BT_CHNL_IDS 3
+static struct st_proto_s ti_st_proto[MAX_BT_CHNL_IDS] = {
+	{
+		.chnl_id = 0x04, /* HCI Events */
+		.hdr_len = 2,
+		.offset_len_in_hdr = 1,
+		.len_size = 1, /* sizeof(plen) in struct hci_event_hdr */
+		.reserve = 8,
+	},
+	{
+		.chnl_id = 0x02, /* ACL */
+		.hdr_len = 4,
+		.offset_len_in_hdr = 2,
+		.len_size = 2,	/* sizeof(dlen) in struct hci_acl_hdr */
+		.reserve = 8,
+	},
+	{
+		.chnl_id = 0x03, /* SCO */
+		.hdr_len = 3,
+		.offset_len_in_hdr = 2,
+		.len_size = 1, /* sizeof(dlen) in struct hci_sco_hdr */
+		.reserve = 8,
+	},
+};
+
+/** hci_tty_open Function
+ *  This function will perform an register on ST driver.
+ *
+ *  Parameters :
+ *  @file  : File pointer for BT char driver
+ *  @inod  :
+ *  Returns  0 -  on success
+ *           else suitable error code
+ */
+int hci_tty_open(struct inode *inod, struct file *file)
+{
+	int i = 0, err = 0;
+	unsigned long timeleft;
+	struct ti_st *hst;
+
+	pr_info("inside %s (%p, %p)\n", __func__, inod, file);
+
+	hst = kzalloc(sizeof(*hst), GFP_KERNEL);
+	file->private_data = hst;
+	hst = file->private_data;
+
+	for (i = 0; i < MAX_BT_CHNL_IDS; i++) {
+		ti_st_proto[i].priv_data = hst;
+		ti_st_proto[i].max_frame_size = 1026;
+		ti_st_proto[i].recv = st_receive;
+		ti_st_proto[i].reg_complete_cb = st_reg_completion_cb;
+
+		/* Prepare wait-for-completion handler */
+		init_completion(&hst->wait_reg_completion);
+		/* Reset ST registration callback status flag,
+		 * this value will be updated in
+		 * st_reg_completion_cb()
+		 * function whenever it called from ST driver.
+		 */
+		hst->reg_status = -EINPROGRESS;
+
+		err = st_register(&ti_st_proto[i]);
+		if (!err)
+			goto done;
+
+		if (err != -EINPROGRESS) {
+			pr_err("st_register failed %d", err);
+			goto error;
+		}
+
+		/* ST is busy with either protocol
+		 * registration or firmware download.
+		 */
+		pr_debug("waiting for registration completion signal from ST");
+		timeleft = wait_for_completion_timeout
+			(&hst->wait_reg_completion,
+			 msecs_to_jiffies(BT_REGISTER_TIMEOUT));
+		if (!timeleft) {
+			pr_err("Timeout(%d sec),didn't get reg completion signal from ST",
+			       BT_REGISTER_TIMEOUT / 1000);
+			err = -ETIMEDOUT;
+			goto error;
+		}
+
+		/* Is ST registration callback
+		 * called with ERROR status? */
+		if (hst->reg_status != 0) {
+			pr_err("ST registration completed with invalid status %d",
+			       hst->reg_status);
+			err = -EAGAIN;
+			goto error;
+		}
+
+done:
+		hst->st_write = ti_st_proto[i].write;
+		if (!hst->st_write) {
+			pr_err("undefined ST write function");
+			for (i = 0; i < MAX_BT_CHNL_IDS; i++) {
+				/* Undo registration with ST */
+				err = st_unregister(&ti_st_proto[i]);
+				if (err)
+					pr_err("st_unregister() failed with error %d",
+					       err);
+				hst->st_write = NULL;
+			}
+			return -EIO;
+		}
+	}
+
+	skb_queue_head_init(&hst->rx_list);
+	init_waitqueue_head(&hst->data_q);
+
+	return 0;
+
+error:
+	kfree(hst);
+	return err;
+}
+
+/** hci_tty_release Function
+ *  This function will un-registers from the ST driver.
+ *
+ *  Parameters :
+ *  @file  : File pointer for BT char driver
+ *  @inod  :
+ *  Returns  0 -  on success
+ *           else suitable error code
+ */
+int hci_tty_release(struct inode *inod, struct file *file)
+{
+	int err, i;
+	struct ti_st *hst = file->private_data;
+
+	pr_info("inside %s (%p, %p)\n", __func__, inod, file);
+
+	for (i = 0; i < MAX_BT_CHNL_IDS; i++) {
+		err = st_unregister(&ti_st_proto[i]);
+		if (err)
+			pr_err("st_unregister(%d) failed with error %d",
+			       ti_st_proto[i].chnl_id, err);
+	}
+
+	hst->st_write = NULL;
+	skb_queue_purge(&hst->rx_list);
+	kfree(hst);
+	return err;
+}
+
+/** hci_tty_read Function
+ *
+ *  Parameters :
+ *  @file  : File pointer for BT char driver
+ *  @data  : Data which needs to be passed to APP
+ *  @size  : Length of the data passesd
+ *  offset :
+ *  Returns  Size of packet received -  on success
+ *           else suitable error code
+ */
+ssize_t hci_tty_read(struct file *file, char __user *data, size_t size,
+		     loff_t *offset)
+{
+	int len = 0, tout;
+	struct sk_buff *skb = NULL, *rskb = NULL;
+	struct ti_st	*hst;
+
+	pr_debug("inside %s (%p, %p, %u, %p)\n",
+		 __func__, file, data, size, offset);
+
+	/* Validate input parameters */
+	if ((NULL == file) || (((NULL == data) || (0 == size)))) {
+		pr_err("Invalid input params passed to %s", __func__);
+		return -EINVAL;
+	}
+
+	hst = file->private_data;
+
+	/* cannot come here if poll-ed before reading
+	 * if not poll-ed wait on the same wait_q
+	 */
+	tout = wait_event_interruptible_timeout(hst->data_q,
+						!skb_queue_empty(&hst->rx_list),
+						msecs_to_jiffies(1000));
+	/* Check for timed out condition */
+	if (0 == tout) {
+		pr_err("Device Read timed out\n");
+		return -ETIMEDOUT;
+	}
+
+	/* hst->rx_list not empty skb already present */
+	skb = skb_dequeue(&hst->rx_list);
+	if (!skb) {
+		pr_err("dequed skb is null?\n");
+		return -EIO;
+	}
+
+#ifdef VERBOSE
+	print_hex_dump(KERN_INFO, ">in>", DUMP_PREFIX_NONE,
+		       16, 1, skb->data, skb->len, 0);
+#endif
+
+	/* Forward the data to the user */
+	if (skb->len >= size) {
+		pr_err("FIONREAD not done before read\n");
+		return -ENOMEM;
+	}
+	/* returning skb */
+	rskb = alloc_skb(size, GFP_KERNEL);
+	if (!rskb)
+		return -ENOMEM;
+
+	/* cb[0] has the pkt_type 0x04 or 0x02 or 0x03 */
+	memcpy(skb_put(rskb, 1), &skb->cb[0], 1);
+	memcpy(skb_put(rskb, skb->len), skb->data, skb->len);
+
+	if (copy_to_user(data, rskb->data, rskb->len)) {
+		pr_err("unable to copy to user space\n");
+		/* Queue the skb back to head */
+		skb_queue_head(&hst->rx_list, skb);
+		kfree_skb(rskb);
+		return -EIO;
+	}
+
+	len = rskb->len;	/* len of returning skb */
+	kfree_skb(skb);
+	kfree_skb(rskb);
+	pr_debug("total size read= %d\n", len);
+	return len;
+}
+
+/* hci_tty_write Function
+ *
+ *  Parameters :
+ *  @file   : File pointer for BT char driver
+ *  @data   : packet data from BT application
+ *  @size   : Size of the packet data
+ *  @offset :
+ *  Returns  Size of packet on success
+ *           else suitable error code
+ */
+ssize_t hci_tty_write(struct file *file, const char __user *data,
+		      size_t size, loff_t *offset)
+{
+	struct ti_st *hst = file->private_data;
+	struct	sk_buff *skb;
+
+	pr_debug("inside %s (%p, %p, %u, %p)\n",
+		 __func__, file, data, size, offset);
+
+	if (!hst->st_write) {
+		pr_err(" Can't write to ST, hhci_tty->st_write null ?");
+		return -EINVAL;
+	}
+
+	skb = alloc_skb(size, GFP_KERNEL);
+	/* Validate Created SKB */
+	if (NULL == skb)
+		return -ENOMEM;
+
+	/* Forward the data from the user space to ST core */
+	if (copy_from_user(skb_put(skb, size), data, size)) {
+		pr_err(" Unable to copy from user space");
+		kfree_skb(skb);
+		return -EIO;
+	}
+
+#ifdef VERBOSE
+	pr_debug("start data..");
+	print_hex_dump(KERN_INFO, "<out<", DUMP_PREFIX_NONE,
+		       16, 1, skb->data, size, 0);
+	pr_debug("\n..end data");
+#endif
+
+	hst->st_write(skb);
+	return size;
+}
+
+/** hci_tty_ioctl Function
+ *  This will peform the functions as directed by the command and command
+ *  argument.
+ *
+ *  Parameters :
+ *  @file  : File pointer for BT char driver
+ *  @cmd   : IOCTL Command
+ *  @arg   : Command argument for IOCTL command
+ *  Returns  0 on success
+ *           else suitable error code
+ */
+static long hci_tty_ioctl(struct file *file,
+			  unsigned int cmd, unsigned long arg)
+{
+	struct sk_buff *skb = NULL;
+	int		retcode = 0;
+	struct ti_st	*hst;
+
+	pr_debug("inside %s (%p, %u, %lx)", __func__, file, cmd, arg);
+
+	/* Validate input parameters */
+	if ((NULL == file) || (0 == cmd)) {
+		pr_err("invalid input parameters passed to %s", __func__);
+		return -EINVAL;
+	}
+
+	hst = file->private_data;
+
+	switch (cmd) {
+	case FIONREAD:
+		/* Deque the SKB from the head if rx_list is not empty
+		 * update the argument with skb->len to provide amount of data
+		 * available in the available SKB +1 for the PKT_TYPE
+		 * field not provided in data by TI-ST.
+		 */
+		skb = skb_dequeue(&hst->rx_list);
+		if (skb) {
+			*(unsigned int *)arg = skb->len + 1;
+			/* Re-Store the SKB for furtur Read operations */
+			skb_queue_head(&hst->rx_list, skb);
+		} else {
+			*(unsigned int *)arg = 0;
+		}
+		pr_debug("returning %d\n", *(unsigned int *)arg);
+		break;
+	default:
+		pr_debug("Un-Identified IOCTL %d", cmd);
+		retcode = 0;
+		break;
+	}
+
+	return retcode;
+}
+
+/** hci_tty_poll Function
+ *  This function will wait till some data is received to the hci_tty driver from ST
+ *
+ *  Parameters :
+ *  @file  : File pointer for BT char driver
+ *  @wait  : POLL wait information
+ *  Returns  status of POLL on success
+ *           else suitable error code
+ */
+static unsigned int hci_tty_poll(struct file *file, poll_table *wait)
+{
+	struct ti_st	*hst = file->private_data;
+	unsigned long mask = 0;
+
+	pr_debug("@ %s\n", __func__);
+
+	/* wait to be completed by st_receive */
+	poll_wait(file, &hst->data_q, wait);
+	pr_debug("poll broke\n");
+
+	if (!skb_queue_empty(&hst->rx_list)) {
+		pr_debug("rx list que !empty\n");
+		mask |= POLLIN;	/* TODO: check app for mask */
+	}
+
+	return mask;
+}
+
+/* BT Char driver function pointers
+ * These functions are called from USER space by pefroming File Operations
+ * on /dev/hci_tty node exposed by this driver during init
+ */
+const struct file_operations hci_tty_chrdev_ops = {
+	.owner = THIS_MODULE,
+	.open = hci_tty_open,
+	.read = hci_tty_read,
+	.write = hci_tty_write,
+	.unlocked_ioctl = hci_tty_ioctl,
+	.poll = hci_tty_poll,
+	.release = hci_tty_release,
+};
+
+/*********Functions called during insmod and delmod****************************/
+
+static int hci_tty_major;		/* major number */
+static struct class *hci_tty_class;	/* class during class_create */
+static struct device *hci_tty_dev;	/* dev during device_create */
+/** hci_tty_init Function
+ *  This function Initializes the hci_tty driver parameters and exposes
+ *  /dev/hci_tty node to user space
+ *
+ *  Parameters : NULL
+ *  Returns  0 on success
+ *           else suitable error code
+ */
+static int __init hci_tty_init(void)
+{
+	pr_info("inside %s\n", __func__);
+
+	/* Expose the device DEVICE_NAME to user space
+	 * And obtain the major number for the device
+	 */
+	hci_tty_major = register_chrdev(0, DEVICE_NAME, &hci_tty_chrdev_ops);
+
+	if (0 > hci_tty_major) {
+		pr_err("Error when registering to char dev");
+		return hci_tty_major;
+	}
+
+	/*  udev */
+	hci_tty_class = class_create(THIS_MODULE, DEVICE_NAME);
+	if (IS_ERR(hci_tty_class)) {
+		pr_err("Something went wrong in class_create");
+		unregister_chrdev(hci_tty_major, DEVICE_NAME);
+		return -1;
+	}
+
+	hci_tty_dev =
+		device_create(hci_tty_class, NULL, MKDEV(hci_tty_major, 0),
+			      NULL, DEVICE_NAME);
+	if (IS_ERR(hci_tty_dev)) {
+		pr_err("Error in device create");
+		unregister_chrdev(hci_tty_major, DEVICE_NAME);
+		class_destroy(hci_tty_class);
+		return -1;
+	}
+	pr_info("allocated %d, %d\n", hci_tty_major, 0);
+	return 0;
+}
+
+/** hci_tty_exit Function
+ *  This function Destroys the hci_tty driver parameters and /dev/hci_tty node
+ *
+ *  Parameters : NULL
+ *  Returns   NULL
+ */
+static void __exit hci_tty_exit(void)
+{
+	pr_info("inside %s\n", __func__);
+	pr_info("bye.. freeing up %d\n", hci_tty_major);
+
+	device_destroy(hci_tty_class, MKDEV(hci_tty_major, 0));
+	class_destroy(hci_tty_class);
+	unregister_chrdev(hci_tty_major, DEVICE_NAME);
+}
+
+module_init(hci_tty_init);
+module_exit(hci_tty_exit);
+
+MODULE_AUTHOR("Pavan Savoy <pavan_savoy@ti.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

Re: [PATCH] Bluetooth: Add tty HCI driver

[One Thousand Gnomes]

On Thu, 30 Apr 2015 10:48:09 +0300
Eyal Reizer <eyalreizer@gmail.com> wrote:

> tty_hci driver exposes a /dev/hci_tty character device node, that intends
> to emulate a generic /dev/ttyX device that would be used by the user-space
> Bluetooth stacks to send/receive data to/from the WL combo-connectivity
> chipsets.

We have an in kernel bluetooth stack, why do we care about this - how
will people test and debug driver interactions with binary only bluetooth
stacks in userspace ?

That aside

Either

- Use the tty layer to provide your interface with a correct "emulation",

or

- Don't pretend to be a tty device in the first place

Your code behaviour is nothing like a tty or indeed much like anything
else sane.

> +int hci_tty_open(struct inode *inod, struct file *file)
> +{
> +	int i = 0, err = 0;
> +	unsigned long timeleft;
> +	struct ti_st *hst;
> +
> +	pr_info("inside %s (%p, %p)\n", __func__, inod, file);
> +
> +	hst = kzalloc(sizeof(*hst), GFP_KERNEL);
> +	file->private_data = hst;
> +	hst = file->private_data;

Just crashed if you ran out of memory


> +		pr_debug("waiting for registration completion signal from ST");
> +		timeleft = wait_for_completion_timeout
> +			(&hst->wait_reg_completion,
> +			 msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> +		if (!timeleft) {
> +			pr_err("Timeout(%d sec),didn't get reg completion signal from ST",
> +			       BT_REGISTER_TIMEOUT / 1000);
> +			err = -ETIMEDOUT;
> +			goto error;

Without de-registering stuff ?

 +
> +/** hci_tty_read Function
> + *
> + *  Parameters :
> + *  @file  : File pointer for BT char driver
> + *  @data  : Data which needs to be passed to APP
> + *  @size  : Length of the data passesd
> + *  offset :
> + *  Returns  Size of packet received -  on success
> + *           else suitable error code
> + */
> +ssize_t hci_tty_read(struct file *file, char __user *data, size_t size,
> +		     loff_t *offset)
> +{
> +	int len = 0, tout;
> +	struct sk_buff *skb = NULL, *rskb = NULL;
> +	struct ti_st	*hst;
> +
> +	pr_debug("inside %s (%p, %p, %u, %p)\n",
> +		 __func__, file, data, size, offset);
> +
> +	/* Validate input parameters */
> +	if ((NULL == file) || (((NULL == data) || (0 == size)))) {
> +		pr_err("Invalid input params passed to %s", __func__);
> +		return -EINVAL;
> +	}

Why ... if they are broken here then the kernel is already crashed.

> +
> +	hst = file->private_data;
> +
> +	/* cannot come here if poll-ed before reading
> +	 * if not poll-ed wait on the same wait_q
> +	 */
> +	tout = wait_event_interruptible_timeout(hst->data_q,
> +						!skb_queue_empty(&hst->rx_list),
> +						msecs_to_jiffies(1000));
> +	/* Check for timed out condition */
> +	if (0 == tout) {
> +		pr_err("Device Read timed out\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* hst->rx_list not empty skb already present */
> +	skb = skb_dequeue(&hst->rx_list);
> +	if (!skb) {
> +		pr_err("dequed skb is null?\n");
> +		return -EIO;
> +	}
> +
> +#ifdef VERBOSE
> +	print_hex_dump(KERN_INFO, ">in>", DUMP_PREFIX_NONE,
> +		       16, 1, skb->data, skb->len, 0);
> +#endif
> +
> +	/* Forward the data to the user */
> +	if (skb->len >= size) {
> +		pr_err("FIONREAD not done before read\n");
> +		return -ENOMEM;

Even if the user did an FIONREAD this wouldn't work with two threads.
ENOMEM is a nonsense return for it
The semantics of read/write on a tty are not those you have implemented
You don't want to allow users to fill the log with error messages


> +	}
> +	/* returning skb */
> +	rskb = alloc_skb(size, GFP_KERNEL);
> +	if (!rskb)
> +		return -ENOMEM;
> +
> +	/* cb[0] has the pkt_type 0x04 or 0x02 or 0x03 */
> +	memcpy(skb_put(rskb, 1), &skb->cb[0], 1);
> +	memcpy(skb_put(rskb, skb->len), skb->data, skb->len);
> +
> +	if (copy_to_user(data, rskb->data, rskb->len)) {
> +		pr_err("unable to copy to user space\n");

Same problem with error messages

> +		/* Queue the skb back to head */
> +		skb_queue_head(&hst->rx_list, skb);

You've just re-ordered your list if threaded


> +/** hci_tty_ioctl Function
> + *  This will peform the functions as directed by the command and command
> + *  argument.
> + *
> + *  Parameters :
> + *  @file  : File pointer for BT char driver
> + *  @cmd   : IOCTL Command
> + *  @arg   : Command argument for IOCTL command
> + *  Returns  0 on success
> + *           else suitable error code
> + */
> +static long hci_tty_ioctl(struct file *file,
> +			  unsigned int cmd, unsigned long arg)
> +{
> +	struct sk_buff *skb = NULL;
> +	int		retcode = 0;
> +	struct ti_st	*hst;
> +
> +	pr_debug("inside %s (%p, %u, %lx)", __func__, file, cmd, arg);
> +
> +	/* Validate input parameters */
> +	if ((NULL == file) || (0 == cmd)) {
> +		pr_err("invalid input parameters passed to %s", __func__);
> +		return -EINVAL;
> +	}

More nonsense validation
> +
> +	hst = file->private_data;
> +
> +	switch (cmd) {
> +	case FIONREAD:
> +		/* Deque the SKB from the head if rx_list is not empty
> +		 * update the argument with skb->len to provide amount of data
> +		 * available in the available SKB +1 for the PKT_TYPE
> +		 * field not provided in data by TI-ST.
> +		 */
> +		skb = skb_dequeue(&hst->rx_list);
> +		if (skb) {
> +			*(unsigned int *)arg = skb->len + 1;

arg is in userspace is it not - if so you've just added some interesting
holes because you should be using the proper user access functions.

> +			/* Re-Store the SKB for furtur Read operations */
> +			skb_queue_head(&hst->rx_list, skb);

Re-ordering problems again here.

> +		} else {
> +			*(unsigned int *)arg = 0;
> +		}
> +		pr_debug("returning %d\n", *(unsigned int *)arg);
> +		break;
> +	default:
> +		pr_debug("Un-Identified IOCTL %d", cmd);
> +		retcode = 0;
> +		break;
> +	}
> +
> +	return retcode;
> +}
> +
> +/** hci_tty_poll Function
> + *  This function will wait till some data is received to the hci_tty driver from ST
> + *
> + *  Parameters :
> + *  @file  : File pointer for BT char driver
> + *  @wait  : POLL wait information
> + *  Returns  status of POLL on success
> + *           else suitable error code
> + */
> +static unsigned int hci_tty_poll(struct file *file, poll_table *wait)
> +{
> +	struct ti_st	*hst = file->private_data;
> +	unsigned long mask = 0;
> +
> +	pr_debug("@ %s\n", __func__);
> +
> +	/* wait to be completed by st_receive */
> +	poll_wait(file, &hst->data_q, wait);
> +	pr_debug("poll broke\n");

Why "broke" ?

> +
> +	if (!skb_queue_empty(&hst->rx_list)) {
> +		pr_debug("rx list que !empty\n");
> +		mask |= POLLIN;	/* TODO: check app for mask */
> +	}
> +

Alan

Re: [PATCH] Bluetooth: Add tty HCI driver

[Marcel Holtmann]

Hi,

>> tty_hci driver exposes a /dev/hci_tty character device node, that intends
>> to emulate a generic /dev/ttyX device that would be used by the user-space
>> Bluetooth stacks to send/receive data to/from the WL combo-connectivity
>> chipsets.
> 
> We have an in kernel bluetooth stack, why do we care about this - how
> will people test and debug driver interactions with binary only bluetooth
> stacks in userspace ?

NAK to this driver.

If you want to run an external Bluetooth stack, then use HCI User Channel. The functionality already exists.

We do not want to have two drivers for the same hardware. Work with the Bluetooth stack that is present in the kernel and please stop hacking around it.

Regards

Marcel

RE: [PATCH] Bluetooth: Add tty HCI driver

[Reizer, Eyal]

Hi,

> -----Original Message-----
> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Thursday, April 30, 2015 5:25 PM
> To: One Thousand Gnomes
> Cc: Eyal Reizer; linux-kernel@vger.kernel.org; Reizer, Eyal; Pavan Savoy;
> Mahaveer, Vishal; linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH] Bluetooth: Add tty HCI driver
> 
> Hi,
> 
> >> tty_hci driver exposes a /dev/hci_tty character device node, that
> >> intends to emulate a generic /dev/ttyX device that would be used by
> >> the user-space Bluetooth stacks to send/receive data to/from the WL
> >> combo-connectivity chipsets.
> >
> > We have an in kernel bluetooth stack, why do we care about this - how
> > will people test and debug driver interactions with binary only
> > bluetooth stacks in userspace ?
> 
> NAK to this driver.
> 
> If you want to run an external Bluetooth stack, then use HCI User Channel. The
> functionality already exists.

This is a pretty old driver that has been written a couple of years back and is being used by customers (including android that use a userspace based stack).
If there is another driver already available that provides same functionality, of course we will be happy to use it instead. 
Any idea of where we can get info on the HCI User Channel?

> 
> We do not want to have two drivers for the same hardware. Work with the
> Bluetooth stack that is present in the kernel and please stop hacking around it.
> 
There are in fact quite a few products that use commercial user space based Bluetooth stacks.
One reason is that bluez has not been certified but I assume they have other reasons as well.

Best Regards,
Eyal

RE: [PATCH] Bluetooth: Add tty HCI driver

[Reizer, Eyal]

Hi,

> -----Original Message-----
> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Thursday, April 30, 2015 5:25 PM
> To: One Thousand Gnomes
> Cc: Eyal Reizer; linux-kernel@vger.kernel.org; Reizer, Eyal; Pavan Savoy;
> Mahaveer, Vishal; linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH] Bluetooth: Add tty HCI driver
>=20
> Hi,
>=20
> >> tty_hci driver exposes a /dev/hci_tty character device node, that
> >> intends to emulate a generic /dev/ttyX device that would be used by
> >> the user-space Bluetooth stacks to send/receive data to/from the WL
> >> combo-connectivity chipsets.
> >
> > We have an in kernel bluetooth stack, why do we care about this - how
> > will people test and debug driver interactions with binary only
> > bluetooth stacks in userspace ?
>=20
> NAK to this driver.
>=20
> If you want to run an external Bluetooth stack, then use HCI User Channel=
. The
> functionality already exists.

This is a pretty old driver that has been written a couple of years back an=
d is being used by customers (including android that use a userspace based =
stack).
If there is another driver already available that provides same functionali=
ty, of course we will be happy to use it instead.=20
Any idea of where we can get info on the HCI User Channel?

>=20
> We do not want to have two drivers for the same hardware. Work with the
> Bluetooth stack that is present in the kernel and please stop hacking aro=
und it.
>=20
There are in fact quite a few products that use commercial user space based=
 Bluetooth stacks.
One reason is that bluez has not been certified but I assume they have othe=
r reasons as well.

Best Regards,
Eyal

Re: [PATCH] Bluetooth: Add tty HCI driver

[Marcel Holtmann]

Hi Eyal,

>>>> tty_hci driver exposes a /dev/hci_tty character device node, that
>>>> intends to emulate a generic /dev/ttyX device that would be used by
>>>> the user-space Bluetooth stacks to send/receive data to/from the WL
>>>> combo-connectivity chipsets.
>>> 
>>> We have an in kernel bluetooth stack, why do we care about this - how
>>> will people test and debug driver interactions with binary only
>>> bluetooth stacks in userspace ?
>> 
>> NAK to this driver.
>> 
>> If you want to run an external Bluetooth stack, then use HCI User Channel. The
>> functionality already exists.
> 
> This is a pretty old driver that has been written a couple of years back and is being used by customers (including android that use a userspace based stack).
> If there is another driver already available that provides same functionality, of course we will be happy to use it instead. 
> Any idea of where we can get info on the HCI User Channel?

search for my BlueZ for Android presentation from Android Builders Summit from 2014. It has an explanation on how HCI User Channel works.

Otherwise look into bluez.git since it contains a bunch of examples on how to use the HCI User Channel. It is a generic functionality for raw access to the HCI layer. Look for HCI_CHANNEL_USER.

>> We do not want to have two drivers for the same hardware. Work with the
>> Bluetooth stack that is present in the kernel and please stop hacking around it.
>> 
> There are in fact quite a few products that use commercial user space based Bluetooth stacks.
> One reason is that bluez has not been certified but I assume they have other reasons as well.

Please stop this FUD. BlueZ has been qualified multiple times.

And let me tell you this, when doing BlueZ for Android, we went through a lengthy period of qualification and fixing PTS with 65 errata today. I don't know if any commercial stack was actually such a good citizen in ensuring that the certification tools get fixed. And we documented all of it.

Clone the bluez.git tree and look for android/{pts,pics,pixit}-*.txt files. They contain all details for qualifying BlueZ for Android stack.

Regards

Marcel

RE: [PATCH] Bluetooth: Add tty HCI driver

[Ziv, Dotan]

Hi Marcel

>-----Original Message-----
>From: Marcel Holtmann [mailto:marcel@holtmann.org]
>Sent: Thursday, April 30, 2015 5:58 PM
>To: Reizer, Eyal
>Cc: One Thousand Gnomes; Eyal Reizer; linux-kernel@vger.kernel.org; Pavan
>Savoy; Mahaveer, Vishal; linux-bluetooth@vger.kernel.org; Ziv, Dotan
>Subject: Re: [PATCH] Bluetooth: Add tty HCI driver
>
>Hi Eyal,
>
>>>>> tty_hci driver exposes a /dev/hci_tty character device node, that
>>>>> intends to emulate a generic /dev/ttyX device that would be used by
>>>>> the user-space Bluetooth stacks to send/receive data to/from the WL
>>>>> combo-connectivity chipsets.
>>>>
>>>> We have an in kernel bluetooth stack, why do we care about this -
>>>> how will people test and debug driver interactions with binary only
>>>> bluetooth stacks in userspace ?
>>>
>>> NAK to this driver.
>>>
>>> If you want to run an external Bluetooth stack, then use HCI User
>>> Channel. The functionality already exists.
>>
>> This is a pretty old driver that has been written a couple of years back and is
>being used by customers (including android that use a userspace based stack).
>> If there is another driver already available that provides same functionality,
>of course we will be happy to use it instead.
>> Any idea of where we can get info on the HCI User Channel?
>
>search for my BlueZ for Android presentation from Android Builders Summit
>from 2014. It has an explanation on how HCI User Channel works.
>
>Otherwise look into bluez.git since it contains a bunch of examples on how to
>use the HCI User Channel. It is a generic functionality for raw access to the HCI
>layer. Look for HCI_CHANNEL_USER.
>
>>> We do not want to have two drivers for the same hardware. Work with
>>> the Bluetooth stack that is present in the kernel and please stop hacking
>around it.
>>>
>> There are in fact quite a few products that use commercial user space based
>Bluetooth stacks.
>> One reason is that bluez has not been certified but I assume they have
>other reasons as well.
>
>Please stop this FUD. BlueZ has been qualified multiple times.
>
>And let me tell you this, when doing BlueZ for Android, we went through a
>lengthy period of qualification and fixing PTS with 65 errata today. I don't
>know if any commercial stack was actually such a good citizen in ensuring that
>the certification tools get fixed. And we documented all of it.
>

No argue here, as a matter of fact, we (TI) certified BlueZ several times in the past. Our customers are using large variety of Kernel versions and might use a different version of BlueZ. Our BQE requested us to certify the stack per kernel and per BlueZ version which is practically impossible (due to certification costs and the time it takes). 
If you are familiar with a way to bypass it, that would be great.

>Clone the bluez.git tree and look for android/{pts,pics,pixit}-*.txt files. They
>contain all details for qualifying BlueZ for Android stack.
>
>Regards
>
>Marcel

Regards,
Dotan

RE: [PATCH] Bluetooth: Add tty HCI driver

[Ziv, Dotan]

Hi Marcel

>-----Original Message-----
>From: Marcel Holtmann [mailto:marcel@holtmann.org]
>Sent: Thursday, April 30, 2015 5:58 PM
>To: Reizer, Eyal
>Cc: One Thousand Gnomes; Eyal Reizer; linux-kernel@vger.kernel.org; Pavan
>Savoy; Mahaveer, Vishal; linux-bluetooth@vger.kernel.org; Ziv, Dotan
>Subject: Re: [PATCH] Bluetooth: Add tty HCI driver
>
>Hi Eyal,
>
>>>>> tty_hci driver exposes a /dev/hci_tty character device node, that
>>>>> intends to emulate a generic /dev/ttyX device that would be used by
>>>>> the user-space Bluetooth stacks to send/receive data to/from the WL
>>>>> combo-connectivity chipsets.
>>>>
>>>> We have an in kernel bluetooth stack, why do we care about this -
>>>> how will people test and debug driver interactions with binary only
>>>> bluetooth stacks in userspace ?
>>>
>>> NAK to this driver.
>>>
>>> If you want to run an external Bluetooth stack, then use HCI User
>>> Channel. The functionality already exists.
>>
>> This is a pretty old driver that has been written a couple of years back=
 and is
>being used by customers (including android that use a userspace based stac=
k).
>> If there is another driver already available that provides same function=
ality,
>of course we will be happy to use it instead.
>> Any idea of where we can get info on the HCI User Channel?
>
>search for my BlueZ for Android presentation from Android Builders Summit
>from 2014. It has an explanation on how HCI User Channel works.
>
>Otherwise look into bluez.git since it contains a bunch of examples on how=
 to
>use the HCI User Channel. It is a generic functionality for raw access to =
the HCI
>layer. Look for HCI_CHANNEL_USER.
>
>>> We do not want to have two drivers for the same hardware. Work with
>>> the Bluetooth stack that is present in the kernel and please stop hacki=
ng
>around it.
>>>
>> There are in fact quite a few products that use commercial user space ba=
sed
>Bluetooth stacks.
>> One reason is that bluez has not been certified but I assume they have
>other reasons as well.
>
>Please stop this FUD. BlueZ has been qualified multiple times.
>
>And let me tell you this, when doing BlueZ for Android, we went through a
>lengthy period of qualification and fixing PTS with 65 errata today. I don=
't
>know if any commercial stack was actually such a good citizen in ensuring =
that
>the certification tools get fixed. And we documented all of it.
>

No argue here, as a matter of fact, we (TI) certified BlueZ several times i=
n the past. Our customers are using large variety of Kernel versions and mi=
ght use a different version of BlueZ. Our BQE requested us to certify the s=
tack per kernel and per BlueZ version which is practically impossible (due =
to certification costs and the time it takes).=20
If you are familiar with a way to bypass it, that would be great.

>Clone the bluez.git tree and look for android/{pts,pics,pixit}-*.txt files=
. They
>contain all details for qualifying BlueZ for Android stack.
>
>Regards
>
>Marcel

Regards,
Dotan

Re: [PATCH] Bluetooth: Add tty HCI driver

[Marcel Holtmann]

Hi Dotan,

>>>>>> tty_hci driver exposes a /dev/hci_tty character device node, that
>>>>>> intends to emulate a generic /dev/ttyX device that would be used by
>>>>>> the user-space Bluetooth stacks to send/receive data to/from the WL
>>>>>> combo-connectivity chipsets.
>>>>> 
>>>>> We have an in kernel bluetooth stack, why do we care about this -
>>>>> how will people test and debug driver interactions with binary only
>>>>> bluetooth stacks in userspace ?
>>>> 
>>>> NAK to this driver.
>>>> 
>>>> If you want to run an external Bluetooth stack, then use HCI User
>>>> Channel. The functionality already exists.
>>> 
>>> This is a pretty old driver that has been written a couple of years back and is
>> being used by customers (including android that use a userspace based stack).
>>> If there is another driver already available that provides same functionality,
>> of course we will be happy to use it instead.
>>> Any idea of where we can get info on the HCI User Channel?
>> 
>> search for my BlueZ for Android presentation from Android Builders Summit
>> from 2014. It has an explanation on how HCI User Channel works.
>> 
>> Otherwise look into bluez.git since it contains a bunch of examples on how to
>> use the HCI User Channel. It is a generic functionality for raw access to the HCI
>> layer. Look for HCI_CHANNEL_USER.
>> 
>>>> We do not want to have two drivers for the same hardware. Work with
>>>> the Bluetooth stack that is present in the kernel and please stop hacking
>> around it.
>>>> 
>>> There are in fact quite a few products that use commercial user space based
>> Bluetooth stacks.
>>> One reason is that bluez has not been certified but I assume they have
>> other reasons as well.
>> 
>> Please stop this FUD. BlueZ has been qualified multiple times.
>> 
>> And let me tell you this, when doing BlueZ for Android, we went through a
>> lengthy period of qualification and fixing PTS with 65 errata today. I don't
>> know if any commercial stack was actually such a good citizen in ensuring that
>> the certification tools get fixed. And we documented all of it.
>> 
> 
> No argue here, as a matter of fact, we (TI) certified BlueZ several times in the past. Our customers are using large variety of Kernel versions and might use a different version of BlueZ. Our BQE requested us to certify the stack per kernel and per BlueZ version which is practically impossible (due to certification costs and the time it takes). 
> If you are familiar with a way to bypass it, that would be great.

that is why we documented everything for BlueZ for Android in such detail so that the whole host stack can be qualified using PTS. It still means that you have to run 1000+ test cases, but at least you have the full documentation with detailed instructions for it.

Automating PTS is work in progress actually. So that you can just start the process and once it is done, you get your evidence for qualification.

Regards

Marcel