Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3db08cbd-53b5-1a55-76c0-9b353efe52d5@quicinc.com>
Date:   Mon, 6 Jun 2022 23:14:20 +0530
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] cfg80211: Indicate MLO connection info in connect and roam
 callbacks
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>
CC:     <linux-wireless@vger.kernel.org>
References: <1653528391-21718-1-git-send-email-quic_vjakkam@quicinc.com>
 <9270689aeb7de1c2f9764d8e79f28c2b9aafcedc.camel@sipsolutions.net>
From:   Veerendranath Jakkam <quic_vjakkam@quicinc.com>
In-Reply-To: <9270689aeb7de1c2f9764d8e79f28c2b9aafcedc.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Thanks for reviewing the patch.

On 5/27/2022 1:26 PM, Johannes Berg wrote:

>
>> +			for_each_valid_link(cr, link)
>> +				cfg80211_put_bss(wdev->wiphy, cr->links[link].bss);


reduced the repeated code by using "out" label.  Can't make common 
utility function

for both roam and connect callbacks since variables in links struct 
differs for connect result and roam info.

Also, addressed several other comments about code cleanup and nits in v2.


>
>> -	if (WARN_ON(!info->bss))
>> +	if (WARN_ON(!bss_found))
>>   		return;
> This goes with my question earlier - here you're basically assuming
> finding a single BSS is fine. Do we really think so? It used to be we
> wanted all of them, and I kind of tend to think drivers should just make
> sure they have all of them - once we have the entry it can be updated,
> but if we don't have one, we'll never again get the information on the
> BSS, for purposes such as "iw link" output.
>
> I tend to think we should always require all BSS entries to exist, even
> if the driver initially has to make up a fake entry with pretty much no
> information.
>
> Or maybe we have enough information here already (BSSID/frequency) to
> make up a fake entry in cfg80211?
>

Agree, Drivers should have enough formation to create a BSS entry for 
all the links.

As suggested added checks for all bss entries availability in v2 patch.

>> +	if (info->ap_mld_addr) {
>> +		ev->rm.ap_mld_addr = next;
>> +		memcpy((void *)ev->rm.ap_mld_addr, info->ap_mld_addr,
>> +		       ETH_ALEN);
> Why the cast?

Without casting it gives below error.

note: expected 'void *' but argument is of type 'const u8 *' {aka 'const unsigned char *'}

--
veeru