Skip to contentSkip to conversation

mt76: mt7921s: fix the device cannot sleep deeply in suspend

PATCH [email protected] 2 participants 2 messages Nov 20, 2021 – Nov 21, 2021 1 message not in the archive Arm MediaTek

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): YZkDmDIqnePjIF+O@lore-desk--annotate

[email protected]

author
#1 lore raw

From: Sean Wang <[email protected]>

Quoted 100 lines
Quoted 94 lines
Quoted 94 lines

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(-)
drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c +1 -1
index 26b4b875dcc0..61c4c86e79c8 100644
@@ -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;
drivers/net/wireless/mediatek/mt76/mt7921/main.c +0 -2
index e022251b4006..0b2a6b7f22ea 100644
@@ -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);
drivers/net/wireless/mediatek/mt76/mt7921/main.c +0 -1
-	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

drivers/net/wireless/mediatek/mt76/mt7921/sdio.c +2 -7
@@ -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);
Quoted 5 lines

drivers/net/wireless/mediatek/mt76/mt7921/sdio.c static int mt7921s_suspend(struct device · line 231

Quoted 2 lines
drivers/net/wireless/mediatek/mt76/mt7921/sdio.c +1 -1
-	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?

Quoted 4 lines

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.

Quoted 104 lines

Regards,
Lorenzo

Quoted 100 lines
Quoted 70 lines
drivers/net/wireless/mediatek/mt76/mt7921/sdio.c +18 -4
+	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;
drivers/net/wireless/mediatek/mt76/mt7921/sdio.c +1 -0
@@ -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)
drivers/net/wireless/mediatek/mt76/sdio.c +2 -1
index c99acc21225e..b0bc7be0fb1f 100644
@@ -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);
drivers/net/wireless/mediatek/mt76/sdio_txrx.c +0 -0
index 649a56790b89..801590a0a334 100644

drivers/net/wireless/mediatek/mt76/sdio_txrx.c void mt76s_txrx_worker(struct mt76_sdio *sdio) · line 324

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/sdio_txrx.c +2 -1
@@ -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:

drivers/net/wireless/mediatek/mt76/sdio_txrx.c +0 -6
index 649a56790b89..68f30a83fa5d 100644
@@ -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);
Quoted 4 lines

drivers/net/wireless/mediatek/mt76/sdio_txrx.c void mt76s_txrx_worker(struct mt76_sdio *sdio) · line 325

Quoted 4 lines
drivers/net/wireless/mediatek/mt76/sdio_txrx.c +4 -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.

Quoted 13 lines
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/sdio_txrx.c +0 -0
 	/* enable interrupt */
 	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
 	sdio_release_host(sdio->func);

Regards,
Lorenzo

Quoted 3 lines

--
2.25.1

#2 lore raw
Quoted 107 lines

From: Sean Wang <[email protected]>

Quoted 100 lines
Quoted 94 lines
Quoted 94 lines

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(-)
drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c +1 -1
index 26b4b875dcc0..61c4c86e79c8 100644
@@ -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;
drivers/net/wireless/mediatek/mt76/mt7921/main.c +0 -2
index e022251b4006..0b2a6b7f22ea 100644
@@ -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);
drivers/net/wireless/mediatek/mt76/mt7921/main.c +0 -1
-	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

drivers/net/wireless/mediatek/mt76/mt7921/sdio.c +2 -7
@@ -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);
Quoted 5 lines

drivers/net/wireless/mediatek/mt76/mt7921/sdio.c static int mt7921s_suspend(struct device · line 231

Quoted 2 lines
drivers/net/wireless/mediatek/mt76/mt7921/sdio.c +1 -1
-	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?

Quoted 4 lines

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?

Quoted 4 lines

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

Quoted 114 lines
Quoted 105 lines

Regards,
Lorenzo

Quoted 100 lines
Quoted 70 lines
drivers/net/wireless/mediatek/mt76/mt7921/sdio.c +18 -4
+	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;
drivers/net/wireless/mediatek/mt76/mt7921/sdio.c +1 -0
@@ -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)
drivers/net/wireless/mediatek/mt76/sdio.c +2 -1
index c99acc21225e..b0bc7be0fb1f 100644
@@ -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);
drivers/net/wireless/mediatek/mt76/sdio_txrx.c +0 -0
index 649a56790b89..801590a0a334 100644

drivers/net/wireless/mediatek/mt76/sdio_txrx.c void mt76s_txrx_worker(struct mt76_sdio *sdio) · line 324

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/sdio_txrx.c +2 -1
@@ -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:

drivers/net/wireless/mediatek/mt76/sdio_txrx.c +0 -6
index 649a56790b89..68f30a83fa5d 100644
@@ -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);
Quoted 4 lines

drivers/net/wireless/mediatek/mt76/sdio_txrx.c void mt76s_txrx_worker(struct mt76_sdio *sdio) · line 325

Quoted 4 lines
drivers/net/wireless/mediatek/mt76/sdio_txrx.c +4 -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.

Quoted 2 lines

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?

Quoted 15 lines
Quoted 14 lines
Quoted 11 lines
drivers/net/wireless/mediatek/mt76/sdio_txrx.c +0 -0
 	/* enable interrupt */
 	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
 	sdio_release_host(sdio->func);

Regards,
Lorenzo

Quoted 3 lines

--
2.25.1