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

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

[Amitkumar Karwar]

From: Karun Eagalapati <karun256@gmail.com>

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 <karun256@gmail.com>
Signed-off-by: Amitkumar Karwar <amit.karwar@redpinesignals.com>
---
 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

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

[Kalle Valo]

Amitkumar Karwar <amitkarwar@gmail.com> writes:

> From: Karun Eagalapati <karun256@gmail.com>
>
> 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

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

[Amitkumar Karwar]

On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Amitkumar Karwar <amitkarwar@gmail.com> writes:
>
>> From: Karun Eagalapati <karun256@gmail.com>
>>
>> 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

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

[Kalle Valo]

Amitkumar Karwar <amitkarwar@gmail.com> writes:

> On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Amitkumar Karwar <amitkarwar@gmail.com> writes:
>>
>>> From: Karun Eagalapati <karun256@gmail.com>
>>>
>>> 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.

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.

>> 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

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

[Brian Norris]

Hi Amitkumar,

On Wed, Oct 11, 2017 at 12:24:17PM +0300, Kalle Valo wrote:
> Amitkumar Karwar <amitkarwar@gmail.com> writes:
> > On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> >> 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

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

[Amitkumar Karwar]

On Thu, Oct 12, 2017 at 6:03 AM, Brian Norris <briannorris@chromium.org> wrote:
> Hi Amitkumar,
>
> On Wed, Oct 11, 2017 at 12:24:17PM +0300, Kalle Valo wrote:
>> Amitkumar Karwar <amitkarwar@gmail.com> writes:
>> > On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> >> 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

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

[Amitkumar Karwar]

On Wed, Oct 11, 2017 at 2:54 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Amitkumar Karwar <amitkarwar@gmail.com> writes:
>
>> On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> Amitkumar Karwar <amitkarwar@gmail.com> writes:
>>>
>>>> From: Karun Eagalapati <karun256@gmail.com>
>>>>
>>>> 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.
>
> 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.

>
>>> 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