| Message ID | 20180310141501.2214-20-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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,
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
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 --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
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(+)