| Message ID | 1476989132.14078.12.camel@sipsolutions.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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
> 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
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
--- 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); } }