All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work
       [not found] <YtXYffraDN7RxoTr@lore-desk--annotate>
@ 2022-07-20 22:32 ` sean.wang
  0 siblings, 0 replies; 3+ messages in thread
From: sean.wang @ 2022-07-20 22:32 UTC (permalink / raw)
  To: lorenzo.bianconi
  Cc: nbd, sean.wang, Soul.Huang, YN.Chen, Leon.Yen, Eric-SY.Chang,
	Deren.Wu, km.lin, jenhao.yang, robin.chiu, Eddie.Chen, ch.yeh,
	posh.sun, ted.huang, Stella.Chang, Tom.Chou, steve.lee, jsiuda,
	frankgor, jemele, abhishekpandit, shawnku, linux-wireless,
	linux-mediatek

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work
  2022-07-18 20:52 sean.wang
@ 2022-07-18 22:02 ` Lorenzo Bianconi
  0 siblings, 0 replies; 3+ messages in thread
From: Lorenzo Bianconi @ 2022-07-18 22:02 UTC (permalink / raw)
  To: sean.wang
  Cc: nbd, lorenzo.bianconi, Soul.Huang, YN.Chen, Leon.Yen,
	Eric-SY.Chang, Deren.Wu, km.lin, jenhao.yang, robin.chiu,
	Eddie.Chen, ch.yeh, posh.sun, ted.huang, Stella.Chang, Tom.Chou,
	steve.lee, jsiuda, frankgor, kuabhs, druth, abhishekpandit,
	shawnku, linux-wireless, linux-mediatek

[-- 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 --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work
@ 2022-07-18 20:52 sean.wang
  2022-07-18 22:02 ` Lorenzo Bianconi
  0 siblings, 1 reply; 3+ messages in thread
From: sean.wang @ 2022-07-18 20:52 UTC (permalink / raw)
  To: nbd, lorenzo.bianconi
  Cc: sean.wang, Soul.Huang, YN.Chen, Leon.Yen, Eric-SY.Chang,
	Deren.Wu, km.lin, jenhao.yang, robin.chiu, Eddie.Chen, ch.yeh,
	posh.sun, ted.huang, Stella.Chang, Tom.Chou, steve.lee, jsiuda,
	frankgor, kuabhs, druth, abhishekpandit, shawnku, linux-wireless,
	linux-mediatek

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-20 22:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <YtXYffraDN7RxoTr@lore-desk--annotate>
2022-07-20 22:32 ` [PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work sean.wang
2022-07-18 20:52 sean.wang
2022-07-18 22:02 ` Lorenzo Bianconi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.