diff mbox

Add an option to suppress provision discovery during P2P Join

Message ID 6C370B347C3FE8438C9692873287D2E1195AE35D60@SJEXCHCCR01.corp.ad.broadcom.com
State Superseded
Headers show

Commit Message

Jithu Jance Nov. 21, 2011, 10:54 a.m. UTC 
This patch adds an option to suppress provision discovery req during P2P Join. This is required in scenarios where a P2P client is connecting to a P2P GO that supports only PIN DISPLAY option. 
The client side has to use the PIN displayed by the GO and we can't issue a p2p_connect on the client side till the PIN is known(since pin has to be entered along with p2p_connect command).

so in this case, we have to issue an explicit Provision Discovery before issuing connect command so that the GO can display its PIN and we can use the displayed pin in the subsequent p2p_connect command. 
Since we have already done the PD, the "p2p_connect join" command shouldn't initiate another provision discovery.

Please see whether the patch is okay.

Also, Please share your thoughts on adding an extra "no_pd" argument to p2p_connect command. 


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

---
 wpa_supplicant/config.c                     |    1 +
 wpa_supplicant/config.h                     |    5 +++++
 wpa_supplicant/config_file.c                |    3 +++
 wpa_supplicant/config_winreg.c              |    4 ++++
 wpa_supplicant/dbus/dbus_new_handlers_p2p.c |    8 ++++++++
 wpa_supplicant/p2p_supplicant.c             |   12 ++++++++++++
 6 files changed, 33 insertions(+), 0 deletions(-)

Comments

Jouni Malinen Nov. 21, 2011, 4:50 p.m. UTC | #1 
On Mon, Nov 21, 2011 at 02:54:27AM -0800, Jithu Jance wrote:
> This patch adds an option to suppress provision discovery req during P2P Join. This is required in scenarios where a P2P client is connecting to a P2P GO that supports only PIN DISPLAY option. 
> The client side has to use the PIN displayed by the GO and we can't issue a p2p_connect on the client side till the PIN is known(since pin has to be entered along with p2p_connect command).
> 
> so in this case, we have to issue an explicit Provision Discovery before issuing connect command so that the GO can display its PIN and we can use the displayed pin in the subsequent p2p_connect command. 
> Since we have already done the PD, the "p2p_connect join" command shouldn't initiate another provision discovery.

This looks somewhat problematic and would likely not comply with the P2P
specification requirements. How are you sending the Provision Discovery
Request prior to p2p_connect join command? Please note that the Group ID
attribute is added only in the p2p_connect join case and as such, a
separate p2p_prov_disc command prior to p2p_connect would not actually
send correct Provision Discovery Request frame.

> Also, Please share your thoughts on adding an extra "no_pd" argument to p2p_connect command. 

That approach would be quite a bit easier to understand while I have
problems figuring out why this patch is adding a new configuration
parameter for unconditionally dropping the Provision Discovery prior to
joining a group.

I think this functionality would likely require an extension to
p2p_prov_disc command to allow it to specify which group is being used
(without this, Provision Discovery Request is for the purpose of forming
a new group; not for joining an existing group). With that added, it
should be easy to handle the rest automatically, i.e., p2p_connect join
should figure out whether to send Provision Discovery Request based on
whether p2p_prov_disc was used prior to it with the new parameter to
indicate which P2P group to target with the Provision Discovery Request.

> diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
> @@ -763,6 +763,7 @@ static void wpas_p2p_clone_config(struct wpa_supplicant *dst,
> @@ -2503,6 +2504,17 @@ static void wpas_p2p_scan_res_join(struct wpa_supplicant *wpa_s,
>  			break;
>  		}
>  
> +		if (wpa_s->conf->p2p_suppress_pd) {
> +			/* If Provision Discovery is suppressed,
> +			 * Start join operation immediately. The Provision
> +			 * discovery in this case would have been explicitly
> +			 * initiated before the p2p_connect command.
> +			 */
> +			wpa_printf(MSG_DEBUG, "P2P: Provision "
> +				"Discovery Request from Join context suppressed");
> +			goto start;
> +		}

This does not look correct. P2P specification requires the Provision
Discovery Request to be sent prior to joining a running group. I see no
point in having a global configuration option for doing this. It would
be way too easy to result in non-compliant behavior with this.

If this Provision Discovery Request needs to be skipped, the decision to
do show better be conditional on a prior operation. In other words, this
could either be done automatically based on an earlier Provision
Discovery operation having been executed recently with the target GO or
at least based on a parameter to p2p_connect.

As a side note, this place to skip the p2p_prov_disc_req() call does not
look correct. pending_pd_before_join should not be set in that case (nor
is there much point in printing the debug message claiming that the
Provision Discovery Request is to be sent if that part is skipped).
Jithu Jance Nov. 22, 2011, 11:10 a.m. UTC | #2 
Hi Jouni,

I may not have understood the functionality correctly. Please Correct me, if I am wrong. 
Consider a scenario where a P2P client wants to Join P2P GO whose wps config method is
advertised as DISPLAY.

In this case, I think we have to use explicit provision discovery before issuing 
p2p_connect. The explicit discovery is used to retreive the PIN from GO side. 
Below mentioned are the commands that i have used. is there any other way to achieve this??


  Device A (P2P Client)					Device B (P2P GO with PIN DISPLAY)
-------------------------------------------------------------------------------------------
p2p_find

P2p_prov_disc <GO_DEV_ADDRESS> display           

                    			      PROV_DISC_SHOW_PIN EVENT >>> DISPLAY GO's PIN																			

   p2p_connect <GO_DEV_ADDR> <GO_PIN> join
   [since we have already done the PD, I think we shouldn't repeat it. 
    Otherwise the GO may regenerate the PIN on receipt of prov disc request.]

> How are you sending the Provision Discovery
> Request prior to p2p_connect join command? Please note that the Group ID
> attribute is added only in the p2p_connect join case and as such, a
> separate p2p_prov_disc command prior to p2p_connect would not actually
> send correct Provision Discovery Request frame.
The command that I have used is p2p_prov_disc <go_dev_addr> <req_config_method>
The prov disc req is sent from Client's p2p_device address interface. The group id
will only be created on the subsequent p2p_connect command. Is group id really 
required in this scenario??. Correct me, if my understanding is wrong.


> I see no point in having a global configuration option for doing this. It would
> be way too easy to result in non-compliant behavior with this.
> If this Provision Discovery Request needs to be skipped, the decision to
> do show better be conditional on a prior operation. In other words, this
> could either be done automatically based on an earlier Provision
> Discovery operation having been executed recently with the target GO or
> at least based on a parameter to p2p_connect.

Agree with you. The sending of provision discovery should be based on the availablity of provisioning info. 
But, if we store the provisioning info, when should it be cleared/reset?? Is it safe to assume that the 
provisioning info received for a particular GO will hold good till there is a change in advertised Config methods
or till DEVICE_LOST event??






- Jithu Jance

-----Original Message-----
From: hostap-bounces@lists.shmoo.com [mailto:hostap-bounces@lists.shmoo.com] On Behalf Of Jouni Malinen
Sent: Monday, November 21, 2011 10:20 PM
To: hostap@lists.shmoo.com
Subject: Re: [PATCH] Add an option to suppress provision discovery during P2P Join

On Mon, Nov 21, 2011 at 02:54:27AM -0800, Jithu Jance wrote:
> This patch adds an option to suppress provision discovery req during P2P Join. This is required in scenarios where a P2P client is connecting to a P2P GO that supports only PIN DISPLAY option. 
> The client side has to use the PIN displayed by the GO and we can't issue a p2p_connect on the client side till the PIN is known(since pin has to be entered along with p2p_connect command).
> 
> so in this case, we have to issue an explicit Provision Discovery before issuing connect command so that the GO can display its PIN and we can use the displayed pin in the subsequent p2p_connect command. 
> Since we have already done the PD, the "p2p_connect join" command shouldn't initiate another provision discovery.

This looks somewhat problematic and would likely not comply with the P2P
specification requirements. How are you sending the Provision Discovery
Request prior to p2p_connect join command? Please note that the Group ID
attribute is added only in the p2p_connect join case and as such, a
separate p2p_prov_disc command prior to p2p_connect would not actually
send correct Provision Discovery Request frame.

> Also, Please share your thoughts on adding an extra "no_pd" argument to p2p_connect command. 

That approach would be quite a bit easier to understand while I have
problems figuring out why this patch is adding a new configuration
parameter for unconditionally dropping the Provision Discovery prior to
joining a group.

I think this functionality would likely require an extension to
p2p_prov_disc command to allow it to specify which group is being used
(without this, Provision Discovery Request is for the purpose of forming
a new group; not for joining an existing group). With that added, it
should be easy to handle the rest automatically, i.e., p2p_connect join
should figure out whether to send Provision Discovery Request based on
whether p2p_prov_disc was used prior to it with the new parameter to
indicate which P2P group to target with the Provision Discovery Request.

> diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
> @@ -763,6 +763,7 @@ static void wpas_p2p_clone_config(struct wpa_supplicant *dst,
> @@ -2503,6 +2504,17 @@ static void wpas_p2p_scan_res_join(struct wpa_supplicant *wpa_s,
>  			break;
>  		}
>  
> +		if (wpa_s->conf->p2p_suppress_pd) {
> +			/* If Provision Discovery is suppressed,
> +			 * Start join operation immediately. The Provision
> +			 * discovery in this case would have been explicitly
> +			 * initiated before the p2p_connect command.
> +			 */
> +			wpa_printf(MSG_DEBUG, "P2P: Provision "
> +				"Discovery Request from Join context suppressed");
> +			goto start;
> +		}

This does not look correct. P2P specification requires the Provision
Discovery Request to be sent prior to joining a running group. I see no
point in having a global configuration option for doing this. It would
be way too easy to result in non-compliant behavior with this.

If this Provision Discovery Request needs to be skipped, the decision to
do show better be conditional on a prior operation. In other words, this
could either be done automatically based on an earlier Provision
Discovery operation having been executed recently with the target GO or
at least based on a parameter to p2p_connect.

As a side note, this place to skip the p2p_prov_disc_req() call does not
look correct. pending_pd_before_join should not be set in that case (nor
is there much point in printing the debug message claiming that the
Provision Discovery Request is to be sent if that part is skipped).
Jouni Malinen Nov. 22, 2011, 3:05 p.m. UTC | #3 
On Tue, Nov 22, 2011 at 03:10:57AM -0800, Jithu Jance wrote:
> Consider a scenario where a P2P client wants to Join P2P GO whose wps config method is
> advertised as DISPLAY.
> 
> In this case, I think we have to use explicit provision discovery before issuing 
> p2p_connect. The explicit discovery is used to retreive the PIN from GO side. 

Well, this depends on the GO implementation. Some GO devices may have a
PIN displayed somehow all the time. Anyway, yes, there may be cases
where a Provision Discovery request may be needed.

>   Device A (P2P Client)					Device B (P2P GO with PIN DISPLAY)
> -------------------------------------------------------------------------------------------
> p2p_find
> 
> P2p_prov_disc <GO_DEV_ADDRESS> display           
> 
>                     			      PROV_DISC_SHOW_PIN EVENT >>> DISPLAY GO's PIN																			

This is not correct. The Provision Discovery Request sent by
p2p_prov_disc is for the purpose of starting group formation while you
are trying to get the already running GO to display a PIN. The
difference is in whether the Group Id attribute is included in the
message (it needs to be included for this join-a-group case, but that
p2p_prov_disc command does not add it).

>    p2p_connect <GO_DEV_ADDR> <GO_PIN> join
>    [since we have already done the PD, I think we shouldn't repeat it. 
>     Otherwise the GO may regenerate the PIN on receipt of prov disc request.]

If the previous step had actually sent a Provision Discovery Request
with Group Id attribute, I would agree with this. However, it didn't and
as such, leaving out the correct frame here would result in incorrect
behavior. Anyway, the proper way to address this is to extend
p2p_prov_disc behavior

> The command that I have used is p2p_prov_disc <go_dev_addr> <req_config_method>
> The prov disc req is sent from Client's p2p_device address interface. The group id
> will only be created on the subsequent p2p_connect command. Is group id really 
> required in this scenario??. Correct me, if my understanding is wrong.

Yes, the Group Id is required in Provision Discovery Request that is
sent prior to joining a running group. Since the group is already
running, the Group Id is known and no new Group Id gets created with
p2p_connect. If you were talking about forming a new group, the Group Id
would indeed be known only after the p2p_connect (and GO Negotiation).

> Agree with you. The sending of provision discovery should be based on the availablity of provisioning info. 
> But, if we store the provisioning info, when should it be cleared/reset?? Is it safe to assume that the 
> provisioning info received for a particular GO will hold good till there is a change in advertised Config methods
> or till DEVICE_LOST event??

For this particular case of joining a running group, I think it would be
fine to store the information when the extended p2p_prov_disc command is
used and that could then be used on the following p2p_connect join
operation. In theory, there could be some kind of time limit added to
clear the configuration (e.g., the 2 minute WPS walk time), but other
than that, I don't see any particular need for additional clearing of
this information.
diff mbox

Patch

diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c
index b446a3f..9a4f452 100644
--- a/wpa_supplicant/config.c
+++ b/wpa_supplicant/config.c
@@ -2480,6 +2480,7 @@  static const struct global_parse_data global_fields[] = {
 	{ INT_RANGE(persistent_reconnect, 0, 1), 0 },
 	{ INT_RANGE(p2p_intra_bss, 0, 1), CFG_CHANGED_P2P_INTRA_BSS },
 	{ INT(p2p_group_idle), 0 },
+	{ INT(p2p_suppress_pd), 0 },
 #endif /* CONFIG_P2P */
 	{ FUNC(country), CFG_CHANGED_COUNTRY },
 	{ INT(bss_max_count), 0 },
diff --git a/wpa_supplicant/config.h b/wpa_supplicant/config.h
index ae496ff..5080a7e 100644
--- a/wpa_supplicant/config.h
+++ b/wpa_supplicant/config.h
@@ -382,6 +382,11 @@  struct wpa_config {
 	unsigned int p2p_group_idle;
 
 	/**
+	 * p2p_suppress_pd - Suppress Provision Discovery during P2P Join.
+	 */
+	unsigned int p2p_suppress_pd;
+
+	/**
 	 * bss_max_count - Maximum number of BSS entries to keep in memory
 	 */
 	unsigned int bss_max_count;
diff --git a/wpa_supplicant/config_file.c b/wpa_supplicant/config_file.c
index ee8b451..6fda669 100644
--- a/wpa_supplicant/config_file.c
+++ b/wpa_supplicant/config_file.c
@@ -683,6 +683,9 @@  static void wpa_config_write_global(FILE *f, struct wpa_config *config)
 		fprintf(f, "p2p_intra_bss=%u\n", config->p2p_intra_bss);
 	if (config->p2p_group_idle)
 		fprintf(f, "p2p_group_idle=%u\n", config->p2p_group_idle);
+	if (config->p2p_suppress_pd)
+		fprintf(f, "p2p_suppress_pd=%u\n",
+			config->p2p_suppress_pd);
 #endif /* CONFIG_P2P */
 	if (config->country[0] && config->country[1]) {
 		fprintf(f, "country=%c%c\n",
diff --git a/wpa_supplicant/config_winreg.c b/wpa_supplicant/config_winreg.c
index ea3a2ac..104d6b7 100644
--- a/wpa_supplicant/config_winreg.c
+++ b/wpa_supplicant/config_winreg.c
@@ -266,6 +266,8 @@  static int wpa_config_read_global(struct wpa_config *config, HKEY hk)
 		hk, TEXT("p2p_ssid_postfix"));
 	wpa_config_read_reg_dword(hk, TEXT("p2p_group_idle"),
 				  (int *) &config->p2p_group_idle);
+	wpa_config_read_reg_dword(hk, TEXT("p2p_suppress_pd"),
+				  (int *) &config->p2p_suppress_pd);
 #endif /* CONFIG_P2P */
 
 	wpa_config_read_reg_dword(hk, TEXT("bss_max_count"),
@@ -612,6 +614,8 @@  static int wpa_config_write_global(struct wpa_config *config, HKEY hk)
 				    config->p2p_ssid_postfix);
 	wpa_config_write_reg_dword(hk, TEXT("p2p_group_idle"),
 				   config->p2p_group_idle, 0);
+	wpa_config_write_reg_dword(hk, TEXT("p2p_suppress_pd"),
+				   config->p2p_suppress_pd, 0);
 #endif /* CONFIG_P2P */
 
 	wpa_config_write_reg_dword(hk, TEXT("bss_max_count"),
diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
index f2c5a18..cb336a3 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
+++ b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
@@ -822,6 +822,11 @@  dbus_bool_t wpas_dbus_getter_p2p_device_properties(DBusMessageIter *iter,
 					 wpa_s->conf->p2p_group_idle))
 		goto err_no_mem;
 
+	/* Enable Provision Discovery for P2P Join */
+	if (!wpa_dbus_dict_append_uint32(&dict_iter, "SuppressPd",
+					 wpa_s->conf->p2p_suppress_pd))
+		goto err_no_mem;
+
 	/* Dissasociation low ack */
 	if (!wpa_dbus_dict_append_uint32(&dict_iter, "disassoc_low_ack",
 					 wpa_s->conf->disassoc_low_ack))
@@ -973,6 +978,9 @@  dbus_bool_t wpas_dbus_setter_p2p_device_properties(DBusMessageIter *iter,
 		} else if ((os_strcmp(entry.key, "GroupIdle") == 0) &&
 			   (entry.type == DBUS_TYPE_UINT32))
 			wpa_s->conf->p2p_group_idle = entry.uint32_value;
+		} else if ((os_strcmp(entry.key, "SuppressPd") == 0) &&
+			   (entry.type == DBUS_TYPE_UINT32))
+			wpa_s->conf->p2p_suppress_pd = entry.uint32_value;
 		else if (os_strcmp(entry.key, "disassoc_low_ack") == 0 &&
 			 entry.type == DBUS_TYPE_UINT32)
 			wpa_s->conf->disassoc_low_ack = entry.uint32_value;
diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
index c6484af..2382953 100644
--- a/wpa_supplicant/p2p_supplicant.c
+++ b/wpa_supplicant/p2p_supplicant.c
@@ -763,6 +763,7 @@  static void wpas_p2p_clone_config(struct wpa_supplicant *dst,
 	d->num_sec_device_types = s->num_sec_device_types;
 
 	d->p2p_group_idle = s->p2p_group_idle;
+	d->p2p_suppress_pd = s->p2p_suppress_pd;
 	d->p2p_intra_bss = s->p2p_intra_bss;
 	d->persistent_reconnect = s->persistent_reconnect;
 }
@@ -2503,6 +2504,17 @@  static void wpas_p2p_scan_res_join(struct wpa_supplicant *wpa_s,
 			break;
 		}
 
+		if (wpa_s->conf->p2p_suppress_pd) {
+			/* If Provision Discovery is suppressed,
+			 * Start join operation immediately. The Provision
+			 * discovery in this case would have been explicitly
+			 * initiated before the p2p_connect command.
+			 */
+			wpa_printf(MSG_DEBUG, "P2P: Provision "
+				"Discovery Request from Join context suppressed");
+			goto start;
+		}
+
 		if (p2p_prov_disc_req(wpa_s->global->p2p,
 				      wpa_s->pending_join_dev_addr, method, 1)
 		    < 0) {