| Message ID | 1346781171-16854-2-git-send-email-arik@wizery.com |
|---|---|
| State | Accepted |
| Commit | 50285f5ca8086cca45afa42cd23c3a3c1cd58f40 |
| Headers | show |
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
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.
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
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.
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).
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
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);