Re: bluetooth related firmware loader spew on resume.

At Wed, 26 Nov 2014 16:21:49 +0100,
Takashi Iwai wrote:

At Wed, 26 Nov 2014 15:27:57 +0100,
Oliver Neukum wrote:

quoted

On Wed, 2014-11-26 at 15:12 +0100, Takashi Iwai wrote:

quoted

At Wed, 26 Nov 2014 23:05:20 +0900,
Marcel Holtmann wrote:

quoted

Hi Oliver,

quoted

quoted

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;
 }