From: Sean Wang <[email protected]>
This is a preliminary patch for the following patch
("mt76: mt7921s: fix the device cannot sleep deeply in suspend).
mt76_connac_mcu_set_hif_suspend eventually would be handled in each
bus-level suspend/resume handler in either mt7921/sdio.c or mt7921/pci.c
depending on what type of the bus the device is running on. We can move
mt76_connac_mcu_set_hif_suspend to bus-related files to simplify the logic.
Signed-off-by: Sean Wang <[email protected]>
---
.../net/wireless/mediatek/mt76/mt7921/main.c | 13 ++-----------
.../net/wireless/mediatek/mt76/mt7921/pci.c | 18 +++++-------------
.../net/wireless/mediatek/mt76/mt7921/sdio.c | 18 +++++-------------
3 files changed, 12 insertions(+), 37 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
index b144f5491798..e022251b4006 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
@@ -1232,7 +1232,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
{
struct mt7921_dev *dev = mt7921_hw_dev(hw);
struct mt7921_phy *phy = mt7921_hw_phy(hw);
- int err;
cancel_delayed_work_sync(&phy->scan_work);
cancel_delayed_work_sync(&phy->mt76->mac_work);
@@ -1250,25 +1249,18 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
mt76_connac_mcu_set_suspend_iter,
&dev->mphy);
- err = mt76_connac_mcu_set_hif_suspend(&dev->mt76, true);
-
mt7921_mutex_release(dev);
- return err;
+ return 0;
}
static int mt7921_resume(struct ieee80211_hw *hw)
{
struct mt7921_dev *dev = mt7921_hw_dev(hw);
struct mt7921_phy *phy = mt7921_hw_phy(hw);
- int err;
mt7921_mutex_acquire(dev);
- err = mt76_connac_mcu_set_hif_suspend(&dev->mt76, false);
- if (err < 0)
- goto out;
-
set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
ieee80211_iterate_active_interfaces(hw,
@@ -1278,11 +1270,10 @@ static int mt7921_resume(struct ieee80211_hw *hw)
ieee80211_queue_delayed_work(hw, &phy->mt76->mac_work,
MT7921_WATCHDOG_TIME);
-out:
mt7921_mutex_release(dev);
- return err;
+ return 0;
}
static void mt7921_set_wakeup(struct ieee80211_hw *hw, bool enabled)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
index 305b63fa1a8a..c29dde23d4ab 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
@@ -235,7 +235,6 @@ static int mt7921_pci_suspend(struct pci_dev *pdev, pm_message_t state)
struct mt76_dev *mdev = pci_get_drvdata(pdev);
struct mt7921_dev *dev = container_of(mdev, struct mt7921_dev, mt76);
struct mt76_connac_pm *pm = &dev->pm;
- bool hif_suspend;
int i, err;
pm->suspended = true;
@@ -246,12 +245,9 @@ static int mt7921_pci_suspend(struct pci_dev *pdev, pm_message_t state)
if (err < 0)
goto restore_suspend;
- hif_suspend = !test_bit(MT76_STATE_SUSPEND, &dev->mphy.state);
- if (hif_suspend) {
- err = mt76_connac_mcu_set_hif_suspend(mdev, true);
- if (err)
- 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
@@ -302,8 +298,7 @@ static int mt7921_pci_suspend(struct pci_dev *pdev, pm_message_t state)
if (!pm->ds_enable)
mt76_connac_mcu_set_deep_sleep(&dev->mt76, false);
- if (hif_suspend)
- mt76_connac_mcu_set_hif_suspend(mdev, false);
+ mt76_connac_mcu_set_hif_suspend(mdev, false);
restore_suspend:
pm->suspended = false;
@@ -356,10 +351,7 @@ static int mt7921_pci_resume(struct pci_dev *pdev)
if (!pm->ds_enable)
mt76_connac_mcu_set_deep_sleep(&dev->mt76, false);
- if (!test_bit(MT76_STATE_SUSPEND, &dev->mphy.state))
- err = mt76_connac_mcu_set_hif_suspend(mdev, false);
-
- return err;
+ return mt76_connac_mcu_set_hif_suspend(mdev, false);
}
#endif /* CONFIG_PM */
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
index ddf0eeb8b688..5fee489c7a99 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
@@ -203,7 +203,6 @@ static int mt7921s_suspend(struct device *__dev)
struct mt7921_dev *dev = sdio_get_drvdata(func);
struct mt76_connac_pm *pm = &dev->pm;
struct mt76_dev *mdev = &dev->mt76;
- bool hif_suspend;
int err;
pm->suspended = true;
@@ -214,12 +213,9 @@ static int mt7921s_suspend(struct device *__dev)
if (err < 0)
goto restore_suspend;
- hif_suspend = !test_bit(MT76_STATE_SUSPEND, &dev->mphy.state);
- if (hif_suspend) {
- err = mt76_connac_mcu_set_hif_suspend(mdev, true);
- if (err)
- 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
@@ -253,8 +249,7 @@ static int mt7921s_suspend(struct device *__dev)
if (!pm->ds_enable)
mt76_connac_mcu_set_deep_sleep(mdev, false);
- if (hif_suspend)
- mt76_connac_mcu_set_hif_suspend(mdev, false);
+ mt76_connac_mcu_set_hif_suspend(mdev, false);
restore_suspend:
pm->suspended = false;
@@ -285,10 +280,7 @@ static int mt7921s_resume(struct device *__dev)
if (!pm->ds_enable)
mt76_connac_mcu_set_deep_sleep(mdev, false);
- if (!test_bit(MT76_STATE_SUSPEND, &dev->mphy.state))
- err = mt76_connac_mcu_set_hif_suspend(mdev, false);
-
- return err;
+ return mt76_connac_mcu_set_hif_suspend(mdev, false);
}
static const struct dev_pm_ops mt7921s_pm_ops = {
--
2.25.1
From: Sean Wang <[email protected]>
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 <[email protected]>
Signed-off-by: Sean Wang <[email protected]>
---
.../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);
+ 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
--
2.25.1
> From: Sean Wang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Sean Wang <[email protected]>
> ---
> .../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
>
> > From: Sean Wang <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Sean Wang <[email protected]>
> > ---
> > .../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
> >
From: Sean Wang <[email protected]>
>> > From: Sean Wang <[email protected]>
>> >
>> > 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 <[email protected]>
>> > Signed-off-by: Sean Wang <[email protected]>
>> > ---
>> > .../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?
mt76_connac_mcu_set_hif_suspend have to rely on txrx_worker and net_worker to
send the command MCU_UNI_CMD_HIF_CTRL and receive the corresponding event,
so that is why we cannnot disable txrx_worker and net_worker with mt76_worker_disable
until the mt76_connac_mcu_set_hif_suspend is done.
>
>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);
>> +
If doing so, mt76s_txqs_empty may not always be true because enqueuing packets
to q_tx or MCU command to q_mcu simultanenously from the other contexts in
different cpu is possible.
It seemed to me we should check it for each iteration to guarantee that we can
wake up the one that is waiting for the all the queues are empty at some time.
>> /* enable interrupt */
>> sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>> sdio_release_host(sdio->func);
>>
>> Regards,
>> Lorenzo
>>
>> > --
>> > 2.25.1
>> >
>
>
>
> From: Sean Wang <[email protected]>
>
> >> > From: Sean Wang <[email protected]>
> >> >
> >> > 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 <[email protected]>
> >> > Signed-off-by: Sean Wang <[email protected]>
> >> > ---
> >> > .../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?
>
> mt76_connac_mcu_set_hif_suspend have to rely on txrx_worker and net_worker to
> send the command MCU_UNI_CMD_HIF_CTRL and receive the corresponding event,
> so that is why we cannnot disable txrx_worker and net_worker with mt76_worker_disable
> until the mt76_connac_mcu_set_hif_suspend is done.
ack, right. I forgot about this double dependency :)
>
> >
> >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);
> >> +
>
> If doing so, mt76s_txqs_empty may not always be true because enqueuing packets
> to q_tx or MCU command to q_mcu simultanenously from the other contexts in
> different cpu is possible.
>
> It seemed to me we should check it for each iteration to guarantee that we can
> wake up the one that is waiting for the all the queues are empty at some time.
IIUC what we are interested here is there are no queued frames into the hw
queues during suspend or reset, right?
>
> >> /* enable interrupt */
> >> sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >> sdio_release_host(sdio->func);
> >>
> >> Regards,
> >> Lorenzo
> >>
> >> > --
> >> > 2.25.1
> >> >
> >
> >
> >
>
From: Sean Wang <[email protected]>
<snip>
>> >>
>> >> - 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);
>> >> +
>>
>> If doing so, mt76s_txqs_empty may not always be true because enqueuing
>> packets to q_tx or MCU command to q_mcu simultanenously from the other
>> contexts in different cpu is possible.
>>
>> It seemed to me we should check it for each iteration to guarantee
>> that we can wake up the one that is waiting for the all the queues are empty at some time.
>
>IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
That is not completely true. Take the suspend procedure on mt7921s as an example.
That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
The MCU data and WiFi are all handled in mt76s_txrx_worker so we should synchronize all of
the Tx queues are all empty and then handle mt76_connac_mcu_set_hif_suspend to guarantee
mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.
>
>>
>> >> /* enable interrupt */
>> >> sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>> >> sdio_release_host(sdio->func);
>> >>
>> >> Regards,
>> >> Lorenzo
>> >>
>> >> > --
>> >> > 2.25.1
>> >> >
>> >
>> >
>> >
>>
>
> From: Sean Wang <[email protected]>
>
>
> <snip>
>
> >> >>
> >> >> - 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);
> >> >> +
> >>
> >> If doing so, mt76s_txqs_empty may not always be true because enqueuing
> >> packets to q_tx or MCU command to q_mcu simultanenously from the other
> >> contexts in different cpu is possible.
> >>
> >> It seemed to me we should check it for each iteration to guarantee
> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
> >
> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
>
> That is not completely true. Take the suspend procedure on mt7921s as an example.
>
> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
>
> The MCU data and WiFi are all handled in mt76s_txrx_worker so we should synchronize all of
> the Tx queues are all empty and then handle mt76_connac_mcu_set_hif_suspend to guarantee
> mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.
ack, correct, "there are no queued frames into the hw queues right after
mt76_connac_mcu_set_hif_suspend."
What I mean is we are not really checking there are no frames in the hw queue
here, but mt76 sdio has processed all the frames, got my point? maybe it is
what we are looking for..
Regards,
Lorenzo
>
> >
> >>
> >> >> /* enable interrupt */
> >> >> sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >> >> sdio_release_host(sdio->func);
> >> >>
> >> >> Regards,
> >> >> Lorenzo
> >> >>
> >> >> > --
> >> >> > 2.25.1
> >> >> >
> >> >
> >> >
> >> >
> >>
> >
From: Sean Wang <[email protected]>
>> From: Sean Wang <[email protected]>
>>
>>
>> <snip>
>>
>> >> >>
>> >> >> - 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);
>> >> >> +
>> >>
>> >> If doing so, mt76s_txqs_empty may not always be true because
>> >> enqueuing packets to q_tx or MCU command to q_mcu simultanenously
>> >> from the other contexts in different cpu is possible.
>> >>
>> >> It seemed to me we should check it for each iteration to guarantee
>> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
>> >
>> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
>>
>> That is not completely true. Take the suspend procedure on mt7921s as an example.
>>
>> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
>>
>> The MCU data and WiFi are all handled in mt76s_txrx_worker so we
>> should synchronize all of the Tx queues are all empty and then handle
>> mt76_connac_mcu_set_hif_suspend to guarantee mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.
>
>ack, correct, "there are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
>What I mean is we are not really checking there are no frames in the hw queue here, but mt76 sdio has processed all the frames, got my point? maybe it is what we are looking for..
It seemed to me there is no a way to check if no frames in the hw queue for mt7921s
I think we can discuss the topic "it is ok move the check out of the loop" with another patch in the future
because it seemed not related to the patch.
>
>Regards,
>Lorenzo
>
>>
>> >
>> >>
>> >> >> /* enable interrupt */
>> >> >> sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>> >> >> sdio_release_host(sdio->func);
>> >> >>
>> >> >> Regards,
>> >> >> Lorenzo
>> >> >>
>> >> >> > --
>> >> >> > 2.25.1
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >>
>> >
> From: Sean Wang <[email protected]>
>
> >> From: Sean Wang <[email protected]>
> >>
> >>
> >> <snip>
> >>
> >> >> >>
> >> >> >> - 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);
> >> >> >> +
> >> >>
> >> >> If doing so, mt76s_txqs_empty may not always be true because
> >> >> enqueuing packets to q_tx or MCU command to q_mcu simultanenously
> >> >> from the other contexts in different cpu is possible.
> >> >>
> >> >> It seemed to me we should check it for each iteration to guarantee
> >> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
> >> >
> >> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
> >>
> >> That is not completely true. Take the suspend procedure on mt7921s as an example.
> >>
> >> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
> >>
> >> The MCU data and WiFi are all handled in mt76s_txrx_worker so we
> >> should synchronize all of the Tx queues are all empty and then handle
> >> mt76_connac_mcu_set_hif_suspend to guarantee mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.
> >
> >ack, correct, "there are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
> >What I mean is we are not really checking there are no frames in the hw queue here, but mt76 sdio has processed all the frames, got my point? maybe it is what we are looking for..
>
> It seemed to me there is no a way to check if no frames in the hw queue for mt7921s
>
> I think we can discuss the topic "it is ok move the check out of the loop" with another patch in the future
>
> because it seemed not related to the patch.
sure, fine to me.
Regards,
Lorenzo
>
> >
> >Regards,
> >Lorenzo
> >
> >>
> >> >
> >> >>
> >> >> >> /* enable interrupt */
> >> >> >> sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >> >> >> sdio_release_host(sdio->func);
> >> >> >>
> >> >> >> Regards,
> >> >> >> Lorenzo
> >> >> >>
> >> >> >> > --
> >> >> >> > 2.25.1
> >> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >>
> >> >