Added support for Atheros AR300x UART Bluetooth Chip
Subsystems (2)
PATCH 1/1 Added support for Atheros AR300x UART Bluetooth Chip
--- drivers/bluetooth/Kconfig | 10 + drivers/bluetooth/Makefile | 1 + drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++ drivers/bluetooth/hci_ath.h | 72 ++++++ drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_uart.h | 8 +- 6 files changed, 641 insertions(+), 1 deletions(-) create mode 100755 drivers/bluetooth/hci_ath.c create mode 100755 drivers/bluetooth/hci_ath.h
drivers/bluetooth/Kconfig +10 -0
@@ -58,6 +58,16 @@ config BT_HCIUART_BCSP Say Y here to compile support for HCI BCSP protocol. +config BT_HCIUART_ATH + bool "Atheros AR3001 Board support" + depends on BT_HCIUART + help + HCIATH (HCI Atheros) is a serial protocol for communication + between Bluetooth device and host. This protocol is required for + serial Bluetooth devices that are based on Atheros AR3001 chips. + + Say Y here to compile support for HCIATH protocol. + config BT_HCIUART_LL bool "HCILL protocol support" depends on BT_HCIUART
drivers/bluetooth/Makefile +1 -0
@@ -26,4 +26,5 @@ hci_uart-y := hci_ldisc.o hci_uart-$(CONFIG_BT_HCIUART_H4) += hci_h4.o hci_uart-$(CONFIG_BT_HCIUART_BCSP) += hci_bcsp.o hci_uart-$(CONFIG_BT_HCIUART_LL) += hci_ll.o +hci_uart-$(CONFIG_BT_HCIUART_ATH) += hci_ath.o hci_uart-objs := $(hci_uart-y)
drivers/bluetooth/hci_ath.c new file +545 -0
545 lines not on this page · raw message
drivers/bluetooth/hci_ath.h new file +72 -0
72 lines not on this page · raw message
drivers/bluetooth/hci_ldisc.c +6 -0
@@ -542,6 +542,9 @@ static int __init hci_uart_init(void)#ifdef CONFIG_BT_HCIUART_LL ll_init ();#endif +#ifdef CONFIG_BT_HCIUART_ATH +ath_init (); +#endif return 0 ; }
@@ -559,6 +562,9 @@ static void __exit hci_uart_exit(void)#ifdef CONFIG_BT_HCIUART_LL ll_deinit ();#endif +#ifdef CONFIG_BT_HCIUART_ATH +ath_deinit (); +#endif /* Release tty registration of line discipline */ if ((err =tty_unregister_ldisc (N_HCI )))
drivers/bluetooth/hci_uart.h +7 -1
@@ -33,13 +33,14 @@#define HCIUARTGETDEVICE _IOR('U', 202, int)/* UART protocols */ -#define HCI_UART_MAX_PROTO 5 +#define HCI_UART_MAX_PROTO 6#define HCI_UART_H4 0#define HCI_UART_BCSP 1#define HCI_UART_3WIRE 2#define HCI_UART_H4DS 3#define HCI_UART_LL 4 +#define HCI_UART_ATH 5struct hci_uart ;
@@ -91,3 +92,8 @@ int bcsp_deinit(void);int ll_init (void );int ll_deinit (void );#endif + +#ifdef CONFIG_BT_HCIUART_ATH +int ath_init (void ); +int ath_deinit (void ); +#endif
Signature · 1 line
1.6.3.3
Hi Marcel,
Please send your comments
Thanks
Quoted 730 lines
Signed-off-by Suraj <[email protected]>--- drivers/bluetooth/Kconfig | 10 + drivers/bluetooth/Makefile | 1 + drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++ drivers/bluetooth/hci_ath.h | 72 ++++++ drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_uart.h | 8 +- 6 files changed, 641 insertions(+), 1 deletions(-) create mode 100755 drivers/bluetooth/hci_ath.c create mode 100755 drivers/bluetooth/hci_ath.hdrivers/bluetooth/Kconfig +10
-0@@ -58,6 +58,16 @@ config BT_HCIUART_BCSP Say Y here to compile support for HCI BCSP protocol. +config BT_HCIUART_ATH + bool "Atheros AR3001 Board support" + depends on BT_HCIUART + help + HCIATH (HCI Atheros) is a serial protocol for communication + between Bluetooth device and host. This protocol is required for + serial Bluetooth devices that are based on Atheros AR3001 chips. + + Say Y here to compile support for HCIATH protocol. + config BT_HCIUART_LL bool "HCILL protocol support" depends on BT_HCIUARTdrivers/bluetooth/Makefile +1
-0@@ -26,4 +26,5 @@ hci_uart-y := hci_ldisc.o hci_uart-$(CONFIG_BT_HCIUART_H4) += hci_h4.o hci_uart-$(CONFIG_BT_HCIUART_BCSP) += hci_bcsp.o hci_uart-$(CONFIG_BT_HCIUART_LL) += hci_ll.o +hci_uart-$(CONFIG_BT_HCIUART_ATH) += hci_ath.o hci_uart-objs := $(hci_uart-y)drivers/bluetooth/hci_ath.c new file +545
-0@@ -0,0 +1,545 @@ +/* +* Copyright (c) 2009-2010 Atheros Communications Inc. +* +* 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. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +* +*/ + +#include <linux/module.h> +#include <linux/kernel.h> + +#include <linux/init.h> +#include <linux/sched.h> +#include <linux/types.h> +#include <linux/fcntl.h> +#include <linux/interrupt.h> +#include <linux/ptrace.h> +#include <linux/poll.h> + +#include <linux/slab.h> +#include <linux/tty.h> +#include <linux/errno.h> +#include <linux/string.h> +#include <linux/signal.h> +#include <linux/ioctl.h> +#include <linux/skbuff.h> +#include <linux/firmware.h> +#include <linux/wait.h> +#include <linux/timer.h> +#include <linux/tty.h> + +#include <net/bluetooth/bluetooth.h> +#include <net/bluetooth/hci.h> + +#include "hci_uart.h" +#include "hci_ath.h" +static void ath_context_switch (struct work_struct *work) +{ +int status; +struct ath_struct *ath; +struct hci_uart *hu; +struct tty_struct *tty; +struct sk_buff *skbuf; + ath = container_of(work, struct ath_struct, ctxtsw); + hu = ath->hu; + tty = hu->tty; + status =ath_wakeup_ar3001 (tty); +if ((status &TIOCM_CTS )) { +while ((skbuf =skb_dequeue (&ath->tx_wait_q))) +skb_queue_tail (&ath->txq, skbuf); + +/* Ready to send Data */ +clear_bit (HCI_UART_SENDING , &hu->tx_state); +hci_uart_tx_wakeup (hu); + } +} + +/* Initialize protocol */ +static int ath_open (struct hci_uart *hu) +{ +struct ath_struct *ath; +BT_DBG ("hu %p" , hu); + ath =kzalloc (sizeof (*ath),GFP_ATOMIC ); +if (!ath) +return -ENOMEM ; +skb_queue_head_init (&ath->txq); +skb_queue_head_init (&ath->tx_wait_q); +skb_queue_head_init (&ath->tx_cmd_wait_q); +spin_lock_init (&ath->hciath_lock); + ath->cur_sleep =0 ; + ath->num_cmds_complete =1 ; + hu->priv = ath; + ath->hu = hu; +init_waitqueue_head (&ath->wqevt); +INIT_WORK (&ath->ctxtsw, ath_context_switch); +return 0 ; +} + +/* Flush protocol data */ +static int ath_flush (struct hci_uart *hu) +{ +struct ath_struct *ath = hu->priv; +BT_DBG ("hu %p" , hu); +skb_queue_purge (&ath->tx_wait_q); +skb_queue_purge (&ath->txq); +skb_queue_purge (&ath->tx_cmd_wait_q); +return 0 ; +} + +/* Close protocol */ +static int ath_close (struct hci_uart *hu) +{ +struct ath_struct *ath; + ath = hu->priv; +BT_DBG ("hu %p" , hu); +skb_queue_purge (&ath->tx_wait_q); +skb_queue_purge (&ath->txq); +skb_queue_purge (&ath->tx_cmd_wait_q); +if (ath->rx_skb) +kfree_skb (ath->rx_skb); +wake_up_interruptible (&ath->wqevt); + hu->priv =NULL ; +kfree (ath); +return 0 ; +} + +/* Enqueue frame for transmittion (padding, crc, etc) */ +/* may be called from two simultaneous tasklets */ +static int ath_enqueue (struct hci_uart *hu,struct sk_buff *skb) +{ +struct ath_struct *ath; + ath = hu->priv; +if (HCI_SCODATA_PKT ==bt_cb (skb)->pkt_type) { + +/* Discard SCO packet as AR3001 does not support SCO over HCI */ +BT_DBG ("SCO Packet over HCI received Dropping\n" ); +kfree (skb); +return 0 ; + } +BT_DBG ("hu %p skb %p" , hu, skb); + +/* Prepend skb with frame type */ +memcpy (skb_push (skb,1 ), &bt_cb (skb)->pkt_type,1 ); +if (ath->num_cmds_complete <=0 + &&HCI_COMMAND_PKT ==bt_cb (skb)->pkt_type) { +skb_queue_tail (&ath->tx_cmd_wait_q, skb); +return 0 ; + } + +skb_queue_tail (&ath->txq, skb); +set_bit (HCI_UART_SENDING , &hu->tx_state); + +schedule_work (&ath->ctxtsw); +return 0 ; +} + +static struct sk_buff *ath_dequeue (struct hci_uart *hu) +{ +struct ath_struct *ath; +struct sk_buff *skbuff; + ath = hu->priv; + skbuff =skb_dequeue (&ath->txq); +if (NULL != skbuff) { +ath_handle_host_data (ath,bt_cb (skbuff)->pkt_type, + &skbuff->data[1 ], skbuff->len -1 ); + } +return skbuff; +} + +static inline int ath_check_data_len (struct ath_struct *ath,int len) +{ + registerint room =skb_tailroom (ath->rx_skb); +BT_DBG ("len %d room %d" , len, room); +if (!len) { +hci_recv_frame (ath->rx_skb); + }else if (len > room) { +BT_ERR ("Data length is too large" ); +kfree_skb (ath->rx_skb); + }else { + ath->rx_state =HCIATH_W4_DATA ; + ath->rx_count = len; +return len; + } + ath->rx_state =HCIATH_W4_PACKET_TYPE ; + ath->rx_skb =NULL ; + ath->rx_count =0 ; +return 0 ; +} + +/* Recv data */ +static int ath_recv (struct hci_uart *hu,void *data,int count) +{ +struct ath_struct *ath = hu->priv; + registerchar *ptr; +struct hci_event_hdr *eh; +struct hci_acl_hdr *ah; +struct hci_sco_hdr *sh; +struct sk_buff *skbuff; + registerint len, type, dlen; + skbuff =NULL ; +BT_DBG ("hu %p count %d rx_state %d rx_count %d" , hu, count, + ath->rx_state, ath->rx_count); + ptr = data; +while (count) { +if (ath->rx_count) { + len = min_t(unsigned int, ath->rx_count, count); +memcpy (skb_put (ath->rx_skb, len), ptr, len); + ath->rx_count -= len; + count -= len; + ptr += len; +if (ath->rx_count) +continue ; +switch (ath->rx_state) { +case HCIATH_W4_DATA : +ath_handle_data_from_controller (ath, +bt_cb + (ath->rx_skb)-> + pkt_type, + ath->rx_skb-> + data, + ath->rx_skb-> + len); +if (HCI_EVENT_PKT == +bt_cb (ath->rx_skb)->pkt_type + && ath->num_cmds_complete >0 ) { + + skbuff = +skb_dequeue (&ath->tx_cmd_wait_q); +if (skbuff !=NULL ) { +skb_queue_tail (&ath->txq, + skbuff); +schedule_work (&ath->ctxtsw); + } + } +if (ath_verify_event_discardable + (hu,bt_cb (ath->rx_skb)->pkt_type, + ath->rx_skb->data)) { +kfree (ath->rx_skb); + ath->rx_skb =NULL ; + }else { +hci_recv_frame (ath->rx_skb); + } + ath->rx_state =HCIATH_W4_PACKET_TYPE ; + ath->rx_skb =NULL ; + ath->rx_count =0 ; +continue ; +case HCIATH_W4_EVENT_HDR : + eh = (struct hci_event_hdr *)ath->rx_skb->data; +BT_DBG ("Event header: evt 0x%2.2x plen %d" , + eh->evt, eh->plen); +ath_check_data_len (ath, eh->plen); +continue ; +case HCIATH_W4_ACL_HDR : + ah = (struct hci_acl_hdr *)ath->rx_skb->data; + dlen =__le16_to_cpu (ah->dlen); +BT_DBG ("ACL header: dlen %d" , dlen); +ath_check_data_len (ath, dlen); +continue ; +case HCIATH_W4_SCO_HDR : + sh = (struct hci_sco_hdr *)ath->rx_skb->data; +BT_DBG ("SCO header: dlen %d" , sh->dlen); +ath_check_data_len (ath, sh->dlen); +continue ; + } + } + +/* HCIATH_W4_PACKET_TYPE */ +switch (*ptr) { +case HCI_EVENT_PKT : +BT_DBG ("Event packet" ); + ath->rx_state =HCIATH_W4_EVENT_HDR ; + ath->rx_count =HCI_EVENT_HDR_SIZE ; + type =HCI_EVENT_PKT ; +break ; +case HCI_ACLDATA_PKT : +BT_DBG ("ACL packet" ); + ath->rx_state =HCIATH_W4_ACL_HDR ; + ath->rx_count =HCI_ACL_HDR_SIZE ; + type =HCI_ACLDATA_PKT ; +break ; +case HCI_SCODATA_PKT : +BT_DBG ("SCO packet" ); + ath->rx_state =HCIATH_W4_SCO_HDR ; + ath->rx_count =HCI_SCO_HDR_SIZE ; + type =HCI_SCODATA_PKT ; +break ; +default : +BT_ERR ("Unknown HCI packet type %2.2x" , (__u8) *ptr); + hu->hdev->stat.err_rx++; + ptr++; + count--; +continue ; + }; + ptr++; + count--; + +/* Allocate packet */ + ath->rx_skb =bt_skb_alloc (HCI_MAX_FRAME_SIZE ,GFP_ATOMIC ); +if (!ath->rx_skb) { +BT_ERR ("Can't allocate mem for new packet" ); + ath->rx_state =HCIATH_W4_PACKET_TYPE ; + ath->rx_count =0 ; +return 0 ; + } + ath->rx_skb->dev = (void *)hu->hdev; +bt_cb (ath->rx_skb)->pkt_type = type; + }return count; +} + +static void ath_controller_sleep_mode (struct hci_uart *hu,bool enable) +{ +unsigned char sleepcmd[] = {0x01 ,0x04 ,0xFC ,0x01 ,0x00 }; + sleepcmd[4 ] = enable; +ath_write_data_to_cntrlr (hu->hdev, sleepcmd,sizeof (sleepcmd)); +} + +int ath_wakeup_ar3001 (struct tty_struct *tty) +{ +struct termios settings; +int status =0x00 ; +mm_segment_t oldfs; + status = tty->driver->ops->tiocmget (tty,NULL ); + +if ((status &TIOCM_CTS )) +return status; + + oldfs =get_fs (); +set_fs (KERNEL_DS ); +n_tty_ioctl_helper (tty,NULL ,TCGETS , (unsigned long )&settings); + + settings.c_cflag &= ~CRTSCTS ; +n_tty_ioctl_helper (tty,NULL ,TCSETS , (unsigned long )&settings); +set_fs (oldfs); + status = tty->driver->ops->tiocmget (tty,NULL ); + + tty->driver->ops->tiocmset (tty,NULL ,0x00 ,TIOCM_RTS ); +mdelay (20 ); + + status = tty->driver->ops->tiocmget (tty,NULL ); + + tty->driver->ops->tiocmset (tty,NULL ,TIOCM_RTS ,0x00 ); +mdelay (20 ); + + status = tty->driver->ops->tiocmget (tty,NULL ); + oldfs =get_fs (); +set_fs (KERNEL_DS ); +n_tty_ioctl_helper (tty,NULL ,TCGETS , (unsigned long )&settings); + + settings.c_cflag |=CRTSCTS ; +n_tty_ioctl_helper (tty,NULL ,TCSETS , (unsigned long )&settings); +set_fs (oldfs); +return status; +} + +int ath_fullboot_config (struct hci_uart *hu,int current_event) +{ +struct sk_buff *skbuf; +struct ath_struct *ath = hu->priv; +static int autosleep; +unsigned char rstevt[] = {0x1 ,0x3 ,0xc ,0x0 }; +if (current_event ==HCI_RESET ) { + +if (ath->cur_sleep) { + autosleep =1 ; +ath_controller_sleep_mode (hu,1 ); +return 1 ; + }else { +return 0 ; + } + } + +if (current_event ==HCI_SET_SLEEP_MODE ) { + +if (autosleep ==0 ) +return 1 ; + +while ((skbuf =skb_dequeue (&ath->tx_wait_q))) +skb_queue_tail (&ath->txq, skbuf); + +ath_write_data_to_host (hu->hdev, rstevt,sizeof (rstevt)); + autosleep =0 ; +return 1 ; + } +return 0 ; +} + +int ath_write_data_to_host (void *dev,unsigned char *data,u8 length) +{ +struct sk_buff *skbuf; +struct hci_dev *hdev; + hdev = (struct hci_dev *)dev; + skbuf =bt_skb_alloc (length,GFP_ATOMIC ); +if (!skbuf) { +BT_ERR ("Memory allocation failed" ); +return -1 ; + } +skb_orphan (skbuf); + +/* First byte will be added as packet type */ +bt_cb (skbuf)->pkt_type = data[0 ]; + skbuf->dev = (void *)hdev; +memcpy (skb_put (skbuf, length -1 ), &data[1 ], length -1 ); +hci_recv_frame (skbuf); +return length; +} + +int ath_write_data_to_cntrlr (void *dev,unsigned char *Data,u8 len) +{ +struct sk_buff *skbuff; +struct ath_struct *ath; +struct hci_uart *hu; +struct hci_dev *hdev; +if (NULL == dev) { +BT_DBG ("NULL handle received %p \n" , dev); +return 0 ; + } + hdev = (struct hci_dev *)dev; + hu = (struct hci_uart *)hdev->driver_data; +if (hu ==NULL ) { +BT_DBG ("NULL handle received %p \n" , hdev); +return 0 ; + } + ath = hu->priv; +if (ath ==NULL ) { +BT_DBG ("NULL handle received \n" ); +return 0 ; + } + skbuff =bt_skb_alloc (len,GFP_ATOMIC ); +if (skbuff ==NULL ) { +BT_DBG ("Malloc Fail memory %p \n" , skbuff); +return 0 ; + } +skb_orphan (skbuff); + +if (len !=0 ) +memcpy (skb_put (skbuff, len), Data, len); + +bt_cb (skbuff)->pkt_type =HCI_COMMAND_PKT ; + skbuff->dev = dev; +if (ath->num_cmds_complete >0 ) { +skb_queue_tail (&ath->txq, skbuff); +schedule_work (&ath->ctxtsw); + }else { +skb_queue_tail (&ath->tx_cmd_wait_q, skbuff); + } +return len; +} + +int ath_check_sleep_cmd (struct ath_struct *ath,unsigned char *packet) +{ +if (packet[0 ] ==HCI_EVENT_PKT && packet[1 ] ==0xFC ) + ath->cur_sleep = packet[3 ]; + +return 0 ; +} + +int ath_verify_event_discardable (struct hci_uart *hu,unsigned char pkt_type, +unsigned char *packet) +{ +if (pkt_type !=HCI_EVENT_PKT ) +return 0 ; + +switch (packet[0 ]) { +case 0x0E :/* Command Complete Event */ +if (packet[3 ] ==0x03 && packet[4 ] ==0x0C ) { + +/* Command complete for HCI Reset Received */ +return ath_fullboot_config (hu,HCI_RESET ); + }else if (packet[3 ] ==0x04 && packet[4 ] ==0xFC ) { +return ath_fullboot_config (hu,HCI_SET_SLEEP_MODE ); + } +break ; + } +return 0 ; +} + +/* Update the number of commands that can be sent to controller */ +void ath_handle_hci_cmd (struct ath_struct *ath,u8 * packet) +{ +spin_lock (&ath->hciath_lock); + ath->num_cmds_complete--; +spin_unlock (&ath->hciath_lock); +} + +void ath_handle_hci_event (struct ath_struct *ath,u8 * packet) +{ +switch (packet[0 ]) { +case 0x05 :/* ACL Disconnection Complete Event */ +break ; +case 0x03 :/* ACL Connection Complete Event */ +break ; +case 0x0E :/* Command Complete Event */ +spin_lock (&ath->hciath_lock); + ath->num_cmds_complete = packet[2 ]; +spin_unlock (&ath->hciath_lock); +break ; +case 0x0F :/* Command Status Event */ +spin_lock (&ath->hciath_lock); + ath->num_cmds_complete = packet[3 ]; +spin_unlock (&ath->hciath_lock); +break ; + } +} + +void ath_handle_host_data (struct ath_struct *ath,u8 pktType,u8 * packet, +unsigned int len) +{ +switch (pktType) { +case HCI_ACLDATA_PKT :/* ACL packets */ +break ; +case HCI_COMMAND_PKT :/* HCI Commands */ +ath_handle_hci_cmd (ath, packet); +ath_check_sleep_cmd (ath, packet); +break ; + } +} + +void ath_handle_data_from_controller (struct ath_struct *ath,u8 pktType, +u8 *packet,unsigned int len) +{ +switch (pktType) { +case HCI_ACLDATA_PKT :/* ACL packets */ +break ; +case HCI_EVENT_PKT :/* HCI Events */ +ath_handle_hci_event (ath, packet); +break ; + } +} + +static struct hci_uart_proto athp = { + .id =HCI_UART_ATH , + .open = ath_open, + .close = ath_close, + .recv = ath_recv, + .enqueue = ath_enqueue, + .dequeue = ath_dequeue, + .flush = ath_flush, +}; + +int ath_init (void ) +{ +int err =hci_uart_register_proto (&athp); +if (!err) +BT_INFO ("HCIATH protocol initialized" ); + +else +BT_ERR ("HCIATH protocol registration failed" ); +return err; +} + +int ath_deinit (void ) +{ +return hci_uart_unregister_proto (&athp); +}drivers/bluetooth/hci_ath.h new file +72
-0@@ -0,0 +1,72 @@ +/* +* Copyright (c) 2009-2010 Atheros Communications Inc. +* +* 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. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +* +*/ + +#ifndef __HCI_ATH_H_ +#define __HCI_ATH_H_ + +#include <linux/fs.h> +#include <linux/errno.h> +#include <linux/string.h> +#include <linux/signal.h> +#include <linux/timer.h> + +#include <linux/ioctl.h> +#include <linux/skbuff.h> +#include <linux/firmware.h> +#include <linux/wait.h> + +#include <net/bluetooth/bluetooth.h> +#include <net/bluetooth/hci_core.h> + +/* HCIATH receiver States */ +#define HCIATH_W4_PACKET_TYPE 0 +#define HCIATH_W4_EVENT_HDR 1 +#define HCIATH_W4_ACL_HDR 2 +#define HCIATH_W4_SCO_HDR 3 +#define HCIATH_W4_DATA 4 + +#define HCI_SET_SLEEP_MODE 0xFC04 +#define HCI_RESET 0x030C +struct ath_struct { +struct hci_uart *hu; +unsigned int rx_state; +unsigned int rx_count; +unsigned int cur_sleep; +int num_cmds_complete; +spinlock_t hciath_lock; +struct sk_buff *rx_skb; +struct sk_buff_head txq; +struct sk_buff_head tx_wait_q; +struct sk_buff_head tx_cmd_wait_q; +wait_queue_head_t wqevt; +struct work_struct ctxtsw; +}; +int ath_write_data_to_cntrlr (void *dev,unsigned char *Data,u8 len); +int ath_write_data_to_host (void *dev,unsigned char *data,u8 length); +int ath_wakeup_ar3001 (struct tty_struct *tty); +int ath_verify_event_discardable (struct hci_uart *hu,unsigned char pkt_type, +unsigned char *packet); +int ath_check_sleep_cmd (struct ath_struct *ath,unsigned char *packet); +void ath_handle_host_data (struct ath_struct *ath,u8 pktType,u8 *packet, +unsigned int len); +void ath_handle_data_from_controller (struct ath_struct *ath,u8 pktType, +u8 *packet,unsigned int len); + + +#endif /* __HCI_ATH_H_ */ drivers/bluetooth/hci_ldisc.c +6
-0@@ -542,6 +542,9 @@ static int __init hci_uart_init(void)#ifdef CONFIG_BT_HCIUART_LL ll_init ();#endif +#ifdef CONFIG_BT_HCIUART_ATH +ath_init (); +#endif return 0 ; }@@ -559,6 +562,9 @@ static void __exit hci_uart_exit(void)#ifdef CONFIG_BT_HCIUART_LL ll_deinit ();#endif +#ifdef CONFIG_BT_HCIUART_ATH +ath_deinit (); +#endif /* Release tty registration of line discipline */ if ((err =tty_unregister_ldisc (N_HCI )))drivers/bluetooth/hci_uart.h +7
-2@@ -33,13 +33,14 @@#define HCIUARTGETDEVICE _IOR('U', 202, int)/* UART protocols */ -#define HCI_UART_MAX_PROTO 5 +#define HCI_UART_MAX_PROTO 6#define HCI_UART_H4 0#define HCI_UART_BCSP 1#define HCI_UART_3WIRE 2#define HCI_UART_H4DS 3#define HCI_UART_LL 4 +#define HCI_UART_ATH 5struct hci_uart ;@@ -91,3 +92,8 @@ int bcsp_deinit(void);int ll_init (void );int ll_deinit (void );#endif + +#ifdef CONFIG_BT_HCIUART_ATH +int ath_init (void ); +int ath_deinit (void ); +#endif --1.6.3.3
Marcel Holtmann
maintainerHi Vikram,
Quoted 18 lines
drivers/bluetooth/Kconfig | 10 + drivers/bluetooth/Makefile | 1 + drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++ drivers/bluetooth/hci_ath.h | 72 ++++++ drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_uart.h | 8 +- 6 files changed, 641 insertions(+), 1 deletions(-) create mode 100755 drivers/bluetooth/hci_ath.c create mode 100755 drivers/bluetooth/hci_ath.hdrivers/bluetooth/Kconfig +0
-0@@ -58,6 +58,16 @@ config BT_HCIUART_BCSP Say Y here to compile support for HCI BCSP protocol.
drivers/bluetooth/Kconfig config BT_HCIUART_BCSP · line 70
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+config BT_HCIUART_ATH + bool "Atheros AR3001 Board support" + depends on BT_HCIUART + help + HCIATH (HCI Atheros) is a serial protocol for communication + between Bluetooth device and host. This protocol is required for + serial Bluetooth devices that are based on Atheros AR3001 chips. + + Say Y here to compile support for HCIATH protocol. +
it would be nice if you add a document explaining this special Atheros protocol that is used.
Quoted 31 lines
drivers/bluetooth/Kconfig +31
-0+static void ath_context_switch(struct work_struct *work) +{ + int status; + struct ath_struct *ath; + struct hci_uart *hu; + struct tty_struct *tty; + struct sk_buff *skbuf; + ath = container_of(work, struct ath_struct, ctxtsw); + hu = ath->hu; + tty = hu->tty; + status = ath_wakeup_ar3001(tty); + if ((status & TIOCM_CTS)) { + while ((skbuf = skb_dequeue(&ath->tx_wait_q))) + skb_queue_tail(&ath->txq, skbuf); + + /* Ready to send Data */ + clear_bit(HCI_UART_SENDING, &hu->tx_state); + hci_uart_tx_wakeup(hu); + } +} + +/* Initialize protocol */ +static int ath_open(struct hci_uart *hu) +{ + struct ath_struct *ath; + BT_DBG("hu %p", hu); + ath = kzalloc(sizeof(*ath), GFP_ATOMIC); + if (!ath) + return -ENOMEM; + skb_queue_head_init(&ath->txq); + skb_queue_head_init(&ath->tx_wait_q);
drivers/bluetooth/Kconfig config BT_HCIUART_BCSP
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ skb_queue_head_init(&ath->tx_cmd_wait_q); + spin_lock_init(&ath->hciath_lock); + ath->cur_sleep = 0; + ath->num_cmds_complete = 1; + hu->priv = ath; + ath->hu = hu; + init_waitqueue_head(&ath->wqevt); + INIT_WORK(&ath->ctxtsw, ath_context_switch); + return 0; +}
In all the function, use at least some empty lines in between so that somebody can try to read these function. Cramping everything in less space doesn't make it simpler.
drivers/bluetooth/Kconfig
Quoted 2 lines
drivers/bluetooth/Kconfig +2
-0+/* Enqueue frame for transmittion (padding, crc, etc) */ +/* may be called from two simultaneous tasklets */
Why are you stealing the comments from hci_ll.c. Are you using actually padding and CRC?
drivers/bluetooth/Kconfig
Quoted 4 lines
drivers/bluetooth/Kconfig +4
-0+static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb) +{ + struct ath_struct *ath; + ath = hu->priv;
Direct assignment on declaration.
drivers/bluetooth/Kconfig
Quoted 1 line
drivers/bluetooth/Kconfig +1
-0+ if (HCI_SCODATA_PKT == bt_cb(skb)->pkt_type) {
Always the other way around. Variable compares to constant. Not the constant to variable.
drivers/bluetooth/Kconfig
Quoted 7 lines
drivers/bluetooth/Kconfig +7
-0+static struct sk_buff *ath_dequeue(struct hci_uart *hu) +{ + struct ath_struct *ath; + struct sk_buff *skbuff; + ath = hu->priv; + skbuff = skb_dequeue(&ath->txq); + if (NULL != skbuff) {
Never ever use the variable name skbuff and also if (skbuff) here. However preferable this is if (!skb) return NULL;
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ ath_handle_host_data(ath, bt_cb(skbuff)->pkt_type, + &skbuff->data[1], skbuff->len - 1); + } + return skbuff; +} + +static inline int ath_check_data_len(struct ath_struct *ath, int len) +{ + register int room = skb_tailroom(ath->rx_skb); + BT_DBG("len %d room %d", len, room);
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ if (!len) { + hci_recv_frame(ath->rx_skb); + } else if (len > room) { + BT_ERR("Data length is too large"); + kfree_skb(ath->rx_skb); + } else { + ath->rx_state = HCIATH_W4_DATA; + ath->rx_count = len; + return len; + }
How is this if statement even suppose to work?
Quoted 115 lines
drivers/bluetooth/Kconfig +115
-0+ ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_skb = NULL; + ath->rx_count = 0; + return 0; +} + +/* Recv data */ +static int ath_recv(struct hci_uart *hu, void *data, int count) +{ + struct ath_struct *ath = hu->priv; + register char *ptr; + struct hci_event_hdr *eh; + struct hci_acl_hdr *ah; + struct hci_sco_hdr *sh; + struct sk_buff *skbuff; + register int len, type, dlen; + skbuff = NULL; + BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count, + ath->rx_state, ath->rx_count); + ptr = data; + while (count) { + if (ath->rx_count) { + len = min_t(unsigned int, ath->rx_count, count); + memcpy(skb_put(ath->rx_skb, len), ptr, len); + ath->rx_count -= len; + count -= len; + ptr += len; + if (ath->rx_count) + continue; + switch (ath->rx_state) { + case HCIATH_W4_DATA: + ath_handle_data_from_controller(ath, + bt_cb + (ath->rx_skb)-> + pkt_type, + ath->rx_skb-> + data, + ath->rx_skb-> + len); + if (HCI_EVENT_PKT == + bt_cb(ath->rx_skb)->pkt_type + && ath->num_cmds_complete > 0) { + + skbuff = + skb_dequeue(&ath->tx_cmd_wait_q); + if (skbuff != NULL) { + skb_queue_tail(&ath->txq, + skbuff); + schedule_work(&ath->ctxtsw); + } + } + if (ath_verify_event_discardable + (hu, bt_cb(ath->rx_skb)->pkt_type, + ath->rx_skb->data)) { + kfree(ath->rx_skb); + ath->rx_skb = NULL; + } else { + hci_recv_frame(ath->rx_skb); + } + ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_skb = NULL; + ath->rx_count = 0; + continue; + case HCIATH_W4_EVENT_HDR: + eh = (struct hci_event_hdr *)ath->rx_skb->data; + BT_DBG("Event header: evt 0x%2.2x plen %d", + eh->evt, eh->plen); + ath_check_data_len(ath, eh->plen); + continue; + case HCIATH_W4_ACL_HDR: + ah = (struct hci_acl_hdr *)ath->rx_skb->data; + dlen = __le16_to_cpu(ah->dlen); + BT_DBG("ACL header: dlen %d", dlen); + ath_check_data_len(ath, dlen); + continue; + case HCIATH_W4_SCO_HDR: + sh = (struct hci_sco_hdr *)ath->rx_skb->data; + BT_DBG("SCO header: dlen %d", sh->dlen); + ath_check_data_len(ath, sh->dlen); + continue; + } + } + + /* HCIATH_W4_PACKET_TYPE */ + switch (*ptr) { + case HCI_EVENT_PKT: + BT_DBG("Event packet"); + ath->rx_state = HCIATH_W4_EVENT_HDR; + ath->rx_count = HCI_EVENT_HDR_SIZE; + type = HCI_EVENT_PKT; + break; + case HCI_ACLDATA_PKT: + BT_DBG("ACL packet"); + ath->rx_state = HCIATH_W4_ACL_HDR; + ath->rx_count = HCI_ACL_HDR_SIZE; + type = HCI_ACLDATA_PKT; + break; + case HCI_SCODATA_PKT: + BT_DBG("SCO packet"); + ath->rx_state = HCIATH_W4_SCO_HDR; + ath->rx_count = HCI_SCO_HDR_SIZE; + type = HCI_SCODATA_PKT; + break; + default: + BT_ERR("Unknown HCI packet type %2.2x", (__u8) *ptr); + hu->hdev->stat.err_rx++; + ptr++; + count--; + continue; + }; + ptr++; + count--; + + /* Allocate packet */ + ath->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE, GFP_ATOMIC);
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ if (!ath->rx_skb) { + BT_ERR("Can't allocate mem for new packet"); + ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_count = 0; + return 0; + } + ath->rx_skb->dev = (void *)hu->hdev; + bt_cb(ath->rx_skb)->pkt_type = type; + } return count; +}
We have a common exported function called hci_recv_fragment that should be doing exactly this.
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +6
-0+static void ath_controller_sleep_mode(struct hci_uart *hu, bool enable) +{ + unsigned char sleepcmd[] = { 0x01, 0x04, 0xFC, 0x01, 0x00 }; + sleepcmd[4] = enable; + ath_write_data_to_cntrlr(hu->hdev, sleepcmd, sizeof(sleepcmd)); +}
Special vendor commands need a comment. Explain the data structure of them and what are the options.
Quoted 27 lines
drivers/bluetooth/Kconfig +27
-0+int ath_wakeup_ar3001(struct tty_struct *tty) +{ + struct termios settings; + int status = 0x00; + mm_segment_t oldfs; + status = tty->driver->ops->tiocmget(tty, NULL); + + if ((status & TIOCM_CTS)) + return status; + + oldfs = get_fs(); + set_fs(KERNEL_DS); + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings); + + settings.c_cflag &= ~CRTSCTS; + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings); + set_fs(oldfs); + status = tty->driver->ops->tiocmget(tty, NULL); + + tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS); + mdelay(20); + + status = tty->driver->ops->tiocmget(tty, NULL); + + tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00); + mdelay(20); +
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ status = tty->driver->ops->tiocmget(tty, NULL); + oldfs = get_fs(); + set_fs(KERNEL_DS); + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings); + + settings.c_cflag |= CRTSCTS; + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings); + set_fs(oldfs); + return status; +}
Why is this magic in kernel space and not inside hciattach?
Quoted 21 lines
drivers/bluetooth/Kconfig +21
-0+int ath_fullboot_config(struct hci_uart *hu, int current_event) +{ + struct sk_buff *skbuf; + struct ath_struct *ath = hu->priv; + static int autosleep; + unsigned char rstevt[] = { 0x1, 0x3, 0xc, 0x0 }; + if (current_event == HCI_RESET) { + + if (ath->cur_sleep) { + autosleep = 1; + ath_controller_sleep_mode(hu, 1); + return 1; + } else { + return 0; + } + } + + if (current_event == HCI_SET_SLEEP_MODE) { + + if (autosleep == 0) + return 1;
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ + while ((skbuf = skb_dequeue(&ath->tx_wait_q))) + skb_queue_tail(&ath->txq, skbuf); + + ath_write_data_to_host(hu->hdev, rstevt, sizeof(rstevt)); + autosleep = 0; + return 1; + } + return 0; +}
Why is this function not static? And all the others?
Quoted 9 lines
drivers/bluetooth/Kconfig +9
-0+int ath_write_data_to_host(void *dev, unsigned char *data, u8 length) +{ + struct sk_buff *skbuf; + struct hci_dev *hdev; + hdev = (struct hci_dev *)dev; + skbuf = bt_skb_alloc(length, GFP_ATOMIC); + if (!skbuf) { + BT_ERR("Memory allocation failed"); + return -1;
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ } + skb_orphan(skbuf); + + /* First byte will be added as packet type */ + bt_cb(skbuf)->pkt_type = data[0]; + skbuf->dev = (void *)hdev; + memcpy(skb_put(skbuf, length - 1), &data[1], length - 1); + hci_recv_frame(skbuf); + return length; +}
What is this exactly for? Do you have a secondary receive path?
Quoted 31 lines
drivers/bluetooth/Kconfig +31
-0+int ath_write_data_to_cntrlr(void *dev, unsigned char *Data, u8 len) +{ + struct sk_buff *skbuff; + struct ath_struct *ath; + struct hci_uart *hu; + struct hci_dev *hdev; + if (NULL == dev) { + BT_DBG("NULL handle received %p \n", dev); + return 0; + } + hdev = (struct hci_dev *)dev; + hu = (struct hci_uart *)hdev->driver_data; + if (hu == NULL) { + BT_DBG("NULL handle received %p \n", hdev); + return 0; + } + ath = hu->priv; + if (ath == NULL) { + BT_DBG("NULL handle received \n"); + return 0; + } + skbuff = bt_skb_alloc(len, GFP_ATOMIC); + if (skbuff == NULL) { + BT_DBG("Malloc Fail memory %p \n", skbuff); + return 0; + } + skb_orphan(skbuff); + + if (len != 0) + memcpy(skb_put(skbuff, len), Data, len); +
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ bt_cb(skbuff)->pkt_type = HCI_COMMAND_PKT; + skbuff->dev = dev; + if (ath->num_cmds_complete > 0) { + skb_queue_tail(&ath->txq, skbuff); + schedule_work(&ath->ctxtsw); + } else { + skb_queue_tail(&ath->tx_cmd_wait_q, skbuff); + } + return len; +}
Messing with num_cmds_complete inside the driver is wrong. What are you doing here?
drivers/bluetooth/Kconfig
Quoted 4 lines
drivers/bluetooth/Kconfig +4
-0+int ath_check_sleep_cmd(struct ath_struct *ath, unsigned char *packet) +{ + if (packet[0] == HCI_EVENT_PKT && packet[1] == 0xFC) + ath->cur_sleep = packet[3];
How is this working. Every vendor event is sleep event?
Quoted 9 lines
drivers/bluetooth/Kconfig +9
-0+int ath_verify_event_discardable(struct hci_uart *hu, unsigned char pkt_type, + unsigned char *packet) +{ + if (pkt_type != HCI_EVENT_PKT) + return 0; + + switch (packet[0]) { + case 0x0E: /* Command Complete Event */ + if (packet[3] == 0x03 && packet[4] == 0x0C) {
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ + /* Command complete for HCI Reset Received */ + return ath_fullboot_config(hu, HCI_RESET); + } else if (packet[3] == 0x04 && packet[4] == 0xFC) { + return ath_fullboot_config(hu, HCI_SET_SLEEP_MODE); + } + break; + } + return 0; +}
What are you doing here?
Quoted 9 lines
drivers/bluetooth/Kconfig +9
-0+void ath_handle_hci_event(struct ath_struct *ath, u8 * packet) +{ + switch (packet[0]) { + case 0x05: /* ACL Disconnection Complete Event */ + break; + case 0x03: /* ACL Connection Complete Event */ + break; + case 0x0E: /* Command Complete Event */ + spin_lock(&ath->hciath_lock);
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ ath->num_cmds_complete = packet[2]; + spin_unlock(&ath->hciath_lock); + break; + case 0x0F: /* Command Status Event */ + spin_lock(&ath->hciath_lock); + ath->num_cmds_complete = packet[3]; + spin_unlock(&ath->hciath_lock); + break; + } +}
Something is really wrong in this driver? Are you really know how this is suppose to work if you have to interfere this much?
Quoted 14 lines
drivers/bluetooth/Kconfig +14
-0+void ath_handle_host_data(struct ath_struct *ath, u8 pktType, u8 * packet, + unsigned int len) +{ + switch (pktType) { + case HCI_ACLDATA_PKT: /* ACL packets */ + break; + case HCI_COMMAND_PKT: /* HCI Commands */ + ath_handle_hci_cmd(ath, packet); + ath_check_sleep_cmd(ath, packet); + break; + } +} + +void ath_handle_data_from_controller(struct ath_struct *ath, u8 pktType,
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ u8 *packet, unsigned int len) +{ + switch (pktType) { + case HCI_ACLDATA_PKT: /* ACL packets */ + break; + case HCI_EVENT_PKT: /* HCI Events */ + ath_handle_hci_event(ath, packet); + break; + } +}
Seriously? are you implemented your own Bluetooth stack? Please explain what you really need. And then the host stack can provide it for you. This is not acceptable.
Regards
Marcel
-----Original Message-----
From: Marcel Holtmann [mailto:[email protected]]
Sent: Monday, February 22, 2010 8:00 AM
To: Vikram Kandukuri
Cc: [email protected]; Suraj Sumangala; Luis Rodriguez
Subject: Re: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip
Hi Vikram,
Quoted 18 lines
drivers/bluetooth/Kconfig | 10 + drivers/bluetooth/Makefile | 1 + drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++ drivers/bluetooth/hci_ath.h | 72 ++++++ drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_uart.h | 8 +- 6 files changed, 641 insertions(+), 1 deletions(-) create mode 100755 drivers/bluetooth/hci_ath.c create mode 100755 drivers/bluetooth/hci_ath.hdrivers/bluetooth/Kconfig +0
-0@@ -58,6 +58,16 @@ config BT_HCIUART_BCSP Say Y here to compile support for HCI BCSP protocol.
drivers/bluetooth/Kconfig config BT_HCIUART_BCSP · line 70
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+config BT_HCIUART_ATH + bool "Atheros AR3001 Board support" + depends on BT_HCIUART + help + HCIATH (HCI Atheros) is a serial protocol for communication + between Bluetooth device and host. This protocol is required for + serial Bluetooth devices that are based on Atheros AR3001 chips. + + Say Y here to compile support for HCIATH protocol. +
it would be nice if you add a document explaining this special Atheros protocol that is used.
Quoted 33 lines
Quoted 1 line
We will do the same
drivers/bluetooth/Kconfig +31
-0+static void ath_context_switch(struct work_struct *work) +{ + int status; + struct ath_struct *ath; + struct hci_uart *hu; + struct tty_struct *tty; + struct sk_buff *skbuf; + ath = container_of(work, struct ath_struct, ctxtsw); + hu = ath->hu; + tty = hu->tty; + status = ath_wakeup_ar3001(tty); + if ((status & TIOCM_CTS)) { + while ((skbuf = skb_dequeue(&ath->tx_wait_q))) + skb_queue_tail(&ath->txq, skbuf); + + /* Ready to send Data */ + clear_bit(HCI_UART_SENDING, &hu->tx_state); + hci_uart_tx_wakeup(hu); + } +} + +/* Initialize protocol */ +static int ath_open(struct hci_uart *hu) +{ + struct ath_struct *ath; + BT_DBG("hu %p", hu); + ath = kzalloc(sizeof(*ath), GFP_ATOMIC); + if (!ath) + return -ENOMEM; + skb_queue_head_init(&ath->txq); + skb_queue_head_init(&ath->tx_wait_q);
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ skb_queue_head_init(&ath->tx_cmd_wait_q); + spin_lock_init(&ath->hciath_lock); + ath->cur_sleep = 0; + ath->num_cmds_complete = 1; + hu->priv = ath; + ath->hu = hu; + init_waitqueue_head(&ath->wqevt); + INIT_WORK(&ath->ctxtsw, ath_context_switch); + return 0; +}
In all the function, use at least some empty lines in between so that somebody can try to read these function. Cramping everything in less space doesn't make it simpler.
drivers/bluetooth/Kconfig
Quoted 4 lines
Quoted 1 line
We will make the necessary changes
drivers/bluetooth/Kconfig +2
-0+/* Enqueue frame for transmittion (padding, crc, etc) */ +/* may be called from two simultaneous tasklets */
Why are you stealing the comments from hci_ll.c. Are you using actually padding and CRC?
drivers/bluetooth/Kconfig
Quoted 6 lines
Quoted 1 line
Cleanup will be done
drivers/bluetooth/Kconfig +4
-0+static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb) +{ + struct ath_struct *ath; + ath = hu->priv;
Direct assignment on declaration.
Quoted 3 lines
Quoted 1 line
Will be done
+ if (
HCI_SCODATA_PKT== bt_cb(skb)->pkt_type) {
Always the other way around. Variable compares to constant. Not the constant to variable.
drivers/bluetooth/Kconfig
Quoted 9 lines
Quoted 1 line
Will change it accordingly
drivers/bluetooth/Kconfig +7
-0+static struct sk_buff *ath_dequeue(struct hci_uart *hu) +{ + struct ath_struct *ath; + struct sk_buff *skbuff; + ath = hu->priv; + skbuff = skb_dequeue(&ath->txq); + if (NULL != skbuff) {
Never ever use the variable name skbuff and also if (skbuff) here. However preferable this is if (!skb) return NULL;
Quoted 12 lines
Quoted 1 line
Will change the variable name and implementation as mentioned
drivers/bluetooth/Kconfig +10
-0+ ath_handle_host_data(ath, bt_cb(skbuff)->pkt_type, + &skbuff->data[1], skbuff->len - 1); + } + return skbuff; +} + +static inline int ath_check_data_len(struct ath_struct *ath, int len) +{ + register int room = skb_tailroom(ath->rx_skb); + BT_DBG("len %d room %d", len, room);
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ if (!len) { + hci_recv_frame(ath->rx_skb); + } else if (len > room) { + BT_ERR("Data length is too large"); + kfree_skb(ath->rx_skb); + } else { + ath->rx_state = HCIATH_W4_DATA; + ath->rx_count = len; + return len; + }
How is this if statement even suppose to work?
Quoted 117 lines
Quoted 1 line
This implementation was there in previous
hci_lland other implementation which we have used as our templatedrivers/bluetooth/Kconfig +115
-0+ ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_skb = NULL; + ath->rx_count = 0; + return 0; +} + +/* Recv data */ +static int ath_recv(struct hci_uart *hu, void *data, int count) +{ + struct ath_struct *ath = hu->priv; + register char *ptr; + struct hci_event_hdr *eh; + struct hci_acl_hdr *ah; + struct hci_sco_hdr *sh; + struct sk_buff *skbuff; + register int len, type, dlen; + skbuff = NULL; + BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count, + ath->rx_state, ath->rx_count); + ptr = data; + while (count) { + if (ath->rx_count) { + len = min_t(unsigned int, ath->rx_count, count); + memcpy(skb_put(ath->rx_skb, len), ptr, len); + ath->rx_count -= len; + count -= len; + ptr += len; + if (ath->rx_count) + continue; + switch (ath->rx_state) { + case HCIATH_W4_DATA: + ath_handle_data_from_controller(ath, + bt_cb + (ath->rx_skb)-> + pkt_type, + ath->rx_skb-> + data, + ath->rx_skb-> + len); + if (HCI_EVENT_PKT == + bt_cb(ath->rx_skb)->pkt_type + && ath->num_cmds_complete > 0) { + + skbuff = + skb_dequeue(&ath->tx_cmd_wait_q); + if (skbuff != NULL) { + skb_queue_tail(&ath->txq, + skbuff); + schedule_work(&ath->ctxtsw); + } + } + if (ath_verify_event_discardable + (hu, bt_cb(ath->rx_skb)->pkt_type, + ath->rx_skb->data)) { + kfree(ath->rx_skb); + ath->rx_skb = NULL; + } else { + hci_recv_frame(ath->rx_skb); + } + ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_skb = NULL; + ath->rx_count = 0; + continue; + case HCIATH_W4_EVENT_HDR: + eh = (struct hci_event_hdr *)ath->rx_skb->data; + BT_DBG("Event header: evt 0x%2.2x plen %d", + eh->evt, eh->plen); + ath_check_data_len(ath, eh->plen); + continue; + case HCIATH_W4_ACL_HDR: + ah = (struct hci_acl_hdr *)ath->rx_skb->data; + dlen = __le16_to_cpu(ah->dlen); + BT_DBG("ACL header: dlen %d", dlen); + ath_check_data_len(ath, dlen); + continue; + case HCIATH_W4_SCO_HDR: + sh = (struct hci_sco_hdr *)ath->rx_skb->data; + BT_DBG("SCO header: dlen %d", sh->dlen); + ath_check_data_len(ath, sh->dlen); + continue; + } + } + + /* HCIATH_W4_PACKET_TYPE */ + switch (*ptr) { + case HCI_EVENT_PKT: + BT_DBG("Event packet"); + ath->rx_state = HCIATH_W4_EVENT_HDR; + ath->rx_count = HCI_EVENT_HDR_SIZE; + type = HCI_EVENT_PKT; + break; + case HCI_ACLDATA_PKT: + BT_DBG("ACL packet"); + ath->rx_state = HCIATH_W4_ACL_HDR; + ath->rx_count = HCI_ACL_HDR_SIZE; + type = HCI_ACLDATA_PKT; + break; + case HCI_SCODATA_PKT: + BT_DBG("SCO packet"); + ath->rx_state = HCIATH_W4_SCO_HDR; + ath->rx_count = HCI_SCO_HDR_SIZE; + type = HCI_SCODATA_PKT; + break; + default: + BT_ERR("Unknown HCI packet type %2.2x", (__u8) *ptr); + hu->hdev->stat.err_rx++; + ptr++; + count--; + continue; + }; + ptr++; + count--; + + /* Allocate packet */ + ath->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE, GFP_ATOMIC);
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ if (!ath->rx_skb) { + BT_ERR("Can't allocate mem for new packet"); + ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_count = 0; + return 0; + } + ath->rx_skb->dev = (void *)hu->hdev; + bt_cb(ath->rx_skb)->pkt_type = type; + } return count; +}
We have a common exported function called hci_recv_fragment that should be doing exactly this.
drivers/bluetooth/Kconfig
Quoted 8 lines
Quoted 1 line
Explained at bottom
drivers/bluetooth/Kconfig +6
-0+static void ath_controller_sleep_mode(struct hci_uart *hu, bool enable) +{ + unsigned char sleepcmd[] = { 0x01, 0x04, 0xFC, 0x01, 0x00 }; + sleepcmd[4] = enable; + ath_write_data_to_cntrlr(hu->hdev, sleepcmd, sizeof(sleepcmd)); +}
Special vendor commands need a comment. Explain the data structure of them and what are the options.
Quoted 29 lines
Quoted 1 line
Will make the necessary changes
drivers/bluetooth/Kconfig +27
-0+int ath_wakeup_ar3001(struct tty_struct *tty) +{ + struct termios settings; + int status = 0x00; + mm_segment_t oldfs; + status = tty->driver->ops->tiocmget(tty, NULL); + + if ((status & TIOCM_CTS)) + return status; + + oldfs = get_fs(); + set_fs(KERNEL_DS); + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings); + + settings.c_cflag &= ~CRTSCTS; + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings); + set_fs(oldfs); + status = tty->driver->ops->tiocmget(tty, NULL); + + tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS); + mdelay(20); + + status = tty->driver->ops->tiocmget(tty, NULL); + + tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00); + mdelay(20); +
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ status = tty->driver->ops->tiocmget(tty, NULL); + oldfs = get_fs(); + set_fs(KERNEL_DS); + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings); + + settings.c_cflag |= CRTSCTS; + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings); + set_fs(oldfs); + return status; +}
Why is this magic in kernel space and not inside hciattach?
Quoted 1 line
Quoted 1 line
We have a sleep feature which makes the controller go to deep sleep
if there is no Bluetooth activity for a while,
so the driver will verify if the controller is sleeping and
try to wake up the controller using the RTS/CTS mechanism.
This has to be done before sending any packet to controller.
This cannot be done in hciattach as this is required to be called throughout the life of the driver and not just when the driver is loaded.
Quoted 21 lines
drivers/bluetooth/Kconfig +21
-0+int ath_fullboot_config(struct hci_uart *hu, int current_event) +{ + struct sk_buff *skbuf; + struct ath_struct *ath = hu->priv; + static int autosleep; + unsigned char rstevt[] = { 0x1, 0x3, 0xc, 0x0 }; + if (current_event == HCI_RESET) { + + if (ath->cur_sleep) { + autosleep = 1; + ath_controller_sleep_mode(hu, 1); + return 1; + } else { + return 0; + } + } + + if (current_event == HCI_SET_SLEEP_MODE) { + + if (autosleep == 0) + return 1;
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ + while ((skbuf = skb_dequeue(&ath->tx_wait_q))) + skb_queue_tail(&ath->txq, skbuf); + + ath_write_data_to_host(hu->hdev, rstevt, sizeof(rstevt)); + autosleep = 0; + return 1; + } + return 0; +}
Why is this function not static? And all the others?
Quoted 11 lines
Quoted 1 line
Will make all the functions static
drivers/bluetooth/Kconfig +9
-0+int ath_write_data_to_host(void *dev, unsigned char *data, u8 length) +{ + struct sk_buff *skbuf; + struct hci_dev *hdev; + hdev = (struct hci_dev *)dev; + skbuf = bt_skb_alloc(length, GFP_ATOMIC); + if (!skbuf) { + BT_ERR("Memory allocation failed"); + return -1;
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ } + skb_orphan(skbuf); + + /* First byte will be added as packet type */ + bt_cb(skbuf)->pkt_type = data[0]; + skbuf->dev = (void *)hdev; + memcpy(skb_put(skbuf, length - 1), &data[1], length - 1); + hci_recv_frame(skbuf); + return length; +}
What is this exactly for? Do you have a secondary receive path?
Quoted 1 line
Quoted 1 line
Yes, we have situation where driver sends HCI events to depending on acknowledgement of sleep enable vendor specific command.
Explained at the bottom.
Quoted 31 lines
drivers/bluetooth/Kconfig +31
-0+int ath_write_data_to_cntrlr(void *dev, unsigned char *Data, u8 len) +{ + struct sk_buff *skbuff; + struct ath_struct *ath; + struct hci_uart *hu; + struct hci_dev *hdev; + if (NULL == dev) { + BT_DBG("NULL handle received %p \n", dev); + return 0; + } + hdev = (struct hci_dev *)dev; + hu = (struct hci_uart *)hdev->driver_data; + if (hu == NULL) { + BT_DBG("NULL handle received %p \n", hdev); + return 0; + } + ath = hu->priv; + if (ath == NULL) { + BT_DBG("NULL handle received \n"); + return 0; + } + skbuff = bt_skb_alloc(len, GFP_ATOMIC); + if (skbuff == NULL) { + BT_DBG("Malloc Fail memory %p \n", skbuff); + return 0; + } + skb_orphan(skbuff); + + if (len != 0) + memcpy(skb_put(skbuff, len), Data, len); +
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ bt_cb(skbuff)->pkt_type = HCI_COMMAND_PKT; + skbuff->dev = dev; + if (ath->num_cmds_complete > 0) { + skb_queue_tail(&ath->txq, skbuff); + schedule_work(&ath->ctxtsw); + } else { + skb_queue_tail(&ath->tx_cmd_wait_q, skbuff); + } + return len; +}
Messing with num_cmds_complete inside the driver is wrong. What are you doing here?
drivers/bluetooth/Kconfig
Quoted 6 lines
Quoted 1 line
Explanation given at bottom
drivers/bluetooth/Kconfig +4
-0+int ath_check_sleep_cmd(struct ath_struct *ath, unsigned char *packet) +{ + if (packet[0] == HCI_EVENT_PKT && packet[1] == 0xFC) + ath->cur_sleep = packet[3];
How is this working. Every vendor event is sleep event?
Quoted 11 lines
Quoted 1 line
Will correct it. It should have been packet[0] ==
0x04.drivers/bluetooth/Kconfig +9
-0+int ath_verify_event_discardable(struct hci_uart *hu, unsigned char pkt_type, + unsigned char *packet) +{ + if (pkt_type != HCI_EVENT_PKT) + return 0; + + switch (packet[0]) { + case 0x0E: /* Command Complete Event */ + if (packet[3] == 0x03 && packet[4] == 0x0C) {
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ + /* Command complete for HCI Reset Received */ + return ath_fullboot_config(hu, HCI_RESET); + } else if (packet[3] == 0x04 && packet[4] == 0xFC) { + return ath_fullboot_config(hu, HCI_SET_SLEEP_MODE); + } + break; + } + return 0; +}
What are you doing here?
Quoted 11 lines
Quoted 1 line
Events for commands sent from driver need not go to the host. They are discarded at the driver level.
drivers/bluetooth/Kconfig +9
-0+void ath_handle_hci_event(struct ath_struct *ath, u8 * packet) +{ + switch (packet[0]) { + case 0x05: /* ACL Disconnection Complete Event */ + break; + case 0x03: /* ACL Connection Complete Event */ + break; + case 0x0E: /* Command Complete Event */ + spin_lock(&ath->hciath_lock);
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ ath->num_cmds_complete = packet[2]; + spin_unlock(&ath->hciath_lock); + break; + case 0x0F: /* Command Status Event */ + spin_lock(&ath->hciath_lock); + ath->num_cmds_complete = packet[3]; + spin_unlock(&ath->hciath_lock); + break; + } +}
Something is really wrong in this driver? Are you really know how this is suppose to work if you have to interfere this much?
Quoted 16 lines
Quoted 1 line
Explanation given at the bottom
drivers/bluetooth/Kconfig +14
-0+void ath_handle_host_data(struct ath_struct *ath, u8 pktType, u8 * packet, + unsigned int len) +{ + switch (pktType) { + case HCI_ACLDATA_PKT: /* ACL packets */ + break; + case HCI_COMMAND_PKT: /* HCI Commands */ + ath_handle_hci_cmd(ath, packet); + ath_check_sleep_cmd(ath, packet); + break; + } +} + +void ath_handle_data_from_controller(struct ath_struct *ath, u8 pktType,
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+ u8 *packet, unsigned int len) +{ + switch (pktType) { + case HCI_ACLDATA_PKT: /* ACL packets */ + break; + case HCI_EVENT_PKT: /* HCI Events */ + ath_handle_hci_event(ath, packet); + break; + } +}
Seriously? are you implemented your own Bluetooth stack? Please explain what you really need. And then the host stack can provide it for you. This is not acceptable.
Quoted 1 line
Quoted 1 line
The AR3001 supports power management feature that can be enabled from host during hciattach.
This will let the controller go to sleep if there is no data transfer for some time.
This feature will be disabled automatically on receiving HCI RESET command and has to be enabled
Again by sending Sleep enable Vendor specific command after every Reset.
This required the driver to keep track of HCI RESET command complete event and sent Sleep Enable command on receiving the event. Host will be notified of the HCI Reset command complete event only after the Sleep enable command is acknowledged by controller. This is the reason for the following implementation changes
1. We had to avoid using hci_recv_fragment as we need to verify and decide if the packet has to be sent to Host (In case of HCI RESET).
2. We had to add ath_write_data_to_host so that we can sent HCI RESET
command complete on receiving Sleep Enable command complete event from controller.
3. We had to add ath_write_data_to_cntrlr so that driver can send the Sleep enable command after every HCI RESET.
4. Have ath_verify_event_discardable which keep track of the HCI RESET and Sleep Enable Command complete event and decide whether to send the HCI RESET event to host or send only after sleep enable.
Quoted 1 line
Quoted 1 line
Regarding the use of
num_cmds_completeinside driver,
we have to send vendor specific commands from the driver to configure the controller depending on the status of ACL connection.
So, there is a chance that the this might cause driver to send a command to controller
when it is already busy with command sent from host.
That is the reason why we had to go for streamlining the command flow in the driver.
If you can export hci_send_cmd API in hci_core.c, then we might be able to use it to send a command to controller without worrying about command streamlining.
Please do let me know your comments,
Regards
Suraj
On Mon, Feb 22, 2010 at 12:54:44AM -0800, Suraj Sumangala wrote:
Suraj,
please see:
http://en.wikipedia.org/wiki/Posting_style#Bottom-posting
This will be easier on you if you use a mail user agent where bottom posting is easily done from, like Thunderbird, or mutt.
It was very hard to read your reply to Marcel.
Luis
Quoted 21 lines · Marcel Holtmann
Hi Vikram,
Quoted 18 lines
drivers/bluetooth/Kconfig | 10 + drivers/bluetooth/Makefile | 1 + drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++ drivers/bluetooth/hci_ath.h | 72 ++++++ drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_uart.h | 8 +- 6 files changed, 641 insertions(+), 1 deletions(-) create mode 100755 drivers/bluetooth/hci_ath.c create mode 100755 drivers/bluetooth/hci_ath.hdrivers/bluetooth/Kconfig +0
-0@@ -58,6 +58,16 @@ config BT_HCIUART_BCSP Say Y here to compile support for HCI BCSP protocol.
Quoted 15 lines
drivers/bluetooth/Kconfig config BT_HCIUART_BCSP
Quoted 11 lines
drivers/bluetooth/Kconfig +10
-0+config BT_HCIUART_ATH + bool "Atheros AR3001 Board support" + depends on BT_HCIUART + help + HCIATH (HCI Atheros) is a serial protocol for communication + between Bluetooth device and host. This protocol is required for + serial Bluetooth devices that are based on Atheros AR3001 chips. + + Say Y here to compile support for HCIATH protocol. +it would be nice if you add a document explaining this special Atheros protocol that is used.
I will send the document with the required changes
Quoted 39 lines
Quoted 39 lines
drivers/bluetooth/Kconfig +38
-0+static void ath_context_switch(struct work_struct *work) +{ + int status; + struct ath_struct *ath; + struct hci_uart *hu; + struct tty_struct *tty; + struct sk_buff *skbuf; + ath = container_of(work, struct ath_struct, ctxtsw); + hu = ath->hu; + tty = hu->tty; + status = ath_wakeup_ar3001(tty); + if ((status & TIOCM_CTS)) { + while ((skbuf = skb_dequeue(&ath->tx_wait_q))) + skb_queue_tail(&ath->txq, skbuf); + + /* Ready to send Data */ + clear_bit(HCI_UART_SENDING, &hu->tx_state); + hci_uart_tx_wakeup(hu); + } +} + +/* Initialize protocol */ +static int ath_open(struct hci_uart *hu) +{ + struct ath_struct *ath; + BT_DBG("hu %p", hu); + ath = kzalloc(sizeof(*ath), GFP_ATOMIC); + if (!ath) + return -ENOMEM; + skb_queue_head_init(&ath->txq); + skb_queue_head_init(&ath->tx_wait_q); + skb_queue_head_init(&ath->tx_cmd_wait_q); + spin_lock_init(&ath->hciath_lock); + ath->cur_sleep = 0; + ath->num_cmds_complete = 1; + hu->priv = ath; + ath->hu = hu; + init_waitqueue_head(&ath->wqevt);
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 4 lines
drivers/bluetooth/Kconfig +3
-0+ INIT_WORK(&ath->ctxtsw, ath_context_switch); + return 0; +}In all the function, use at least some empty lines in between so that somebody can try to read these function. Cramping everything in less space doesn't make it simpler.
We will make the necessary changes
Quoted 8 lines
drivers/bluetooth/Kconfig
Quoted 4 lines
drivers/bluetooth/Kconfig +2
-0+/* Enqueue frame for transmittion (padding, crc, etc) */ +/* may be called from two simultaneous tasklets */Why are you stealing the comments from
hci_ll.c. Are you using actually padding and CRC?
Will do the cleanup along with the other changes mentioned
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +4
-0+static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb) +{ + struct ath_struct *ath; + ath = hu->priv;Direct assignment on declaration.
Will correct it along with other changes
Quoted 7 lines
Quoted 3 lines
+ if (
HCI_SCODATA_PKT== bt_cb(skb)->pkt_type) {Always the other way around. Variable compares to constant. Not the constant to variable.
Will correct it along with other changes
Quoted 13 lines
drivers/bluetooth/Kconfig
Quoted 9 lines
drivers/bluetooth/Kconfig +7
-0+static struct sk_buff *ath_dequeue(struct hci_uart *hu) +{ + struct ath_struct *ath; + struct sk_buff *skbuff; + ath = hu->priv; + skbuff = skb_dequeue(&ath->txq); + if (NULL != skbuff) {Never ever use the variable name skbuff and also if (skbuff) here. However preferable this is if (!skb) return NULL;
I will take care of the variable name change
Quoted 16 lines
Quoted 16 lines
drivers/bluetooth/Kconfig +15
-0+ ath_handle_host_data(ath, bt_cb(skbuff)->pkt_type, + &skbuff->data[1], skbuff->len - 1); + } + return skbuff; +} + +static inline int ath_check_data_len(struct ath_struct *ath, int len) +{ + register int room = skb_tailroom(ath->rx_skb); + BT_DBG("len %d room %d", len, room); + if (!len) { + hci_recv_frame(ath->rx_skb); + } else if (len > room) { + BT_ERR("Data length is too large"); + kfree_skb(ath->rx_skb);
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +5
-0+ } else { + ath->rx_state = HCIATH_W4_DATA; + ath->rx_count = len; + return len; + }How is this if statement even suppose to work?
Retained this code as this was part of the hci_ll implementation that we used as template. Will make the necessary changes.
Quoted 122 lines
Quoted 122 lines
drivers/bluetooth/Kconfig +121
-0+ ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_skb = NULL; + ath->rx_count = 0; + return 0; +} + +/* Recv data */ +static int ath_recv(struct hci_uart *hu, void *data, int count) +{ + struct ath_struct *ath = hu->priv; + register char *ptr; + struct hci_event_hdr *eh; + struct hci_acl_hdr *ah; + struct hci_sco_hdr *sh; + struct sk_buff *skbuff; + register int len, type, dlen; + skbuff = NULL; + BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count, + ath->rx_state, ath->rx_count); + ptr = data; + while (count) { + if (ath->rx_count) { + len = min_t(unsigned int, ath->rx_count, count); + memcpy(skb_put(ath->rx_skb, len), ptr, len); + ath->rx_count -= len; + count -= len; + ptr += len; + if (ath->rx_count) + continue; + switch (ath->rx_state) { + case HCIATH_W4_DATA: + ath_handle_data_from_controller(ath, + bt_cb + (ath->rx_skb)-> + pkt_type, + ath->rx_skb-> + data, + ath->rx_skb-> + len); + if (HCI_EVENT_PKT == + bt_cb(ath->rx_skb)->pkt_type + && ath->num_cmds_complete > 0) { + + skbuff = + skb_dequeue(&ath->tx_cmd_wait_q); + if (skbuff != NULL) { + skb_queue_tail(&ath->txq, + skbuff); + schedule_work(&ath->ctxtsw); + } + } + if (ath_verify_event_discardable + (hu, bt_cb(ath->rx_skb)->pkt_type, + ath->rx_skb->data)) { + kfree(ath->rx_skb); + ath->rx_skb = NULL; + } else { + hci_recv_frame(ath->rx_skb); + } + ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_skb = NULL; + ath->rx_count = 0; + continue; + case HCIATH_W4_EVENT_HDR: + eh = (struct hci_event_hdr *)ath->rx_skb->data; + BT_DBG("Event header: evt 0x%2.2x plen %d", + eh->evt, eh->plen); + ath_check_data_len(ath, eh->plen); + continue; + case HCIATH_W4_ACL_HDR: + ah = (struct hci_acl_hdr *)ath->rx_skb->data; + dlen = __le16_to_cpu(ah->dlen); + BT_DBG("ACL header: dlen %d", dlen); + ath_check_data_len(ath, dlen); + continue; + case HCIATH_W4_SCO_HDR: + sh = (struct hci_sco_hdr *)ath->rx_skb->data; + BT_DBG("SCO header: dlen %d", sh->dlen); + ath_check_data_len(ath, sh->dlen); + continue; + } + } + + /* HCIATH_W4_PACKET_TYPE */ + switch (*ptr) { + case HCI_EVENT_PKT: + BT_DBG("Event packet"); + ath->rx_state = HCIATH_W4_EVENT_HDR; + ath->rx_count = HCI_EVENT_HDR_SIZE; + type = HCI_EVENT_PKT; + break; + case HCI_ACLDATA_PKT: + BT_DBG("ACL packet"); + ath->rx_state = HCIATH_W4_ACL_HDR; + ath->rx_count = HCI_ACL_HDR_SIZE; + type = HCI_ACLDATA_PKT; + break; + case HCI_SCODATA_PKT: + BT_DBG("SCO packet"); + ath->rx_state = HCIATH_W4_SCO_HDR; + ath->rx_count = HCI_SCO_HDR_SIZE; + type = HCI_SCODATA_PKT; + break; + default: + BT_ERR("Unknown HCI packet type %2.2x", (__u8) *ptr); + hu->hdev->stat.err_rx++; + ptr++; + count--; + continue; + }; + ptr++; + count--; + + /* Allocate packet */ + ath->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE, GFP_ATOMIC); + if (!ath->rx_skb) { + BT_ERR("Can't allocate mem for new packet"); + ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_count = 0; + return 0; + }
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 5 lines
drivers/bluetooth/Kconfig +4
-0+ ath->rx_skb->dev = (void *)hu->hdev; + bt_cb(ath->rx_skb)->pkt_type = type; + } return count; +}We have a common exported function called
hci_recv_fragmentthat should be doing exactly this.
I have provided the explanation at the bottom
Quoted 12 lines
drivers/bluetooth/Kconfig
Quoted 8 lines
drivers/bluetooth/Kconfig +6
-0+static void ath_controller_sleep_mode(struct hci_uart *hu, bool enable) +{ + unsigned char sleepcmd[] = { 0x01, 0x04, 0xFC, 0x01, 0x00 }; + sleepcmd[4] = enable; + ath_write_data_to_cntrlr(hu->hdev, sleepcmd, sizeof(sleepcmd)); +}Special vendor commands need a comment. Explain the data structure of them and what are the options.
Will add the necessary comments
Quoted 33 lines
Quoted 33 lines
drivers/bluetooth/Kconfig +32
-0+int ath_wakeup_ar3001(struct tty_struct *tty) +{ + struct termios settings; + int status = 0x00; + mm_segment_t oldfs; + status = tty->driver->ops->tiocmget(tty, NULL); + + if ((status & TIOCM_CTS)) + return status; + + oldfs = get_fs(); + set_fs(KERNEL_DS); + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings); + + settings.c_cflag &= ~CRTSCTS; + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings); + set_fs(oldfs); + status = tty->driver->ops->tiocmget(tty, NULL); + + tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS); + mdelay(20); + + status = tty->driver->ops->tiocmget(tty, NULL); + + tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00); + mdelay(20); + + status = tty->driver->ops->tiocmget(tty, NULL); + oldfs = get_fs(); + set_fs(KERNEL_DS); + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings); +
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +5
-0+ settings.c_cflag |= CRTSCTS; + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings); + set_fs(oldfs); + return status; +}Why is this magic in kernel space and not inside hciattach?
if there is no Bluetooth activity for a while,
so the driver will verify if the controller is sleeping and
try to wake up the controller using the RTS/CTS mechanism.
This has to be done before sending any packet to controller.
This cannot be done in hciattach as this is required to be called throughout the life of the driver and not just when the driver is loaded.
Quoted 27 lines
Quoted 27 lines
drivers/bluetooth/Kconfig +26
-0+int ath_fullboot_config(struct hci_uart *hu, int current_event) +{ + struct sk_buff *skbuf; + struct ath_struct *ath = hu->priv; + static int autosleep; + unsigned char rstevt[] = { 0x1, 0x3, 0xc, 0x0 }; + if (current_event == HCI_RESET) { + + if (ath->cur_sleep) { + autosleep = 1; + ath_controller_sleep_mode(hu, 1); + return 1; + } else { + return 0; + } + } + + if (current_event == HCI_SET_SLEEP_MODE) { + + if (autosleep == 0) + return 1; + + while ((skbuf = skb_dequeue(&ath->tx_wait_q))) + skb_queue_tail(&ath->txq, skbuf); + + ath_write_data_to_host(hu->hdev, rstevt, sizeof(rstevt));
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +5
-0+ autosleep = 0; + return 1; + } + return 0; +}Why is this function not static? And all the others?
will make every function static
Quoted 15 lines
Quoted 15 lines
drivers/bluetooth/Kconfig +14
-0+int ath_write_data_to_host(void *dev, unsigned char *data, u8 length) +{ + struct sk_buff *skbuf; + struct hci_dev *hdev; + hdev = (struct hci_dev *)dev; + skbuf = bt_skb_alloc(length, GFP_ATOMIC); + if (!skbuf) { + BT_ERR("Memory allocation failed"); + return -1; + } + skb_orphan(skbuf); + + /* First byte will be added as packet type */ + bt_cb(skbuf)->pkt_type = data[0];
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +5
-0+ skbuf->dev = (void *)hdev; + memcpy(skb_put(skbuf, length - 1), &data[1], length - 1); + hci_recv_frame(skbuf); + return length; +}What is this exactly for? Do you have a secondary receive path?
Yes, we have situation where driver sends HCI events to depending on acknowledgement of sleep enable vendor specific command. the reason is explained at the bottom
Quoted 38 lines
Quoted 38 lines
drivers/bluetooth/Kconfig +37
-0+int ath_write_data_to_cntrlr(void *dev, unsigned char *Data, u8 len) +{ + struct sk_buff *skbuff; + struct ath_struct *ath; + struct hci_uart *hu; + struct hci_dev *hdev; + if (NULL == dev) { + BT_DBG("NULL handle received %p \n", dev); + return 0; + } + hdev = (struct hci_dev *)dev; + hu = (struct hci_uart *)hdev->driver_data; + if (hu == NULL) { + BT_DBG("NULL handle received %p \n", hdev); + return 0; + } + ath = hu->priv; + if (ath == NULL) { + BT_DBG("NULL handle received \n"); + return 0; + } + skbuff = bt_skb_alloc(len, GFP_ATOMIC); + if (skbuff == NULL) { + BT_DBG("Malloc Fail memory %p \n", skbuff); + return 0; + } + skb_orphan(skbuff); + + if (len != 0) + memcpy(skb_put(skbuff, len), Data, len); + + bt_cb(skbuff)->pkt_type = HCI_COMMAND_PKT; + skbuff->dev = dev; + if (ath->num_cmds_complete > 0) { + skb_queue_tail(&ath->txq, skbuff); + schedule_work(&ath->ctxtsw); + } else {
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 5 lines
drivers/bluetooth/Kconfig +4
-0+ skb_queue_tail(&ath->tx_cmd_wait_q, skbuff); + } + return len; +}Messing with
num_cmds_completeinside the driver is wrong. What are you doing here?
I have explained the reason at the bottom
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +4
-0+int ath_check_sleep_cmd(struct ath_struct *ath, unsigned char *packet) +{ + if (packet[0] == HCI_EVENT_PKT && packet[1] == 0xFC) + ath->cur_sleep = packet[3];How is this working. Every vendor event is sleep event?
Will correct it. It should have been packet[0] == 0x04.
Quoted 15 lines
Quoted 15 lines
drivers/bluetooth/Kconfig +14
-0+int ath_verify_event_discardable(struct hci_uart *hu, unsigned char pkt_type, + unsigned char *packet) +{ + if (pkt_type != HCI_EVENT_PKT) + return 0; + + switch (packet[0]) { + case 0x0E: /* Command Complete Event */ + if (packet[3] == 0x03 && packet[4] == 0x0C) { + + /* Command complete for HCI Reset Received */ + return ath_fullboot_config(hu, HCI_RESET); + } else if (packet[3] == 0x04 && packet[4] == 0xFC) { + return ath_fullboot_config(hu, HCI_SET_SLEEP_MODE);
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +5
-0+ } + break; + } + return 0; +}What are you doing here?
Events for vendor specific commands sent from driver need not go to the host. They are discarded at the driver level
Quoted 16 lines
Quoted 16 lines
drivers/bluetooth/Kconfig +15
-0+void ath_handle_hci_event(struct ath_struct *ath, u8 * packet) +{ + switch (packet[0]) { + case 0x05: /* ACL Disconnection Complete Event */ + break; + case 0x03: /* ACL Connection Complete Event */ + break; + case 0x0E: /* Command Complete Event */ + spin_lock(&ath->hciath_lock); + ath->num_cmds_complete = packet[2]; + spin_unlock(&ath->hciath_lock); + break; + case 0x0F: /* Command Status Event */ + spin_lock(&ath->hciath_lock); + ath->num_cmds_complete = packet[3];
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 5 lines
drivers/bluetooth/Kconfig +4
-0+ spin_unlock(&ath->hciath_lock); + break; + } +}Something is really wrong in this driver? Are you really know how this is suppose to work if you have to interfere this much?
I have provided the reason for this implementation at the bottom
Quoted 22 lines
Quoted 22 lines
drivers/bluetooth/Kconfig +21
-0+void ath_handle_host_data(struct ath_struct *ath, u8 pktType, u8 * packet, + unsigned int len) +{ + switch (pktType) { + case HCI_ACLDATA_PKT: /* ACL packets */ + break; + case HCI_COMMAND_PKT: /* HCI Commands */ + ath_handle_hci_cmd(ath, packet); + ath_check_sleep_cmd(ath, packet); + break; + } +} + +void ath_handle_data_from_controller(struct ath_struct *ath, u8 pktType, + u8 *packet, unsigned int len) +{ + switch (pktType) { + case HCI_ACLDATA_PKT: /* ACL packets */ + break; + case HCI_EVENT_PKT: /* HCI Events */ + ath_handle_hci_event(ath, packet);
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 4 lines
drivers/bluetooth/Kconfig +3
-0+ break; + } +}Seriously? are you implemented your own Bluetooth stack? Please explain what you really need. And then the host stack can provide it for you. This is not acceptable.
The reason for this implementation is.
The AR3001 supports power management feature that can be enabled from host during hciattach.
This will let the controller go to sleep if there is no data transfer for some time. This feature is disabled automatically on receiving HCI RESET command and has to be enabled again by sending Sleep enable Vendor specific command after every HCI Reset command.
This required the driver to keep track of HCI RESET command complete event and sent Sleep Enable command on receiving the event. Host will be notified of the HCI Reset command complete event only after the Sleep enable command is acknowledged by controller. This is the reason for the following implementation changes
1. We had to avoid using hci_recv_fragment as we need to verify and decide if the packet has to be sent to Host (In case of HCI RESET).
2. We had to add ath_write_data_to_host so that we can sent HCI RESET
command complete on receiving Sleep Enable command complete event from controller.
3. We had to add ath_write_data_to_cntrlr so that driver can send the Sleep enable command after every HCI RESET.
4. Have ath_verify_event_discardable which keep track of the HCI RESET and Sleep Enable Command complete event and decide whether to send the HCI RESET event to host or send only after sleep enable.
Regarding the use of num_cmds_complete inside driver
Due to a controller specific optimization, we have to send vendor specific commands from the driver to configure the controller depending on the status of ACL connection. This might cause driver to send a command to controller when it is already busy with command sent from host. That is the reason why we had to go for streamlining the command flow in the driver.
If you can export hci_send_cmd API in hci_core.c, then we might be able to use it to send a command to controller without worrying about command streamlining.
Please do let me know your comments,
Regards
Suraj
Quoted 6 lines
Regards
Marcel
link starvation during multi profile scenario
Hi Marcel,
I am facing an issue on Bluetooth Multiprofile secnario on CSR board as well as Atheros board on Ubuntu git tree head source. The scenario is
1. From Ubuntu make A2DP connection
2. Start Streaming
3. From Ubuntu do FTP connection
4. Send file from Ubuntu to remote device.
5. Take A2DP sink (Headset) out of range.
6. FTP operation stops.
From my observation, it looks like the stalled A2DP connection is using up all HCI buffers and starving the FTP connection of HCI buffers. I have written a small enhancement for this scenario.
Please have a look at it and let me know if you have a cleaner and better implementation.
Please find my changes below,
Regards
Suraj
Author: Suraj Sumangala <[email protected]>
Date: Fri Feb 26 12:16:50 2010 +0530
Avoid link starvation in multi-link scenario
include/net/bluetooth/hci_core.h +0 -0
@@ -181,6 +181,9 @@ struct hci_conn {
unsigned int sent;
+
+ unsigned int acl_avail;
+
struct sk_buff_head data_q;
struct timer_list disc_timer;
net/bluetooth/hci_core.c +0 -0
@@ -1359,6 +1359,9 @@ static inline struct hci_conn *hci_low_sent(struct hci_dev *hdev, __u8 type, int
if (c->state != BT_CONNECTED && c->state != BT_CONFIG) continue;
+ if (c->acl_avail == 0)
+ continue;
+
num++;
if (c->sent < min) {
@@ -1423,6 +1426,10 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
hdev->acl_cnt--;
conn->sent++;
+
+ if (conn->acl_avail != 0)
+ conn->acl_avail--;
+
}
}
}
net/bluetooth/hci_event.c +0 -0
@@ -900,6 +900,7 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
if (conn->type == ACL_LINK) {
struct hci_cp_read_remote_features cp;
cp.handle = ev->handle;
+ conn->acl_avail = hdev->acl_pkts;
hci_send_cmd(hdev, HCI_OP_READ_REMOTE_FEATURES,
sizeof(cp), &cp);
}
@@ -1447,6 +1448,9 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s if (conn->type == ACL_LINK) {
if ((hdev->acl_cnt += count) > hdev->acl_pkts)hdev->acl_cnt = hdev->acl_pkts;
+
+ conn->acl_avail = count;
+
} else {
if ((hdev->sco_cnt += count) > hdev->sco_pkts)hdev->sco_cnt = hdev->sco_pkts;
Quoted 616 lines · Suraj Sumangala
Quoted 22 lines · Marcel Holtmann
Hi Vikram,
Quoted 18 lines
drivers/bluetooth/Kconfig | 10 + drivers/bluetooth/Makefile | 1 + drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++ drivers/bluetooth/hci_ath.h | 72 ++++++ drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_uart.h | 8 +- 6 files changed, 641 insertions(+), 1 deletions(-) create mode 100755 drivers/bluetooth/hci_ath.c create mode 100755 drivers/bluetooth/hci_ath.hdrivers/bluetooth/Kconfig +0
-0@@ -58,6 +58,16 @@ config BT_HCIUART_BCSP Say Y here to compile support for HCI BCSP protocol.Quoted 16 lines
drivers/bluetooth/Kconfig config BT_HCIUART_BCSP
Quoted 12 lines
drivers/bluetooth/Kconfig +10
-0+config BT_HCIUART_ATH + bool "Atheros AR3001 Board support" + depends on BT_HCIUART + help + HCIATH (HCI Atheros) is a serial protocol for communication + between Bluetooth device and host. This protocol is required for + serial Bluetooth devices that are based on Atheros AR3001 chips. + + Say Y here to compile support for HCIATH protocol. +it would be nice if you add a document explaining this special Atheros protocol that is used.
I will send the document with the required changes
Quoted 38 lines
Quoted 38 lines
drivers/bluetooth/Kconfig +38
-0+static void ath_context_switch(struct work_struct *work) +{ + int status; + struct ath_struct *ath; + struct hci_uart *hu; + struct tty_struct *tty; + struct sk_buff *skbuf; + ath = container_of(work, struct ath_struct, ctxtsw); + hu = ath->hu; + tty = hu->tty; + status = ath_wakeup_ar3001(tty); + if ((status & TIOCM_CTS)) { + while ((skbuf = skb_dequeue(&ath->tx_wait_q))) + skb_queue_tail(&ath->txq, skbuf); + + /* Ready to send Data */ + clear_bit(HCI_UART_SENDING, &hu->tx_state); + hci_uart_tx_wakeup(hu); + } +} + +/* Initialize protocol */ +static int ath_open(struct hci_uart *hu) +{ + struct ath_struct *ath; + BT_DBG("hu %p", hu); + ath = kzalloc(sizeof(*ath), GFP_ATOMIC); + if (!ath) + return -ENOMEM; + skb_queue_head_init(&ath->txq); + skb_queue_head_init(&ath->tx_wait_q); + skb_queue_head_init(&ath->tx_cmd_wait_q); + spin_lock_init(&ath->hciath_lock); + ath->cur_sleep = 0; + ath->num_cmds_complete = 1; + hu->priv = ath; + ath->hu = hu; + init_waitqueue_head(&ath->wqevt);Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 5 lines
drivers/bluetooth/Kconfig +3
-0+ INIT_WORK(&ath->ctxtsw, ath_context_switch); + return 0; +}In all the function, use at least some empty lines in between so that somebody can try to read these function. Cramping everything in less space doesn't make it simpler.
We will make the necessary changes
Quoted 8 lines
drivers/bluetooth/Kconfig
Quoted 4 lines
drivers/bluetooth/Kconfig +2
-0+/* Enqueue frame for transmittion (padding, crc, etc) */ +/* may be called from two simultaneous tasklets */Why are you stealing the comments from
hci_ll.c. Are you using actually padding and CRC?Will do the cleanup along with the other changes mentioned
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +4
-0+static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb) +{ + struct ath_struct *ath; + ath = hu->priv;Direct assignment on declaration.
Will correct it along with other changes
Quoted 7 lines
Quoted 3 lines
+ if (
HCI_SCODATA_PKT== bt_cb(skb)->pkt_type) {Always the other way around. Variable compares to constant. Not the constant to variable.
Will correct it along with other changes
Quoted 13 lines
drivers/bluetooth/Kconfig
Quoted 9 lines
drivers/bluetooth/Kconfig +7
-0+static struct sk_buff *ath_dequeue(struct hci_uart *hu) +{ + struct ath_struct *ath; + struct sk_buff *skbuff; + ath = hu->priv; + skbuff = skb_dequeue(&ath->txq); + if (NULL != skbuff) {Never ever use the variable name skbuff and also if (skbuff) here. However preferable this is if (!skb) return NULL;
I will take care of the variable name change
Quoted 15 lines
Quoted 15 lines
drivers/bluetooth/Kconfig +15
-0+ ath_handle_host_data(ath, bt_cb(skbuff)->pkt_type, + &skbuff->data[1], skbuff->len - 1); + } + return skbuff; +} + +static inline int ath_check_data_len(struct ath_struct *ath, int len) +{ + register int room = skb_tailroom(ath->rx_skb); + BT_DBG("len %d room %d", len, room); + if (!len) { + hci_recv_frame(ath->rx_skb); + } else if (len > room) { + BT_ERR("Data length is too large"); + kfree_skb(ath->rx_skb);Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 7 lines
drivers/bluetooth/Kconfig +5
-0+ } else { + ath->rx_state = HCIATH_W4_DATA; + ath->rx_count = len; + return len; + }How is this if statement even suppose to work?
Retained this code as this was part of the
hci_llimplementation that we used as template. Will make the necessary changes.Quoted 121 lines
Quoted 121 lines
drivers/bluetooth/Kconfig +121
-0+ ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_skb = NULL; + ath->rx_count = 0; + return 0; +} + +/* Recv data */ +static int ath_recv(struct hci_uart *hu, void *data, int count) +{ + struct ath_struct *ath = hu->priv; + register char *ptr; + struct hci_event_hdr *eh; + struct hci_acl_hdr *ah; + struct hci_sco_hdr *sh; + struct sk_buff *skbuff; + register int len, type, dlen; + skbuff = NULL; + BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count, + ath->rx_state, ath->rx_count); + ptr = data; + while (count) { + if (ath->rx_count) { + len = min_t(unsigned int, ath->rx_count, count); + memcpy(skb_put(ath->rx_skb, len), ptr, len); + ath->rx_count -= len; + count -= len; + ptr += len; + if (ath->rx_count) + continue; + switch (ath->rx_state) { + case HCIATH_W4_DATA: + ath_handle_data_from_controller(ath, + bt_cb + (ath->rx_skb)-> + pkt_type, + ath->rx_skb-> + data, + ath->rx_skb-> + len); + if (HCI_EVENT_PKT == + bt_cb(ath->rx_skb)->pkt_type + && ath->num_cmds_complete > 0) { + + skbuff = + skb_dequeue(&ath->tx_cmd_wait_q); + if (skbuff != NULL) { + skb_queue_tail(&ath->txq, + skbuff); + schedule_work(&ath->ctxtsw); + } + } + if (ath_verify_event_discardable + (hu, bt_cb(ath->rx_skb)->pkt_type, + ath->rx_skb->data)) { + kfree(ath->rx_skb); + ath->rx_skb = NULL; + } else { + hci_recv_frame(ath->rx_skb); + } + ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_skb = NULL; + ath->rx_count = 0; + continue; + case HCIATH_W4_EVENT_HDR: + eh = (struct hci_event_hdr *)ath->rx_skb->data; + BT_DBG("Event header: evt 0x%2.2x plen %d", + eh->evt, eh->plen); + ath_check_data_len(ath, eh->plen); + continue; + case HCIATH_W4_ACL_HDR: + ah = (struct hci_acl_hdr *)ath->rx_skb->data; + dlen = __le16_to_cpu(ah->dlen); + BT_DBG("ACL header: dlen %d", dlen); + ath_check_data_len(ath, dlen); + continue; + case HCIATH_W4_SCO_HDR: + sh = (struct hci_sco_hdr *)ath->rx_skb->data; + BT_DBG("SCO header: dlen %d", sh->dlen); + ath_check_data_len(ath, sh->dlen); + continue; + } + } + + /* HCIATH_W4_PACKET_TYPE */ + switch (*ptr) { + case HCI_EVENT_PKT: + BT_DBG("Event packet"); + ath->rx_state = HCIATH_W4_EVENT_HDR; + ath->rx_count = HCI_EVENT_HDR_SIZE; + type = HCI_EVENT_PKT; + break; + case HCI_ACLDATA_PKT: + BT_DBG("ACL packet"); + ath->rx_state = HCIATH_W4_ACL_HDR; + ath->rx_count = HCI_ACL_HDR_SIZE; + type = HCI_ACLDATA_PKT; + break; + case HCI_SCODATA_PKT: + BT_DBG("SCO packet"); + ath->rx_state = HCIATH_W4_SCO_HDR; + ath->rx_count = HCI_SCO_HDR_SIZE; + type = HCI_SCODATA_PKT; + break; + default: + BT_ERR("Unknown HCI packet type %2.2x", (__u8) *ptr); + hu->hdev->stat.err_rx++; + ptr++; + count--; + continue; + }; + ptr++; + count--; + + /* Allocate packet */ + ath->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE, GFP_ATOMIC); + if (!ath->rx_skb) { + BT_ERR("Can't allocate mem for new packet"); + ath->rx_state = HCIATH_W4_PACKET_TYPE; + ath->rx_count = 0; + return 0; + }Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +4
-0+ ath->rx_skb->dev = (void *)hu->hdev; + bt_cb(ath->rx_skb)->pkt_type = type; + } return count; +}We have a common exported function called
hci_recv_fragmentthat should be doing exactly this.I have provided the explanation at the bottom
Quoted 12 lines
drivers/bluetooth/Kconfig
Quoted 8 lines
drivers/bluetooth/Kconfig +6
-0+static void ath_controller_sleep_mode(struct hci_uart *hu, bool enable) +{ + unsigned char sleepcmd[] = { 0x01, 0x04, 0xFC, 0x01, 0x00 }; + sleepcmd[4] = enable; + ath_write_data_to_cntrlr(hu->hdev, sleepcmd, sizeof(sleepcmd)); +}Special vendor commands need a comment. Explain the data structure of them and what are the options.
Will add the necessary comments
Quoted 32 lines
Quoted 32 lines
drivers/bluetooth/Kconfig +32
-0+int ath_wakeup_ar3001(struct tty_struct *tty) +{ + struct termios settings; + int status = 0x00; + mm_segment_t oldfs; + status = tty->driver->ops->tiocmget(tty, NULL); + + if ((status & TIOCM_CTS)) + return status; + + oldfs = get_fs(); + set_fs(KERNEL_DS); + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings); + + settings.c_cflag &= ~CRTSCTS; + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings); + set_fs(oldfs); + status = tty->driver->ops->tiocmget(tty, NULL); + + tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS); + mdelay(20); + + status = tty->driver->ops->tiocmget(tty, NULL); + + tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00); + mdelay(20); + + status = tty->driver->ops->tiocmget(tty, NULL); + oldfs = get_fs(); + set_fs(KERNEL_DS); + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings); +Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 7 lines
drivers/bluetooth/Kconfig +5
-0+ settings.c_cflag |= CRTSCTS; + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings); + set_fs(oldfs); + return status; +}Why is this magic in kernel space and not inside hciattach?
if there is no Bluetooth activity for a while,
so the driver will verify if the controller is sleeping and
try to wake up the controller using the RTS/CTS mechanism.
This has to be done before sending any packet to controller.
This cannot be done in hciattach as this is required to be called throughout the life of the driver and not just when the driver is loaded.Quoted 26 lines
Quoted 26 lines
drivers/bluetooth/Kconfig +26
-0+int ath_fullboot_config(struct hci_uart *hu, int current_event) +{ + struct sk_buff *skbuf; + struct ath_struct *ath = hu->priv; + static int autosleep; + unsigned char rstevt[] = { 0x1, 0x3, 0xc, 0x0 }; + if (current_event == HCI_RESET) { + + if (ath->cur_sleep) { + autosleep = 1; + ath_controller_sleep_mode(hu, 1); + return 1; + } else { + return 0; + } + } + + if (current_event == HCI_SET_SLEEP_MODE) { + + if (autosleep == 0) + return 1; + + while ((skbuf = skb_dequeue(&ath->tx_wait_q))) + skb_queue_tail(&ath->txq, skbuf); + + ath_write_data_to_host(hu->hdev, rstevt, sizeof(rstevt));Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 7 lines
drivers/bluetooth/Kconfig +5
-0+ autosleep = 0; + return 1; + } + return 0; +}Why is this function not static? And all the others?
will make every function static
Quoted 14 lines
Quoted 14 lines
drivers/bluetooth/Kconfig +14
-0+int ath_write_data_to_host(void *dev, unsigned char *data, u8 length) +{ + struct sk_buff *skbuf; + struct hci_dev *hdev; + hdev = (struct hci_dev *)dev; + skbuf = bt_skb_alloc(length, GFP_ATOMIC); + if (!skbuf) { + BT_ERR("Memory allocation failed"); + return -1; + } + skb_orphan(skbuf); + + /* First byte will be added as packet type */ + bt_cb(skbuf)->pkt_type = data[0];Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 7 lines
drivers/bluetooth/Kconfig +5
-0+ skbuf->dev = (void *)hdev; + memcpy(skb_put(skbuf, length - 1), &data[1], length - 1); + hci_recv_frame(skbuf); + return length; +}What is this exactly for? Do you have a secondary receive path?
Yes, we have situation where driver sends HCI events to depending on acknowledgement of sleep enable vendor specific command. the reason is explained at the bottom
Quoted 37 lines
Quoted 37 lines
drivers/bluetooth/Kconfig +37
-0+int ath_write_data_to_cntrlr(void *dev, unsigned char *Data, u8 len) +{ + struct sk_buff *skbuff; + struct ath_struct *ath; + struct hci_uart *hu; + struct hci_dev *hdev; + if (NULL == dev) { + BT_DBG("NULL handle received %p \n", dev); + return 0; + } + hdev = (struct hci_dev *)dev; + hu = (struct hci_uart *)hdev->driver_data; + if (hu == NULL) { + BT_DBG("NULL handle received %p \n", hdev); + return 0; + } + ath = hu->priv; + if (ath == NULL) { + BT_DBG("NULL handle received \n"); + return 0; + } + skbuff = bt_skb_alloc(len, GFP_ATOMIC); + if (skbuff == NULL) { + BT_DBG("Malloc Fail memory %p \n", skbuff); + return 0; + } + skb_orphan(skbuff); + + if (len != 0) + memcpy(skb_put(skbuff, len), Data, len); + + bt_cb(skbuff)->pkt_type = HCI_COMMAND_PKT; + skbuff->dev = dev; + if (ath->num_cmds_complete > 0) { + skb_queue_tail(&ath->txq, skbuff); + schedule_work(&ath->ctxtsw); + } else {Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +4
-0+ skb_queue_tail(&ath->tx_cmd_wait_q, skbuff); + } + return len; +}Messing with
num_cmds_completeinside the driver is wrong. What are you doing here?I have explained the reason at the bottom
Quoted 9 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +4
-0+int ath_check_sleep_cmd(struct ath_struct *ath, unsigned char *packet) +{ + if (packet[0] == HCI_EVENT_PKT && packet[1] == 0xFC) + ath->cur_sleep = packet[3];How is this working. Every vendor event is sleep event?
Will correct it. It should have been packet[0] ==
0x04.Quoted 14 lines
Quoted 14 lines
drivers/bluetooth/Kconfig +14
-0+int ath_verify_event_discardable(struct hci_uart *hu, unsigned char pkt_type, + unsigned char *packet) +{ + if (pkt_type != HCI_EVENT_PKT) + return 0; + + switch (packet[0]) { + case 0x0E: /* Command Complete Event */ + if (packet[3] == 0x03 && packet[4] == 0x0C) { + + /* Command complete for HCI Reset Received */ + return ath_fullboot_config(hu, HCI_RESET); + } else if (packet[3] == 0x04 && packet[4] == 0xFC) { + return ath_fullboot_config(hu, HCI_SET_SLEEP_MODE);Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 7 lines
drivers/bluetooth/Kconfig +5
-0+ } + break; + } + return 0; +}What are you doing here?
Events for vendor specific commands sent from driver need not go to the host. They are discarded at the driver level
Quoted 15 lines
Quoted 15 lines
drivers/bluetooth/Kconfig +15
-0+void ath_handle_hci_event(struct ath_struct *ath, u8 * packet) +{ + switch (packet[0]) { + case 0x05: /* ACL Disconnection Complete Event */ + break; + case 0x03: /* ACL Connection Complete Event */ + break; + case 0x0E: /* Command Complete Event */ + spin_lock(&ath->hciath_lock); + ath->num_cmds_complete = packet[2]; + spin_unlock(&ath->hciath_lock); + break; + case 0x0F: /* Command Status Event */ + spin_lock(&ath->hciath_lock); + ath->num_cmds_complete = packet[3];Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 6 lines
drivers/bluetooth/Kconfig +4
-0+ spin_unlock(&ath->hciath_lock); + break; + } +}Something is really wrong in this driver? Are you really know how this is suppose to work if you have to interfere this much?
I have provided the reason for this implementation at the bottom
Quoted 21 lines
Quoted 21 lines
drivers/bluetooth/Kconfig +21
-0+void ath_handle_host_data(struct ath_struct *ath, u8 pktType, u8 * packet, + unsigned int len) +{ + switch (pktType) { + case HCI_ACLDATA_PKT: /* ACL packets */ + break; + case HCI_COMMAND_PKT: /* HCI Commands */ + ath_handle_hci_cmd(ath, packet); + ath_check_sleep_cmd(ath, packet); + break; + } +} + +void ath_handle_data_from_controller(struct ath_struct *ath, u8 pktType, + u8 *packet, unsigned int len) +{ + switch (pktType) { + case HCI_ACLDATA_PKT: /* ACL packets */ + break; + case HCI_EVENT_PKT: /* HCI Events */ + ath_handle_hci_event(ath, packet);Quoted 10 lines
drivers/bluetooth/Kconfig
Quoted 5 lines
drivers/bluetooth/Kconfig +3
-0+ break; + } +}Seriously? are you implemented your own Bluetooth stack? Please explain what you really need. And then the host stack can provide it for you. This is not acceptable.
The reason for this implementation is.
The AR3001 supports power management feature that can be enabled from host during hciattach.
This will let the controller go to sleep if there is no data transfer for some time. This feature is disabled automatically on receiving HCI RESET command and has to be enabled again by sending Sleep enable Vendor specific command after every HCI Reset command.
This required the driver to keep track of HCI RESET command complete event and sent Sleep Enable command on receiving the event. Host will be notified of the HCI Reset command complete event only after the Sleep enable command is acknowledged by controller. This is the reason for the following implementation changes
1. We had to avoid using
hci_recv_fragmentas we need to verify and decide if the packet has to be sent to Host (In case of HCI RESET).
2. We had to addath_write_data_to_hostso that we can sent HCI RESET
command complete on receiving Sleep Enable command complete event from controller.
3. We had to addath_write_data_to_cntrlrso that driver can send the Sleep enable command after every HCI RESET.
4. Haveath_verify_event_discardablewhich keep track of the HCI RESET and Sleep Enable Command complete event and decide whether to send the HCI RESET event to host or send only after sleep enable.Regarding the use of
num_cmds_completeinside driverDue to a controller specific optimization, we have to send vendor specific commands from the driver to configure the controller depending on the status of ACL connection. This might cause driver to send a command to controller when it is already busy with command sent from host.
That is the reason why we had to go for streamlining the command flow in the driver.
Quoted 18 lines
If you can export
hci_send_cmdAPI inhci_core.c, then we might be able to use it to send a command to controller without worrying about command streamlining.Please do let me know your comments,
Regards
SurajQuoted 7 lines
Regards
Marcel
Hi Marcel,
can you please let me know your comments regarding my answers. I can then have a second look at our implementation based on your comments.
Regards
Suraj
PATCH 1/1 Added support for Atheros AR300x Bluetooth Chip
In reply to Vikram Kandukuri---drivers/bluetooth/Kconfig | 11 ++drivers/bluetooth/Makefile | 1 +drivers/bluetooth/hci_ath.c | 353 +++++++++++++++++++++++++++++++++++++++++ drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_uart.h | 8 +-
5 files changed, 378 insertions(+), 1 deletions(-)
create mode 100755 drivers/bluetooth/hci_ath.c
drivers/bluetooth/Kconfig +0 -0
@@ -58,6 +58,17 @@ config BT_HCIUART_BCSP
Say Y here to compile support for HCI BCSP protocol.
+config BT_HCIUART_ATH
+ bool "Atheros AR300x Board support"
+ depends on BT_HCIUART
+ help
+ HCIATH (HCI Atheros) is a serial protocol for communication
+ between Bluetooth device and host with support for Atheros AR300x
+ power management feature. This protocol is required for
+ serial Bluetooth devices that are based on Atheros AR300x chips. +
+ Say Y here to compile support for Atheros AR300x Chips. +
config BT_HCIUART_LL
bool "HCILL protocol support"
depends on BT_HCIUART
drivers/bluetooth/Makefile +0 -0
@@ -26,4 +26,5 @@ hci_uart-y := hci_ldisc.o
hci_uart-$(CONFIG_BT_HCIUART_H4) += hci_h4.o
hci_uart-$(CONFIG_BT_HCIUART_BCSP) += hci_bcsp.o
hci_uart-$(CONFIG_BT_HCIUART_LL) += hci_ll.o
+hci_uart-$(CONFIG_BT_HCIUART_ATH) += hci_ath.o
hci_uart-objs := $(hci_uart-y)
drivers/bluetooth/hci_ath.c new file +353 -0
353 lines not on this page · raw message
drivers/bluetooth/hci_ldisc.c +0 -0
@@ -542,6 +542,9 @@ static int __init hci_uart_init(void)
#ifdef CONFIG_BT_HCIUART_LLll_init();
#endif
+#ifdef CONFIG_BT_HCIUART_ATH
+ ath_init();
+#endif
return 0;
}
@@ -559,6 +562,9 @@ static void __exit hci_uart_exit(void) #ifdef CONFIG_BT_HCIUART_LLll_deinit();
#endif
+#ifdef CONFIG_BT_HCIUART_ATH
+ ath_deinit();
+#endif
/* Release tty registration of line discipline */
if ((err = tty_unregister_ldisc(N_HCI)))
drivers/bluetooth/hci_uart.h +0 -0
@@ -33,13 +33,14 @@
#define HCIUARTGETDEVICE _IOR('U', 202, int)
/* UART protocols */
-#define HCI_UART_MAX_PROTO 5
+#define HCI_UART_MAX_PROTO 6
#define HCI_UART_H4 0
#define HCI_UART_BCSP 1
#define HCI_UART_3WIRE 2
#define HCI_UART_H4DS 3
#define HCI_UART_LL 4
+#define HCI_UART_ATH 5
struct hci_uart;
@@ -91,3 +92,8 @@ int bcsp_deinit(void);
int ll_init(void);
int ll_deinit(void);
#endif
+
+#ifdef CONFIG_BT_HCIUART_ATH
+int ath_init(void);
+int ath_deinit(void);
+#endif
Signature · 1 line
1.6.3.3
Marcel Holtmann
maintainerRe: [PATCH] Added support for Atheros AR300x Bluetooth Chip
Hi Suraj,
Quoted 19 lines
Signed-off-by Suraj <[email protected]>---
drivers/bluetooth/Kconfig| 11 ++drivers/bluetooth/Makefile| 1 +drivers/bluetooth/hci_ath.c| 353 +++++++++++++++++++++++++++++++++++++++++drivers/bluetooth/hci_ldisc.c| 6 +drivers/bluetooth/hci_uart.h| 8 +-5 files changed, 378 insertions(+), 1 deletions(-)create mode 100755
drivers/bluetooth/hci_ath.cdrivers/bluetooth/Kconfig +0
-0@@ -58,6 +58,17 @@ config BT_HCIUART_BCSPSay Y here to compile support for HCI BCSP protocol.
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+config BT_HCIUART_ATH + bool "Atheros AR300x Board support" + depends on BT_HCIUART + help + HCIATH (HCI Atheros) is a serial protocol for communication + between Bluetooth device and host with support for Atheros AR300x + power management feature. This protocol is required for + serial Bluetooth devices that are based on Atheros AR300x chips. + + Say Y here to compile support for Atheros AR300x Chips.
your patch is messed up. Make sure your mailer doesn't mess with tabs and whitespaces.
Regards
Marcel
Re: [PATCH] Added support for Atheros AR300x Bluetooth Chip
Quoted 21 lines · Marcel Holtmann
Hi Suraj,
Quoted 19 lines
Signed-off-by Suraj <[email protected]>---
drivers/bluetooth/Kconfig| 11 ++drivers/bluetooth/Makefile| 1 +drivers/bluetooth/hci_ath.c| 353 +++++++++++++++++++++++++++++++++++++++++drivers/bluetooth/hci_ldisc.c| 6 +drivers/bluetooth/hci_uart.h| 8 +-5 files changed, 378 insertions(+), 1 deletions(-)create mode 100755
drivers/bluetooth/hci_ath.cdrivers/bluetooth/Kconfig +0
-0@@ -58,6 +58,17 @@ config BT_HCIUART_BCSPSay Y here to compile support for HCI BCSP protocol.
Quoted 13 lines
drivers/bluetooth/Kconfig
Quoted 10 lines
drivers/bluetooth/Kconfig +10
-0+config BT_HCIUART_ATH + bool "Atheros AR300x Board support" + depends on BT_HCIUART + help + HCIATH (HCI Atheros) is a serial protocol for communication + between Bluetooth device and host with support for Atheros AR300x + power management feature. This protocol is required for + serial Bluetooth devices that are based on Atheros AR300x chips. + + Say Y here to compile support for Atheros AR300x Chips.your patch is messed up. Make sure your mailer doesn't mess with tabs and whitespaces.
I had issues with our Exchange server for patches but that was only for receiving patches [1], but that is now fixed. I don't think the SMTP server should screw with patches on their way out. Suraj, did you actually intend for the patch to go out as is?
[1] http://bombadil.infradead.org/~mcgrof/MS-exchange-sucks-for-patches/
Luis