Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH] Supporting wifi load balancing and band steering
(Naveen Singh)
2. Re: [PATCH] gsupplicant: Optimize AddNetwork Handler by
avoiding DBUS call. (Naveen Singh)
----------------------------------------------------------------------
Message: 1
Date: Mon, 14 Mar 2016 15:30:32 -0700
From: Naveen Singh <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected], Naveen Singh <[email protected]>
Subject: Re: [PATCH] Supporting wifi load balancing and band steering
Message-ID:
<CAFK1zRDvEN=dfSFJHoWaUcNJdsM=Q9c=knxgr+fqoqoeuxd...@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
On Mon, Mar 14, 2016 at 2:41 AM, Patrik Flykt <[email protected]>
wrote:
>
> Hi,
>
> On Mon, 2016-03-07 at 16:47 -0800, [email protected] wrote:
> > From: Naveen Singh <[email protected]>
> >
> > Enterprise APS sometimes refuse association with assoc response
> > status code 17. AP basically wants to steer client to a right
> > band or a better AP for load balancing purposes. This does not
> > use to work as connman has no way to know that association was
> > denied and it would treat this disconnect notification as a normal
> > disconnect. wpa supplicant was sending DisconnectReason on DBUS as
> > a part of PropertyChanged signal but there was no AssocStatusCode.
> > In this commit id of hostapd (c7fb678f3109e62af1ef39be9b12bf8370c35bde)
>
> In which version of wpa_supplicant is this change?
>
This is in master. This change was done by me. I was waiting for this
change to be applied in wpa_supplicant before posting this change to
connman. There should not be any side effect of this change even if someone
is using older version of wpa_supplicant.
>
> > wpa supplicant is also sending assoc status code as a part of
> > PropertyChanged DBUS signal. Idea is that on a disconnect notification
> > from wpa supplicant, if the prior interface state is associating
> > (means a conection is underway) and association was denied with status
> > code 17 then do not proceed. This would let wpa_supplicant chose another
> > BSSID where association would stick.
> >
> > This is been tested by running a stress test which tries connection with
> > an enterprise AP which frequently denies association with status code 17.
>
> Ok.
>
> > ---
> > gsupplicant/gsupplicant.h | 2 ++
> > gsupplicant/supplicant.c | 19 ++++++++++++++++++-
> > plugins/wifi.c | 45
> +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/gsupplicant/gsupplicant.h b/gsupplicant/gsupplicant.h
> > index a2a7605..2f9806e 100644
> > --- a/gsupplicant/gsupplicant.h
> > +++ b/gsupplicant/gsupplicant.h
> > @@ -353,6 +353,8 @@ struct _GSupplicantCallbacks {
> > GSupplicantPeerState state);
> > void (*peer_request) (GSupplicantPeer *peer);
> > void (*debug) (const char *str);
> > + void (*update_disconnect_reasoncode)(GSupplicantInterface
> *interface, int reasoncode);
> > + void (*update_assoc_status_code)(GSupplicantInterface *interface,
> int reasoncode);
> > };
> >
> > typedef struct _GSupplicantCallbacks GSupplicantCallbacks;
> > diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
> > index c8fbef6..a5195b1 100644
> > --- a/gsupplicant/supplicant.c
> > +++ b/gsupplicant/supplicant.c
> > @@ -2135,9 +2135,26 @@ static void interface_property(const char *key,
> DBusMessageIter *iter,
> > } else if (g_strcmp0(key, "Networks") == 0) {
> > supplicant_dbus_array_foreach(iter,
> interface_network_added,
> > interface);
> > - } else
> > + } else if (g_strcmp0(key, "DisconnectReason") == 0) {
> > + int reason;
> > + if (dbus_message_iter_get_arg_type(iter) !=
> DBUS_TYPE_INVALID) {
> > + dbus_message_iter_get_basic(iter, &reason);
> > + if (callbacks_pointer &&
> callbacks_pointer->update_disconnect_reasoncode && reason != 0) {
> > +
> callbacks_pointer->update_disconnect_reasoncode(interface, reason);
> > + }
> > + }
> > + } else if (g_strcmp0(key, "AssocStatusCode") == 0) {
> > + int status_code;
> > + if (dbus_message_iter_get_arg_type(iter) !=
> DBUS_TYPE_INVALID) {
> > + dbus_message_iter_get_basic(iter, &status_code);
> > + if (callbacks_pointer &&
> callbacks_pointer->update_assoc_status_code) {
> > +
> callbacks_pointer->update_assoc_status_code(interface, status_code);
> > + }
> > + }
>
> Currently the order of wpa_supplicant properties must be such that all
> necessary information is available when the callback call is made here.
> If (or when) something changes with wpa_supplicant, it is safer if the
> attributes are collected first with the calling code reacting after
> that. Otherwise some information might not have been collected from the
> messages.
>
Are you suggesting that we call getter functions here to query
status/reason code. This may not work as wpa_supplicant sends disconnect
state signal immediately after sending status and reason code. So if
connman would query it may happen that DBUS result is sent after disconnect
notifications until we make our getter functions synchronous. I have seen
that these DBUS signals, always carry the associated value. Do you think
otherwise?
>
> Here one should follow the callback_*() convention so that the code is
> more uniform overall.
>
Sure I will make the change.
>
> > + } else {
> > SUPPLICANT_DBG("key %s type %c",
> > key, dbus_message_iter_get_arg_type(iter));
> > + }
> > }
> >
> > static void scan_network_update(DBusMessageIter *iter, void *user_data)
> > diff --git a/plugins/wifi.c b/plugins/wifi.c
> > index bb1cabc..6219b81 100644
> > --- a/plugins/wifi.c
> > +++ b/plugins/wifi.c
> > @@ -71,6 +71,8 @@
> > #define P2P_LISTEN_PERIOD 500
> > #define P2P_LISTEN_INTERVAL 2000
> >
> > +#define GSUP_80211_ASSOC_STATUS_NO_ADDITIONAL_CLIENT 17
> > +#define WPA_SUP_LOAD_SHAPING_MAX_RETRIES 3
> > static struct connman_technology *wifi_technology = NULL;
> > static struct connman_technology *p2p_technology = NULL;
> >
> > @@ -128,6 +130,7 @@ struct wifi_data {
> > unsigned flags;
> > unsigned int watch;
> > int retries;
> > + int wpa_sup_load_shaping_retries;
> > struct hidden_params *hidden;
> > bool postpone_hidden;
> > struct wifi_tethering_info *tethering_param;
> > @@ -144,6 +147,8 @@ struct wifi_data {
> > bool p2p_connecting;
> > bool p2p_device;
> > int servicing;
> > + int disconnect_reasoncode;
> > + int assoc_statuscode;
> > };
> >
> > static GList *iface_list = NULL;
> > @@ -2291,6 +2296,19 @@ static bool
> handle_wps_completion(GSupplicantInterface *interface,
> > return true;
> > }
> >
> > +static bool handle_assoc_status_code(GSupplicantInterface *interface,
> > + struct wifi_data *wifi)
> > +{
> > + if (wifi->state == G_SUPPLICANT_STATE_ASSOCIATING &&
> > + wifi->assoc_statuscode ==
> GSUP_80211_ASSOC_STATUS_NO_ADDITIONAL_CLIENT &&
> > + wifi->wpa_sup_load_shaping_retries <
> WPA_SUP_LOAD_SHAPING_MAX_RETRIES) {
> > + wifi->wpa_sup_load_shaping_retries ++;
> > + return TRUE;
> > + }
> > + wifi->wpa_sup_load_shaping_retries = 0;
> > + return FALSE;
> > +}
>
> Hmm. Is it less changes and cross-file dependencies if the assoc status
> code is handled in gsupplicant/supplicant.c instead of the wifi plugin?
> With that wpa_supplicant specifics would be handled earlier and the wifi
> pluging could do with less logic.
>
The reasons why I had the callback for disassoc and deauth reason code is
that it makes their handling much easier. We wanted to have this handling
done only in a particular state (like when the state is ASSOCIATING as we
know this problem can happen only during that state). I always thought
gsupplicant as the one getting the events from wpa_supplicant (or doing a
call to wpa_supplicant) and then calling appropriate call backs to plugin
sections.
>
> If there won't be 3 retries, the connect attempt will be cancelled with
> the service timer, right?
>
Yes the same way as if for some reason there is no response back from
wpa_Supplicant.
>
> > +
> > static bool handle_4way_handshake_failure(GSupplicantInterface
> *interface,
> > struct connman_network *network,
> > struct wifi_data *wifi)
> > @@ -2382,6 +2400,10 @@ static void interface_state(GSupplicantInterface
> *interface)
> > break;
> >
> > connman_network_set_connected(network, true);
> > +
> > + wifi->disconnect_reasoncode = 0;
> > + wifi->assoc_statuscode = 0;
> > + wifi->wpa_sup_load_shaping_retries = 0;
> > break;
> >
> > case G_SUPPLICANT_STATE_DISCONNECTED:
> > @@ -2399,6 +2421,9 @@ static void interface_state(GSupplicantInterface
> *interface)
> > if (is_idle(wifi))
> > break;
> >
> > + if (handle_assoc_status_code(interface, wifi))
> > + break;
> > +
> > /* If previous state was 4way-handshake, then
> > * it's either: psk was incorrect and thus we retry
> > * or if we reach the maximum retries we declare the
> > @@ -2935,6 +2960,24 @@ static void debug(const char *str)
> > connman_debug("%s", str);
> > }
> >
> > +static void wifi_disconnect_reasoncode(GSupplicantInterface *interface,
> int reasoncode)
> > +{
> > + struct wifi_data *wifi =
> g_supplicant_interface_get_data(interface);
> > +
> > + if (wifi != NULL) {
> > + wifi->disconnect_reasoncode = reasoncode;
> > + }
> > +}
> > +
> > +static void wifi_assoc_status_code(GSupplicantInterface *interface, int
> status_code)
> > +{
> > + struct wifi_data *wifi =
> g_supplicant_interface_get_data(interface);
> > +
> > + if (wifi != NULL) {
> > + wifi->assoc_statuscode = status_code;
> > + }
> > +}
> > +
> > static const GSupplicantCallbacks callbacks = {
> > .system_ready = system_ready,
> > .system_killed = system_killed,
> > @@ -2953,6 +2996,8 @@ static const GSupplicantCallbacks callbacks = {
> > .peer_changed = peer_changed,
> > .peer_request = peer_request,
> > .debug = debug,
> > + .update_disconnect_reasoncode = wifi_disconnect_reasoncode,
> > + .update_assoc_status_code = wifi_assoc_status_code,
> > };
> >
> >
>
> Patrik
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.01.org/pipermail/connman/attachments/20160314/ef732cd9/attachment-0001.html>
------------------------------
Message: 2
Date: Mon, 14 Mar 2016 15:40:33 -0700
From: Naveen Singh <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected], Naveen Singh <[email protected]>
Subject: Re: [PATCH] gsupplicant: Optimize AddNetwork Handler by
avoiding DBUS call.
Message-ID:
<CAFK1zRApFOmg2P49KWofteu-r0_xMuLiODe=tdsuiA-NLT=t...@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
On Mon, Mar 14, 2016 at 2:46 AM, Patrik Flykt <[email protected]>
wrote:
>
> Hi,
>
> On Mon, 2016-03-07 at 17:55 -0800, [email protected] wrote:
> > From: Naveen Singh <[email protected]>
> >
> > In case network path is not NULL, network should only be removed
> > if the new network (to be added) is different from what is sitting
> > in wpa_supplicant. This would avoid two DBUS calls: AddNetwork and
> > SelectNetwork.
> > ---
> > gsupplicant/supplicant.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
> > index c8fbef6..b4920fc 100644
> > --- a/gsupplicant/supplicant.c
> > +++ b/gsupplicant/supplicant.c
> > @@ -181,6 +181,7 @@ struct _GSupplicantInterface {
> > GHashTable *bss_mapping;
> > void *data;
> > const char *pending_peer_path;
> > + char * added_ssid;
> > };
> >
> > struct g_supplicant_bss {
> > @@ -4111,6 +4112,9 @@ static void interface_add_network_result(const
> char *error,
> >
> > interface->network_path = g_strdup(path);
> >
> > + g_free(interface->added_ssid);
> > + interface->added_ssid = g_strdup(data->ssid->ssid);
> > +
> > supplicant_dbus_method_call(data->interface->path,
> > SUPPLICANT_INTERFACE ".Interface", "SelectNetwork",
> > interface_select_network_params,
> > @@ -4708,6 +4712,17 @@ int
> g_supplicant_interface_connect(GSupplicantInterface *interface,
> > g_free(data->path);
> > dbus_free(data);
> >
> > + /* If this add network is for the same SSID for
> which
> > + * wpa_supplicant already has a profile then do
> not need
> > + * to add another profile. Only if the profile
> that needs
> > + * to get added is different from what is there in
> wpa_s
> > + * delete the current one. interface->added_ssid
> is populated
> > + * once add network request is successful.*/
> > +
> > + if (g_strcmp0(interface->added_ssid, ssid->ssid)
> == 0) {
> > + return -EALREADY;
> > + }
>
> Shouldn't interface->added_ssid also be cleared on disconnect? It seems
> it will be remembered over a previous disconnect (or then there is a
> memory leak somewhere) if only touched in
> interface_add_network_result()?
>
I thought about it. We do not want to remove this network on disconnect
notification because wpa_supplicant may be trying to connect to same SSID.
This should only be removed when network is removed (for example when we
are calling network_remove and then we can clear inteface->added_ssid in
network_remove_result). There is a leak here but it only exists when a
network is removed but then there is no connection attempts in future (as a
result of which add_network never gets called and hence it never gets
cleared). But I thought it was rare. To be correct I can make this change
and propose the new patch.
>
> > +
> > intf_data = dbus_malloc0(sizeof(*intf_data));
> > if (!intf_data)
> > return -ENOMEM;
>
> Patrik
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.01.org/pipermail/connman/attachments/20160314/c46db226/attachment.html>
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 5, Issue 20
**************************************
