RE: [v11,3/3] Bluetooth: hci_uart: Support firmware download for Marvell
Hi Loic, > -Original Message- > From: Loic Poulain [mailto:loic.poul...@intel.com] > Sent: Thursday, June 30, 2016 4:24 PM > To: Amitkumar Karwar; Jeffy Chen; linux-blueto...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; Ganapathi Bhat; Cathy Luo; Marcel > Holtmann > Subject: Re: [v11,3/3] Bluetooth: hci_uart: Support firmware download > for Marvell > > Hi Amitkumar, > > > > Hi Marcel, > > I suggest you to add Marcel as recipient of your patches. > > > > >> From: Jeffy Chen [mailto:jeffy.c...@rock-chips.com] > >> Sent: Friday, June 24, 2016 11:32 AM > >> To: Amitkumar Karwar; linux-blueto...@vger.kernel.org > >> Cc: linux-kernel@vger.kernel.org; Ganapathi Bhat > >> Subject: Re: [v11,3/3] Bluetooth: hci_uart: Support firmware download > >> for Marvell > >> > >> On 2016-5-6 23:31, Amitkumar Karwar wrote: > >>> From: Ganapathi Bhat > >>> > >>> This patch implement firmware download feature for Marvell Bluetooth > >>> devices. If firmware is already downloaded, it will skip > downloading. > > > >>> +static struct sk_buff *mrvl_process_fw_data(struct hci_uart *hu, > >>> + struct sk_buff *skb, > >>> + u8 *buf, int count) > >>> +{ > >>> + struct mrvl_data *mrvl = hu->priv; > >>> + struct fw_data *fw_data = mrvl->fwdata; > >>> + int i = 0, len; > >>> + > >>> + if (!skb) { > >>> + while (buf[i] != fw_data->expected_ack && i < count) > >>> + i++; > >>> + if (i == count) > >>> + return ERR_PTR(-EILSEQ); > >>> + > >>> + skb = bt_skb_alloc(MRVL_FW_HDR_LEN, GFP_KERNEL); > > Why you don't test skb here. > > >>> + } > >>> + > >>> + if (!skb) > >>> + return ERR_PTR(-ENOMEM); > >>> + > >>> + len = count - i; > >>> + memcpy(skb_put(skb, len), &buf[i], len); > > You copy all the remaining data from buf into your skb, but what if buf > contains more than one packet ? out of skb. > Don't assume that buf contains a full packet as well as only one packet. > > Thanks for your comments. We have added necessarily checks to address these comments. I will submit updated version shortly. Regards, Amitkumar Karwar
Re: [v11,3/3] Bluetooth: hci_uart: Support firmware download for Marvell
Hi Amitkumar, Hi Marcel, I suggest you to add Marcel as recipient of your patches. From: Jeffy Chen [mailto:jeffy.c...@rock-chips.com] Sent: Friday, June 24, 2016 11:32 AM To: Amitkumar Karwar; linux-blueto...@vger.kernel.org Cc: linux-kernel@vger.kernel.org; Ganapathi Bhat Subject: Re: [v11,3/3] Bluetooth: hci_uart: Support firmware download for Marvell On 2016-5-6 23:31, Amitkumar Karwar wrote: From: Ganapathi Bhat This patch implement firmware download feature for Marvell Bluetooth devices. If firmware is already downloaded, it will skip downloading. +static struct sk_buff *mrvl_process_fw_data(struct hci_uart *hu, + struct sk_buff *skb, + u8 *buf, int count) +{ + struct mrvl_data *mrvl = hu->priv; + struct fw_data *fw_data = mrvl->fwdata; + int i = 0, len; + + if (!skb) { + while (buf[i] != fw_data->expected_ack && i < count) + i++; + if (i == count) + return ERR_PTR(-EILSEQ); + + skb = bt_skb_alloc(MRVL_FW_HDR_LEN, GFP_KERNEL); Why you don't test skb here. + } + + if (!skb) + return ERR_PTR(-ENOMEM); + + len = count - i; + memcpy(skb_put(skb, len), &buf[i], len); You copy all the remaining data from buf into your skb, but what if buf contains more than one packet ? out of skb. Don't assume that buf contains a full packet as well as only one packet. Regards, Loic
RE: [v11,3/3] Bluetooth: hci_uart: Support firmware download for Marvell
Hi Marcel, > From: Jeffy Chen [mailto:jeffy.c...@rock-chips.com] > Sent: Friday, June 24, 2016 11:32 AM > To: Amitkumar Karwar; linux-blueto...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; Ganapathi Bhat > Subject: Re: [v11,3/3] Bluetooth: hci_uart: Support firmware download > for Marvell > > On 2016-5-6 23:31, Amitkumar Karwar wrote: > > From: Ganapathi Bhat > > > > This patch implement firmware download feature for Marvell Bluetooth > > devices. If firmware is already downloaded, it will skip downloading. > > > > Signed-off-by: Ganapathi Bhat > > Signed-off-by: Amitkumar Karwar > > --- > > v2: Fixed compilation warning reported by kbuild test robot > > v3: Addressed review comments from Marcel Holtmann > > a) Removed vendor specific code from hci_ldisc.c > > b) Get rid of static forward declaration > > c) Removed unnecessary heavy nesting > > d) Git rid of module parameter and global variables > > e) Add logic to pick right firmware image > > v4: Addresses review comments from Alan > > a) Use existing kernel helper APIs instead of writing own. > > b) Replace mdelay() with msleep() > > v5: Addresses review comments from Loic Poulain > > a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG > > b) Used static functions where required and removed forward > delcarations > > c) Edited comments for the function hci_uart_recv_data > > d) Made HCI_UART_DNLD_FW flag a part of driver private data > > v6: Addresses review comments from Loic Poulain > > a) Used skb instead of array to store firmware data during > download > > b) Used hci_uart_tx_wakeup and enqueued packets instead of tty > write > > c) Used GFP_KERNEL instead of GFP_ATOMIC > > v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change > resolves > > errors reported by kbuild test robot. > > v8: Addressed review comments from Marcel Holtmann > > a) Removed unnecessary memory allocation failure messages > > b) Get rid of btmrvl.h header file and add definitions in > > hci_mrvl.c file > > v9: Addressed review comments from Marcel Holtmann > > a) Moved firmware download code from setup to prepare handler. > > b) Change messages from bt_dev_*->BT_*, as hdev isn't available > during firmware > > download. > > v10: Addressed review comments from Marcel Holtmann > > a) Added new callback recv_for_prepare to receive data from > device > > during prepare phase > > b) Avoided using private flags (HCI_UART_DNLD_FW) as new receive > callback is > > added for the same purpose > > c) Used kernel API to handle unaligned data > > d) Moved mrvl_set_baud functionality inside setup callback > > v11: Write data through ldisc in mrvl_send_ack() instead of directly > calling > > write method(One Thousand Gnomes). > > --- > > drivers/bluetooth/Kconfig | 11 + > > drivers/bluetooth/Makefile| 1 + > > drivers/bluetooth/hci_ldisc.c | 6 + > > drivers/bluetooth/hci_mrvl.c | 543 > ++ > > drivers/bluetooth/hci_uart.h | 8 +- > > 5 files changed, 568 insertions(+), 1 deletion(-) > > create mode 100644 drivers/bluetooth/hci_mrvl.c > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig > > index cf50fd2..daafd0c 100644 > > --- a/drivers/bluetooth/Kconfig > > +++ b/drivers/bluetooth/Kconfig > > @@ -180,6 +180,17 @@ config BT_HCIUART_AG6XX > > > > Say Y here to compile support for Intel AG6XX protocol. > > > > +config BT_HCIUART_MRVL > > + bool "Marvell protocol support" > > + depends on BT_HCIUART > > + select BT_HCIUART_H4 > > + help > > + Marvell is serial protocol for communication between Bluetooth > > + device and host. This protocol is required for most Marvell > Bluetooth > > + devices with UART interface. > > + > > + Say Y here to compile support for HCI MRVL protocol. > > + > > config BT_HCIBCM203X > > tristate "HCI BCM203x USB driver" > > depends on USB > > diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile > > index 9c18939..364dbb6 100644 > > --- a/drivers/bluetooth/Makefile > > +++ b/drivers/bluetooth/Makefile > > @@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL) += hci_intel.o > > hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o > > hci_uart-$(CONFIG_BT_HC
Re: [v11,3/3] Bluetooth: hci_uart: Support firmware download for Marvell
On 2016-5-6 23:31, Amitkumar Karwar wrote: From: Ganapathi Bhat This patch implement firmware download feature for Marvell Bluetooth devices. If firmware is already downloaded, it will skip downloading. Signed-off-by: Ganapathi Bhat Signed-off-by: Amitkumar Karwar --- v2: Fixed compilation warning reported by kbuild test robot v3: Addressed review comments from Marcel Holtmann a) Removed vendor specific code from hci_ldisc.c b) Get rid of static forward declaration c) Removed unnecessary heavy nesting d) Git rid of module parameter and global variables e) Add logic to pick right firmware image v4: Addresses review comments from Alan a) Use existing kernel helper APIs instead of writing own. b) Replace mdelay() with msleep() v5: Addresses review comments from Loic Poulain a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG b) Used static functions where required and removed forward delcarations c) Edited comments for the function hci_uart_recv_data d) Made HCI_UART_DNLD_FW flag a part of driver private data v6: Addresses review comments from Loic Poulain a) Used skb instead of array to store firmware data during download b) Used hci_uart_tx_wakeup and enqueued packets instead of tty write c) Used GFP_KERNEL instead of GFP_ATOMIC v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change resolves errors reported by kbuild test robot. v8: Addressed review comments from Marcel Holtmann a) Removed unnecessary memory allocation failure messages b) Get rid of btmrvl.h header file and add definitions in hci_mrvl.c file v9: Addressed review comments from Marcel Holtmann a) Moved firmware download code from setup to prepare handler. b) Change messages from bt_dev_*->BT_*, as hdev isn't available during firmware download. v10: Addressed review comments from Marcel Holtmann a) Added new callback recv_for_prepare to receive data from device during prepare phase b) Avoided using private flags (HCI_UART_DNLD_FW) as new receive callback is added for the same purpose c) Used kernel API to handle unaligned data d) Moved mrvl_set_baud functionality inside setup callback v11: Write data through ldisc in mrvl_send_ack() instead of directly calling write method(One Thousand Gnomes). --- drivers/bluetooth/Kconfig | 11 + drivers/bluetooth/Makefile| 1 + drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_mrvl.c | 543 ++ drivers/bluetooth/hci_uart.h | 8 +- 5 files changed, 568 insertions(+), 1 deletion(-) create mode 100644 drivers/bluetooth/hci_mrvl.c diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index cf50fd2..daafd0c 100644 --- a/drivers/bluetooth/Kconfig +++ b/drivers/bluetooth/Kconfig @@ -180,6 +180,17 @@ config BT_HCIUART_AG6XX Say Y here to compile support for Intel AG6XX protocol. +config BT_HCIUART_MRVL + bool "Marvell protocol support" + depends on BT_HCIUART + select BT_HCIUART_H4 + help + Marvell is serial protocol for communication between Bluetooth + device and host. This protocol is required for most Marvell Bluetooth + devices with UART interface. + + Say Y here to compile support for HCI MRVL protocol. + config BT_HCIBCM203X tristate "HCI BCM203x USB driver" depends on USB diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile index 9c18939..364dbb6 100644 --- a/drivers/bluetooth/Makefile +++ b/drivers/bluetooth/Makefile @@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL) += hci_intel.o hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o hci_uart-$(CONFIG_BT_HCIUART_QCA) += hci_qca.o hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o +hci_uart-$(CONFIG_BT_HCIUART_MRVL) += hci_mrvl.o hci_uart-objs := $(hci_uart-y) ccflags-y += -D__CHECK_ENDIAN__ diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c index 047e786..4896b6f 100644 --- a/drivers/bluetooth/hci_ldisc.c +++ b/drivers/bluetooth/hci_ldisc.c @@ -821,6 +821,9 @@ static int __init hci_uart_init(void) #ifdef CONFIG_BT_HCIUART_AG6XX ag6xx_init(); #endif +#ifdef CONFIG_BT_HCIUART_MRVL + mrvl_init(); +#endif return 0; } @@ -856,6 +859,9 @@ static void __exit hci_uart_exit(void) #ifdef CONFIG_BT_HCIUART_AG6XX ag6xx_deinit(); #endif +#ifdef CONFIG_BT_HCIUART_MRVL + mrvl_deinit(); +#endif /* Release tty registration of line discipline */ err = tty_unregister_ldisc(N_HCI); diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c new file mode 100644 index 000..2686901 --- /dev/null +++ b/drivers/bluetooth/hci_mrvl.c @@ -0,0 +1,543 @@ +/* Bluetooth HCI UART driver for Marvell devices + * + * Copyright (C) 2016, Marvell International Ltd. + * + * Acknowledgements:
Re: [v11,3/3] Bluetooth: hci_uart: Support firmware download for Marvell
On 2016年05月06日 23:31, Amitkumar Karwar wrote: From: Ganapathi Bhat This patch implement firmware download feature for Marvell Bluetooth devices. If firmware is already downloaded, it will skip downloading. Signed-off-by: Ganapathi Bhat Signed-off-by: Amitkumar Karwar Tested-by: Caesar Wang Tested on chromeos4.4. so you can free add my test tag: I don't find the cover-letter, so add it in here. Cc: linux-rockc...@lists.infradead.org, that's interesting in this series patches. --- v2: Fixed compilation warning reported by kbuild test robot v3: Addressed review comments from Marcel Holtmann a) Removed vendor specific code from hci_ldisc.c b) Get rid of static forward declaration c) Removed unnecessary heavy nesting d) Git rid of module parameter and global variables e) Add logic to pick right firmware image v4: Addresses review comments from Alan a) Use existing kernel helper APIs instead of writing own. b) Replace mdelay() with msleep() v5: Addresses review comments from Loic Poulain a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG b) Used static functions where required and removed forward delcarations c) Edited comments for the function hci_uart_recv_data d) Made HCI_UART_DNLD_FW flag a part of driver private data v6: Addresses review comments from Loic Poulain a) Used skb instead of array to store firmware data during download b) Used hci_uart_tx_wakeup and enqueued packets instead of tty write c) Used GFP_KERNEL instead of GFP_ATOMIC v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change resolves errors reported by kbuild test robot. v8: Addressed review comments from Marcel Holtmann a) Removed unnecessary memory allocation failure messages b) Get rid of btmrvl.h header file and add definitions in hci_mrvl.c file v9: Addressed review comments from Marcel Holtmann a) Moved firmware download code from setup to prepare handler. b) Change messages from bt_dev_*->BT_*, as hdev isn't available during firmware download. v10: Addressed review comments from Marcel Holtmann a) Added new callback recv_for_prepare to receive data from device during prepare phase b) Avoided using private flags (HCI_UART_DNLD_FW) as new receive callback is added for the same purpose c) Used kernel API to handle unaligned data d) Moved mrvl_set_baud functionality inside setup callback v11: Write data through ldisc in mrvl_send_ack() instead of directly calling write method(One Thousand Gnomes). --- drivers/bluetooth/Kconfig | 11 + drivers/bluetooth/Makefile| 1 + drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_mrvl.c | 543 ++ drivers/bluetooth/hci_uart.h | 8 +- 5 files changed, 568 insertions(+), 1 deletion(-) create mode 100644 drivers/bluetooth/hci_mrvl.c diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index cf50fd2..daafd0c 100644 --- a/drivers/bluetooth/Kconfig +++ b/drivers/bluetooth/Kconfig @@ -180,6 +180,17 @@ config BT_HCIUART_AG6XX Say Y here to compile support for Intel AG6XX protocol. +config BT_HCIUART_MRVL + bool "Marvell protocol support" + depends on BT_HCIUART + select BT_HCIUART_H4 + help + Marvell is serial protocol for communication between Bluetooth + device and host. This protocol is required for most Marvell Bluetooth + devices with UART interface. + + Say Y here to compile support for HCI MRVL protocol. + config BT_HCIBCM203X tristate "HCI BCM203x USB driver" depends on USB diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile index 9c18939..364dbb6 100644 --- a/drivers/bluetooth/Makefile +++ b/drivers/bluetooth/Makefile @@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL) += hci_intel.o hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o hci_uart-$(CONFIG_BT_HCIUART_QCA) += hci_qca.o hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o +hci_uart-$(CONFIG_BT_HCIUART_MRVL) += hci_mrvl.o hci_uart-objs := $(hci_uart-y) ccflags-y += -D__CHECK_ENDIAN__ diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c index 047e786..4896b6f 100644 --- a/drivers/bluetooth/hci_ldisc.c +++ b/drivers/bluetooth/hci_ldisc.c @@ -821,6 +821,9 @@ static int __init hci_uart_init(void) #ifdef CONFIG_BT_HCIUART_AG6XX ag6xx_init(); #endif +#ifdef CONFIG_BT_HCIUART_MRVL + mrvl_init(); +#endif return 0; } @@ -856,6 +859,9 @@ static void __exit hci_uart_exit(void) #ifdef CONFIG_BT_HCIUART_AG6XX ag6xx_deinit(); #endif +#ifdef CONFIG_BT_HCIUART_MRVL + mrvl_deinit(); +#endif /* Release tty registration of line discipline */ err = tty_unregister_ldisc(N_HCI); diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c new file mode 10064