| Message ID | 20120408171730.GA9219@w1.fi |
|---|---|
| State | Accepted |
| Headers | show |
Hi Jouni & Johannes, > Like Johannes pointed out, it would be nice to get a patch file that is > not corrupted (tabs were removed or converted to a single space, etc., > that prevented the patch from applying). Similarly, it would be good to > get the D-Bus indication added, too. No matter how much I try, still some how I miss to find the white space corruption. :( Sorry for the trouble. I am not sure yet on whether corruption happens at my editor(customized vim) or at my email client. Any tips or ideas would be helpful. Regarding DBUS, I still don't have a very good understanding of D-BUS interface. So thought of working on it once initial changes are frozen/done. Regarding the patch, do we really need to iterate through the interface list?. I was assuming that the wpas_drv_shared_freq implementation should take care of this.In case of virtual interfaces sharing a single PHY, the shared_freq handler should return the freq in use (STA associated freq/ GO operating freq). Again, this iteration check would be required for drivers which doesn't implement the shared_freq handler and still have shared virtual interfaces over a single PHY. I may be missing something here also. Please correct my understanding, if wrong. Apart from this doubt, everything else looks fine with the cleaned up version of the patch. - Jithu On Sun, Apr 8, 2012 at 10:47 PM, Jouni Malinen <j@w1.fi> wrote: > On Tue, Mar 06, 2012 at 10:33:55AM +0530, Jithu Jance wrote: > > Single channel concurrency Patch [2/4] > > > > This patch handles the case where a p2p join fails due to a freq conflict > > with the existing STA connection. > > Like Johannes pointed out, it would be nice to get a patch file that is > not corrupted (tabs were removed or converted to a single space, etc., > that prevented the patch from applying). Similarly, it would be good to > get the D-Bus indication added, too. > > > diff --git a/wpa_supplicant/p2p_supplicant.c > > @@ -2644,6 +2645,14 @@ static void wpas_p2p_scan_res_join(struct > > wpa_supplicant *wpa_s, > > wpa_printf(MSG_DEBUG, "P2P: Target GO operating frequency " > > "from P2P peer table: %d MHz", freq); > > } > > + > > + if (!(wpa_s->drv_flags & WPA_DRIVER_FLAGS_MULTI_CHANNEL_CONCURRENT) && > > + ((shared_freq = wpa_drv_shared_freq(wpa_s)) > 0) && (shared_freq != > > freq)) { > > + wpa_msg(wpa_s->parent, MSG_INFO, > > + P2P_EVENT_GROUP_FORMATION_FAILURE "reason=FREQ_CONFLICT"); > > + return; > > + } > > This is not enough and not in the correct location. The following freq > update based on BSS table is needed for many cases. In addition, the > wpa_s->global->ifaces list should be iterated in search of an > conflicting operating channel. > > I added a more complete version of this type of validation: > > > commit 4b156206092d1d500407abbed7007071deee47d0 > > P2P: Abort join-group operation if concurrent group cannot be supported > > If the driver does not indicate support for multi-channel concurrency, > abort join-group operation if the end result would result in use of > multiple operating frequencies with the same radio. > > Signed-hostap: Jouni Malinen <j@w1.fi> > > diff --git a/wpa_supplicant/p2p_supplicant.c > b/wpa_supplicant/p2p_supplicant.c > index 1c339d2..fa675fe 100644 > --- a/wpa_supplicant/p2p_supplicant.c > +++ b/wpa_supplicant/p2p_supplicant.c > @@ -2605,6 +2605,44 @@ static void wpas_p2p_pd_before_join_timeout(void > *eloop_ctx, void *timeout_ctx) > } > > > +static int wpas_check_freq_conflict(struct wpa_supplicant *wpa_s, int > freq) > +{ > + struct wpa_supplicant *iface; > + int shared_freq; > + u8 bssid[ETH_ALEN]; > + > + if (wpa_s->drv_flags & WPA_DRIVER_FLAGS_MULTI_CHANNEL_CONCURRENT) > + return 0; > + > + for (iface = wpa_s->global->ifaces; iface; iface = iface->next) { > + if (!wpas_p2p_create_iface(wpa_s) && iface == wpa_s) > + continue; > + if (iface->current_ssid == NULL || iface->assoc_freq == 0) > + continue; > + if (wpa_drv_get_bssid(iface, bssid) == 0) { > + if (freq != (int) wpa_s->assoc_freq) { > + wpa_printf(MSG_DEBUG, "P2P: Frequency " > + "conflict - %s connected on %d > MHz " > + "- new connection on %d MHz", > + wpa_s->ifname, > wpa_s->assoc_freq, > + freq); > + return 1; > + } > + } > + } > + > + shared_freq = wpa_drv_shared_freq(wpa_s); > + if (shared_freq > 0 && shared_freq != freq) { > + wpa_printf(MSG_DEBUG, "P2P: Frequency conflict - shared " > + "virtual interface connected on %d MHz - new " > + "connection on %d MHz", shared_freq, freq); > + return 1; > + } > + > + return 0; > +} > + > + > static void wpas_p2p_scan_res_join(struct wpa_supplicant *wpa_s, > struct wpa_scan_results *scan_res) > { > @@ -2655,6 +2693,13 @@ static void wpas_p2p_scan_res_join(struct > wpa_supplicant *wpa_s, > if (freq > 0) { > u16 method; > > + if (wpas_check_freq_conflict(wpa_s, freq) > 0) { > + wpa_msg(wpa_s->parent, MSG_INFO, > + P2P_EVENT_GROUP_FORMATION_FAILURE > + "reason=FREQ_CONFLICT"); > + return; > + } > + > wpa_printf(MSG_DEBUG, "P2P: Send Provision Discovery > Request " > "prior to joining an existing group (GO " MACSTR > " freq=%u MHz)", > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap >
On Mon, Apr 09, 2012 at 12:44:54PM +0530, Jithu Jance wrote: > Regarding the patch, do we really need to iterate through the interface > list?. I was assuming that the wpas_drv_shared_freq implementation should > take care of this.In case of virtual interfaces sharing a single PHY, the > shared_freq handler should return the freq in use (STA associated freq/ GO > operating freq). The initial design of shared_freq() was really to use it only for the case where there are virtual interfaces controlled by something else than the current wpa_supplicant process (e.g., another wpa_supplicant process or something completely different in case where P2P support is managed by wpa_supplicant but a legacy station interface is run by another software component). For driver_nl80211.c, my assumption was that the same wpa_supplicant process would be used and as such, the expectation was that shared_freq() would not actually be needed at all.. While there is now shared_freq() in driveR_nl80211.c, I'm not sure I would really like to drop the concept of being able to figure out this type of details through iteration of the internal data structure in the core wpa_supplicant implementation. > Again, this iteration check would be required for drivers which doesn't > implement the shared_freq handler and still have shared virtual interfaces > over a single PHY. I may be missing something here also. Please correct my > understanding, if wrong. Apart from this doubt, everything else looks fine > with the cleaned up version of the patch. Well, that was the original assumption for driver_nl80211.c..
M On Apr 16, 2012 3:22 PM, "Jouni Malinen" <j@w1.fi> wrote: > On Mon, Apr 09, 2012 at 12:44:54PM +0530, Jithu Jance wrote: > > Regarding the patch, do we really need to iterate through the interface > > list?. I was assuming that the wpas_drv_shared_freq implementation should > > take care of this.In case of virtual interfaces sharing a single PHY, the > > shared_freq handler should return the freq in use (STA associated freq/ > GO > > operating freq). > > The initial design of shared_freq() was really to use it only for the > case where there are virtual interfaces controlled by something else > than the current wpa_supplicant process (e.g., another wpa_supplicant > process or something completely different in case where P2P support is > managed by wpa_supplicant but a legacy station interface is run by > another software component). For driver_nl80211.c, my assumption was > that the same wpa_supplicant process would be used and as such, the > expectation was that shared_freq() would not actually be needed at all.. > > While there is now shared_freq() in driveR_nl80211.c, I'm not sure I > would really like to drop the concept of being able to figure out this > type of details through iteration of the internal data structure in the > core wpa_supplicant implementation. > > > Again, this iteration check would be required for drivers which doesn't > > implement the shared_freq handler and still have shared virtual > interfaces > > over a single PHY. I may be missing something here also. Please correct > my > > understanding, if wrong. Apart from this doubt, everything else looks > fine > > with the cleaned up version of the patch. > > Well, that was the original assumption for driver_nl80211.c.. > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap >
diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c index 1c339d2..fa675fe 100644 --- a/wpa_supplicant/p2p_supplicant.c +++ b/wpa_supplicant/p2p_supplicant.c @@ -2605,6 +2605,44 @@ static void wpas_p2p_pd_before_join_timeout(void *eloop_ctx, void *timeout_ctx) } +static int wpas_check_freq_conflict(struct wpa_supplicant *wpa_s, int freq) +{ + struct wpa_supplicant *iface; + int shared_freq; + u8 bssid[ETH_ALEN]; + + if (wpa_s->drv_flags & WPA_DRIVER_FLAGS_MULTI_CHANNEL_CONCURRENT) + return 0; + + for (iface = wpa_s->global->ifaces; iface; iface = iface->next) { + if (!wpas_p2p_create_iface(wpa_s) && iface == wpa_s) + continue; + if (iface->current_ssid == NULL || iface->assoc_freq == 0) + continue; + if (wpa_drv_get_bssid(iface, bssid) == 0) { + if (freq != (int) wpa_s->assoc_freq) { + wpa_printf(MSG_DEBUG, "P2P: Frequency " + "conflict - %s connected on %d MHz " + "- new connection on %d MHz", + wpa_s->ifname, wpa_s->assoc_freq, + freq); + return 1; + } + } + } + + shared_freq = wpa_drv_shared_freq(wpa_s); + if (shared_freq > 0 && shared_freq != freq) { + wpa_printf(MSG_DEBUG, "P2P: Frequency conflict - shared " + "virtual interface connected on %d MHz - new " + "connection on %d MHz", shared_freq, freq); + return 1; + } + + return 0; +} + + static void wpas_p2p_scan_res_join(struct wpa_supplicant *wpa_s, struct wpa_scan_results *scan_res) { @@ -2655,6 +2693,13 @@ static void wpas_p2p_scan_res_join(struct wpa_supplicant *wpa_s, if (freq > 0) { u16 method; + if (wpas_check_freq_conflict(wpa_s, freq) > 0) { + wpa_msg(wpa_s->parent, MSG_INFO, + P2P_EVENT_GROUP_FORMATION_FAILURE + "reason=FREQ_CONFLICT"); + return; + } + wpa_printf(MSG_DEBUG, "P2P: Send Provision Discovery Request " "prior to joining an existing group (GO " MACSTR " freq=%u MHz)",