| Message ID | s5hwq04zypv.wl-tiwai@suse.de |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 19 May 2015, Takashi Iwai wrote: > > > I am not convinced. Now we are hacking the Bluetooth core layer > > > (which has nothing to do with the drivers suspend/resume or > > > probe) to do something different so that we do not see this > > > warning. > > > > > > I can not do anything about the platform in question choosing a > > > unplug/replug for suspend/resume instead of having a proper USB > > > suspend and resume handling. That is pretty much out of our > > > control. Actually one can do something about this. I mean, one _can_ implement proper USB suspend and resume handling in the Bluetooth driver. At this point the details aren't clear to me, but perhaps if the driver in question had a reset_resume callback then it might work better. > > > I would rather have the USB subsystem delay the probe() > > > callback if we tell it to. This is possible. I am not sure it would be the right thing to do, though. What happens if the probe routine gets called early on during the boot-up procedure, before userspace is up and running? The same thing should happen here. > > > Of just have request_firmware() > > > actually sleep until userspace is ready. Seriously, why is > > > request_firmware not just sleeping for us. It won't work. The request_firmware call is part of the probe sequence, which in turn is part of the resume sequence. Userspace doesn't start running again until the resume sequence is finished. If request_firmware waited for userspace, it would hang. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-05-19 at 10:26 -0400, Alan Stern wrote: > On Tue, 19 May 2015, Takashi Iwai wrote: > > > > > I am not convinced. Now we are hacking the Bluetooth core layer > > > > (which has nothing to do with the drivers suspend/resume or > > > > probe) to do something different so that we do not see this > > > > warning. > > > > > > > > I can not do anything about the platform in question choosing a > > > > unplug/replug for suspend/resume instead of having a proper USB > > > > suspend and resume handling. That is pretty much out of our > > > > control. > > Actually one can do something about this. I mean, one _can_ implement > proper USB suspend and resume handling in the Bluetooth driver. At > this point the details aren't clear to me, but perhaps if the driver in > question had a reset_resume callback then it might work better. I doubt this would work. By losing power the BT controller is thrown out of its cell. It looks to me like fundamentally BT needs to fully reestablish the network from scratch after a loss of power. > > > > I would rather have the USB subsystem delay the probe() > > > > callback if we tell it to. > > This is possible. I am not sure it would be the right thing to do, > though. What happens if the probe routine gets called early on during > the boot-up procedure, before userspace is up and running? The same > thing should happen here. Yes. Basically if you want firmware during probe the firmware infrastructure has to be there. That is if you build such a module statically the firmware must be included in the kernel image. > > > > Of just have request_firmware() > > > > actually sleep until userspace is ready. Seriously, why is > > > > request_firmware not just sleeping for us. > > It won't work. The request_firmware call is part of the probe > sequence, which in turn is part of the resume sequence. Userspace > doesn't start running again until the resume sequence is finished. If > request_firmware waited for userspace, it would hang. I'd recommend the sledge hammer. Never free the firmware while the hardware is connected or the system sleeping. If you must do this there is a notifier chain. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, >>>> I am not convinced. Now we are hacking the Bluetooth core layer >>>> (which has nothing to do with the drivers suspend/resume or >>>> probe) to do something different so that we do not see this >>>> warning. >>>> >>>> I can not do anything about the platform in question choosing a >>>> unplug/replug for suspend/resume instead of having a proper USB >>>> suspend and resume handling. That is pretty much out of our >>>> control. > > Actually one can do something about this. I mean, one _can_ implement > proper USB suspend and resume handling in the Bluetooth driver. At > this point the details aren't clear to me, but perhaps if the driver in > question had a reset_resume callback then it might work better. the btusb.ko driver has suspend/resume support. Are you saying we also need reset_resume support? >>>> I would rather have the USB subsystem delay the probe() >>>> callback if we tell it to. > > This is possible. I am not sure it would be the right thing to do, > though. What happens if the probe routine gets called early on during > the boot-up procedure, before userspace is up and running? The same > thing should happen here. For modules this will be hard. Since you need userspace before being able to load the modules. If built-in code, then in theory this might be possible. Depending on the order of the init sections. >>>> Of just have request_firmware() >>>> actually sleep until userspace is ready. Seriously, why is >>>> request_firmware not just sleeping for us. > > It won't work. The request_firmware call is part of the probe > sequence, which in turn is part of the resume sequence. Userspace > doesn't start running again until the resume sequence is finished. If > request_firmware waited for userspace, it would hang. Then I really have no idea on how to solve this unless we silence the warning from request_firmware. From a driver perspective we go back trough probe(). So the driver has to treat this as a new device. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Tue, 19 May 2015 10:26:46 -0400 (EDT), Alan Stern wrote: > > > > > Of just have request_firmware() > > > > actually sleep until userspace is ready. Seriously, why is > > > > request_firmware not just sleeping for us. > > It won't work. The request_firmware call is part of the probe > sequence, which in turn is part of the resume sequence. Userspace > doesn't start running again until the resume sequence is finished. If > request_firmware waited for userspace, it would hang. Note that the recent request_firmware() doesn't need the user-space invocation (unless the fallback is explicitly enabled) but loads the file directly. And, request_firmware() for the cached data is valid to be called in the resume path. Takashi -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 19 May 2015, Marcel Holtmann wrote: > Hi Alan, > > >>>> I am not convinced. Now we are hacking the Bluetooth core layer > >>>> (which has nothing to do with the drivers suspend/resume or > >>>> probe) to do something different so that we do not see this > >>>> warning. > >>>> > >>>> I can not do anything about the platform in question choosing a > >>>> unplug/replug for suspend/resume instead of having a proper USB > >>>> suspend and resume handling. That is pretty much out of our > >>>> control. > > > > Actually one can do something about this. I mean, one _can_ implement > > proper USB suspend and resume handling in the Bluetooth driver. At > > this point the details aren't clear to me, but perhaps if the driver in > > question had a reset_resume callback then it might work better. > > the btusb.ko driver has suspend/resume support. Are you saying we > also need reset_resume support? I don't know; I'm not familiar enough with how Bluetooth works. If the device loses power and requires its firmware to be loaded again, then a reset_resume would end up doing much the same thing as probe anyway. So implementing reset_resume might not make much difference. > >>>> I would rather have the USB subsystem delay the probe() > >>>> callback if we tell it to. > > > > This is possible. I am not sure it would be the right thing to do, > > though. What happens if the probe routine gets called early on during > > the boot-up procedure, before userspace is up and running? The same > > thing should happen here. > > For modules this will be hard. Since you need userspace before being > able to load the modules. If built-in code, then in theory this might > be possible. Depending on the order of the init sections. Yes, I meant built-in. > >>>> Of just have request_firmware() > >>>> actually sleep until userspace is ready. Seriously, why is > >>>> request_firmware not just sleeping for us. > > > > It won't work. The request_firmware call is part of the probe > > sequence, which in turn is part of the resume sequence. Userspace > > doesn't start running again until the resume sequence is finished. If > > request_firmware waited for userspace, it would hang. > > Then I really have no idea on how to solve this unless we silence the > warning from request_firmware. From a driver perspective we go back > trough probe(). So the driver has to treat this as a new device. Oliver's suggestion to keep the firmware in memory may be the only reasonable solution. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-05-19 at 19:13 +0200, Takashi Iwai wrote: > At Tue, 19 May 2015 10:26:46 -0400 (EDT), > Alan Stern wrote: > > > > > > > Of just have request_firmware() > > > > > actually sleep until userspace is ready. Seriously, why is > > > > > request_firmware not just sleeping for us. > > > > It won't work. The request_firmware call is part of the probe > > sequence, which in turn is part of the resume sequence. Userspace > > doesn't start running again until the resume sequence is finished. If > > request_firmware waited for userspace, it would hang. > > Note that the recent request_firmware() doesn't need the user-space > invocation (unless the fallback is explicitly enabled) but loads the That is a dangerous approach. You cannot be sure you can do file IO. It depends on the exact shape of the device tree. > file directly. And, request_firmware() for the cached data is valid > to be called in the resume path. Well, yes, if your data is cached in RAM, all is well. But that leads to the same problem one step further. What must be cached? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Tue, 19 May 2015 19:42:55 +0200, Oliver Neukum wrote: > > On Tue, 2015-05-19 at 19:13 +0200, Takashi Iwai wrote: > > At Tue, 19 May 2015 10:26:46 -0400 (EDT), > > Alan Stern wrote: > > > > > > > > > Of just have request_firmware() > > > > > > actually sleep until userspace is ready. Seriously, why is > > > > > > request_firmware not just sleeping for us. > > > > > > It won't work. The request_firmware call is part of the probe > > > sequence, which in turn is part of the resume sequence. Userspace > > > doesn't start running again until the resume sequence is finished. If > > > request_firmware waited for userspace, it would hang. > > > > Note that the recent request_firmware() doesn't need the user-space > > invocation (unless the fallback is explicitly enabled) but loads the > > That is a dangerous approach. You cannot be sure you can do file IO. > It depends on the exact shape of the device tree. It's the reason why firmware loader still takes UMH lock (thus we're seeing this very problem). > > file directly. And, request_firmware() for the cached data is valid > > to be called in the resume path. > > Well, yes, if your data is cached in RAM, all is well. But that leads > to the same problem one step further. What must be cached? The data is cached in RAM. More specifically, the former loaded firmware files are reloaded and saved at suspend for each device object. See fw_pm_notify() in firmware_class.c. The question is then why the cached data isn't used. I have no concrete answer to it for now, need more investigation, but my wild guess is that it's because the device itself is being renewed. Or, something wrong in firmware_class.c. Takashi -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-05-20 at 08:29 +0200, Takashi Iwai wrote: > The data is cached in RAM. More specifically, the former loaded > firmware files are reloaded and saved at suspend for each device > object. See fw_pm_notify() in firmware_class.c. OK, this may be a stupid idea, but do we know the firmware was successfully loaded in the first place? Also btusb is in the habit of falling back to a generic firmware in some places. It seems to me that caching firmware is conceptually not enough, but we'd also need to record the absence of firmware images. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Oliver, >> The data is cached in RAM. More specifically, the former loaded >> firmware files are reloaded and saved at suspend for each device >> object. See fw_pm_notify() in firmware_class.c. > > OK, this may be a stupid idea, but do we know the firmware > was successfully loaded in the first place? > Also btusb is in the habit of falling back to a generic > firmware in some places. It seems to me that caching > firmware is conceptually not enough, but we'd also need > to record the absence of firmware images. in a lot of cases the firmware is optional. The device will operate fine without the firmware. There are a few devices where the firmware is required, but for many it just contains patches. It would be nice if we could tell request_firmware() if it is optional or mandatory firmware. Or if it should just cache the status of a missing firmware as well. As long as the device in question gets disconnected and we run through the USB driver probe() callback again, the btusb.ko driver can not do anything smart in this case. It has to be done in request_firmware(). Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 20, 2015 at 4:40 PM, Oliver Neukum <oneukum@suse.com> wrote: > On Wed, 2015-05-20 at 08:29 +0200, Takashi Iwai wrote: >> The data is cached in RAM. More specifically, the former loaded >> firmware files are reloaded and saved at suspend for each device >> object. See fw_pm_notify() in firmware_class.c. > > OK, this may be a stupid idea, but do we know the firmware > was successfully loaded in the first place? Yes, the firmware loader records that as device resource. In reality, there won't be lots of devices requiring firmware in one running system, so the idea of caching for every successful loading is workable. > Also btusb is in the habit of falling back to a generic > firmware in some places. It seems to me that caching > firmware is conceptually not enough, but we'd also need > to record the absence of firmware images. The caching can't cover the case which starts to load fw during resume in the 1st time. > > Regards > Oliver > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 171841ad1008..47eb5551c119 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -97,21 +97,6 @@ static inline long firmware_loading_timeout(void) return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET; } -/* firmware behavior options */ -#define FW_OPT_UEVENT (1U << 0) -#define FW_OPT_NOWAIT (1U << 1) -#ifdef CONFIG_FW_LOADER_USER_HELPER -#define FW_OPT_USERHELPER (1U << 2) -#else -#define FW_OPT_USERHELPER 0 -#endif -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK -#define FW_OPT_FALLBACK FW_OPT_USERHELPER -#else -#define FW_OPT_FALLBACK 0 -#endif -#define FW_OPT_NO_WARN (1U << 3) - struct firmware_cache { /* firmware_buf instance will be added into the below list */ spinlock_t lock; @@ -1085,7 +1070,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device, } /* called from request_firmware() and request_firmware_work_func() */ -static int +int _request_firmware(const struct firmware **firmware_p, const char *name, struct device *device, unsigned int opt_flags) { @@ -1099,13 +1084,16 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!name || name[0] == '\0') return -EINVAL; + /* Need to pin this module until return */ + __module_get(THIS_MODULE); + ret = _request_firmware_prepare(&fw, name, device); if (ret <= 0) /* error or already assigned */ goto out; ret = 0; timeout = firmware_loading_timeout(); - if (opt_flags & FW_OPT_NOWAIT) { + if (opt_flags & FW_OPT_UMH_LOCK_WAIT) { timeout = usermodehelper_read_lock_wait(timeout); if (!timeout) { dev_dbg(device, "firmware: %s loading timed out\n", @@ -1147,67 +1135,10 @@ _request_firmware(const struct firmware **firmware_p, const char *name, } *firmware_p = fw; - return ret; -} - -/** - * request_firmware: - send firmware request and wait for it - * @firmware_p: pointer to firmware image - * @name: name of firmware file - * @device: device for which firmware is being loaded - * - * @firmware_p will be used to return a firmware image by the name - * of @name for device @device. - * - * Should be called from user context where sleeping is allowed. - * - * @name will be used as $FIRMWARE in the uevent environment and - * should be distinctive enough not to be confused with any other - * firmware image for this or any other device. - * - * Caller must hold the reference count of @device. - * - * The function can be called safely inside device's suspend and - * resume callback. - **/ -int -request_firmware(const struct firmware **firmware_p, const char *name, - struct device *device) -{ - int ret; - - /* Need to pin this module until return */ - __module_get(THIS_MODULE); - ret = _request_firmware(firmware_p, name, device, - FW_OPT_UEVENT | FW_OPT_FALLBACK); - module_put(THIS_MODULE); - return ret; -} -EXPORT_SYMBOL(request_firmware); - -/** - * request_firmware_direct: - load firmware directly without usermode helper - * @firmware_p: pointer to firmware image - * @name: name of firmware file - * @device: device for which firmware is being loaded - * - * This function works pretty much like request_firmware(), but this doesn't - * fall back to usermode helper even if the firmware couldn't be loaded - * directly from fs. Hence it's useful for loading optional firmwares, which - * aren't always present, without extra long timeouts of udev. - **/ -int request_firmware_direct(const struct firmware **firmware_p, - const char *name, struct device *device) -{ - int ret; - - __module_get(THIS_MODULE); - ret = _request_firmware(firmware_p, name, device, - FW_OPT_UEVENT | FW_OPT_NO_WARN); module_put(THIS_MODULE); return ret; } -EXPORT_SYMBOL_GPL(request_firmware_direct); +EXPORT_SYMBOL_GPL(_request_firmware); /** * release_firmware: - release the resource associated with a firmware image @@ -1291,6 +1222,7 @@ request_firmware_nowait( fw_work->context = context; fw_work->cont = cont; fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK | + FW_OPT_UMH_LOCK_WAIT | (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER); if (!try_module_get(module)) { diff --git a/include/linux/firmware.h b/include/linux/firmware.h index 5c41c5e75b5c..460aa30965cf 100644 --- a/include/linux/firmware.h +++ b/include/linux/firmware.h @@ -39,23 +39,21 @@ struct builtin_fw { __used __section(.builtin_fw) = { name, blob, size } #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE)) -int request_firmware(const struct firmware **fw, const char *name, - struct device *device); +int _request_firmware(const struct firmware **firmware_p, const char *name, + struct device *device, unsigned int opt_flags); int request_firmware_nowait( struct module *module, bool uevent, const char *name, struct device *device, gfp_t gfp, void *context, void (*cont)(const struct firmware *fw, void *context)); -int request_firmware_direct(const struct firmware **fw, const char *name, - struct device *device); - void release_firmware(const struct firmware *fw); #else -static inline int request_firmware(const struct firmware **fw, - const char *name, - struct device *device) +static inline int +_request_firmware(const struct firmware **firmware_p, const char *name, + struct device *device, unsigned int opt_flags) { return -EINVAL; } + static inline int request_firmware_nowait( struct module *module, bool uevent, const char *name, struct device *device, gfp_t gfp, void *context, @@ -67,13 +65,69 @@ static inline int request_firmware_nowait( static inline void release_firmware(const struct firmware *fw) { } +#endif -static inline int request_firmware_direct(const struct firmware **fw, - const char *name, - struct device *device) +/* firmware behavior options */ +#define FW_OPT_UEVENT (1U << 0) +#define FW_OPT_NOWAIT (1U << 1) +#ifdef CONFIG_FW_LOADER_USER_HELPER +#define FW_OPT_USERHELPER (1U << 2) +#else +#define FW_OPT_USERHELPER 0 +#endif +#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK +#define FW_OPT_FALLBACK FW_OPT_USERHELPER +#else +#define FW_OPT_FALLBACK 0 +#endif +#define FW_OPT_NO_WARN (1U << 3) +#define FW_OPT_UMH_LOCK_WAIT (1U << 4) + +/** + * request_firmware: - send firmware request and wait for it + * @firmware_p: pointer to firmware image + * @name: name of firmware file + * @device: device for which firmware is being loaded + * + * @firmware_p will be used to return a firmware image by the name + * of @name for device @device. + * + * Should be called from user context where sleeping is allowed. + * + * @name will be used as $FIRMWARE in the uevent environment and + * should be distinctive enough not to be confused with any other + * firmware image for this or any other device. + * + * Caller must hold the reference count of @device. + * + * The function can be called safely inside device's suspend and + * resume callback. + **/ +static inline int +request_firmware(const struct firmware **firmware_p, const char *name, + struct device *device) { - return -EINVAL; + return _request_firmware(firmware_p, name, device, + FW_OPT_UEVENT | FW_OPT_FALLBACK); +} + +/** + * request_firmware_direct: - load firmware directly without usermode helper + * @firmware_p: pointer to firmware image + * @name: name of firmware file + * @device: device for which firmware is being loaded + * + * This function works pretty much like request_firmware(), but this doesn't + * fall back to usermode helper even if the firmware couldn't be loaded + * directly from fs. Hence it's useful for loading optional firmwares, which + * aren't always present, without extra long timeouts of udev. + **/ +static inline int +request_firmware_direct(const struct firmware **firmware_p, + const char *name, struct device *device) +{ + return _request_firmware(firmware_p, name, device, + FW_OPT_UEVENT | FW_OPT_NO_WARN); } -#endif #endif