On Mon, 23 Apr 2007 14:48:13 -0400, Michael Wu wrote:
> This converts sub_if_lock to a rw lock and makes all code touching
> sub_if_list use it, grabs mdev's tx lock in set_multicast_list to
> synchronize multicast configuration, and simplifies some related code.
>
> [...]
> --- a/net/mac80211/ieee80211.c
> +++ b/net/mac80211/ieee80211.c
> [...]
> @@ -2148,6 +2148,7 @@ static void ieee80211_set_multicast_list(struct net_device *dev)
> struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> unsigned short flags;
>
> + netif_tx_lock(local->mdev);
Shouldn't it be netif_tx_lock_bh?
> if (((dev->flags & IFF_ALLMULTI) != 0) ^ (sdata->allmulti != 0)) {
> if (sdata->allmulti) {
> sdata->allmulti = 0;
> @@ -2177,9 +2178,12 @@ static void ieee80211_set_multicast_list(struct net_device *dev)
> flags |= IFF_ALLMULTI;
> if (local->iff_promiscs)
> flags |= IFF_PROMISC;
> + read_lock(&local->sub_if_lock);
> local->ops->set_multicast_list(local_to_hw(local), flags,
> local->mc_count);
Would be nice to have documented that set_multicast_list is called in
atomic context.
> + read_unlock(&local->sub_if_lock);
> }
> + netif_tx_unlock(local->mdev);
> }
>
> struct dev_mc_list *ieee80211_get_mc_list_item(struct ieee80211_hw *hw,
> @@ -2220,12 +2224,11 @@ static struct net_device_stats *ieee80211_get_stats(struct net_device *dev)
> return &(sdata->stats);
> }
>
> -void ieee80211_if_shutdown(struct net_device *dev)
> +static void ieee80211_if_shutdown(struct net_device *dev)
> {
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>
> - ASSERT_RTNL();
As you already said, this is not necessary.
> [...]
> @@ -3871,6 +3884,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
> struct sk_buff *skb_new;
> u8 *bssid = ieee80211_get_bssid(hdr, skb->len - radiotap_len);
>
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
> rx.u.rx.ra_match = 1;
> switch (sdata->type) {
> @@ -3906,10 +3920,9 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
> rx.u.rx.ra_match = 0;
> } else if (!sta)
> sta = rx.sta =
> - ieee80211_ibss_add_sta(local->mdev,
> + ieee80211_ibss_add_sta(sdata->dev,
> skb, bssid,
> hdr->addr2);
> - /* FIXME: call with sdata->dev */
> break;
> case IEEE80211_IF_TYPE_AP:
> if (!bssid) {
> @@ -3964,6 +3977,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
> &rx, sta);
> } else
> dev_kfree_skb(skb);
> + read_unlock(&local->sub_if_lock);
Documenting that sub_if_lock cannot be taken under sta_bss_lock nor
under sta_lock would be nice.
local->ops->conf_tx and ->set_time are called under spinlock, are we ok with that?
> [...]
> --- a/net/mac80211/ieee80211_ioctl.c
> +++ b/net/mac80211/ieee80211_ioctl.c
> @@ -1003,23 +1003,30 @@ static int ieee80211_ioctl_add_if(struct net_device *dev,
> if (left < sizeof(struct hostapd_if_wds))
> return -EPROTO;
>
> - res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
> + res = ieee80211_if_add(dev, param->u.if_info.name, &new_dev,
> + IEEE80211_IF_TYPE_WDS);
> if (res)
> return res;
> - ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_WDS);
> res = ieee80211_if_update_wds(new_dev, wds->remote_addr);
> - if (res)
> - __ieee80211_if_del(wdev_priv(dev->ieee80211_ptr),
> - IEEE80211_DEV_TO_SUB_IF(new_dev));
> + if (unlikely(res)) {
> + struct ieee80211_local *local =
> + wdev_priv(dev->ieee80211_ptr);
> + struct ieee80211_sub_if_data *sdata =
> + IEEE80211_DEV_TO_SUB_IF(new_dev);
> + write_lock_bh(&local->sub_if_lock);
> + list_del(&sdata->list);
> + write_unlock_bh(&local->sub_if_lock);
> + __ieee80211_if_del(local, sdata);
> + }
A good candidate for a separate function :-) But it can be done later
when this is needed at some other place.
> return res;
> case HOSTAP_IF_VLAN:
> if (left < sizeof(struct hostapd_if_vlan))
> return -EPROTO;
>
> - res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
> + res = ieee80211_if_add(dev, param->u.if_info.name, NULL,
> + IEEE80211_IF_TYPE_VLAN);
> if (res)
> return res;
> - ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_VLAN);
> #if 0
> res = ieee80211_if_update_vlan(new_dev, vlan->id);
> if (res)
I know that the next few lines are commented out but please fix them as
well or remove the #ifed out code completely, otherwise we may be in
trouble later.
> [...]
> @@ -1093,36 +1098,34 @@ static int ieee80211_ioctl_update_if(struct net_device *dev,
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> struct net_device *wds_dev = NULL;
> struct ieee80211_sub_if_data *sdata;
> + int ret;
>
> if (left < sizeof(struct ieee80211_if_wds))
> return -EPROTO;
>
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
> if (strcmp(param->u.if_info.name,
> sdata->dev->name) == 0) {
> wds_dev = sdata->dev;
> + dev_hold(wds_dev);
> break;
> }
> }
> + read_unlock(&local->sub_if_lock);
>
> if (!wds_dev || sdata->type != IEEE80211_IF_TYPE_WDS)
> return -ENODEV;
>
> - return ieee80211_if_update_wds(wds_dev, wds->remote_addr);
> + ret = ieee80211_if_update_wds(wds_dev, wds->remote_addr);
> + dev_put(wds_dev);
This was the place with unnecessary dev_hold/dev_put you talked about,
right? Just wanted to show I'm paying attention :-)
> [...]
> @@ -2239,6 +2239,7 @@ static int ieee80211_ioctl_clear_keys(struct net_device *dev)
> struct sta_info *sta;
>
> memset(addr, 0xff, ETH_ALEN);
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
> for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
> keyconf = NULL;
> @@ -2256,6 +2257,7 @@ static int ieee80211_ioctl_clear_keys(struct net_device *dev)
> }
> sdata->default_key = NULL;
> }
> + read_unlock(&local->sub_if_lock);
Again, local->ops->set_key called under spinlock.
>
> spin_lock_bh(&local->sta_lock);
> list_for_each_entry(sta, &local->sta_list, list) {
> @@ -2388,6 +2390,7 @@ static int ieee80211_ioctl_default_wep_only(struct ieee80211_local *local,
> struct ieee80211_sub_if_data *sdata;
>
> local->default_wep_only = value;
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list)
> for (i = 0; i < NUM_DEFAULT_KEYS; i++)
> if (value)
> @@ -2396,6 +2399,7 @@ static int ieee80211_ioctl_default_wep_only(struct ieee80211_local *local,
> else
> ieee80211_key_disable_hwaccel(local,
> sdata->keys[i]);
> + read_unlock(&local->sub_if_lock);
>
> return 0;
> }
> @@ -2406,7 +2410,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
> int i = 0;
> struct ieee80211_sub_if_data *sdata;
>
> - spin_lock_bh(&local->sub_if_lock);
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
>
> if (sdata->dev == local->mdev)
> @@ -2415,7 +2419,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
> /* If there is an AP interface then depend on userspace to
> set default_wep_only correctly. */
> if (sdata->type == IEEE80211_IF_TYPE_AP) {
> - spin_unlock_bh(&local->sub_if_lock);
> + read_unlock(&local->sub_if_lock);
> return;
> }
>
> @@ -2427,7 +2431,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
> else
> ieee80211_ioctl_default_wep_only(local, 0);
Recursively taken lock. Ok, it's rwlock locked for reading, but I'd
still prefer not doing that.
>
> - spin_unlock_bh(&local->sub_if_lock);
> + read_unlock(&local->sub_if_lock);
> }
> [...]
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
-
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
