[RFT 0/7] firmware: enable caching of firmware for reboot optimization

[RFT 0/7] firmware: enable caching of firmware for reboot optimization

[Luis R. Rodriguez]

Some devices have optimizations in place which enable devices to retain the
firmware on reboot. Some drivers check for this and if present on probe they
don't request the firmware. This can be an issue if the system suspends after
this given the firmware is still needed on resume. Since the request firmware
call was not issued no cache was done.

This series exposes a way for drivers to setup the cache, while also
doing some minor sanity checks. This series only contains the delta from
my last submission of patches for the firmware API for v4.17.

Cantabile, please give these patches a spin and let me know if it fixes
your reported issue. They depend on other pending patches I have in line
waiting to be merged so the easiest I thing I think is for you to test my
20180227-firmware-cache branch [0], based on Linus' tree. To get that
tree, cd into your Linus git tree and do:

git remote add mcgrof https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git
git checkout -b 20180227-firmware-cache mcgrof/20180227-firmware-cache

Please let me know if this resolves your issue and thanks for your report.

  Luis

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180227-firmware-cache


Luis R. Rodriguez (7):
  rename: _request_firmware_load() fw_load_sysfs_fallback()
  firmware: fix checking for return values for fw_add_devm_name()
  firmware: make fw_add_devm_name() return 0 if cache present
  firmware: add helper to check to see if fw cache is setup
  firmware: ensure the firmware cache is not used on incompatible calls
  firmware: add request_firmware_cache() to help with cache on reboot
  mt7601u: use request_firmware_cache() to address cache on reboot

 .../driver-api/firmware/fallback-mechanisms.rst    |  2 +-
 .../driver-api/firmware/request_firmware.rst       | 14 +++++
 drivers/base/firmware_fallback.c                   |  4 +-
 drivers/base/firmware_loader.c                     | 61 ++++++++++++++++++++--
 drivers/net/wireless/mediatek/mt7601u/mcu.c        |  2 +-
 include/linux/firmware.h                           |  3 ++
 6 files changed, 77 insertions(+), 9 deletions(-)

-- 
2.16.2

[RFT 1/7] rename: _request_firmware_load() fw_load_sysfs_fallback()

[Luis R. Rodriguez]

This reflects much clearer what is being done.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst | 2 +-
 drivers/base/firmware_fallback.c                          | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index 4055ac76b288..f353783ae0be 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -112,7 +112,7 @@ Since a device is created for the sysfs interface to help load firmware as a
 fallback mechanism userspace can be informed of the addition of the device by
 relying on kobject uevents. The addition of the device into the device
 hierarchy means the fallback mechanism for firmware loading has been initiated.
-For details of implementation refer to _request_firmware_load(), in particular
+For details of implementation refer to fw_load_sysfs_fallback(), in particular
 on the use of dev_set_uevent_suppress() and kobject_uevent().
 
 The kernel's kobject uevent mechanism is implemented in lib/kobject_uevent.c,
diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c
index 13fa5ff2b46c..ce7ccfe82c69 100644
--- a/drivers/base/firmware_fallback.c
+++ b/drivers/base/firmware_fallback.c
@@ -536,7 +536,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 }
 
 /* load a firmware via user helper */
-static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
+static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
 				  unsigned int opt_flags, long timeout)
 {
 	int retval = 0;
@@ -621,7 +621,7 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 	}
 
 	fw_sysfs->fw_priv = firmware->priv;
-	ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
+	ret = fw_load_sysfs_fallback(fw_sysfs, opt_flags, timeout);
 
 	if (!ret)
 		ret = assign_fw(firmware, device, opt_flags);
-- 
2.16.2

[RFT 2/7] firmware: fix checking for return values for fw_add_devm_name()

[Luis R. Rodriguez]

fw_add_devm_name() adds device's name onto the devres for the
device so that prior to suspend we cache the firmware onto memory,
so that on resume the firmware is reliably available. We never
were checking for success for this call though, meaning in some
really rare cases we my have never setup the firmware cache for
a device, which could in turn make resume fail.

This is all theoretical, no known issues have been reported.
This small issue has been present way since the addition of the
devres firmware cache names on v3.7.

Fixes: f531f05ae9437 ("firmware loader: store firmware name into devres list")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_loader.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 21dd31ef08ae..48932581c70c 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -431,6 +431,7 @@ int assign_fw(struct firmware *fw, struct device *device,
 	      unsigned int opt_flags)
 {
 	struct fw_priv *fw_priv = fw->priv;
+	int ret;
 
 	mutex_lock(&fw_lock);
 	if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {
@@ -447,8 +448,13 @@ int assign_fw(struct firmware *fw, struct device *device,
 	 */
 	/* don't cache firmware handled without uevent */
 	if (device && (opt_flags & FW_OPT_UEVENT) &&
-	    !(opt_flags & FW_OPT_NOCACHE))
-		fw_add_devm_name(device, fw_priv->fw_name);
+	    !(opt_flags & FW_OPT_NOCACHE)) {
+		ret = fw_add_devm_name(device, fw_priv->fw_name);
+		if (ret && ret != 1) {
+			mutex_unlock(&fw_lock);
+			return ret;
+		}
+	}
 
 	/*
 	 * After caching firmware image is started, let it piggyback
-- 
2.16.2

[RFT 3/7] firmware: make fw_add_devm_name() return 0 if cache present

[Luis R. Rodriguez]

Currently fw_add_devm_name() returns 1 if the firmware cache
was already set. This makes it complicated for us to check for
correctness. It is actually non-fatal if the firmware cache
is already setup, so just return 0, and simplify the checkers.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_loader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 48932581c70c..a385622bf3e1 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -403,7 +403,7 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 
 	fwn = fw_find_devm_name(dev, name);
 	if (fwn)
-		return 1;
+		return 0;
 
 	fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
 			   GFP_KERNEL);
@@ -450,7 +450,7 @@ int assign_fw(struct firmware *fw, struct device *device,
 	if (device && (opt_flags & FW_OPT_UEVENT) &&
 	    !(opt_flags & FW_OPT_NOCACHE)) {
 		ret = fw_add_devm_name(device, fw_priv->fw_name);
-		if (ret && ret != 1) {
+		if (ret) {
 			mutex_unlock(&fw_lock);
 			return ret;
 		}
-- 
2.16.2

[RFT 4/7] firmware: add helper to check to see if fw cache is setup

[Luis R. Rodriguez]

Add a helper to check if the firmware cache is already setup for a device.
This will be used later.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_loader.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index a385622bf3e1..5f3d44d0bfe0 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -396,13 +396,23 @@ static struct fw_name_devm *fw_find_devm_name(struct device *dev,
 	return fwn;
 }
 
-/* add firmware name into devres list */
-static int fw_add_devm_name(struct device *dev, const char *name)
+static bool fw_cache_is_setup(struct device *dev, const char *name)
 {
 	struct fw_name_devm *fwn;
 
 	fwn = fw_find_devm_name(dev, name);
 	if (fwn)
+		return true;
+
+	return false;
+}
+
+/* add firmware name into devres list */
+static int fw_add_devm_name(struct device *dev, const char *name)
+{
+	struct fw_name_devm *fwn;
+
+	if (fw_cache_is_setup(dev, name))
 		return 0;
 
 	fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
-- 
2.16.2

[RFT 5/7] firmware: ensure the firmware cache is not used on incompatible calls

[Luis R. Rodriguez]

request_firmware_into_buf() explicitly disables the firmware cache,
meanwhile the firmware cache cannot be used when request_firmware_nowait()
is used without the uevent. Enforce a sanity check for this to avoid future
issues undocumented behaviours should misuses of the firmware cache
happen later.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_loader.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 5f3d44d0bfe0..99c540164074 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -431,6 +431,11 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 	return 0;
 }
 #else
+static bool fw_cache_is_setup(struct device *dev, const char *name)
+{
+	return false;
+}
+
 static int fw_add_devm_name(struct device *dev, const char *name)
 {
 	return 0;
@@ -672,6 +677,9 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
 {
 	int ret;
 
+	if (fw_cache_is_setup(device, name))
+		return -EOPNOTSUPP;
+
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, buf, size,
 				FW_OPT_UEVENT | FW_OPT_NOCACHE);
@@ -769,6 +777,9 @@ request_firmware_nowait(
 	fw_work->opt_flags = FW_OPT_NOWAIT |
 		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
 
+	if (!uevent && fw_cache_is_setup(device, name))
+		return -EOPNOTSUPP;
+
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
 		kfree(fw_work);
-- 
2.16.2

[RFT 6/7] firmware: add request_firmware_cache() to help with cache on reboot

[Luis R. Rodriguez]

Some devices have an optimization in place to enable the firmware to
be retaineed during a system reboot, so after reboot the device can skip
requesting and loading the firmware. This can save up to 1s in load
time. The mt7601u 802.11 device happens to be such a device.

When these devices retain the firmware on a reboot and then suspend
they can miss looking for the firmware on resume. To help with this we
need a way to cache the firmware when such an optimization has taken
place.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 .../driver-api/firmware/request_firmware.rst       | 14 +++++++++++++
 drivers/base/firmware_loader.c                     | 24 ++++++++++++++++++++++
 include/linux/firmware.h                           |  3 +++
 3 files changed, 41 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index cc0aea880824..b554f4338859 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -44,6 +44,20 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
    :functions: request_firmware_nowait
 
+Special optimizations on reboot
+===============================
+
+Some devices have an optimization in place to enable the firmware to be
+retained during system reboot. When such optimizations are used the driver
+author must ensure the firmware is still available on resume from suspend,
+this can be done with request_firmware_cache() insted of requesting for the
+firmare to be loaded.
+
+request_firmware_cache()
+-----------------------
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware_load
+
 request firmware API expected driver use
 ========================================
 
diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 99c540164074..c122f6431e0c 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -656,6 +656,30 @@ int request_firmware_direct(const struct firmware **firmware_p,
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * request_firmware_cache: - cache firmware for suspend so resume can use it
+ * @name: name of firmware file
+ * @device: device for which firmware should be cached for
+ *
+ * There are some devices with an optimization that enables the device to not
+ * require loading firmware on system reboot. This optimization may still
+ * require the firmware present on resume from suspend. This routine can be
+ * used to ensure the firmware is present on resume from suspend in these
+ * situations. This helper is not compatible with drivers which use
+ * request_firmware_into_buf() or request_firmware_nowait() with no uevent set.
+ **/
+int request_firmware_cache(struct device *device, const char *name)
+{
+	int ret;
+
+	mutex_lock(&fw_lock);
+	ret = fw_add_devm_name(device, name);
+	mutex_unlock(&fw_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_cache);
+
 /**
  * request_firmware_into_buf - load firmware into a previously allocated buffer
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d4508080348d..166867910ebc 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -85,4 +85,7 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 }
 
 #endif
+
+int request_firmware_cache(struct device *device, const char *name);
+
 #endif
-- 
2.16.2

[RFT 7/7] mt7601u: use request_firmware_cache() to address cache on reboot

[Luis R. Rodriguez]

request_firmware_cache() will ensure the firmware is available on resume
from suspend if on reboot the device retains the firmware.

This optimization is in place given otherwise on reboot we have to
reload the firmware, the opmization saves us about max 1s, minimum 10ms.

Reported-by: Cantabile <cantabile.desu@gmail.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c b/drivers/net/wireless/mediatek/mt7601u/mcu.c
index 65a8004418ea..b90456a4b4d7 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
@@ -421,7 +421,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev *dev)
 					 MT_USB_DMA_CFG_TX_BULK_EN));
 
 	if (firmware_running(dev))
-		return 0;
+		return request_firmware_cache(dev->dev, MT7601U_FIRMWARE);
 
 	ret = request_firmware(&fw, MT7601U_FIRMWARE, dev->dev);
 	if (ret)
-- 
2.16.2

Re: [RFT 1/7] rename: _request_firmware_load() fw_load_sysfs_fallback()

[Kees Cook]

On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> This reflects much clearer what is being done.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  Documentation/driver-api/firmware/fallback-mechanisms.rst | 2 +-
>  drivers/base/firmware_fallback.c                          | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> index 4055ac76b288..f353783ae0be 100644
> --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> @@ -112,7 +112,7 @@ Since a device is created for the sysfs interface to help load firmware as a
>  fallback mechanism userspace can be informed of the addition of the device by
>  relying on kobject uevents. The addition of the device into the device
>  hierarchy means the fallback mechanism for firmware loading has been initiated.
> -For details of implementation refer to _request_firmware_load(), in particular
> +For details of implementation refer to fw_load_sysfs_fallback(), in particular
>  on the use of dev_set_uevent_suppress() and kobject_uevent().
>
>  The kernel's kobject uevent mechanism is implemented in lib/kobject_uevent.c,
> diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c
> index 13fa5ff2b46c..ce7ccfe82c69 100644
> --- a/drivers/base/firmware_fallback.c
> +++ b/drivers/base/firmware_fallback.c
> @@ -536,7 +536,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
>  }
>
>  /* load a firmware via user helper */

As long as this is being renamed, maybe add full kern-doc for this function?

-Kees

> -static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
> +static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
>                                   unsigned int opt_flags, long timeout)
>  {
>         int retval = 0;
> @@ -621,7 +621,7 @@ static int fw_load_from_user_helper(struct firmware *firmware,
>         }
>
>         fw_sysfs->fw_priv = firmware->priv;
> -       ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
> +       ret = fw_load_sysfs_fallback(fw_sysfs, opt_flags, timeout);
>
>         if (!ret)
>                 ret = assign_fw(firmware, device, opt_flags);
> --
> 2.16.2
>



-- 
Kees Cook
Pixel Security

Re: [RFT 2/7] firmware: fix checking for return values for fw_add_devm_name()

[Kees Cook]

On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> fw_add_devm_name() adds device's name onto the devres for the
> device so that prior to suspend we cache the firmware onto memory,
> so that on resume the firmware is reliably available. We never
> were checking for success for this call though, meaning in some
> really rare cases we my have never setup the firmware cache for
> a device, which could in turn make resume fail.
>
> This is all theoretical, no known issues have been reported.
> This small issue has been present way since the addition of the
> devres firmware cache names on v3.7.
>
> Fixes: f531f05ae9437 ("firmware loader: store firmware name into devres list")
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  drivers/base/firmware_loader.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
> index 21dd31ef08ae..48932581c70c 100644
> --- a/drivers/base/firmware_loader.c
> +++ b/drivers/base/firmware_loader.c
> @@ -431,6 +431,7 @@ int assign_fw(struct firmware *fw, struct device *device,
>               unsigned int opt_flags)
>  {
>         struct fw_priv *fw_priv = fw->priv;
> +       int ret;
>
>         mutex_lock(&fw_lock);
>         if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {
> @@ -447,8 +448,13 @@ int assign_fw(struct firmware *fw, struct device *device,
>          */
>         /* don't cache firmware handled without uevent */
>         if (device && (opt_flags & FW_OPT_UEVENT) &&
> -           !(opt_flags & FW_OPT_NOCACHE))
> -               fw_add_devm_name(device, fw_priv->fw_name);
> +           !(opt_flags & FW_OPT_NOCACHE)) {
> +               ret = fw_add_devm_name(device, fw_priv->fw_name);
> +               if (ret && ret != 1) {

Why not if (ret < 0) ?

-Kees

> +                       mutex_unlock(&fw_lock);
> +                       return ret;
> +               }
> +       }
>
>         /*
>          * After caching firmware image is started, let it piggyback
> --
> 2.16.2
>



-- 
Kees Cook
Pixel Security

Re: [RFT 3/7] firmware: make fw_add_devm_name() return 0 if cache present

[Kees Cook]

On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Currently fw_add_devm_name() returns 1 if the firmware cache
> was already set. This makes it complicated for us to check for
> correctness. It is actually non-fatal if the firmware cache
> is already setup, so just return 0, and simplify the checkers.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

That'll teach me to read all the patches first. ;)

Honestly, I'd just fold this into the prior patch: there's only one
caller and it's exactly about checking the return value.

-Kees

> ---
>  drivers/base/firmware_loader.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
> index 48932581c70c..a385622bf3e1 100644
> --- a/drivers/base/firmware_loader.c
> +++ b/drivers/base/firmware_loader.c
> @@ -403,7 +403,7 @@ static int fw_add_devm_name(struct device *dev, const char *name)
>
>         fwn = fw_find_devm_name(dev, name);
>         if (fwn)
> -               return 1;
> +               return 0;
>
>         fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
>                            GFP_KERNEL);
> @@ -450,7 +450,7 @@ int assign_fw(struct firmware *fw, struct device *device,
>         if (device && (opt_flags & FW_OPT_UEVENT) &&
>             !(opt_flags & FW_OPT_NOCACHE)) {
>                 ret = fw_add_devm_name(device, fw_priv->fw_name);
> -               if (ret && ret != 1) {
> +               if (ret) {
>                         mutex_unlock(&fw_lock);
>                         return ret;
>                 }
> --
> 2.16.2
>



-- 
Kees Cook
Pixel Security

Re: [RFT 2/7] firmware: fix checking for return values for fw_add_devm_name()

[Luis R. Rodriguez]

On Tue, Feb 27, 2018 at 03:29:53PM -0800, Kees Cook wrote:
> On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > fw_add_devm_name() adds device's name onto the devres for the
> > device so that prior to suspend we cache the firmware onto memory,
> > so that on resume the firmware is reliably available. We never
> > were checking for success for this call though, meaning in some
> > really rare cases we my have never setup the firmware cache for
> > a device, which could in turn make resume fail.
> >
> > This is all theoretical, no known issues have been reported.
> > This small issue has been present way since the addition of the
> > devres firmware cache names on v3.7.
> >
> > Fixes: f531f05ae9437 ("firmware loader: store firmware name into devres list")
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  drivers/base/firmware_loader.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
> > index 21dd31ef08ae..48932581c70c 100644
> > --- a/drivers/base/firmware_loader.c
> > +++ b/drivers/base/firmware_loader.c
> > @@ -431,6 +431,7 @@ int assign_fw(struct firmware *fw, struct device *device,
> >               unsigned int opt_flags)
> >  {
> >         struct fw_priv *fw_priv = fw->priv;
> > +       int ret;
> >
> >         mutex_lock(&fw_lock);
> >         if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {
> > @@ -447,8 +448,13 @@ int assign_fw(struct firmware *fw, struct device *device,
> >          */
> >         /* don't cache firmware handled without uevent */
> >         if (device && (opt_flags & FW_OPT_UEVENT) &&
> > -           !(opt_flags & FW_OPT_NOCACHE))
> > -               fw_add_devm_name(device, fw_priv->fw_name);
> > +           !(opt_flags & FW_OPT_NOCACHE)) {
> > +               ret = fw_add_devm_name(device, fw_priv->fw_name);
> > +               if (ret && ret != 1) {
> 
> Why not if (ret < 0) ?

Could be, either way I remove this later for just (ret) after I adjust
fw_add_devm_name() to not return 1 anymore.

So its just a temporal check, I added this patch first so that this can
be merged into kernels who want the fix.

  Luis

> 
> -Kees
> 
> > +                       mutex_unlock(&fw_lock);
> > +                       return ret;
> > +               }
> > +       }
> >
> >         /*
> >          * After caching firmware image is started, let it piggyback
> > --
> > 2.16.2
> >
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Re: [RFT 3/7] firmware: make fw_add_devm_name() return 0 if cache present

[Luis R. Rodriguez]

On Tue, Feb 27, 2018 at 03:31:11PM -0800, Kees Cook wrote:
> On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > Currently fw_add_devm_name() returns 1 if the firmware cache
> > was already set. This makes it complicated for us to check for
> > correctness. It is actually non-fatal if the firmware cache
> > is already setup, so just return 0, and simplify the checkers.
> >
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> 
> That'll teach me to read all the patches first. ;)
> 
> Honestly, I'd just fold this into the prior patch: there's only one
> caller and it's exactly about checking the return value.

Sounds good.

  Luis

Re: [RFT 1/7] rename: _request_firmware_load() fw_load_sysfs_fallback()

[Luis R. Rodriguez]

On Tue, Feb 27, 2018 at 03:28:21PM -0800, Kees Cook wrote:
> On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > This reflects much clearer what is being done.
> >
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  Documentation/driver-api/firmware/fallback-mechanisms.rst | 2 +-
> >  drivers/base/firmware_fallback.c                          | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > index 4055ac76b288..f353783ae0be 100644
> > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > @@ -112,7 +112,7 @@ Since a device is created for the sysfs interface to help load firmware as a
> >  fallback mechanism userspace can be informed of the addition of the device by
> >  relying on kobject uevents. The addition of the device into the device
> >  hierarchy means the fallback mechanism for firmware loading has been initiated.
> > -For details of implementation refer to _request_firmware_load(), in particular
> > +For details of implementation refer to fw_load_sysfs_fallback(), in particular
> >  on the use of dev_set_uevent_suppress() and kobject_uevent().
> >
> >  The kernel's kobject uevent mechanism is implemented in lib/kobject_uevent.c,
> > diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c
> > index 13fa5ff2b46c..ce7ccfe82c69 100644
> > --- a/drivers/base/firmware_fallback.c
> > +++ b/drivers/base/firmware_fallback.c
> > @@ -536,7 +536,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
> >  }
> >
> >  /* load a firmware via user helper */
> 
> As long as this is being renamed, maybe add full kern-doc for this function?

Sure why not.

 Luis

Re: [RFT 0/7] firmware: enable caching of firmware for reboot optimization

[cantabile]

On 28/02/18 01:20, Luis R. Rodriguez wrote:
> Cantabile, please give these patches a spin and let me know if it fixes
> your reported issue. They depend on other pending patches I have in line
> waiting to be merged so the easiest I thing I think is for you to test my
> 20180227-firmware-cache branch [0], based on Linus' tree. To get that
> tree, cd into your Linus git tree and do:
> 
> git remote add mcgrof https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git
> git checkout -b 20180227-firmware-cache mcgrof/20180227-firmware-cache
> 
> Please let me know if this resolves your issue and thanks for your report.
> 

I confirmed that request_firmware_cache is called, and the firmware is 
loaded from the cache when resuming from hibernation. The bug is fixed. 
Thank you.

Re: [RFT 0/7] firmware: enable caching of firmware for reboot optimization

[Luis R. Rodriguez]

On Wed, Feb 28, 2018 at 08:03:26PM +0200, cantabile wrote:
> On 28/02/18 01:20, Luis R. Rodriguez wrote:
> > Cantabile, please give these patches a spin and let me know if it fixes
> > your reported issue. They depend on other pending patches I have in line
> > waiting to be merged so the easiest I thing I think is for you to test my
> > 20180227-firmware-cache branch [0], based on Linus' tree. To get that
> > tree, cd into your Linus git tree and do:
> > 
> > git remote add mcgrof https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git
> > git checkout -b 20180227-firmware-cache mcgrof/20180227-firmware-cache
> > 
> > Please let me know if this resolves your issue and thanks for your report.
> > 
> 
> I confirmed that request_firmware_cache is called, and the firmware is
> loaded from the cache when resuming from hibernation. The bug is fixed.

Great, thanks for testing.

Curious, is it also called if you suspend to RAM instead of just using hibernation?

  Luis
> Thank you.
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Re: [RFT 0/7] firmware: enable caching of firmware for reboot optimization

[cantabile]

On 28/02/18 20:45, Luis R. Rodriguez wrote:
> On Wed, Feb 28, 2018 at 08:03:26PM +0200, cantabile wrote:
>> On 28/02/18 01:20, Luis R. Rodriguez wrote:
>>> Cantabile, please give these patches a spin and let me know if it fixes
>>> your reported issue. They depend on other pending patches I have in line
>>> waiting to be merged so the easiest I thing I think is for you to test my
>>> 20180227-firmware-cache branch [0], based on Linus' tree. To get that
>>> tree, cd into your Linus git tree and do:
>>>
>>> git remote add mcgrof https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git
>>> git checkout -b 20180227-firmware-cache mcgrof/20180227-firmware-cache
>>>
>>> Please let me know if this resolves your issue and thanks for your report.
>>>
>>
>> I confirmed that request_firmware_cache is called, and the firmware is
>> loaded from the cache when resuming from hibernation. The bug is fixed.
> 
> Great, thanks for testing.
> 
> Curious, is it also called if you suspend to RAM instead of just using hibernation?
> 

request_firmware_cache is called when resuming from suspend to RAM.