[PATCH 0/2] mt76: fix a leftover race in runtime-pm for mt7663/mt7921

[PATCH 0/2] mt76: fix a leftover race in runtime-pm for mt7663/mt7921

[Lorenzo Bianconi]

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(+)

-- 
2.33.1

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

[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 <lorenzo@kernel.org>
---
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
index defef3496246..0744f6e42ba3 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
@@ -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;
-- 
2.33.1

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

[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 <lorenzo@kernel.org>
---
 drivers/net/wireless/mediatek/mt76/mt7615/mac.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mac.c b/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
index 2d81cbf2600c..bc72791cdcb5 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
@@ -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;
-- 
2.33.1

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

[Kalle Valo]

Lorenzo Bianconi <lorenzo@kernel.org> writes:

> 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 <lorenzo@kernel.org>
> ---
>  drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> index defef3496246..0744f6e42ba3 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> @@ -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.

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

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

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

[Felix Fietkau]

On 2022-01-11 11:35, Kalle Valo wrote:
> Lorenzo Bianconi <lorenzo@kernel.org> writes:
> 
>> 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 <lorenzo@kernel.org>
>> ---
>>  drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>> index defef3496246..0744f6e42ba3 100644
>> --- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>> @@ -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.
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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

> Lorenzo Bianconi <lorenzo@kernel.org> writes:
> 
> > 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 <lorenzo@kernel.org>
> > ---
> >  drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> > index defef3496246..0744f6e42ba3 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> > @@ -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.

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

> 
> -- 
> https://patchwork.kernel.org/project/linux-wireless/list/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Kalle Valo]

Felix Fietkau <nbd@nbd.name> writes:

> On 2022-01-11 11:35, Kalle Valo wrote:
>> Lorenzo Bianconi <lorenzo@kernel.org> writes:
>>
>>> 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 <lorenzo@kernel.org>
>>> ---
>>>  drivers/net/wireless/mediatek/mt76/mt7921/mac.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>>> index defef3496246..0744f6e42ba3 100644
>>> --- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>>> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>>> @@ -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.
> 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.

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

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

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