Re: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip

Hi Vikram,

it would be nice if you add a document explaining this special Atheros
protocol that is used.

+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;
+}

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.

+/* 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?

+static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
+{
+	struct ath_struct *ath;
+	ath = hu->priv;

Direct assignment on declaration.

+	if (HCI_SCODATA_PKT == bt_cb(skb)->pkt_type) {

Always the other way around. Variable compares to constant. Not the
constant to variable.

+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;

+		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);
+	} else {
+		ath->rx_state = HCIATH_W4_DATA;
+		ath->rx_count = len;
+		return len;
+	}

How is this if statement even suppose to work?

+	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;
+		}
+		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.

+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.

+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;
+}

Why is this magic in kernel space and not inside hciattach?

+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;
+}

Why is this function not static? And all the others?

+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;
+}

What is this exactly for? Do you have a secondary receive path?

+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;
+}

Messing with num_cmds_complete inside the driver is wrong. What are you
doing here?

+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?

+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;
+}

What are you doing here?

+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;
+	}
+}

Something is really wrong in this driver? Are you really know how this
is suppose to work if you have to interfere this much?

+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;
+	}
+}

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