On Mon, 2010-06-07 at 17:09 +0200, ext Johannes Berg wrote: > On Mon, 2010-06-07 at 16:42 +0300, Juuso Oikarinen wrote: > > > > > +#ifdef CONFIG_INET > > > > + cancel_work_sync(&sdata->u.mgd.arp_config_work); > > > > +#endif > > > > > > No can do, this is under RTNL and thus can't block waiting for a work > > > that acquires the RTNL ... the work might already be running, waiting > > > for the RTNL, by the time you get here. This will also get you a lockdep > > > complaint. > > > > The work-func escapes based on ieee80211_sdata_running before acquiring > > the rtnl - hence here it should never get to the lock. I saw the same > > being used in those other work funcs too. I was beginning work to try to > > validate that, so it's not enough? > > No, it's not enough. Like I said: "the work might already be running, > waiting for the RTNL" -- and once it obtains the RTNL, sdata has already > been freed! > > > > This is why > > > > > > > @@ -379,7 +379,8 @@ static int ieee80211_ifa_changed(struct notifier_block *nb, > > > > ifmgd = &sdata->u.mgd; > > > > mutex_lock(&ifmgd->mtx); > > > > if (ifmgd->associated) > > > > - ieee80211_set_arp_filter(sdata); > > > > + ieee80211_queue_work(&sdata->local->hw, > > > > + &sdata->u.mgd.arp_config_work); > > > > mutex_unlock(&ifmgd->mtx); > > > > > > No need to do change it here since the rtnl is held outside. > > > > Yes, I know rtnl is held outside. I changed this for two reasons. First, > > I think it maybe better if the driver function is always called in the > > same scope. Secondly, this gets rid of inetdevs intermediate > > configurations (if you change IP address, it will first remove the > > previous one, and immediately after add a new one, resulting in two > > calls to the driver config function.) > > Interesting. But that only works if it does that both together under > rtnl. I don't think the context really matters -- it should be the same > at least from a locking POV you have both rtnl and mgd->mtx acquired. > > > > Also, and this applies to the change in mlme.c too, you must never put > > > work that acquires the rtnl onto the mac80211 workqueue ... that's what > > > you were trying to fix to start with! > > > > > But because the interface might go away before your work runs, you're in > > > a stupid situation where you can't really use a per-interface work > > > either ... I think you probably need to have the work in ieee80211_local > > > and iterate the interface list. > > > > I thought about iterating the interface list. I assume you imply calling > > the configure function for every interface. > > Only if it's associated, I guess. > > > Going still back to the current patch: assuming that you overlooked the > > sdata_running() call in the arp_config_work() function, and we can after > > all cancel_work_sync in _stop(), would using the kernel's default > > workqueue solve the rtnl problem, or are the rtnl dependencies there > > too? > > Using the default wq would solve the rtnl from workqueue problem, > obviously, but wouldn't fix the cancel_work_sync problem. > Yeah, I actually did realize this during the night. No, I wasn't sleepless - my brain still has a tendency to generate realizations for the joy of the morning. I'll try to think about alternative solutions. The driver provided list idea is neat, but I think will scale badly for multiple vif's. -Juuso > johannes > -- 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
