mt76: fix a leftover race in runtime-pm for mt7663/mt7921

PATCH In review Lorenzo Bianconi 3 participants 7 messages Dec 30, 2021 – Jan 11, 2022

Subsystems (2)

Arm MediaTek MediaTek MT76 wireless LAN driver

lore

Share & tools

lore

Lorenzo Bianconi

author maintainer

2021-12-30 20:47 UTC #0 lore raw

cover mt76: fix a leftover race in runtime-pm for mt7663/mt7921

Lorenzo Bianconi (2):
mt76: mt7921: fix a leftover race in runtime-pm
mt76: mt7615: fix a leftover race in runtime-pm

 drivers/net/wireless/mediatek/mt76/mt7615/mac.c | 8 ++++++++
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
 2 files changed, 16 insertions(+)

Signature · 1 line

2.33.1

Lorenzo Bianconi

author maintainer

2021-12-30 20:47 UTC #1 lore raw

PATCH 1/2 mt76: mt7921: fix a leftover race in runtime-pm

Fix a possible race in mt7921_pm_power_save_work() if rx/tx napi schedules ps_work and we are currently accessing device register on a different cpu.

Fixes 1d8efc741df8 ("mt76: mt7921: introduce Runtime PM support")

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/net/wireless/mediatek/mt76/mt7921/mac.c +8 -0

index defef3496246..0744f6e42ba3 100644
@@ -1553,6 +1553,14 @@ void mt7921_pm_power_save_work(struct work_struct *work)
 	    dev->fw_assert)
 		goto out;
 
+	if (mutex_is_locked(&dev->mt76.mutex))
+		/* if mt76 mutex is held we should not put the device
+		 * to sleep since we are currently accessing device
+		 * register map. We need to wait for the next power_save
+		 * trigger.
+		 */
+		goto out;
+
 	if (time_is_after_jiffies(dev->pm.last_activity + delta)) {
 		delta = dev->pm.last_activity + delta - jiffies;
 		goto out;

Signature · 1 line

2.33.1

Lorenzo Bianconi

author maintainer

2021-12-30 20:47 UTC #6 lore raw

PATCH 2/2 mt76: mt7615: fix a leftover race in runtime-pm

In reply to Lorenzo Bianconi

Fix a possible race in mt7615_pm_power_save_work() if rx/tx napi schedules ps_work and we are currently accessing device register on a different cpu.

Fixes db928f1ab9789 ("mt76: mt7663: rely on mt76_connac_pm_ref/mt76_connac_pm_unref in tx/rx napi")

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/wireless/mediatek/mt76/mt7615/mac.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/net/wireless/mediatek/mt76/mt7615/mac.c +8 -0

index 2d81cbf2600c..bc72791cdcb5 100644
@@ -2131,6 +2131,14 @@ void mt7615_pm_power_save_work(struct work_struct *work)
 	    test_bit(MT76_HW_SCHED_SCANNING, &dev->mphy.state))
 		goto out;
 
+	if (mutex_is_locked(&dev->mt76.mutex))
+		/* if mt76 mutex is held we should not put the device
+		 * to sleep since we are currently accessing device
+		 * register map. We need to wait for the next power_save
+		 * trigger.
+		 */
+		goto out;
+
 	if (time_is_after_jiffies(dev->pm.last_activity + delta)) {
 		delta = dev->pm.last_activity + delta - jiffies;
 		goto out;

Signature · 1 line

2.33.1

Kalle Valo

2022-01-11 10:35 UTC #2 lore raw

Re: [PATCH 1/2] mt76: mt7921: fix a leftover race in runtime-pm

In reply to Lorenzo Bianconi

Quoted 15 lines · Lorenzo Bianconi

Fix a possible race in mt7921_pm_power_save_work() if rx/tx napi schedules ps_work and we are currently accessing device register on a different cpu.
Fixes 1d8efc741df8 ("mt76: mt7921: introduce Runtime PM support")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/net/wireless/mediatek/mt76/mt7921/mac.c +0 -0
index defef3496246..0744f6e42ba3 100644

drivers/net/wireless/mediatek/mt76/mt7921/mac.c void mt7921_pm_power_save_work(struct work_struct *work) · line 1562

Quoted 10 lines

drivers/net/wireless/mediatek/mt76/mt7921/mac.c +7 -0

@@ -1553,6 +1553,14 @@ void mt7921_pm_power_save_work(struct work_struct *work)
 	    dev->fw_assert)
 		goto out;
 
+	if (mutex_is_locked(&dev->mt76.mutex))
+		/* if mt76 mutex is held we should not put the device
+		 * to sleep since we are currently accessing device
+		 * register map. We need to wait for the next power_save
+		 * trigger.
+		 */
+		goto out;

This looks fishy to me. What protects the case when ps_work is run first and at the same time another cpu starts accessing the registers?

Do note that I didn't check the code, so I might be missing something.

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Felix Fietkau

maintainer

2022-01-11 10:54 UTC #3 lore raw

Re: [PATCH 1/2] mt76: mt7921: fix a leftover race in runtime-pm

Quoted 31 lines · Kalle Valo

Quoted 15 lines · Lorenzo Bianconi
Fix a possible race in mt7921_pm_power_save_work() if rx/tx napi schedules ps_work and we are currently accessing device register on a different cpu.
Fixes 1d8efc741df8 ("mt76: mt7921: introduce Runtime PM support")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/net/wireless/mediatek/mt76/mt7921/mac.c +0 -0
index defef3496246..0744f6e42ba3 100644
drivers/net/wireless/mediatek/mt76/mt7921/mac.c void mt7921_pm_power_save_work(struct work_struct *work) · line 1562
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/mt7921/mac.c +7 -0
@@ -1553,6 +1553,14 @@ void mt7921_pm_power_save_work(struct work_struct *work)
 	    dev->fw_assert)
 		goto out;
 
+	if (mutex_is_locked(&dev->mt76.mutex))
+		/* if mt76 mutex is held we should not put the device
+		 * to sleep since we are currently accessing device
+		 * register map. We need to wait for the next power_save
+		 * trigger.
+		 */
+		goto out;
This looks fishy to me. What protects the case when ps_work is run first and at the same time another cpu starts accessing the registers?

Quoted 1 line

Do note that I didn't check the code, so I might be missing something.

For atomic context there is a locked counter pm->wake.count which is used to prevent the device from going to sleep. If the device is sleeping already on irq/tx, it is woken up and the function is rescheduled. The device is never put to sleep while the wake count is non-zero.

For non-atomic context, the mutex is always held. There is a wrapper for acquiring and releasing the mutex, which cancels the work after acquiring the mutex and reschedules the delayed work after updating the last activity timestamp (which gets checked here after checking the mutex).

The corner case that needs this mutex check here is when the work was scheduled again after processing some NAPI, tx or irq activity and the work gets run all while another cpu is in the middle of a long running non-atomic activity that holds the mutex.

For that we really do need the simple mutex_is_locked check, since actually holding the lock here would cause a deadlock with the mutex_acquire wrapper.

- Felix

Lorenzo Bianconi

author maintainer

2022-01-11 10:59 UTC #5 lore raw

Re: [PATCH 1/2] mt76: mt7921: fix a leftover race in runtime-pm

In reply to Kalle Valo

Quoted 31 lines

Quoted 15 lines · Lorenzo Bianconi
Fix a possible race in mt7921_pm_power_save_work() if rx/tx napi schedules ps_work and we are currently accessing device register on a different cpu.
Fixes 1d8efc741df8 ("mt76: mt7921: introduce Runtime PM support")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/net/wireless/mediatek/mt76/mt7921/mac.c +0 -0
index defef3496246..0744f6e42ba3 100644
drivers/net/wireless/mediatek/mt76/mt7921/mac.c void mt7921_pm_power_save_work(struct work_struct *work) · line 1562
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/mt7921/mac.c +7 -0
@@ -1553,6 +1553,14 @@ void mt7921_pm_power_save_work(struct work_struct *work)
 	    dev->fw_assert)
 		goto out;
 
+	if (mutex_is_locked(&dev->mt76.mutex))
+		/* if mt76 mutex is held we should not put the device
+		 * to sleep since we are currently accessing device
+		 * register map. We need to wait for the next power_save
+		 * trigger.
+		 */
+		goto out;
This looks fishy to me. What protects the case when ps_work is run first and at the same time another cpu starts accessing the registers?

Quoted 1 line

Do note that I didn't check the code, so I might be missing something.

before accessing chip registers, we run mt7921_mutex_acquire() so we grab mt76 mutex and run mt76_connac_pm_wake(). In mt76_connac_pm_wake() we cancel ps_work, so it is not possible to access regs while mt7921_pm_power_save_work() is running. The only leftover case is the other way around, i.e. if we schedule mt7921_pm_power_save_work while we are already reading/writing chip regs. This is only possible when mt7921_pm_power_save_work is scheduled by rx_napi and this patch is fixing the latter case. Agree?

Regards,
Lorenzo

Quoted 5 lines

Signature · 4 lines

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Kalle Valo

2022-01-11 15:01 UTC #4 lore raw

Re: [PATCH 1/2] mt76: mt7921: fix a leftover race in runtime-pm

In reply to Felix Fietkau

Quoted 44 lines · Felix Fietkau

Quoted 30 lines · Kalle Valo
Quoted 15 lines · Lorenzo Bianconi
Fix a possible race in mt7921_pm_power_save_work() if rx/tx napi schedules ps_work and we are currently accessing device register on a different cpu.
Fixes 1d8efc741df8 ("mt76: mt7921: introduce Runtime PM support")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/net/wireless/mediatek/mt76/mt7921/mac.c +0 -0
index defef3496246..0744f6e42ba3 100644
drivers/net/wireless/mediatek/mt76/mt7921/mac.c void mt7921_pm_power_save_work(struct work_struct *work) · line 1561
Quoted 9 lines
drivers/net/wireless/mediatek/mt76/mt7921/mac.c +6 -0
@@ -1553,6 +1553,14 @@ void mt7921_pm_power_save_work(struct work_struct *work)
 	    dev->fw_assert)
 		goto out;
 +	if (mutex_is_locked(&dev->mt76.mutex))
+		/* if mt76 mutex is held we should not put the device
+		 * to sleep since we are currently accessing device
+		 * register map. We need to wait for the next power_save
+		 * trigger.
+		 */
+		goto out;
This looks fishy to me. What protects the case when ps_work is run first and at the same time another cpu starts accessing the registers?
Quoted 1 line
Do note that I didn't check the code, so I might be missing something.
For atomic context there is a locked counter pm->wake.count which is used to prevent the device from going to sleep. If the device is sleeping already on irq/tx, it is woken up and the function is rescheduled. The device is never put to sleep while the wake count is non-zero.
For non-atomic context, the mutex is always held. There is a wrapper for acquiring and releasing the mutex, which cancels the work after acquiring the mutex and reschedules the delayed work after updating the last activity timestamp (which gets checked here after checking the mutex).

Quoted 8 lines

The corner case that needs this mutex check here is when the work was scheduled again after processing some NAPI, tx or irq activity and the work gets run all while another cpu is in the middle of a long running non-atomic activity that holds the mutex.
For that we really do need the simple mutex_is_locked check, since actually holding the lock here would cause a deadlock with the mutex_acquire wrapper.

Very good, thanks Felix and Lorenzo for explaining all this. I just always get wary when I see mutex_is_locked() being used.

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches