Re: [PATCH] net: mac80211: rx.c: Use built-in RCU list checking

[Johannes Berg <johannes@sipsolutions.net>]

> If list_for_each_entry_rcu() is called from non rcu protection
> i.e without holding rcu_read_lock, but under the protection of
> a different lock then we can pass that as the condition for lockdep checking
> because otherwise lockdep will complain if list_for_each_entry_rcu()
> is used without rcu protection. So, if we do not pass this argument
> (cond) it may lead to false lockdep warnings.

Sure. But what's the specific warning you see?

> > > -	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > > +	list_for_each_entry_rcu(sdata, &local->interfaces, list,
> > > +				lockdep_is_held(&rx->local->rx_path_lock)) {
> > >  		if (!ieee80211_sdata_running(sdata))
> > >  			continue;
> > 
> > This is not related at all.
> 
> I analysed the following traces:
> ieee80211_rx_handlers() -> ieee80211_rx_handlers_result() -> ieee80211_rx_cooked_monitor()
> 
> here ieee80211_rx_handlers() is holding the rx->local->rx_path_lock and
> therefore I used this for the cond argument.
> 
>  If this is not right, can you help me in figuring out that which other
>  lock is held?

It's _clearly_ not right, that's the RX spinlock, it has nothing to do
with the interface list.

But I'd have to see the warning. Perhaps the driver you're using is
wrongly calling something in the stack.

> > >  	lockdep_assert_held(&local->sta_mtx);
> > >  
> > > -	list_for_each_entry_rcu(sta, &local->sta_list, list) {
> > > +	list_for_each_entry_rcu(sta, &local->sta_list, list,
> > > +				lockdep_is_held(&local->sta_mtx)) {
> > 
> > And this isn't even a real RCU iteration, since we _must_ hold the mutex
> > here.
> > 
> Yeah exactly, dropping _rcu (use list_for_each_entry()) would be a good option in this case.
> Let me know if that is alright and I will send a new patch with all the
> changes required.

Seems fine, also better to split the patches anyway.

johannes