Re: [PATCH 2/2] Bluetooth: btusb: ath3k: Cache firmware for already patched Bluetooth chip

[Kai-Heng Feng]

On Thu, Aug 24, 2017 at 5:15 PM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
> Hi Kai-Heng,
>
>> When a system reboot, the USB power never gets cut off, so the firmware
>> is already updated on the Bluetooth chip.
>>
>> Several btusb setup functions check firmware updated status before
>> download firmware, the loading part will be skipped if it's updated.
>> Because the firmware is never asked by request_firmware(),
>> firmware_class does not know it needs to be cached before system enters
>> sleep.
>>
>> Now, system suspend/resume may cause the driver failed to request the
>> firmware because it's not in the firmware cache:
>>
>> [   87.539434] firmware request while host is not available
>>
>> This can be solved by calling request_firmware() even if the chip is
>> already updated - now the firmware_class knows what to cache.
>>
>> In this case, we don't really need to wait for the firmware content, so
>> we use the async version of request_firmware().
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
>> ---
>> drivers/bluetooth/ath3k.c | 49 +++++++++++++++++++++++++++++++++++++++++
>> drivers/bluetooth/btusb.c | 56 +++++++++++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 103 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
>> index 280849dba51e..27a7415f9fd7 100644
>> --- a/drivers/bluetooth/ath3k.c
>> +++ b/drivers/bluetooth/ath3k.c
>> @@ -208,6 +208,54 @@ static const struct usb_device_id ath3k_blist_tbl[] = {
>> #define TIMEGAP_USEC_MIN      50
>> #define TIMEGAP_USEC_MAX      100
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static void ath3k_request_firmware_done(const struct firmware *firmware,
>> +                                     void *context)
>> +{
>> +     const char *name = (const char *)context;
>> +
>> +     if (!firmware) {
>> +             BT_WARN("firmware %s will not be cached", name);
>> +             goto done;
>> +     }
>> +
>> +     BT_DBG("firmware %s will be cached", name);
>> +
>> +     release_firmware(firmware);
>> +done:
>> +     kfree_const(name);
>> +}
>> +
>> +static int ath3k_request_firmware_async(struct usb_device *udev,
>> +                                     const char *fwname)
>> +{
>> +     const char *name;
>> +     int err;
>> +
>> +     name = kstrdup_const(fwname, GFP_KERNEL);
>> +     if (!name)
>> +             return -ENOMEM;
>> +
>> +     err = request_firmware_nowait(THIS_MODULE, true, name, &udev->dev,
>> +                                   GFP_KERNEL, (void *)name,
>> +                                   ath3k_request_firmware_done);
>> +     if (err) {
>> +             BT_WARN("%s %s: failed to async request firmware for file: %s (%d)",
>> +                     udev->manufacturer, udev->product, name, err);
>> +             kfree_const(name);
>> +             return err;
>> +     }
>> +
>> +     return 0;
>> +}
>> +#else
>> +static int ath3k_request_firmware_async(struct usb_device *udev,
>> +                                     const char *fwname)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>> static int ath3k_load_firmware(struct usb_device *udev,
>>                               const struct firmware *firmware)
>> {
>> @@ -420,6 +468,7 @@ static int ath3k_load_patch(struct usb_device *udev)
>>
>>       if (fw_state & ATH3K_PATCH_UPDATE) {
>>               BT_DBG("Patch was already downloaded");
>> +             ath3k_request_firmware_async(udev, filename);
>>               return 0;
>>       }
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 732fe6c3e789..7de2156debd8 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -1459,6 +1459,54 @@ static void btusb_waker(struct work_struct *work)
>>       usb_autopm_put_interface(data->intf);
>> }
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static void btusb_request_firmware_done(const struct firmware *firmware,
>> +                                     void *context)
>> +{
>> +     const char *name = (const char *)context;
>> +
>> +     if (!firmware) {
>> +             BT_WARN("firmware %s will not be cached", name);
>> +             goto done;
>> +     }
>> +
>> +     BT_DBG("firmware %s will be cached", name);
>> +
>> +     release_firmware(firmware);
>> +done:
>> +     kfree_const(name);
>> +}
>> +
>> +static int btusb_request_firmware_async(struct hci_dev *hdev,
>> +                                     const char *fwname)
>> +{
>> +     const char *name;
>> +     int err;
>> +
>> +     name = kstrdup_const(fwname, GFP_KERNEL);
>> +     if (!name)
>> +             return -ENOMEM;
>> +
>> +     err = request_firmware_nowait(THIS_MODULE, true, name, &hdev->dev,
>> +                                   GFP_KERNEL, (void *)name,
>> +                                   btusb_request_firmware_done);
>> +     if (err) {
>> +             BT_WARN("%s: failed to async request firmware for file: %s (%d)",
>> +                     hdev->name, name, err);
>> +             kfree_const(name);
>> +             return err;
>> +     }
>> +
>> +     return 0;
>> +}
>> +#else
>> +static int btusb_request_firmware_async(struct hci_dev *hdev,
>> +                                     const char *fwname)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>> static int btusb_setup_bcm92035(struct hci_dev *hdev)
>> {
>>       struct sk_buff *skb;
>> @@ -1724,6 +1772,8 @@ static int btusb_setup_intel(struct hci_dev *hdev)
>>       if (ver.fw_patch_num) {
>>               BT_INFO("%s: Intel device is already patched. patch num: %02x",
>>                       hdev->name, ver.fw_patch_num);
>> +             btusb_request_firmware_async(hdev, fwname);
>> +             btusb_request_firmware_async(hdev, default_fwname);
>>               goto complete;
>>       }
>
> I do not like intermixing of different vendors in a single patch.

Thanks, I'll split them into Intel/QCA/ath3k patches.

Do you have any concern about the approach?

>
> Regards
>
> Marcel
>