Patchwork [RFC,2/4] P2P: Handling single channel concurrency

login
register
mail settings
Submitter Jouni Malinen
Date April 8, 2012, 5:17 p.m.
Message ID <20120408171730.GA9219@w1.fi>
Download mbox | patch
Permalink /patch/151365/
State Accepted
Headers show

Comments

Jouni Malinen - April 8, 2012, 5:17 p.m.
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>
Jithu Jance - April 9, 2012, 7:14 a.m.
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
>
Jouni Malinen - April 16, 2012, 10:22 p.m.
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..
Irfan Sheriff - June 14, 2012, 3:29 p.m.
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
>

Patch

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)",