diff mbox

[v3,19/20] firmware: add request_firmware_cache() to help with cache on reboot

Message ID 20180310141501.2214-20-mcgrof@kernel.org (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Luis Chamberlain March 10, 2018, 2:15 p.m. UTC
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/main.c                | 24 ++++++++++++++++++++++
 include/linux/firmware.h                           |  3 +++
 3 files changed, 41 insertions(+)

Comments

Greg Kroah-Hartman March 20, 2018, 8:30 a.m. UTC | #1
On Sat, Mar 10, 2018 at 06:15:00AM -0800, Luis R. Rodriguez wrote:
> 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/main.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/main.c b/drivers/base/firmware_loader/main.c
> index 2913bb0e5e7b..04bf853f60e9 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.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);

I know you are just following the existing naming scheme, but please
let's not continue the problem here.  Can you prefix all of the firmware
exported symbols with "firmware_", and then the verb.  So this would be
"firmware_request_cache", and the older function would be
"firmware_request_direct".

I've stopped here in the patch series, applying the others now, so feel
free to rebase and resend what I've missed, and the minor fixups for the
prior patches.

thanks,

greg k-h
Luis Chamberlain March 20, 2018, 5:34 p.m. UTC | #2
On Tue, Mar 20, 2018 at 09:30:55AM +0100, Greg KH wrote:
> On Sat, Mar 10, 2018 at 06:15:00AM -0800, Luis R. Rodriguez wrote:
> > +EXPORT_SYMBOL_GPL(request_firmware_cache);
> 
> I know you are just following the existing naming scheme, but please
> let's not continue the problem here.  Can you prefix all of the firmware
> exported symbols with "firmware_", and then the verb.  So this would be
> "firmware_request_cache",

Sure.

>  and the older function would be
> "firmware_request_direct".

Sure, so you want to also rename the old exported symbols, and add a macro
or static inline to enable use of the older names?

> I've stopped here in the patch series, applying the others now, so feel
> free to rebase and resend what I've missed, and the minor fixups for the
> prior patches.

Sure thing.

   Luis
Greg Kroah-Hartman March 20, 2018, 5:38 p.m. UTC | #3
On Tue, Mar 20, 2018 at 05:34:09PM +0000, Luis R. Rodriguez wrote:
> On Tue, Mar 20, 2018 at 09:30:55AM +0100, Greg KH wrote:
> > On Sat, Mar 10, 2018 at 06:15:00AM -0800, Luis R. Rodriguez wrote:
> > > +EXPORT_SYMBOL_GPL(request_firmware_cache);
> > 
> > I know you are just following the existing naming scheme, but please
> > let's not continue the problem here.  Can you prefix all of the firmware
> > exported symbols with "firmware_", and then the verb.  So this would be
> > "firmware_request_cache",
> 
> Sure.
> 
> >  and the older function would be
> > "firmware_request_direct".
> 
> Sure, so you want to also rename the old exported symbols, and add a macro
> or static inline to enable use of the older names?

Renaming is best, let's not keep them around for no good reason :)

thanks,

greg k-h
Luis Chamberlain March 20, 2018, 6:24 p.m. UTC | #4
On Tue, Mar 20, 2018 at 06:38:01PM +0100, Greg KH wrote:
> On Tue, Mar 20, 2018 at 05:34:09PM +0000, Luis R. Rodriguez wrote:
> > On Tue, Mar 20, 2018 at 09:30:55AM +0100, Greg KH wrote:
> > > On Sat, Mar 10, 2018 at 06:15:00AM -0800, Luis R. Rodriguez wrote:
> > > > +EXPORT_SYMBOL_GPL(request_firmware_cache);
> > > 
> > > I know you are just following the existing naming scheme, but please
> > > let's not continue the problem here.  Can you prefix all of the firmware
> > > exported symbols with "firmware_", and then the verb.  So this would be
> > > "firmware_request_cache",
> > 
> > Sure.
> > 
> > >  and the older function would be
> > > "firmware_request_direct".
> > 
> > Sure, so you want to also rename the old exported symbols, and add a macro
> > or static inline to enable use of the older names?
> 
> Renaming is best, let's not keep them around for no good reason :)

Sure thing, I'll rename the old firmware calls.

Since that would be a cross-tree collateral evolution, as we have discussed
before over these at kernel summits, I'd prefer to leave that large set of
renames toward the end of the series, as the last patch. This way the
Coccinelle SmPL / sed script could be run at the very end, and in case of merge
conflicts re-run.

Which BTW... *long term*, I believe a possible way to address these cross-tree
collateral evolutions, due to new merge updates and conflicts, could be for us
to embed the SmPL / sed scripts into git notes, and in turn have an *optional* git
configuration which could attempt a merge conflict resolution through
alternative means, one of which -- if enabled -- could be to try to run the SmPL or
sed script in the attached git note (if you downloaded them). One of the other
benefits of this is the commit log is no longer visually impaired by such long
semantic patches or scripts, and gives us a nice way to pull these up through a
generic tooling mechanism. This could all just be optional, no one would have
to push or fetch these -- only if they wanted the possible added benefits of
having them.

*Iff* this seems sensible, this would only mean kernel.org would have to start
accepting git notes long term, for those who optionally want to push them or
fetch them.

FWIW I've confirmed with github that they now accepts git notes, the UI for
github just needs time to develop how to interact with them, for now they just
hide them. One of the difficulties with a UI interface for them is that by
default git notes *do not* use a namespace reflective for a branch or anything.
We could stick to a actual namespace for branch / commit:

"iso" for isomorphism

git note --ref branch/iso-coccinelle/ add commit-id
vi refs/notes/branch/iso-coccinelle/commit-id
git note --ref branch/iso-coccinelle/ edit commit-id

This is rather crude though so simple interface such as the following could
be added as well:

git smpl add sp.cocci commit-id
git smpl edit commit-id
git smpl list v4.13..v4.14

Similar thing could be done for crash dumps, if we wanted...

Since this still would need to be discussed, and if agreed then implemented, in
the meantime I can just do it the old school way of embedding the SmPL and/or
sed script into the commit log, but left as the last patch in the series.

  Luis
Konstantin Ryabitsev March 20, 2018, 6:54 p.m. UTC | #5
On 03/20/18 14:24, Luis R. Rodriguez wrote:
> *Iff* this seems sensible, this would only mean kernel.org would have to start
> accepting git notes long term, for those who optionally want to push them or
> fetch them.

We don't disallow them. You just need to make sure you're fetching and
pushing them, because they aren't by default. You can set this in your
kernel.org origin section of .git/config:

[remote origin]
  ...
  fetch = +refs/notes/*:refs/notes/*

And then push them separately as "git push origin refs/notes/*".

Since frontends clone with --mirror, the notes will be available on all
git.kernel.org nodes (e.g. see
https://git.kernel.org/mricon/hook-test/c/a8d310d4c13)

If you do start using notes, I strongly suggest you pick a dedicated
refspace for it instead of putting things into the default
refs/notes/commits, e.g.:

git notes --ref crosstree [...]

This will create refs/notes/crosstree that has less of a chance to be
clobbered by someone else's use of notes.

Best,
Luis Chamberlain March 20, 2018, 7:51 p.m. UTC | #6
On Tue, Mar 20, 2018 at 02:54:44PM -0400, Konstantin Ryabitsev wrote:
> On 03/20/18 14:24, Luis R. Rodriguez wrote:
> > *Iff* this seems sensible, this would only mean kernel.org would have to start
> > accepting git notes long term, for those who optionally want to push them or
> > fetch them.
> 
> We don't disallow them. You just need to make sure you're fetching and
> pushing them, because they aren't by default. You can set this in your
> kernel.org origin section of .git/config:
> 
> [remote origin]
>   ...
>   fetch = +refs/notes/*:refs/notes/*
> 
> And then push them separately as "git push origin refs/notes/*".

Superb, thanks!

> Since frontends clone with --mirror, the notes will be available on all
> git.kernel.org nodes (e.g. see
> https://git.kernel.org/mricon/hook-test/c/a8d310d4c13)

Good to know thanks, and it looks good!

> If you do start using notes, I strongly suggest you pick a dedicated
> refspace for it instead of putting things into the default
> refs/notes/commits, e.g.:
> 
> git notes --ref crosstree [...]
> 
> This will create refs/notes/crosstree that has less of a chance to be
> clobbered by someone else's use of notes.

Indeed, the default is crap. I was suggesting perhaps having it also be
per branch, so:

git note --ref branch/iso-coccinelle/ add commit-id

If it was required sed I guess

git note --ref branch/sed/ add commit-id

And if we wanted a resolution handler which could manage the above for you:

git note --ref branch/merger-script add commit-id

Which could use the above two to run what is needed to mimic the commit.

  Luis
Greg Kroah-Hartman March 21, 2018, 10:04 a.m. UTC | #7
On Tue, Mar 20, 2018 at 06:24:09PM +0000, Luis R. Rodriguez wrote:
> On Tue, Mar 20, 2018 at 06:38:01PM +0100, Greg KH wrote:
> > On Tue, Mar 20, 2018 at 05:34:09PM +0000, Luis R. Rodriguez wrote:
> > > On Tue, Mar 20, 2018 at 09:30:55AM +0100, Greg KH wrote:
> > > > On Sat, Mar 10, 2018 at 06:15:00AM -0800, Luis R. Rodriguez wrote:
> > > > > +EXPORT_SYMBOL_GPL(request_firmware_cache);
> > > > 
> > > > I know you are just following the existing naming scheme, but please
> > > > let's not continue the problem here.  Can you prefix all of the firmware
> > > > exported symbols with "firmware_", and then the verb.  So this would be
> > > > "firmware_request_cache",
> > > 
> > > Sure.
> > > 
> > > >  and the older function would be
> > > > "firmware_request_direct".
> > > 
> > > Sure, so you want to also rename the old exported symbols, and add a macro
> > > or static inline to enable use of the older names?
> > 
> > Renaming is best, let's not keep them around for no good reason :)
> 
> Sure thing, I'll rename the old firmware calls.

Ah, in looking at this closer, that might take a bit of time, as there
are a _lot_ of callers of request_firmware().  So maybe a macro/wrapper
is good for that one so we can propagate the changes through the
different subsystems and do this over a few kernel releases.

But for anything new, let's get it right the first time :)

thanks,

greg k-h
diff mbox

Patch

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/main.c b/drivers/base/firmware_loader/main.c
index 2913bb0e5e7b..04bf853f60e9 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.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