Re: [PATCH 4/5] wifi: brcmfmac: Support bss_info up to v112

[Daniel Berlin]

On Tue, Nov 7, 2023 at 6:51 AM Arend van Spriel
<arend.vanspriel@xxxxxxxxxxxx> wrote:
>
> On 11/7/2023 12:11 PM, Hector Martin wrote:
> > On 20/10/2023 18.59, Arend van Spriel wrote:
> >> On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> >>> From: Hector Martin <marcan@xxxxxxxxx>
> >>>
> >>> 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 <marcan@xxxxxxxxx>
> >>> ---
> >>>    .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  5 ++-
> >>>    .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 37 +++++++++++++++++--
> >>>    2 files changed, 36 insertions(+), 6 deletions(-)
> >>>
>
> [...]
>
> >>> @@ -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.
> >
> > bcmflags was previously an implied padding byte. If you actually check
> > the offsets of the subsequent fields, you'll see they haven't changed.
> > In fact this was added at some point in the past and just missing here,
> > and is a general case of "padding bytes were not explicitly specified"
> > which is arguably an anti-pattern and should never have been the case.
>
> Yeah. Let's not argue ;-) I did miss something here and leave it with
> that. What about the EHT stuff? I would prefer to keep it out unless
> full EHT support is added.

The EHT stuff you flagged (defines, etc) in the other patches can be
left out - this one can't since it is definitionally part of the v112
struct.