Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 4 Mar 2022 00:54:39 +0200
From:   Jouni Malinen <j@w1.fi>
To:     Lorenzo Bianconi <lorenzo@kernel.org>
Cc:     hostap@lists.infradead.org, linux-wireless@vger.kernel.org,
        nbd@nbd.name, ryder.lee@mediatek.com, evelyn.tsai@mediatek.com,
        lorenzo.bianconi@redhat.com
Subject: Re: [PATCH 6/9] DFS: introduce hostapd_dfs_request_channel_switch
 routine
Message-ID: <20220303225439.GD33342@w1.fi>
References: <cover.1640014128.git.lorenzo@kernel.org>
 <c1bf7dc195281330e7108c533da5770adf479d84.1640014128.git.lorenzo@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c1bf7dc195281330e7108c533da5770adf479d84.1640014128.git.lorenzo@kernel.org>
Precedence: bulk

On Mon, Dec 20, 2021 at 04:48:21PM +0100, Lorenzo Bianconi wrote:
> This is a preliminary patch to add Channel Switch Announcement for
> background radar detection.

> diff --git a/src/ap/dfs.c b/src/ap/dfs.c
> @@ -940,6 +940,85 @@ int hostapd_is_dfs_chan_available(struct hostapd_iface *iface)
>  
> +static int hostapd_dfs_request_channel_switch(struct hostapd_iface *iface,

> +	struct hostapd_hw_modes *cmode = iface->current_mode;
> +	int ieee80211_mode = IEEE80211_MODE_AP, err, i;

That i needs to be unsigned int (like it was in the function from which
it was moved to here) to avoid a compiler warning.

> +	u8 new_vht_oper_chwidth;

> +	new_vht_oper_chwidth = hostapd_get_oper_chwidth(iface->conf);
> +	hostapd_set_oper_chwidth(iface->conf,
> +				 hostapd_get_oper_chwidth(iface->conf));

This looks quite strange.. hostapd_dfs_start_channel_switch() used
current_vht_oper_chwidth to store the current value from beginning of
the function and restored it at this location. The new version in this
helper function seems to something strange, or well, nothing. That does
not feel correct.

>  static int hostapd_dfs_start_channel_switch(struct hostapd_iface *iface)
>  {
> -	unsigned int i;

That's where i was unsigned..

> -	u8 current_vht_oper_chwidth = hostapd_get_oper_chwidth(iface->conf);

And that's where the current value is picked at the beginning of
hostapd_dfs_start_channel_switch().

> -	new_vht_oper_chwidth = hostapd_get_oper_chwidth(iface->conf);
> -	hostapd_set_oper_chwidth(iface->conf, current_vht_oper_chwidth);

And this looks quite different compared to what was in the new helper
function.

-- 
Jouni Malinen                                            PGP id EFC895FA