mwifiex: fix S0ix/AP-scanning after suspend by sw method instead of hw

[kitakar5525]

Hi, I've just opened a new branch that tries to fix mwifiex suspend issues in a more simple way.

Issues in upstream mwifiex regarding suspend:

• reportedly breaks S0ix on suspend
• sometimes breaks AP scanning after suspend

The previous patch mwifiex_pcie: remove()/probe() card on suspend()/resume() does remove()/probe() card on suspend()/resume(). This may be too aggressive.
The new patch mwifiex: pcie: use shutdown_sw()/reinit_sw() on suspend()/resume() tries to fix by instead sw-based way (simpler/milder)

At least I confirmed that AP scanning always works after suspend on SB1.
However, I can't confirm S0ix because SB1 can't achieve S0ix by other unknown issues anyway.

So, first, I want you folks to test if S0ix still works after this PR.
Thanks.

Note that this is not yet for upstreaming (e.g. comment at top of suspend()/resume() functions, debug output, etc.)

EDIT: the second patch mwifiex: fix mwifiex_shutdown_sw() causing sw reset failure is required for the main part of this PR (mwifiex: pcie: use shutdown_sw()/reinit_sw() on suspend()/resume())

[kitakar5525]

Ah, typo in commit message. "by some reasons" -> "for some reasons"
Fix it on next rebase.

EDIT: fixed.

[kitakar5525]

mwifiex: fix mwifiex_shutdown_sw() causing sw reset failure

Maybe adding Fixes tag is better:

Fixes: 4c5dae5 ("mwifiex: add PCIe function level reset support")

EDIT: added.

[qzed]

So, first, I want you folks to test if S0ix still works after this PR.

On my SB2, S0ix still seems to work.

[qzed]

Otherwise, this does look good to me! Maybe add that this will disable wakeups on the device in your commit message. This might also reset some internal state.

[kitakar5525]

Maybe add that this will disable wakeups on the device in your commit message. This might also reset some internal state.

Right. I'll write it clearly.

[kitakar5525]

Hi, I just rebased the third (main) patch.

• Changed comments at top of suspend()/resume() for upstreaming

• Removed (kind of debug) outputs from suspend()/resume() that indicated using shutdown_sw()/reinit_sw() instead

• Improved commit message
  Especially, here:

  As a side effect, this patch disables wakeups (means can't use
  Wake-On-WLAN anymore, if it was working before), and might also
  reset some internal states.
  

  and here:

  - Removed mwifiex_enable_wake()/mwifiex_disable_wake()
    as incompatible and redundant. Shutdowned driver can't cause a wakeup.
  

By the way, is this comment Shutdowned driver can't cause a wakeup. true? and how do you think if preserving mwifiex_enable_wake()/mwifiex_disable_wake() is useful? (I've currently deleted as the commit message states)

Also, currently, suspend() performs mwifiex_shutdown_sw() unconditionally. But may be better to check if it's already suspended or not, as Host Sleep checks like this:

kernel/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c

Lines 538 to 542 in d569b8e

	if (adapter->hs_activated) {
	mwifiex_dbg(adapter, CMD,
	"cmd: HS Already activated\n");
	return true;
	}

I'll do this next.

[kitakar5525]

Also, currently, suspend() performs mwifiex_shutdown_sw() unconditionally. But may be better to check if it's already suspended or not, as Host Sleep checks like this:

kernel/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c

Lines 538 to 542 in d569b8e

	if (adapter->hs_activated) {
	mwifiex_dbg(adapter, CMD,
	"cmd: HS Already activated\n");
	return true;
	}

I'll do this next.

That said, why do we need to check if device is already suspended/resumed as upstream does?

kernel/drivers/net/wireless/marvell/mwifiex/pcie.c

Lines 206 to 210 in 25501d3

	if (!test_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags)) {
	mwifiex_dbg(adapter, WARN,
	"Device already resumed\n");
	return 0;
	}

If it's useless, I'd like to remove the check instead.

[qzed]

By the way, is this comment Shutdowned driver can't cause a wakeup. true?

I can only guess. If it works the way I think it works, then the WiFi controller would normally keep on working (in a suspended mode), so essentially be powered on. If it detects a wakeup package, it'll signal a wakeup via the IRQ (I think that's normally a GPIO pin or something like that). If we now shut down the card, the EC firmware might not be running any more, so it won't detect and issue wakeups. At least that's my theory. Without knowing more about either how those things usually work or how the mwifiex chip works in particular, all of this might be wrong of course.

Oh and it should be Shut down driver, I think (past tense of "shut down" is also "shut down").

and how do you think if preserving mwifiex_enable_wake()/mwifiex_disable_wake() is useful? (I've currently deleted as the commit message states)

I would keep them removed, for now at least. If the EC can't issue any wakeups when shut down, having them there could cause the wrong impression (i.e. that wakeups are still expected to work). In the worst-case scenario it could also cause spurious wakeups if the EC would be not properly configured for suspend, but I kind of doubt that.

That said, why do we need to check if device is already suspended/resumed as upstream does?

I honestly don't know. is it possible that something like runtime-suspend could enter Host Suspend while the device is still active, and suspend() then cause a "double suspend"? If not then I guess that's not needed.

[kitakar5525]

By the way, is this comment Shutdowned driver can't cause a wakeup. true?

I can only guess. If it works the way I think it works, then the WiFi controller would normally keep on working (in a suspended mode), so essentially be powered on. If it detects a wakeup package, it'll signal a wakeup via the IRQ (I think that's normally a GPIO pin or something like that). If we now shut down the card, the EC firmware might not be running any more, so it won't detect and issue wakeups. At least that's my theory. Without knowing more about either how those things usually work or how the mwifiex chip works in particular, all of this might be wrong of course.

• Rewrote the reason of removing mwifiex_enable_wake()/mwifiex_disable_wake() in a more unspecified way:

  - Removed mwifiex_enable_wake()/mwifiex_disable_wake()
    as incompatible and redundant because the driver will be shut down
    instead of Host Sleep.
  

Oh and it should be Shut down driver, I think (past tense of "shut down" is also "shut down").

Oh, right. Fixed.

• Corrected English verb shutdown -> shut down

and how do you think if preserving mwifiex_enable_wake()/mwifiex_disable_wake() is useful? (I've currently deleted as the commit message states)

I would keep them removed, for now at least. If the EC can't issue any wakeups when shut down, having them there could cause the wrong impression (i.e. that wakeups are still expected to work). In the worst-case scenario it could also cause spurious wakeups if the EC would be not properly configured for suspend, but I kind of doubt that.

Thanks. I kept removing mwifiex_enable_wake()/mwifiex_disable_wake().

That said, why do we need to check if device is already suspended/resumed as upstream does?

I honestly don't know. is it possible that something like runtime-suspend could enter Host Suspend while the device is still active, and suspend() then cause a "double suspend"? If not then I guess that's not needed.

• Added comment for not checking suspend state on suspend()

  Note that now there is no check whether it's suspended before performing
  mwifiex_shutdown_sw() on suspend().
  With the previous Host Sleep method, the check was done by looking at
  adapter->hs_activated in mwifiex_enable_hs() [sta_ioctl.c], but not
  MWIFIEX_IS_SUSPENDED. So, what the previous method checked was instead
  Host Sleep state, not suspend itself. Therefore, there is no need to check
  the suspend state now.
  
  Also removed comment for suspend state check at top of suspend() 
  accordingly.
  

[kitakar5525]

Other changes:

mwifiex: fix mwifiex_shutdown_sw() causing sw reset failure

• Fixed "by some reasons" -> "for some reasons"
• Added Fixes tag Fixes: 4c5dae59d2e9 ("mwifiex: add PCIe function level reset support")

mwifiex: pcie: use shutdown_sw()/reinit_sw() on suspend()/resume()

• Re-added comment "If already not resumed" at top of resume()
  I had removed the comment before because I intended to remove the check
  of MWIFIEX_IS_SUSPENDED in resume(), but ended up preserving it.

• Added new commit mwifiex: change comment for shutdown_sw()/reinit_sw() reflecting current state for upstreaming

[kitakar5525]

Might need more investigation for why flush_workqueue() causes kernel oops. I'll do this next.

[kitakar5525]

I tried re-adding flush_workqueue() (after mwifiex_shutdown_sw())

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index aef24b7c71c0..688bc94ba189 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -171,6 +171,8 @@ static int mwifiex_pcie_suspend(struct device *dev)
 		return -EFAULT;
 	}
 
+	flush_workqueue(adapter->workqueue);
+
 	/* Indicate device suspended */
 	set_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags);
 

This resulted in SB1 not resuming from suspend.

The last journal output

[92430.637881] kernel: PM: suspend entry (s2idle)
[92430.682405] kernel: Filesystems sync: 0.044 seconds

So, no crash log recorded.

note: commit torvalds/linux@ec815dd added the flush_workqueue() for Host Sleep.

I have no idea what I should do, but upstream may know anyway. At least I haven't encountered issues with this patch PR.

[qzed]

Could be some deadlock again. AFAIK there are some suspend debugging options you could try. But yeah, maybe upstream has any idea on what could go wrong.

[qzed]

Looks good. Feel free to merge when you think it's ready.

[kitakar5525]

Small changes to commit message/title:

mwifiex: pcie: use shutdown_sw()/reinit_sw() on suspend()/resume()

• corrected some English grammar

• the following change:

-Note that now there is no check whether it's suspended before performing
-mwifiex_shutdown_sw() on suspend().
+Note that suspend() no longer checks if it's already suspended.

• removed the following from commit message:

I reused the contents of suspend()/resume() functions as much as possible,
and removed only the parts that are incompatible or redundant with
shutdown_sw()/reinit_sw().

- Removed wait_for_completion() as redundant
  mwifiex_shutdown_sw() does this.
- Removed flush_workqueue() as incompatible
  Causes kernel crashing.
- Removed mwifiex_enable_wake()/mwifiex_disable_wake()
  as incompatible and redundant because the driver will be shut down
  instead of entering Host Sleep.

I think sending this as extra comment is better.

change comment for shutdown_sw()/reinit_sw() reflecting current state

• changed patch name to update comment for shutdown_sw()/reinit_sw() reflecting current state

[kitakar5525]

Looks good. Feel free to merge when you think it's ready.

Thanks, I'll check commit message (mainly English grammar 😀) lastly for each PR and merge it.

[kitakar5525]

Ah sorry, the last rebase caused a word missing

-When using the Host Sleep method, it prevents the platform to reach
+When using the Host Sleep method, it prevents the platform to reach S0ix
during suspend. Also, after suspend, sometimes AP scanning won't work,

Fixed in this rebase. I consider this PR is ready now.

[kitakar5525]

Rebased on top of current v5.8-surface-devel for simplicity

[kitakar5525]

By the way, I noticed the following changes regarding S0ix (printed /sys/kernel/debug/pmc_core contents as early as possible after resume) on SB1 (Host Sleep method vs shutdown_sw() method):

-MPHY CORE LANE 12               	State: Not power gated
-PCH IP: 6  - SPC                             	State: On
-DMIPCIE3 PLL                    	State: Active
+MPHY CORE LANE 12               	State: Power gated
+PCH IP: 6  - SPC                             	State: Off
+DMIPCIE3 PLL                    	State: Idle

So, this indicates that the shutdown_sw() suspend method is definitely working.