RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> From: Marcel Holtmann [mailto:marcel@xxxxxxxxxxxx]
> Sent: Friday, April 22, 2016 6:19 PM
> To: Amitkumar Karwar
> Cc: Linux Bluetooth; LKML; Ganapathi Bhat; Cathy Luo
> Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
> 
> Hi Amitkumar,
> 
> >>>> +
> >>>> +static int mrvl_setup(struct hci_uart *hu) {
> >>>> +	struct mrvl_data *mrvl = hu->priv;
> >>>> +
> >>>> +	mrvl_init_fw_data(hu);
> >>>> +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> >>>> +
> >>>> +	return hci_uart_dnld_fw(hu);
> >>>> +}
> >>>
> >>> So this is clearly the wrong spot. When ->setup is called it is
> >>> expected that HCI is ready. You are misusing it here.
> >>>
> >>
> >> Sure. We will move this to mrvl_open() where HCI is not yet
> initialized.
> >
> > We tried moving firmware download to mrvl_open(), but it's not
> feasible. "hu->proto" is not yet initialized at that time. So when the
> data/ack is received during firmware download, we can't have Marvell
> specific handling. Also, I can see other vendor's (broadcomm, Intel)
> have done firmware download in setup handler.
> 
> firmware download in ->setup() is fine as long as it uses HCI commands.
> If it does not use HCI commands, then we need to come up with something
> new.
> 
> The problem here is that for all intense and purposes once ->setup() is
> called, then assumption is that you are in an HCI capable transport and
> it is ready.
> 

We have maintained a flag(in mrvl->flags) which remains on until firmware download gets completed. We drop HCI frames in "->enqueue" if firmware download is in progress. I don't see any possible issues if firmware download is included in ->setup().

request_firmware() API expects a device pointer (hdev->dev). We won't get valid pointer until HCI is ready.
Also, we are downloading firmware data chunks through normal Tx path(scheduling hu->write_work etc) where hdev comes into picture.

Let me know your thoughts.

Regards,
Amitkumar