| Message ID | 20170217020903.6370-2-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Luca Coelho |
| Headers | show |
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
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
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;
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(-)