Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <cover.1697719099.git.dberlin@dberlin.org> <d2ef482bcf01fc515224cd508d730590a5294df2.1697719099.git.dberlin@dberlin.org>
In-Reply-To: <d2ef482bcf01fc515224cd508d730590a5294df2.1697719099.git.dberlin@dberlin.org>
From:   Daniel Berlin <dberlin@dberlin.org>
Date:   Sat, 21 Oct 2023 10:40:41 -0400
Message-ID: <CAF4BwTVQf25uCCVovtce+kjz0pFFMZX=Ln36R4Vd-1LEsxrcMw@mail.gmail.com>
Subject: Re: [PATCH 1/1] [brcmfmac] Fix regulatory domain handling to reset
 bands properly
To:     Arend van Spriel <aspriel@gmail.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>
Cc:     linux-wireless@vger.kernel.org,
        brcm80211-dev-list.pdl@broadcom.com,
        SHA-cyfmac-dev-list@infineon.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

As an aside, the alternative would be to simply not ignore any
attempts to set the regulatory domain, regardless of whether it's 00
or the chip is already set to that country.
It doesn't happen that often, so it's not clear it's worth avoiding it at a=
ll.
There are some things i'd have to fix to make it work resiliently well
(for example, i know we set the 2g bw cap where we do because it has
to be done with the interface down), but i can fix those if needed.

On Thu, Oct 19, 2023 at 10:18=E2=80=AFAM Daniel Berlin <dberlin@dberlin.org=
> wrote:
>
> Currently, we ignore the default country in the reg notifier.
> We also register a custom regulatory domain, which is set
> as the default.
> As a result, the chip is likely to be set to the correct country,
> but the regulatory domain will not match it.
>
> When the regulatory notifier is then called, we see the countries
> are the same and do not change anything, even though the domain
> is wrong.
>
> This patch forces us to reset the bands on the first country change
> even if the chip is already set to that country.
>
> We also restore the original band info before reconstructing channel
> info, as the new regdom power limits may be higher than what is
> currently set.
>
> Signed-off-by: Daniel Berlin <dberlin@dberlin.org>
> ---
>  .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 38 +++++++++++++++----
>  .../broadcom/brcm80211/brcmfmac/cfg80211.h    |  2 +
>  2 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c =
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 3656790ec4c9..7bc479ccc24b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -7199,11 +7199,23 @@ static int brcmf_construct_chaninfo(struct brcmf_=
cfg80211_info *cfg,
>                 goto fail_pbuf;
>         }
>
> +       /* Changing regulatory domain may change channels and limits
> +        * To ensure that we correctly set the new band info, copy the or=
iginal
> +        * info first.
> +        */
>         band =3D wiphy->bands[NL80211_BAND_2GHZ];
> -       if (band)
> +       if (band) {
> +               memcpy(band->channels, &__wl_2ghz_channels,
> +                      sizeof(__wl_2ghz_channels));
> +               band->n_channels =3D ARRAY_SIZE(__wl_2ghz_channels);
>                 for (i =3D 0; i < band->n_channels; i++)
>                         band->channels[i].flags =3D IEEE80211_CHAN_DISABL=
ED;
> +       }
>         band =3D wiphy->bands[NL80211_BAND_5GHZ];
> -       if (band)
> +       if (band) {
> +               memcpy(band->channels, &__wl_5ghz_channels,
> +                      sizeof(__wl_5ghz_channels));
> +               band->n_channels =3D ARRAY_SIZE(__wl_5ghz_channels);
>                 for (i =3D 0; i < band->n_channels; i++)
>                         band->channels[i].flags =3D IEEE80211_CHAN_DISABL=
ED;
> +       }
> @@ -7210,8 +7222,11 @@
>         band =3D wiphy->bands[NL80211_BAND_6GHZ];
> -       if (band)
> +       if (band) {
> +               memcpy(band->channels, &__wl_6ghz_channels,
> +                      sizeof(__wl_6ghz_channels));
> +               band->n_channels =3D ARRAY_SIZE(__wl_6ghz_channels);
>                 for (i =3D 0; i < band->n_channels; i++)
>                         band->channels[i].flags =3D IEEE80211_CHAN_DISABL=
ED;
> -
> +       }
>         total =3D le32_to_cpu(list->count);
>         if (total > BRCMF_MAX_CHANSPEC_LIST) {
>                 bphy_err(drvr, "Invalid count of channel Spec. (%u)\n",
> @@ -8601,9 +8616,17 @@ static void brcmf_cfg80211_reg_notifier(struct wip=
hy *wiphy,
>         }
>
>         err =3D brcmf_translate_country_code(ifp->drvr, req->alpha2, &ccr=
eq);
> -       if (err)
> -               return;
> -
> +       if (err) {
> +               /* Because we ignore the default country code above,
> +                * we will start out in our custom reg domain, but the ch=
ip
> +                * may already be set to the right country.
> +                * As such, we force the bands to be re-set the first
> +                * time we try to set a country for real.
> +                */
> +               if (err !=3D -EAGAIN || !cfg->force_band_setup)
> +                       return;
> +       }
> +       cfg->force_band_setup =3D false;
>         err =3D brcmf_fil_iovar_data_set(ifp, "country", &ccreq, sizeof(c=
creq));
>         if (err) {
>                 bphy_err(drvr, "Firmware rejected country setting\n");
> @@ -8670,6 +8693,7 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(s=
truct brcmf_pub *drvr,
>         cfg->pub =3D drvr;
>         init_vif_event(&cfg->vif_event);
>         INIT_LIST_HEAD(&cfg->vif_list);
> +       cfg->force_band_setup =3D true;
>
>         vif =3D brcmf_alloc_vif(cfg, NL80211_IFTYPE_STATION);
>         if (IS_ERR(vif))
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h =
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> index 0e1fa3f0dea2..7e60ceeeeb3a 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> @@ -327,6 +327,7 @@ struct brcmf_cfg80211_wowl {
>   * @dongle_up: indicate whether dongle up or not.
>   * @roam_on: on/off switch for dongle self-roaming.
>   * @scan_tried: indicates if first scan attempted.
> + * @force_band_setup: indicates if we should force band setup
>   * @dcmd_buf: dcmd buffer.
>   * @extra_buf: mainly to grab assoc information.
>   * @debugfsdir: debugfs folder for this device.
> @@ -357,6 +358,7 @@ struct brcmf_cfg80211_info {
>         bool pwr_save;
>         bool dongle_up;
>         bool scan_tried;
> +       bool force_band_setup;
>         u8 *dcmd_buf;
>         u8 *extra_buf;
>         struct dentry *debugfsdir;
> --
> 2.41.0
>