linux-kernel - Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <AFF1BDDA-EB1A-4C99-976E-643A093FFD48@holtmann.org>
Date:	Wed, 20 Apr 2016 16:42:53 +0200
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Amitkumar Karwar <akarwar@...vell.com>
Cc:	Linux Bluetooth <linux-bluetooth@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Ganapathi Bhat <gbhat@...vell.com>
Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

Hi Amitkumar,

> Thanks for your review.
> We will address these comments in updated version.
> 
>>> +
>>> +/* Receive data */
>>> +static int mrvl_recv(struct hci_uart *hu, const void *data, int
>>> +count) {
>>> +	struct mrvl_data *mrvl = hu->priv;
>>> +
>>> +	if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags)) {
>>> +		mrvl->fwdata->skb = mrvl_process_fw_data(hu, mrvl->fwdata-
>>> skb,
>>> +							 (u8 *)data, count);
>>> +		if (IS_ERR(mrvl->fwdata->skb)) {
>>> +			int err = PTR_ERR(mrvl->fwdata->skb);
>>> +
>>> +			bt_dev_err(hu->hdev,
>>> +				   "Receive firmware data failed (%d)", err);
>>> +			mrvl->fwdata->skb = NULL;
>>> +			return err;
>>> +		}
>>> +		return 0;
>>> +	}
>> 
>> This part actually worries me a bit. Yes, we can do it this way, but in
>> general it sounds a bit more like we need a generic approach in
>> hci_ldisc.c to handle pre-HCI firmware loading and/or setup.
>> 
>> In the btusb.c driver we added ->setup_on_usb callback. And for it
>> sounds like we need something similar here. So that hci_ldisc.c can
>> handle most of the basic TX/RX.
> 
> Even if we added "->setup_on_usb" in hci_ldisc.c, we will need protocol specific changes in receive path during firmware download.
> With this patch, those changes are smoothly handled in hci_mrvl.c file.
> [hci_uart_tty_receive] -> [proto->recv] -> [mrvl_recv] -> [normal Rx path/FW download Rx handling]

we might need to find a better callback name, but yes, we want to make this generic in hci_ldisc.c. I do not have a perfect solution or quick answer, but I think this needs a bit more generic framework.

Regards

Marcel