Re: [PATCH v2 6/9] netdev: add netdev_ft_reassociate

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 12/6/23 09:07, James Prestwood wrote:
> Essentially exposes (and renames) netdev_ft_tx_associate in order to
> be called similarly to netdev_reassociate/netdev_connect where a
> connect callback can be provided. This will fix the current bug where
> if association times out during FT IWD will hang and never transition
> to disconnected.
> 

I like it!

Nitpick below...

> This also removes the calling of the FT_ROAMED event and instead just
> calls the connect callback (since its now set). This unifies the
> callback path for reassociation and FT roaming.
> ---
>   src/netdev.c | 44 ++++++++++++++++++++++++++------------------
>   src/netdev.h |  5 +++++
>   2 files changed, 31 insertions(+), 18 deletions(-)
> 

<snip>

> diff --git a/src/netdev.c b/src/netdev.c
> index f2e887b4..7d52ffea 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -1409,16 +1409,14 @@ static void netdev_connect_ok(struct netdev *netdev)
>   			scan_bss_free(netdev->fw_roam_bss);
>   
>   		netdev->fw_roam_bss = NULL;
> -	} else if (netdev->in_ft) {
> -		if (netdev->event_filter)
> -			netdev->event_filter(netdev, NETDEV_EVENT_FT_ROAMED,
> -						NULL, netdev->user_data);
> -		netdev->in_ft = false;
>   	} else if (netdev->connect_cb) {
>   		netdev->connect_cb(netdev, NETDEV_RESULT_OK, NULL,
>   					netdev->user_data);
>   		netdev->connect_cb = NULL;
> -	}
> +		netdev->in_ft = false;
> +		netdev->in_reassoc = false;

Why do we set in_reassoc to false here?  Shouldn't it be taken care of already 
by the associate_event?

> +	} else
> +		l_warn("Connection event without a connect callback!");
>   
>   	netdev_rssi_polling_update(netdev);
>   

<snip>

> @@ -6256,7 +6265,6 @@ static int netdev_init(void)
>   	__eapol_set_install_pmk_func(netdev_set_pmk);
>   
>   	__ft_set_tx_frame_func(netdev_tx_ft_frame);
> -	__ft_set_tx_associate_func(netdev_ft_tx_associate);

This part probably belongs in the next patch or even patch 8.

>   
>   	unicast_watch = l_genl_add_unicast_watch(genl, NL80211_GENL_NAME,
>   						netdev_unicast_notify,

Regards,
-Denis