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

[James Prestwood <prestwoj@gmail.com>]

On 12/6/23 08:40, Denis Kenzior wrote:
> 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?
I did this out of an abundance of caution. But yes I see we set it to 
false if !netdev->ap && !netdev->in_ft, so I can remove this.
>
>> +    } 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.

The problem is I removed the function so gcc will warn/break the build.

>
>>         unicast_watch = l_genl_add_unicast_watch(genl, 
>> NL80211_GENL_NAME,
>>                           netdev_unicast_notify,
>
> Regards,
> -Denis
>