diff mbox series

[4/8] iwlwifi: mvm: fix race in sync rx queue notification

Message ID 20191004131414.27372-5-luca@coelho.fi (mailing list archive)
State Accepted
Delegated to: Luca Coelho
Headers show
Series iwlwifi: fixes intended for 5.4 2019-10-04 | expand

Commit Message

Luca Coelho Oct. 4, 2019, 1:14 p.m. UTC 
From: Naftali Goldstein <naftali.goldstein@intel.com>

Consider the following flow:
 1. Driver starts to sync the rx queues due to a delba.
    mvm->queue_sync_cookie=1.
    This rx-queues-sync is synchronous, so it doesn't increment the
    cookie until all rx queues handle the notification from FW.
 2. During this time, driver starts to sync rx queues due to nssn sync
    required.
    The cookie's value is still 1, but it doesn't matter since this
    rx-queue-sync is non-synchronous so in the notification handler the
    cookie is ignored.
    What _does_ matter is that this flow increments the cookie to 2
    immediately.
    Remember though that the FW won't start servicing this command until
    it's done with the previous one.
 3. FW is still handling the first command, so it sends a notification
    with internal_notif->sync=1, and internal_notif->cookie=0, which
    triggers a WARN_ONCE.

The solution for this race is to only use the mvm->queue_sync_cookie in
case of a synchronous sync-rx-queues. This way in step 2 the cookie's
value won't change so we avoid the WARN.

The commit in the "fixes" field is the first commit to introduce
non-synchronous sending of this command to FW.

Signed-off-by: Naftali Goldstein <naftali.goldstein@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Kalle Valo Oct. 4, 2019, 1:41 p.m. UTC | #1 
Luca Coelho <luca@coelho.fi> writes:

> From: Naftali Goldstein <naftali.goldstein@intel.com>
>
> Consider the following flow:
>  1. Driver starts to sync the rx queues due to a delba.
>     mvm->queue_sync_cookie=1.
>     This rx-queues-sync is synchronous, so it doesn't increment the
>     cookie until all rx queues handle the notification from FW.
>  2. During this time, driver starts to sync rx queues due to nssn sync
>     required.
>     The cookie's value is still 1, but it doesn't matter since this
>     rx-queue-sync is non-synchronous so in the notification handler the
>     cookie is ignored.
>     What _does_ matter is that this flow increments the cookie to 2
>     immediately.
>     Remember though that the FW won't start servicing this command until
>     it's done with the previous one.
>  3. FW is still handling the first command, so it sends a notification
>     with internal_notif->sync=1, and internal_notif->cookie=0, which
>     triggers a WARN_ONCE.
>
> The solution for this race is to only use the mvm->queue_sync_cookie in
> case of a synchronous sync-rx-queues. This way in step 2 the cookie's
> value won't change so we avoid the WARN.
>
> The commit in the "fixes" field is the first commit to introduce
> non-synchronous sending of this command to FW.

But I don't see a Fixes field anywhere :)
Luca Coelho Oct. 4, 2019, 6:06 p.m. UTC | #2 
On Fri, 2019-10-04 at 16:41 +0300, Kalle Valo wrote:
> Luca Coelho <luca@coelho.fi> writes:
> 
> > From: Naftali Goldstein <naftali.goldstein@intel.com>
> > 
> > Consider the following flow:
> >  1. Driver starts to sync the rx queues due to a delba.
> >     mvm->queue_sync_cookie=1.
> >     This rx-queues-sync is synchronous, so it doesn't increment the
> >     cookie until all rx queues handle the notification from FW.
> >  2. During this time, driver starts to sync rx queues due to nssn sync
> >     required.
> >     The cookie's value is still 1, but it doesn't matter since this
> >     rx-queue-sync is non-synchronous so in the notification handler the
> >     cookie is ignored.
> >     What _does_ matter is that this flow increments the cookie to 2
> >     immediately.
> >     Remember though that the FW won't start servicing this command until
> >     it's done with the previous one.
> >  3. FW is still handling the first command, so it sends a notification
> >     with internal_notif->sync=1, and internal_notif->cookie=0, which
> >     triggers a WARN_ONCE.
> > 
> > The solution for this race is to only use the mvm->queue_sync_cookie in
> > case of a synchronous sync-rx-queues. This way in step 2 the cookie's
> > value won't change so we avoid the WARN.
> > 
> > The commit in the "fixes" field is the first commit to introduce
> > non-synchronous sending of this command to FW.
> 
> But I don't see a Fixes field anywhere :)

Hmmm, good catch.  My script should have added it.  One more thing to
check... *sigh*

This is the aforementioned commit:

Fixes: 3c514bf831ac ("iwlwifi: mvm: add a loose synchronization of the NSSN across Rx queues")

I'll add it and include it when I send the pull-req.

Thanks!

--
Cheers,
Luca.
diff mbox series

Patch

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
index cd1b10042fbf..d31f96c3f925 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
@@ -4881,11 +4881,11 @@  void iwl_mvm_sync_rx_queues_internal(struct iwl_mvm *mvm,
 	if (!iwl_mvm_has_new_rx_api(mvm))
 		return;
 
-	notif->cookie = mvm->queue_sync_cookie;
-
-	if (notif->sync)
+	if (notif->sync) {
+		notif->cookie = mvm->queue_sync_cookie;
 		atomic_set(&mvm->queue_sync_counter,
 			   mvm->trans->num_rx_queues);
+	}
 
 	ret = iwl_mvm_notify_rx_queue(mvm, qmask, (u8 *)notif,
 				      size, !notif->sync);
@@ -4905,7 +4905,8 @@  void iwl_mvm_sync_rx_queues_internal(struct iwl_mvm *mvm,
 
 out:
 	atomic_set(&mvm->queue_sync_counter, 0);
-	mvm->queue_sync_cookie++;
+	if (notif->sync)
+		mvm->queue_sync_cookie++;
 }
 
 static void iwl_mvm_sync_rx_queues(struct ieee80211_hw *hw)