[PATCH 2/4] nl80211: fix scan request and its related events handling with MLO

[Aditya Kumar Singh]

On 6/12/24 02:44, Jouni Malinen wrote:
> On Mon, Apr 22, 2024 at 04:49:04PM +0530, Aditya Kumar Singh wrote:
>> Currently, whenever a scan is started, it uses drv's first BSS only
>> whether it is AP or STA interface. However with MLO AP related changes,
>> same drv could be used by other BSSes as well which needs scanning. Hence,
>> the current logic will not work since scan needs to be handled on
>> non-first BSS as well.
>>
>> Add changes to move the logic of always using drv's first BSS during scan
>> events to using BSS on which the event arrived.
>>
>> Also, for ML AP operation, even though BSS is same, link BSS also
>> needs to be identified. Hence add a back pointer in BSS struct which would
>> be used to point to the link BSS which requested scan on that BSS.
>> This will help in routing the scan events to appropriate BSS ctx.
> 
> 
>> diff --git a/src/ap/ap_drv_ops.c b/src/ap/ap_drv_ops.c
>> @@ -781,6 +781,12 @@ bool hostapd_drv_nl80211(struct hostapd_data *hapd)
>>   int hostapd_driver_scan(struct hostapd_data *hapd,
>>   			struct wpa_driver_scan_params *params)
>>   {
>> +	params->mlo_link_id = -1;
> 
> Isn't that needed in wpa_supplicant/driver_i.h for wpa_drv_scan() as
> well? This is going in as 0 now which does not feel correct.
> 

oh yes, I'll fix this in next version.

>> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
>> @@ -709,6 +709,14 @@ struct wpa_driver_scan_params {
>>   	s8 link_id;
>>   
>> +	/**
>> +	 * mlo_link_id - Link ID (in case of MLO)
>> +	 *
>> +	 * If this is set to value >= 0, after scan completion, this would be
>> +	 * used to route the event to proper driver private data.
>> +	 */
>> +	u8 mlo_link_id;
> 
> That should be s8 for that >= 0 to make sense.

Sure. Thanks for pointing that out.

> 
>> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
>> index c6af0f02f619..b2efcddf058f 100644
>> --- a/src/drivers/driver_nl80211.c
>> +++ b/src/drivers/driver_nl80211.c
>> @@ -4195,6 +4195,22 @@ struct i802_link * nl80211_get_link(struct i802_bss *bss, s8 link_id)
> 
>> @@ -10259,7 +10275,9 @@ static int wpa_driver_nl80211_get_survey(void *priv, unsigned int freq)
>>   	if (err)
>>   		wpa_printf(MSG_ERROR, "nl80211: Failed to process survey data");
>>   	else
>> -		wpa_supplicant_event(drv->ctx, EVENT_SURVEY, &data);
>> +		wpa_supplicant_event((bss->scan_link && bss->scan_link->ctx) ?
>> +						bss->scan_link->ctx : bss->ctx,
>> +						EVENT_SURVEY, &data);
> 
> Wrong indentation level..

Will rectify.

> 
>> diff --git a/src/drivers/driver_nl80211_event.c b/src/drivers/driver_nl80211_event.c
> 
>> +			wpa_printf(MSG_DEBUG, "nl80211: Scan event on unknown link");
> 
> Too long a line (>80 chars); should be split after the comma.

Sure will do.

> 
>> +		if (mld_link->ctx) {
>> +			u8 link_id = nl80211_get_link_id_from_link(bss, mld_link);
> 
> Too long a line; should be
> 
> u8 link_id;
> 
> link_id = ...

Sure, thanks for pointing it out.

> 
>> +			wpa_printf(MSG_DEBUG, "nl80211: Scan event for link_id %d",
> 
> Too long a line.
> 
>> -static void do_process_drv_event(struct i802_bss *bss, int cmd,
>> -				 struct nlattr **tb)
>> +static void nl80211_scan_event(struct i802_bss *bss, enum nl80211_commands cmd,
>> +			       struct nlattr *tb[])
> 
> This diff gets really confusing since this mixes is refactoring (moving
> the scan event handlers to a separate function) and actual functionality
> changes. Please do not mix such things into a single patch. This is much
> clearer to review when that nl80211_scan_event() is not added and the
> functionality within do_process_drv_event() is modified instead. A
> separate patch can be proposed for refactoring, but to be honest, I
> don't see the value of that for this particular case.

Okay will directly make changes without refactoring. Thanks for the 
suggestion.

> 
>> diff --git a/src/drivers/driver_nl80211_scan.c b/src/drivers/driver_nl80211_scan.c
>> @@ -440,11 +449,19 @@ int wpa_driver_nl80211_scan(struct i802_bss *bss,
>> -	eloop_cancel_timeout(wpa_driver_nl80211_scan_timeout, drv, drv->ctx);
>> +	eloop_cancel_timeout(wpa_driver_nl80211_scan_timeout, drv, bss->ctx);
> 
> This looks scary and/or incomplete.. There are many locations where the
> eloop timeouts were not modified and the calls there still use drv->ctx.
> This is the case for the vendor commands for scan and also for clearing
> up eloop timeouts when deinitializing. It was far from obvious that this
> would work correctly for all cases.
> 

Okay. Let me take a look from vendor scan perspective.