So, at least in the example code I have, all variants of the 109 and
112 structures both have bcnflags in that place - it was always
missing here.
For example, see
https://android.googlesource.com/kernel/google-modules/wlan/bcmdhd/bcm4398/+/refs/heads/android-gs-shusky-5.15-u-qpr1-beta2/include/wlioctl.h
and compare v109 and v112 of bssinfo.
As such, the 109 and 112 structures are compatible given these definitions.
I don't know if what is there right now is wrong, or it is "yet
another variant" of the 109 structure that we need to handle.
Any idea what the ground truth is?
If we need to separate the structures and support 3 variants (2 109
variants, 1 112 variant), we probably need to start figuring out the
more general structure versioning issue.
We just aren't implementing the bigger ones (for example, there are 5
versions of pfn scanresults, and 6 versions of roam profiles. Most are
still in use, AFAIK)
Any thoughts?
For the bss info here, I suspect I could hide most of this, and use a
structure of function pointers + versioned data that returns the
nn-versioned info that is being asked about (IE the same way we do
with buscore, etc).
IE struct bss_info_data {
void *versioned_data;
nl80211_band (*get_band_for_idx)(struct bss_info_data *, u8 idx);
}
Most of the time we are converting it from this structure to 80211 info, etc.
I haven't looked too hard to see if we are really digging around in
this structure, but I would suspect it could be encapsulated.
This would in turn let us avoid using feature flags to control
structure versions (we would just set them after querying the
firmware), and would make it easier to clean up/refactor code.
I'm open to other ideas as well.
On Fri, Oct 20, 2023 at 5:59 AM Arend van Spriel
<[email protected]> wrote:
>
> On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> > From: Hector Martin <[email protected]>
> >
> > The structures are compatible and just add fields, so we can just treat
> > it as always v112. If we start using new fields, that will have to be
> > gated on the version.
>
> Seems EHT is creeping in here.
>
> Having doubts about compatibility statement (see below)...
>
> > Signed-off-by: Hector Martin <[email protected]>
> > ---
> > .../broadcom/brcm80211/brcmfmac/cfg80211.c | 5 ++-
> > .../broadcom/brcm80211/brcmfmac/fwil_types.h | 37 +++++++++++++++++--
> > 2 files changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > index 4cf728368892..bc8355d7f9b5 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > @@ -3496,8 +3496,9 @@ static s32 brcmf_inform_bss(struct brcmf_cfg80211_info *cfg)
> >
> > bss_list = (struct brcmf_scan_results *)cfg->escan_info.escan_buf;
> > if (bss_list->count != 0 &&
> > - bss_list->version != BRCMF_BSS_INFO_VERSION) {
> > - bphy_err(drvr, "Version %d != WL_BSS_INFO_VERSION\n",
> > + (bss_list->version < BRCMF_BSS_INFO_MIN_VERSION ||
> > + bss_list->version > BRCMF_BSS_INFO_MAX_VERSION)) {
> > + bphy_err(drvr, "BSS info version %d unsupported\n",
> > bss_list->version);
> > return -EOPNOTSUPP;
> > }
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> > index 1077e6f1d61a..81f2d77cb004 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> > @@ -18,7 +18,8 @@
> > #define BRCMF_ARP_OL_HOST_AUTO_REPLY 0x00000004
> > #define BRCMF_ARP_OL_PEER_AUTO_REPLY 0x00000008
> >
> > -#define BRCMF_BSS_INFO_VERSION 109 /* curr ver of brcmf_bss_info_le struct */
> > +#define BRCMF_BSS_INFO_MIN_VERSION 109 /* min ver of brcmf_bss_info_le struct */
> > +#define BRCMF_BSS_INFO_MAX_VERSION 112 /* max ver of brcmf_bss_info_le struct */
> > #define BRCMF_BSS_RSSI_ON_CHANNEL 0x0004
> >
> > #define BRCMF_STA_BRCM 0x00000001 /* Running a Broadcom driver */
> > @@ -323,28 +324,56 @@ struct brcmf_bss_info_le {
> > __le16 capability; /* Capability information */
> > u8 SSID_len;
> > u8 SSID[32];
> > + u8 bcnflags; /* additional flags w.r.t. beacon */
>
> Ehm. Coming back to your statement "structures are compatible and just
> add fields". How are they compatible? You now treat v109 struct as v112
> so fields below are shifted because of bcnflags. So you read invalid
> information. This does not fly or I am missing something here.
>
> > struct {
> > __le32 count; /* # rates in this set */
> > u8 rates[16]; /* rates in 500kbps units w/hi bit set if basic */
> > } rateset; /* supported rates */
> > __le16 chanspec; /* chanspec for bss */
> > __le16 atim_window; /* units are Kusec */
>
> [...]
On Fri, Oct 20, 2023 at 1:31 PM Daniel Berlin <[email protected]> wrote:
>
> So, at least in the example code I have, all variants of the 109 and
> 112 structures both have bcnflags in that place - it was always
> missing here.
> For example, see
> https://android.googlesource.com/kernel/google-modules/wlan/bcmdhd/bcm4398/+/refs/heads/android-gs-shusky-5.15-u-qpr1-beta2/include/wlioctl.h
> and compare v109 and v112 of bssinfo.
>
> As such, the 109 and 112 structures are compatible given these definitions.
>
> I don't know if what is there right now is wrong, or it is "yet
> another variant" of the 109 structure that we need to handle.
> Any idea what the ground truth is?
Circling back to this - i have checked other sources as well -
infineon also has u8 (though it's marked as padding) there, etc.
As far as i can tell, the structure should have that u8 there in all
versions i can find.
Nevertheless, I think I can make it not matter ;)
I'm going to post an RFC of some patches that handle this and other
structure versioning
things through function pointer structures that we set based on
interface versions available. So instead of setting feature flags, we
query the various iovars/firmware
info for the right interface versions, and set up the
structures/function pointers to handle the versions
we will get from the firmware.
This also makes the common code cleaner as they no longer have to deal
with the structure differences - for example, brcmf_cfg80211_connect
now just calls
something like drvr->join_params_handler.get_struct_from_connect to
get an extended join params struct of the right version
and doesn't look inside the result anymore. It's an RFC so we can
argue about the approach and APIs, i just did what seemed
easy/obvious.
I have converted bss info, join params, scan params, and netinfo
versions to use this approach, and all now support all versions of the
structures available.
If we go that route, it wont matter if we overlay bss info structures or not.
As an aside, while doing the function pointer work, i discovered some
other structure bugs (missing fields) that seem to be causing
failures/fallbacks on every chip i tested (no matter who the vendor
was).
So for example, some of the extended join parameter substructs are
missing fields, and every chip i tried (5 variants, representing 2
vendors and 3 join_param versions) all gave
BUFTOSHORT when trying to use the join iovar as a result, and all fell
back to using the SET_SSID method.
With the v0/v1 structure fixed, they all use the join iovar successfully and
never fall back.
--Dan