Re: [PATCH wireless-next v2 1/2] wifi: UHR: define DPS/DBE/P-EDCA elements and fix size parsing

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

Nice, thanks for sending this.

> + * subclause 9.4.1.87. Refer Figure 9-207u.

> + * Please refer IEEE80211_UHR_DPS*

"refer to"? Or "See Figure ..."?

> +#define IEEE80211_UHR_DPS_PADDING_DELAY			0x0000003F
> +#define IEEE80211_UHR_DPS_RESERVED1			0x000000C0
> +#define IEEE80211_UHR_DPS_TRANSITION_DELAY		0x00003F00
> +#define IEEE80211_UHR_DPS_RESERVED2			0x0000C000
> +#define IEEE80211_UHR_DPS_ICF_REQUIRED			0x00010000
> +#define IEEE80211_UHR_DPS_PARAMETERIZED_FLAG		0x00020000
> +#define IEEE80211_UHR_DPS_LC_MODE_BW			0x001C0000
> +#define IEEE80211_UHR_DPS_LC_MODE_NSS			0x01E00000
> +#define IEEE80211_UHR_DPS_LC_MODE_MCS			0x1E000000
> +#define IEEE80211_UHR_DPS_MOBILE_AP_DPS_STATIC_HCM	0x20000000
> +#define IEEE80211_UHR_DPS_RESERVED3			0xC0000000

Not sure I'd defined the ..._RESERVED* ones at all? There's no way we'd
ever use them really unless they get defined to something else.

> + * subclause 9.4.1.87. Refer Figure 9-207u.
> + *
> + * Please refer IEEE80211_UHR_DPS*

Also here as well.

> +#define IEEE80211_UHR_DBE_OPER_BANDWIDTH			0x07
> +#define IEEE80211_UHR_DBE_OPER_DIS_SUBCHANNEL_BITMAP_PRES	0x08
> +#define IEEE80211_UHR_DBE_OPER_RESERVED				0xF0

same comment about reserved

> +#define IEEE80211_UHR_DBE_OPER_DIS_SUBCHANNEL_BITMAP		0xFFFF

And this ... I think should just not be there? When would we ever mask a
16 bit value with an all 16 bits mask?

> +#define IEEE80211_UHR_P_EDCA_RESERVED		0x7800
> +/**

reserved again, don't think it makes sense - and probably should have a
blank line before the /**

>  static inline const struct ieee80211_uhr_npca_info *
>  ieee80211_uhr_npca_info(const struct ieee80211_uhr_operation *oper)
>  {
> +	const u8 *pos;
> +
>  	if (!(oper->params & cpu_to_le16(IEEE80211_UHR_OPER_PARAMS_NPCA_ENA)))
>  		return NULL;
>  
> -	/* FIXME: DPS */
> +	pos = oper->variable;

I'd probably have directly put 'const u8 *pos = oper->variable;', but
doesn't really matter.

johannes