Re: [PATCH 4/7] wl12xx: prevent scheduling while suspending (WoW enabled)

[Luciano Coelho <coelho@ti.com>]

On Thu, 2011-05-12 at 21:52 +0200, Johannes Berg wrote:
> On Thu, 2011-05-12 at 22:48 +0300, Luciano Coelho wrote:
> 
> > > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> > > index 9ca71ce..308855a 100644
> > > --- a/drivers/net/wireless/wl12xx/main.c
> > > +++ b/drivers/net/wireless/wl12xx/main.c
> > > @@ -1338,6 +1338,28 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
> > >  	struct wl1271 *wl = hw->priv;
> > >  	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
> > >  	wl->wow_enabled = !!wow;
> > > +	if (wl->wow_enabled) {
> > > +		/* flush any remaining work */
> > > +		wl1271_debug(DEBUG_MAC80211, "flushing remaining works");
> > > +		flush_delayed_work(&wl->scan_complete_work);
> > > +
> > > +		/*
> > > +		 * disable and re-enable interrupts in order to flush
> > > +		 * the threaded_irq
> > > +		 */
> > > +		wl1271_disable_interrupts(wl);
> > > +
> > > +		/*
> > > +		 * set suspended flag to avoid triggering a new threaded_irq
> > > +		 * work. no need for spinlock as interrupts are disabled.
> > > +		 */
> > > +		set_bit(WL1271_FLAG_SUSPENDED, &wl->flags);
> > 
> > The set_bit() function is atomic, so you wouldn't need the spinlock here
> > anyway.  Couldn't you use __set_bit() instead, to avoid the expense of
> > using an atomic function when it's not necessary?
> 
> That's only possible if the spinlock is held for all modifications of
> the variable. If there are any modifications that don't hold it, then
> you need the atomic version.

Ah, you're right.   wl->flags is being modified in other places too (ie.
other bits are being set/cleared in other parts of the code), so there
could be some conflicts.  I had just considered the flag as an
independent boolean, which it obviously isn't.

My next comment about changing to __set_bit() can also be disregarded.

-- 
Cheers,
Luca.