Subject: [PATCH] P2P Provision discovery to be sent to GO operating freq

[Jouni Malinen]

On Fri, Oct 21, 2011 at 11:45:08AM +0530, Neeraj Garg wrote:
> In some occurrences, when listen_freq and the final operating frequency of
> the GO are different, then the current supplicant code in p2p_pd.c ends up
> sending provision discovery request frame to the listen frequency. Since GO
> is no longer listening on listen_freq (it has moved to oper channel), frame
> doesn't get received by GO.

While the operating channel can indeed be different from the listen
channel, I would like to understand better when dev->listen_freq would
not be the GO's operating channel. I guess this could happen if the GO
is found again from its listen channel after having first been found
from operating channel.. Is that the sequence you were thinking of here?

> Supplicant code in p2p.c update the listen_freq and the operating frequency
> both in the add_device context. But at the time of sending the provision
> discovery, it checks if listen_freq is 0 or not.

Please note that listen_freq is set to the same value as oper_freq when
the GO is discovered. For oper_freq to be non-zero and different from
listen_freq, the P2P Device that operates as GO would need to be
discovered in Listen state (in non-GO role) on another channel..

> I have created a patch for this problem. In the add_device context, we will
> update the go_state as REMOTE_GO.

This does not sound correct. go_state is a variable used for GO
Negotiation and changing it or using it in this way can break something.

> diff --git a/src/p2p/p2p.c b/src/p2p/p2p.c
> @@ -574,8 +574,10 @@ int p2p_add_device(struct p2p_data *p2p, const u8
> *addr, int freq, int level,
>   freq, msg.ds_params ? *msg.ds_params : -1);
>   }
>   dev->listen_freq = freq;
> - if (msg.group_info)
> + if (msg.group_info) {
>   dev->oper_freq = freq;
> + dev->go_state = REMOTE_GO;
> + }

I don't think this is correct.

> diff --git a/src/p2p/p2p_pd.c b/src/p2p/p2p_pd.c
> @@ -283,8 +283,10 @@ int p2p_send_prov_disc_req(struct p2p_data *p2p, struct
> p2p_device *dev,
>  {
> - freq = dev->listen_freq > 0 ? dev->listen_freq : dev->oper_freq;
> + if (dev->go_state == REMOTE_GO)
> + freq = dev->oper_freq;
> + else
> + freq = dev->listen_freq > 0 ? dev->listen_freq : dev->oper_freq;

go_state cannot be used here, so if there is an error case, that should
be fixed differently. For most cases, I would expect dev->listen_freq to
be the correct one to use. However, there is a possible race where
listen_freq could be updated after having set oper_freq. I'd guess this
would mainly (only?) affect the case where Provision Discovery is used
to inform an already running GO that we are about to join. For that, it
could be enough to do something like

    if (join && dev->oper_freq)
	freq = dev->oper_freq;

Or well, even more robust mechanism would be to pass in the operating
frequency to p2p_send_prov_disc_req() function based on the latest scan
results since dev->oper_freq is not really guaranteed to be current (and
a P2P Device could even theoretically be running multiple groups on
different operating channels at the same time).
 
-- 
Jouni Malinen                                            PGP id EFC895FA