| Message ID | 6C370B347C3FE8438C9692873287D2E1195AE35D6B@SJEXCHCCR01.corp.ad.broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Tue, 2011-12-27 at 03:15 -0800, Jithu Jance wrote: > This patch attempts to handle the single channel concurrency scenario where we initially have a P2P connection and then a STA connection is attempted. > > As a first step, i tried implementing following things. > 1. When STA connection is attempted, try to find whether there is any frequency conflict between any other interface. > 2. If the conflicting interface is a P2P GO, announce channel switch in beacons and move the GO. /* Just a hook is provided. Functionality is not yet implemented */ > 3. If GO is not able to move the channel or the driver doesn't support the functionality, remove the P2P GO interface. > 4. If the conflicting interface is a P2P Client, disconnect & remove the client interface. > > > Request your opinion on the below patch. > It would be nice if there were some hooks available to enable user to have an opinion about the action being taken. The STA connection may (1) be automated and be requested without user's knowledge, (2) be requested by user not knowing that the current connection may be dropped, etc. For example, user may not want to be disconnected from the current WFD session with their TV when trying to send email at same time, user may prefer to defer sending the email. Just dropping the WFD session is a rather bad user experience in this example. In any case if such a drastic action is taken I do think we need to give user an opportunity to make the decision. Please note that I am not proposing policy inside wpa_supplicant, but it seems like we need to start looking into making hooks available to support this. Reinette
Hi Reinette, Thanks for your suggestion. Please find my commments inline. > It would be nice if there were some hooks available to enable user to > have an opinion about the action being taken. The STA connection may (1) > be automated and be requested without user's knowledge, (2) be requested > by user not knowing that the current connection may be dropped, etc. For > example, user may not want to be disconnected from the current WFD > session with their TV when trying to send email at same time, user may > prefer to defer sending the email. Just dropping the WFD session is a > rather bad user experience in this example. Agree with you. Sharing my thoughts on acheiving this user notification part. One option is to have a config file parameter which framework can set and user would be able to change that. This will allow user to decide to prefer STA over P2P or P2P over STA. In addition to this, we may add a new reason code to GROUP_REMOVE event to indicate that group has been removed due to frequency conflict. This way framework/user app can come to know about it and notify user. This policing is not run-time. But this probably can be attempted as a first step. Another option. This is kind of a runtime decision making. But not sure whether this is the right way to do it. 1. On attempting a STA connection, check whether there is a frequency conflict with any other interface. 2. On detecting the frequency conflict following steps can be taken a) Disable the current STA network to avoid connection. supplicant won't try to connect until it is again enabled from the application. b) sent an event/notificaton to User space i) User app/framework on getting event should ask user persmission to forgo P2P connection ii) If user opts "no", the station config remains disabled and P2P connection stays as is. iii) If user opts "yes", the framework removes the p2p group and enables the STA config. Since network blob is enabled, sta connection is again attempted and this time supplicant won't find a conflict since p2p interface doesn't exist. Request opinion on above thoughts. - Jithu Jance.
Hi Jithu, On Thu, 2012-01-05 at 06:24 -0800, Jithu Jance wrote: > > It would be nice if there were some hooks available to enable user to > > have an opinion about the action being taken. The STA connection may (1) > > be automated and be requested without user's knowledge, (2) be requested > > by user not knowing that the current connection may be dropped, etc. For > > example, user may not want to be disconnected from the current WFD > > session with their TV when trying to send email at same time, user may > > prefer to defer sending the email. Just dropping the WFD session is a > > rather bad user experience in this example. > > Agree with you. Sharing my thoughts on acheiving this user notification part. > One option is to have a config file parameter which framework can set and user > would be able to change that. This will allow user to decide to prefer STA over > P2P or P2P over STA. This sounds good. Maybe higher level options like "first come first served" could make things even easier to get this off the ground. Maybe even start with one default, which then does not even need a configuration parameter - it could just be the one default that wpa_supplicant supports unless there are hooks that indicate otherwise. I am trying to be careful with the choices here since (based on my potentially flawed assumption of a typical user) a typical user may not care about the technology (P2P vs STA). A user may want to know things like "if I try to print this document then I will loose my internet connection" which is much higher level than what wpa_supplicant should care about. > In addition to this, we may add a new reason code to GROUP_REMOVE > event to indicate that group has been removed due to frequency conflict. This way > framework/user app can come to know about it and notify user. This policing is > not run-time. But this probably can be attempted as a first step. Excellent, yes. In addition to the reason added to GROUP_REMOVE there should maybe also be a reason code to the STA side of things. Some logic may need to be added to connection managers to support this smoothly so that the connection manager will not keep trying to associate when something like an already present P2P connection prevents it from doing so. > Another option. This is kind of a runtime decision making. But not sure whether this > is the right way to do it. I do think runtime decision making is the way to go. > 1. On attempting a STA connection, check whether there is a frequency conflict with any other > interface. Here we do have the "interface capabilities" information available (see "cfg80211: advertise possible interface combinations" in kernel) to help with this decision. I do not think this is being used by wpa_supplicant yet. I am not sure whether we would like to make things this complicated inside wpa_supplicant core either and things could be worked out if wpa_supplicant makes this information available to another component that will make decisions with or without user interaction. > 2. On detecting the frequency conflict following steps can be taken As mentioned above I am not sure we want to complicate things inside wpa_supplicant. > a) Disable the current STA network to avoid connection. supplicant won't try to connect until it is again > enabled from the application. > b) sent an event/notificaton to User space > i) User app/framework on getting event should ask user persmission to forgo P2P connection > ii) If user opts "no", the station config remains disabled and P2P connection stays as is. > iii) If user opts "yes", the framework removes the p2p group and enables the STA config. Since network blob is enabled, > sta connection is again attempted and this time supplicant won't find a conflict since p2p interface doesn't exist. I agree with the high level concepts you show here. You present two steps and I think it can be summarized that (a) will be the default decision made by wpa_supplicant if there are not any hooks, (b) will be whatever the "hook" decides. One way to accomplish this is to have a new component that implements a set of hooks. This component may be the one that could retrieve all the interface capability information from wpa_supplicant (once it becomes available), it could have a database of "known good behaviors", it may even contain default behaviors (because a user previously selected a "Do not ask me again" option), etc. This can be made pretty fancy. A hook itself could be a question combined with enough state information to make a decision. The answer could be a "yes" or "no" and may include more information in response (like which channels are available for use). How the hook comes to the answer can be a variety of things. It can query its database or it can post a question to the user. Since this component is contained it can be enhanced to support a very good user experience. One thing that I am not clear on is the "framework" you mention in your description. As you mention, when a decision comes back from the hooks then there may be a few steps that needs to be performed before the original request can be serviced. Like in your example when the user opts "yes" then the P2P group needs to be removed and STA connection reattempted. It seems that there may be a need for a small layer on top of (but maybe not separate from) wpa_supplicant that can arbitrate these potentially conflicting incoming requests (which may not be limited to connectivity) and take care of the additional steps needed to service the original request. > Request opinion on above thoughts. You asked for it :) Reinette
On Thu, Jan 05, 2012 at 11:12:23AM -0800, Reinette Chatre wrote: > This sounds good. Maybe higher level options like "first come first > served" could make things even easier to get this off the ground. Maybe > even start with one default, which then does not even need a > configuration parameter - it could just be the one default that > wpa_supplicant supports unless there are hooks that indicate otherwise. It sounds like a good idea to start with that and maybe even leave it there. I'm not sure why wpa_supplicant would need to know more. In other words, allow either STA or P2P operation to be started when nothing else is running and reject any new command that would result in having to stop a previously started connection (ideally with an error code indicating the reason). In other words, something above wpa_supplicant would take care of user preferences on how to behave based on use cases etc. It would explicitly need to stop previously started operations before starting the new conflicting one - wpa_supplicant should not need extra code for that part. > > In addition to this, we may add a new reason code to GROUP_REMOVE > > event to indicate that group has been removed due to frequency conflict. This way > > framework/user app can come to know about it and notify user. This policing is > > not run-time. But this probably can be attempted as a first step. > > Excellent, yes. In addition to the reason added to GROUP_REMOVE there > should maybe also be a reason code to the STA side of things. Some logic > may need to be added to connection managers to support this smoothly so > that the connection manager will not keep trying to associate when > something like an already present P2P connection prevents it from doing > so. This reason code in group remove events is P2P_GROUP_REMOVAL_UNAVAILABLE and driver can already trigger it with EVENT_INTERFACE_UNAVAILABLE on the group interface. That event just needs to be added to nl80211 so that this goes through (i.e., this has been used only with some out-of-tree driver wrappers so far). Well.. The connection manager can try, but that will be rejected (assuming the STA network does not happen to be available on the same channel). Anyway, connection managers will obviously need to become more aware of concurrent operations for smoother user experience.
On Tue, Dec 27, 2011 at 03:15:49AM -0800, Jithu Jance wrote: > This patch attempts to handle the single channel concurrency scenario where we initially have a P2P connection and then a STA connection is attempted. > > As a first step, i tried implementing following things. > 1. When STA connection is attempted, try to find whether there is any frequency conflict between any other interface. > 2. If the conflicting interface is a P2P GO, announce channel switch in beacons and move the GO. /* Just a hook is provided. Functionality is not yet implemented */ This sounds fine. > 3. If GO is not able to move the channel or the driver doesn't support the functionality, remove the P2P GO interface. > 4. If the conflicting interface is a P2P Client, disconnect & remove the client interface. How about rejecting the new STA connection instead of killing existing connection? In other words, some entity above wpa_supplicant would need to kill the group first before being able to get the STA connection in such a case. > diff --git a/src/drivers/driver.h b/src/drivers/driver.h > @@ -2483,6 +2483,17 @@ struct wpa_driver_ops { > + /** > + * go_switch_channel - Announce channel switch and migrate the GO to a > + * given frequency. > + * @priv: Private driver interface data > + * @freq: frequency in MHz > + * Returns: 0 on success, -1 on failure > + * > + * This function is used to move the GO to the legacy STA channel to avoid > + * frequency conflict in single channel concurrency. > + */ > + int (*go_switch_channel)(void *priv, unsigned int freq); I would assume that this could be made generic to both non-P2P AP and P2P GO, so the same could be just switch_channels instead of something P2P specific. > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > @@ -1415,6 +1416,16 @@ void wpa_supplicant_associate(struct wpa_supplicant *wpa_s, > + /* If multichannel concurrency is not supported, check for any frequency > + * conflict and take appropriate action. > + */ > + if (((freq = wpa_drv_shared_freq(wpa_s)) > 0) && (freq != params.freq) && > + !(wpa_s->drv_flags & WPA_DRIVER_FLAGS_MULTI_CHANNEL_CONCURRENT)) { I would reorder those conditions to avoid the wpa_drv_shared_freq() call is multi-channel concurrency is supported.
Hi Jouni, On Sun, 2012-01-08 at 20:59 -0800, Jouni Malinen wrote: > In other words, something above wpa_supplicant would take care of users totally separe > preferences on how to behave based on use cases etc. It would explicitly > need to stop previously started operations before starting the new > conflicting one - wpa_supplicant should not need extra code for that > part. I agree that something above wpa_supplicant should take care of user preferences and how to behave. I am not sure that this can efficiently be accomplished without adding _any_ code to wpa_supplicant. How I understand things a complete separate solution like this would add an additional communication hop (eg. D-Bus) to things and it will also require changes to all communication managers to now start talking to this new entity instead of wpa_supplicant. Reinette
Thanks Jouni & Reinette for your comments. Please find my comments inline. * * > > > 3. If GO is not able to move the channel or the driver doesn't support > the functionality, remove the P2P GO interface. > > 4. If the conflicting interface is a P2P Client, disconnect & remove the > client interface. > > How about rejecting the new STA connection instead of killing existing > connection? In other words, some entity above wpa_supplicant would need > to kill the group first before being able to get the STA connection in > such a case. > Do you mean that we will keep the BSS disabled and then let the application/entity above know about the frequency conflict. Then the application would take a call whether to remove the p2p group or not. If application choose to remove p2p_group, it may need to enable the disabled BSS so that the STA connection can proceed. If this is what you meant, then i guess probably we might have to define a new event which will notify the application of the frequency conflict with enough data [the network id of the bss in conflict so that the application can enable the network blob after removing the P2P group]. Please correct me if my understanding is wrong. If this is fine with you, i shall generate a new patch and send it to you. > > > diff --git a/src/drivers/driver.h b/src/drivers/driver.h > > @@ -2483,6 +2483,17 @@ struct wpa_driver_ops { > > + /** > > + * go_switch_channel - Announce channel switch and migrate the GO > to a > > + * given frequency. > > + * @priv: Private driver interface data > > + * @freq: frequency in MHz > > + * Returns: 0 on success, -1 on failure > > + * > > + * This function is used to move the GO to the legacy STA channel > to avoid > > + * frequency conflict in single channel concurrency. > > + */ > > + int (*go_switch_channel)(void *priv, unsigned int freq); > > I would assume that this could be made generic to both non-P2P AP and > P2P GO, so the same could be just switch_channels instead of something > P2P specific. > I will update this part. > > > diff --git a/wpa_supplicant/wpa_supplicant.c > b/wpa_supplicant/wpa_supplicant.c > > @@ -1415,6 +1416,16 @@ void wpa_supplicant_associate(struct > wpa_supplicant *wpa_s, > > + /* If multichannel concurrency is not supported, check for any > frequency > > + * conflict and take appropriate action. > > + */ > > + if (((freq = wpa_drv_shared_freq(wpa_s)) > 0) && (freq != > params.freq) && > > + !(wpa_s->drv_flags & > WPA_DRIVER_FLAGS_MULTI_CHANNEL_CONCURRENT)) { > > I would reorder those conditions to avoid the wpa_drv_shared_freq() call > is multi-channel concurrency is supported. > Agreed. :) - Jithu Jance.
On Wed, Jan 11, 2012 at 10:40:26AM -0800, Jithu Jance wrote: > > How about rejecting the new STA connection instead of killing existing > > connection? In other words, some entity above wpa_supplicant would need > > to kill the group first before being able to get the STA connection in > > such a case. > Do you mean that we will keep the BSS disabled and then let the > application/entity above know about the frequency conflict. Then the > application would take a call whether to remove the p2p group or not. If > application choose to remove p2p_group, it may need to enable the disabled > BSS so that the STA connection can proceed. Well, I did not actually consider it in that level of detail, but yes, something like this was indeed the intention here. > If this is what you meant, then > i guess probably we might have to define a new event which will notify the > application of the frequency conflict with enough data [the network id of > the bss in conflict so that the application can enable the network blob > after removing the P2P group]. Please correct me if my understanding is > wrong. This is the part I did not think about, i.e., I was more or less thinking of just making the command that requested connection fail. Though, that doesn't really work in many cases, so this type of event may indeed be the way to go. > If this is fine with you, i shall generate a new patch and send it to you. Yes, please.
diff --git a/src/drivers/driver.h b/src/drivers/driver.h index 53d1c25..104059e 100644 --- a/src/drivers/driver.h +++ b/src/drivers/driver.h @@ -2483,6 +2483,17 @@ struct wpa_driver_ops { */ void (*poll_client)(void *priv, const u8 *own_addr, const u8 *addr, int qos); + /** + * go_switch_channel - Announce channel switch and migrate the GO to a + * given frequency. + * @priv: Private driver interface data + * @freq: frequency in MHz + * Returns: 0 on success, -1 on failure + * + * This function is used to move the GO to the legacy STA channel to avoid + * frequency conflict in single channel concurrency. + */ + int (*go_switch_channel)(void *priv, unsigned int freq); }; diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index 2299cc8..7e758c4 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -8285,14 +8285,17 @@ static int wpa_driver_nl80211_shared_freq(void *priv) struct wpa_driver_nl80211_data, list) { if (drv == driver || os_strcmp(drv->phyname, driver->phyname) != 0 || - !driver->associated) + (!driver->associated && !is_ap_interface(driver->nlmode))) continue; wpa_printf(MSG_DEBUG, "nl80211: Found a match for PHY %s - %s " MACSTR, driver->phyname, driver->first_bss.ifname, MAC2STR(driver->first_bss.addr)); - freq = nl80211_get_assoc_freq(driver); + if(is_ap_interface(driver->nlmode)) + freq = driver->first_bss.freq; + else + freq = nl80211_get_assoc_freq(driver); wpa_printf(MSG_DEBUG, "nl80211: Shared freq for PHY %s: %d", drv->phyname, freq); } diff --git a/wpa_supplicant/driver_i.h b/wpa_supplicant/driver_i.h index d61b3fd..92f291b 100644 --- a/wpa_supplicant/driver_i.h +++ b/wpa_supplicant/driver_i.h @@ -663,4 +663,11 @@ static inline void wpa_drv_set_rekey_info(struct wpa_supplicant *wpa_s, wpa_s->driver->set_rekey_info(wpa_s->drv_priv, kek, kck, replay_ctr); } +static inline int wpa_drv_go_switch_channel(struct wpa_supplicant *wpa_s, + int freq) +{ + if (!wpa_s->driver->go_switch_channel) + return -1; + return wpa_s->driver->go_switch_channel(wpa_s->drv_priv, freq); +} #endif /* DRIVER_I_H */ diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c index 5ff94ef..cd693ab 100644 --- a/wpa_supplicant/p2p_supplicant.c +++ b/wpa_supplicant/p2p_supplicant.c @@ -66,7 +66,40 @@ static int wpas_p2p_create_iface(struct wpa_supplicant *wpa_s); static void wpas_p2p_cross_connect_setup(struct wpa_supplicant *wpa_s); static void wpas_p2p_group_idle_timeout(void *eloop_ctx, void *timeout_ctx); static void wpas_p2p_set_group_idle_timeout(struct wpa_supplicant *wpa_s); +static int wpas_p2p_go_switch_channel(struct wpa_supplicant *wpa_s, int freq) +{ + int ret = -1; + /* Need to add support for channel switch for softmac drivers */ + + return ret; +} + +void wpas_p2p_handle_frequency_conflicts(struct wpa_supplicant *wpa_s, int freq) +{ + struct wpa_supplicant *iface = NULL; + for (iface = wpa_s->global->ifaces; iface; iface = iface->next) { + if((iface->p2p_group_interface) && (iface->current_ssid) && + (iface->current_ssid->frequency != freq)) { + + if (iface->p2p_group_interface == P2P_GROUP_INTERFACE_GO) { + /* Try to see whether we can move the GO. If it + * is not possible, remove the GO interface + */ + if((wpas_p2p_go_switch_channel(iface, freq) == 0) + || (wpa_drv_go_switch_channel(iface, freq) == 0)) { + wpa_printf(MSG_ERROR, "P2P: GO Moved to freq(%d)", freq); + iface->current_ssid->frequency = freq; + continue; + } + } + + /* If GO cannot be moved or if the conflicting interface is a + * P2P Client, remove the interface */ + wpas_p2p_disconnect(iface); + } + } +} static void wpas_p2p_scan_res_handler(struct wpa_supplicant *wpa_s, struct wpa_scan_results *scan_res) diff --git a/wpa_supplicant/p2p_supplicant.h b/wpa_supplicant/p2p_supplicant.h index 3150f04..4fa155b 100644 --- a/wpa_supplicant/p2p_supplicant.h +++ b/wpa_supplicant/p2p_supplicant.h @@ -31,6 +31,8 @@ void wpas_p2p_remain_on_channel_cb(struct wpa_supplicant *wpa_s, unsigned int freq, unsigned int duration); void wpas_p2p_cancel_remain_on_channel_cb(struct wpa_supplicant *wpa_s, unsigned int freq); +void wpas_p2p_handle_frequency_conflicts(struct wpa_supplicant *wpa_s, + int freq); int wpas_p2p_group_remove(struct wpa_supplicant *wpa_s, const char *ifname); int wpas_p2p_group_add(struct wpa_supplicant *wpa_s, int persistent_group, int freq); diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index 675b592..5b4eac9 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -1121,6 +1121,7 @@ void wpa_supplicant_associate(struct wpa_supplicant *wpa_s, struct wpa_driver_capa capa; int assoc_failed = 0; struct wpa_ssid *old_ssid; + int freq = 0; #ifdef CONFIG_IBSS_RSN ibss_rsn_deinit(wpa_s->ibss_rsn); @@ -1415,6 +1416,16 @@ void wpa_supplicant_associate(struct wpa_supplicant *wpa_s, else params.uapsd = -1; + /* If multichannel concurrency is not supported, check for any frequency + * conflict and take appropriate action. + */ + if (((freq = wpa_drv_shared_freq(wpa_s)) > 0) && (freq != params.freq) && + !(wpa_s->drv_flags & WPA_DRIVER_FLAGS_MULTI_CHANNEL_CONCURRENT)) { + wpa_printf(MSG_ERROR, "Shared interface with conflicting frequency found (%d != %d)" + , freq, params.freq); + wpas_p2p_handle_frequency_conflicts(wpa_s, params.freq); + } + ret = wpa_drv_associate(wpa_s, ¶ms); if (ret < 0) { wpa_msg(wpa_s, MSG_INFO, "Association request to the driver "