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