Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <cover.1697650207.git.dberlin@dberlin.org> <52c993fd93e13ac015be935a5284294c9a74ea8e.1697650207.git.dberlin@dberlin.org>
 <cc58057b-bc08-f717-1676-13046fc26c5c@broadcom.com>
In-Reply-To: <cc58057b-bc08-f717-1676-13046fc26c5c@broadcom.com>
From:   Daniel Berlin <dberlin@dberlin.org>
Date:   Fri, 20 Oct 2023 12:35:16 -0400
Message-ID: <CAF4BwTUTmUd0c-y_NfSi9WkCnDO9bhtpx03Aai1ByH5auq9YXw@mail.gmail.com>
Subject: Re: [PATCH 2/5] [brcmfmac] Add support for 6G bands
To:     Arend van Spriel <arend.vanspriel@broadcom.com>
Cc:     Arend van Spriel <aspriel@gmail.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        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"
Precedence: bulk

>
>
> maybe handle channel 2 here as well, ie.:
>         .center_freq = ((_channel) == 2) ? 5935 : 5950 + (5 * (_channel)),
>
> > +     .hw_value               = (_channel),                   \
> > +     .max_antenna_gain       = 0,                            \
> > +     .max_power              = 30,                           \
> > +}
>
> so we can drop this one below...
>
Will do.

>
>
> > +             /* We ignore this BSS ID rather than try to continue on.
> > +              * Otherwise we will cause an OOPs because our frequency is 0.
> > +              * The main case this occurs is some new frequency band
> > +              * we have not seen before, and if we return an error,
> > +              * we will cause the scan to fail.  It seems better to
> > +              * report the error, skip this BSS, and move on.
> > +              */
> > +             return 0;
> > +     }
> >       bss_data.chan = ieee80211_get_channel(wiphy, freq);
>
> How could this fail? Our wiphy registers all possible channels so if
> ieee80211_channel_to_frequency() succeeds ieee80211_get_channel() can
> not fail.

I agree.
>
>
> > @@ -6965,6 +7066,10 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
> >               for (i = 0; i < band->n_channels; i++)
> >                       band->channels[i].flags = IEEE80211_CHAN_DISABLED;
> >       band = wiphy->bands[NL80211_BAND_5GHZ];
> > +     if (band)
>
> Eh. Why is this conditional? We are creating all bands in the wiphy
> instance so why the null check here?


I just matched what was there, I can remove all of them if we want.

(I'll take care of the rest of the comments between here)

>
> > -     brcmf_dbg(INFO, "nmode=%d, vhtmode=%d, bw_cap=(%d, %d)\n",
> > +     brcmf_dbg(INFO,
> > +               "nmode=%d, vhtmode=%d, bw_cap=(%d, %d, %d), he_cap=(%d, %d)\n",
> >                 nmode, vhtmode, bw_cap[NL80211_BAND_2GHZ],
> > -               bw_cap[NL80211_BAND_5GHZ]);
> > +               bw_cap[NL80211_BAND_5GHZ], bw_cap[NL80211_BAND_6GHZ],
> > +               he_cap[0], he_cap[1]);
>
> So are these he mac and phy capabilities? ...

No, unfortunately, it's either 1 or 0 on these chips, and all chips i tested.
This is the hardware capability iovar.

In the debug firmware i have access to (not apple's), i do see a
command that looks like it may give the he cap, but i can't find how
it would ever be triggered.
(The iovar code for the iovar above is either always just return 0 or return 1)
There are no obvious iovars that relate, and  the absolute latest
bcmdhd hardcodes the he caps, as do infineon's latest ifx code.
:(
I'l hack around see if i can get the caps out of it.

I'll double check other ones.

> > @@ -8331,18 +8589,21 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
> >       if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_DUMP_OBSS))
> >               ops->dump_survey = brcmf_cfg80211_dump_survey;
> >
> > -     err = wiphy_register(wiphy);
> > -     if (err < 0) {
> > -             bphy_err(drvr, "Could not register wiphy device (%d)\n", err);
> > -             goto priv_out;
> > -     }
> > -
> > +     /* We have to configure the bands before we register the wiphy device
> > +      * because it requires that band capabilities be correct.
> > +      */
>
> Is it?

If you register the 6g band without he_cap set, 80211 is unhappy.
It sanity checks the bands in wiphy_register, and we get caught in the
HE supported check for 6g.

See here:
https://elixir.bootlin.com/linux/latest/source/net/wireless/core.c#L823

In general, you can see it sanity checks the bands/etc as part of
registration.  It happens that 6g triggers the he one, but it seems in
general the bands are supposed to be sane before registration.


> The order was deliberate. brcmf_setup_wiphybands() calls
> brcmf_construct_chaninfo() which disables all channels. When you do that
> before wiphy_register() the orig_flags of the channel will be DISABLED
> and can never be used.

I'll take care of this by copying the flags around.

>
> >       err = brcmf_setup_wiphybands(cfg);
> >       if (err) {
> >               bphy_err(drvr, "Setting wiphy bands failed (%d)\n", err);
> >               goto wiphy_unreg_out;
> >       }
> >
> > +     err = wiphy_register(wiphy);
> > +     if (err < 0) {
> > +             bphy_err(drvr, "Could not register wiphy device (%d)\n", err);
> > +             goto priv_out;
> > +     }
> > +
> >       /* If cfg80211 didn't disable 40MHz HT CAP in wiphy_register(),
> >        * setup 40MHz in 2GHz band and enable OBSS scanning.
> >        */
>