Return-path: Received: from mail-qk0-f195.google.com ([209.85.220.195]:35516 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751903AbdK3OXP (ORCPT ); Thu, 30 Nov 2017 09:23:15 -0500 MIME-Version: 1.0 In-Reply-To: References: <1510730444-3237-1-git-send-email-amitkarwar@gmail.com> <1510730444-3237-6-git-send-email-amitkarwar@gmail.com> <6E0D3434-F2E9-4B4D-B66E-78DE921BB2CF@holtmann.org> From: Amitkumar Karwar Date: Thu, 30 Nov 2017 19:53:13 +0530 Message-ID: (sfid-20171130_152356_637825_EFAC8A7C) Subject: Re: [PATCH 5/8] Bluetooth: btrsi: add new rsi bluetooth driver To: Marcel Holtmann Cc: Kalle Valo , linux-wireless , Amitkumar Karwar , Prameela Rani Garnepudi , "open list:BLUETOOTH DRIVERS" , Siva Rebbagondla Content-Type: text/plain; charset="UTF-8" Sender: linux-wireless-owner@vger.kernel.org List-ID: On Wed, Nov 29, 2017 at 7:03 PM, Marcel Holtmann wrot= e: > 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 >>>> Signed-off-by: Siva Rebbagondla >>>> Signed-off-by: Amitkumar Karwar >>>> --- >>>> 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 th= e >>>> 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 unneed= ed since that is already covered with the above menu clause. >>> >>> >>>> + default m >>> >>> No. New drivers are by default off. >> >> We made it by default on, as RSI core module depends on this. >> >>> >>>> + 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 t= he >>>> + kernel or say M to compile as a module. >>>> + >>> >>> What I am missing is the depends on the RSI core piece that is needed. >> >> Actually RSI core module depends on RSI BT module as per our design. >> As soon as firmware is downloaded by RSI core module, we get WLAN and >> BT card ready frames from firmware. BT initialization has to happen at >> this point. So btrsi module should be loaded first. > > that should be solved by the dependencies and not by a default m. Understood. I will take care of it and submit v3. > >> >>> >>>> 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) +=3D btqca.o >>>> >>>> obj-$(CONFIG_BT_HCIUART_NOKIA) +=3D hci_nokia.o >>>> >>>> +obj-$(CONFIG_BT_RSI) +=3D btrsi.o >>>> + >>>> btmrvl-y :=3D btmrvl_main.o >>>> btmrvl-$(CONFIG_DEBUG_FS) +=3D 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 f= or any >>>> + * purpose with or without fee is hereby granted, provided that the a= bove >>>> + * copyright notice and this permission notice appear in all copies. >>>> + * >>>> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARR= ANTIES >>>> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF >>>> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABL= E FOR >>>> +e* ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAM= AGES >>>> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN= AN >>>> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING O= UT OF >>>> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. >>>> + */ >>>> +#include >>>> +#include >>>> +#include >>>> +#include "rsi_hci.h" >>>> + >>>> +static struct rsi_mod_ops rsi_bt_ops =3D { >>>> + .attach =3D rsi_hci_attach, >>>> + .detach =3D rsi_hci_detach, >>>> + .recv_pkt =3D 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; >>> >>> =3D hci_get_drvdata(dev); >>> >>> There is no need to do that separate. >>> >>>> + struct sk_buff *new_skb =3D NULL; >>>> + >>>> + int status =3D 0; >>>> + >>>> + h_adapter =3D hci_get_drvdata(hdev); >>>> + if (!h_adapter) { >>>> + status =3D -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 !=3D RSI_BT_FSM_DEVICE_READY) { >>>> + BT_INFO("BT Device not ready\n"); >>>> + status =3D -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 =3D -EBUSY; >>>> + goto fail; >>>> + } >>> >>> This should no longer be needed. We moved this into the core and beside= s 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=E2=80=99t use dev_kfree_skb() here. I think the Bluetooth subsystem= is not ready for that. >>> >>>> + status =3D -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 r= oom */ >>>> + u16 new_len =3D skb->len + RSI_HEADROOM_FOR_BT_HAL; >>>> + >>>> + new_skb =3D 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 =3D bt_cb(skb)->pkt_type; >>>> + dev_kfree_skb(skb); >>>> + skb =3D new_skb; >>> >>> Just allocate a new skb with proper headroom. The data part can be shar= ed. Doing this fully manual is just a bad idea. >>> >>> I also get the feeling that long term we want to change the Bluetooth d= rivers to specify the required SKB headroom so that they always get SKBs th= at are large enough for their needs. >>> >>>> + } >>>> + if (h_adapter->proto_ops->coex_send_pkt) >>>> + return h_adapter->proto_ops->coex_send_pkt(h_adapter->pr= iv, 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 =3D (struct rsi_hci_adapter *)= priv; >>>> + struct hci_dev *hdev =3D NULL; >>>> + struct sk_buff *skb; >>>> + int pkt_len =3D (le16_to_cpu(*(__le16 *)pkt)) & 0x0fff; >>>> + u8 queue_no =3D (le16_to_cpu(*(__le16 *)pkt) & 0x7000) >> 12; >>> >>> We have get_unaligned_le16 helpers for this. >>> >>>> + >>>> + if (h_adapter->fsm_state !=3D 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 chec= ked and the HCI device not be registered. >>> >>>> + >>>> + if (queue_no =3D=3D RSI_BT_MGMT_Q) { >>>> + u8 msg_type =3D 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 h= ere bt_dev_info is preferred anyway. >>> >>>> + >>>> + skb =3D dev_alloc_skb(pkt_len); >>>> + if (!skb) >>>> + return -ENOMEM; >>>> + >>>> + hdev =3D h_adapter->hdev; >>>> + memcpy(skb->data, pkt + RSI_FRAME_DESC_SIZE, pkt_len); >>>> + skb_put(skb, pkt_len); >>>> + h_adapter->hdev->stat.byte_rx +=3D skb->len; >>>> + >>>> + skb->dev =3D (void *)hdev; >>>> + bt_cb(skb)->pkt_type =3D 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 =3D NULL; >>>> + struct hci_dev *hdev; >>>> + int status =3D 0; >>>> + >>>> + h_adapter =3D kzalloc(sizeof(*h_adapter), GFP_KERNEL); >>>> + if (!h_adapter) >>>> + return -ENOMEM; >>>> + >>>> + h_adapter->priv =3D priv; >>>> + ops->set_bt_context(priv, h_adapter); >>>> + h_adapter->proto_ops =3D ops; >>>> + ops->bt_ops =3D &rsi_bt_ops; >>>> + h_adapter->fsm_state =3D RSI_BT_FSM_DEVICE_READY; >>>> + >>>> + hdev =3D hci_alloc_dev(); >>>> + if (!hdev) { >>>> + BT_ERR("Failed to alloc HCI device\n"); >>>> + goto err; >>>> + } >>>> + h_adapter->hdev =3D hdev; >>>> + >>>> + if (ops->get_host_intf(priv) =3D=3D RSI_HOST_INTF_SDIO) >>>> + hdev->bus =3D HCI_SDIO; >>>> + else >>>> + hdev->bus =3D HCI_USB; >>>> + >>>> + hci_set_drvdata(hdev, h_adapter); >>>> + hdev->dev_type =3D HCI_PRIMARY; >>>> + hdev->open =3D rsi_hci_open; >>>> + hdev->close =3D rsi_hci_close; >>>> + hdev->flush =3D rsi_hci_flush; >>>> + hdev->send =3D rsi_hci_send_pkt; >>>> + >>>> + status =3D 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 =3D RSI_BT_FSM_DEVICE_READY; >>>> + >>>> + return 0; >>>> + >>>> +err: >>>> + if (hdev) { >>>> + hci_unregister_dev(hdev); >>>> + hci_free_dev(hdev); >>>> + h_adapter->hdev =3D NULL; >>>> + } >>>> + kfree(h_adapter); >>>> + >>>> + return -EINVAL; >>>> +} >>>> + >>>> +void rsi_hci_detach(void *priv) >>>> +{ >>>> + struct rsi_hci_adapter *h_adapter =3D (struct rsi_hci_adapter *)= priv; >>>> + struct hci_dev *hdev; >>>> + >>>> + if (!h_adapter) >>>> + return; >>>> + >>>> + hdev =3D h_adapter->hdev; >>>> + >>>> + BT_INFO("Detaching %s\n", hdev->name); >>>> + >>>> + if (hdev) { >>>> + hci_unregister_dev(hdev); >>>> + hci_free_dev(hdev); >>>> + h_adapter->hdev =3D NULL; >>>> + } >>>> + >>>> + kfree(h_adapter); >>>> + h_adapter->fsm_state =3D 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 f= or any >>>> + * purpose with or without fee is hereby granted, provided that the a= bove >>>> + * copyright notice and this permission notice appear in all copies. >>>> + * >>>> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARR= ANTIES >>>> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF >>>> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABL= E FOR >>>> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAM= AGES >>>> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN= AN >>>> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING O= UT OF >>>> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. >>>> + */ >>>> + >>>> +#ifndef __RSI_HCI_H__ >>>> +#define __RSI_HCI_H__ >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +/* 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 =3D 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 he= ader 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. >> >> rsi core module initializes rsi_bt_ops by calling exported function >> rsi_get_hci_ops(). > > Still exporting both is pointless. There are a few too many exports / pub= lic functions here. Make as much static as possible. Sure. I have now removed unnecessary exports. I will submit v3 patch series= . Regards, Amitkumar