| Message ID | 20170217020903.6370-3-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Luca Coelho |
| Headers | show |
> > 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? > --- > 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.c > b/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 struct > firmware *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
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/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 struct firmware *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:
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> --- drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 34 ++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 9 deletions(-)