Re: [PATCH v8 5/5] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Simon Wunderlich <sw@simonwunderlich.de>]

Hey Luca,

let me add first that there is another locking regression introduced by my 
latest fixes for the IBSS mode. I'll send a fix in a minute, please include it 
when you test/rebase.

Appearently there are a few locking problems in your patch too (do you have a 
chance to test it, somehow?), see comments inline:

> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -2982,21 +2982,19 @@ cfg80211_beacon_dup(struct cfg80211_beacon_data
> *beacon) return new_beacon;
>  }
> 
> -void ieee80211_csa_finalize_work(struct work_struct *work)
> +void ieee80211_csa_finish(struct ieee80211_vif *vif)
>  {
> -	struct ieee80211_sub_if_data *sdata =
> -		container_of(work, struct ieee80211_sub_if_data,
> -			     csa_finalize_work);
> -	struct ieee80211_local *local = sdata->local;
> -	int err, changed = 0;
> +	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> 
> -	sdata_lock(sdata);
> -	/* AP might have been stopped while waiting for the lock. */
> -	if (!sdata->vif.csa_active)
> -		goto unlock;
> +	ieee80211_queue_work(&sdata->local->hw,
> +			     &sdata->csa_finalize_work);
> +}
> +EXPORT_SYMBOL(ieee80211_csa_finish);
> 
> -	if (!ieee80211_sdata_running(sdata))
> -		goto unlock;
> +static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	int err, changed = 0;
> 
>  	sdata->radar_required = sdata->csa_radar_required;
>  	err = ieee80211_vif_change_channel(sdata, &changed);

This hunk is a little hard to understand, but at least in the resulting file I 
have here, csa_finalize() does sdata_unlock but never locks it. Lockdep 
complains about locking imbalance, and actually that's true ...

> @@ -3048,6 +3046,29 @@ unlock:
>  	sdata_unlock(sdata);
>  }
> 
> +void ieee80211_csa_finalize_work(struct work_struct *work)
> +{
> +	struct ieee80211_sub_if_data *sdata =
> +		container_of(work, struct ieee80211_sub_if_data,
> +			     csa_finalize_work);
> +
> +	sdata_lock(sdata);
> +	/* AP might have been stopped while waiting for the lock. */
> +	if (!sdata->vif.csa_active)
> +		goto unlock;
> +
> +	if (!ieee80211_sdata_running(sdata))
> +		goto unlock;
> +
> +	if (!ieee80211_sdata_running(sdata))
> +		return;

Why are you checking the same thing twice? The second time without unlocking, 
that's unsafe too ...

> +
> +	ieee80211_csa_finalize(sdata);
> +
> +unlock:
> +	sdata_unlock(sdata);
> +}
> +
>  int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>  			     struct cfg80211_csa_settings *params)
>  {

Cheers,
    Simon