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

[Jouni Malinen]

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.

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

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

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

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

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

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

-- 
Jouni Malinen                                            PGP id EFC895FA