Re: bluetooth related firmware loader spew on resume.

[Takashi Iwai <tiwai@suse.de>]

At Wed, 26 Nov 2014 16:21:49 +0100,
Takashi Iwai wrote:
> 
> At Wed, 26 Nov 2014 15:27:57 +0100,
> Oliver Neukum wrote:
> > 
> > On Wed, 2014-11-26 at 15:12 +0100, Takashi Iwai wrote:
> > > At Wed, 26 Nov 2014 23:05:20 +0900,
> > > Marcel Holtmann wrote:
> > > > 
> > > > Hi Oliver,
> > > > 
> > > > >> In order to paper over this, we may also remember the failing firmware
> > > > >> and avoid loading it.  This might be an easer way than the endless
> > > > >> fight against UMH race...
> > > > > 
> > > > > 
> > > > > the full fix would be to implement reset_resume() for btusb.
> > > > > It seems to me that setup() should be split in two methods,
> > > > > one to request the firmware from user space and the second
> > > > > to transfer it to the device. reset_resume() would just need
> > > > > to repeat the second operation.
> > > > 
> > > > so when you do hci_register_dev, then hdev->setup is only called once. I really mean only once per lifetime of the hci_dev. So you would need to unregister the hci_dev first before hdev->setup will ever be called again. So I am not sure this is actually the problem here. The problem here is entirely within request_firmware() unless of course we run through the USB probe handlers again. Which I do not see happening here.
> > > > 
> > > > And we have hdev->setup this way since normally the Bluetooth devices keep their firmware patches and not forget about them and suspend-resume cycles. If the USB device of course jumps of the bus during it then all bets are off anyway.
> > > 
> > > Usually you can avoid unnecessary rebinding when you provide a proper
> > > reset_resume callback.  I guess that's what Oliver suggested.
> > 
> > Yes, but even in reset_resume() you would need to redo the setup
> > part, as the device lost power. The basic problem of requesting
> > the firmware wouldn't be solved.
> 
> Requesting the firmware in the resume path itself is OK.  There are
> many drivers doing so.  But the primary problem in btusb is that it's
> triggered at the wrong timing.  (And the second problem is that the
> firmware loader doesn't cache the non-existing files, so it goes
> through lengthy code paths for reconfirming that the file doesn't
> exist.)

So, below is a quick hack for avoiding the f/w loader activation
during resume.  I just compile-tested, so no idea whether it really
works or not.


Takashi

---
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 3d785ebb48d3..e928bde45d7d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -167,6 +167,7 @@ struct fw_name_devm {
 
 #define	FW_LOADER_NO_CACHE	0
 #define	FW_LOADER_START_CACHE	1
+#define	FW_LOADER_CACHE_ONLY	2
 
 static int fw_cache_piggyback_on_request(const char *name);
 
@@ -1112,6 +1113,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
+	if (fw_cache.state == FW_LOADER_CACHE_ONLY) {
+		ret = -ENOENT;
+		goto out;
+	}
+
 	ret = 0;
 	timeout = firmware_loading_timeout();
 	if (opt_flags & FW_OPT_NOWAIT) {
@@ -1633,7 +1639,8 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 /* stop caching firmware once syscore_suspend is reached */
 static int fw_suspend(void)
 {
-	fw_cache.state = FW_LOADER_NO_CACHE;
+	/* use only cached firmware until fully resumed */
+	fw_cache.state = FW_LOADER_CACHE_ONLY;
 	return 0;
 }