At Thu, 21 May 2015 14:07:11 +0200,
Marcel Holtmann wrote:
>
> Hi Takashi,
>
> >>>>>> The data is cached in RAM. More specifically, the former loaded
> >>>>>> firmware files are reloaded and saved at suspend for each device
> >>>>>> object. See fw_pm_notify() in firmware_class.c.
> >>>>>
> >>>>> OK, this may be a stupid idea, but do we know the firmware
> >>>>> was successfully loaded in the first place?
> >>>>> Also btusb is in the habit of falling back to a generic
> >>>>> firmware in some places. It seems to me that caching
> >>>>> firmware is conceptually not enough, but we'd also need
> >>>>> to record the absence of firmware images.
> >>>>
> >>>> in a lot of cases the firmware is optional. The device will operate fine without the firmware. There are a few devices where the firmware is required, but for many it just contains patches.
> >>>>
> >>>> It would be nice if we could tell request_firmware() if it is optional or mandatory firmware. Or if it should just cache the status of a missing firmware as well.
> >>>
> >>> OK, below is a quick hack to record the failed f/w files, too.
> >>> Not sure whether this helps, though. Proper tests are appreciated.
> >>>
> >>>
> >>
> >> This doesn't quite work. We end up with the name on fw_names but
> >> the firmware isn't actually on the firmware cache list.
> >>
> >> If request_firmware fails to get the firmware from the filesystem,
> >> release firmware will be called which is going to free the
> >> firmware_buf which has been marked as failed anyway. The only
> >> way to make this work would be to always piggy back and increase
> >> the ref so it always stays around. But this also marks the firmware
> >> as a permanent failure. There would need to be a hook somewhere
> >> to force a cache drop, else there would be no way to add new
> >> firmware to a running system without a reboot.
> >>
> >> Perhaps we split the difference: keep a list of firmware images
> >> that failed to load in the past and if one is requested during
> >> a time when usermodehelper isn't available, silently return an
> >> error? This way, if correct firmware is loaded at a regular time
> >> the item can be removed from the list.
> >
> > Well, IMO, it's way too much expectation for the generic f/w loader.
> > The driver itself must know already which should be really loaded.
> > The fact is that it's the driver who calls the function that might not
> > work in the resume path. So the driver can deal with such exceptions
> > at best.
>
> I keep repeating myself here. From the driver point of view it goes
> via probe() callback of the USB driver. So the driver does not
> know. For the driver it looks like a brand new device. There are
> platforms that might decide to just kill the power to the USB bus
> where the Bluetooth controller sits on. It gets the power back on
> resume. However this means it is a brand new device at that
> point. So the driver should not have to remember everything.
Then avoiding the failed firmware is no solution, indeed.
If it's a new probe, it should be never executed during resume.
That is, either freeze the work like Laura's patch or explicitly allow
the UMH lock wait like my patch. Laura's patch has a merit that it's
much simpler. OTOH, if you want to keep the changes only in
request_firmware() call, you can think of changes like my patch; a
revised version is attached below.
Takashi
---
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841ad1008..87157f557263 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -97,21 +97,6 @@ static inline long firmware_loading_timeout(void)
return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
}
-/* firmware behavior options */
-#define FW_OPT_UEVENT (1U << 0)
-#define FW_OPT_NOWAIT (1U << 1)
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-#define FW_OPT_USERHELPER (1U << 2)
-#else
-#define FW_OPT_USERHELPER 0
-#endif
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-#define FW_OPT_FALLBACK FW_OPT_USERHELPER
-#else
-#define FW_OPT_FALLBACK 0
-#endif
-#define FW_OPT_NO_WARN (1U << 3)
-
struct firmware_cache {
/* firmware_buf instance will be added into the below list */
spinlock_t lock;
@@ -1085,8 +1070,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
}
/* called from request_firmware() and request_firmware_work_func() */
-static int
-_request_firmware(const struct firmware **firmware_p, const char *name,
+int _request_firmware(const struct firmware **firmware_p, const char *name,
struct device *device, unsigned int opt_flags)
{
struct firmware *fw;
@@ -1099,13 +1083,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
if (!name || name[0] == '\0')
return -EINVAL;
+ /* Need to pin this module until return */
+ __module_get(THIS_MODULE);
ret = _request_firmware_prepare(&fw, name, device);
if (ret <= 0) /* error or already assigned */
goto out;
ret = 0;
timeout = firmware_loading_timeout();
- if (opt_flags & FW_OPT_NOWAIT) {
+ if (opt_flags & FW_OPT_UMH_LOCK_WAIT) {
timeout = usermodehelper_read_lock_wait(timeout);
if (!timeout) {
dev_dbg(device, "firmware: %s loading timed out\n",
@@ -1147,8 +1133,10 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
}
*firmware_p = fw;
+ module_put(THIS_MODULE);
return ret;
}
+EXPORT_SYMBOL_GPL(_request_firmware);
/**
* request_firmware: - send firmware request and wait for it
@@ -1174,14 +1162,8 @@ int
request_firmware(const struct firmware **firmware_p, const char *name,
struct device *device)
{
- int ret;
-
- /* Need to pin this module until return */
- __module_get(THIS_MODULE);
- ret = _request_firmware(firmware_p, name, device,
+ return _request_firmware(firmware_p, name, device,
FW_OPT_UEVENT | FW_OPT_FALLBACK);
- module_put(THIS_MODULE);
- return ret;
}
EXPORT_SYMBOL(request_firmware);
@@ -1199,13 +1181,8 @@ EXPORT_SYMBOL(request_firmware);
int request_firmware_direct(const struct firmware **firmware_p,
const char *name, struct device *device)
{
- int ret;
-
- __module_get(THIS_MODULE);
- ret = _request_firmware(firmware_p, name, device,
+ return _request_firmware(firmware_p, name, device,
FW_OPT_UEVENT | FW_OPT_NO_WARN);
- module_put(THIS_MODULE);
- return ret;
}
EXPORT_SYMBOL_GPL(request_firmware_direct);
@@ -1291,6 +1268,7 @@ request_firmware_nowait(
fw_work->context = context;
fw_work->cont = cont;
fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
+ FW_OPT_UMH_LOCK_WAIT |
(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
if (!try_module_get(module)) {
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d21f3b4176d3..3465f1e4030e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1633,6 +1633,11 @@ out:
return ret;
}
+#define bt_request_firmware(fw, name, dev) \
+ _request_firmware(fw, name, dev, \
+ FW_OPT_UEVENT | FW_OPT_FALLBACK | \
+ FW_OPT_UMH_LOCK_WAIT)
+
static int btusb_setup_rtl8723a(struct hci_dev *hdev)
{
struct btusb_data *data = dev_get_drvdata(&hdev->dev);
@@ -1641,7 +1646,7 @@ static int btusb_setup_rtl8723a(struct hci_dev *hdev)
int ret;
BT_INFO("%s: rtl: loading rtl_bt/rtl8723a_fw.bin", hdev->name);
- ret = request_firmware(&fw, "rtl_bt/rtl8723a_fw.bin", &udev->dev);
+ ret = bt_request_firmware(&fw, "rtl_bt/rtl8723a_fw.bin", &udev->dev);
if (ret < 0) {
BT_ERR("%s: Failed to load rtl_bt/rtl8723a_fw.bin", hdev->name);
return ret;
@@ -1678,7 +1683,7 @@ static int btusb_setup_rtl8723b(struct hci_dev *hdev, u16 lmp_subver,
int ret;
BT_INFO("%s: rtl: loading %s", hdev->name, fw_name);
- ret = request_firmware(&fw, fw_name, &udev->dev);
+ ret = bt_request_firmware(&fw, fw_name, &udev->dev);
if (ret < 0) {
BT_ERR("%s: Failed to load %s", hdev->name, fw_name);
return ret;
@@ -1754,7 +1759,7 @@ static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev *hdev,
ver->fw_variant, ver->fw_revision, ver->fw_build_num,
ver->fw_build_ww, ver->fw_build_yy);
- ret = request_firmware(&fw, fwname, &hdev->dev);
+ ret = bt_request_firmware(&fw, fwname, &hdev->dev);
if (ret < 0) {
if (ret == -EINVAL) {
BT_ERR("%s Intel firmware file request failed (%d)",
@@ -1770,7 +1775,7 @@ static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev *hdev,
*/
snprintf(fwname, sizeof(fwname), "intel/ibt-hw-%x.%x.bseq",
ver->hw_platform, ver->hw_variant);
- if (request_firmware(&fw, fwname, &hdev->dev) < 0) {
+ if (bt_request_firmware(&fw, fwname, &hdev->dev) < 0) {
BT_ERR("%s failed to open default Intel fw file: %s",
hdev->name, fwname);
return NULL;
@@ -2482,7 +2487,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
snprintf(fwname, sizeof(fwname), "intel/ibt-11-%u.sfi",
le16_to_cpu(params->dev_revid));
- err = request_firmware(&fw, fwname, &hdev->dev);
+ err = bt_request_firmware(&fw, fwname, &hdev->dev);
if (err < 0) {
BT_ERR("%s: Failed to load Intel firmware file (%d)",
hdev->name, err);
@@ -2905,7 +2910,7 @@ static int btusb_setup_qca_load_rampatch(struct hci_dev *hdev,
snprintf(fwname, sizeof(fwname), "qca/rampatch_usb_%08x.bin", ver_rom);
- err = request_firmware(&fw, fwname, &hdev->dev);
+ err = bt_request_firmware(&fw, fwname, &hdev->dev);
if (err) {
BT_ERR("%s: failed to request rampatch file: %s (%d)",
hdev->name, fwname, err);
@@ -2948,7 +2953,7 @@ static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
le32_to_cpu(ver->rom_version));
- err = request_firmware(&fw, fwname, &hdev->dev);
+ err = bt_request_firmware(&fw, fwname, &hdev->dev);
if (err) {
BT_ERR("%s: failed to request NVM file: %s (%d)",
hdev->name, fwname, err);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5c41c5e75b5c..68859bc365eb 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -26,6 +26,22 @@ struct builtin_fw {
unsigned long size;
};
+/* firmware behavior options */
+#define FW_OPT_UEVENT (1U << 0) /* enable uevent */
+#define FW_OPT_NOWAIT (1U << 1) /* handle in background wq */
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_USERHELPER (1U << 2)
+#else
+#define FW_OPT_USERHELPER 0
+#endif
+#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
+#define FW_OPT_FALLBACK FW_OPT_USERHELPER /* fallback via userhelper */
+#else
+#define FW_OPT_FALLBACK 0
+#endif
+#define FW_OPT_NO_WARN (1U << 3) /* no warning for failure */
+#define FW_OPT_UMH_LOCK_WAIT (1U << 4) /* wait usermodehelper lock */
+
/* We have to play tricks here much like stringify() to get the
__COUNTER__ macro to be expanded as we want it */
#define __fw_concat1(x, y) x##y
@@ -39,6 +55,8 @@ struct builtin_fw {
__used __section(.builtin_fw) = { name, blob, size }
#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
+int _request_firmware(const struct firmware **fw, const char *name,
+ struct device *device, int opt_flags);
int request_firmware(const struct firmware **fw, const char *name,
struct device *device);
int request_firmware_nowait(
@@ -50,6 +68,12 @@ int request_firmware_direct(const struct firmware **fw, const char *name,
void release_firmware(const struct firmware *fw);
#else
+static inline int _request_firmware(const struct firmware **fw,
+ const char *name, struct device *device,
+ int opt_flags)
+{
+ return -EINVAL;
+}
static inline int request_firmware(const struct firmware **fw,
const char *name,
struct device *device)
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
