[rt2x00-users] [RFC 1/3] rt2x00: Simplify locking in rt2x00mac_bss_info_changed

[Helmut Schaa]

Am Samstag 04 Dezember 2010 schrieb Ivo Van Doorn:
> Hi,
> 
> > Locking the bssid should not be needed since it is only accessed from
> > within rt2x00mac_bss_info_changed.
> >
> > Signed-off-by: Helmut Schaa <helmut.schaa at googlemail.com>
> > ---
> >
> > Ivo, do you remember why this locking was needed? At least it looks to me as if
> > it is not required anymore.
> >
> > Could somebody please review my assumption?
> 
> By the looks of it, it protected the BSSID and the changed_flags.
> The locking looks a bit as overkill at the moment, but I think it was
> used more in the past and over time the code was restructured to
> require less locking.
> 
> 
> >  drivers/net/wireless/rt2x00/rt2x00mac.c |   18 +++---------------
> >  1 files changed, 3 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
> > index b49170d..70c60f2 100644
> > --- a/drivers/net/wireless/rt2x00/rt2x00mac.c
> > +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
> > @@ -614,26 +614,14 @@ void rt2x00mac_bss_info_changed(struct ieee80211_hw *hw,
> >        if (!test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags))
> >                return;
> >
> > -       spin_lock(&intf->lock);
> > -
> >        /*
> > -        * conf->bssid can be NULL if coming from the internal
> > -        * beacon update routine.
> > +        * Update bssid.
> >         */
> > -       if (changes & BSS_CHANGED_BSSID)
> > +       if (changes & BSS_CHANGED_BSSID) {
> >                memcpy(&intf->bssid, bss_conf->bssid, ETH_ALEN);
> > -
> > -       spin_unlock(&intf->lock);
> > -
> > -       /*
> > -        * Call rt2x00_config_intf() outside of the spinlock context since
> > -        * the call will sleep for USB drivers. By using the ieee80211_if_conf
> > -        * values as arguments we make keep access to rt2x00_intf thread safe
> > -        * even without the lock.
> > -        */
> > -       if (changes & BSS_CHANGED_BSSID)
> >                rt2x00lib_config_intf(rt2x00dev, intf, vif->type, NULL,
> >                                      bss_conf->bssid);
> > +       }
> 
> Could there be any problems in the multi-BSS case where
> add_interface/remove_interface is called concurrently with
> rt2x00mac_bss_info_changed() ?
> Because then there could be a race condition with the calls to
> rt2x00lib_config_intf

Good point, I'll recheck that.

Thanks,
Helmut