2:03 p.m.
From: Jose Blanquicet <jose.blanquicet-melendez(a)magnetimarelli.com>
This patch set does not remove any auto-connect/roaming functionality,
it aims to avoit keeping service in the list after AP deauthenticate
us and it is not found in the scan.
The first three patches revert the patches that add the connectable
flag because such implementation prevents ConnMan to remove the service
from the services list when it is no longer in range, resulting in a
confusion for users because they will continue seeing a service in their
list when it is not actually available.
Doing so, ConnMan will remove the service as other services and then,
once it comes back, wpa_s will notify ConnMan about it through a
"BSSAdded" signal which will start auto-connect procedure and state
machines would continue correctly. In case wpa_s does not send such
signal before notifying we got connected, which should not happen, the
last two patches of this set introduce a notification named
"network_associated" which should handle this border case. I tested it
by simulating ConnMan do not process "BSSAdded" signals and it worked,
it means that state machines moved correctly and connection trigger
from wpa_s completed successfully.
I do not think I could trigger all use cases thus I would appreciate if
people could test auto-connect and roaming use cases. Mainly
auto-connect when AP is who deauthenticate STA. For instance, Saurav
Babu in the scenario he recently mentioned when AP blocks STA's MAC
address.
Jose Blanquicet (5):
Revert "device: Use network's connectable flag"
Revert "plugins/wifi: Marking network's Connectable flag as true."
Revert "network: connectable flag in network structure"
gsupplicant: Add callback to notify the associated network
wifi: Set current network in case of reconnection
gsupplicant/gsupplicant.h | 1 +
gsupplicant/supplicant.c | 25 +++++++++++++++
include/network.h | 5 ---
plugins/wifi.c | 82 +++++++++++++++++++++++++++++------------------
src/device.c | 6 +---
src/network.c | 13 --------
6 files changed, 78 insertions(+), 54 deletions(-)
--
1.9.1
2:03 p.m.
New subject: [PATCH 1/5] Revert "device: Use network's connectable flag"
From: Jose Blanquicet <jose.blanquicet-melendez(a)magnetimarelli.com>
This reverts commit 9aad337b765dc6ae195e5f21d7764a71b2e82801.
---
src/device.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/device.c b/src/device.c
index 2e1a3cd..88de0e7 100644
--- a/src/device.c
+++ b/src/device.c
@@ -671,8 +671,7 @@ static void mark_network_unavailable(gpointer key, gpointer value,
struct connman_network *network = value;
if (connman_network_get_connected(network) ||
- connman_network_get_connecting(network) ||
- connman_network_get_connectable(network))
+ connman_network_get_connecting(network))
return;
connman_network_set_available(network, false);
@@ -689,9 +688,6 @@ static gboolean remove_unavailable_network(gpointer key, gpointer
value,
if (connman_network_get_available(network))
return FALSE;
- if (connman_network_get_connectable(network))
- return FALSE;
-
return TRUE;
}
--
1.9.1
2:03 p.m.
New subject: [PATCH 2/5] Revert "plugins/wifi: Marking network's Connectable flag as true."
From: Jose Blanquicet <jose.blanquicet-melendez(a)magnetimarelli.com>
This reverts commit ff9decb006ba69e83f47401a57cd7f4a5d5ba9f7.
---
plugins/wifi.c | 31 -------------------------------
1 file changed, 31 deletions(-)
diff --git a/plugins/wifi.c b/plugins/wifi.c
index f8d88fa..a721101 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -2113,21 +2113,7 @@ static int network_connect(struct connman_network *network)
wifi->pending_network = network;
g_free(ssid);
} else {
-
- /*
- * This is the network that is going to get plumbed into wpa_s
- * Mark the previous network that is plumbed in wpa_s as not
- * connectable and then the current one as connectable.
- * This flag will be used to ensure that the network that is
- * sitting in wpa_s never gets marked unavailable even though
- * the scan did not find this network.
- */
- if (wifi->network) {
- connman_network_set_connectable(wifi->network, false);
- }
-
wifi->network = connman_network_ref(network);
- connman_network_set_connectable(wifi->network, true);
wifi->retries = 0;
return g_supplicant_interface_connect(interface, ssid,
@@ -2151,7 +2137,6 @@ static void disconnect_callback(int result, GSupplicantInterface
*interface,
}
if (wifi->network) {
- connman_network_set_connectable(wifi->network, false);
connman_network_set_connected(wifi->network, false);
wifi->network = NULL;
}
@@ -2781,22 +2766,6 @@ static void network_removed(GSupplicantNetwork *network)
if (!connman_network)
return;
- /*
- * wpa_s did not find this network in last scan and hence it generated
- * this callback. In case if this is the network with which device
- * was connected to, even though network_removed was called, wpa_s
- * will keep trying to connect to the same network and once the
- * network is back, it will proceed with the connection. Now if
- * connman would have removed this network from network hash table,
- * on a successful connection complete indication service state
- * machine will not move. End result would be only a L2 level
- * connection and no IP address. This check ensures that even if the
- * network_removed gets called for the previously connected network
- * do not remove it from network hash table.
- */
- if (wifi->network == connman_network)
- return;
-
wifi->networks = g_slist_remove(wifi->networks, connman_network);
connman_device_remove_network(wifi->device, connman_network);
--
1.9.1
2:03 p.m.
New subject: [PATCH 3/5] Revert "network: connectable flag in network structure"
From: Jose Blanquicet <jose.blanquicet-melendez(a)magnetimarelli.com>
This reverts commit b7d20de51c5293fc28bb840e95a11b31747f51d8.
---
include/network.h | 5 -----
src/network.c | 13 -------------
2 files changed, 18 deletions(-)
diff --git a/include/network.h b/include/network.h
index 5d44669..4fc20c1 100644
--- a/include/network.h
+++ b/include/network.h
@@ -103,11 +103,6 @@ bool connman_network_get_connected(struct connman_network *network);
bool connman_network_get_associating(struct connman_network *network);
-bool connman_network_get_connectable(struct connman_network *network);
-
-int connman_network_set_connectable(struct connman_network *network,
- bool connectable);
-
void connman_network_clear_hidden(void *user_data);
int connman_network_connect_hidden(struct connman_network *network,
char *identity, char* passphrase, void *user_data);
diff --git a/src/network.c b/src/network.c
index 5b7ef55..4f881aa 100644
--- a/src/network.c
+++ b/src/network.c
@@ -50,7 +50,6 @@ struct connman_network {
bool available;
bool connected;
bool roaming;
- bool connectable;
uint8_t strength;
uint16_t frequency;
char *identifier;
@@ -830,18 +829,6 @@ static gint compare_priority(gconstpointer a, gconstpointer b)
return driver2->priority - driver1->priority;
}
-int connman_network_set_connectable(struct connman_network *network,
- bool connectable)
-{
- network->connectable = connectable;
- return 0;
-}
-
-bool connman_network_get_connectable(struct connman_network *network)
-{
- return network->connectable;
-}
-
/**
* connman_network_driver_register:
* @driver: network driver definition
--
1.9.1
2:03 p.m.
New subject: [PATCH 4/5] gsupplicant: Add callback to notify the associated network
From: Jose Blanquicet <jose.blanquicet-melendez(a)magnetimarelli.com>
Notify WiFi plugin about the network with which we got associated in
order to allow it to get aligned with what is happening in wpa_s.
---
gsupplicant/gsupplicant.h | 1 +
gsupplicant/supplicant.c | 25 +++++++++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/gsupplicant/gsupplicant.h b/gsupplicant/gsupplicant.h
index 678cf8b..db61595 100644
--- a/gsupplicant/gsupplicant.h
+++ b/gsupplicant/gsupplicant.h
@@ -351,6 +351,7 @@ struct _GSupplicantCallbacks {
void (*network_removed) (GSupplicantNetwork *network);
void (*network_changed) (GSupplicantNetwork *network,
const char *property);
+ void (*network_associated) (GSupplicantNetwork *network);
void (*peer_found) (GSupplicantPeer *peer);
void (*peer_lost) (GSupplicantPeer *peer);
void (*peer_changed) (GSupplicantPeer *peer,
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 36c4dd5..6cfd190 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -602,6 +602,17 @@ static void callback_network_changed(GSupplicantNetwork *network,
callbacks_pointer->network_changed(network, property);
}
+static void callback_network_associated(GSupplicantNetwork *network)
+{
+ if (!callbacks_pointer)
+ return;
+
+ if (!callbacks_pointer->network_associated)
+ return;
+
+ callbacks_pointer->network_associated(network);
+}
+
static void callback_peer_found(GSupplicantPeer *peer)
{
if (!callbacks_pointer)
@@ -2234,7 +2245,21 @@ static void interface_property(const char *key, DBusMessageIter
*iter,
g_strdup(interface->ifname), g_strdup(str));
}
} else if (g_strcmp0(key, "CurrentBSS") == 0) {
+ GSupplicantNetwork *network;
+ const char *path = NULL;
+
+ dbus_message_iter_get_basic(iter, &path);
+ if (g_strcmp0(path, "/") == 0)
+ return;
+
interface_bss_added_without_keys(iter, interface);
+
+ network = g_hash_table_lookup(interface->bss_mapping, path);
+ if (!network)
+ return;
+
+ if (interface->state == G_SUPPLICANT_STATE_ASSOCIATED)
+ callback_network_associated(network);
} else if (g_strcmp0(key, "CurrentNetwork") == 0) {
interface_network_added(iter, interface);
} else if (g_strcmp0(key, "BSSs") == 0) {
--
1.9.1
2:03 p.m.
New subject: [PATCH 5/5] wifi: Set current network in case of reconnection
From: Jose Blanquicet <jose.blanquicet-melendez(a)magnetimarelli.com>
Use associated notification from gsuplicant to ensure ConnMan is
prepared when wpa_s triggers a connection because it got associated
with the last AP we were connected to which previously deauthenticated
us. Otherwise, WiFi plugin may receive a notification it does not expect
which indicates interface state is "COMPLETED" and state machines would
not move, resulting in a L2 connection and no IP address.
This should no happen because wpa_s should always notify there is a
new BSS through a "BSSAdded" signal before notifing we got connected to
it. If so, ConnMan would create the corresponding service and
auto-connect would start and everything would be in its place when wpa_s
notifies we got connected.
In any case, this patch makes ConnMan prepared for such situation.
---
plugins/wifi.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/plugins/wifi.c b/plugins/wifi.c
index a721101..fe43c47 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -2800,6 +2800,56 @@ static void network_changed(GSupplicantNetwork *network, const char
*property)
}
}
+static void network_associated(GSupplicantNetwork *network)
+{
+ GSupplicantInterface *interface;
+ struct wifi_data *wifi;
+ struct connman_network *connman_network;
+ const char *identifier;
+
+ DBG("");
+
+ interface = g_supplicant_network_get_interface(network);
+ if (!interface)
+ return;
+
+ wifi = g_supplicant_interface_get_data(interface);
+ if (!wifi)
+ return;
+
+ identifier = g_supplicant_network_get_identifier(network);
+
+ connman_network = connman_device_get_network(wifi->device, identifier);
+ if (!connman_network)
+ return;
+
+ if (wifi->network) {
+ if (wifi->network == connman_network)
+ return;
+
+ /*
+ * This should never happen, we got associated with
+ * a network different than the one we were expecting.
+ */
+ DBG("Associated to %p while expecting %p",
+ connman_network, wifi->network);
+
+ connman_network_set_associating(wifi->network, false);
+ }
+
+ DBG("Reconnecting to previous network %p from wpa_s", connman_network);
+
+ wifi->network = connman_network_ref(connman_network);
+ wifi->retries = 0;
+
+ /*
+ * Set network/service in association state is the only step we could
+ * missed if wifi->network was not set in previous interface's states.
+ */
+ if (!wifi->connected)
+ connman_network_set_associating(wifi->network, true);
+}
+
static void apply_peer_services(GSupplicantPeer *peer,
struct connman_peer *connman_peer)
{
@@ -3011,6 +3061,7 @@ static const GSupplicantCallbacks callbacks = {
.network_added = network_added,
.network_removed = network_removed,
.network_changed = network_changed,
+ .network_associated = network_associated,
.peer_found = peer_found,
.peer_lost = peer_lost,
.peer_changed = peer_changed,
--
1.9.1
12:14 p.m.
On Fri, 2017-02-17 at 14:03 +0000, blanquicet(a)gmail.com wrote:
From: Jose Blanquicet
<jose.blanquicet-melendez(a)magnetimarelli.com>
This patch set does not remove any auto-connect/roaming
functionality,
it aims to avoit keeping service in the list after AP deauthenticate
us and it is not found in the scan.
The first three patches revert the patches that add the connectable
flag because such implementation prevents ConnMan to remove the
service
from the services list when it is no longer in range, resulting in a
confusion for users because they will continue seeing a service in
their
list when it is not actually available.
Doing so, ConnMan will remove the service as other services and then,
once it comes back, wpa_s will notify ConnMan about it through a
"BSSAdded" signal which will start auto-connect procedure and state
machines would continue correctly. In case wpa_s does not send such
signal before notifying we got connected, which should not happen,
the
last two patches of this set introduce a notification named
"network_associated" which should handle this border case. I tested
it
by simulating ConnMan do not process "BSSAdded" signals and it
worked,
it means that state machines moved correctly and connection trigger
from wpa_s completed successfully.
I do not think I could trigger all use cases thus I would appreciate
if
people could test auto-connect and roaming use cases. Mainly
auto-connect when AP is who deauthenticate STA. For instance, Saurav
Babu in the scenario he recently mentioned when AP blocks STA's MAC
address.
Thanks! Compiled ConnMan with these and the P2P technology patch. So
far so good, but let me leave this running for a while still...
Cheers,
Patrik
Jose Blanquicet (5):
Revert "device: Use network's connectable flag"
Revert "plugins/wifi: Marking network's Connectable flag as true."
Revert "network: connectable flag in network structure"
gsupplicant: Add callback to notify the associated network
wifi: Set current network in case of reconnection
gsupplicant/gsupplicant.h | 1 +
gsupplicant/supplicant.c | 25 +++++++++++++++
include/network.h | 5 ---
plugins/wifi.c | 82 +++++++++++++++++++++++++++++------
------------
src/device.c | 6 +---
src/network.c | 13 --------
6 files changed, 78 insertions(+), 54 deletions(-)
6:55 a.m.
On Mon, 2017-02-20 at 14:14 +0200, Patrik Flykt wrote:
On Fri, 2017-02-17 at 14:03 +0000, blanquicet(a)gmail.com wrote:
> From: Jose Blanquicet <jose.blanquicet-melendez(a)magnetimarelli.com>
>
> This patch set does not remove any auto-connect/roaming
> functionality,
> it aims to avoit keeping service in the list after AP
> deauthenticate
> us and it is not found in the scan.
>
> The first three patches revert the patches that add the connectable
> flag because such implementation prevents ConnMan to remove the
> service
> from the services list when it is no longer in range, resulting in
> a
> confusion for users because they will continue seeing a service in
> their
> list when it is not actually available.
>
> Doing so, ConnMan will remove the service as other services and
> then,
> once it comes back, wpa_s will notify ConnMan about it through a
> "BSSAdded" signal which will start auto-connect procedure and state
> machines would continue correctly. In case wpa_s does not send such
> signal before notifying we got connected, which should not happen,
> the
> last two patches of this set introduce a notification named
> "network_associated" which should handle this border case. I tested
> it
> by simulating ConnMan do not process "BSSAdded" signals and it
> worked,
> it means that state machines moved correctly and connection trigger
> from wpa_s completed successfully.
>
> I do not think I could trigger all use cases thus I would
> appreciate
> if
> people could test auto-connect and roaming use cases. Mainly
> auto-connect when AP is who deauthenticate STA. For instance,
> Saurav
> Babu in the scenario he recently mentioned when AP blocks STA's MAC
> address.
Thanks! Compiled ConnMan with these and the P2P technology patch. So
far so good, but let me leave this running for a while still...
Seems to work much better for the connected case. I used to see
disconnects followed by reconnects quite a few times a day, they are
now gone - or then the WiFi network got fixed this year, but is of a
much lesser probability than ConnMan gettign fixed.
That said, waking up from suspend makes wpa_supplicant disconnect every
connection attempt after resuming with:
connmand[6925]: plugins/wifi.c:disconnect_callback() result -5 supplicant interface
0x557a82af5bd0 wifi 0x557a82aeb840
This goes away if wpa_supplicant is restarted. Some other "feature" of
wpa_supplicant got exposed as a result?
Cheers,
Patrik
2:51 p.m.
Hi Patrik,
On Mon, Feb 27, 2017 at 6:55 AM, Patrik Flykt
<Patrik.Flykt(a)linux.intel.com> wrote:
Seems to work much better for the connected case. I used to see
disconnects followed by reconnects quite a few times a day, they are
now gone - or then the WiFi network got fixed this year, but is of a
much lesser probability than ConnMan gettign fixed.
That said, waking up from suspend makes wpa_supplicant disconnect every
connection attempt after resuming with:
connmand[6925]: plugins/wifi.c:disconnect_callback() result -5 supplicant interface
0x557a82af5bd0 wifi 0x557a82aeb840
This goes away if wpa_supplicant is restarted. Some other "feature" of
wpa_supplicant got exposed as a result?
Sorry for the delay to reply, I was out-of-office. This is my
analysis, I hope I correctly got what you mean.
Well, we do not allow to suspend our devices thus we have never tried
it before and this is indeed another case of reconnection after a
disconnection notified from wpa_s. However, this does not trigger this
path set because AP does not actually "disappear".
In any case, I tried to reproduce your problem and I found that when
the system is suspended, I immediately can see on the AP the
disconnection of such STA. Then, on the STA side, right after wpa_s
wakes up from "System suspend notification", it receives the
deauthenticate event. At this point, wpa_suuplicant is aware of the
actual link state and it correctly notifies disconnection to ConnMan
which starts auto-connect procedure. Now, ConnMan is ready for the
incoming reconnection and the full-chain get updated successfully in
my system.
Therefore, I could not reproduce your problem, I am not sure how
ConnMan fell on disconnect_callback() in this use case. Looking
briefly at the code, you could only fall there if is ConnMan itself
who asks for disconnection and this should not be the case. I have a
question, is this a regression after this patch set or is this
something that already happens in the past?
In addition, as you know, that -5 in your log is the mapping ConnMan
does to wpa_s errors, I think ConnMan should at least print the actual
wpa_s error in order to find it into the logs later for the analysis.
For instance, that would be very useful in cases like this, do you
agree?
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 36c4dd5..0862e45 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -4987,7 +4987,9 @@ static void interface_disconnect_result(const char *error,
SUPPLICANT_DBG("");
if (error) {
+ SUPPLICANT_DBG("error %s");
result = -EIO;
+
if (g_strcmp0("org.freedesktop.DBus.Error.UnknownMethod",
error) == 0)
result = -ECONNABORTED;
Regards,
Jose Blanquicet
9:16 a.m.
Hi Patrik,
On 03/01/17 15:51, Jose Blanquicet wrote:
In addition, as you know, that -5 in your log is the mapping ConnMan
does to wpa_s errors, I think ConnMan should at least print the actual
wpa_s error in order to find it into the logs later for the analysis.
For instance, that would be very useful in cases like this, do you
agree?
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 36c4dd5..0862e45 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -4987,7 +4987,9 @@ static void interface_disconnect_result(const char *error,
SUPPLICANT_DBG("");
if (error) {
+ SUPPLICANT_DBG("error %s");
result = -EIO;
+
if (g_strcmp0("org.freedesktop.DBus.Error.UnknownMethod",
error) == 0)
result = -ECONNABORTED;
Any news on this one?
Thanks,
Daniel
10:24 a.m.
Hi,
On Mon, Mar 13, 2017 at 9:16 AM, Daniel Wagner wrote:
> In addition, as you know, that -5 in your log is the mapping
ConnMan
> does to wpa_s errors, I think ConnMan should at least print the actual
> wpa_s error in order to find it into the logs later for the analysis.
> For instance, that would be very useful in cases like this, do you
> agree?
>
> diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
> index 36c4dd5..0862e45 100644
> --- a/gsupplicant/supplicant.c
> +++ b/gsupplicant/supplicant.c
> @@ -4987,7 +4987,9 @@ static void interface_disconnect_result(const char *error,
> SUPPLICANT_DBG("");
>
> if (error) {
> + SUPPLICANT_DBG("error %s");
> result = -EIO;
> +
> if (g_strcmp0("org.freedesktop.DBus.Error.UnknownMethod",
> error) == 0)
> result = -ECONNABORTED;
>
Any news on this one?
Sorry, I think all of you noticed the mistake, it was a quick/draft
idea I shared but I did not even compiled it. If you agree to add this
comment I will send a proper patch.
Regards,
Jose Blanquicet
11:01 a.m.
On Mon, 2017-03-13 at 10:24 +0000, Jose Blanquicet wrote:
Sorry, I think all of you noticed the mistake, it was a quick/draft
idea I shared but I did not even compiled it. If you agree to add
this
comment I will send a proper patch.
Yes, do send an update so we can trace down what connman/wpa_supplicant
think of the error condition.
Cheers,
Patrik
3:48 p.m.
Hi,
On Mon, Mar 13, 2017 at 11:01 AM, Patrik Flykt
<Patrik.Flykt(a)linux.intel.com> wrote:
Yes, do send an update so we can trace down what
connman/wpa_supplicant
think of the error condition.
Coming back to this topic ...
I found a couple of issues in the first version of the patch set,
first I did not considering the "variability" of wpa_supplicant, as I
explain in the new version of the patch, wpa_supplicant does not send
the CurrentBSS at the same state thus all states after associating
must be considered. The other issue is that IWD was also using the
connectable flag, I just removed its use because other issues are
strictly related to the way wpa_supplicant manages the network
profiles and reconnection.
Please check new patch set to know all the details and let me know
what do you think.
Best regards,
Jose Blanquicet
1348
days inactive
1472
days old
12 comments
4 participants
participants (4)
-
blanquicet＠gmail.com -
Daniel Wagner -
Jose Blanquicet
-
Patrik Flykt