Return-Path: Date: Thu, 27 Nov 2014 15:43:57 +0100 Message-ID: From: Takashi Iwai To: Oliver Neukum Cc: Marcel Holtmann , Dave Jones , Linux Kernel , pgynther@google.com, linux-bluetooth@vger.kernel.org Subject: Re: bluetooth related firmware loader spew on resume. In-Reply-To: References: <20141111181228.GA27815@redhat.com> <1416996623.3171.7.camel@linux-0dmf.site> <0E1D95E7-64D2-4BEA-AFAA-4B119838F24E@holtmann.org> <1417012077.5928.11.camel@linux-0dmf.site> MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII List-ID: 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; }