[PATCH 1/2] mt76: mt7921s: make pm->suspended usage consistent

[PATCH 1/2] mt76: mt7921s: make pm->suspended usage consistent

[sean.wang]

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);
+
+	pm->suspended = false;
+
+	return err;
 }
 
 static const struct dev_pm_ops mt7921s_pm_ops = {
-- 
2.25.1

[PATCH 2/2] mt76: mt7921s: fix suspend error with enlarging mcu timeout value

[sean.wang]

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

Fix the false positive suspend error that may occur on mt7921s
with enlarging mcu timeout value.

The reason why we have to enlarge mcu timeout from HZ / 3 to HZ is
we should consider the additional overhead caused by running
concurrently with btmtksdio (a MT7921 bluetooth SDIO driver) that
would compete for the same SDIO bus in process context to complete
the suspend procedure.

Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/wireless/mediatek/mt76/mt7921/mcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c
index 1227d626e9d3..be87e134216b 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c
@@ -239,7 +239,7 @@ int mt7921_mcu_fill_message(struct mt76_dev *mdev, struct sk_buff *skb,
 	case MCU_UNI_CMD_HIF_CTRL:
 	case MCU_UNI_CMD_SUSPEND:
 	case MCU_UNI_CMD_OFFLOAD:
-		mdev->mcu.timeout = HZ / 3;
+		mdev->mcu.timeout = HZ;
 		break;
 	default:
 		mdev->mcu.timeout = 3 * HZ;
-- 
2.25.1

Re: [PATCH 1/2] mt76: mt7921s: make pm->suspended usage consistent

[Lorenzo Bianconi]

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

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

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

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

Re: [PATCH 1/2] mt76: mt7921s: make pm->suspended usage consistent

[sean.wang]

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

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

Re: [PATCH 1/2] mt76: mt7921s: make pm->suspended usage consistent

[Lorenzo Bianconi]

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

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

Regards,
Lorenzo

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

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

Re: [PATCH 1/2] mt76: mt7921s: make pm->suspended usage consistent

[sean.wang]

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