Re: [RFC 2/5] iwlwifi: fix request_module() use

[Luis R. Rodriguez]

On Tue, Feb 21, 2017 at 09:17:15PM +0100, Luis R. Rodriguez wrote:
> On Tue, Feb 21, 2017 at 07:15:41PM +0100, Luis R. Rodriguez wrote:
> > On Tue, Feb 21, 2017 at 07:16:16AM +, Grumbach, Emmanuel wrote:
> > > > 
> > > > a) just remove the print and use instead request_module_nowait() (this 
> > > > is
> > > > more in alignment of what your code actually does today; or
> > > > 
> > > > b) fix the request_module() use so that the error print matches the
> > > > expected and proper recommended use of request_module() (what this patch
> > > > does)
> > > > 
> > > > I prefer a) actually but I had to show what b) looked like first :)
> > >
> > > Me too. Let's do the simple thing. After all, it's been working for 5 
> > > years
> > > now (maybe more?) and I don't see a huge need to verify that the opmode
> > > module has been loaded.  It is very unlikely to fail anyway, and in the 
> > > case
> > > it did fail, it's not that we can do much from iwlwifi point of view. 
> > 
> > I tend to agree with you on this, retries would be the only sensible thing 
> > to
> > do, but why do that -- the error should be logged right and addressed by any
> > upper layers. Its one reason to consider in the future adding verifiers
> > as built-in optional part of module loading.
> 
> It would seem we still need to offload the opmode start as it is the one that
> really should be issuing the completion, otherwise we would end up sending a
> completion while the opmode module is being loaded asynchronously. The changes
> are for that are still very likely desirable as it should help with speeding
> boot up.
> 
> So the sharing of the opcode start will go first.
> 
> Will send v2.

Actually the completion was always being sent prior to request_module(), so this
would not change anything really. The sharing of the opcode then is optional,
and I can send separately in another series.

  Luis

Re: [RFC 2/5] iwlwifi: fix request_module() use

[Luis R. Rodriguez]

On Tue, Feb 21, 2017 at 07:15:41PM +0100, Luis R. Rodriguez wrote:
> On Tue, Feb 21, 2017 at 07:16:16AM +, Grumbach, Emmanuel wrote:
> > > 
> > > a) just remove the print and use instead request_module_nowait() (this is
> > > more in alignment of what your code actually does today; or
> > > 
> > > b) fix the request_module() use so that the error print matches the
> > > expected and proper recommended use of request_module() (what this patch
> > > does)
> > > 
> > > I prefer a) actually but I had to show what b) looked like first :)
> >
> > Me too. Let's do the simple thing. After all, it's been working for 5 years
> > now (maybe more?) and I don't see a huge need to verify that the opmode
> > module has been loaded.  It is very unlikely to fail anyway, and in the case
> > it did fail, it's not that we can do much from iwlwifi point of view. 
> 
> I tend to agree with you on this, retries would be the only sensible thing to
> do, but why do that -- the error should be logged right and addressed by any
> upper layers. Its one reason to consider in the future adding verifiers
> as built-in optional part of module loading.

It would seem we still need to offload the opmode start as it is the one that
really should be issuing the completion, otherwise we would end up sending a
completion while the opmode module is being loaded asynchronously. The changes
are for that are still very likely desirable as it should help with speeding
boot up.

So the sharing of the opcode start will go first.

Will send v2.

  Luis

Re: [RFC 2/5] iwlwifi: fix request_module() use

[Luis R. Rodriguez]

On Tue, Feb 21, 2017 at 07:16:16AM +, Grumbach, Emmanuel wrote:
> > 
> > a) just remove the print and use instead request_module_nowait() (this is
> > more in alignment of what your code actually does today; or
> > 
> > b) fix the request_module() use so that the error print matches the
> > expected and proper recommended use of request_module() (what this patch
> > does)
> > 
> > I prefer a) actually but I had to show what b) looked like first :)
>
> Me too. Let's do the simple thing. After all, it's been working for 5 years
> now (maybe more?) and I don't see a huge need to verify that the opmode
> module has been loaded.  It is very unlikely to fail anyway, and in the case
> it did fail, it's not that we can do much from iwlwifi point of view. 

I tend to agree with you on this, retries would be the only sensible thing to
do, but why do that -- the error should be logged right and addressed by any
upper layers. Its one reason to consider in the future adding verifiers
as built-in optional part of module loading.

> iwlwifi will stay loaded and sit idle since no opmode will be there to start
> using the hardware. We will keep having the device claimed, and will keep the
> interrupt registered and all that. No WiFi for you, but no harm caused
> either.

Fine by me. Will send follow up simple patches.

  Luis

RE: [RFC 2/5] iwlwifi: fix request_module() use

[Grumbach, Emmanuel]

> 
> On Sun, Feb 19, 2017 at 09:47:59AM +, Grumbach, Emmanuel wrote:
> > >
> > > 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 
> >
> > 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.
> 
> Right I get that.
> 
> The code today complains if its respective opmode module was not loaded if
> request_module() did not return 0. As the commit log explains, relying on a
> return code of 0 to ensure a module loads is not sufficient. So the current
> print is almost pointless, so best we either:
> 
> a) just remove the print and use instead request_module_nowait() (this is
> more
>in alignment of what your code actually does today; or
> 
> b) fix the request_module() use so that the error print matches the
> expected
>and proper recommended use of request_module() (what this patch does)
> 
> I prefer a) actually but I had to show what b) looked like first :)
> 

Me too. Let's do the simple thing. After all, it's been working for 5 years now 
(maybe more?)
and I don't see a huge need to verify that the opmode module has been loaded.
It is very unlikely to fail anyway, and in the case it did fail, it's not that 
we can do much
from iwlwifi point of view. iwlwifi will stay loaded and sit idle since no 
opmode will
be there to start using the hardware. We will keep having the device claimed, 
and will
keep the interrupt registered and all that. No WiFi for you, but no harm caused 
either.

Re: [RFC 2/5] iwlwifi: fix request_module() use

[Luis R. Rodriguez]

On Sun, Feb 19, 2017 at 09:47:59AM +, Grumbach, Emmanuel wrote:
> > 
> > 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 
> 
> 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.

Right I get that.

The code today complains if its respective opmode module was not loaded
if request_module() did not return 0. As the commit log explains, relying
on a return code of 0 to ensure a module loads is not sufficient. So the
current print is almost pointless, so best we either:

a) just remove the print and use instead request_module_nowait() (this is more
   in alignment of what your code actually does today; or

b) fix the request_module() use so that the error print matches the expected
   and proper recommended use of request_module() (what this patch does)

I prefer a) actually but I had to show what b) looked like first :)

The only issue with a) is today we have no *slim* way to let drivers
load a module asynchronously and then later verify it did get loaded.
>From a *quick* look this grammatical form of request_module_nowait() and
a verifier is essentially is not widely popular or not present at all
today. A verifier seems reasonable if you use request_module_nowait()
though and want to be pedantic about ensuring the module is there.

What this might look like for iwlwifi? Something like this:

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c 
b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index be466a074c1d..8059e1dab061 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -137,11 +137,16 @@ static struct iwlwifi_opmode_table {
const char *name;   /* name: iwldvm, iwlmvm, etc */
const struct iwl_op_mode_ops *ops;  /* pointer to op_mode ops */
struct list_head drv;   /* list of devices using this op_mode */
+   bool load_requested;/* Do we need to load a driver ? */
+   struct iwl_drv *drv_req;/* Device that set load_requested */
 } iwlwifi_opmode_table[] = {   /* ops set when driver is initialized */
[DVM_OP_MODE] = { .name = "iwldvm", .ops = NULL },
[MVM_OP_MODE] = { .name = "iwlmvm", .ops = NULL },
 };
 
+static void iwlwifi_load_opmode(struct work_struct *work);
+static DECLARE_DELAYED_WORK(iwl_opload_work, iwlwifi_load_opmode);
+
 #define IWL_DEFAULT_SCAN_CHANNELS 40
 
 /*
@@ -1231,6 +1236,43 @@ static void _iwl_op_mode_stop(struct iwl_drv *drv)
}
 }
 
+static void iwlwifi_load_opmode(struct work_struct *work)
+{
+   struct iwl_drv *drv = NULL;
+   struct iwlwifi_opmode_table *op;
+   unsigned int i;
+
+   mutex_lock(_opmode_table_mtx);
+   for (i = 0; i < ARRAY_SIZE(iwlwifi_opmode_table); i++) {
+   op = _opmode_table[i];
+   if (!op->load_requested)
+   continue;
+   drv = op->drv_req;
+
+   if (!op->ops && drv) {
+   IWL_ERR(drv,
+   "failed to load module %s, is dynamic loading 
enabled?\n",
+   op->name);
+   complete(>request_firmware_complete);
+   device_release_driver(drv->trans->dev);
+   mutex_unlock(_opmode_table_mtx);
+   return;
+   }
+
+   op->load_requested = false;
+   op->drv_req = NULL;
+   }
+   mutex_unlock(_opmode_table_mtx);
+
+
+   /*
+* Complete the firmware request last so that
+* a driver unbind (stop) doesn't run while we
+* are doing the opmode start().
+*/
+   complete(>request_firmware_complete);
+}
+
 /**
  * iwl_req_fw_callback - callback when firmware was loaded
  *
@@ -1250,7 +1292,6 @@ static void iwl_req_fw_callback(const struct firmware 
*ucode_raw, void *context)
size_t 

RE: [RFC 2/5] iwlwifi: fix request_module() use

[Grumbach, Emmanuel]

> 
> 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 

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", _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(_opmode_table_mtx);
> + if (!op->ops)
> + ret = -ENOENT;
> + mutex_unlock(_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