From: Sean Wang <[email protected]>
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 <[email protected]>
Signed-off-by: YN Chen <[email protected]>
Signed-off-by: Sean Wang <[email protected]>
---
.../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
> From: Sean Wang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: YN Chen <[email protected]>
> Signed-off-by: Sean Wang <[email protected]>
> ---
> .../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
>
From: Sean Wang <[email protected]>
>>> From: Sean Wang <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: YN Chen <[email protected]>
>> Signed-off-by: Sean Wang <[email protected]>
>> ---
>> .../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
>>
>