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.

Regards

Marcel