Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   <sean.wang@mediatek.com>
To:     <lorenzo.bianconi@redhat.com>
CC:     <nbd@nbd.name>, <sean.wang@mediatek.com>,
        <Soul.Huang@mediatek.com>, <YN.Chen@mediatek.com>,
        <Leon.Yen@mediatek.com>, <Eric-SY.Chang@mediatek.com>,
        <Deren.Wu@mediatek.com>, <km.lin@mediatek.com>,
        <jenhao.yang@mediatek.com>, <robin.chiu@mediatek.com>,
        <Eddie.Chen@mediatek.com>, <ch.yeh@mediatek.com>,
        <posh.sun@mediatek.com>, <ted.huang@mediatek.com>,
        <Stella.Chang@mediatek.com>, <Tom.Chou@mediatek.com>,
        <steve.lee@mediatek.com>, <jsiuda@google.com>,
        <frankgor@google.com>, <jemele@google.com>,
        <abhishekpandit@google.com>, <shawnku@google.com>,
        <linux-wireless@vger.kernel.org>,
        <linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH 1/3] mt76: mt7921s: fix the deadlock caused by sdio->stat_work
Date:   Thu, 21 Jul 2022 06:32:36 +0800
Message-ID: <1658356356-29058-1-git-send-email-sean.wang@mediatek.com>
In-Reply-To: <YtXYffraDN7RxoTr@lore-desk--annotate>
References: <YtXYffraDN7RxoTr@lore-desk--annotate>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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