Patchwork [2/3] P2P: always re-select oper channel if not hard coded

login
register
mail settings
Submitter Arik Nemtsov
Date Sept. 4, 2012, 5:52 p.m.
Message ID <1346781171-16854-2-git-send-email-arik@wizery.com>
Download mbox | patch
Permalink /patch/181647/
State Accepted
Commit 50285f5ca8086cca45afa42cd23c3a3c1cd58f40
Headers show

Comments

Arik Nemtsov - Sept. 4, 2012, 5:52 p.m.
Since the operating channel is randomly set to 1/6/11 on init, which is
always in the channel intersection, we were effectively ignoring the set
of p2p preferred channels.
Fix this by trying to get the best channel we can, unless the user hard
coded the operating channel in the configuration file. Fall back to the
initial randomly selected channel if a better one cannot be chosen.

Signed-hostap: Arik Nemtsov <arik@wizery.com>
---
 src/p2p/p2p_go_neg.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
Arik Nemtsov - Sept. 24, 2012, 2:14 p.m.
On Tue, Sep 4, 2012 at 8:52 PM, Arik Nemtsov <arik@wizery.com> wrote:
> Since the operating channel is randomly set to 1/6/11 on init, which is
> always in the channel intersection, we were effectively ignoring the set
> of p2p preferred channels.
> Fix this by trying to get the best channel we can, unless the user hard
> coded the operating channel in the configuration file. Fall back to the
> initial randomly selected channel if a better one cannot be chosen.
>
> Signed-hostap: Arik Nemtsov <arik@wizery.com>
> ---
>  src/p2p/p2p_go_neg.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/src/p2p/p2p_go_neg.c b/src/p2p/p2p_go_neg.c
> index 031b3a1..e1ba466 100644
> --- a/src/p2p/p2p_go_neg.c
> +++ b/src/p2p/p2p_go_neg.c
> @@ -391,6 +391,20 @@ static void p2p_reselect_channel(struct p2p_data *p2p,
>         }
>
>         /*
> +        * Try to see if the original channel is in the intersection. If
> +        * so, no need to change anything, as it already contains some
> +        * randomness.
> +        */
> +       if (p2p_channels_includes(intersection, p2p->op_reg_class,
> +                                 p2p->op_channel)) {
> +               wpa_msg(p2p->cfg->msg_ctx, MSG_DEBUG,
> +                       "P2P: Using original operating class and channel "
> +                       "(reg_class %u channel %u) from intersection",
> +                       p2p->op_reg_class, p2p->op_channel);
> +               return;
> +       }
> +
> +       /*
>          * Fall back to whatever is included in the channel intersection since
>          * no better options seems to be available.
>          */
> @@ -639,7 +653,8 @@ void p2p_process_go_neg_req(struct p2p_data *p2p, const u8 *sa,
>                                 wpa_hexdump(MSG_DEBUG, "P2P: channels",
>                                             c->channel, c->channels);
>                         }
> -                       if (!p2p_channels_includes(&intersection,
> +                       if (!p2p->cfg->cfg_op_channel ||
> +                           !p2p_channels_includes(&intersection,

There's a similar fix for p2p_process_go_neg_resp() which I forgot
here. But let's see if Jouni is ok with this approach before sending a
v2.

Arik
Jouni Malinen - Dec. 25, 2012, 6:07 p.m.
On Tue, Sep 04, 2012 at 08:52:50PM +0300, Arik Nemtsov wrote:
> Since the operating channel is randomly set to 1/6/11 on init, which is
> always in the channel intersection, we were effectively ignoring the set
> of p2p preferred channels.
> Fix this by trying to get the best channel we can, unless the user hard
> coded the operating channel in the configuration file. Fall back to the
> initial randomly selected channel if a better one cannot be chosen.

I've been trying to discourage use of p2p_oper_{reg_class,channel}
parameters in the configuration file since these were not really ever
designed to be used for normal use cases (i.e., they were only for
initial testing). As such, I'm not sure whether I would really like to
apply this change since it adds more uses for these parameters. The
proper way to force a specific channel is with the freq parameter to the
P2P command that is used to form the group.
Arik Nemtsov - Dec. 25, 2012, 7:49 p.m.
On Tue, Dec 25, 2012 at 8:07 PM, Jouni Malinen <j@w1.fi> wrote:
> On Tue, Sep 04, 2012 at 08:52:50PM +0300, Arik Nemtsov wrote:
>> Since the operating channel is randomly set to 1/6/11 on init, which is
>> always in the channel intersection, we were effectively ignoring the set
>> of p2p preferred channels.
>> Fix this by trying to get the best channel we can, unless the user hard
>> coded the operating channel in the configuration file. Fall back to the
>> initial randomly selected channel if a better one cannot be chosen.
>
> I've been trying to discourage use of p2p_oper_{reg_class,channel}
> parameters in the configuration file since these were not really ever
> designed to be used for normal use cases (i.e., they were only for
> initial testing). As such, I'm not sure whether I would really like to
> apply this change since it adds more uses for these parameters. The
> proper way to force a specific channel is with the freq parameter to the
> P2P command that is used to form the group.

Note this change is essential to make patch 3/3 regarding HT40 (which
you applied) work correctly.

We would never call p2p_reselect_channel() normally, since
p2p_channels_includes(p2p->op_channel) always returns true. This is
because p2p->op_channel = 1/6/11 from the p2p init code..

I'm attaching a new version of this patch, which includes the fix to
p2p_process_go_neg_resp() I mentioned. It is rebased on the (close to)
latest code.

Arik
Jouni Malinen - Dec. 27, 2012, 5:32 a.m.
On Tue, Dec 25, 2012 at 09:49:52PM +0200, Arik Nemtsov wrote:
> Note this change is essential to make patch 3/3 regarding HT40 (which
> you applied) work correctly.

Does the HT40 patch do any harm on its own, i.e., do I need to revert
it?

> We would never call p2p_reselect_channel() normally, since
> p2p_channels_includes(p2p->op_channel) always returns true. This is
> because p2p->op_channel = 1/6/11 from the p2p init code..

I see.. This may need changes, but this patch 2/3 is not enough to
address this.

> I'm attaching a new version of this patch, which includes the fix to
> p2p_process_go_neg_resp() I mentioned. It is rebased on the (close to)
> latest code.

This seems to break frequency specification as a p2p_connect parameter.
That case must not allow the p2p_reselect_channel() mechanisms to change
the forced channel. In addition, the debug log entry in the beginning of
p2p_reselect_channel() is quite confusing if the function gets called
with the new reason since the peer did not really reject the selected
channel in that case.
Jouni Malinen - Dec. 27, 2012, 6:41 a.m.
On Thu, Dec 27, 2012 at 07:32:58AM +0200, Jouni Malinen wrote:
> On Tue, Dec 25, 2012 at 09:49:52PM +0200, Arik Nemtsov wrote:
> > I'm attaching a new version of this patch, which includes the fix to
> > p2p_process_go_neg_resp() I mentioned. It is rebased on the (close to)
> > latest code.
> 
> This seems to break frequency specification as a p2p_connect parameter.
> That case must not allow the p2p_reselect_channel() mechanisms to change
> the forced channel. In addition, the debug log entry in the beginning of
> p2p_reselect_channel() is quite confusing if the function gets called
> with the new reason since the peer did not really reject the selected
> channel in that case.

It looks like the key missing part was the use of P2P_DEV_FORCE_FREQ
flag which was already missing before this patch. I fixed and cleaned up
the previous implementation and then applied a rebased version of this
patch 2/3 with a somewhat more accurate commit log (and with the
wpa_msg() call moved to the caller to avoid confusing debug log
entries).
Arik Nemtsov - Dec. 27, 2012, 8:51 a.m.
On Thu, Dec 27, 2012 at 8:41 AM, Jouni Malinen <j@w1.fi> wrote:
> On Thu, Dec 27, 2012 at 07:32:58AM +0200, Jouni Malinen wrote:
>> On Tue, Dec 25, 2012 at 09:49:52PM +0200, Arik Nemtsov wrote:
>> > I'm attaching a new version of this patch, which includes the fix to
>> > p2p_process_go_neg_resp() I mentioned. It is rebased on the (close to)
>> > latest code.
>>
>> This seems to break frequency specification as a p2p_connect parameter.
>> That case must not allow the p2p_reselect_channel() mechanisms to change
>> the forced channel. In addition, the debug log entry in the beginning of
>> p2p_reselect_channel() is quite confusing if the function gets called
>> with the new reason since the peer did not really reject the selected
>> channel in that case.
>
> It looks like the key missing part was the use of P2P_DEV_FORCE_FREQ
> flag which was already missing before this patch. I fixed and cleaned up
> the previous implementation and then applied a rebased version of this
> patch 2/3 with a somewhat more accurate commit log (and with the
> wpa_msg() call moved to the caller to avoid confusing debug log
> entries).

The patch looks good. Thanks.

Arik

Patch

diff --git a/src/p2p/p2p_go_neg.c b/src/p2p/p2p_go_neg.c
index 031b3a1..e1ba466 100644
--- a/src/p2p/p2p_go_neg.c
+++ b/src/p2p/p2p_go_neg.c
@@ -391,6 +391,20 @@  static void p2p_reselect_channel(struct p2p_data *p2p,
 	}
 
 	/*
+	 * Try to see if the original channel is in the intersection. If
+	 * so, no need to change anything, as it already contains some
+	 * randomness.
+	 */
+	if (p2p_channels_includes(intersection, p2p->op_reg_class,
+				  p2p->op_channel)) {
+		wpa_msg(p2p->cfg->msg_ctx, MSG_DEBUG,
+			"P2P: Using original operating class and channel "
+			"(reg_class %u channel %u) from intersection",
+			p2p->op_reg_class, p2p->op_channel);
+		return;
+	}
+
+	/*
 	 * Fall back to whatever is included in the channel intersection since
 	 * no better options seems to be available.
 	 */
@@ -639,7 +653,8 @@  void p2p_process_go_neg_req(struct p2p_data *p2p, const u8 *sa,
 				wpa_hexdump(MSG_DEBUG, "P2P: channels",
 					    c->channel, c->channels);
 			}
-			if (!p2p_channels_includes(&intersection,
+			if (!p2p->cfg->cfg_op_channel ||
+			    !p2p_channels_includes(&intersection,
 						   p2p->op_reg_class,
 						   p2p->op_channel))
 				p2p_reselect_channel(p2p, &intersection);