Re: [PATCH v2 resend 1/2] Introduced a load_firmware callback to struct hci_dev

[Marcel Holtmann <marcel@holtmann.org>]

Hi Jesse,

> >> >> load_firmware will be called at the end of hci_dev_open() if it
> >> >> is defined.
> >> >>
> >> >> Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
> >> >> ---
> >> >>  include/net/bluetooth/hci_core.h |    1 +
> >> >>  net/bluetooth/hci_core.c         |    2 ++
> >> >>  2 files changed, 3 insertions(+)
> >> >>
> >> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> >> index 593cd1d..40972a3 100644
> >> >> --- a/include/net/bluetooth/hci_core.h
> >> >> +++ b/include/net/bluetooth/hci_core.h
> >> >> @@ -281,6 +281,7 @@ struct hci_dev {
> >> >>       int (*send)(struct sk_buff *skb);
> >> >>       void (*notify)(struct hci_dev *hdev, unsigned int evt);
> >> >>       int (*ioctl)(struct hci_dev *hdev, unsigned int cmd, unsigned long arg);
> >> >> +     void (*load_firmware)(struct hci_dev *hdev);
> >> >>  };
> >> >>
> >> >>  struct hci_conn {
> >> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> >> index d4de5db..49be87a 100644
> >> >> --- a/net/bluetooth/hci_core.c
> >> >> +++ b/net/bluetooth/hci_core.c
> >> >> @@ -725,6 +725,8 @@ int hci_dev_open(__u16 dev)
> >> >>  done:
> >> >>       hci_req_unlock(hdev);
> >> >>       hci_dev_put(hdev);
> >> >> +     if (!ret && hdev->load_firmware)
> >> >> +             hdev->load_firmware(hdev);
> >> >>       return ret;
> >> >>  }
> >> >>
> >> >
> >> > has anybody thought this through actually? Do we need to reload the
> >> > firmware after every HCI_Reset? Since hci_dev_open() is used at least
> >> > twice during normal operation. And for every RFKILL or power down/up
> >> > cycle of the chip.
> >> >
> >> > And there is an internal process of hci_dev_open() trigger on
> >> > registration and others triggered by hciconfig hci0 up. I am pretty much
> >> > against having to wait for all this firmware loading crap during every
> >> > bring up of the device. Especially since it always does a trip via
> >> > request_firmware().
> >>
> >> In the second patch, firmware loading would be done only once per
> >> power cycle of the chip. Since I think it should be the device driver, not hci,
> >> who knows when and how to load firmware, the lock is placed in btusb.c.
> >
> > and how does the driver knows these details? That makes no sense. How
> > does the driver know it got rebooted?
> >
> > The hci_dev_open() will start the transport. And as I explained before,
> > that can happen twice during boot time.
> 
> Please take a look at the second part of this patchset, which is in patch 2/2.
> Loading firmware is needed when the chip is rebooted, and a reboot would trigger
> a probe in btusb. So btusb can know a firmware loading is needed whenever
> a new patchram device is probed. And the load_firmware callback in
> btusb would do
> test_and_set_bit to ensure that the loading process would only be done once.

that is horrible hackish. So NAK from my side. Look at what I told the
Intel guys to do on supporting their USB dongle. You have the same
problem and I want it solved in a similar way.

The probe() callback is actually not a good idea either btw. It is for
binding a driver. Binding and unbinding the driver has nothing to do
with reboot of the device.

Regards

Marcel