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

[Karthikeyan Kathirvel <karthikeyan.kathirvel@oss.qualcomm.com>]

On 2/12/2026 11:04 PM, Johannes Berg wrote:
> 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 ..."?
> 

I will use "See Figure" and "Refer to IEEE80211_UHR_DPS*"

>> +#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.
> 

Sure will remove those

>> + * subclause 9.4.1.87. Refer Figure 9-207u.
>> + *
>> + * Please refer IEEE80211_UHR_DPS*
> 
> Also here as well.
> 

Will take care

>> +#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
>

Will take care

>> +#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?
> 

Oops!!! Agree, will remove it

>> +#define IEEE80211_UHR_P_EDCA_RESERVED		0x7800
>> +/**
> 
> reserved again, don't think it makes sense - and probably should have a
> blank line before the /**
> 

Sure, will modify and remove it

>>   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.
> 
Will initialize during declaration in next version
> johannes

/KK