Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <9de7dbefe3cf0682fd28e5f4c42324e6a58ce508.camel@sipsolutions.net>
Subject: Re: [PATCH v3 1/3] nl80211: Extended ROC support for 40-80 MHz
 bandwidth
From:   Johannes Berg <johannes@sipsolutions.net>
To:     P Praneesh <ppranees@codeaurora.org>, ath11k@lists.infradead.org
Cc:     linux-wireless@vger.kernel.org,
        Sathishkumar Muruganandam <murugana@codeaurora.org>
Date:   Mon, 25 Oct 2021 21:59:28 +0200
In-Reply-To: <1634906227-22028-2-git-send-email-ppranees@codeaurora.org>
References: <1634906227-22028-1-git-send-email-ppranees@codeaurora.org>
         <1634906227-22028-2-git-send-email-ppranees@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Fri, 2021-10-22 at 18:07 +0530, P Praneesh wrote:
> 
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -4181,7 +4181,7 @@ struct cfg80211_ops {
>  
> 
> 
> 
>  	int	(*remain_on_channel)(struct wiphy *wiphy,
>  				     struct wireless_dev *wdev,
> -				     struct ieee80211_channel *chan,
> +				     struct cfg80211_chan_def *chandef,
>  				     unsigned int duration,
>  				     u64 *cookie);

I find it's odd you update this, but
> 
>  	if (!roc->mgmt_tx_cookie)
>  		cfg80211_remain_on_channel_expired(&roc->sdata->wdev,
> -						   roc->cookie, roc->chan,
> +						   roc->cookie,
> +						   roc->chandef.chan,
>  						   GFP_KERNEL);

not the reporting API? Would seem better to pass back the whole thing?

> @@ -211,6 +212,7 @@ static unsigned long ieee80211_end_finished_rocs(struct ieee80211_local *local,
>  		 * ROC session before the actual requested time. In such a case
>  		 * end the ROC session (disregarding the remaining time).
>  		 */
> +
>  		if (roc->abort || roc->hw_begun || remaining <= 0)
>  			ieee80211_roc_notify_destroy(roc);


spurious change?

> @@ -311,7 +316,7 @@ static void _ieee80211_start_next_roc(struct ieee80211_local *local)
>  	list_for_each_entry(tmp, &local->roc_list, list) {
>  		if (tmp == roc)
>  			continue;
> -		if (tmp->sdata != roc->sdata || tmp->chan != roc->chan)
> +		if (tmp->sdata != roc->sdata || tmp->chandef.chan != roc->chandef.chan)
>  			break;

This really doesn't work - you need to actually compare the chandefs for
being identical or at least compatible when merging etc. happens here.

At least if we want mac80211 to support this feature (more on that
later)

> @@ -331,7 +337,7 @@ static void _ieee80211_start_next_roc(struct ieee80211_local *local)
>  			 */
>  			list_for_each_entry(tmp, &local->roc_list, list) {
>  				if (tmp->sdata != roc->sdata ||
> -				    tmp->chan != roc->chan)
> +				    tmp->chandef.chan != roc->chandef.chan)
>  					break;

likewise

> +	roc->chandef.chan = chandef->chan;
> +	roc->chandef.width = chandef->width;
> +	roc->chandef.center_freq1 = chandef->center_freq1;
> +	roc->chandef.center_freq2 = chandef->center_freq2;

That probably should use a struct assignment - there are some more
struct members you're now not setting.
> 
>  	mutex_lock(&local->mtx);
> -	ret = ieee80211_start_roc_work(local, sdata, chan,
> -				       duration, cookie, NULL,
> -				       IEEE80211_ROC_TYPE_NORMAL);
> +	if (chandef) {
> +		ret = ieee80211_start_roc_work(local, sdata, chandef,
> +					       duration, cookie, NULL,
> +					       IEEE80211_ROC_TYPE_NORMAL);
> +	}

No need for braces, but how would that actually be NULL?!
> 
> +	chandef.chan = params->chan;

Uh, well, the other fields of a stack struct really want to be
initialized too :)


> @@ -1244,8 +1244,9 @@ TRACE_EVENT(drv_remain_on_channel,
>  	TP_fast_assign(
>  		LOCAL_ASSIGN;
>  		VIF_ASSIGN;
> -		__entry->center_freq = chan->center_freq;
> -		__entry->freq_offset = chan->freq_offset;
> +		CHANDEF_ASSIGN(chandef);
> +		__entry->center_freq = chandef->chan->center_freq;
> +		__entry->freq_offset = chandef->chan->freq_offset;

Why would we need center_freq/freq_offset if we have a whole chandef?

We're not usually taking much care with compatibility here with tracing.

> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -11282,7 +11282,7 @@ static int nl80211_remain_on_channel(struct
> sk_buff *skb,
>  		goto free_msg;
>  	}
>  
> -	err = rdev_remain_on_channel(rdev, wdev, chandef.chan,
> +	err = rdev_remain_on_channel(rdev, wdev, &chandef,
>  				     duration, &cookie);
>  
>  	if (err)
> 

Really though, the biggest issue I see with this that you added no
feature advertisement, no checks for the channel even being valid,
nothing?

Seems like we should have some kind of feature check, at least to reject
40/80/160/whatever bandwidths with all the existing drivers you only
updated the API on, but didn't actually implement the new changes? I.e.
this really needs to be opt-in somehow.

And for mac80211 we need to implement some different merge logic, etc.,
it seems.

johannes