> From: Sean Wang > > 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 > Signed-off-by: Sean Wang > --- > .../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? > + 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 >