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

[Kalle Valo <kvalo@codeaurora.org>]

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