Received-SPF: pass (google.com: domain of linux-wireless+bounces-7296-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <45e98a80-9a32-fe1a-2780-ae704b95082a@quicinc.com>
Date: Tue, 7 May 2024 22:55:50 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [RFC v2 1/2] wifi: nl80211: Add attribute to send critical update
 parameters
References: <20240403162225.3096228-1-quic_rrchinan@quicinc.com>
 <20240403162225.3096228-2-quic_rrchinan@quicinc.com>
 <292247f4b838356b0c4e3ded2ff9fdbabd682040.camel@sipsolutions.net>
Content-Language: en-US
From: Rathees Kumar R Chinannan <quic_rrchinan@quicinc.com>
To: Johannes Berg <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>
In-Reply-To: <292247f4b838356b0c4e3ded2ff9fdbabd682040.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 5/3/2024 2:42 PM, Johannes Berg wrote:
> On Wed, 2024-04-03 at 21:52 +0530, Rathees Kumar R Chinannan wrote:
>> Add NL80211_ATTR_RXMGMT_CRITICAL_UPDATE attribute to send critical
>> update parameters to hostapd on NL80211_CMD_FRAME.
>>
>> User space application requires these CU params to update fields on probe
>> and assoc response frame. So, during probe or assoc request frame receive,
>> send these params as a new attribute on existing NL80211_CMD_FRAME for
>> AP MLD.
>>
>> Change in CU parameters should be sent to user space either before or
>> along with probe or assoc request frame receive to ensure that user space
>> uses latest CU values and BPCC while generating response to the received
>> frames. So, add the critical update parameters as a new attribute to
>> existing NL80211_CMD_FRAME command instead of sending this on a separate
>> NL80211 event.
>>
>> Based on the suggestion received on below link, add extended feature
>> NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD flag if driver handles
>> synchronization among all the links and update critical information on
>> partner link beacon for AP MLD and user space can update critical
>> information only on impacted link beacon template. Add this critical
>> update attribute on NL80211_CMD_FRAME only when this flag is set
>> by driver.
>>
>> Link: https://lore.kernel.org/all/df711a5978b84856a54953a32e4b05923b48870a.camel@sipsolutions.net/
> 
> I'm not sure we're interpreting this the same way ;-)
> 
> At least now that I'm reading it again, I was thinking in that thread
> about how beacons on partner links are updated. That whole thread was
> about handling that?
>
yes, it is about beacon update on partner links.

>> Suggested-by: Johannes Berg <johannes@sipsolutions.net>
>> Signed-off-by: Rathees Kumar R Chinannan <quic_rrchinan@quicinc.com>
>> ---
>>   include/net/cfg80211.h       |  10 +++
>>   include/uapi/linux/nl80211.h | 104 +++++++++++++++++++++++++++++
>>   net/wireless/nl80211.c       | 123 ++++++++++++++++++++++++++++++++++-
>>   3 files changed, 236 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 2e2be4fd2bb6..496a9d4956de 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -6142,7 +6142,11 @@ void wiphy_delayed_work_flush(struct wiphy *wiphy,
>>    *	unprotected beacon report
>>    * @links: array of %IEEE80211_MLD_MAX_NUM_LINKS elements containing @addr
>>    *	@ap and @client for each link
>> + * @links.ap.bpcc: Bss param change count value for each link
> 
> BSS
> 
Sure, will correct this.

>> + * @links.ap.switch_count: CSA/BCCA count for each link
> 
> Had to think about BCCA a bit too much ... maybe explain it? :)
> 
> Also can someone actually be trying to do channel switch and color
> change at the same time?
>
Currently kernel doesn't allow another color change if one is already 
active or if CSA is active. Similarly, during channel switch if there is 
a color change in progress, it will get aborted.

>> + * @links.ap.critical_flag: Critical update flag for each link
>>    * @valid_links: bitmap describing what elements of @links are valid
>> + * @critical_update: critical params updated on any wdev link
> 
> Why have two critical indications? And they're even named differently?
> I'd probably rename the inner to 'critical_update' and remove the outer,
> it's easy to loop over (valid) links?
> 
Sure, will remove the second one and iterate valid links for the same.

>>   static inline const u8 *wdev_address(struct wireless_dev *wdev)
>> @@ -8340,6 +8348,7 @@ void cfg80211_conn_failed(struct net_device *dev, const u8 *mac_addr,
>>    * @flags: flags, as defined in &enum nl80211_rxmgmt_flags
>>    * @rx_tstamp: Hardware timestamp of frame RX in nanoseconds
>>    * @ack_tstamp: Hardware timestamp of ack TX in nanoseconds
>> + * @critical_update: critical params updated for the received frame
>>    */
>>   struct cfg80211_rx_info {
>>   	int freq;
>> @@ -8351,6 +8360,7 @@ struct cfg80211_rx_info {
>>   	u32 flags;
>>   	u64 rx_tstamp;
>>   	u64 ack_tstamp;
>> +	bool critical_update;
> 
> Not sure what this means?
> 
This flag will be set on mac80211 when critical parameters are received 
on driver along with probe/assoc request frame receive.
>> + * @NL80211_ATTR_RXMGMT_CRITICAL_UPDATE: This is a nested attribute for driver
>> + *	supporting critical update feature for AP MLD. When used with
>> + *	%NL80211_CMD_FRAME it contains attribute defined in &enum nl80211_cu_attrs
>> + *	to send critical update params for list of MLDs. Driver adds this attribute
>> + *	only for probe, assoc and reassoc request frame. User-space can use these
>> + *	params to update CU fields on corresponding response frame. This attribute
>> + *	is needed only on beacon offload case and it is not needed on beacon
>> + *	non-offload case since user space itself has these data.
> 
> The wording of "beacon offload" vs. "beacon non-offload" seems, at best,
> imprecise? This should be about offloading partner link updates, or
> something like that?
> 
Sure, will update this.

> Also I find this API a bit confusing, why is this sent in an RX frame
> notification, which is stateless today, but you only send it once on the
> first RX notification after it was set. Is that some kind of
> optimisation?
>
Yes, it is sent not only on first RX notification, it is sent later as 
well if there is a change in the value received from driver when 
compared to previous values.

> I'd think it would make more sense to send a separate notification of
> the data, and then you don't even need to store the data in the wdev.
> 
> Maybe then if we find out that it updates too frequently, we can defer
> that separate notification (e.g. create the SKB for it, store a pointer
> to that, and send it only on the first frame RX).
> 
CU parameters should be sent to user space either before or
along with probe or assoc request frame receive to ensure that user 
space uses latest CU values and BPCC while generating response to the 
received frames.
Do you recommend to send it as separate NL80211 command before handling 
these management frame RX ? Please suggest.

>> @@ -3401,6 +3410,7 @@ enum nl80211_attrs {
>>   
>>   	NL80211_ATTR_ASSOC_SPP_AMSDU,
>>   
>> +	NL80211_ATTR_RXMGMT_CRITICAL_UPDATE,
>>   	/* add attributes here, update the policy in nl80211.c */
> 
> nit: blank line
>
Sure, will correct this.

>> + * @NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD: User space sets critical
>> + *	information only on impacted link through @NL80211_CMD_SET_BEACON.
>> + *	Driver/Device handles synchronization among all the links and update
>> + *	critical information on partner link beacon in case of MLD.
> 
> That's not really _quite_ what this implies today.
> 
> I mean, yeah, that's the idea, but you're actually using it for
> something else. I don't even mind, but maybe it'd make sense to put this
> into a separate patch, that starts building out the infrastructure for
> handling all of this, simply with this for starters to say "not needed
> in this case"?
> 
> (I think maybe I'm missing the broader picture a bit in these patches)
> 
ok, will raise a separate patch for this feature flag.
And patch to add critical update information on impacted link during set 
beacon template is still under review.

>> +/*
>> + * Critical update attribute length for a MLD list with two nested
>> + * attributes. Each nla_nest_start() reserves four bytes.
>> + */
>> +#define NL80211_CU_ATTR_MLDS_LEN	8
> ...
>> +#define NL80211_CU_ATTR_MLD_LEN		16
> ...
>> +#define NL80211_CU_ATTR_LINK_LEN	32
> 
> Please don't do that, use nla_attr_size() or something?
> 
> But maybe anyway we can find a better representation.
> 
Sure, will check this.

>> +/**
>> + * nl80211_cu_attrs - critical update attributes
>> + *
>> + *
> 
> nit: double blank line
> 
Ok

>> + * @__NL80211_CU_ATTR_INVALID: invalid
>> + * @NL80211_CU_ATTR_MLDS: nested attribute specifying list of MLDs,
>> + * see &enum nl80211_cu_mld_attrs
>> + * @__NL80211_CU_ATTR_LAST: internal use
>> + * @NL80211_CU_ATTR_MAX: maximum critical update attribute
>> + */
>> +enum nl80211_cu_attrs {
>> +	__NL80211_CU_ATTR_INVALID,
>> +
>> +	NL80211_CU_ATTR_MLDS,
>> +
>> +	/* keep last */
>> +	__NL80211_CU_ATTR_LAST,
>> +	NL80211_CU_ATTR_MAX = __NL80211_CU_ATTR_LAST - 1
>> +};
> 
> But I'm not sure why you need this level anyway?
> 
> Why would you need to represent multiple MLDs for a single interface?
> You've tagged it to RX (see above for comments on that) but that's per
> interface anyway, so certainly per MLD?
>
If multiple vaps are created on a radio, then on MBSSID IE for each Non 
TX BSSID profile, we have to update CU bits & BPCC on Multi link 
element. These non-TX vaps can be part of different MLDs, so we need to 
pass the critical information for all MLDs instead of single MLD.

>> +++ b/net/wireless/nl80211.c
>> @@ -468,6 +468,25 @@ static const struct netlink_range_validation nl80211_punct_bitmap_range = {
>>   	.max = 0xffff,
>>   };
>>   
>> +static const struct nla_policy
>> +link_policy[NL80211_CU_MLD_LINK_ATTR_MAX + 1] = {
>> +	[NL80211_CU_MLD_LINK_ATTR_ID] = { .type = NLA_U8 },
>> +	[NL80211_CU_MLD_LINK_ATTR_CRITICAL_FLAG] = { .type = NLA_FLAG },
>> +	[NL80211_CU_MLD_LINK_ATTR_BPCC] = { .type = NLA_U8 },
>> +	[NL80211_CU_MLD_LINK_ATTR_SWITCH_COUNT] = { .type = NLA_U8 },
>> +};
>> +
>> +static const struct nla_policy
>> +mld_policy[NL80211_CU_MLD_ATTR_MAX + 1] = {
>> +	[NL80211_CU_MLD_ATTR_IFINDEX] = { .type = NLA_U32 },
>> +	[NL80211_CU_MLD_ATTR_LINKS] = NLA_POLICY_NESTED(link_policy),
>> +};
>> +
>> +static const struct nla_policy
>> +cu_policy[NL80211_CU_ATTR_MAX + 1] = {
>> +	[NL80211_CU_ATTR_MLDS] = NLA_POLICY_NESTED(mld_policy),
>> +};
>> +
>>   static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
>>   	[0] = { .strict_start_type = NL80211_ATTR_HE_OBSS_PD },
>>   	[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
>> @@ -826,6 +845,7 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
>>   	[NL80211_ATTR_MLO_TTLM_DLINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8),
>>   	[NL80211_ATTR_MLO_TTLM_ULINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8),
>>   	[NL80211_ATTR_ASSOC_SPP_AMSDU] = { .type = NLA_FLAG },
>> +	[NL80211_ATTR_RXMGMT_CRITICAL_UPDATE] = NLA_POLICY_NESTED(cu_policy),
> 
> All of these are outgoing though, so probably shouldn't even have a
> policy (which implies NLA_REJECT, i.e. not used incoming)?
> 
Sure, will remove this change.

>> -	msg = nlmsg_new(100 + info->len, gfp);
>> +	if (wiphy_ext_feature_isset(
>> +		    wdev->wiphy,
>> +		    NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD) &&
>> +	    info->critical_update)
> 
> I don't even see how the flag check is needed here, since it relies on
> the driver setting info->critical_update?
> 
> johannes

Sure, will remove this flag check.