On Fri, 2011-01-14 at 15:16 +0200, ext Luciano Coelho wrote:
> Hi Juuso,
>
> On Fri, 2011-01-14 at 12:48 +0100, juuso.oikarinen@xxxxxxxxx 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
