Received-SPF: softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <25ed0281-0880-1e8f-154c-53ee056e8879@quicinc.com>
Date:   Tue, 7 Jun 2022 17:24:17 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [RFC v2] cfg80211: Indicate MLO connection info in connect and
 roam callbacks
Content-Language: en-US
To:     Veerendranath Jakkam <quic_vjakkam@quicinc.com>,
        <johannes@sipsolutions.net>
CC:     <linux-wireless@vger.kernel.org>
References: <1654536690-12588-1-git-send-email-quic_vjakkam@quicinc.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <1654536690-12588-1-git-send-email-quic_vjakkam@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/6/2022 10:31 AM, Veerendranath Jakkam wrote:
> The MLO links used for connection with an MLD AP are decided by the
> driver in case of SME offloaded to driver.
> 
> Add support for the drivers to indicate the information of links used
> for MLO connection in connect and roam callbacks, update the connected
> links information in wdev from connect/roam result sent by driver.
> Also, send the connected links information to userspace.
> 
> Add a netlink flag attribute to indicate that userspace supports
> handling of MLO connection. Drivers must not do MLO connection when this
> flag is not set. This is to maintain backwards compatibility with older
> supplicant versions which doesn't have support for MLO connection.
> 
> Signed-off-by: Veerendranath Jakkam <quic_vjakkam@quicinc.com>
> ---
[snip]
I had one bikeshed observation...

> @@ -17176,6 +17199,38 @@ void nl80211_send_connect_result(struct cfg80211_registered_device *rdev,
>   	       nla_put(msg, NL80211_ATTR_PMKID, WLAN_PMKID_LEN, cr->fils.pmkid)))))
>   		goto nla_put_failure;
>   
> +	if (cr->valid_links) {
> +		int i = 0;

perhaps = 1? see below

> +		struct nlattr *nested;
> +
> +		nested = nla_nest_start(msg, NL80211_ATTR_MLO_LINKS);
> +		if (!nested)
> +			goto nla_put_failure;
> +
> +		for_each_valid_link(cr, link) {
> +			struct nlattr *nested_mlo_links;
> +			const u8 *bssid = cr->links[link].bss ?
> +					  cr->links[link].bss->bssid :
> +					  cr->links[link].bssid;
> +
> +			nested_mlo_links = nla_nest_start(msg, i + 1);

why i+1?
if you don't want to use 0 for the first entry (why not?) then init i to 1

alternately since this is the only place i is used, use ++i or i++ as 
you see fit and remove the i++ later

ultimately we should only need to calculate i+1 once

these comments apply to nl80211_send_roamed() as well

(BTW I do see this pattern in some of the existing code, but I also see 
0 being used for the first record)