[RFC 0/5] Move HCI UART vendor specific setup to kernel

[RFC 0/5] Move HCI UART vendor specific setup to kernel

[Frederic Danis]

This is a proposal to move the vendor specific setup (firmware loading and
final speed setup) to the kernel.
This will allow vendor specific setup function to use the HCI protocols already
implemented in kernel.

This adds 2 new IOCTLs (HCIUARTSETDEVTYPE and HCIUARTSETBAUDRATE) to be able to
select vendor specific setup function and final baudrate to use.

User space application in charge of attaching Bluetooth UART device will have to
call them before changing the proto to use (HCIUARTSETPROTO).

UART baud rate setup needs to set tty_set_termios() publicly available
(partially reverting patch "tty: Remove external interface for tty_set_termios()"
from Peter Hurley 2015-01-25 SHA1 632f32e2107).

A first work is provided based on Broadcom chip integrated in Asus T100.

Future: A third IOCTL (HCIUARTLISTDEVTYPE) can be added to allow user space
application to list supported Bluetooth device types by kernel.

Frederic Danis (5):
  Bluetooth: hci_uart: Add HCIUARTSETDEVTYPE ioctl
  Bluetooth: hci_uart: Add BCM specific setup function
  Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl
  tty: Re-add external interface for tty_set_termios()
  Bluetooth: hci_uart: Add BCM specific UART speed management

 drivers/bluetooth/Kconfig     |  10 ++
 drivers/bluetooth/Makefile    |   1 +
 drivers/bluetooth/hci_bcm.c   | 351 ++++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_ldisc.c |  59 +++++++
 drivers/bluetooth/hci_uart.h  |  10 ++
 drivers/tty/tty_ioctl.c       |   3 +-
 include/linux/tty.h           |   1 +
 7 files changed, 434 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bluetooth/hci_bcm.c

-- 
1.9.1

[RFC 1/5] Bluetooth: hci_uart: Add HCIUARTSETDEVTYPE ioctl

[Frederic Danis]

This allows user space application to set the correct vendor specific
setup function.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_ldisc.c | 50 +++++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_uart.h  |  4 ++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 1363dc6..cb7fcc6 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -467,9 +467,19 @@ static int hci_uart_register_dev(struct hci_uart *hu)
 	return 0;
 }
 
+struct hci_uart_devtype_t {
+	char type[HCI_UART_DEVTYPE_SIZE];
+	int (*setup)(struct hci_uart *hu);
+};
+
+struct hci_uart_devtype_t devtypes[] = {
+		{ "", 0 }
+};
+
 static int hci_uart_set_proto(struct hci_uart *hu, int id)
 {
 	struct hci_uart_proto *p;
+	int i;
 	int err;
 
 	p = hci_uart_get_proto(id);
@@ -482,6 +492,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
 
 	hu->proto = p;
 
+	/* Select vendor specific setup */
+	hu->proto->setup = NULL;
+	for (i = 0; devtypes[i].type[0]; i++) {
+		if (!strcmp(devtypes[i].type, hu->dev_type))
+			hu->proto->setup = devtypes[i].setup;
+	}
+
 	err = hci_uart_register_dev(hu);
 	if (err) {
 		p->close(hu);
@@ -507,6 +524,33 @@ static int hci_uart_set_flags(struct hci_uart *hu, unsigned long flags)
 	return 0;
 }
 
+static int hci_tty_ioctl_set_devtype(struct hci_uart *hu, unsigned int cmd,
+							unsigned long arg)
+{
+	unsigned int size = _IOC_SIZE(cmd);
+	char dev_type[HCI_UART_DEVTYPE_SIZE];
+	int i;
+	bool found = false;
+
+	if (size > HCI_UART_DEVTYPE_SIZE)
+		return -EINVAL;
+
+	if (copy_from_user(dev_type, (void __user *)arg, size))
+		return -EFAULT;
+
+	for (i = 0; devtypes[i].type[0]; i++) {
+		if (!strcmp(devtypes[i].type, dev_type))
+			found = true;
+	}
+
+	if (!found)
+		return -EINVAL;
+
+	strncpy(hu->dev_type, dev_type, size);
+
+	return 0;
+}
+
 /* hci_uart_tty_ioctl()
  *
  *    Process IOCTL system call for the tty device.
@@ -565,6 +609,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file * file,
 	case HCIUARTGETFLAGS:
 		return hu->hdev_flags;
 
+	case HCIUARTSETDEVTYPE:
+		err = hci_tty_ioctl_set_devtype(hu, cmd, arg);
+		if (err)
+			return err;
+		break;
+
 	default:
 		err = n_tty_ioctl_helper(tty, file, cmd, arg);
 		break;
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 074ed29..bd56409 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -33,6 +33,7 @@
 #define HCIUARTGETDEVICE	_IOR('U', 202, int)
 #define HCIUARTSETFLAGS		_IOW('U', 203, int)
 #define HCIUARTGETFLAGS		_IOR('U', 204, int)
+#define HCIUARTSETDEVTYPE	_IOW('U', 205, int)
 
 /* UART protocols */
 #define HCI_UART_MAX_PROTO	6
@@ -50,6 +51,8 @@
 #define HCI_UART_INIT_PENDING	3
 #define HCI_UART_EXT_CONFIG	4
 
+#define HCI_UART_DEVTYPE_SIZE	32
+
 struct hci_uart;
 
 struct hci_uart_proto {
@@ -68,6 +71,7 @@ struct hci_uart {
 	struct hci_dev		*hdev;
 	unsigned long		flags;
 	unsigned long		hdev_flags;
+	char			dev_type[HCI_UART_DEVTYPE_SIZE];
 
 	struct work_struct	init_ready;
 	struct work_struct	write_work;
-- 
1.9.1

[RFC 2/5] Bluetooth: hci_uart: Add BCM specific setup function

[Frederic Danis]

BCM specific setup loads firmware.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/Kconfig     |  10 ++
 drivers/bluetooth/Makefile    |   1 +
 drivers/bluetooth/hci_bcm.c   | 240 ++++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_ldisc.c |   5 +
 drivers/bluetooth/hci_uart.h  |   4 +
 5 files changed, 260 insertions(+)
 create mode 100644 drivers/bluetooth/hci_bcm.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 364f080..59c4eb0 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -94,6 +94,16 @@ config BT_HCIUART_3WIRE
 
 	  Say Y here to compile support for Three-wire UART protocol.
 
+config BT_HCIUART_BCM
+	bool "HCI BCM UART driver"
+	depends on BT_HCIUART_H4
+	help
+	  Bluetooth HCI UART BCM driver.
+	  This driver provides the firmware loading mechanism for the Broadcom
+	  UART based devices.
+
+	  Say Y here to compile support for HCI UART BCM devices.
+
 config BT_HCIBCM203X
 	tristate "HCI BCM203x USB driver"
 	depends on USB
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 9fe8a87..9acbee3 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -29,6 +29,7 @@ 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-$(CONFIG_BT_HCIUART_3WIRE)	+= hci_h5.o
+hci_uart-$(CONFIG_BT_HCIUART_BCM)	+= hci_bcm.o
 hci_uart-objs				:= $(hci_uart-y)
 
 ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
new file mode 100644
index 0000000..7cec648
--- /dev/null
+++ b/drivers/bluetooth/hci_bcm.c
@@ -0,0 +1,240 @@
+/*
+*  Broadcom Bluetooth vendor functions
+*
+*  Copyright (C) 2005-2008  Marcel Holtmann <marcel@holtmann.org>
+*  Copyright (C) 2015, Intel Corporation
+*
+*  This program is free software; you can redistribute it and/or modify
+*  it under the terms of the GNU General Public License as published by
+*  the Free Software Foundation; either version 2 of the License, or
+*  (at your option) any later version.
+*/
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/skbuff.h>
+#include <linux/firmware.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "hci_uart.h"
+
+static int hci_bcm_reset(struct hci_uart *hu)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		BT_ERR("%s: HCI_OP_RESET failed (%ld)",
+		       hdev->name, PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int hci_bcm_read_local_name(struct hci_uart *hu, char *name, size_t size)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+	struct hci_rp_read_local_name *rp;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		BT_ERR("%s: HCI_OP_READ_LOCAL_NAME failed (%ld)",
+		       hdev->name, PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	if (skb->len != sizeof(struct hci_rp_read_local_name)) {
+		BT_ERR("%s: HCI_OP_READ_LOCAL_NAME event length mismatch",
+		       hdev->name);
+		kfree_skb(skb);
+		return -EIO;
+	}
+
+	rp = (struct hci_rp_read_local_name *)skb->data;
+	if (rp->status) {
+		BT_ERR("%s: HCI_OP_READ_LOCAL_NAME error status (%02x)",
+		       hdev->name, rp->status);
+		kfree_skb(skb);
+		return -EIO;
+	}
+
+	strncpy(name, rp->name, size - 1);
+	name[size - 1] = '\0';
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int hci_bcm_read_local_version(struct hci_uart *hu, char *name,
+								size_t size)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+	struct hci_rp_read_local_version *rp;
+	uint16_t ver;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
+		       hdev->name, PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	if (skb->len != sizeof(struct hci_rp_read_local_version)) {
+		BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION event length mismatch",
+		       hdev->name);
+		kfree_skb(skb);
+		return -EIO;
+	}
+
+	rp = (struct hci_rp_read_local_version *)skb->data;
+	if (rp->status) {
+		BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION error status (%02x)",
+		       hdev->name, rp->status);
+		kfree_skb(skb);
+		return -EIO;
+	}
+
+	ver = le16_to_cpu(rp->lmp_subver);
+	snprintf(name, size, "%3.3u.%3.3u.%3.3u", (ver & 0x7000) >> 13,
+		 (ver & 0x1f00) >> 8, ver & 0x00ff);
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static const struct firmware *hci_bcm_get_fw(struct hci_uart *hu,
+						const char *fwname)
+{
+	struct hci_dev *hdev = hu->hdev;
+	const struct firmware *fw;
+	int ret;
+
+	ret = request_firmware(&fw, fwname, &hdev->dev);
+	if (ret < 0) {
+		if (ret == -EINVAL) {
+			BT_ERR("%s: BCM firmware file request failed (%d)",
+			       hdev->name, ret);
+			return NULL;
+		}
+
+		BT_ERR("%s: failed to open BCM firmware file: %s(%d)",
+		       hdev->name, fwname, ret);
+
+		return NULL;
+	}
+
+	BT_INFO("%s: BCM Bluetooth firmware file: %s", hdev->name, fwname);
+
+	return fw;
+}
+
+static int hci_bcm_load_firmware(struct hci_uart *hu, const struct firmware *fw)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+	const u8 *fw_ptr;
+	size_t fw_size;
+	const struct hci_command_hdr *cmd;
+	const u8 *cmd_param;
+	u16 opcode;
+
+	BT_DBG("%s: %p", hdev->name, hu);
+
+	skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		BT_ERR("%s: failed to write download mode command (%ld)",
+		       hdev->name, PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+	kfree_skb(skb);
+
+	/* Wait 50ms to let the firmware placed in download mode */
+	msleep(50);
+
+	fw_ptr = fw->data;
+	fw_size = fw->size;
+
+	while (fw_size >= sizeof(*cmd)) {
+		cmd = (struct hci_command_hdr *)fw_ptr;
+		fw_ptr += sizeof(*cmd);
+		fw_size -= sizeof(*cmd);
+
+		if (fw_size < cmd->plen) {
+			BT_ERR("%s: patch is corrupted", hdev->name);
+			return -EINVAL;
+		}
+
+		cmd_param = fw_ptr;
+		fw_ptr += cmd->plen;
+		fw_size -= cmd->plen;
+
+		opcode = le16_to_cpu(cmd->opcode);
+
+		skb = __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_param,
+				     HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			BT_ERR("%s: patch command %04x failed (%ld)",
+			       hdev->name, opcode, PTR_ERR(skb));
+			return PTR_ERR(skb);
+		}
+		kfree_skb(skb);
+	}
+
+	/* Wait for firmware ready */
+	msleep(2000);
+
+	return 0;
+}
+
+int hci_bcm_setup(struct hci_uart *hu)
+{
+	char name[20];
+	char version[12];
+	char fwname[64];
+	const struct firmware *fw = NULL;
+	int err = 0;
+
+	BT_DBG("%s: %p", hu->hdev->name, hu);
+
+	err = hci_bcm_reset(hu);
+	if (err)
+		goto failed;
+
+	err = hci_bcm_read_local_name(hu, name, sizeof(name));
+	if (err)
+		goto failed;
+
+	err = hci_bcm_read_local_version(hu, version, sizeof(version));
+	if (err)
+		goto failed;
+
+	snprintf(fwname, sizeof(fwname), "brcm/%s_%s.hcd", name, version);
+
+	fw = hci_bcm_get_fw(hu, fwname);
+	if (fw) {
+		err = hci_bcm_load_firmware(hu, fw);
+		if (err)
+			goto failed;
+
+		err = hci_bcm_reset(hu);
+		if (err)
+			goto failed;
+	}
+
+failed:
+	if (fw)
+		release_firmware(fw);
+
+	return err;
+}
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index cb7fcc6..e8412f8 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -473,6 +473,11 @@ struct hci_uart_devtype_t {
 };
 
 struct hci_uart_devtype_t devtypes[] = {
+#ifdef CONFIG_BT_HCIUART_BCM
+		/* Broadcom */
+		{ "bcm", hci_bcm_setup},
+#endif
+
 		{ "", 0 }
 };
 
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index bd56409..bf6f0e5 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -121,3 +121,7 @@ int ath_deinit(void);
 int h5_init(void);
 int h5_deinit(void);
 #endif
+
+#ifdef CONFIG_BT_HCIUART_BCM
+int hci_bcm_setup(struct hci_uart *hu);
+#endif
-- 
1.9.1

[RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Frederic Danis]

This allows user space application to set final speed requested for UART
device. UART port is open at init speed by user space application.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_ldisc.c | 4 ++++
 drivers/bluetooth/hci_uart.h  | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index e8412f8..4b09369 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -614,6 +614,10 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file * file,
 	case HCIUARTGETFLAGS:
 		return hu->hdev_flags;
 
+	case HCIUARTSETBAUDRATE:
+		hu->speed = arg;
+		break;
+
 	case HCIUARTSETDEVTYPE:
 		err = hci_tty_ioctl_set_devtype(hu, cmd, arg);
 		if (err)
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index bf6f0e5..dcbedaf 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -34,6 +34,7 @@
 #define HCIUARTSETFLAGS		_IOW('U', 203, int)
 #define HCIUARTGETFLAGS		_IOR('U', 204, int)
 #define HCIUARTSETDEVTYPE	_IOW('U', 205, int)
+#define HCIUARTSETBAUDRATE	_IOW('U', 206, int)
 
 /* UART protocols */
 #define HCI_UART_MAX_PROTO	6
@@ -72,6 +73,7 @@ struct hci_uart {
 	unsigned long		flags;
 	unsigned long		hdev_flags;
 	char			dev_type[HCI_UART_DEVTYPE_SIZE];
+	unsigned long		speed;
 
 	struct work_struct	init_ready;
 	struct work_struct	write_work;
-- 
1.9.1

[RFC 4/5] tty: Re-add external interface for tty_set_termios()

[Frederic Danis]

This is needed by Bluetooth hci_uart module to be able to change speed
of Bluetooth controller and local UART.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/tty/tty_ioctl.c | 3 ++-
 include/linux/tty.h     | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 632fc81..8e53fe4 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -536,7 +536,7 @@ EXPORT_SYMBOL(tty_termios_hw_change);
  *	Locking: termios_rwsem
  */
 
-static int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
+int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
 {
 	struct ktermios old_termios;
 	struct tty_ldisc *ld;
@@ -569,6 +569,7 @@ static int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
 	up_write(&tty->termios_rwsem);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(tty_set_termios);
 
 /**
  *	set_termios		-	set termios values for a tty
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 358a337..fe5623c 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -491,6 +491,7 @@ static inline speed_t tty_get_baud_rate(struct tty_struct *tty)
 
 extern void tty_termios_copy_hw(struct ktermios *new, struct ktermios *old);
 extern int tty_termios_hw_change(struct ktermios *a, struct ktermios *b);
+extern int tty_set_termios(struct tty_struct *tty, struct ktermios *kt);
 
 extern struct tty_ldisc *tty_ldisc_ref(struct tty_struct *);
 extern void tty_ldisc_deref(struct tty_ldisc *);
-- 
1.9.1

[RFC 5/5] Bluetooth: hci_uart: Add BCM specific UART speed management

[Frederic Danis]

This allows to setup the final speed for UART communication.
This also allows to load firmware at final speed.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 111 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 7cec648..7501732 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -11,6 +11,7 @@
 */
 
 #include <linux/kernel.h>
+#include <linux/tty.h>
 #include <linux/errno.h>
 #include <linux/skbuff.h>
 #include <linux/firmware.h>
@@ -20,6 +21,9 @@
 
 #include "hci_uart.h"
 
+#define BCM43XX_CLOCK_48 1
+#define BCM43XX_CLOCK_24 2
+
 static int hci_bcm_reset(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
@@ -113,6 +117,87 @@ static int hci_bcm_read_local_version(struct hci_uart *hu, char *name,
 	return 0;
 }
 
+static int hci_bcm_set_clock(struct hci_uart *hu, unsigned char clock)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+
+	BT_DBG("%s: Set Controller clock (%d)", hdev->name, clock);
+
+	skb = __hci_cmd_sync(hdev, 0xfc45, 1, &clock, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		BT_ERR("%s: failed to write update clock command (%ld)",
+		       hdev->name, PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	if (skb->data[0]) {
+		u8 evt_status = skb->data[0];
+
+		BT_ERR("%s: write update clock event failed (%02x)",
+		       hdev->name, evt_status);
+		kfree_skb(skb);
+		return -bt_to_errno(evt_status);
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int hci_bcm_set_speed(struct hci_uart *hu, uint32_t speed)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+	unsigned char param[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+	struct ktermios ktermios;
+
+	if (speed > 3000000 && hci_bcm_set_clock(hu, BCM43XX_CLOCK_48))
+		return -EINVAL;
+
+	param[2] = (uint8_t) (speed);
+	param[3] = (uint8_t) (speed >> 8);
+	param[4] = (uint8_t) (speed >> 16);
+	param[5] = (uint8_t) (speed >> 24);
+
+	BT_DBG("%s: Set Controller UART speed to %d bit/s", hdev->name, speed);
+
+	skb = __hci_cmd_sync(hdev, 0xfc18, sizeof(param), param,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		BT_ERR("%s: failed to write update baudrate command (%ld)",
+		       hdev->name, PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	if (skb->data[0]) {
+		u8 evt_status = skb->data[0];
+
+		BT_ERR("%s: write update baudrate event failed (%02x)",
+		       hdev->name, evt_status);
+		kfree_skb(skb);
+		return -bt_to_errno(evt_status);
+	}
+
+	kfree_skb(skb);
+
+	if (!hu->tty) {
+		BT_ERR("%s: Can't set host baud rate", hdev->name);
+		return -EIO;
+	}
+
+	ktermios = hu->tty->termios;
+	ktermios.c_cflag &= ~CBAUD;
+	ktermios.c_cflag |= BOTHER;
+	tty_termios_encode_baud_rate(&ktermios, speed, speed);
+
+	tty_set_termios(hu->tty, &ktermios);
+
+	BT_DBG("%s: New tty speed: %d", hdev->name, hu->tty->termios.c_ispeed);
+
+	return 0;
+}
+
 static const struct firmware *hci_bcm_get_fw(struct hci_uart *hu,
 						const char *fwname)
 {
@@ -223,15 +308,41 @@ int hci_bcm_setup(struct hci_uart *hu)
 
 	fw = hci_bcm_get_fw(hu, fwname);
 	if (fw) {
+		struct ktermios ktermios;
+		speed_t init_speed = hu->tty->termios.c_ispeed;
+
+		BT_DBG("%s: tty speed: %d",
+		       hu->hdev->name, hu->tty->termios.c_ispeed);
+
+		if (hu->speed) {
+			err = hci_bcm_set_speed(hu, hu->speed);
+			if (err)
+				goto failed;
+		}
+
 		err = hci_bcm_load_firmware(hu, fw);
 		if (err)
 			goto failed;
 
+		/* Controller speed has been reset to default speed */
+		if (hu->speed) {
+			ktermios = hu->tty->termios;
+			tty_termios_encode_baud_rate(&ktermios, init_speed,
+						     init_speed);
+			tty_set_termios(hu->tty, &ktermios);
+
+			BT_DBG("%s: New tty speed: %d",
+			       hu->hdev->name, hu->tty->termios.c_ispeed);
+		}
+
 		err = hci_bcm_reset(hu);
 		if (err)
 			goto failed;
 	}
 
+	if (hu->speed)
+		err = hci_bcm_set_speed(hu, hu->speed);
+
 failed:
 	if (fw)
 		release_firmware(fw);
-- 
1.9.1

Re: [RFC 1/5] Bluetooth: hci_uart: Add HCIUARTSETDEVTYPE ioctl

[Marcel Holtmann]

Hi Fred,

> This allows user space application to set the correct vendor specific
> setup function.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 50 +++++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_uart.h  |  4 ++++
> 2 files changed, 54 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 1363dc6..cb7fcc6 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -467,9 +467,19 @@ static int hci_uart_register_dev(struct hci_uart *hu)
> 	return 0;
> }
> 
> +struct hci_uart_devtype_t {
> +	char type[HCI_UART_DEVTYPE_SIZE];
> +	int (*setup)(struct hci_uart *hu);
> +};
> +
> +struct hci_uart_devtype_t devtypes[] = {
> +		{ "", 0 }
> +};
> +

I am actually not in favor of this and I do not think this is needed.

If you want a different hdev->setup function, then you need to implement a new protocol. We already have the HCIUARTSETPROTO ioctl and that should be the main selection point on which driver/vendor is attached to the TTY.

Lets focus on just using HCIUARTSETPROTO for now and when needed just introduce a new driver/vendor in case we want to add special setup code. We can have generic_* type of helpers in hci_ldisc.c for generic H:4 stuff in case it is just a H:4 driver. We can use select in Kconfig to ensure that hci_h4.c gets include when another driver/vendor needs it.

Regards

Marcel

Re: [RFC 4/5] tty: Re-add external interface for tty_set_termios()

[Marcel Holtmann]

Hi Fred,

> This is needed by Bluetooth hci_uart module to be able to change speed
> of Bluetooth controller and local UART.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/tty/tty_ioctl.c | 3 ++-
> include/linux/tty.h     | 1 +
> 2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index 632fc81..8e53fe4 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -536,7 +536,7 @@ EXPORT_SYMBOL(tty_termios_hw_change);
>  *	Locking: termios_rwsem
>  */
> 
> -static int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
> +int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
> {
> 	struct ktermios old_termios;
> 	struct tty_ldisc *ld;
> @@ -569,6 +569,7 @@ static int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
> 	up_write(&tty->termios_rwsem);
> 	return 0;
> }
> +EXPORT_SYMBOL_GPL(tty_set_termios);

can you send this one to the Linux TTY mailing list first to get at least an Ack from Greg KH on this.

Regards

Marcel

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Marcel Holtmann]

Hi Fred,

> This allows user space application to set final speed requested for UART
> device. UART port is open at init speed by user space application.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 4 ++++
> drivers/bluetooth/hci_uart.h  | 2 ++
> 2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index e8412f8..4b09369 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -614,6 +614,10 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file * file,
> 	case HCIUARTGETFLAGS:
> 		return hu->hdev_flags;
> 
> +	case HCIUARTSETBAUDRATE:
> +		hu->speed = arg;
> +		break;
> +
> 	case HCIUARTSETDEVTYPE:
> 		err = hci_tty_ioctl_set_devtype(hu, cmd, arg);
> 		if (err)
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index bf6f0e5..dcbedaf 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -34,6 +34,7 @@
> #define HCIUARTSETFLAGS		_IOW('U', 203, int)
> #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> #define HCIUARTSETDEVTYPE	_IOW('U', 205, int)
> +#define HCIUARTSETBAUDRATE	_IOW('U', 206, int)

lets do a combination of SET and GET here. We want to be able readout the the baudrate configured as well.

The only question I have is we want to do some enumeration or the plain baud rate value. Any thoughts on this?

Regards

Marcel

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

On 04/02/2015 10:37 AM, Frederic Danis wrote:
> This allows user space application to set final speed requested for UART
> device. UART port is open at init speed by user space application.

If userspace opened the tty and knows this is a UART, why can it not
use the for-purpose tty ioctls to change the line rate?

Regards,
Peter Hurley

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Frederic Danis]

[ +cc GregKH, JiriS ]

On 03/04/2015 12:54, Peter Hurley wrote:
> On 04/02/2015 10:37 AM, Frederic Danis wrote:
>> This allows user space application to set final speed requested for UART
>> device. UART port is open at init speed by user space application.
>
> If userspace opened the tty and knows this is a UART, why can it not
> use the for-purpose tty ioctls to change the line rate?

The purpose of this Bluetooth set of patches is to move device setup for 
Bluetooth UART devices into the kernel.
The idea is that userspace only needs to open the tty at initial speed 
and set correct line discipline, then userspace has no more to do, its 
up to HCI UART driver to manage the device (including firmware loading, 
bluetooth address and device speed setup).

However the driver may want to modify device speed (by using vendor 
specific HCI commands) to operate at full speed. Moreover, in case of 
firmware loading, some devices reset their speed to the default one, 
which will imply to change UART host speed to be able to continue 
communication.

Userspace can not know when all those speed changes occur.

Regards

Fred

-- 
Frederic Danis                            Open Source Technology Center
frederic.danis@intel.com                              Intel Corporation

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

On 04/03/2015 08:49 AM, Frederic Danis wrote:
> [ +cc GregKH, JiriS ]
> 
> On 03/04/2015 12:54, Peter Hurley wrote:
>> On 04/02/2015 10:37 AM, Frederic Danis wrote:
>>> This allows user space application to set final speed requested for UART
>>> device. UART port is open at init speed by user space application.
>>
>> If userspace opened the tty and knows this is a UART, why can it not
>> use the for-purpose tty ioctls to change the line rate?
> 
> The purpose of this Bluetooth set of patches is to move device setup for Bluetooth UART devices into the kernel.
> The idea is that userspace only needs to open the tty at initial speed and set correct line discipline, then userspace has no more to do, its up to HCI UART driver to manage the device (including firmware loading, bluetooth address and device speed setup).
> 
> However the driver may want to modify device speed (by using vendor specific HCI commands) to operate at full speed. Moreover, in case of firmware loading, some devices reset their speed to the default one, which will imply to change UART host speed to be able to continue communication.
> 
> Userspace can not know when all those speed changes occur.

The line discipline is always notified of line rate changes
via the set_termios() method, so if you add that to the hci_ldisc,
you'll be able to keep the BT device in sync with your
vendor-specific commands.

I'd like to be able to help out with the firmware download problem
but it's not clear to me at what point that is being executed since
the patch that adds a setup() method to the protocol doesn't compile:

/home/peter/src/kernels/mainline/drivers/bluetooth/hci_ldisc.c: In function ‘hci_uart_set_proto’:
/home/peter/src/kernels/mainline/drivers/bluetooth/hci_ldisc.c:485:11: error: ‘struct hci_uart_proto’ has no member named ‘setup’
  hu->proto->setup = NULL;
           ^
/home/peter/src/kernels/mainline/drivers/bluetooth/hci_ldisc.c:488:13: error: ‘struct hci_uart_proto’ has no member named ‘setup’
    hu->proto->setup = devtypes[i].setup;
             ^
make[3]: *** [drivers/bluetooth/hci_ldisc.o] Error 1

Regards,
Peter Hurley

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

On 04/03/2015 09:45 AM, Peter Hurley wrote:
> I'd like to be able to help out with the firmware download problem
> but it's not clear to me at what point that is being executed since
> the patch that adds a setup() method to the protocol doesn't compile:
> 
> /home/peter/src/kernels/mainline/drivers/bluetooth/hci_ldisc.c: In function ‘hci_uart_set_proto’:
> /home/peter/src/kernels/mainline/drivers/bluetooth/hci_ldisc.c:485:11: error: ‘struct hci_uart_proto’ has no member named ‘setup’
>   hu->proto->setup = NULL;
>            ^
> /home/peter/src/kernels/mainline/drivers/bluetooth/hci_ldisc.c:488:13: error: ‘struct hci_uart_proto’ has no member named ‘setup’
>     hu->proto->setup = devtypes[i].setup;
>              ^
> make[3]: *** [drivers/bluetooth/hci_ldisc.o] Error 1

Ah, I see it's in -next. Ok, I'll go look at that.

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Loic Poulain]

Hi Peter,

On 03/04/2015 15:45, Peter Hurley wrote:
> The line discipline is always notified of line rate changes
> via the set_termios() method, so if you add that to the hci_ldisc,
> you'll be able to keep the BT device in sync with your
> vendor-specific commands.
If I'm not wrong, line discipline is notified after the tty termios change.
So, it's too late to send the  HCI change speed command.
Moreover, user space is not aware of the device behavior, the driver should
be the only one to manage the device and its serial link.
Here, user space is just a bootstrap which hands over to the driver.

Regards,
Loic

-- 
Intel Open Source Technology Center
http://oss.intel.com/

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

Hi Loic,

On 04/03/2015 10:18 AM, Loic Poulain wrote:
> Hi Peter,
> 
> On 03/04/2015 15:45, Peter Hurley wrote:
>> The line discipline is always notified of line rate changes
>> via the set_termios() method, so if you add that to the hci_ldisc,
>> you'll be able to keep the BT device in sync with your
>> vendor-specific commands.
> If I'm not wrong, line discipline is notified after the tty termios change.
> So, it's too late to send the  HCI change speed command.
> Moreover, user space is not aware of the device behavior, the driver should
> be the only one to manage the device and its serial link.
> Here, user space is just a bootstrap which hands over to the driver.

You realize that you're telling me that userspace is unaware of
this in a thread that begins with a proposed userspace ioctl change to
the line discipline? An ioctl change that adds a new ioctl to set the
speed in a line discipline? For which there are already lots of ioctls
to set the line speed?

Setting aside the problem with the firmware load, I see no reason why
this can't be done within the existing line discipline framework.

Regards,
Peter Hurley

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Marcel Holtmann]

Hi Peter,

>>> The line discipline is always notified of line rate changes
>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>> you'll be able to keep the BT device in sync with your
>>> vendor-specific commands.
>> If I'm not wrong, line discipline is notified after the tty termios change.
>> So, it's too late to send the  HCI change speed command.
>> Moreover, user space is not aware of the device behavior, the driver should
>> be the only one to manage the device and its serial link.
>> Here, user space is just a bootstrap which hands over to the driver.
> 
> You realize that you're telling me that userspace is unaware of
> this in a thread that begins with a proposed userspace ioctl change to
> the line discipline? An ioctl change that adds a new ioctl to set the
> speed in a line discipline? For which there are already lots of ioctls
> to set the line speed?

actually the problem is a little bit different here. Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.

The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.

This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.

So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.

> Setting aside the problem with the firmware load, I see no reason why
> this can't be done within the existing line discipline framework.

So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.

In summary what we have to do these days is this:

- Send Reset
- Send a few basic HCI commands
- Send baud rate change command
- Change actual baud rate on the TTY
- Download firmware
- Send Reset to boot the firmware
- Change actual baud rate back to default
- Send baud rate change command
- Change actual baud rate
- Send Reset
- Standard init sequence

Now tell me how the line discipline framework helps us here? Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.

Regards

Marcel

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

Hi Marcel,

On 04/03/2015 01:39 PM, Marcel Holtmann wrote:
> Hi Peter,
> 
>>>> The line discipline is always notified of line rate changes
>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>> you'll be able to keep the BT device in sync with your
>>>> vendor-specific commands.
>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>> So, it's too late to send the  HCI change speed command.
>>> Moreover, user space is not aware of the device behavior, the driver should
>>> be the only one to manage the device and its serial link.
>>> Here, user space is just a bootstrap which hands over to the driver.
>>
>> You realize that you're telling me that userspace is unaware of
>> this in a thread that begins with a proposed userspace ioctl change to
>> the line discipline? An ioctl change that adds a new ioctl to set the
>> speed in a line discipline? For which there are already lots of ioctls
>> to set the line speed?
> 
> actually the problem is a little bit different here.

Thanks for taking the time to outline the operation of hciattach. It's
been a while since I was involved in the BT userspace code so I hadn't
realized how much hciattach had morphed.

> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
> 
> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
> 
> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
> 
> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
> 
>> Setting aside the problem with the firmware load, I see no reason why
>> this can't be done within the existing line discipline framework.
> 
> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
> 
> In summary what we have to do these days is this:
> 
> - Send Reset
> - Send a few basic HCI commands
> - Send baud rate change command
> - Change actual baud rate on the TTY
> - Download firmware
> - Send Reset to boot the firmware
> - Change actual baud rate back to default
> - Send baud rate change command
> - Change actual baud rate
> - Send Reset
> - Standard init sequence
> 
> Now tell me how the line discipline framework helps us here?

Well,
1. in hciattach I would have set the N_HCI line discipline immediately
   after opening the uart device.
2. in the N_HCI line discipline, I would have implemented write() so
   userspace command write()s would still write through to the uart
   device.
3. either,
   a. I would have added a new line discipline method for pre-termios changes, or
   b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.

   and handled sending target line speed change commands there.
   Userspace would simply change the line speed.

Userspace firmware loads are straightforward.

However, it seems that the new intel approach (tied in with the protocol)
is trickier to accommodate, but I haven't spent a lot of time trying to follow
that setup.

I'm not sure I understand why the firmware load is tied to the protocol;
can the intel part load a different firmware that implements a different protocol?


> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.

I'm conscious of the limitations of using tty/serial in the current manner.

A fun project for one of these vendors would be to split the serial core
into a separate tty driver and an abstraction layer that would allow other
kernel subsystems to enslave the port.

Regards,
Peter Hurley

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Marcel Holtmann]

Hi Peter,

>>>>> The line discipline is always notified of line rate changes
>>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>>> you'll be able to keep the BT device in sync with your
>>>>> vendor-specific commands.
>>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>>> So, it's too late to send the  HCI change speed command.
>>>> Moreover, user space is not aware of the device behavior, the driver should
>>>> be the only one to manage the device and its serial link.
>>>> Here, user space is just a bootstrap which hands over to the driver.
>>> 
>>> You realize that you're telling me that userspace is unaware of
>>> this in a thread that begins with a proposed userspace ioctl change to
>>> the line discipline? An ioctl change that adds a new ioctl to set the
>>> speed in a line discipline? For which there are already lots of ioctls
>>> to set the line speed?
>> 
>> actually the problem is a little bit different here.
> 
> Thanks for taking the time to outline the operation of hciattach. It's
> been a while since I was involved in the BT userspace code so I hadn't
> realized how much hciattach had morphed.
> 
>> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
>> 
>> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
>> 
>> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
>> 
>> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
>> 
>>> Setting aside the problem with the firmware load, I see no reason why
>>> this can't be done within the existing line discipline framework.
>> 
>> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
>> 
>> In summary what we have to do these days is this:
>> 
>> - Send Reset
>> - Send a few basic HCI commands
>> - Send baud rate change command
>> - Change actual baud rate on the TTY
>> - Download firmware
>> - Send Reset to boot the firmware
>> - Change actual baud rate back to default
>> - Send baud rate change command
>> - Change actual baud rate
>> - Send Reset
>> - Standard init sequence
>> 
>> Now tell me how the line discipline framework helps us here?
> 
> Well,
> 1. in hciattach I would have set the N_HCI line discipline immediately
>   after opening the uart device.
> 2. in the N_HCI line discipline, I would have implemented write() so
>   userspace command write()s would still write through to the uart
>   device.
> 3. either,
>   a. I would have added a new line discipline method for pre-termios changes, or
>   b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.
> 
>   and handled sending target line speed change commands there.
>   Userspace would simply change the line speed.
> 
> Userspace firmware loads are straightforward.
> 
> However, it seems that the new intel approach (tied in with the protocol)
> is trickier to accommodate, but I haven't spent a lot of time trying to follow
> that setup.
> 
> I'm not sure I understand why the firmware load is tied to the protocol;
> can the intel part load a different firmware that implements a different protocol?

the firmware loading procedure for USB and UART is actually the same. That applies to Broadcom and Intel controllers for sure and most likely also for other vendors. One goal is to share the firmware loading code between USB and UART based SKUs of these controllers. Where are not there yet, but that is where this is heading. So while we can hack around certain things in userspace, we really do not want to do that since it just means duplicated code.

Implementing the write() is a bad idea in my opinion. That is like injecting HCI commands behind your back. All sorts of messy things can happen if you allow that. And that is why this is actually not even implemented at the moment. Remember that HCI protocol comes with flow control for their commands and events. Injecting something without taking care of the command flow control means you are doing evil stuff. The firmware loading is using HCI commands and thus it should go through the high-level Bluetooth core that understands HCI commands.

Our idea is essentially that we set the N_HCI line discipline and then set the vendor protocol. That is it. From that point on it is the kernels problem to handle the bringup of the Bluetooth device. Including firmware download, address configuration and so on. Before setting N_HCI we would correctly configure the default baudrate and all other needed settings. However that is as much as we do from userspace.

For this to work, we need to be able to tell the TTY to change its baudrate. And this has to happen at least 3 times now. Higher baudrate before downloading the firmware, back to default baudrate after that, switch to higher baudrate and only then we are at normal operation.

So how does intercepting the termios ioctl will help us in this regard. The kernel needs to control the baudrate change and not userspace.

>> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.
> 
> I'm conscious of the limitations of using tty/serial in the current manner.
> 
> A fun project for one of these vendors would be to split the serial core
> into a separate tty driver and an abstraction layer that would allow other
> kernel subsystems to enslave the port.

They are proposing TTY slave devices and maybe that will help eventually, but finding agreement on that seems hard as well. And that is also mostly for handling the extra GPIO configuration for power up/down the Bluetooth chip behind the TTY and not the TTY itself.

I would love if we could just expose these TTYs as bi-directional pipes on a "serial bus" and just have a driver attach to it instead of going through a line discipline.

Regards

Marcel

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

On 04/04/2015 07:35 PM, Marcel Holtmann wrote:
> Hi Peter,
> 
>>>>>> The line discipline is always notified of line rate changes
>>>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>>>> you'll be able to keep the BT device in sync with your
>>>>>> vendor-specific commands.
>>>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>>>> So, it's too late to send the  HCI change speed command.
>>>>> Moreover, user space is not aware of the device behavior, the driver should
>>>>> be the only one to manage the device and its serial link.
>>>>> Here, user space is just a bootstrap which hands over to the driver.
>>>>
>>>> You realize that you're telling me that userspace is unaware of
>>>> this in a thread that begins with a proposed userspace ioctl change to
>>>> the line discipline? An ioctl change that adds a new ioctl to set the
>>>> speed in a line discipline? For which there are already lots of ioctls
>>>> to set the line speed?
>>>
>>> actually the problem is a little bit different here.
>>
>> Thanks for taking the time to outline the operation of hciattach. It's
>> been a while since I was involved in the BT userspace code so I hadn't
>> realized how much hciattach had morphed.
>>
>>> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
>>>
>>> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
>>>
>>> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
>>>
>>> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
>>>
>>>> Setting aside the problem with the firmware load, I see no reason why
>>>> this can't be done within the existing line discipline framework.
>>>
>>> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
>>>
>>> In summary what we have to do these days is this:
>>>
>>> - Send Reset
>>> - Send a few basic HCI commands
>>> - Send baud rate change command
>>> - Change actual baud rate on the TTY
>>> - Download firmware
>>> - Send Reset to boot the firmware
>>> - Change actual baud rate back to default
>>> - Send baud rate change command
>>> - Change actual baud rate
>>> - Send Reset
>>> - Standard init sequence
>>>
>>> Now tell me how the line discipline framework helps us here?
>>
>> Well,
>> 1. in hciattach I would have set the N_HCI line discipline immediately
>>   after opening the uart device.
>> 2. in the N_HCI line discipline, I would have implemented write() so
>>   userspace command write()s would still write through to the uart
>>   device.
>> 3. either,
>>   a. I would have added a new line discipline method for pre-termios changes, or
>>   b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.
>>
>>   and handled sending target line speed change commands there.
>>   Userspace would simply change the line speed.
>>
>> Userspace firmware loads are straightforward.
>>
>> However, it seems that the new intel approach (tied in with the protocol)
>> is trickier to accommodate, but I haven't spent a lot of time trying to follow
>> that setup.
>>
>> I'm not sure I understand why the firmware load is tied to the protocol;
>> can the intel part load a different firmware that implements a different protocol?
> 
> the firmware loading procedure for USB and UART is actually the same. That applies to Broadcom and Intel controllers for sure and most likely also for other vendors. One goal is to share the firmware loading code between USB and UART based SKUs of these controllers. Where are not there yet, but that is where this is heading. So while we can hack around certain things in userspace, we really do not want to do that since it just means duplicated code.
> 
> Implementing the write() is a bad idea in my opinion. That is like injecting HCI commands behind your back. All sorts of messy things can happen if you allow that. And that is why this is actually not even implemented at the moment. Remember that HCI protocol comes with flow control for their commands and events. Injecting something without taking care of the command flow control means you are doing evil stuff. The firmware loading is using HCI commands and thus it should go through the high-level Bluetooth core that understands HCI commands.

hciattach is doing lots of write(fd) right now; those raw serial writes would simply be
going though N_HCI line discipline write() instead of the N_TTY line discipline write().


> Our idea is essentially that we set the N_HCI line discipline and then set the vendor protocol. That is it. From that point on it is the kernels problem to handle the bringup of the Bluetooth device. Including firmware download, address configuration and so on. Before setting N_HCI we would correctly configure the default baudrate and all other needed settings. However that is as much as we do from userspace.
> 
> For this to work, we need to be able to tell the TTY to change its baudrate. And this has to happen at least 3 times now. Higher baudrate before downloading the firmware, back to default baudrate after that, switch to higher baudrate and only then we are at normal operation.
> 
> So how does intercepting the termios ioctl will help us in this regard. The kernel needs to control the baudrate change and not userspace.

The tty is a userspace object with a userspace api, simple as that.
That's why hciattach has to stay alive, because it has to own the tty file
reference.

Anything directly to tty from kernel is a hack that is missing locks and
bypassing state.


>>> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.
>>
>> I'm conscious of the limitations of using tty/serial in the current manner.
>>
>> A fun project for one of these vendors would be to split the serial core
>> into a separate tty driver and an abstraction layer that would allow other
>> kernel subsystems to enslave the port.
> 
> They are proposing TTY slave devices and maybe that will help eventually, but finding agreement on that seems hard as well. And that is also mostly for handling the extra GPIO configuration for power up/down the Bluetooth chip behind the TTY and not the TTY itself.

That's different.
 
> I would love if we could just expose these TTYs as bi-directional pipes on a "serial bus" and just have a driver attach to it instead of going through a line discipline.

That's what I'm saying.

Ultimately, the stuff you want to implement on the BT side will need a native
kernel interface where you own the device reference.

Regards,
Peter Hurley

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Marcel Holtmann]

Hi Peter,

>>>>>>> The line discipline is always notified of line rate changes
>>>>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>>>>> you'll be able to keep the BT device in sync with your
>>>>>>> vendor-specific commands.
>>>>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>>>>> So, it's too late to send the  HCI change speed command.
>>>>>> Moreover, user space is not aware of the device behavior, the driver should
>>>>>> be the only one to manage the device and its serial link.
>>>>>> Here, user space is just a bootstrap which hands over to the driver.
>>>>> 
>>>>> You realize that you're telling me that userspace is unaware of
>>>>> this in a thread that begins with a proposed userspace ioctl change to
>>>>> the line discipline? An ioctl change that adds a new ioctl to set the
>>>>> speed in a line discipline? For which there are already lots of ioctls
>>>>> to set the line speed?
>>>> 
>>>> actually the problem is a little bit different here.
>>> 
>>> Thanks for taking the time to outline the operation of hciattach. It's
>>> been a while since I was involved in the BT userspace code so I hadn't
>>> realized how much hciattach had morphed.
>>> 
>>>> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
>>>> 
>>>> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
>>>> 
>>>> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
>>>> 
>>>> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
>>>> 
>>>>> Setting aside the problem with the firmware load, I see no reason why
>>>>> this can't be done within the existing line discipline framework.
>>>> 
>>>> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
>>>> 
>>>> In summary what we have to do these days is this:
>>>> 
>>>> - Send Reset
>>>> - Send a few basic HCI commands
>>>> - Send baud rate change command
>>>> - Change actual baud rate on the TTY
>>>> - Download firmware
>>>> - Send Reset to boot the firmware
>>>> - Change actual baud rate back to default
>>>> - Send baud rate change command
>>>> - Change actual baud rate
>>>> - Send Reset
>>>> - Standard init sequence
>>>> 
>>>> Now tell me how the line discipline framework helps us here?
>>> 
>>> Well,
>>> 1. in hciattach I would have set the N_HCI line discipline immediately
>>>  after opening the uart device.
>>> 2. in the N_HCI line discipline, I would have implemented write() so
>>>  userspace command write()s would still write through to the uart
>>>  device.
>>> 3. either,
>>>  a. I would have added a new line discipline method for pre-termios changes, or
>>>  b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.
>>> 
>>>  and handled sending target line speed change commands there.
>>>  Userspace would simply change the line speed.
>>> 
>>> Userspace firmware loads are straightforward.
>>> 
>>> However, it seems that the new intel approach (tied in with the protocol)
>>> is trickier to accommodate, but I haven't spent a lot of time trying to follow
>>> that setup.
>>> 
>>> I'm not sure I understand why the firmware load is tied to the protocol;
>>> can the intel part load a different firmware that implements a different protocol?
>> 
>> the firmware loading procedure for USB and UART is actually the same. That applies to Broadcom and Intel controllers for sure and most likely also for other vendors. One goal is to share the firmware loading code between USB and UART based SKUs of these controllers. Where are not there yet, but that is where this is heading. So while we can hack around certain things in userspace, we really do not want to do that since it just means duplicated code.
>> 
>> Implementing the write() is a bad idea in my opinion. That is like injecting HCI commands behind your back. All sorts of messy things can happen if you allow that. And that is why this is actually not even implemented at the moment. Remember that HCI protocol comes with flow control for their commands and events. Injecting something without taking care of the command flow control means you are doing evil stuff. The firmware loading is using HCI commands and thus it should go through the high-level Bluetooth core that understands HCI commands.
> 
> hciattach is doing lots of write(fd) right now; those raw serial writes would simply be
> going though N_HCI line discipline write() instead of the N_TTY line discipline write().

and we want to get rid of these since it implements the HCI protocol layer. And it implements it pretty poorly. It is mainly write(), fingers-crossed, hope you get the right bytes back.

It works for dead simple protocols like H:5, but for BCSP, 3-Wire or the vendor specific power management, this is all not going to work out. The problem area arises when you have to recover from an error. All of this falls flat on its face.

>> Our idea is essentially that we set the N_HCI line discipline and then set the vendor protocol. That is it. From that point on it is the kernels problem to handle the bringup of the Bluetooth device. Including firmware download, address configuration and so on. Before setting N_HCI we would correctly configure the default baudrate and all other needed settings. However that is as much as we do from userspace.
>> 
>> For this to work, we need to be able to tell the TTY to change its baudrate. And this has to happen at least 3 times now. Higher baudrate before downloading the firmware, back to default baudrate after that, switch to higher baudrate and only then we are at normal operation.
>> 
>> So how does intercepting the termios ioctl will help us in this regard. The kernel needs to control the baudrate change and not userspace.
> 
> The tty is a userspace object with a userspace api, simple as that.
> That's why hciattach has to stay alive, because it has to own the tty file
> reference.
> 
> Anything directly to tty from kernel is a hack that is missing locks and
> bypassing state.

So what you are saying is that we want to have a callback mechanism back into hciattach. So when kernel needs a changed baudrate, it should tell hciattach, it will then change the baudrate, and tell the kernel when it did so.

I wonder if I could just repurpose the ldisc read/write/poll callback handlers to implement such a simple callback/notification protocol. That would actually make sense to me since the kernel wants to drive the TTY with the agreed upon HCI transport protocol from the point we attached N_HCI to it.

>>>> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.
>>> 
>>> I'm conscious of the limitations of using tty/serial in the current manner.
>>> 
>>> A fun project for one of these vendors would be to split the serial core
>>> into a separate tty driver and an abstraction layer that would allow other
>>> kernel subsystems to enslave the port.
>> 
>> They are proposing TTY slave devices and maybe that will help eventually, but finding agreement on that seems hard as well. And that is also mostly for handling the extra GPIO configuration for power up/down the Bluetooth chip behind the TTY and not the TTY itself.
> 
> That's different.
> 
>> I would love if we could just expose these TTYs as bi-directional pipes on a "serial bus" and just have a driver attach to it instead of going through a line discipline.
> 
> That's what I'm saying.
> 
> Ultimately, the stuff you want to implement on the BT side will need a native
> kernel interface where you own the device reference.

Even the HSU on our devices expose a TTY instead of an actual HSU bus where we could have be a HSU driver on it. If you know how we can get this changed, I am all ears.

The hci_uart driver and N_HCI line discipline will be still needed since there are actual development boards with USB/TTY converters attached to it. However all the embedded devices where it is all hardwired, proper exposed busses or platform devices would be a lot better.

Regards

Marcel

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

On 04/04/2015 10:29 PM, Marcel Holtmann wrote:
> Hi Peter,
> 
>>>>>>>> The line discipline is always notified of line rate changes
>>>>>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>>>>>> you'll be able to keep the BT device in sync with your
>>>>>>>> vendor-specific commands.
>>>>>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>>>>>> So, it's too late to send the  HCI change speed command.
>>>>>>> Moreover, user space is not aware of the device behavior, the driver should
>>>>>>> be the only one to manage the device and its serial link.
>>>>>>> Here, user space is just a bootstrap which hands over to the driver.
>>>>>>
>>>>>> You realize that you're telling me that userspace is unaware of
>>>>>> this in a thread that begins with a proposed userspace ioctl change to
>>>>>> the line discipline? An ioctl change that adds a new ioctl to set the
>>>>>> speed in a line discipline? For which there are already lots of ioctls
>>>>>> to set the line speed?
>>>>>
>>>>> actually the problem is a little bit different here.
>>>>
>>>> Thanks for taking the time to outline the operation of hciattach. It's
>>>> been a while since I was involved in the BT userspace code so I hadn't
>>>> realized how much hciattach had morphed.
>>>>
>>>>> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
>>>>>
>>>>> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
>>>>>
>>>>> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
>>>>>
>>>>> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
>>>>>
>>>>>> Setting aside the problem with the firmware load, I see no reason why
>>>>>> this can't be done within the existing line discipline framework.
>>>>>
>>>>> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
>>>>>
>>>>> In summary what we have to do these days is this:
>>>>>
>>>>> - Send Reset
>>>>> - Send a few basic HCI commands
>>>>> - Send baud rate change command
>>>>> - Change actual baud rate on the TTY
>>>>> - Download firmware
>>>>> - Send Reset to boot the firmware
>>>>> - Change actual baud rate back to default
>>>>> - Send baud rate change command
>>>>> - Change actual baud rate
>>>>> - Send Reset
>>>>> - Standard init sequence
>>>>>
>>>>> Now tell me how the line discipline framework helps us here?
>>>>
>>>> Well,
>>>> 1. in hciattach I would have set the N_HCI line discipline immediately
>>>>  after opening the uart device.
>>>> 2. in the N_HCI line discipline, I would have implemented write() so
>>>>  userspace command write()s would still write through to the uart
>>>>  device.
>>>> 3. either,
>>>>  a. I would have added a new line discipline method for pre-termios changes, or
>>>>  b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.
>>>>
>>>>  and handled sending target line speed change commands there.
>>>>  Userspace would simply change the line speed.
>>>>
>>>> Userspace firmware loads are straightforward.
>>>>
>>>> However, it seems that the new intel approach (tied in with the protocol)
>>>> is trickier to accommodate, but I haven't spent a lot of time trying to follow
>>>> that setup.
>>>>
>>>> I'm not sure I understand why the firmware load is tied to the protocol;
>>>> can the intel part load a different firmware that implements a different protocol?
>>>
>>> the firmware loading procedure for USB and UART is actually the same. That applies to Broadcom and Intel controllers for sure and most likely also for other vendors. One goal is to share the firmware loading code between USB and UART based SKUs of these controllers. Where are not there yet, but that is where this is heading. So while we can hack around certain things in userspace, we really do not want to do that since it just means duplicated code.
>>>
>>> Implementing the write() is a bad idea in my opinion. That is like injecting HCI commands behind your back. All sorts of messy things can happen if you allow that. And that is why this is actually not even implemented at the moment. Remember that HCI protocol comes with flow control for their commands and events. Injecting something without taking care of the command flow control means you are doing evil stuff. The firmware loading is using HCI commands and thus it should go through the high-level Bluetooth core that understands HCI commands.
>>
>> hciattach is doing lots of write(fd) right now; those raw serial writes would simply be
>> going though N_HCI line discipline write() instead of the N_TTY line discipline write().
> 
> and we want to get rid of these since it implements the HCI protocol layer. And it implements it pretty poorly. It is mainly write(), fingers-crossed, hope you get the right bytes back.
> 
> It works for dead simple protocols like H:5, but for BCSP, 3-Wire or the vendor specific power management, this is all not going to work out. The problem area arises when you have to recover from an error. All of this falls flat on its face.

Ok, what code should I look at to understand your in-kernel interface
requirements?

Is it at least from process context?

>>> Our idea is essentially that we set the N_HCI line discipline and then set the vendor protocol. That is it. From that point on it is the kernels problem to handle the bringup of the Bluetooth device. Including firmware download, address configuration and so on. Before setting N_HCI we would correctly configure the default baudrate and all other needed settings. However that is as much as we do from userspace.
>>>
>>> For this to work, we need to be able to tell the TTY to change its baudrate. And this has to happen at least 3 times now. Higher baudrate before downloading the firmware, back to default baudrate after that, switch to higher baudrate and only then we are at normal operation.
>>>
>>> So how does intercepting the termios ioctl will help us in this regard. The kernel needs to control the baudrate change and not userspace.
>>
>> The tty is a userspace object with a userspace api, simple as that.
>> That's why hciattach has to stay alive, because it has to own the tty file
>> reference.
>>
>> Anything directly to tty from kernel is a hack that is missing locks and
>> bypassing state.
> 
> So what you are saying is that we want to have a callback mechanism back into hciattach. So when kernel needs a changed baudrate, it should tell hciattach, it will then change the baudrate, and tell the kernel when it did so.
> 
> I wonder if I could just repurpose the ldisc read/write/poll callback handlers to implement such a simple callback/notification protocol. That would actually make sense to me since the kernel wants to drive the TTY with the agreed upon HCI transport protocol from the point we attached N_HCI to it.

Any of the above would be so ugly.
Until I can work up something better, the lesser evil is using
tty_set_termios().

Please cc me for patches using tty_set_termios() or other tty workarounds.

>>>>> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.
>>>>
>>>> I'm conscious of the limitations of using tty/serial in the current manner.
>>>>
>>>> A fun project for one of these vendors would be to split the serial core
>>>> into a separate tty driver and an abstraction layer that would allow other
>>>> kernel subsystems to enslave the port.
>>>
>>> They are proposing TTY slave devices and maybe that will help eventually, but finding agreement on that seems hard as well. And that is also mostly for handling the extra GPIO configuration for power up/down the Bluetooth chip behind the TTY and not the TTY itself.
>>
>> That's different.
>>
>>> I would love if we could just expose these TTYs as bi-directional pipes on a "serial bus" and just have a driver attach to it instead of going through a line discipline.
>>
>> That's what I'm saying.
>>
>> Ultimately, the stuff you want to implement on the BT side will need a native
>> kernel interface where you own the device reference.
> 
> Even the HSU on our devices expose a TTY instead of an actual HSU bus where we could have be a HSU driver on it. If you know how we can get this changed, I am all ears.

There's no magic bullet here; it would be a lot of work.

> The hci_uart driver and N_HCI line discipline will be still needed since there are actual development boards with USB/TTY converters attached to it. However all the embedded devices where it is all hardwired, proper exposed busses or platform devices would be a lot better.

But never both simultaneously on the same device, right?
Because the idea would be that the UART is exclusive and wholly owned, without
a userspace dev node.

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Marcel Holtmann]

Hi Peter,

>>>>>>>>> The line discipline is always notified of line rate changes
>>>>>>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>>>>>>> you'll be able to keep the BT device in sync with your
>>>>>>>>> vendor-specific commands.
>>>>>>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>>>>>>> So, it's too late to send the  HCI change speed command.
>>>>>>>> Moreover, user space is not aware of the device behavior, the driver should
>>>>>>>> be the only one to manage the device and its serial link.
>>>>>>>> Here, user space is just a bootstrap which hands over to the driver.
>>>>>>> 
>>>>>>> You realize that you're telling me that userspace is unaware of
>>>>>>> this in a thread that begins with a proposed userspace ioctl change to
>>>>>>> the line discipline? An ioctl change that adds a new ioctl to set the
>>>>>>> speed in a line discipline? For which there are already lots of ioctls
>>>>>>> to set the line speed?
>>>>>> 
>>>>>> actually the problem is a little bit different here.
>>>>> 
>>>>> Thanks for taking the time to outline the operation of hciattach. It's
>>>>> been a while since I was involved in the BT userspace code so I hadn't
>>>>> realized how much hciattach had morphed.
>>>>> 
>>>>>> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
>>>>>> 
>>>>>> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
>>>>>> 
>>>>>> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
>>>>>> 
>>>>>> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
>>>>>> 
>>>>>>> Setting aside the problem with the firmware load, I see no reason why
>>>>>>> this can't be done within the existing line discipline framework.
>>>>>> 
>>>>>> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
>>>>>> 
>>>>>> In summary what we have to do these days is this:
>>>>>> 
>>>>>> - Send Reset
>>>>>> - Send a few basic HCI commands
>>>>>> - Send baud rate change command
>>>>>> - Change actual baud rate on the TTY
>>>>>> - Download firmware
>>>>>> - Send Reset to boot the firmware
>>>>>> - Change actual baud rate back to default
>>>>>> - Send baud rate change command
>>>>>> - Change actual baud rate
>>>>>> - Send Reset
>>>>>> - Standard init sequence
>>>>>> 
>>>>>> Now tell me how the line discipline framework helps us here?
>>>>> 
>>>>> Well,
>>>>> 1. in hciattach I would have set the N_HCI line discipline immediately
>>>>> after opening the uart device.
>>>>> 2. in the N_HCI line discipline, I would have implemented write() so
>>>>> userspace command write()s would still write through to the uart
>>>>> device.
>>>>> 3. either,
>>>>> a. I would have added a new line discipline method for pre-termios changes, or
>>>>> b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.
>>>>> 
>>>>> and handled sending target line speed change commands there.
>>>>> Userspace would simply change the line speed.
>>>>> 
>>>>> Userspace firmware loads are straightforward.
>>>>> 
>>>>> However, it seems that the new intel approach (tied in with the protocol)
>>>>> is trickier to accommodate, but I haven't spent a lot of time trying to follow
>>>>> that setup.
>>>>> 
>>>>> I'm not sure I understand why the firmware load is tied to the protocol;
>>>>> can the intel part load a different firmware that implements a different protocol?
>>>> 
>>>> the firmware loading procedure for USB and UART is actually the same. That applies to Broadcom and Intel controllers for sure and most likely also for other vendors. One goal is to share the firmware loading code between USB and UART based SKUs of these controllers. Where are not there yet, but that is where this is heading. So while we can hack around certain things in userspace, we really do not want to do that since it just means duplicated code.
>>>> 
>>>> Implementing the write() is a bad idea in my opinion. That is like injecting HCI commands behind your back. All sorts of messy things can happen if you allow that. And that is why this is actually not even implemented at the moment. Remember that HCI protocol comes with flow control for their commands and events. Injecting something without taking care of the command flow control means you are doing evil stuff. The firmware loading is using HCI commands and thus it should go through the high-level Bluetooth core that understands HCI commands.
>>> 
>>> hciattach is doing lots of write(fd) right now; those raw serial writes would simply be
>>> going though N_HCI line discipline write() instead of the N_TTY line discipline write().
>> 
>> and we want to get rid of these since it implements the HCI protocol layer. And it implements it pretty poorly. It is mainly write(), fingers-crossed, hope you get the right bytes back.
>> 
>> It works for dead simple protocols like H:5, but for BCSP, 3-Wire or the vendor specific power management, this is all not going to work out. The problem area arises when you have to recover from an error. All of this falls flat on its face.
> 
> Ok, what code should I look at to understand your in-kernel interface
> requirements?

the chips are using mainly HCI vendor commands with in some cases extra packet types for power management. The kernel side is mainly that we have a special setup routing that drivers can use to program the controller. However the driver uses HCI commands via __hci_cmd_sync(). Good example from bluetooth-next might be drivers/bluetooth/btbcm.c since there we already split out the common pieces. Keep in mind that the HCI commands are in most cases the same no matter what the transport underneath is. UART just needs an extra baud rate change.

> Is it at least from process context?

The setup callback in executed from a workqueue.

>>>> Our idea is essentially that we set the N_HCI line discipline and then set the vendor protocol. That is it. From that point on it is the kernels problem to handle the bringup of the Bluetooth device. Including firmware download, address configuration and so on. Before setting N_HCI we would correctly configure the default baudrate and all other needed settings. However that is as much as we do from userspace.
>>>> 
>>>> For this to work, we need to be able to tell the TTY to change its baudrate. And this has to happen at least 3 times now. Higher baudrate before downloading the firmware, back to default baudrate after that, switch to higher baudrate and only then we are at normal operation.
>>>> 
>>>> So how does intercepting the termios ioctl will help us in this regard. The kernel needs to control the baudrate change and not userspace.
>>> 
>>> The tty is a userspace object with a userspace api, simple as that.
>>> That's why hciattach has to stay alive, because it has to own the tty file
>>> reference.
>>> 
>>> Anything directly to tty from kernel is a hack that is missing locks and
>>> bypassing state.
>> 
>> So what you are saying is that we want to have a callback mechanism back into hciattach. So when kernel needs a changed baudrate, it should tell hciattach, it will then change the baudrate, and tell the kernel when it did so.
>> 
>> I wonder if I could just repurpose the ldisc read/write/poll callback handlers to implement such a simple callback/notification protocol. That would actually make sense to me since the kernel wants to drive the TTY with the agreed upon HCI transport protocol from the point we attached N_HCI to it.
> 
> Any of the above would be so ugly.
> Until I can work up something better, the lesser evil is using
> tty_set_termios().
> 
> Please cc me for patches using tty_set_termios() or other tty workarounds.

I was proposing to use tty->ops->set_termios. Is there any benefit from calling the native ops compared to using the not yet exposed wrapper?

>>>>>> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.
>>>>> 
>>>>> I'm conscious of the limitations of using tty/serial in the current manner.
>>>>> 
>>>>> A fun project for one of these vendors would be to split the serial core
>>>>> into a separate tty driver and an abstraction layer that would allow other
>>>>> kernel subsystems to enslave the port.
>>>> 
>>>> They are proposing TTY slave devices and maybe that will help eventually, but finding agreement on that seems hard as well. And that is also mostly for handling the extra GPIO configuration for power up/down the Bluetooth chip behind the TTY and not the TTY itself.
>>> 
>>> That's different.
>>> 
>>>> I would love if we could just expose these TTYs as bi-directional pipes on a "serial bus" and just have a driver attach to it instead of going through a line discipline.
>>> 
>>> That's what I'm saying.
>>> 
>>> Ultimately, the stuff you want to implement on the BT side will need a native
>>> kernel interface where you own the device reference.
>> 
>> Even the HSU on our devices expose a TTY instead of an actual HSU bus where we could have be a HSU driver on it. If you know how we can get this changed, I am all ears.
> 
> There's no magic bullet here; it would be a lot of work.
> 
>> The hci_uart driver and N_HCI line discipline will be still needed since there are actual development boards with USB/TTY converters attached to it. However all the embedded devices where it is all hardwired, proper exposed busses or platform devices would be a lot better.
> 
> But never both simultaneously on the same device, right?
> Because the idea would be that the UART is exclusive and wholly owned, without
> a userspace dev node.

Yes. It is exclusive access to the device. No userspace device node needed.

So if we could just even set the line discipline from the kernel similar to attaching a driver to a device, then that would be something I would be interested as well.

Regards

Marcel

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

On 04/10/2015 12:07 PM, Marcel Holtmann wrote:
> Hi Peter,
> 
>>>>>>>>>> The line discipline is always notified of line rate changes
>>>>>>>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>>>>>>>> you'll be able to keep the BT device in sync with your
>>>>>>>>>> vendor-specific commands.
>>>>>>>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>>>>>>>> So, it's too late to send the  HCI change speed command.
>>>>>>>>> Moreover, user space is not aware of the device behavior, the driver should
>>>>>>>>> be the only one to manage the device and its serial link.
>>>>>>>>> Here, user space is just a bootstrap which hands over to the driver.
>>>>>>>>
>>>>>>>> You realize that you're telling me that userspace is unaware of
>>>>>>>> this in a thread that begins with a proposed userspace ioctl change to
>>>>>>>> the line discipline? An ioctl change that adds a new ioctl to set the
>>>>>>>> speed in a line discipline? For which there are already lots of ioctls
>>>>>>>> to set the line speed?
>>>>>>>
>>>>>>> actually the problem is a little bit different here.
>>>>>>
>>>>>> Thanks for taking the time to outline the operation of hciattach. It's
>>>>>> been a while since I was involved in the BT userspace code so I hadn't
>>>>>> realized how much hciattach had morphed.
>>>>>>
>>>>>>> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
>>>>>>>
>>>>>>> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
>>>>>>>
>>>>>>> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
>>>>>>>
>>>>>>> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
>>>>>>>
>>>>>>>> Setting aside the problem with the firmware load, I see no reason why
>>>>>>>> this can't be done within the existing line discipline framework.
>>>>>>>
>>>>>>> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
>>>>>>>
>>>>>>> In summary what we have to do these days is this:
>>>>>>>
>>>>>>> - Send Reset
>>>>>>> - Send a few basic HCI commands
>>>>>>> - Send baud rate change command
>>>>>>> - Change actual baud rate on the TTY
>>>>>>> - Download firmware
>>>>>>> - Send Reset to boot the firmware
>>>>>>> - Change actual baud rate back to default
>>>>>>> - Send baud rate change command
>>>>>>> - Change actual baud rate
>>>>>>> - Send Reset
>>>>>>> - Standard init sequence
>>>>>>>
>>>>>>> Now tell me how the line discipline framework helps us here?
>>>>>>
>>>>>> Well,
>>>>>> 1. in hciattach I would have set the N_HCI line discipline immediately
>>>>>> after opening the uart device.
>>>>>> 2. in the N_HCI line discipline, I would have implemented write() so
>>>>>> userspace command write()s would still write through to the uart
>>>>>> device.
>>>>>> 3. either,
>>>>>> a. I would have added a new line discipline method for pre-termios changes, or
>>>>>> b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.
>>>>>>
>>>>>> and handled sending target line speed change commands there.
>>>>>> Userspace would simply change the line speed.
>>>>>>
>>>>>> Userspace firmware loads are straightforward.
>>>>>>
>>>>>> However, it seems that the new intel approach (tied in with the protocol)
>>>>>> is trickier to accommodate, but I haven't spent a lot of time trying to follow
>>>>>> that setup.
>>>>>>
>>>>>> I'm not sure I understand why the firmware load is tied to the protocol;
>>>>>> can the intel part load a different firmware that implements a different protocol?
>>>>>
>>>>> the firmware loading procedure for USB and UART is actually the same. That applies to Broadcom and Intel controllers for sure and most likely also for other vendors. One goal is to share the firmware loading code between USB and UART based SKUs of these controllers. Where are not there yet, but that is where this is heading. So while we can hack around certain things in userspace, we really do not want to do that since it just means duplicated code.
>>>>>
>>>>> Implementing the write() is a bad idea in my opinion. That is like injecting HCI commands behind your back. All sorts of messy things can happen if you allow that. And that is why this is actually not even implemented at the moment. Remember that HCI protocol comes with flow control for their commands and events. Injecting something without taking care of the command flow control means you are doing evil stuff. The firmware loading is using HCI commands and thus it should go through the high-level Bluetooth core that understands HCI commands.
>>>>
>>>> hciattach is doing lots of write(fd) right now; those raw serial writes would simply be
>>>> going though N_HCI line discipline write() instead of the N_TTY line discipline write().
>>>
>>> and we want to get rid of these since it implements the HCI protocol layer. And it implements it pretty poorly. It is mainly write(), fingers-crossed, hope you get the right bytes back.
>>>
>>> It works for dead simple protocols like H:5, but for BCSP, 3-Wire or the vendor specific power management, this is all not going to work out. The problem area arises when you have to recover from an error. All of this falls flat on its face.
>>
>> Ok, what code should I look at to understand your in-kernel interface
>> requirements?
> 
> the chips are using mainly HCI vendor commands with in some cases extra packet types for power management. The kernel side is mainly that we have a special setup routing that drivers can use to program the controller. However the driver uses HCI commands via __hci_cmd_sync(). Good example from bluetooth-next might be drivers/bluetooth/btbcm.c since there we already split out the common pieces. Keep in mind that the HCI commands are in most cases the same no matter what the transport underneath is. UART just needs an extra baud rate change.
> 
>> Is it at least from process context?
> 
> The setup callback in executed from a workqueue.

Ok.

>>>>> Our idea is essentially that we set the N_HCI line discipline and then set the vendor protocol. That is it. From that point on it is the kernels problem to handle the bringup of the Bluetooth device. Including firmware download, address configuration and so on. Before setting N_HCI we would correctly configure the default baudrate and all other needed settings. However that is as much as we do from userspace.
>>>>>
>>>>> For this to work, we need to be able to tell the TTY to change its baudrate. And this has to happen at least 3 times now. Higher baudrate before downloading the firmware, back to default baudrate after that, switch to higher baudrate and only then we are at normal operation.
>>>>>
>>>>> So how does intercepting the termios ioctl will help us in this regard. The kernel needs to control the baudrate change and not userspace.
>>>>
>>>> The tty is a userspace object with a userspace api, simple as that.
>>>> That's why hciattach has to stay alive, because it has to own the tty file
>>>> reference.
>>>>
>>>> Anything directly to tty from kernel is a hack that is missing locks and
>>>> bypassing state.
>>>
>>> So what you are saying is that we want to have a callback mechanism back into hciattach. So when kernel needs a changed baudrate, it should tell hciattach, it will then change the baudrate, and tell the kernel when it did so.
>>>
>>> I wonder if I could just repurpose the ldisc read/write/poll callback handlers to implement such a simple callback/notification protocol. That would actually make sense to me since the kernel wants to drive the TTY with the agreed upon HCI transport protocol from the point we attached N_HCI to it.
>>
>> Any of the above would be so ugly.
>> Until I can work up something better, the lesser evil is using
>> tty_set_termios().
>>
>> Please cc me for patches using tty_set_termios() or other tty workarounds.
> 
> I was proposing to use tty->ops->set_termios. Is there any benefit from calling the native ops compared to using the not yet exposed wrapper?

tty_set_termios() performs the termios swap before calling the
driver; and grabs the proper lock.

As it is, the wrapper fn that calls tty_set_termios() will want to
perform a tty_wait_until_sent(tty, 0) before tty_set_termios();
otherwise, hci command bytes could still be in the xmit buffer
when the line settings are changed by the UART driver's set_termios().
[That doesn't happen automagically because there's times when the
line settings are commanded to change without waiting.]


>>>>>>> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.
>>>>>>
>>>>>> I'm conscious of the limitations of using tty/serial in the current manner.
>>>>>>
>>>>>> A fun project for one of these vendors would be to split the serial core
>>>>>> into a separate tty driver and an abstraction layer that would allow other
>>>>>> kernel subsystems to enslave the port.
>>>>>
>>>>> They are proposing TTY slave devices and maybe that will help eventually, but finding agreement on that seems hard as well. And that is also mostly for handling the extra GPIO configuration for power up/down the Bluetooth chip behind the TTY and not the TTY itself.
>>>>
>>>> That's different.
>>>>
>>>>> I would love if we could just expose these TTYs as bi-directional pipes on a "serial bus" and just have a driver attach to it instead of going through a line discipline.
>>>>
>>>> That's what I'm saying.
>>>>
>>>> Ultimately, the stuff you want to implement on the BT side will need a native
>>>> kernel interface where you own the device reference.
>>>
>>> Even the HSU on our devices expose a TTY instead of an actual HSU bus where we could have be a HSU driver on it. If you know how we can get this changed, I am all ears.
>>
>> There's no magic bullet here; it would be a lot of work.
>>
>>> The hci_uart driver and N_HCI line discipline will be still needed since there are actual development boards with USB/TTY converters attached to it. However all the embedded devices where it is all hardwired, proper exposed busses or platform devices would be a lot better.
>>
>> But never both simultaneously on the same device, right?
>> Because the idea would be that the UART is exclusive and wholly owned, without
>> a userspace dev node.
> 
> Yes. It is exclusive access to the device. No userspace device node needed.

Ok.

> So if we could just even set the line discipline from the kernel similar to attaching a driver to a device, then that would be something I would be interested as well.

That's probably doable right now; most of the heavy lifting of setting
the line discipline is in a single function. If this were being done from workqueue,
it'd have to be an unbounded queue because of the implicit module load of
the line discipline.

Regards,
Peter Hurley

Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Marcel Holtmann]

Hi Peter,

>>>>>>>>>>> The line discipline is always notified of line rate changes
>>>>>>>>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>>>>>>>>> you'll be able to keep the BT device in sync with your
>>>>>>>>>>> vendor-specific commands.
>>>>>>>>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>>>>>>>>> So, it's too late to send the  HCI change speed command.
>>>>>>>>>> Moreover, user space is not aware of the device behavior, the driver should
>>>>>>>>>> be the only one to manage the device and its serial link.
>>>>>>>>>> Here, user space is just a bootstrap which hands over to the driver.
>>>>>>>>> 
>>>>>>>>> You realize that you're telling me that userspace is unaware of
>>>>>>>>> this in a thread that begins with a proposed userspace ioctl change to
>>>>>>>>> the line discipline? An ioctl change that adds a new ioctl to set the
>>>>>>>>> speed in a line discipline? For which there are already lots of ioctls
>>>>>>>>> to set the line speed?
>>>>>>>> 
>>>>>>>> actually the problem is a little bit different here.
>>>>>>> 
>>>>>>> Thanks for taking the time to outline the operation of hciattach. It's
>>>>>>> been a while since I was involved in the BT userspace code so I hadn't
>>>>>>> realized how much hciattach had morphed.
>>>>>>> 
>>>>>>>> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
>>>>>>>> 
>>>>>>>> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
>>>>>>>> 
>>>>>>>> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
>>>>>>>> 
>>>>>>>> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
>>>>>>>> 
>>>>>>>>> Setting aside the problem with the firmware load, I see no reason why
>>>>>>>>> this can't be done within the existing line discipline framework.
>>>>>>>> 
>>>>>>>> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
>>>>>>>> 
>>>>>>>> In summary what we have to do these days is this:
>>>>>>>> 
>>>>>>>> - Send Reset
>>>>>>>> - Send a few basic HCI commands
>>>>>>>> - Send baud rate change command
>>>>>>>> - Change actual baud rate on the TTY
>>>>>>>> - Download firmware
>>>>>>>> - Send Reset to boot the firmware
>>>>>>>> - Change actual baud rate back to default
>>>>>>>> - Send baud rate change command
>>>>>>>> - Change actual baud rate
>>>>>>>> - Send Reset
>>>>>>>> - Standard init sequence
>>>>>>>> 
>>>>>>>> Now tell me how the line discipline framework helps us here?
>>>>>>> 
>>>>>>> Well,
>>>>>>> 1. in hciattach I would have set the N_HCI line discipline immediately
>>>>>>> after opening the uart device.
>>>>>>> 2. in the N_HCI line discipline, I would have implemented write() so
>>>>>>> userspace command write()s would still write through to the uart
>>>>>>> device.
>>>>>>> 3. either,
>>>>>>> a. I would have added a new line discipline method for pre-termios changes, or
>>>>>>> b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.
>>>>>>> 
>>>>>>> and handled sending target line speed change commands there.
>>>>>>> Userspace would simply change the line speed.
>>>>>>> 
>>>>>>> Userspace firmware loads are straightforward.
>>>>>>> 
>>>>>>> However, it seems that the new intel approach (tied in with the protocol)
>>>>>>> is trickier to accommodate, but I haven't spent a lot of time trying to follow
>>>>>>> that setup.
>>>>>>> 
>>>>>>> I'm not sure I understand why the firmware load is tied to the protocol;
>>>>>>> can the intel part load a different firmware that implements a different protocol?
>>>>>> 
>>>>>> the firmware loading procedure for USB and UART is actually the same. That applies to Broadcom and Intel controllers for sure and most likely also for other vendors. One goal is to share the firmware loading code between USB and UART based SKUs of these controllers. Where are not there yet, but that is where this is heading. So while we can hack around certain things in userspace, we really do not want to do that since it just means duplicated code.
>>>>>> 
>>>>>> Implementing the write() is a bad idea in my opinion. That is like injecting HCI commands behind your back. All sorts of messy things can happen if you allow that. And that is why this is actually not even implemented at the moment. Remember that HCI protocol comes with flow control for their commands and events. Injecting something without taking care of the command flow control means you are doing evil stuff. The firmware loading is using HCI commands and thus it should go through the high-level Bluetooth core that understands HCI commands.
>>>>> 
>>>>> hciattach is doing lots of write(fd) right now; those raw serial writes would simply be
>>>>> going though N_HCI line discipline write() instead of the N_TTY line discipline write().
>>>> 
>>>> and we want to get rid of these since it implements the HCI protocol layer. And it implements it pretty poorly. It is mainly write(), fingers-crossed, hope you get the right bytes back.
>>>> 
>>>> It works for dead simple protocols like H:5, but for BCSP, 3-Wire or the vendor specific power management, this is all not going to work out. The problem area arises when you have to recover from an error. All of this falls flat on its face.
>>> 
>>> Ok, what code should I look at to understand your in-kernel interface
>>> requirements?
>> 
>> the chips are using mainly HCI vendor commands with in some cases extra packet types for power management. The kernel side is mainly that we have a special setup routing that drivers can use to program the controller. However the driver uses HCI commands via __hci_cmd_sync(). Good example from bluetooth-next might be drivers/bluetooth/btbcm.c since there we already split out the common pieces. Keep in mind that the HCI commands are in most cases the same no matter what the transport underneath is. UART just needs an extra baud rate change.
>> 
>>> Is it at least from process context?
>> 
>> The setup callback in executed from a workqueue.
> 
> Ok.
> 
>>>>>> Our idea is essentially that we set the N_HCI line discipline and then set the vendor protocol. That is it. From that point on it is the kernels problem to handle the bringup of the Bluetooth device. Including firmware download, address configuration and so on. Before setting N_HCI we would correctly configure the default baudrate and all other needed settings. However that is as much as we do from userspace.
>>>>>> 
>>>>>> For this to work, we need to be able to tell the TTY to change its baudrate. And this has to happen at least 3 times now. Higher baudrate before downloading the firmware, back to default baudrate after that, switch to higher baudrate and only then we are at normal operation.
>>>>>> 
>>>>>> So how does intercepting the termios ioctl will help us in this regard. The kernel needs to control the baudrate change and not userspace.
>>>>> 
>>>>> The tty is a userspace object with a userspace api, simple as that.
>>>>> That's why hciattach has to stay alive, because it has to own the tty file
>>>>> reference.
>>>>> 
>>>>> Anything directly to tty from kernel is a hack that is missing locks and
>>>>> bypassing state.
>>>> 
>>>> So what you are saying is that we want to have a callback mechanism back into hciattach. So when kernel needs a changed baudrate, it should tell hciattach, it will then change the baudrate, and tell the kernel when it did so.
>>>> 
>>>> I wonder if I could just repurpose the ldisc read/write/poll callback handlers to implement such a simple callback/notification protocol. That would actually make sense to me since the kernel wants to drive the TTY with the agreed upon HCI transport protocol from the point we attached N_HCI to it.
>>> 
>>> Any of the above would be so ugly.
>>> Until I can work up something better, the lesser evil is using
>>> tty_set_termios().
>>> 
>>> Please cc me for patches using tty_set_termios() or other tty workarounds.
>> 
>> I was proposing to use tty->ops->set_termios. Is there any benefit from calling the native ops compared to using the not yet exposed wrapper?
> 
> tty_set_termios() performs the termios swap before calling the
> driver; and grabs the proper lock.
> 
> As it is, the wrapper fn that calls tty_set_termios() will want to
> perform a tty_wait_until_sent(tty, 0) before tty_set_termios();
> otherwise, hci command bytes could still be in the xmit buffer
> when the line settings are changed by the UART driver's set_termios().
> [That doesn't happen automagically because there's times when the
> line settings are commanded to change without waiting.]

okay. Good to know.

>>>>>>>> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.
>>>>>>> 
>>>>>>> I'm conscious of the limitations of using tty/serial in the current manner.
>>>>>>> 
>>>>>>> A fun project for one of these vendors would be to split the serial core
>>>>>>> into a separate tty driver and an abstraction layer that would allow other
>>>>>>> kernel subsystems to enslave the port.
>>>>>> 
>>>>>> They are proposing TTY slave devices and maybe that will help eventually, but finding agreement on that seems hard as well. And that is also mostly for handling the extra GPIO configuration for power up/down the Bluetooth chip behind the TTY and not the TTY itself.
>>>>> 
>>>>> That's different.
>>>>> 
>>>>>> I would love if we could just expose these TTYs as bi-directional pipes on a "serial bus" and just have a driver attach to it instead of going through a line discipline.
>>>>> 
>>>>> That's what I'm saying.
>>>>> 
>>>>> Ultimately, the stuff you want to implement on the BT side will need a native
>>>>> kernel interface where you own the device reference.
>>>> 
>>>> Even the HSU on our devices expose a TTY instead of an actual HSU bus where we could have be a HSU driver on it. If you know how we can get this changed, I am all ears.
>>> 
>>> There's no magic bullet here; it would be a lot of work.
>>> 
>>>> The hci_uart driver and N_HCI line discipline will be still needed since there are actual development boards with USB/TTY converters attached to it. However all the embedded devices where it is all hardwired, proper exposed busses or platform devices would be a lot better.
>>> 
>>> But never both simultaneously on the same device, right?
>>> Because the idea would be that the UART is exclusive and wholly owned, without
>>> a userspace dev node.
>> 
>> Yes. It is exclusive access to the device. No userspace device node needed.
> 
> Ok.
> 
>> So if we could just even set the line discipline from the kernel similar to attaching a driver to a device, then that would be something I would be interested as well.
> 
> That's probably doable right now; most of the heavy lifting of setting
> the line discipline is in a single function. If this were being done from workqueue,
> it'd have to be an unbounded queue because of the implicit module load of
> the line discipline.

I just double checked. Both of our workqueues are unbound.

        hdev->req_workqueue = alloc_workqueue("%s", WQ_HIGHPRI | WQ_UNBOUND |    
                                              WQ_MEM_RECLAIM, 1, hdev->name);

However these workqueues are not the ones that will attach the line discipline anyway. That will be a driver detail and not a core detail.

Regards

Marcel