RE: [PATCH] wifi: rtw89: Don't return default channel from disabled bands

[Ping-Ke Shih <pkshih@realtek.com>]

Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote:
> rtw89_get_default_chandef() assumes the lowest frequency channel in the
> 2GHz band is available on all hardware, and always returns that as the
> default channel. This is no longer the case after commit 355626a2c232
> ("wifi: rtw89: 8852cu: add quirk to disable 2.4 GHz band"), and the
> current logic results in kernel WARNs and null pointer dereferences on
> boards with that quirk set.

Could you share the kernel WARN?

> 
> Update rtw89_get_default_chandef() to consider the available bands when
> picking the default channel.
> 
> Fixes: 355626a2c232 ("wifi: rtw89: 8852cu: add quirk to disable 2.4 GHz band")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  drivers/net/wireless/realtek/rtw89/chan.c |  2 +-
>  drivers/net/wireless/realtek/rtw89/core.c | 21 ++++++++++++++++++---
>  drivers/net/wireless/realtek/rtw89/core.h |  3 ++-
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/chan.c b/drivers/net/wireless/realtek/rtw89/chan.c
> index 6f11335b4968..6512fc9eef29 100644
> --- a/drivers/net/wireless/realtek/rtw89/chan.c
> +++ b/drivers/net/wireless/realtek/rtw89/chan.c
> @@ -297,7 +297,7 @@ static void rtw89_config_default_chandef(struct rtw89_dev *rtwdev)
>  {
>         struct cfg80211_chan_def chandef = {0};
> 
> -       rtw89_get_default_chandef(&chandef);
> +       rtw89_get_default_chandef(rtwdev, &chandef);
>         __rtw89_config_entity_chandef(rtwdev, RTW89_CHANCTX_0, &chandef);
>  }
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index 397ebbfcac09..5ae9523667c6 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -393,10 +393,25 @@ static void rtw89_traffic_stats_accu(struct rtw89_dev *rtwdev,
>         }
>  }
> 
> -void rtw89_get_default_chandef(struct cfg80211_chan_def *chandef)
> +void rtw89_get_default_chandef(struct rtw89_dev *rtwdev,
> +                              struct cfg80211_chan_def *chandef)
>  {
> -       cfg80211_chandef_create(chandef, &rtw89_channels_2ghz[0],
> -                               NL80211_CHAN_NO_HT);
> +       u8 support_bands = rtwdev->chip->support_bands;
> +       struct ieee80211_channel *default_channel;
> +
> +       if (support_bands & BIT(NL80211_BAND_2GHZ) &&
> +           !test_bit(RTW89_QUIRK_DISABLE_2GHZ, rtwdev->quirks)) {

I'd prefer the style implemented in rtw89_core_set_supported_band() before
this if-branch.

	if (test_bit(RTW89_QUIRK_DISABLE_2GHZ, rtwdev->quirks))
		support_bands &= ~BIT(NL80211_BAND_2GHZ);


> +               default_channel = &rtw89_channels_2ghz[0];
> +       } else if (support_bands & BIT(NL80211_BAND_5GHZ)) {
> +               default_channel = &rtw89_channels_5ghz[0];
> +       } else if (support_bands & BIT(NL80211_BAND_6GHZ)) {
> +               default_channel = &rtw89_channels_6ghz[0];
> +       } else {
> +               rtw89_err(rtwdev, "Failed to get default channel, no band supported\n");
> +               return;

If it somehow falls into this case, won't it warn or null-dereference?


> +       }
> +
> +       cfg80211_chandef_create(chandef, default_channel, NL80211_CHAN_NO_HT);
>  }
> 
>  void rtw89_get_channel_params(const struct cfg80211_chan_def *chandef,