[PATCH 2/3] rsi: sdio: Add WOWLAN support for S4 hibernate state

From: Karun Eagalapati <redacted>

We are disabling of interrupts from firmware in freeze handler.
Also setting power management capability KEEP_MMC_POWER to make
device wakeup for WoWLAN trigger.
At restore, we observed a device reset on some platforms. Hence
reloading of firmware and device initialization is performed.

Signed-off-by: Karun Eagalapati <redacted>
Signed-off-by: Amitkumar Karwar <redacted>
---
 drivers/net/wireless/rsi/rsi_91x_sdio.c | 109 ++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index 28efa17..b8b5795 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c

@@ -1202,9 +1202,118 @@ static int rsi_resume(struct device *dev)
 	return 0;
 }
 
+static int rsi_freeze(struct device *dev)
+{
+	int ret;
+	struct sdio_func *pfunction = dev_to_sdio_func(dev);
+	struct rsi_hw *adapter = sdio_get_drvdata(pfunction);
+	struct rsi_common *common;
+	struct rsi_91x_sdiodev *sdev;
+
+	rsi_dbg(INFO_ZONE, "SDIO Bus freeze ===>\n");
+
+	if (!adapter) {
+		rsi_dbg(ERR_ZONE, "Device is not ready\n");
+		return -ENODEV;
+	}
+	common = adapter->priv;
+	sdev = (struct rsi_91x_sdiodev *)adapter->rsi_dev;
+
+#ifdef CONFIG_RSI_WOW
+	if ((common->wow_flags & RSI_WOW_ENABLED) &&
+	    (common->wow_flags & RSI_WOW_NO_CONNECTION))
+		rsi_dbg(ERR_ZONE,
+			"##### Device can not wake up through WLAN\n");
+#endif
+	ret = rsi_sdio_disable_interrupts(pfunction);
+
+	if (sdev->write_fail)
+		rsi_dbg(INFO_ZONE, "###### Device is not ready #######\n");
+
+	ret = rsi_set_sdio_pm_caps(adapter);
+	if (ret)
+		rsi_dbg(INFO_ZONE, "Setting power management caps failed\n");
+
+	rsi_dbg(INFO_ZONE, "***** RSI module freezed *****\n");
+
+	return 0;
+}
+
+static int rsi_thaw(struct device *dev)
+{
+	struct sdio_func *pfunction = dev_to_sdio_func(dev);
+	struct rsi_hw *adapter = sdio_get_drvdata(pfunction);
+	struct rsi_common *common = adapter->priv;
+
+	rsi_dbg(ERR_ZONE, "SDIO Bus thaw =====>\n");
+
+	common->fsm_state = FSM_CARD_NOT_READY;
+	common->iface_down = true;
+
+	rsi_sdio_enable_interrupts(pfunction);
+
+	rsi_dbg(INFO_ZONE, "***** RSI module thaw done *****\n");
+
+	return 0;
+}
+
+static int rsi_sdio_reinit_device(struct rsi_hw *adapter)
+{
+	struct rsi_91x_sdiodev *sdev = adapter->rsi_dev;
+	struct sdio_func *pfunction = sdev->pfunction;
+	int ii;
+
+	for (ii = 0; ii < NUM_SOFT_QUEUES; ii++)
+		skb_queue_purge(&adapter->priv->tx_queue[ii]);
+
+	rsi_mac80211_detach(adapter);
+
+	/* Initialize device again */
+	sdio_claim_host(pfunction);
+
+	sdio_release_irq(pfunction);
+	rsi_reset_card(pfunction);
+
+	sdio_enable_func(pfunction);
+	rsi_setupcard(adapter);
+	rsi_init_sdio_slave_regs(adapter);
+	sdio_claim_irq(pfunction, rsi_handle_interrupt);
+	rsi_hal_device_init(adapter);
+
+	sdio_release_host(pfunction);
+
+	return 0;
+}
+
+static int rsi_restore(struct device *dev)
+{
+	struct sdio_func *pfunction = dev_to_sdio_func(dev);
+	struct rsi_hw *adapter = sdio_get_drvdata(pfunction);
+	struct rsi_common *common = adapter->priv;
+
+	rsi_dbg(INFO_ZONE, "SDIO Bus restore ======>\n");
+
+	common->fsm_state = FSM_FW_NOT_LOADED;
+	common->iface_down = true;
+
+	/* Initialize device again */
+	rsi_sdio_reinit_device(adapter);
+
+#ifdef CONFIG_RSI_WOW
+	common->wow_flags = 0;
+#endif
+	common->iface_down = false;
+
+	rsi_dbg(INFO_ZONE, "RSI module restored\n");
+
+	return 0;
+}
 static const struct dev_pm_ops rsi_pm_ops = {
 	.suspend = rsi_suspend,
 	.resume = rsi_resume,
+	.freeze = rsi_freeze,
+	.thaw = rsi_thaw,
+	.restore = rsi_restore,
 };
 #endif
 

-- 
2.7.4

Amitkumar Karwar [off-list ref] writes:

From: Karun Eagalapati <redacted>

We are disabling of interrupts from firmware in freeze handler.
Also setting power management capability KEEP_MMC_POWER to make
device wakeup for WoWLAN trigger.
At restore, we observed a device reset on some platforms. Hence
reloading of firmware and device initialization is performed.

With "reloading of firmware and device initialization" you mean calling
rsi_mac80211_detach()?

void rsi_mac80211_detach(struct rsi_hw *adapter)
{
	struct ieee80211_hw *hw = adapter->hw;
	enum nl80211_band band;

	if (hw) {
		ieee80211_stop_queues(hw);
		ieee80211_unregister_hw(hw);
		ieee80211_free_hw(hw);
	}

That looks like a quite heavy sledgehammer workaround to a resume
problem. Is it really the best way to handle this?

And even if that would be the right approach it needs to be properly
described in the commit log, a vague sentence in the end of a commit log
is not enough.

-- 
Kalle Valo

On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo [off-list ref] wrote:

Amitkumar Karwar [off-list ref] writes:

quoted

From: Karun Eagalapati <redacted>

We are disabling of interrupts from firmware in freeze handler.
Also setting power management capability KEEP_MMC_POWER to make
device wakeup for WoWLAN trigger.
At restore, we observed a device reset on some platforms. Hence
reloading of firmware and device initialization is performed.

With "reloading of firmware and device initialization" you mean calling
rsi_mac80211_detach()?

After rsi_mac80211_detach, we have a call for rsi_hal_device_init() in
which reloading of firmware happens.

void rsi_mac80211_detach(struct rsi_hw *adapter)
{
        struct ieee80211_hw *hw = adapter->hw;
        enum nl80211_band band;

        if (hw) {
                ieee80211_stop_queues(hw);
                ieee80211_unregister_hw(hw);
                ieee80211_free_hw(hw);
        }

That looks like a quite heavy sledgehammer workaround to a resume
problem. Is it really the best way to handle this?

I agree with you. I will appreciate if someone propose a better
solution. Following are details about the problem.

Connection remain alive after suspend(S4 state). System is resumed
from S4 through GPIO pin after receiving magic packet. But SDIO
doesn't work after this. We need to do perform SDIO reset and
redownload the firmware. Mac80211 is under impression that connection
is alive. We keep on getting mac80211 handler calls and data while
firmware re-download is in progress. This leads to different race
issues and occasionally kernel crashes. detaching from mac80211 solves
the problem here.

And even if that would be the right approach it needs to be properly
described in the commit log, a vague sentence in the end of a commit log
is not enough.

Understood. I will add detailed description and send updated version
if the patch is fine.

Regards,
Amitkumar Karwar

Amitkumar Karwar [off-list ref] writes:

On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo [off-list ref] wrote:

quoted

Amitkumar Karwar [off-list ref] writes:

quoted

From: Karun Eagalapati <redacted>

We are disabling of interrupts from firmware in freeze handler.
Also setting power management capability KEEP_MMC_POWER to make
device wakeup for WoWLAN trigger.
At restore, we observed a device reset on some platforms. Hence
reloading of firmware and device initialization is performed.

With "reloading of firmware and device initialization" you mean calling
rsi_mac80211_detach()?

After rsi_mac80211_detach, we have a call for rsi_hal_device_init() in
which reloading of firmware happens.

quoted

void rsi_mac80211_detach(struct rsi_hw *adapter)
{
        struct ieee80211_hw *hw = adapter->hw;
        enum nl80211_band band;

        if (hw) {
                ieee80211_stop_queues(hw);
                ieee80211_unregister_hw(hw);
                ieee80211_free_hw(hw);
        }

That looks like a quite heavy sledgehammer workaround to a resume
problem. Is it really the best way to handle this?

I agree with you. I will appreciate if someone propose a better
solution. Following are details about the problem.

Connection remain alive after suspend(S4 state). System is resumed
from S4 through GPIO pin after receiving magic packet. But SDIO
doesn't work after this. We need to do perform SDIO reset and
redownload the firmware. Mac80211 is under impression that connection
is alive. We keep on getting mac80211 handler calls and data while
firmware re-download is in progress. This leads to different race
issues and occasionally kernel crashes. detaching from mac80211 solves
the problem here.

So what I think is the right approach is that the firmare is restarted
behind the scenes and user space doesn't even notice it, for example
that's what ath10k does. There's ieee80211_restart_hw() even for just
that.

We discussed about this also on one of mwifiex patches from Brian Norris
and there it was concluded that for cfg80211 driver it's ok to delete
the whole interface when restarting the firmware. But mac80211 drivers
can do better.

quoted

And even if that would be the right approach it needs to be properly
described in the commit log, a vague sentence in the end of a commit log
is not enough.

Understood. I will add detailed description and send updated version
if the patch is fine.

Not sure if this is fine or not. I think what you do here is ugly but I
guess it's better than nothing?

-- 
Kalle Valo

Hi Amitkumar,

On Wed, Oct 11, 2017 at 12:24:17PM +0300, Kalle Valo wrote:

Amitkumar Karwar [off-list ref] writes:

quoted

On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo [off-list ref] wrote:

quoted

And even if that would be the right approach it needs to be properly
described in the commit log, a vague sentence in the end of a commit log
is not enough.

Understood. I will add detailed description and send updated version
if the patch is fine.

Not sure if this is fine or not. I think what you do here is ugly but I
guess it's better than nothing?

I don't see why you can't try to reuse the existing mac80211 reset;
seems like you'd need to factor out some pieces of rsi_reset_card() and
call that from the ieee80211_ops::start() method. Then, you just call
ieee80211_restart_hw() from the appropriate place, instead of
implementing your own tear-down/reset.

Brian

On Wed, Oct 11, 2017 at 2:54 PM, Kalle Valo [off-list ref] wrote:

Amitkumar Karwar [off-list ref] writes:

quoted

On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo [off-list ref] wrote:

quoted

Amitkumar Karwar [off-list ref] writes:

quoted

From: Karun Eagalapati <redacted>

We are disabling of interrupts from firmware in freeze handler.
Also setting power management capability KEEP_MMC_POWER to make
device wakeup for WoWLAN trigger.
At restore, we observed a device reset on some platforms. Hence
reloading of firmware and device initialization is performed.

With "reloading of firmware and device initialization" you mean calling
rsi_mac80211_detach()?

After rsi_mac80211_detach, we have a call for rsi_hal_device_init() in
which reloading of firmware happens.

quoted

void rsi_mac80211_detach(struct rsi_hw *adapter)
{
        struct ieee80211_hw *hw = adapter->hw;
        enum nl80211_band band;

        if (hw) {
                ieee80211_stop_queues(hw);
                ieee80211_unregister_hw(hw);
                ieee80211_free_hw(hw);
        }

That looks like a quite heavy sledgehammer workaround to a resume
problem. Is it really the best way to handle this?

I agree with you. I will appreciate if someone propose a better
solution. Following are details about the problem.

Connection remain alive after suspend(S4 state). System is resumed
from S4 through GPIO pin after receiving magic packet. But SDIO
doesn't work after this. We need to do perform SDIO reset and
redownload the firmware. Mac80211 is under impression that connection
is alive. We keep on getting mac80211 handler calls and data while
firmware re-download is in progress. This leads to different race
issues and occasionally kernel crashes. detaching from mac80211 solves
the problem here.

So what I think is the right approach is that the firmare is restarted
behind the scenes and user space doesn't even notice it, for example
that's what ath10k does. There's ieee80211_restart_hw() even for just
that.

We discussed about this also on one of mwifiex patches from Brian Norris
and there it was concluded that for cfg80211 driver it's ok to delete
the whole interface when restarting the firmware. But mac80211 drivers
can do better.

Thanks for the suggestions. I went through the discussion for mwifiex patch.
We are exploring ieee80211_restart_hw() API approach.

quoted

quoted

And even if that would be the right approach it needs to be properly
described in the commit log, a vague sentence in the end of a commit log
is not enough.

Understood. I will add detailed description and send updated version
if the patch is fine.

Not sure if this is fine or not. I think what you do here is ugly but I
guess it's better than nothing?

Agreed. I have dropped the idea of sending updated patch with only
description change

Regards,
Amitkumar Karwar

On Thu, Oct 12, 2017 at 6:03 AM, Brian Norris [off-list ref] wrote:

Hi Amitkumar,

On Wed, Oct 11, 2017 at 12:24:17PM +0300, Kalle Valo wrote:

quoted

Amitkumar Karwar [off-list ref] writes:

quoted

On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo [off-list ref] wrote:

quoted

And even if that would be the right approach it needs to be properly
described in the commit log, a vague sentence in the end of a commit log
is not enough.

Understood. I will add detailed description and send updated version
if the patch is fine.

Not sure if this is fine or not. I think what you do here is ugly but I
guess it's better than nothing?

I don't see why you can't try to reuse the existing mac80211 reset;
seems like you'd need to factor out some pieces of rsi_reset_card() and
call that from the ieee80211_ops::start() method. Then, you just call
ieee80211_restart_hw() from the appropriate place, instead of
implementing your own tear-down/reset.

Thanks for the feedback
We will try ieee80211_restart_hw() based approach.

Regards,
Amitkumar