On Monday 23 April 2007 16:41, Jiri Benc wrote:
> > + netif_tx_lock(local->mdev);
>
> Shouldn't it be netif_tx_lock_bh?
>
Softirqs should already be disabled by the caller.
> > 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.
>
Sure, but this is no different from other network drivers.
> > [...]
> > @@ -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?
>
Since conf_tx is being called from ieee80211_sta.c, I'd rather have it run
from the workqueue. As far set_time.. would be nice to call it outside of the
tasklet but that can be addressed in another patch. Calling it under readlock
is okay since writes are so rare and the callback isn't allowed to sleep
anyway.
> > 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.
>
It doesn't even compile now since ieee80211_if_update_vlan doesn't exist
anymore.
> > [...]
> > @@ -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 :-)
>
Yup.
> > [...]
> > @@ -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.
>
The good thing about this rwlock is that writers are extremely rare. (and not
going to happen in this case since we're holding rtnl which is a prerequisite
to adding/removing virtual interfaces)
> Recursively taken lock. Ok, it's rwlock locked for reading, but I'd
> still prefer not doing that.
>
Opps. Good catch.
I'll post a new patch in an hour or so.
Thanks,
-Michael Wu
Attachment:
pgpUVah2KBA2g.pgp
Description: PGP signature
