[PATCH] station: Move netconfig_reset() to common path

[PATCH] station: Move netconfig_reset() to common path

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

Move the netconfig_reset calls to inside
station_reset_connection_state() instead of having each caller do
netconfig_reset() followed by station_reset_connection_state().  One of
the callers was missing the netconfig_reset() and this was resulting in
the "netconfig: Failed to start DHCPv4 client for interface %u" warning.
---
 src/station.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/station.c b/src/station.c
index 934815f2..61649c74 100644
--- a/src/station.c
+++ b/src/station.c
@@ -1501,6 +1501,9 @@ static void station_reset_connection_state(struct station *station)
 	if (!network)
 		return;
 
+	if (station->netconfig)
+		netconfig_reset(station->netconfig);
+
 	if (station->state == STATION_STATE_CONNECTED ||
 			station->state == STATION_STATE_CONNECTING ||
 			station->state == STATION_STATE_CONNECTING_AUTO ||
@@ -1530,9 +1533,6 @@ static void station_disassociated(struct station *station)
 {
 	l_debug("%u", netdev_get_ifindex(station->netdev));
 
-	if (station->netconfig)
-		netconfig_reset(station->netconfig);
-
 	station_reset_connection_state(station);
 
 	station_enter_state(station, STATION_STATE_DISCONNECTED);
@@ -2930,9 +2930,6 @@ static void station_disconnect_onconnect(struct station *station,
 		return;
 	}
 
-	if (station->netconfig)
-		netconfig_reset(station->netconfig);
-
 	station_reset_connection_state(station);
 
 	station_enter_state(station, STATION_STATE_DISCONNECTING);
@@ -3234,9 +3231,6 @@ int station_disconnect(struct station *station)
 	if (!station->connected_bss)
 		return -ENOTCONN;
 
-	if (station->netconfig)
-		netconfig_reset(station->netconfig);
-
 	/*
 	 * If the disconnect somehow fails we won't know if we're still
 	 * connected so we may as well indicate now that we're no longer
-- 
2.30.2

Re: [PATCH] station: Move netconfig_reset() to common path

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

On Thu, 30 Sept 2021 at 00:57, Denis Kenzior <denkenz(a)gmail.com> wrote:
> > I don't remember what the reasoning was, but station_connect_cb does
> > handle some mechanisms that seem desirable here also, like trying the
> > next bss from the autoconnect list immediately and blacklisting BSSes.
>
> The connect failed path is generally used if the connection simply fails.
> Either due to an invalid password, or due to handshake timeout, or some other
> reason where the status_code can be returned by netdev.  Once we're successfully
> connected, and the driver/kernel decides to disconnect us due to a firmware
> crash/bug/whatever, then I'm not sure blacklisting the bss should be done.
> Besides, we already sent the dbus message reply, etc.
>
> Now, maybe some elements of what the connect failed path does might be needed in
> this failure case.  But to determine that we would need to know why the
> disconnect happens? How often? Is it every time? Sometimes? Once in a blue moon,
> etc.

Unfortunately this applies to most failure modes in a connection (at
802.11 level or at IP level -- for the user they're the same thing)
and I think the binary blacklisted/not blacklisted model we use
doesn't really work.  IIRC my initial patches used a different model
where the blacklist was a kind of penalty list where BSSes and/or
networks that fail more often are pushed further in the priority list.
In the end when we've tried all the BSSes in range we have nothing to
lose by retrying some of the ones we've already tried, we just don't
want to put them before other BSSes.  This can be done in a way that
over time the list converges to sorted by how often a network/bss
fails.

This also allows you to learn from some very clear cues that we're not
handling, like a user manually switching to another network.

Best regards

Re: [PATCH] station: Move netconfig_reset() to common path

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]

Hi Andrew,

>> Now, maybe some elements of what the connect failed path does might be needed in
>> this failure case.  But to determine that we would need to know why the
>> disconnect happens? How often? Is it every time? Sometimes? Once in a blue moon,
>> etc.
> 
> Unfortunately this applies to most failure modes in a connection (at
> 802.11 level or at IP level -- for the user they're the same thing)

I agree, but details matter.  So what is it you're proposing exactly?

> and I think the binary blacklisted/not blacklisted model we use
> doesn't really work.  IIRC my initial patches used a different model
> where the blacklist was a kind of penalty list where BSSes and/or
> networks that fail more often are pushed further in the priority list.

I remember, but we have since moved on from that model of autoconnect.  We 
autoconnect networks now, not individual BSSes.  Eventually we will move to a 
network priority model in the future where the user will specify the priority of 
a given network for autoconnect.  Blacklisting within the network is a different 
topic.  Also note that we have two levels of blacklisting, so it isn't quite as 
simple as you describe.

> In the end when we've tried all the BSSes in range we have nothing to
> lose by retrying some of the ones we've already tried, we just don't
> want to put them before other BSSes.  This can be done in a way that

We do that already.

> over time the list converges to sorted by how often a network/bss
> fails.
> 
> This also allows you to learn from some very clear cues that we're not
> handling, like a user manually switching to another network.

Regards,
-Denis

Re: [PATCH] station: Move netconfig_reset() to common path

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Hi Andrew,

> I don't remember what the reasoning was, but station_connect_cb does
> handle some mechanisms that seem desirable here also, like trying the
> next bss from the autoconnect list immediately and blacklisting BSSes.

The connect failed path is generally used if the connection simply fails. 
Either due to an invalid password, or due to handshake timeout, or some other 
reason where the status_code can be returned by netdev.  Once we're successfully 
connected, and the driver/kernel decides to disconnect us due to a firmware 
crash/bug/whatever, then I'm not sure blacklisting the bss should be done. 
Besides, we already sent the dbus message reply, etc.

Now, maybe some elements of what the connect failed path does might be needed in 
this failure case.  But to determine that we would need to know why the 
disconnect happens? How often? Is it every time? Sometimes? Once in a blue moon, 
etc.

> In this case it seems the problem was triggered by a driver problem
> (or our misunderstanding on when the radio is free for scanning) but
> if a specific BSS always fails during netconfig then we should
> blacklist it.
> 

Sure, but then these actions should be taken in the station_disassociated path, 
or a brand new path specifically for this case.

Regards,
-Denis

Re: [PATCH] station: Move netconfig_reset() to common path

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On Thu, 30 Sept 2021 at 00:12, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 9/29/21 5:05 PM, Andrew Zaborowski wrote:
> > On Wed, 29 Sept 2021 at 22:06, Denis Kenzior <denkenz(a)gmail.com> wrote:
> >> On 9/29/21 11:42 AM, Andrew Zaborowski wrote:
> >>> Move the netconfig_reset calls to inside
> >>> station_reset_connection_state() instead of having each caller do
> >>
> >> This is on purpose.  There should be no need to call netconfig_reset if
> >> netconfig_configure wasn't called in the first place.
> >
> > In this case it *was* called, then for some reason the driver
> > deauthenticated when we requested a scan (from_ap is false), netdev
>
> That would be useful info and should be in the commit description ;)
>
> > emitted a NETDEV_EVENT_DISCONNECT_BY_SME and we end up in
> > station_connect_cb.  You're right in that station_connect_cb also
>
> Then that sounds like a bug or another side-effect of delaying the CONNECTED
> state until netconfig is done.    NETDEV_EVENT_DISCONNECT_BY_SME should result
> in station_disassociated() and not station_connect_cb()

I don't remember what the reasoning was, but station_connect_cb does
handle some mechanisms that seem desirable here also, like trying the
next bss from the autoconnect list immediately and blacklisting BSSes.
In this case it seems the problem was triggered by a driver problem
(or our misunderstanding on when the radio is free for scanning) but
if a specific BSS always fails during netconfig then we should
blacklist it.

Best regards

Re: [PATCH] station: Move netconfig_reset() to common path

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

Hi Andrew,

On 9/29/21 5:05 PM, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Wed, 29 Sept 2021 at 22:06, Denis Kenzior <denkenz(a)gmail.com> wrote:
>> On 9/29/21 11:42 AM, Andrew Zaborowski wrote:
>>> Move the netconfig_reset calls to inside
>>> station_reset_connection_state() instead of having each caller do
>>
>> This is on purpose.  There should be no need to call netconfig_reset if
>> netconfig_configure wasn't called in the first place.
> 
> In this case it *was* called, then for some reason the driver
> deauthenticated when we requested a scan (from_ap is false), netdev

That would be useful info and should be in the commit description ;)

> emitted a NETDEV_EVENT_DISCONNECT_BY_SME and we end up in
> station_connect_cb.  You're right in that station_connect_cb also

Then that sounds like a bug or another side-effect of delaying the CONNECTED 
state until netconfig is done.    NETDEV_EVENT_DISCONNECT_BY_SME should result 
in station_disassociated() and not station_connect_cb()

> handles failures before the netconfig_configure call, so if you want
> to avoid the potentially unneeded netconfig_reset() I can move it
> inside station_disconnect_event to avoid bigger changes.

Regards,
-Denis

Re: [PATCH] station: Move netconfig_reset() to common path

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

Hi Denis,

On Wed, 29 Sept 2021 at 22:06, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 9/29/21 11:42 AM, Andrew Zaborowski wrote:
> > Move the netconfig_reset calls to inside
> > station_reset_connection_state() instead of having each caller do
>
> This is on purpose.  There should be no need to call netconfig_reset if
> netconfig_configure wasn't called in the first place.

In this case it *was* called, then for some reason the driver
deauthenticated when we requested a scan (from_ap is false), netdev
emitted a NETDEV_EVENT_DISCONNECT_BY_SME and we end up in
station_connect_cb.  You're right in that station_connect_cb also
handles failures before the netconfig_configure call, so if you want
to avoid the potentially unneeded netconfig_reset() I can move it
inside station_disconnect_event to avoid bigger changes.

Best regards

Re: [PATCH] station: Move netconfig_reset() to common path

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

Hi Andrew,

On 9/29/21 11:42 AM, Andrew Zaborowski wrote:
> Move the netconfig_reset calls to inside
> station_reset_connection_state() instead of having each caller do

This is on purpose.  There should be no need to call netconfig_reset if 
netconfig_configure wasn't called in the first place.

> netconfig_reset() followed by station_reset_connection_state().  One of
> the callers was missing the netconfig_reset() and this was resulting in
> the "netconfig: Failed to start DHCPv4 client for interface %u" warning.

You need to figure out why calling netconfig_load_settings causes side-effects 
and fix that instead.

> ---
>   src/station.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 

Regards,
-Denis