[PATCH] brcmfmac: support STA info struct v7

[PATCH] brcmfmac: support STA info struct v7

[Dan Haab]

The newest firmwares provide STA info using v7 of the struct. As v7
isn't backward compatible, a union is needed.

Even though brcmfmac does not use any of the new info it's important to
provide the proper struct buffer. Without this change new firmwares will
fallback to the very limited v3 instead of something in between such as
v4.

Signed-off-by: Dan Haab <dan.haab@luxul.com>
---
 .../broadcom/brcm80211/brcmfmac/fwil_types.h       | 39 ++++++++++++++++++----
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
index d5bb81e..189d576 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
@@ -176,6 +176,8 @@
 
 #define BRCMF_VHT_CAP_MCS_MAP_NSS_MAX	8
 
+#define BRCMF_HE_CAP_MCS_MAP_NSS_MAX	8
+
 /* MAX_CHUNK_LEN is the maximum length for data passing to firmware in each
  * ioctl. It is relatively small because firmware has small maximum size input
  * playload restriction for ioctls.
@@ -601,13 +603,36 @@ struct brcmf_sta_info_le {
 	__le32 rx_pkts_retried;        /* # rx with retry bit set */
 	__le32 tx_rate_fallback;       /* lowest fallback TX rate */
 
-	/* Fields valid for ver >= 5 */
-	struct {
-		__le32 count;					/* # rates in this set */
-		u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
-		u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
-		__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
-	} rateset_adv;
+	union {
+		struct {
+			struct {
+				__le32 count;					/* # rates in this set */
+				u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
+				u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
+				__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
+			} rateset_adv;
+		} v5;
+
+		struct {
+			__le32 rx_dur_total;	/* total user RX duration (estimated) */
+			__le16 chanspec;	/** chanspec this sta is on */
+			__le16 pad;
+			struct {
+				__le16 version;					/* version */
+				__le16 len;					/* length */
+				__le32 count;					/* # rates in this set */
+				u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
+				u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
+				__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
+				__le16 he_mcs[BRCMF_HE_CAP_MCS_MAP_NSS_MAX];	/* supported he mcs index bit map per nss */
+			} rateset_adv;		/* rateset along with mcs index bitmap */
+			__le16 wpauth;		/* authentication type */
+			u8 algo;		/* crypto algorithm */
+			__le32 tx_rspec;	/* Rate of last successful tx frame */
+			__le32 rx_rspec;	/* Rate of last successful rx frame */
+			__le32 wnm_cap;		/* wnm capabilities */
+		} v7;
+	};
 };
 
 struct brcmf_chanspec_list {
-- 
1.9.1

Re: [PATCH] brcmfmac: support STA info struct v7

[Arend van Spriel]

On 10/11/2018 10:19 PM, Dan Haab wrote:
> The newest firmwares provide STA info using v7 of the struct. As v7
> isn't backward compatible, a union is needed.
>
> Even though brcmfmac does not use any of the new info it's important to
> provide the proper struct buffer. Without this change new firmwares will
> fallback to the very limited v3 instead of something in between such as
> v4.

Hi Dan,

I don't have a real problem with adding this v7 stuff, but it looks 
different from what I have here which has additional fields. For what 
new firmwares and more importantly what devices do you need this. I only 
know it is used for 43684, which we do not support with brcmfmac (yet).

Regards,
Arend

> Signed-off-by: Dan Haab <dan.haab@luxul.com>
> ---
>  .../broadcom/brcm80211/brcmfmac/fwil_types.h       | 39 ++++++++++++++++++----
>  1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> index d5bb81e..189d576 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> @@ -176,6 +176,8 @@
>
>  #define BRCMF_VHT_CAP_MCS_MAP_NSS_MAX	8
>
> +#define BRCMF_HE_CAP_MCS_MAP_NSS_MAX	8
> +
>  /* MAX_CHUNK_LEN is the maximum length for data passing to firmware in each
>   * ioctl. It is relatively small because firmware has small maximum size input
>   * playload restriction for ioctls.
> @@ -601,13 +603,36 @@ struct brcmf_sta_info_le {
>  	__le32 rx_pkts_retried;        /* # rx with retry bit set */
>  	__le32 tx_rate_fallback;       /* lowest fallback TX rate */
>
> -	/* Fields valid for ver >= 5 */
> -	struct {
> -		__le32 count;					/* # rates in this set */
> -		u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
> -		u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
> -		__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
> -	} rateset_adv;
> +	union {
> +		struct {
> +			struct {
> +				__le32 count;					/* # rates in this set */
> +				u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
> +				u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
> +				__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
> +			} rateset_adv;
> +		} v5;
> +
> +		struct {
> +			__le32 rx_dur_total;	/* total user RX duration (estimated) */
> +			__le16 chanspec;	/** chanspec this sta is on */
> +			__le16 pad;
> +			struct {
> +				__le16 version;					/* version */
> +				__le16 len;					/* length */
> +				__le32 count;					/* # rates in this set */
> +				u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
> +				u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
> +				__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
> +				__le16 he_mcs[BRCMF_HE_CAP_MCS_MAP_NSS_MAX];	/* supported he mcs index bit map per nss */
> +			} rateset_adv;		/* rateset along with mcs index bitmap */
> +			__le16 wpauth;		/* authentication type */
> +			u8 algo;		/* crypto algorithm */
> +			__le32 tx_rspec;	/* Rate of last successful tx frame */
> +			__le32 rx_rspec;	/* Rate of last successful rx frame */
> +			__le32 wnm_cap;		/* wnm capabilities */
> +		} v7;
> +	};
>  };
>
>  struct brcmf_chanspec_list {
>

Re: [PATCH] brcmfmac: support STA info struct v7

[Rip Route]

On Fri, Oct 12, 2018 at 3:02 AM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 10/11/2018 10:19 PM, Dan Haab wrote:
> > The newest firmwares provide STA info using v7 of the struct. As v7
> > isn't backward compatible, a union is needed.
> >
> > Even though brcmfmac does not use any of the new info it's important to
> > provide the proper struct buffer. Without this change new firmwares will
> > fallback to the very limited v3 instead of something in between such as
> > v4.
>
> Hi Dan,
>
> I don't have a real problem with adding this v7 stuff, but it looks
> different from what I have here which has additional fields. For what
> new firmwares and more importantly what devices do you need this. I only
> know it is used for 43684, which we do not support with brcmfmac (yet).

Arend,

This is to be used with firmwares we generated from the SDK recommended
to me by by Broadcom, 7.14.170.34. These firmwares are used on these
Luxul devices: XWR-3150v2 and XAP-1610.

Cheers,
Dan

Re: [PATCH] brcmfmac: support STA info struct v7

[Rafał Miłecki]

On Thu, 11 Oct 2018 at 22:21, Dan Haab <riproute@gmail.com> wrote:
> The newest firmwares provide STA info using v7 of the struct. As v7
> isn't backward compatible, a union is needed.
>
> Even though brcmfmac does not use any of the new info it's important to
> provide the proper struct buffer. Without this change new firmwares will
> fallback to the very limited v3 instead of something in between such as
> v4.
>
> Signed-off-by: Dan Haab <dan.haab@luxul.com>

It's too bad Broadcom's existing struct has been changed instead of
just being extended.

The patch looks good to me though. I just wanted to share my opinion /
ping due to patch being marked as "Deferred".

Reviewed-by: Rafał Miłecki <rafal@milecki.pl>

Re: [PATCH] brcmfmac: support STA info struct v7

[Kalle Valo]

Rafał Miłecki <zajec5@gmail.com> writes:

> On Thu, 11 Oct 2018 at 22:21, Dan Haab <riproute@gmail.com> wrote:
>> The newest firmwares provide STA info using v7 of the struct. As v7
>> isn't backward compatible, a union is needed.
>>
>> Even though brcmfmac does not use any of the new info it's important to
>> provide the proper struct buffer. Without this change new firmwares will
>> fallback to the very limited v3 instead of something in between such as
>> v4.
>>
>> Signed-off-by: Dan Haab <dan.haab@luxul.com>
>
> It's too bad Broadcom's existing struct has been changed instead of
> just being extended.
>
> The patch looks good to me though. I just wanted to share my opinion /
> ping due to patch being marked as "Deferred".
>
> Reviewed-by: Rafał Miłecki <rafal@milecki.pl>

Good that you brought this up, I wasn't sure what to do with it so I
marked as Deferred. Arend, please let me know what I should do.

-- 
Kalle Valo

Re: [PATCH] brcmfmac: support STA info struct v7

[Arend van Spriel]

On 11/7/2018 11:02 AM, Kalle Valo wrote:
> Rafał Miłecki <zajec5@gmail.com> writes:
>
>> On Thu, 11 Oct 2018 at 22:21, Dan Haab <riproute@gmail.com> wrote:
>>> The newest firmwares provide STA info using v7 of the struct. As v7
>>> isn't backward compatible, a union is needed.
>>>
>>> Even though brcmfmac does not use any of the new info it's important to
>>> provide the proper struct buffer. Without this change new firmwares will
>>> fallback to the very limited v3 instead of something in between such as
>>> v4.
>>>
>>> Signed-off-by: Dan Haab <dan.haab@luxul.com>
>>
>> It's too bad Broadcom's existing struct has been changed instead of
>> just being extended.
>>
>> The patch looks good to me though. I just wanted to share my opinion /
>> ping due to patch being marked as "Deferred".
>>
>> Reviewed-by: Rafał Miłecki <rafal@milecki.pl>
>
> Good that you brought this up, I wasn't sure what to do with it so I
> marked as Deferred. Arend, please let me know what I should do.

Currently there is no issue as there is currently no image in 
linux-firmware relying on v7 structure. However, it is not unlikely that 
people are using firmware from other sources. As said earlier I am fine 
with this change although v7 structure already was extended. I have a 
small remark on the patch, which I will send out later.

Regards,
Arend

Re: [PATCH] brcmfmac: support STA info struct v7

[Arend van Spriel]

On 10/11/2018 10:19 PM, Dan Haab wrote:
> The newest firmwares provide STA info using v7 of the struct. As v7
> isn't backward compatible, a union is needed.
>
> Even though brcmfmac does not use any of the new info it's important to
> provide the proper struct buffer. Without this change new firmwares will
> fallback to the very limited v3 instead of something in between such as
> v4.
>
> Signed-off-by: Dan Haab <dan.haab@luxul.com>
> ---
>  .../broadcom/brcm80211/brcmfmac/fwil_types.h       | 39 ++++++++++++++++++----
>  1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> index d5bb81e..189d576 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> @@ -176,6 +176,8 @@
>
>  #define BRCMF_VHT_CAP_MCS_MAP_NSS_MAX	8
>
> +#define BRCMF_HE_CAP_MCS_MAP_NSS_MAX	8
> +
>  /* MAX_CHUNK_LEN is the maximum length for data passing to firmware in each
>   * ioctl. It is relatively small because firmware has small maximum size input
>   * playload restriction for ioctls.
> @@ -601,13 +603,36 @@ struct brcmf_sta_info_le {
>  	__le32 rx_pkts_retried;        /* # rx with retry bit set */
>  	__le32 tx_rate_fallback;       /* lowest fallback TX rate */
>
> -	/* Fields valid for ver >= 5 */
> -	struct {
> -		__le32 count;					/* # rates in this set */
> -		u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
> -		u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
> -		__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
> -	} rateset_adv;
> +	union {
> +		struct {
> +			struct {
> +				__le32 count;					/* # rates in this set */
> +				u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
> +				u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
> +				__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
> +			} rateset_adv;
> +		} v5;
> +
> +		struct {
> +			__le32 rx_dur_total;	/* total user RX duration (estimated) */
> +			__le16 chanspec;	/** chanspec this sta is on */
> +			__le16 pad;

Here we add explicity padding field.

> +			struct {
> +				__le16 version;					/* version */
> +				__le16 len;					/* length */
> +				__le32 count;					/* # rates in this set */
> +				u8 rates[BRCMF_MAXRATES_IN_SET];		/* rates in 500kbps units w/hi bit set if basic */
> +				u8 mcs[BRCMF_MCSSET_LEN];			/* supported mcs index bit map */
> +				__le16 vht_mcs[BRCMF_VHT_CAP_MCS_MAP_NSS_MAX];	/* supported mcs index bit map per nss */
> +				__le16 he_mcs[BRCMF_HE_CAP_MCS_MAP_NSS_MAX];	/* supported he mcs index bit map per nss */
> +			} rateset_adv;		/* rateset along with mcs index bitmap */
> +			__le16 wpauth;		/* authentication type */
> +			u8 algo;		/* crypto algorithm */

So I prefer to have padding explicit here as well, ie. a u8 field.

> +			__le32 tx_rspec;	/* Rate of last successful tx frame */
> +			__le32 rx_rspec;	/* Rate of last successful rx frame */
> +			__le32 wnm_cap;		/* wnm capabilities */
> +		} v7;
> +	};
>  };

Regards,
Arend