On Wed, Nov 15, 2017 at 9:07 PM, Marcel Holtmann <mar...@holtmann.org> wrote: > Hi Amitkumar, > >> Redpine bluetooth driver is a thin driver which depends on >> 'rsi_91x' driver for transmitting and receiving packets >> to/from device. It creates hci interface when attach() is >> called from 'rsi_91x' module. >> >> Signed-off-by: Prameela Rani Garnepudi <prameela.j0...@gmail.com> >> Signed-off-by: Siva Rebbagondla <siva.rebbagon...@redpinesignals.com> >> Signed-off-by: Amitkumar Karwar <amit.kar...@redpinesignals.com> >> --- >> drivers/bluetooth/Kconfig | 12 ++ >> drivers/bluetooth/Makefile | 2 + >> drivers/bluetooth/btrsi.c | 268 >> ++++++++++++++++++++++++++++++++++++++++++++ >> drivers/bluetooth/rsi_hci.h | 51 +++++++++ >> 4 files changed, 333 insertions(+) >> create mode 100644 drivers/bluetooth/btrsi.c >> create mode 100644 drivers/bluetooth/rsi_hci.h >> >> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig >> index 60e1c7d..ca58d74 100644 >> --- a/drivers/bluetooth/Kconfig >> +++ b/drivers/bluetooth/Kconfig >> @@ -378,4 +378,16 @@ config BT_QCOMSMD >> Say Y here to compile support for HCI over Qualcomm SMD into the >> kernel or say M to compile as a module. >> >> +config BT_RSI >> + tristate "Redpine HCI support" >> + depends on BT && BT_RFCOMM > > the depends on BT_RFCOMM is wrong. And even the depends on BT is unneeded > since that is already covered with the above menu clause. > > >> + default m > > No. New drivers are by default off. > >> + help >> + Redpine BT driver. >> + This driver handles BT traffic from upper layers and pass >> + to the RSI_91x coex module for further scheduling to device >> + >> + Say Y here to compile support for HCI over Qualcomm SMD into the >> + kernel or say M to compile as a module. >> + > > What I am missing is the depends on the RSI core piece that is needed. > >> endmenu >> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile >> index 4e4e44d..712af83a 100644 >> --- a/drivers/bluetooth/Makefile >> +++ b/drivers/bluetooth/Makefile >> @@ -28,6 +28,8 @@ obj-$(CONFIG_BT_QCA) += btqca.o >> >> obj-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o >> >> +obj-$(CONFIG_BT_RSI) += btrsi.o >> + >> btmrvl-y := btmrvl_main.o >> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o >> >> diff --git a/drivers/bluetooth/btrsi.c b/drivers/bluetooth/btrsi.c >> new file mode 100644 >> index 0000000..c52f418 >> --- /dev/null >> +++ b/drivers/bluetooth/btrsi.c >> @@ -0,0 +1,268 @@ >> +/** >> + * Copyright (c) 2017 Redpine Signals Inc. >> + * >> + * Permission to use, copy, modify, and/or distribute this software for any >> + * purpose with or without fee is hereby granted, provided that the above >> + * copyright notice and this permission notice appear in all copies. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES >> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF >> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR >> +e* ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES >> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN >> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF >> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. >> + */ >> +#include <linux/version.h> >> +#include <linux/module.h> >> +#include <linux/kernel.h> >> +#include "rsi_hci.h" >> + >> +static struct rsi_mod_ops rsi_bt_ops = { >> + .attach = rsi_hci_attach, >> + .detach = rsi_hci_detach, >> + .recv_pkt = rsi_hci_recv_pkt, >> +}; >> + >> +static int rsi_hci_open(struct hci_dev *hdev) >> +{ >> + BT_INFO("%s open\n", hdev->name); > > No BT_INFO here. We are not spamming dmesg with pointless information. And > this applies to all of these. > >> + >> + return 0; >> +} >> + >> +static int rsi_hci_close(struct hci_dev *hdev) >> +{ >> + BT_INFO("%s closed\n", hdev->name); >> + >> + return 0; >> +} >> + >> +static int rsi_hci_flush(struct hci_dev *hdev) >> +{ >> + BT_ERR("%s flush\n", hdev->name); >> + >> + return 0; >> +} >> + >> +static int rsi_hci_send_pkt(struct hci_dev *hdev, struct sk_buff *skb) >> +{ >> + struct rsi_hci_adapter *h_adapter; > > = hci_get_drvdata(dev); > > There is no need to do that separate. > >> + struct sk_buff *new_skb = NULL; >> + >> + int status = 0; >> + >> + h_adapter = hci_get_drvdata(hdev); >> + if (!h_adapter) { >> + status = -EFAULT; >> + goto fail; >> + } > > And this error check seems unneeded since the Bluetooth core will never call > ->send unless it is all correctly set up. > >> + >> + if (h_adapter->fsm_state != RSI_BT_FSM_DEVICE_READY) { >> + BT_INFO("BT Device not ready\n"); >> + status = -ENODEV; >> + goto fail; >> + } > > Why are we registering a HCI device that might not be ready. This will be a > massive bug in the whole system. Also one that is not recoverable. If > something goes wrong, then lets unregister the HCI device. > >> + >> + if (!test_bit(HCI_RUNNING, &hdev->flags)) { >> + status = -EBUSY; >> + goto fail; >> + } > > This should no longer be needed. We moved this into the core and besides some > old USB drivers, nobody is using this anymore. > >> + >> + switch (bt_cb(skb)->pkt_type) { >> + case HCI_COMMAND_PKT: >> + hdev->stat.cmd_tx++; >> + break; >> + >> + case HCI_ACLDATA_PKT: >> + hdev->stat.acl_tx++; >> + break; >> + >> + case HCI_SCODATA_PKT: >> + hdev->stat.sco_tx++; >> + break; >> + >> + default: >> + dev_kfree_skb(skb); > > Don’t use dev_kfree_skb() here. I think the Bluetooth subsystem is not ready > for that. > >> + status = -EILSEQ; >> + goto fail; >> + } >> + >> + if (skb_headroom(skb) < RSI_HEADROOM_FOR_BT_HAL) { >> + /* Re-allocate one more skb with sufficent headroom */ >> + /* Space for Descriptor (16 bytes) is required in head room */ >> + u16 new_len = skb->len + RSI_HEADROOM_FOR_BT_HAL; >> + >> + new_skb = dev_alloc_skb(new_len); >> + if (!new_skb) { >> + dev_kfree_skb(skb); >> + return -ENOMEM; >> + } >> + skb_reserve(new_skb, RSI_HEADROOM_FOR_BT_HAL); >> + skb_put(new_skb, skb->len); >> + memcpy(new_skb->data, skb->data, skb->len); >> + bt_cb(new_skb)->pkt_type = bt_cb(skb)->pkt_type; >> + dev_kfree_skb(skb); >> + skb = new_skb; > > Just allocate a new skb with proper headroom. The data part can be shared. > Doing this fully manual is just a bad idea. > > I also get the feeling that long term we want to change the Bluetooth drivers > to specify the required SKB headroom so that they always get SKBs that are > large enough for their needs. > >> + } >> + if (h_adapter->proto_ops->coex_send_pkt) >> + return h_adapter->proto_ops->coex_send_pkt(h_adapter->priv, >> skb, >> + RSI_BT_Q); > > This is worrisome. You need to send. If this callback is not available, then > better not register the HCI device at all. > >> + >> +fail: >> + dev_kfree_skb(skb); >> + return status; >> +} >> + >> +int rsi_hci_recv_pkt(void *priv, u8 *pkt) >> +{ >> + struct rsi_hci_adapter *h_adapter = (struct rsi_hci_adapter *)priv; >> + struct hci_dev *hdev = NULL; >> + struct sk_buff *skb; >> + int pkt_len = (le16_to_cpu(*(__le16 *)pkt)) & 0x0fff; >> + u8 queue_no = (le16_to_cpu(*(__le16 *)pkt) & 0x7000) >> 12; > > We have get_unaligned_le16 helpers for this. > >> + >> + if (h_adapter->fsm_state != RSI_BT_FSM_DEVICE_READY) { >> + BT_INFO("BT Device not ready\n"); >> + return 0; >> + } > > Why call this function if the device is not ready? Can that not be checked > and the HCI device not be registered. > >> + >> + if (queue_no == RSI_BT_MGMT_Q) { >> + u8 msg_type = pkt[14] & 0xFF; >> + >> + switch (msg_type) { >> + case RSI_RESULT_CONFIRM: >> + BT_INFO("BT Result Confirm\n"); >> + return 0; >> + case RSI_BT_BER: >> + BT_INFO("BT Ber\n"); >> + return 0; >> + case RSI_BT_CW: >> + BT_INFO("BT CW\n"); >> + return 0; >> + default: >> + break; >> + } >> + } > > How noisy is this? Also no \n required for the BT_INFO functions. And here > bt_dev_info is preferred anyway. > >> + >> + skb = dev_alloc_skb(pkt_len); >> + if (!skb) >> + return -ENOMEM; >> + >> + hdev = h_adapter->hdev; >> + memcpy(skb->data, pkt + RSI_FRAME_DESC_SIZE, pkt_len); >> + skb_put(skb, pkt_len); >> + h_adapter->hdev->stat.byte_rx += skb->len; >> + >> + skb->dev = (void *)hdev; >> + bt_cb(skb)->pkt_type = pkt[14]; >> + >> + return hci_recv_frame(hdev, skb); >> +} >> +EXPORT_SYMBOL_GPL(rsi_hci_recv_pkt); >> + >> +int rsi_hci_attach(void *priv, struct rsi_proto_ops *ops) >> +{ >> + struct rsi_hci_adapter *h_adapter = NULL; >> + struct hci_dev *hdev; >> + int status = 0; >> + >> + h_adapter = kzalloc(sizeof(*h_adapter), GFP_KERNEL); >> + if (!h_adapter) >> + return -ENOMEM; >> + >> + h_adapter->priv = priv; >> + ops->set_bt_context(priv, h_adapter); >> + h_adapter->proto_ops = ops; >> + ops->bt_ops = &rsi_bt_ops; >> + h_adapter->fsm_state = RSI_BT_FSM_DEVICE_READY; >> + >> + hdev = hci_alloc_dev(); >> + if (!hdev) { >> + BT_ERR("Failed to alloc HCI device\n"); >> + goto err; >> + } >> + h_adapter->hdev = hdev; >> + >> + if (ops->get_host_intf(priv) == RSI_HOST_INTF_SDIO) >> + hdev->bus = HCI_SDIO; >> + else >> + hdev->bus = HCI_USB; >> + >> + hci_set_drvdata(hdev, h_adapter); >> + hdev->dev_type = HCI_PRIMARY; >> + hdev->open = rsi_hci_open; >> + hdev->close = rsi_hci_close; >> + hdev->flush = rsi_hci_flush; >> + hdev->send = rsi_hci_send_pkt; >> + >> + status = hci_register_dev(hdev); >> + if (status < 0) { >> + BT_ERR("%s: HCI registration failed with errcode %d\n", >> + __func__, status); >> + goto err; >> + } >> + >> + BT_INFO("%s interface created\n", hdev->name); >> + >> + h_adapter->fsm_state = RSI_BT_FSM_DEVICE_READY; >> + >> + return 0; >> + >> +err: >> + if (hdev) { >> + hci_unregister_dev(hdev); >> + hci_free_dev(hdev); >> + h_adapter->hdev = NULL; >> + } >> + kfree(h_adapter); >> + >> + return -EINVAL; >> +} >> + >> +void rsi_hci_detach(void *priv) >> +{ >> + struct rsi_hci_adapter *h_adapter = (struct rsi_hci_adapter *)priv; >> + struct hci_dev *hdev; >> + >> + if (!h_adapter) >> + return; >> + >> + hdev = h_adapter->hdev; >> + >> + BT_INFO("Detaching %s\n", hdev->name); >> + >> + if (hdev) { >> + hci_unregister_dev(hdev); >> + hci_free_dev(hdev); >> + h_adapter->hdev = NULL; >> + } >> + >> + kfree(h_adapter); >> + h_adapter->fsm_state = RSI_BT_FSM_DEVICE_NOT_READY; >> +} >> + >> +struct rsi_mod_ops *rsi_get_hci_ops(void) >> +{ >> + return &rsi_bt_ops; >> +}; >> +EXPORT_SYMBOL_GPL(rsi_get_hci_ops); >> + >> +static int rsi_91x_bt_module_init(void) >> +{ >> + BT_INFO("%s: BT Module init called\n", __func__); >> + >> + return 0; >> +} >> + >> +static void rsi_91x_bt_module_exit(void) >> +{ >> + BT_INFO("%s: BT Module exit called\n", __func__); >> +} >> + >> +module_init(rsi_91x_bt_module_init); >> +module_exit(rsi_91x_bt_module_exit); >> +MODULE_AUTHOR("Redpine Signals Inc"); >> +MODULE_DESCRIPTION("RSI BT driver"); >> +MODULE_SUPPORTED_DEVICE("RSI-BT"); >> +MODULE_LICENSE("Dual BSD/GPL"); >> diff --git a/drivers/bluetooth/rsi_hci.h b/drivers/bluetooth/rsi_hci.h >> new file mode 100644 >> index 0000000..6f44231 >> --- /dev/null >> +++ b/drivers/bluetooth/rsi_hci.h >> @@ -0,0 +1,51 @@ >> +/** >> + * Copyright (c) 2017 Redpine Signals Inc. >> + * >> + * Permission to use, copy, modify, and/or distribute this software for any >> + * purpose with or without fee is hereby granted, provided that the above >> + * copyright notice and this permission notice appear in all copies. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES >> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF >> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR >> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES >> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN >> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF >> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. >> + */ >> + >> +#ifndef __RSI_HCI_H__ >> +#define __RSI_HCI_H__ >> + >> +#include <net/bluetooth/bluetooth.h> >> +#include <net/bluetooth/hci_core.h> >> +#include <linux/rsi_header.h> >> +#include <net/genetlink.h> >> + >> +/* RX frame types */ >> +#define RSI_RESULT_CONFIRM 0x80 >> +#define RSI_BT_PER 0x10 >> +#define RSI_BT_BER 0x11 >> +#define RSI_BT_CW 0x12 >> + >> +#define RSI_HEADROOM_FOR_BT_HAL 16 >> + >> +enum bt_fsm_state { >> + RSI_BT_FSM_DEVICE_NOT_READY = 0, >> + RSI_BT_FSM_DEVICE_READY, >> +}; >> + >> +struct rsi_hci_adapter { >> + void *priv; >> + struct rsi_proto_ops *proto_ops; >> + enum bt_fsm_state fsm_state; >> + struct hci_dev *hdev; >> +}; > > If they are btrsi.c specific, they should be in btrsi.c and not in a header > file. > >> + >> +#define RSI_FRAME_DESC_SIZE 16 >> + >> +int rsi_hci_attach(void *priv, struct rsi_proto_ops *ops); >> +void rsi_hci_detach(void *priv); >> +int rsi_hci_recv_pkt(void *data, u8 *pkt); > > And this is the part I do not get. What is the deal with the rsi_bt_ops. > Having both exported is pointless.
Thanks for your review. We will prepare updated version which addresses these comments. Regards, Amitkumar Karwar