P2P: Fix inconsistency in displaying wps pin

>>> In this particular sequence, I don't really see much point in even
>>> displaying the P2P-PROV-DISC-SHOW-PIN event. In fact, it does not
>>> necessarily even get shown if the Provision Discovery Response is not
>>> received

> but the UI program that issued the
> p2p_connect command in the first place better know which end is going to
> be displaying the PIN and it should be able to figure out easily that
> the value returned by p2p_connect is this.

> there is no
> guarantee on this event being shown in this join-a-running-group case.. 

Sounds like for "p2p_connect <devaddr> pin display join" command, suppressing Prov Disc SHOW-PIN would be a better
approach. 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??


Signed-hostap: Jithu Jance <jithu@broadcom.com>

---
 wpa_supplicant/p2p_supplicant.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

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
>