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; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/