diff mbox

cfg80211: race problem between suspend and disconnect event

Message ID 1476989132.14078.12.camel@sipsolutions.net
State Not Applicable
Headers show

Commit Message

Johannes Berg Oct. 20, 2016, 6:45 p.m. UTC
Hi,

> Mwifiex driver rejects del_key() requests from cfg80211 during
> suspend. They came very late when driver's cfg80211_suspend handler
> is already executed and driver is in the middle of SDIO's suspend
> handler.

Interesting. Rejecting those calls is probably perfectly reasonable,
and in fact it's not clear to me why we even try to delete the keys
after we've disconnected - any driver implementation should have
removed them already anyway? You probably don't actually care about the
key removal either?

That said though, there's also the critical protocol stop and the
set_qos_map(NULL) call, which removes the QoS mapping. It doesn't look
like you support this right now in your driver, but in any case it'd be
pretty strange to have that happen after or during suspend.

> Please let us know if you have any suggestions to resolves this with
> cfg80211/driver change.

For cfg80211 we could do something like this:



However, that assumes that you actually cfg80211_disconnected()
synchronously while being asked to disconnect, which doesn't seem to be
true from looking at mwifiex, if HostCmd_CMD_802_11_DEAUTHENTICATE
command can be sent to the firmware then you wait for EVENT_LINK_LOST,
EVENT_DEAUTHENTICATED or EVENT_DISASSOCIATED to come back from the
firmware, so this cfg80211 change won't help.


So somehow you'd have to synchronize with the firmware as well, to
process all those things before suspend, I guess?

We could then export cfg80211_process_rdev_events() as
cfg80211_process_wiphy_events() or so, so that you can call that at an
appropriate place from your suspend handler, after having synchronized
with the firmware?

johannes

Comments

Amitkumar Karwar Oct. 21, 2016, 3:48 p.m. UTC | #1
Hi Johannes,

> From: Johannes Berg [mailto:johannes@sipsolutions.net]

> Sent: Friday, October 21, 2016 12:16 AM

> To: Amitkumar Karwar

> Cc: Kalle Valo; Brian Norris; Nishant Sarmukadam; Cathy Luo; linux-

> wireless@vger.kernel.org; Ganapathi Bhat

> Subject: Re: cfg80211: race problem between suspend and disconnect event

> 

> Hi,

> 

> > Mwifiex driver rejects del_key() requests from cfg80211 during

> > suspend. They came very late when driver's cfg80211_suspend handler is

> > already executed and driver is in the middle of SDIO's suspend

> > handler.

> 

> Interesting. Rejecting those calls is probably perfectly reasonable, and

> in fact it's not clear to me why we even try to delete the keys after

> we've disconnected - any driver implementation should have removed them

> already anyway? You probably don't actually care about the key removal

> either?


Thanks for your reply.

Yes. In our case, *802_11_DEAUTHENTICATE command downloaded to firmware takes care of flushing the keys.

I can see below code in cfg80211's disconnect handling. It seems to be there for long time.

---------------------
/*
 * Delete all the keys ... pairwise keys can't really
 * exist any more anyway, but default keys might.
 */
 if (rdev->ops->del_key)
    for (i = 0; i < 6; i++)
       rdev_del_key(rdev, dev, i, false, NULL);
-----------------------

> 

> That said though, there's also the critical protocol stop and the

> set_qos_map(NULL) call, which removes the QoS mapping. It doesn't look

> like you support this right now in your driver, but in any case it'd be

> pretty strange to have that happen after or during suspend.

> 

> > Please let us know if you have any suggestions to resolves this with

> > cfg80211/driver change.

> 

> For cfg80211 we could do something like this:

> 

> --- a/net/wireless/sysfs.c

> +++ b/net/wireless/sysfs.c

> @@ -104,13 +104,16 @@ static int wiphy_suspend(struct device *dev)

> 

>  	rtnl_lock();

>  	if (rdev->wiphy.registered) {

> -		if (!rdev->wiphy.wowlan_config)

> +		if (!rdev->wiphy.wowlan_config) {

>  			cfg80211_leave_all(rdev);

> +			cfg80211_process_rdev_events(rdev);

> +		}

>  		if (rdev->ops->suspend)

>  			ret = rdev_suspend(rdev, rdev->wiphy.wowlan_config);

>  		if (ret == 1) {

>  			/* Driver refuse to configure wowlan */

>  			cfg80211_leave_all(rdev);

> +			cfg80211_process_rdev_events(rdev);

>  			ret = rdev_suspend(rdev, NULL);

>  		}

>  	}

> 

> 

> However, that assumes that you actually cfg80211_disconnected()

> synchronously while being asked to disconnect, which doesn't seem to be

> true from looking at mwifiex, if HostCmd_CMD_802_11_DEAUTHENTICATE

> command can be sent to the firmware then you wait for EVENT_LINK_LOST,

> EVENT_DEAUTHENTICATED or EVENT_DISASSOCIATED to come back from the

> firmware, so this cfg80211 change won't help.

> 


I think, your cfg80211 change will help. We do ensure that cfg80211_disconnected() is called before exiting mwifiex_cfg80211_disconnect().
Sending HostCmd_CMD_802_11_DEAUTHENTICATE command to firmware is a blocking call. cfg80211_disconnected() is called while handling that command's response. 

mwifiex_ret_802_11_deauthenticate()->mwifiex_reset_connect_state()->cfg80211_disconnected()

> 

> So somehow you'd have to synchronize with the firmware as well, to

> process all those things before suspend, I guess?

> 

> We could then export cfg80211_process_rdev_events() as

> cfg80211_process_wiphy_events() or so, so that you can call that at an

> appropriate place from your suspend handler, after having synchronized

> with the firmware?


This would not be needed. 

Regards,
Amitkumar
Johannes Berg Oct. 21, 2016, 9:24 p.m. UTC | #2
> Yes. In our case, *802_11_DEAUTHENTICATE command downloaded to
> firmware takes care of flushing the keys.
> 
> I can see below code in cfg80211's disconnect handling. It seems to
> be there for long time.

Yeah, I saw it, but it's not clear to me that there's much point in it.
In any case, that's an unrelated question in a way, because there
definitely are things happening here that should be "more synchronous",
regardless of whether or not the key deletion makes sense.

> I think, your cfg80211 change will help. We do ensure that
> cfg80211_disconnected() is called before exiting
> mwifiex_cfg80211_disconnect().
> Sending HostCmd_CMD_802_11_DEAUTHENTICATE command to firmware is a
> blocking call. cfg80211_disconnected() is called while handling that
> command's response. 

Ah ok, I missed that - I thought it was asynchronous.

> > So somehow you'd have to synchronize with the firmware as well, to
> > process all those things before suspend, I guess?
> > 
[...]
> This would not be needed. 

Right. Care to test my patch before I properly submit it?

johannes
Amitkumar Karwar Oct. 24, 2016, 2:54 p.m. UTC | #3
Hi Johannes,

> From: Johannes Berg [mailto:johannes@sipsolutions.net]

> Sent: Saturday, October 22, 2016 2:54 AM

> To: Amitkumar Karwar

> Cc: Kalle Valo; Brian Norris; Nishant Sarmukadam; Cathy Luo; linux-

> wireless@vger.kernel.org; Ganapathi Bhat

> Subject: Re: cfg80211: race problem between suspend and disconnect event

> 

> 

> > Yes. In our case, *802_11_DEAUTHENTICATE command downloaded to

> > firmware takes care of flushing the keys.

> >

> > I can see below code in cfg80211's disconnect handling. It seems to be

> > there for long time.

> 

> Yeah, I saw it, but it's not clear to me that there's much point in it.

> In any case, that's an unrelated question in a way, because there

> definitely are things happening here that should be "more synchronous",

> regardless of whether or not the key deletion makes sense.

> 

> > I think, your cfg80211 change will help. We do ensure that

> > cfg80211_disconnected() is called before exiting

> > mwifiex_cfg80211_disconnect().

> > Sending HostCmd_CMD_802_11_DEAUTHENTICATE command to firmware is a

> > blocking call. cfg80211_disconnected() is called while handling that

> > command's response.

> 

> Ah ok, I missed that - I thought it was asynchronous.

> 

> > > So somehow you'd have to synchronize with the firmware as well, to

> > > process all those things before suspend, I guess?

> > >

> [...]

> > This would not be needed.

> 

> Right. Care to test my patch before I properly submit it?


We ran the tests and confirmed that your patch does solve the problem. You can submit it.

Regards,
Amitkumar
diff mbox

Patch

--- a/net/wireless/sysfs.c
+++ b/net/wireless/sysfs.c
@@ -104,13 +104,16 @@  static int wiphy_suspend(struct device *dev)
 
 	rtnl_lock();
 	if (rdev->wiphy.registered) {
-		if (!rdev->wiphy.wowlan_config)
+		if (!rdev->wiphy.wowlan_config) {
 			cfg80211_leave_all(rdev);
+			cfg80211_process_rdev_events(rdev);
+		}
 		if (rdev->ops->suspend)
 			ret = rdev_suspend(rdev, rdev->wiphy.wowlan_config);
 		if (ret == 1) {
 			/* Driver refuse to configure wowlan */
 			cfg80211_leave_all(rdev);
+			cfg80211_process_rdev_events(rdev);
 			ret = rdev_suspend(rdev, NULL);
 		}
 	}