From: Lorenzo Bianconi <lorenzo@kernel.org>
To: sean.wang@mediatek.com
Cc: nbd@nbd.name, lorenzo.bianconi@redhat.com,
Soul.Huang@mediatek.com, YN.Chen@mediatek.com,
Leon.Yen@mediatek.com, Eric-SY.Chang@mediatek.com,
Mark-YW.Chen@mediatek.com, Deren.Wu@mediatek.com,
km.lin@mediatek.com, robin.chiu@mediatek.com,
Eddie.Chen@mediatek.com, ch.yeh@mediatek.com,
posh.sun@mediatek.com, ted.huang@mediatek.com,
Eric.Liang@mediatek.com, Stella.Chang@mediatek.com,
Tom.Chou@mediatek.com, steve.lee@mediatek.com, jsiuda@google.com,
frankgor@google.com, jemele@google.com,
abhishekpandit@google.com, shawnku@google.com,
linux-wireless@vger.kernel.org,
linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend
Date: Sat, 20 Nov 2021 15:18:00 +0100 [thread overview]
Message-ID: <YZkDmDIqnePjIF+O@lore-desk> (raw)
In-Reply-To: <YZjxh5thdYgFV735@lore-desk>
[-- Attachment #1: Type: text/plain, Size: 8628 bytes --]
> > From: Sean Wang <sean.wang@mediatek.com>
> >
> > According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have to
> > be last MCU command to execute in suspend handler and all data traffic
> > have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL starts as well
> > in order that mt7921 can successfully fall into the deep sleep mode.
> >
> > Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
> > another global flag to stop all of the traffic onto the SDIO bus.
> >
> > Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
> > Reported-by: Leon Yen <leon.yen@mediatek.com>
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> > .../wireless/mediatek/mt76/mt76_connac_mcu.c | 2 +-
> > .../net/wireless/mediatek/mt76/mt7921/main.c | 3 --
> > .../net/wireless/mediatek/mt76/mt7921/sdio.c | 34 ++++++++++++-------
> > drivers/net/wireless/mediatek/mt76/sdio.c | 3 +-
> > .../net/wireless/mediatek/mt76/sdio_txrx.c | 3 +-
> > 5 files changed, 27 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > index 26b4b875dcc0..61c4c86e79c8 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
> > struct ieee80211_vif *vif)
> > {
> > struct mt76_phy *phy = priv;
> > - bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
> > + bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
> > struct ieee80211_hw *hw = phy->hw;
> > struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
> > int i;
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > index e022251b4006..0b2a6b7f22ea 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
> > mt7921_mutex_acquire(dev);
> >
> > clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> > -
> > - set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> > ieee80211_iterate_active_interfaces(hw,
> > IEEE80211_IFACE_ITER_RESUME_ALL,
> > mt76_connac_mcu_set_suspend_iter,
> > @@ -1262,7 +1260,6 @@ static int mt7921_resume(struct ieee80211_hw *hw)
> > mt7921_mutex_acquire(dev);
> >
> > set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> > - clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> > ieee80211_iterate_active_interfaces(hw,
> > IEEE80211_IFACE_ITER_RESUME_ALL,
> > mt76_connac_mcu_set_suspend_iter,
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > index 5fee489c7a99..5c88b6b8d097 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
> > int err;
> >
> > pm->suspended = true;
> > + set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> > +
> > cancel_delayed_work_sync(&pm->ps_work);
> > cancel_work_sync(&pm->wake_work);
> >
> > @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
> > if (err < 0)
> > goto restore_suspend;
> >
> > - err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> > - if (err)
> > - goto restore_suspend;
> > -
> > /* always enable deep sleep during suspend to reduce
> > * power consumption
> > */
> > @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device *__dev)
> >
> > mt76_txq_schedule_all(&dev->mphy);
> > mt76_worker_disable(&mdev->tx_worker);
> > - mt76_worker_disable(&mdev->sdio.txrx_worker);
> > mt76_worker_disable(&mdev->sdio.status_worker);
> > - mt76_worker_disable(&mdev->sdio.net_worker);
> > cancel_work_sync(&mdev->sdio.stat_work);
> > clear_bit(MT76_READING_STATS, &dev->mphy.state);
> > -
> > mt76_tx_status_check(mdev, true);
> >
> > - err = mt7921_mcu_fw_pmctrl(dev);
> > + mt76_worker_schedule(&mdev->sdio.txrx_worker);
>
> I guess mt76_worker_disable() is supposed to do it, right?
> I guess we can assume txrx_worker is no longer running here, right?
I can see it now, txrx_worker can be running on the different cpu.
I guess we need to add just the wait_event_timeout() and move
mt76_connac_mcu_set_hif_suspend() below. What do you think?
Can you please try the chunk below?
Regards,
Lorenzo
>
> > + wait_event_timeout(dev->mt76.sdio.wait,
> > + mt76s_txqs_empty(&dev->mt76), 5 * HZ);
> > +
> > + /* It is supposed that SDIO bus is idle at the point */
> > + err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> > if (err)
> > goto restore_worker;
> >
> > + mt76_worker_disable(&mdev->sdio.txrx_worker);
> > + mt76_worker_disable(&mdev->sdio.net_worker);
> > +
> > + err = mt7921_mcu_fw_pmctrl(dev);
> > + if (err)
> > + goto restore_txrx_worker;
> > +
> > sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
> >
> > return 0;
> >
> > +restore_txrx_worker:
> > + mt76_worker_enable(&mdev->sdio.net_worker);
> > + mt76_worker_enable(&mdev->sdio.txrx_worker);
> > + mt76_connac_mcu_set_hif_suspend(mdev, false);
> > +
> > restore_worker:
> > mt76_worker_enable(&mdev->tx_worker);
> > - mt76_worker_enable(&mdev->sdio.txrx_worker);
> > mt76_worker_enable(&mdev->sdio.status_worker);
> > - mt76_worker_enable(&mdev->sdio.net_worker);
> >
> > if (!pm->ds_enable)
> > mt76_connac_mcu_set_deep_sleep(mdev, false);
> >
> > - mt76_connac_mcu_set_hif_suspend(mdev, false);
> > -
> > restore_suspend:
> > + clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> > pm->suspended = false;
> >
> > return err;
> > @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
> > int err;
> >
> > pm->suspended = false;
> > + clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >
> > err = mt7921_mcu_drv_pmctrl(dev);
> > if (err < 0)
> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
> > index c99acc21225e..b0bc7be0fb1f 100644
> > --- a/drivers/net/wireless/mediatek/mt76/sdio.c
> > +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
> > @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
> > resched = true;
> >
> > if (dev->drv->tx_status_data &&
> > - !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
> > + !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
> > + !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
> > queue_work(dev->wq, &dev->sdio.stat_work);
> > } while (nframes > 0);
> >
> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > index 649a56790b89..801590a0a334 100644
> > --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
> > if (ret > 0)
> > nframes += ret;
> >
> > - if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> > + if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
> > + test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
> > if (!mt76s_txqs_empty(dev))
> > continue;
> > else
>
> since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we really
> need to check it for each iteration or is fine something like:
>
> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> index 649a56790b89..68f30a83fa5d 100644
> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> @@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
> if (ret > 0)
> nframes += ret;
>
> - if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> - if (!mt76s_txqs_empty(dev))
> - continue;
> - else
> - wake_up(&sdio->wait);
> - }
> } while (nframes > 0);
>
> + if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> + mt76s_txqs_empty(dev))
> + wake_up(&sdio->wait);
> +
> /* enable interrupt */
> sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> sdio_release_host(sdio->func);
>
> Regards,
> Lorenzo
>
> > --
> > 2.25.1
> >
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2021-11-20 14:18 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-19 23:22 [PATCH 1/2] mt76: mt7921: move mt76_connac_mcu_set_hif_suspend to bus-related files sean.wang
2021-11-19 23:22 ` [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend sean.wang
2021-11-20 13:00 ` Lorenzo Bianconi
2021-11-20 14:18 ` Lorenzo Bianconi [this message]
[not found] <YZkDmDIqnePjIF+O@lore-desk--annotate>
2021-11-20 20:29 ` sean.wang
2021-11-21 21:43 ` Lorenzo Bianconi
[not found] <YZq9cBbzwtvzEKyN@lore-desk--annotate>
2021-11-22 3:46 ` sean.wang
2021-11-22 10:49 ` Lorenzo Bianconi
[not found] <YWcuGcFPGCtaPh+2@lore-desk--annotate>
2021-11-22 17:10 ` sean.wang
2021-11-22 18:10 ` Lorenzo Bianconi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YZkDmDIqnePjIF+O@lore-desk \
--to=lorenzo@kernel.org \
--cc=Deren.Wu@mediatek.com \
--cc=Eddie.Chen@mediatek.com \
--cc=Eric-SY.Chang@mediatek.com \
--cc=Eric.Liang@mediatek.com \
--cc=Leon.Yen@mediatek.com \
--cc=Mark-YW.Chen@mediatek.com \
--cc=Soul.Huang@mediatek.com \
--cc=Stella.Chang@mediatek.com \
--cc=Tom.Chou@mediatek.com \
--cc=YN.Chen@mediatek.com \
--cc=abhishekpandit@google.com \
--cc=ch.yeh@mediatek.com \
--cc=frankgor@google.com \
--cc=jemele@google.com \
--cc=jsiuda@google.com \
--cc=km.lin@mediatek.com \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-wireless@vger.kernel.org \
--cc=lorenzo.bianconi@redhat.com \
--cc=nbd@nbd.name \
--cc=posh.sun@mediatek.com \
--cc=robin.chiu@mediatek.com \
--cc=sean.wang@mediatek.com \
--cc=shawnku@google.com \
--cc=steve.lee@mediatek.com \
--cc=ted.huang@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).