On Sun, 25 Feb 2018 17:54:25 +0000, Luis R. Rodriguez wrote: > On Mon, Feb 19, 2018 at 05:01:27PM +0200, cantabile wrote: > > On 19/02/18 07:55, Jakub Kicinski wrote: > > > On Sat, 17 Feb 2018 13:23:29 +0200, cantabile wrote: > > > > > Thanks for the info. Would it be cleaner to EXPORT fw_add_devm_name() > > > > > and just call that in case driver sees FW is already loaded? That > > > > > should inform the fw subsystem that we want the image around in case of > > > > > hibernation, but there is no need to load it immediately? > > > > > > > > No, I don't believe it's cleaner to expose a private function that you > > > > don't even really need. Remember that calling request_firmware every > > > > time your driver's probe and resume functions are called is normal. It's > > > > the expected behaviour. > > > > > > I'm asking you the extend functionality of a subsystem to be able to > > > cleanly communicate the intent. Not export internal functions. > > > > > > Requesting firmware you don't need and risking failing probe even if FW > > > is already pre-loaded is not correct. Reordering you suggest is > > > brittle and makes little logical sense unless someone guesses your use > > > case. > > > > > > Please at least try to do as advised. Otherwise: > > > > > > Nacked-by: Jakub Kicinski <kubakici@xxxxx> > > > > > > > You're right about the reordering not making sense to someone unfamiliar > > with the problem. I can fix that with a comment. > > > > I can change the patch so that request_firmware will only make the probe > > function fail if the firmware is not already running. > > Note that using request_firmware() on probe typically is also not an > outstanding idea given it delays boot. Not because looking for the firmware > takes time, but instead because processing firmware typically does on > the device. For instance cxgb4 is an example device where processing > firmware takes a long time. > > Delays on probe may mean the "feel good" immediate desktop coming up is delyed. > > Specially if its networking... I see no reason why to process firmware on probe. > > If one can use a workqueue to process verifying if it needs firmware and loading > later, that's more advisable. Quite true, more advanced the FW the longer FW load takes :( Although I would be cautious not to cause issues for network/NFS boot... Perhaps it can wait for such workqueue to finish? > Now, that's all a side topic. > > I will for now agree that it seems pointless to request for firmware always > even if you don't need to, and all you want is to just cache the firmware > on suspend. So I welcome a patch but the justification for it really needs to > be documented very well, and the documentation extended as such. In fact > maybe rename the function to something more sensible. > > Another use case for the firmware cache (which we need to add the documentation) > is that for hibernation we suspend all devices first, get a snapshot, and then > resume devices so we can then write the snapshot to disk. On that resume step > I don't think devices have access to the hard drive for firmware, so cache is > all we have. This may need some confirmation but I suspect this is the case. > Drivers needing firmware on resume for hibernation may need to cache their > firmware. > > I want to understand the case where the firmware is *not* available on resume? > Why did that happen? I seem to have read that on a fresh reboot the firmware > was not needed, and so on probe request_firmware() was not called? Why would > firmware not be required on a reboot? Yes, that is a good question.. John, do you have a theory? My initial thought was that the UEFI/BIOS loads it during pre-boot, but this is a USB card, so it's a bit unlikely that UEFI will have a driver for it... Does this happen when rebooting maybe?
