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/
