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

[Arend van Spriel <arend.vanspriel@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

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@marcan.st>
>>>
>>> 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@marcan.st>
>>> ---
>>>    .../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.

Regards,
Arend

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]