[RFC 0/5] iwlwifi: enhance final opmode work

[RFC 0/5] iwlwifi: enhance final opmode work

[Luis R. Rodriguez]

Although these are iwlwifi patches, there are some core module, async,
firmware questions I'd appreciate a bit more review from folks on -- tx!

Firmware folks / async folks / module folks:

I started to look to generalize the way the iwlwifi driver uses the
firmware API to request for firmware through a series of API versions,
to chain firmware requests. I had to start examining its firmware
callback carefully as it used request_module() and because the driver
data API [0] (an evolution of the firmware API) I've been developing
uses async_schedule() instead of workqueues. Although I had started
using workqueues to match the old firmware API I went with
async_schedule() since I noticed most drivers using an async callback
also had to add a completion struct on their drivers and later waited
for completion at some other point. Using async framework we can just
give users a cookie, and they can use that to sync for completion.

One of the limitations of using async_schedule() though is we cannot
request_module() synchronously on async calls given that the module
initialization code will call async_synchronize_full() if the module
being initialized happened to have used async work on its initialization
routine, otherwise we'd deadlock.

So, I either I change back to workqueus or we live happy with either:

   a) Use request_module_nowait() on async calls -- this works
   b) If you need request_module() on your async call, consider a
      workqueue

Module folks:

One of the issues with a) is that contrary to try_then_request_module()
which helps validate module loading we have had no common generic
lightweight form to validate a module loaded correctly if you use
request_module_nowait(). You can surely use a module notifier with
register_module_notifier() -- but again, that is not lightweight, quick
and easy. From the look of it though, most current users simply call
request_module_nowait() and hope for the best -- there are no verifiers.
That's probably fine (?) -- other than failures can be silent. At least
iwlwifi would want to make a beep about failures it seems, so I went
with b).

In this series I went with b) but still note that iwlwifi cannot use
bare try_then_request_module() cleanly -- so it has its own wrapper (see
patch 02).

iwlfifi folks:

This patch series does not depend on the driver data API, but its is an
example of b). It also contains a series of other fixes and enhancements
for iwlwifi's opmode final work worth revising -- regardless of what
happens with the driver data API. With these patches average boot time
seems to improve slightly, but I'd expect much more testing would be
required before making any clear determination. I used 5 boots per patch.
I started with next-20170213. Following data is average boot times as
per systemd-analyze. The vanilla kernel (patch-00) got more testing as
I'm also using this a base for other testing -- 100 boots.

patch-00 Startup finished in  2.64057s (kernel) +  5.52668s (initrd) +  11.3024s (userspace) =  19.4707s
patch-01 Startup finished in    2.574s (kernel) +   5.1505s (initrd) +  11.0698s (userspace) =  18.7952s
patch-02 Startup finished in    2.618s (kernel) +   5.3102s (initrd) +  11.1736s (userspace) =  19.1026s
patch-03 Startup finished in   2.5612s (kernel) +   5.3274s (initrd) +  11.0226s (userspace) =  18.9122s
patch-04 Startup finished in   2.5475s (kernel) +   5.0895s (initrd) +  10.9272s (userspace) =  18.5652s

[0] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170206-driver-data-try2

Luis R. Rodriguez (5):
  iwlwifi: fix drv cleanup on opmode registration failure
  iwlwifi: fix request_module() use
  iwlwifi: share opmode start work code
  iwlwifi: move opmode loading to shared routine
  iwlwifi: convert final opmode work into a workqueue

 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 140 +++++++++++++++++++--------
 1 file changed, 100 insertions(+), 40 deletions(-)

-- 
2.11.0

[RFC 1/5] iwlwifi: fix drv cleanup on opmode registration failure

[Luis R. Rodriguez]

The firmware async callback handles the device's opmode start
call, but optionally also allows opmode registration to take
care of its opmode start. If the firmware callback handles it
its error path in case of opmode start failure has a few pieces
of code missing from the opmode registration. The opmode
registration hanlder has no cleanup at all. Sync both error
paths.

This should in theory fix a detangled drv from the drv list should
either of the opmode modules loaded and handled registration for the
drv.

The path of having the opmode registration deal with the drv
opmode start is actually the more common path. The other path,
from the async callback is rathe rare (1/8 or so times for me) --
it happens when the the opmode driver's init routine completed
prior to the driver's async callback opmode start call.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index be466a074c1d..e198d6f5fcea 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -1611,8 +1611,13 @@ int iwl_opmode_register(const char *name, const struct iwl_op_mode_ops *ops)
 			continue;
 		op->ops = ops;
 		/* TODO: need to handle exceptional case */
-		list_for_each_entry(drv, &op->drv, list)
+		list_for_each_entry(drv, &op->drv, list) {
 			drv->op_mode = _iwl_op_mode_start(drv, op);
+			if (!drv->op_mode) {
+				complete(&drv->request_firmware_complete);
+				device_release_driver(drv->trans->dev);
+			}
+		}
 
 		mutex_unlock(&iwlwifi_opmode_table_mtx);
 		return 0;
-- 
2.11.0

RE: [RFC 1/5] iwlwifi: fix drv cleanup on opmode registration failure

[Grumbach, Emmanuel]

Hi Luis,

> 
> The firmware async callback handles the device's opmode start call, but
> optionally also allows opmode registration to take care of its opmode start. If
> the firmware callback handles it its error path in case of opmode start failure
> has a few pieces of code missing from the opmode registration. The opmode
> registration hanlder has no cleanup at all. Sync both error paths.
> 
> This should in theory fix a detangled drv from the drv list should either of the
> opmode modules loaded and handled registration for the drv.
> 
> The path of having the opmode registration deal with the drv opmode start is
> actually the more common path. The other path, from the async callback is
> rathe rare (1/8 or so times for me) -- it happens when the the opmode
> driver's init routine completed prior to the driver's async callback opmode
> start call.

I'd claim it should never happen unless you have several devices on the system using the same
opmode, or unless you do:
modprobe iwlwifi  #which will load iwl{d,m}vm
rmmod iwl{d,m}vm #and do _not_ remove iwlwifi
modprobe iwlwifi



> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---

Luca is OOO,  but this looks fine to me.


>  drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> index be466a074c1d..e198d6f5fcea 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> @@ -1611,8 +1611,13 @@ int iwl_opmode_register(const char *name, const
> struct iwl_op_mode_ops *ops)
>  			continue;
>  		op->ops = ops;
>  		/* TODO: need to handle exceptional case */
> -		list_for_each_entry(drv, &op->drv, list)
> +		list_for_each_entry(drv, &op->drv, list) {
>  			drv->op_mode = _iwl_op_mode_start(drv, op);
> +			if (!drv->op_mode) {
> +				complete(&drv-
> >request_firmware_complete);
> +				device_release_driver(drv->trans->dev);
> +			}
> +		}
> 
>  		mutex_unlock(&iwlwifi_opmode_table_mtx);
>  		return 0;
> --
> 2.11.0

Re: [RFC 1/5] iwlwifi: fix drv cleanup on opmode registration failure

[Luis R. Rodriguez]

On Sun, Feb 19, 2017 at 09:16:01AM +0000, Grumbach, Emmanuel wrote:
> > This should in theory fix a detangled drv from the drv list should either of the
> > opmode modules loaded and handled registration for the drv.
> > 
> > The path of having the opmode registration deal with the drv opmode start is
> > actually the more common path. The other path, from the async callback is
> > rathe rare (1/8 or so times for me) -- it happens when the the opmode
> > driver's init routine completed prior to the driver's async callback opmode
> > start call.
> 
> I'd claim it should never happen unless you have several devices on the system using the same
> opmode, or unless you do:
> modprobe iwlwifi  #which will load iwl{d,m}vm
> rmmod iwl{d,m}vm #and do _not_ remove iwlwifi
> modprobe iwlwifi

That is indeed one way one can easily reproduce this. There are however other
ways too. Try a loop of

modprobe -r iwlmvm (which removes iwlwifi) followed by modprobe iwlmvm;

while this check for which path is taken, or better yet check if the
list of drvs is empty on opmode registration. Every now and then I see
the list is empty.

I have a feeling this is then also a rare rarely observed by your QA team
as well, so this code then is also stitching together a set of sequence
calls for both paths.

> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> 
> Luca is OOO,  but this looks fine to me.

Reviewed-by ?

  Luis

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

[Luis R. Rodriguez]

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

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

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

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

[Luis R. Rodriguez]

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

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(&iwlwifi_opmode_table_mtx);
+	for (i = 0; i < ARRAY_SIZE(iwlwifi_opmode_table); i++) {
+		op = &iwlwifi_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(&drv->request_firmware_complete);
+			device_release_driver(drv->trans->dev);
+			mutex_unlock(&iwlwifi_opmode_table_mtx);
+			return;
+		}
+
+		op->load_requested = false;
+		op->drv_req = NULL;
+	}
+	mutex_unlock(&iwlwifi_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(&drv->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 trigger_tlv_sz[FW_DBG_TRIGGER_MAX];
 	u32 api_ver;
 	int i;
-	bool load_module = false;
 	bool usniffer_images = false;
 
 	fw->ucode_capa.max_probe_length = IWL_DEFAULT_MAX_PROBE_LENGTH;
@@ -1455,31 +1496,26 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 			goto out_unbind;
 		}
 	} else {
-		load_module = true;
+		op->load_requested = true;
+		op->drv_req = drv;
 	}
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
 	/*
-	 * Complete the firmware request last so that
-	 * a driver unbind (stop) doesn't run while we
-	 * are doing the start() above.
-	 */
-	complete(&drv->request_firmware_complete);
-
-	/*
 	 * Load the module last so we don't block anything
 	 * else from proceeding if the module fails to load
 	 * or hangs loading.
+	 *
+	 * Always try loading it, even if we were built-in as
+	 * in built-in cases this will be a no-op and so will
+	 * the verifier check.
 	 */
-	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
-	}
+	err = request_module_nowait("%s", op->name);
+	if (err)
+		goto out_unbind;
+
+	schedule_delayed_work(&iwl_opload_work, HZ);
+
 	goto free;
 
  try_again:

So consider this for your driver -- if you agree today's print is rather
pointless upon failure then you'd be OK in just using request_module_nowait()
and removing the print -- and not adding a verifier step 2 like the above.

Only -- it seems you want a verifier.

So you have 2 options with a suboptions:

  1) keep sync request and add the verifier -- as in the original patch in this e-mail
  2) use async request and
	2a) add verifier
	2b) ignore the verifier

I don't see why you'd want 2b) is what I'm trying to say and the point of this
path is to show what a 1) would look like.

The point of this email also is to highlight what it would look like in general
if we wanted verifiers for module request for async_schedule() calls, given
they cannot use request_module() and *must* use request_module_nowait() and
that ultimately begs the question if they want verifiers or not as well.

For iwlwifi and wireless this is only generally relevant for the async
callback for firmware requests, but it seems only iwlwifi uses this form.
I used async_schedule() for the driver data API which I'm developing,
and as such 2a or a solution for 1) was needed in such a way it was
compatible.

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

You are right, but note the 2 possible ways in which the alternative path
can be taken in the prior code we discussed, this should ensure we complain
if upon a load the module is really not present. From what I recall from
my testing it turns out though that in practice this *still* is still allowing
for the case where iwlmvm loads prior to iwlwifi's async fw callback code
checks for the opmode. One can test this with a loop of:

modprobe -r iwlmvm; while true; do modprobe iwlmvm; modprobe -r iwlmvm; dmesg -c && echo ; done

Prior to this add a check for an empty list on the opmode registration, if its
empty then we've hit the path discussed. In the loop above we are *triggering*
the load of iwlmvm first, that's why it *can* load first. If we wanted to ensure
iwlmvm and other other opmode load second we can add a simply symbol dependency
from the opmodes to the iwlwifi driver.

  Luis

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

[Grumbach, Emmanuel]

> 
> On Sun, Feb 19, 2017 at 09:47:59AM +0000, 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 <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.
> 
> 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 Tue, Feb 21, 2017 at 07:16:16AM +0000, 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

[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 +0000, 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 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 +0000, 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

[PATCH v2 0/2] iwlwifi: corner case fix and request module changes

[Luis R. Rodriguez]

This v2 addresses the preference to keep things simple on iwlwifi when
requesting modules and not implementing a verifier for loaing the opmode
module. We now know what a verifier looks like for both sync and async
approaches. The already established long standing practice of just doing
best effort to load suffices and keeps the driver cleaner.

There no change to the first patch. The second patch just embraces
request_module_nowait() instead of implementing a verifier for a sync
call. The remaining patches from the last series will be sent separately.

Luis R. Rodriguez (2):
  iwlwifi: fix drv cleanup on opmode registration failure
  iwlwifi: simplify requesting ops module

 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

-- 
2.11.0

[PATCH v2 1/2] iwlwifi: fix drv cleanup on opmode registration failure

[Luis R. Rodriguez]

The firmware async callback handles the device's opmode start
call, but optionally also allows opmode registration to take
care of its opmode start. If the firmware callback handles it
its error path in case of opmode start failure has a few pieces
of code missing from the opmode registration. The opmode
registration hanlder has no cleanup at all. Sync both error
paths.

This should in theory fix a detangled drv from the drv list should
either of the opmode modules loaded and handled registration for the
drv.

The path of having the opmode registration deal with the drv
opmode start is actually the more common path. The other path,
from the async callback is rathe rare (1/8 or so times for me) --
it happens when the the opmode driver's init routine completed
prior to the driver's async callback opmode start call.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index be466a074c1d..e198d6f5fcea 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -1611,8 +1611,13 @@ int iwl_opmode_register(const char *name, const struct iwl_op_mode_ops *ops)
 			continue;
 		op->ops = ops;
 		/* TODO: need to handle exceptional case */
-		list_for_each_entry(drv, &op->drv, list)
+		list_for_each_entry(drv, &op->drv, list) {
 			drv->op_mode = _iwl_op_mode_start(drv, op);
+			if (!drv->op_mode) {
+				complete(&drv->request_firmware_complete);
+				device_release_driver(drv->trans->dev);
+			}
+		}
 
 		mutex_unlock(&iwlwifi_opmode_table_mtx);
 		return 0;
-- 
2.11.0

[PATCH v2 2/2] iwlwifi: simplify requesting ops module

[Luis R. Rodriguez]

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 for synchronous requests.

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 could use its own wrapper for
requesting its ops module synchronously so it can handle its special
locking requirements on its own but the given code has been in the
kernel for a long time and the preference is to keep its simplicity.

We can do two more things to simplify this even further:

a) just use the async request, request_module_nowait(), to acknowledge
   we are using best effort -- we are not verifying the module is loaded

b) we can unconditionally call request_module_nowait() whether or not
   we were built-in or not, if built-in this will return immediately
   without failure.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index e198d6f5fcea..e96095c1824a 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -1250,7 +1250,6 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 	size_t trigger_tlv_sz[FW_DBG_TRIGGER_MAX];
 	u32 api_ver;
 	int i;
-	bool load_module = false;
 	bool usniffer_images = false;
 
 	fw->ucode_capa.max_probe_length = IWL_DEFAULT_MAX_PROBE_LENGTH;
@@ -1454,8 +1453,6 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 			mutex_unlock(&iwlwifi_opmode_table_mtx);
 			goto out_unbind;
 		}
-	} else {
-		load_module = true;
 	}
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
@@ -1466,20 +1463,10 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 	 */
 	complete(&drv->request_firmware_complete);
 
-	/*
-	 * Load the module last so we don't block anything
-	 * 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
-	}
+	err = request_module_nowait("%s", op->name);
+	if (err)
+		goto out_unbind;
+
 	goto free;
 
  try_again:
-- 
2.11.0

[PATCH v2 0/2] iwlwifi: share opmode start code

[Luis R. Rodriguez]

This v2 split off the opmode handling sharing code into its
own series, it however depends on the request_module_nowait()
change.

The sharing of the opmode handling makes it easier to share fixes
when dealing with opmode handling on devices. It should also hopefully
make the code easier to grok. Lastly, since we are moving things to
a workqueue naturally the module_init() for iwlmvm is not offloaded,
and so this should reduce the boot time by a bit.

As per the average of systemd-analyze on 5 boots using next-20170221
as base:

next-20170221:
Startup finished in   2.6142s (kernel) +   5.1916s (initrd) +  10.8968s (userspace) =  18.7036s

After these patches:
Startup finished in   2.5468s (kernel) +   4.9536s (initrd) +   10.798s (userspace) =  18.2994s

Luis R. Rodriguez (2):
  iwlwifi: share opmode start work code
  iwlwifi: convert final opmode work into a workqueue

 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 93 +++++++++++++++++++---------
 1 file changed, 64 insertions(+), 29 deletions(-)

-- 
2.11.0

[PATCH v2 1/2] iwlwifi: share opmode start work code

[Luis R. Rodriguez]

The firmware async callback and the opmode registration share
some functionality -- to start the drv's opmode. Move this work
into a helper which is shared. This should help us share fixes
should these diverging code paths change.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 90 +++++++++++++++++++---------
 1 file changed, 61 insertions(+), 29 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index e96095c1824a..ea88b5cec869 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -102,6 +102,7 @@ static struct dentry *iwl_dbgfs_root;
  * @op_mode: the running op_mode
  * @trans: transport layer
  * @dev: for debug prints only
+ * @start_requested: start op has been requested and is pending on this device
  * @fw_index: firmware revision to try loading
  * @firmware_name: composite filename of ucode file to load
  * @request_firmware_complete: the firmware has been obtained from user space
@@ -113,6 +114,7 @@ struct iwl_drv {
 	struct iwl_op_mode *op_mode;
 	struct iwl_trans *trans;
 	struct device *dev;
+	bool start_requested;
 
 	int fw_index;                   /* firmware we're trying to load */
 	char firmware_name[64];         /* name of firmware file to load */
@@ -1231,6 +1233,48 @@ static void _iwl_op_mode_stop(struct iwl_drv *drv)
 	}
 }
 
+static void iwlwifi_opmode_start_drv(struct iwlwifi_opmode_table *op,
+				     struct iwl_drv *drv)
+{
+	if (!drv->start_requested)
+		return;
+
+	drv->op_mode = _iwl_op_mode_start(drv, op);
+	drv->start_requested = false;
+
+	/*
+	 * Complete the firmware request last so that
+	 * a driver unbind (stop) doesn't run while we
+	 * are doing the start() above.
+	 */
+	complete(&drv->request_firmware_complete);
+
+	if (!drv->op_mode)
+		device_release_driver(drv->trans->dev);
+}
+
+static void iwlwifi_opmode_start(struct iwlwifi_opmode_table *op)
+{
+	struct iwl_drv *drv;
+
+	list_for_each_entry(drv, &op->drv, list)
+		iwlwifi_opmode_start_drv(op, drv);
+}
+
+static void iwlwifi_opmode_dowork(void)
+{
+	unsigned int i;
+	struct iwlwifi_opmode_table *op;
+
+	mutex_lock(&iwlwifi_opmode_table_mtx);
+	for (i = 0; i < ARRAY_SIZE(iwlwifi_opmode_table); i++) {
+		op = &iwlwifi_opmode_table[i];
+		if (op->ops)
+			iwlwifi_opmode_start(op);
+	}
+	mutex_unlock(&iwlwifi_opmode_table_mtx);
+}
+
 /**
  * iwl_req_fw_callback - callback when firmware was loaded
  *
@@ -1443,30 +1487,17 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 	IWL_INFO(drv, "loaded firmware version %s op_mode %s\n",
 		 drv->fw.fw_version, op->name);
 
+	drv->start_requested = true;
 	/* add this device to the list of devices using this op_mode */
 	list_add_tail(&drv->list, &op->drv);
 
-	if (op->ops) {
-		drv->op_mode = _iwl_op_mode_start(drv, op);
-
-		if (!drv->op_mode) {
-			mutex_unlock(&iwlwifi_opmode_table_mtx);
-			goto out_unbind;
-		}
-	}
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
-	/*
-	 * Complete the firmware request last so that
-	 * a driver unbind (stop) doesn't run while we
-	 * are doing the start() above.
-	 */
-	complete(&drv->request_firmware_complete);
-
 	err = request_module_nowait("%s", op->name);
 	if (err)
 		goto out_unbind;
 
+	iwlwifi_opmode_dowork();
 	goto free;
 
  try_again:
@@ -1588,8 +1619,8 @@ IWL_EXPORT_SYMBOL(iwlwifi_mod_params);
 int iwl_opmode_register(const char *name, const struct iwl_op_mode_ops *ops)
 {
 	int i;
-	struct iwl_drv *drv;
 	struct iwlwifi_opmode_table *op;
+	int ret = -EIO;
 
 	mutex_lock(&iwlwifi_opmode_table_mtx);
 	for (i = 0; i < ARRAY_SIZE(iwlwifi_opmode_table); i++) {
@@ -1597,20 +1628,15 @@ int iwl_opmode_register(const char *name, const struct iwl_op_mode_ops *ops)
 		if (strcmp(op->name, name))
 			continue;
 		op->ops = ops;
-		/* TODO: need to handle exceptional case */
-		list_for_each_entry(drv, &op->drv, list) {
-			drv->op_mode = _iwl_op_mode_start(drv, op);
-			if (!drv->op_mode) {
-				complete(&drv->request_firmware_complete);
-				device_release_driver(drv->trans->dev);
-			}
-		}
-
-		mutex_unlock(&iwlwifi_opmode_table_mtx);
-		return 0;
+		ret = 0;
+		break;
 	}
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
-	return -EIO;
+
+	if (!ret)
+		iwlwifi_opmode_dowork();
+
+	return ret;
 }
 IWL_EXPORT_SYMBOL(iwl_opmode_register);
 
@@ -1626,8 +1652,14 @@ void iwl_opmode_deregister(const char *name)
 		iwlwifi_opmode_table[i].ops = NULL;
 
 		/* call the stop routine for all devices */
-		list_for_each_entry(drv, &iwlwifi_opmode_table[i].drv, list)
+		list_for_each_entry(drv, &iwlwifi_opmode_table[i].drv, list) {
 			_iwl_op_mode_stop(drv);
+			/*
+			 * So that if iwlmvm gets unloaded alone, but then
+			 * loaded again we can kick the old registered devices
+			 */
+			drv->start_requested = true;
+		}
 
 		mutex_unlock(&iwlwifi_opmode_table_mtx);
 		return;
-- 
2.11.0

[PATCH v2 2/2] iwlwifi: convert final opmode work into a workqueue

[Luis R. Rodriguez]

This lets us offload and share all the final opmode related work
necessary for either an opmode driver or new device. This has the most
impact for opmode drivers as this now offloads opmode start for each
device onto the workqueue.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index ea88b5cec869..1db372e9c039 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -143,6 +143,8 @@ static struct iwlwifi_opmode_table {
 	[DVM_OP_MODE] = { .name = "iwldvm", .ops = NULL },
 	[MVM_OP_MODE] = { .name = "iwlmvm", .ops = NULL },
 };
+static void iwlwifi_opmode_dowork(struct work_struct *work);
+static DECLARE_WORK(iwlwifi_opmode_work, iwlwifi_opmode_dowork);
 
 #define IWL_DEFAULT_SCAN_CHANNELS 40
 
@@ -1261,7 +1263,7 @@ static void iwlwifi_opmode_start(struct iwlwifi_opmode_table *op)
 		iwlwifi_opmode_start_drv(op, drv);
 }
 
-static void iwlwifi_opmode_dowork(void)
+static void iwlwifi_opmode_dowork(struct work_struct *unused_work)
 {
 	unsigned int i;
 	struct iwlwifi_opmode_table *op;
@@ -1497,7 +1499,7 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 	if (err)
 		goto out_unbind;
 
-	iwlwifi_opmode_dowork();
+	schedule_work(&iwlwifi_opmode_work);
 	goto free;
 
  try_again:
@@ -1581,6 +1583,7 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans *trans)
 void iwl_drv_stop(struct iwl_drv *drv)
 {
 	wait_for_completion(&drv->request_firmware_complete);
+	cancel_work_sync(&iwlwifi_opmode_work);
 
 	_iwl_op_mode_stop(drv);
 
@@ -1634,7 +1637,7 @@ int iwl_opmode_register(const char *name, const struct iwl_op_mode_ops *ops)
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
 	if (!ret)
-		iwlwifi_opmode_dowork();
+		schedule_work(&iwlwifi_opmode_work);
 
 	return ret;
 }
-- 
2.11.0

[RFC 3/5] iwlwifi: share opmode start work code

[Luis R. Rodriguez]

The firmware async callback and the opmode registration share
some functionality -- to start the drv's opmode. Move this work
into a helper which is shared. This should help us share fixes
should these diverging code paths change.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 93 +++++++++++++++++++---------
 1 file changed, 63 insertions(+), 30 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index 6beb92d19ea7..4a1937c77f90 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -102,6 +102,7 @@ static struct dentry *iwl_dbgfs_root;
  * @op_mode: the running op_mode
  * @trans: transport layer
  * @dev: for debug prints only
+ * @start_requested: start op has been requested and is pending on this device
  * @fw_index: firmware revision to try loading
  * @firmware_name: composite filename of ucode file to load
  * @request_firmware_complete: the firmware has been obtained from user space
@@ -113,6 +114,7 @@ struct iwl_drv {
 	struct iwl_op_mode *op_mode;
 	struct iwl_trans *trans;
 	struct device *dev;
+	bool start_requested;
 
 	int fw_index;                   /* firmware we're trying to load */
 	char firmware_name[64];         /* name of firmware file to load */
@@ -1254,6 +1256,48 @@ static void iwlwifi_try_load_op(struct iwlwifi_opmode_table *op,
 #endif
 }
 
+static void iwlwifi_opmode_start_drv(struct iwlwifi_opmode_table *op,
+				     struct iwl_drv *drv)
+{
+	if (!drv->start_requested)
+		return;
+
+	drv->op_mode = _iwl_op_mode_start(drv, op);
+	drv->start_requested = false;
+
+	/*
+	 * Complete the firmware request last so that
+	 * a driver unbind (stop) doesn't run while we
+	 * are doing the start() above.
+	 */
+	complete(&drv->request_firmware_complete);
+
+	if (!drv->op_mode)
+		device_release_driver(drv->trans->dev);
+}
+
+static void iwlwifi_opmode_start(struct iwlwifi_opmode_table *op)
+{
+	struct iwl_drv *drv;
+
+	list_for_each_entry(drv, &op->drv, list)
+		iwlwifi_opmode_start_drv(op, drv);
+}
+
+static void iwlwifi_opmode_dowork(void)
+{
+	unsigned int i;
+	struct iwlwifi_opmode_table *op;
+
+	mutex_lock(&iwlwifi_opmode_table_mtx);
+	for (i = 0; i < ARRAY_SIZE(iwlwifi_opmode_table); i++) {
+		op = &iwlwifi_opmode_table[i];
+		if (op->ops)
+			iwlwifi_opmode_start(op);
+	}
+	mutex_unlock(&iwlwifi_opmode_table_mtx);
+}
+
 /**
  * iwl_req_fw_callback - callback when firmware was loaded
  *
@@ -1467,27 +1511,14 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 	IWL_INFO(drv, "loaded firmware version %s op_mode %s\n",
 		 drv->fw.fw_version, op->name);
 
+	drv->start_requested = true;
 	/* add this device to the list of devices using this op_mode */
 	list_add_tail(&drv->list, &op->drv);
 
-	if (op->ops) {
-		drv->op_mode = _iwl_op_mode_start(drv, op);
-
-		if (!drv->op_mode) {
-			mutex_unlock(&iwlwifi_opmode_table_mtx);
-			goto out_unbind;
-		}
-	} else {
+	if (!op->ops)
 		load_module = true;
-	}
-	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
-	/*
-	 * Complete the firmware request last so that
-	 * a driver unbind (stop) doesn't run while we
-	 * are doing the start() above.
-	 */
-	complete(&drv->request_firmware_complete);
+	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
 	/*
 	 * Load the module last so we don't block anything
@@ -1496,6 +1527,7 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 	 */
 	if (load_module)
 		iwlwifi_try_load_op(op, drv);
+	iwlwifi_opmode_dowork();
 	goto free;
 
  try_again:
@@ -1617,8 +1649,8 @@ IWL_EXPORT_SYMBOL(iwlwifi_mod_params);
 int iwl_opmode_register(const char *name, const struct iwl_op_mode_ops *ops)
 {
 	int i;
-	struct iwl_drv *drv;
 	struct iwlwifi_opmode_table *op;
+	int ret = -EIO;
 
 	mutex_lock(&iwlwifi_opmode_table_mtx);
 	for (i = 0; i < ARRAY_SIZE(iwlwifi_opmode_table); i++) {
@@ -1626,20 +1658,15 @@ int iwl_opmode_register(const char *name, const struct iwl_op_mode_ops *ops)
 		if (strcmp(op->name, name))
 			continue;
 		op->ops = ops;
-		/* TODO: need to handle exceptional case */
-		list_for_each_entry(drv, &op->drv, list) {
-			drv->op_mode = _iwl_op_mode_start(drv, op);
-			if (!drv->op_mode) {
-				complete(&drv->request_firmware_complete);
-				device_release_driver(drv->trans->dev);
-			}
-		}
-
-		mutex_unlock(&iwlwifi_opmode_table_mtx);
-		return 0;
+		ret = 0;
+		break;
 	}
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
-	return -EIO;
+
+	if (!ret)
+		iwlwifi_opmode_dowork();
+
+	return ret;
 }
 IWL_EXPORT_SYMBOL(iwl_opmode_register);
 
@@ -1655,8 +1682,14 @@ void iwl_opmode_deregister(const char *name)
 		iwlwifi_opmode_table[i].ops = NULL;
 
 		/* call the stop routine for all devices */
-		list_for_each_entry(drv, &iwlwifi_opmode_table[i].drv, list)
+		list_for_each_entry(drv, &iwlwifi_opmode_table[i].drv, list) {
 			_iwl_op_mode_stop(drv);
+			/*
+			 * So that if iwlmvm gets unloaded alone, but then
+			 * loaded again we can kick the old registered devices
+			 */
+			drv->start_requested = true;
+		}
 
 		mutex_unlock(&iwlwifi_opmode_table_mtx);
 		return;
-- 
2.11.0

[RFC 4/5] iwlwifi: move opmode loading to shared routine

[Luis R. Rodriguez]

This helps us compartmentalize all last required opmode work
and declutter the async firmware callback.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 33 +++++++++++++++-------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index 4a1937c77f90..6cfbc3c6e0d6 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -139,6 +139,8 @@ 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 },
@@ -1233,27 +1235,32 @@ 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)
+/* We have to unlock and then lock for the caller */
+static void iwlwifi_try_load_op(struct iwlwifi_opmode_table *op)
 {
 	int ret = 0;
 
+	mutex_unlock(&iwlwifi_opmode_table_mtx);
+
 	ret = request_module("%s", op->name);
-	if (ret)
+	if (ret) {
+		mutex_lock(&iwlwifi_opmode_table_mtx);
 		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,
+		IWL_ERR(op->drv_req,
 			"failed to load module %s (error %d), is dynamic loading enabled?\n",
 			op->name, ret);
 #endif
+	op->load_requested = false;
+	op->drv_req = NULL;
 }
 
 static void iwlwifi_opmode_start_drv(struct iwlwifi_opmode_table *op,
@@ -1294,6 +1301,8 @@ static void iwlwifi_opmode_dowork(void)
 		op = &iwlwifi_opmode_table[i];
 		if (op->ops)
 			iwlwifi_opmode_start(op);
+		else if (op->load_requested)
+			iwlwifi_try_load_op(op);
 	}
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
 }
@@ -1317,7 +1326,6 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 	size_t trigger_tlv_sz[FW_DBG_TRIGGER_MAX];
 	u32 api_ver;
 	int i;
-	bool load_module = false;
 	bool usniffer_images = false;
 
 	fw->ucode_capa.max_probe_length = IWL_DEFAULT_MAX_PROBE_LENGTH;
@@ -1515,18 +1523,13 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 	/* add this device to the list of devices using this op_mode */
 	list_add_tail(&drv->list, &op->drv);
 
-	if (!op->ops)
-		load_module = true;
+	if (!op->ops) {
+		op->load_requested = true;
+		op->drv_req = drv;
+	}
 
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
-	/*
-	 * Load the module last so we don't block anything
-	 * else from proceeding if the module fails to load
-	 * or hangs loading.
-	 */
-	if (load_module)
-		iwlwifi_try_load_op(op, drv);
 	iwlwifi_opmode_dowork();
 	goto free;
 
-- 
2.11.0

[RFC 5/5] iwlwifi: convert final opmode work into a workqueue

[Luis R. Rodriguez]

This lets us offload and share all the final opmode related work
necessary for either an opmode driver or new device. This has the most
impact for opmode drivers as this now offloads opmode start for each
device onto the workqueue.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index 6cfbc3c6e0d6..d39a5c73afdc 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -145,6 +145,8 @@ static struct iwlwifi_opmode_table {
 	[DVM_OP_MODE] = { .name = "iwldvm", .ops = NULL },
 	[MVM_OP_MODE] = { .name = "iwlmvm", .ops = NULL },
 };
+static void iwlwifi_opmode_dowork(struct work_struct *work);
+static DECLARE_WORK(iwlwifi_opmode_work, iwlwifi_opmode_dowork);
 
 #define IWL_DEFAULT_SCAN_CHANNELS 40
 
@@ -1291,7 +1293,7 @@ static void iwlwifi_opmode_start(struct iwlwifi_opmode_table *op)
 		iwlwifi_opmode_start_drv(op, drv);
 }
 
-static void iwlwifi_opmode_dowork(void)
+static void iwlwifi_opmode_dowork(struct work_struct *unused_work)
 {
 	unsigned int i;
 	struct iwlwifi_opmode_table *op;
@@ -1530,7 +1532,7 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
-	iwlwifi_opmode_dowork();
+	schedule_work(&iwlwifi_opmode_work);
 	goto free;
 
  try_again:
@@ -1614,6 +1616,7 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans *trans)
 void iwl_drv_stop(struct iwl_drv *drv)
 {
 	wait_for_completion(&drv->request_firmware_complete);
+	cancel_work_sync(&iwlwifi_opmode_work);
 
 	_iwl_op_mode_stop(drv);
 
@@ -1667,7 +1670,7 @@ int iwl_opmode_register(const char *name, const struct iwl_op_mode_ops *ops)
 	mutex_unlock(&iwlwifi_opmode_table_mtx);
 
 	if (!ret)
-		iwlwifi_opmode_dowork();
+		schedule_work(&iwlwifi_opmode_work);
 
 	return ret;
 }
-- 
2.11.0

Re: [RFC 0/5] iwlwifi: enhance final opmode work

[Johannes Berg]

> One of the limitations of using async_schedule() though is we cannot
> request_module() synchronously on async calls given that the module
> initialization code will call async_synchronize_full() if the module
> being initialized happened to have used async work on its
> initialization routine, otherwise we'd deadlock.
> 
> So, I either I change back to workqueus or we live happy with either:

I really think you should avoid breaking this API and change back.
Drivers have been using it, how to avoid the use-after-free with a
completion is well known, and there's generally no issue.

Making things "easier" while requiring lots of churn everywhere isn't
always a good thing.

If you end up introducing some new API then perhaps that new API would
make sense to use async_schedule(), but I don't really see all that
much point in changing all of this now.

johannes