RE: [RFC 2/5] iwlwifi: fix request_module() use
From: Grumbach, Emmanuel <hidden>
Date: 2017-02-19 09:48:07
Also in:
lkml
The return value of request_module() being 0 does not mean that the driver which was requested has loaded. To properly check that the driver was loaded each driver can use internal mechanisms to vet the driver is now present. The helper try_then_request_module() was added to help with this, allowing drivers to specify their own validation as the first argument. On iwlwifi the use case is a bit more complicated given that the value we need to check for is protected with a mutex later used on the module_init() of the module we are asking for. If we were to lock and request_module() we'd deadlock. iwlwifi needs its own wrapper then so it can handle its special locking requirements. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
I don't see the problem with the current code. We don't assume that everything has been
loaded immediately after request_module returns. We just free the intermediate firmware
structures that won't be using anymore. What happens here is that after request_module
returns, we patiently wait until it is loaded, and when that happens, iwl{d,m}vm's init function
will be called. That one is the one that continues the flow by calling:
ret = iwl_opmode_register("iwlmvm", &iwl_mvm_ops);
(for the iwlmvm case).
Where am I wrong here?
quoted hunk ↗ jump to hunk
--- drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 34 ++++++++++++++++++++--- ----- 1 file changed, 25 insertions(+), 9 deletions(-)diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.cb/drivers/net/wireless/intel/iwlwifi/iwl-drv.c index e198d6f5fcea..6beb92d19ea7 100644--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c@@ -1231,6 +1231,29 @@ static void _iwl_op_mode_stop(struct iwl_drv*drv) } } +static void iwlwifi_try_load_op(struct iwlwifi_opmode_table *op, + struct iwl_drv *drv) +{ + int ret = 0; + + ret = request_module("%s", op->name); + if (ret) + goto out; + + mutex_lock(&iwlwifi_opmode_table_mtx); + if (!op->ops) + ret = -ENOENT; + mutex_unlock(&iwlwifi_opmode_table_mtx); + +out: +#ifdef CONFIG_IWLWIFI_OPMODE_MODULAR + if (ret) + IWL_ERR(drv, + "failed to load module %s (error %d), is dynamic loading enabled?\n", + op->name, ret); +#endif +} + /** * iwl_req_fw_callback - callback when firmware was loaded *@@ -1471,15 +1494,8 @@ static void iwl_req_fw_callback(const structfirmware *ucode_raw, void *context) * else from proceeding if the module fails to load * or hangs loading. */ - if (load_module) { - err = request_module("%s", op->name); -#ifdef CONFIG_IWLWIFI_OPMODE_MODULAR - if (err) - IWL_ERR(drv, - "failed to load module %s (error %d), is dynamic loading enabled?\n", - op->name, err); -#endif - } + if (load_module) + iwlwifi_try_load_op(op, drv); goto free; try_again: -- 2.11.0