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>,
        <Mark-YW.Chen@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>, <Eric.Liang@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/2] mt76: mt7921s: make pm->suspended usage consistent
Date:   Tue, 14 Dec 2021 02:58:05 +0800
Message-ID: <1639421885-6126-1-git-send-email-sean.wang@mediatek.com>
In-Reply-To: <YbcYJ47OKfh0A5Vt@lore-desk--annotate>
References: <YbcYJ47OKfh0A5Vt@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>
>>
>> >> From: Sean Wang <sean.wang@mediatek.com>
>> >>
>> >> Update pm->suspended usage to be consistent with mt7921e driver.
>> >>
>> >> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>> >> ---
>> >>  drivers/net/wireless/mediatek/mt76/mt7921/sdio.c | 7 +++++--
>> >>  1 file changed, 5 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> >> b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> >> index 84be229a899d..44ee9369f6bf 100644
>> >> --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> >> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> >> @@ -278,7 +278,6 @@ static int mt7921s_resume(struct device *__dev)
>> >>	struct mt76_dev *mdev = &dev->mt76;
>> >>	int err;
>> >>
>> >> -	pm->suspended = false;
>> >>	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>> >>
>> >>	err = mt7921_mcu_drv_pmctrl(dev);
>> >> @@ -294,7 +293,11 @@ static int mt7921s_resume(struct device *__dev)
>> >>	if (!pm->ds_enable)
>> >>		mt76_connac_mcu_set_deep_sleep(mdev, false);
>> >>
>> >> -	return mt76_connac_mcu_set_hif_suspend(mdev, false);
>> >> +	err = mt76_connac_mcu_set_hif_suspend(mdev, false);
>> >
>> >should we check return value here? Something like:
>> >
>> >	if (err)
>> >		return err;
>> >
>> >	pm->suspended = false;
>> >	return 0;
>> >
>> >Or, is the chip up even if mt76_connac_mcu_set_hif_suspend() fails?
>>
>> yes, chip is eventually up again by recovered with the following wifi
>> reset
>>
>> with current logic, if do so (not mark pm->suspended back as false to
>> show suspend/resume is over),
>>
>> the pm runtime would not be enabled again after the wifi reset
>
>maybe we should just set pm->suspended = false; in mt7921_mac_reset_work() as we do for hw_full_reset, wdyt?

That looks fine to me. I will submit another patch for that prior to the patch.

>
>Regards,
>Lorenzo
>
>>
>> >> +
>> >> +	pm->suspended = false;
>> >> +
>> >> +	return err;
>> >>  }
>> >>
>> >>  static const struct dev_pm_ops mt7921s_pm_ops = {
>> >> --
>> >> 2.25.1
>> >>