From: Juuso Oikarinen <[email protected]>
PLT mode start/stop is controlled from userspace. When removing module, the
PLT mode state is however not checked, and not cleared. There is the possibility
of some unwanted state to left linger and there is even the possiblity of a
kernel crash if for instance IRQ work is running when the module is removed.
Fix this by stopping PLT mode on module removal, if still running.
Signed-off-by: Juuso Oikarinen <[email protected]>
---
drivers/net/wireless/wl12xx/main.c | 20 +++++++++++++++-----
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 9076555..863e660 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -917,12 +917,10 @@ out:
return ret;
}
-int wl1271_plt_stop(struct wl1271 *wl)
+int __wl1271_plt_stop(struct wl1271 *wl)
{
int ret = 0;
- mutex_lock(&wl->mutex);
-
wl1271_notice("power down");
if (wl->state != WL1271_STATE_PLT) {
@@ -938,12 +936,21 @@ int wl1271_plt_stop(struct wl1271 *wl)
wl->state = WL1271_STATE_OFF;
wl->rx_counter = 0;
-out:
mutex_unlock(&wl->mutex);
-
cancel_work_sync(&wl->irq_work);
cancel_work_sync(&wl->recovery_work);
+ mutex_lock(&wl->mutex);
+out:
+ return ret;
+}
+
+int wl1271_plt_stop(struct wl1271 *wl)
+{
+ int ret;
+ mutex_lock(&wl->mutex);
+ ret = __wl1271_plt_stop(wl);
+ mutex_unlock(&wl->mutex);
return ret;
}
@@ -3109,6 +3116,9 @@ EXPORT_SYMBOL_GPL(wl1271_register_hw);
void wl1271_unregister_hw(struct wl1271 *wl)
{
+ if (wl->state == WL1271_STATE_PLT)
+ __wl1271_plt_stop(wl);
+
unregister_netdevice_notifier(&wl1271_dev_notifier);
ieee80211_unregister_hw(wl->hw);
wl->mac80211_registered = false;
--
1.7.1
On Fri, 2011-01-14 at 12:48 +0100, [email protected] wrote:
> From: Juuso Oikarinen <[email protected]>
>
> PLT mode start/stop is controlled from userspace. When removing module, the
> PLT mode state is however not checked, and not cleared. There is the possibility
> of some unwanted state to left linger and there is even the possiblity of a
> kernel crash if for instance IRQ work is running when the module is removed.
>
> Fix this by stopping PLT mode on module removal, if still running.
>
> Signed-off-by: Juuso Oikarinen <[email protected]>
> ---
Applied. Thanks, Juuso!
--
Cheers,
Luca.
On Fri, 2011-01-14 at 15:16 +0200, ext Luciano Coelho wrote:
> Hi Juuso,
>
> On Fri, 2011-01-14 at 12:48 +0100, [email protected] wrote:
> > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> > index 9076555..863e660 100644
> > --- a/drivers/net/wireless/wl12xx/main.c
> > +++ b/drivers/net/wireless/wl12xx/main.c
> > @@ -917,12 +917,10 @@ out:
> > return ret;
> > }
> >
> > -int wl1271_plt_stop(struct wl1271 *wl)
> > +int __wl1271_plt_stop(struct wl1271 *wl)
> > {
> > int ret = 0;
> >
> > - mutex_lock(&wl->mutex);
> > -
> > wl1271_notice("power down");
> >
> > if (wl->state != WL1271_STATE_PLT) {
> > @@ -938,12 +936,21 @@ int wl1271_plt_stop(struct wl1271 *wl)
> > wl->state = WL1271_STATE_OFF;
> > wl->rx_counter = 0;
> >
> > -out:
> > mutex_unlock(&wl->mutex);
> > -
> > cancel_work_sync(&wl->irq_work);
> > cancel_work_sync(&wl->recovery_work);
> > + mutex_lock(&wl->mutex);
> > +out:
> > + return ret;
> > +}
>
> Again we have this kind of unlock-lock case. Wouldn't it be better to
> move the cancel_work calls outside this function and call them after
> __wl1271_plt_stop() has returned? Yeah, there will be duplicate code if
> we do this, but I think it's a bit safer, isn't it?
>
Heh, I did consider this too. But I did come to the conclusion that in
this case it is safer to unlock/lock than to duplicate the code -
duplicating this type of code has its risks too, and in this case
nothing is actually performed after the last mutex-lock - except for
unlocking in again.
I'll change this to benefit both PoVs: I'll create a separate function
to cancel the works, and just call that function from two places. That
way, the to-be cancelled work items are in a single place.
-Juuso
Hi Juuso,
On Fri, 2011-01-14 at 12:48 +0100, [email protected] wrote:
> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 9076555..863e660 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
> @@ -917,12 +917,10 @@ out:
> return ret;
> }
>
> -int wl1271_plt_stop(struct wl1271 *wl)
> +int __wl1271_plt_stop(struct wl1271 *wl)
> {
> int ret = 0;
>
> - mutex_lock(&wl->mutex);
> -
> wl1271_notice("power down");
>
> if (wl->state != WL1271_STATE_PLT) {
> @@ -938,12 +936,21 @@ int wl1271_plt_stop(struct wl1271 *wl)
> wl->state = WL1271_STATE_OFF;
> wl->rx_counter = 0;
>
> -out:
> mutex_unlock(&wl->mutex);
> -
> cancel_work_sync(&wl->irq_work);
> cancel_work_sync(&wl->recovery_work);
> + mutex_lock(&wl->mutex);
> +out:
> + return ret;
> +}
Again we have this kind of unlock-lock case. Wouldn't it be better to
move the cancel_work calls outside this function and call them after
__wl1271_plt_stop() has returned? Yeah, there will be duplicate code if
we do this, but I think it's a bit safer, isn't it?
--
Cheers,
Luca.
On Mon, 2011-01-17 at 08:04 +0200, ext Juuso Oikarinen wrote:
> On Fri, 2011-01-14 at 15:16 +0200, ext Luciano Coelho wrote:
> > Hi Juuso,
> >
> > On Fri, 2011-01-14 at 12:48 +0100, [email protected] wrote:
> > > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> > > index 9076555..863e660 100644
> > > --- a/drivers/net/wireless/wl12xx/main.c
> > > +++ b/drivers/net/wireless/wl12xx/main.c
> > > @@ -917,12 +917,10 @@ out:
> > > return ret;
> > > }
> >
> > Again we have this kind of unlock-lock case. Wouldn't it be better to
> > move the cancel_work calls outside this function and call them after
> > __wl1271_plt_stop() has returned? Yeah, there will be duplicate code if
> > we do this, but I think it's a bit safer, isn't it?
> >
>
> Heh, I did consider this too. But I did come to the conclusion that in
> this case it is safer to unlock/lock than to duplicate the code -
> duplicating this type of code has its risks too, and in this case
> nothing is actually performed after the last mutex-lock - except for
> unlocking in again.
>
> I'll change this to benefit both PoVs: I'll create a separate function
> to cancel the works, and just call that function from two places. That
> way, the to-be cancelled work items are in a single place.
>
And then I realise we can't easily do that. In the typical case (user
space stops plt mode) there is no work after the last mutex lock, but in
this module-removal scenario there is.
To solve that, we'd have to extend the knowledge about the plt mode onto
the bus specific modules (sdio/spi) because they call the wl12xx
unregister_hw function with the mutex already held.
So in essence, I would like to keep the current patch :o
-Juuso