[PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work

[PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Because wake_work and sdio->stat_work share the same workqueue mt76->wq,
if sdio->stat_work cannot acquire the mutex lock such as that was possibly
held up by mt7921_mutex_acquire, we should exit immediately and schedule
another stat_work to avoid blocking the mt7921_mutex_acquire.

Also, if mt7921_mutex_acquire was called by sdio->stat_work self, the wake
would be blocked by itself, so we have to changing into an unblocking wake
(directly wakeup via mt7921_mcu_drv_pmctrl, not via the wake_work) in the
context.

Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
Co-developed-by: YN Chen <YN.Chen@mediatek.com>
Signed-off-by: YN Chen <YN.Chen@mediatek.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../net/wireless/mediatek/mt76/mt7921/mac.c   | 22 +++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
index 6bd9fc9228a2..75e719175e92 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
@@ -1080,10 +1080,28 @@ bool mt7921_usb_sdio_tx_status_data(struct mt76_dev *mdev, u8 *update)
 {
 	struct mt7921_dev *dev = container_of(mdev, struct mt7921_dev, mt76);
 
-	mt7921_mutex_acquire(dev);
+	if (!mutex_trylock(&mdev->mutex)) {
+		/* Because wake_work and stat_work share the same workqueue
+		 * mt76->wq, if sdio->stat_work cannot acquire the mutex lock,
+		 * we should exit immediately and schedule another stat_work
+		 * to avoid blocking the wake_work.
+		 */
+		struct work_struct *stat_work;
+
+		stat_work = mt76_is_sdio(mdev) ? &mdev->sdio.stat_work :
+			    &mdev->usb.stat_work;
+		queue_work(dev->mt76.wq, stat_work);
+
+		goto out;
+	}
+
+	mt7921_mcu_drv_pmctrl(dev);
 	mt7921_mac_sta_poll(dev);
-	mt7921_mutex_release(dev);
+	mt76_connac_power_save_sched(&mdev->phy, &dev->pm);
 
+	mutex_unlock(&mdev->mutex);
+
+out:
 	return false;
 }
 EXPORT_SYMBOL_GPL(mt7921_usb_sdio_tx_status_data);
-- 
2.25.1

[PATCH 2/3] mt76: sdio: poll sta stat when device transmits data

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

It is not meaningful to poll sta stat when there is no the data traffic.
So polling sta stat when device have transmitted data instead to save
CPU power.

That implies that it is unallowed the stat_work to work while MCU is
being initialized in the really early stage. That is a required patch
for ("mt76: mt7663s: fix the deadlock caused by sdio->stat_work")
because mt7615_mcu_set_drv_ctrl pointer isn't set done until MCU is
ready.

Fixes: d39b52e31aa6 ("mt76: introduce mt76_sdio module")
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/wireless/mediatek/mt76/sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
index aba2a9865821..3b9bb7cd08ad 100644
--- a/drivers/net/wireless/mediatek/mt76/sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/sdio.c
@@ -478,7 +478,7 @@ static void mt76s_status_worker(struct mt76_worker *w)
 		if (ndata_frames > 0)
 			resched = true;
 
-		if (dev->drv->tx_status_data &&
+		if (dev->drv->tx_status_data && ndata_frames > 0 &&
 		    !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);
-- 
2.25.1

[PATCH 3/3] mt76: mt7663s: fix the deadlock caused by sdio->stat_work

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Because wake_work and sdio->stat_work share the same workqueue mt76->wq,
if sdio->stat_work cannot acquire the mutex lock such as that was possibly
held up by mt7615_mutex_acquire, we should exit immediately and schedule
another stat_work to avoid blocking the mt7615_mutex_acquire.

Also, if mt7615_mutex_acquire was called by sdio->stat_work self, the wake
would be blocked by itself, so we have to changing into an unblocking wake
(directly wakeup via mt7615_mcu_drv_pmctrl, not via the wake_work) in the
context.

Fixes: a66cbdd6573d ("mt76: mt7615: introduce mt7663s support")
Co-developed-by: YN Chen <YN.Chen@mediatek.com>
Signed-off-by: YN Chen <YN.Chen@mediatek.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../wireless/mediatek/mt76/mt7615/usb_sdio.c  | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/usb_sdio.c b/drivers/net/wireless/mediatek/mt76/mt7615/usb_sdio.c
index 0052d103e276..5991b23e0d13 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/usb_sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/usb_sdio.c
@@ -157,10 +157,29 @@ bool mt7663_usb_sdio_tx_status_data(struct mt76_dev *mdev, u8 *update)
 {
 	struct mt7615_dev *dev = container_of(mdev, struct mt7615_dev, mt76);
 
-	mt7615_mutex_acquire(dev);
+	if (!mutex_trylock(&mdev->mutex)) {
+		/* Because wake_work and stat_work share the same workqueue
+		 * mt76->wq, if sdio->stat_work cannot acquire the mutex lock,
+		 * we should exit immediately and schedule another stat_work
+		 * to avoid blocking the wake_work.
+		 */
+		struct work_struct *stat_work;
+
+		stat_work = mt76_is_sdio(mdev) ? &mdev->sdio.stat_work :
+			    &mdev->usb.stat_work;
+		queue_work(dev->mt76.wq, stat_work);
+
+		goto out;
+	}
+
+	mt7615_mcu_set_drv_ctrl(dev);
+
 	mt7615_mac_sta_poll(dev);
-	mt7615_mutex_release(dev);
 
+	mt76_connac_power_save_sched(&mdev->phy, &dev->pm);
+	mutex_unlock(&mdev->mutex);
+
+out:
 	return false;
 }
 EXPORT_SYMBOL_GPL(mt7663_usb_sdio_tx_status_data);
-- 
2.25.1

Re: [PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work

[Lorenzo Bianconi]

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

> From: Sean Wang <sean.wang@mediatek.com>
> 
> Because wake_work and sdio->stat_work share the same workqueue mt76->wq,
> if sdio->stat_work cannot acquire the mutex lock such as that was possibly
> held up by mt7921_mutex_acquire, we should exit immediately and schedule
> another stat_work to avoid blocking the mt7921_mutex_acquire.
> 
> Also, if mt7921_mutex_acquire was called by sdio->stat_work self, the wake
> would be blocked by itself, so we have to changing into an unblocking wake
> (directly wakeup via mt7921_mcu_drv_pmctrl, not via the wake_work) in the
> context.

Hi Sean,

it seems to me we are missing some logic here (e.g cancelling ps_work as we do
in mt76_connac_pm_wake()). Is it enough to move mt7921_usb_sdio_tx_status_data
on mac80211 workqueue? Can you see any performance issue? (same for mt7663).

Regards,
Lorenzo

> 
> Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
> Co-developed-by: YN Chen <YN.Chen@mediatek.com>
> Signed-off-by: YN Chen <YN.Chen@mediatek.com>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../net/wireless/mediatek/mt76/mt7921/mac.c   | 22 +++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> index 6bd9fc9228a2..75e719175e92 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
> @@ -1080,10 +1080,28 @@ bool mt7921_usb_sdio_tx_status_data(struct mt76_dev *mdev, u8 *update)
>  {
>  	struct mt7921_dev *dev = container_of(mdev, struct mt7921_dev, mt76);
>  
> -	mt7921_mutex_acquire(dev);
> +	if (!mutex_trylock(&mdev->mutex)) {
> +		/* Because wake_work and stat_work share the same workqueue
> +		 * mt76->wq, if sdio->stat_work cannot acquire the mutex lock,
> +		 * we should exit immediately and schedule another stat_work
> +		 * to avoid blocking the wake_work.
> +		 */
> +		struct work_struct *stat_work;
> +
> +		stat_work = mt76_is_sdio(mdev) ? &mdev->sdio.stat_work :
> +			    &mdev->usb.stat_work;
> +		queue_work(dev->mt76.wq, stat_work);
> +
> +		goto out;
> +	}
> +
> +	mt7921_mcu_drv_pmctrl(dev);
>  	mt7921_mac_sta_poll(dev);
> -	mt7921_mutex_release(dev);
> +	mt76_connac_power_save_sched(&mdev->phy, &dev->pm);
>  
> +	mutex_unlock(&mdev->mutex);
> +
> +out:
>  	return false;
>  }
>  EXPORT_SYMBOL_GPL(mt7921_usb_sdio_tx_status_data);
> -- 
> 2.25.1
> 

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

Re: [PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

>>> From: Sean Wang <sean.wang@mediatek.com>
>>
>> Because wake_work and sdio->stat_work share the same workqueue
>> mt76->wq, if sdio->stat_work cannot acquire the mutex lock such as
>> that was possibly held up by mt7921_mutex_acquire, we should exit
>> immediately and schedule another stat_work to avoid blocking the mt7921_mutex_acquire.
>>
>> Also, if mt7921_mutex_acquire was called by sdio->stat_work self, the
>> wake would be blocked by itself, so we have to changing into an
>> unblocking wake (directly wakeup via mt7921_mcu_drv_pmctrl, not via
>> the wake_work) in the context.
>
>Hi Sean,
>
>it seems to me we are missing some logic here (e.g cancelling ps_work as we do in mt76_connac_pm_wake()). Is it enough to move mt7921_usb_sdio_tx_status_data on mac80211 workqueue? Can you see any performance issue? (same for mt7663).

I will try to reuse the mac80211 workqueue for stat_work that can help mt7921_usb_sdio_tx_status_data as is and fix the deadlock. thanks for your suggestion!

>
>Regards,
>Lorenzo
>
>>
>> Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
>> Co-developed-by: YN Chen <YN.Chen@mediatek.com>
>> Signed-off-by: YN Chen <YN.Chen@mediatek.com>
>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>> ---
>>  .../net/wireless/mediatek/mt76/mt7921/mac.c   | 22 +++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>> b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>> index 6bd9fc9228a2..75e719175e92 100644
>> --- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
>> @@ -1080,10 +1080,28 @@ bool mt7921_usb_sdio_tx_status_data(struct
>> mt76_dev *mdev, u8 *update)  {
>>	struct mt7921_dev *dev = container_of(mdev, struct mt7921_dev,
>> mt76);
>>
>> -	mt7921_mutex_acquire(dev);
>> +	if (!mutex_trylock(&mdev->mutex)) {
>> +		/* Because wake_work and stat_work share the same workqueue
>> +		 * mt76->wq, if sdio->stat_work cannot acquire the mutex lock,
>> +		 * we should exit immediately and schedule another stat_work
>> +		 * to avoid blocking the wake_work.
>> +		 */
>> +		struct work_struct *stat_work;
>> +
>> +		stat_work = mt76_is_sdio(mdev) ? &mdev->sdio.stat_work :
>> +			    &mdev->usb.stat_work;
>> +		queue_work(dev->mt76.wq, stat_work);
>> +
>> +		goto out;
>> +	}
>> +
>> +	mt7921_mcu_drv_pmctrl(dev);
>>	mt7921_mac_sta_poll(dev);
>> -	mt7921_mutex_release(dev);
>> +	mt76_connac_power_save_sched(&mdev->phy, &dev->pm);
>>
>> +	mutex_unlock(&mdev->mutex);
>> +
>> +out:
>>	return false;
>>  }
>>  EXPORT_SYMBOL_GPL(mt7921_usb_sdio_tx_status_data);
>> --
>> 2.25.1
>>
>