Message ID | 0BED5F74AB839C4796CA2BFC2BC311B419C433044C@SJEXCHCCR01.corp.ad.broadcom.com |
---|---|
State | Superseded |
Headers | show |
On Mon, Dec 26, 2011 at 09:23:54PM -0800, Jithu Jance wrote: > Sounds like for "p2p_connect <devaddr> pin display join" command, suppressing Prov Disc SHOW-PIN would be a better > approach. Agreed. > I would also like to confirm about the handling of wpas_notify_p2p_provision_discovery > in this particular scenario. Is it okay to invoke the function with generated_pin = 0 value (the below patch uses this approach) > or should I suppress the invocation of wpas_notify_p2p_provision_discovery when wpas->p2p_pin is populated?? I think it is better to skip this call completely. Actually, these provision discovery response notifications should really be skipped completely in the automatic PD-before-join case since those were not explicitly requested by any external program and they do not really provide any additional information. > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c > @@ -1785,8 +1785,7 @@ void wpas_prov_disc_req(void *ctx, const u8 *peer, u16 config_methods, > - > - if (config_methods & WPS_CONFIG_DISPLAY) { > + if ((config_methods & WPS_CONFIG_DISPLAY) && (wpa_s->p2p_pin[0] == '\0')) { > generated_pin = wps_generate_pin(); Why would we need to modify prov_disc_req callback? This is not used on the P2P device that will be joining a group as a P2P client. > @@ -1809,7 +1808,7 @@ void wpas_prov_disc_resp(void *ctx, const u8 *peer, u16 config_methods) > if (config_methods & WPS_CONFIG_DISPLAY) > wpas_prov_disc_local_keypad(wpa_s, peer, ""); > - else if (config_methods & WPS_CONFIG_KEYPAD) { > + else if ((config_methods & WPS_CONFIG_KEYPAD) && (wpa_s->p2p_pin[0] == '\0')) { > generated_pin = wps_generate_pin(); > wpas_prov_disc_local_display(wpa_s, peer, "", generated_pin); > } else if (config_methods & WPS_CONFIG_PUSHBUTTON) While this could be fine for most cases, I don't really want to depend on wpa_s->p2p_pin getting cleared in all cases. I guess this could potentially skip an event message for Provision Discovery that is done in other contexts. I implemented this a bit differently in commit c19316354ed46e688704c1ebcf1c7efe816ddf31 by skipping the PD Response notifications in PD-before-join case regardless of what config method was requested. This looks like the most consistent and cleanest approach here. This will also require another commit for some cases where the PD Response could have been processed after having already started the scan for the join operation. Commit f63b8542156496ba88ef9f161e5931122d7319b9 makes wpa_supplicant wait for the PD Response prior to continuing the join with the scan-for-connection step. This is also cleaning up the sequence of driver operations in the join step and can make it somewhat easier for some drivers to handle the remain-on-channel/offchannel TX and scan.
Hi Jouni, > I implemented this a bit differently in commit > c19316354ed46e688704c1ebcf1c7efe816ddf31 by skipping the > PD Response notifications in PD-before-join case regardless of > what config method was requested. This looks like the most > consistent and cleanest approach here. > This will also require another commit for some cases where the > PD Response could have been processed after having already > started the scan for the join operation. Commit > f63b8542156496ba88ef9f161e5931122d7319b9 > makes wpa_supplicant wait for the PD Response prior to > continuing the join with the scan-for-connection step. Your commits looks good to me and addresses my requirements. Thanks!! :) - Jithu Jance On Mon, Feb 6, 2012 at 12:32 AM, Jouni Malinen <j@w1.fi> wrote: > On Mon, Dec 26, 2011 at 09:23:54PM -0800, Jithu Jance wrote: > > Sounds like for "p2p_connect <devaddr> pin display join" command, > suppressing Prov Disc SHOW-PIN would be a better > > approach. > > Agreed. > > > I would also like to confirm about the handling of > wpas_notify_p2p_provision_discovery > > in this particular scenario. Is it okay to invoke the function with > generated_pin = 0 value (the below patch uses this approach) > > or should I suppress the invocation of > wpas_notify_p2p_provision_discovery when wpas->p2p_pin is populated?? > > I think it is better to skip this call completely. Actually, these > provision discovery response notifications should really be skipped > completely in the automatic PD-before-join case since those were not > explicitly requested by any external program and they do not really > provide any additional information. > > > diff --git a/wpa_supplicant/p2p_supplicant.c > b/wpa_supplicant/p2p_supplicant.c > > @@ -1785,8 +1785,7 @@ void wpas_prov_disc_req(void *ctx, const u8 *peer, > u16 config_methods, > > - > > - if (config_methods & WPS_CONFIG_DISPLAY) { > > + if ((config_methods & WPS_CONFIG_DISPLAY) && (wpa_s->p2p_pin[0] == > '\0')) { > > generated_pin = wps_generate_pin(); > > Why would we need to modify prov_disc_req callback? This is not used on > the P2P device that will be joining a group as a P2P client. > > > @@ -1809,7 +1808,7 @@ void wpas_prov_disc_resp(void *ctx, const u8 > *peer, u16 config_methods) > > if (config_methods & WPS_CONFIG_DISPLAY) > > wpas_prov_disc_local_keypad(wpa_s, peer, ""); > > - else if (config_methods & WPS_CONFIG_KEYPAD) { > > + else if ((config_methods & WPS_CONFIG_KEYPAD) && > (wpa_s->p2p_pin[0] == '\0')) { > > generated_pin = wps_generate_pin(); > > wpas_prov_disc_local_display(wpa_s, peer, "", > generated_pin); > > } else if (config_methods & WPS_CONFIG_PUSHBUTTON) > > While this could be fine for most cases, I don't really want to depend > on wpa_s->p2p_pin getting cleared in all cases. I guess this could > potentially skip an event message for Provision Discovery that is done > in other contexts. > > I implemented this a bit differently in commit > c19316354ed46e688704c1ebcf1c7efe816ddf31 by skipping the PD Response > notifications in PD-before-join case regardless of what config method > was requested. This looks like the most consistent and cleanest approach > here. > > This will also require another commit for some cases where the PD > Response could have been processed after having already started the scan > for the join operation. Commit f63b8542156496ba88ef9f161e5931122d7319b9 > makes wpa_supplicant wait for the PD Response prior to continuing the > join with the scan-for-connection step. This is also cleaning up the > sequence of driver operations in the join step and can make it somewhat > easier for some drivers to handle the remain-on-channel/offchannel TX > and scan. > > -- > 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 5ff94ef..85be08a 100644 --- a/wpa_supplicant/p2p_supplicant.c +++ b/wpa_supplicant/p2p_supplicant.c @@ -1785,8 +1785,7 @@ void wpas_prov_disc_req(void *ctx, const u8 *peer, u16 config_methods, group ? " group=" : "", group ? group->ifname : ""); params[sizeof(params) - 1] = '\0'; - - if (config_methods & WPS_CONFIG_DISPLAY) { + if ((config_methods & WPS_CONFIG_DISPLAY) && (wpa_s->p2p_pin[0] == '\0')) { generated_pin = wps_generate_pin(); wpas_prov_disc_local_display(wpa_s, peer, params, generated_pin); @@ -1809,7 +1808,7 @@ void wpas_prov_disc_resp(void *ctx, const u8 *peer, u16 config_methods) if (config_methods & WPS_CONFIG_DISPLAY) wpas_prov_disc_local_keypad(wpa_s, peer, ""); - else if (config_methods & WPS_CONFIG_KEYPAD) { + else if ((config_methods & WPS_CONFIG_KEYPAD) && (wpa_s->p2p_pin[0] == '\0')) { generated_pin = wps_generate_pin(); wpas_prov_disc_local_display(wpa_s, peer, "", generated_pin); } else if (config_methods & WPS_CONFIG_PUSHBUTTON)