[net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[wojackbb]

From: Jack Wu <wojackbb@gmail.com>

Because optimizing the power consumption of t7XX,
change auto suspend time to 5000.

The Tests uses a script to loop through the power_state
of t7XX.
(for example: /sys/bus/pci/devices/0000\:72\:00.0/power_state)

* If Auto suspend is 20 seconds,
  test script show power_state have 0~5% of the time was in D3 state
  when host don't have data packet transmission.

* Changed auto suspend time to 5 seconds,
  test script show power_state have 50%~80% of the time was in D3 state
  when host don't have data packet transmission.

We tested Fibocom FM350 and our products using the t7xx and they all
benefited from this.

Signed-off-by: Jack Wu <wojackbb@gmail.com>
---
V2:
 * supplementary commit information
---
---
 drivers/net/wwan/t7xx/t7xx_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
index e556e5bd49ab..dcadd615a025 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.c
+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
@@ -48,7 +48,7 @@
 #define T7XX_INIT_TIMEOUT		20
 #define PM_SLEEP_DIS_TIMEOUT_MS		20
 #define PM_ACK_TIMEOUT_MS		1500
-#define PM_AUTOSUSPEND_MS		20000
+#define PM_AUTOSUSPEND_MS		5000
 #define PM_RESOURCE_POLL_TIMEOUT_US	10000
 #define PM_RESOURCE_POLL_STEP_US	100
 
-- 
2.34.1

Re: [net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[Andrew Lunn]

On Thu, Nov 14, 2024 at 06:20:02PM +0800, wojackbb@gmail.com wrote:
> From: Jack Wu <wojackbb@gmail.com>
> 
> Because optimizing the power consumption of t7XX,
> change auto suspend time to 5000.
> 
> The Tests uses a script to loop through the power_state
> of t7XX.
> (for example: /sys/bus/pci/devices/0000\:72\:00.0/power_state)
> 
> * If Auto suspend is 20 seconds,
>   test script show power_state have 0~5% of the time was in D3 state
>   when host don't have data packet transmission.
> 
> * Changed auto suspend time to 5 seconds,
>   test script show power_state have 50%~80% of the time was in D3 state
>   when host don't have data packet transmission.
> 
> We tested Fibocom FM350 and our products using the t7xx and they all
> benefited from this.
> 
> Signed-off-by: Jack Wu <wojackbb@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[Sergey Ryazanov]

Hello Jack,

On 14.11.2024 12:20, wojackbb@gmail.com wrote:
> From: Jack Wu <wojackbb@gmail.com>
> 
> Because optimizing the power consumption of t7XX,
> change auto suspend time to 5000.
> 
> The Tests uses a script to loop through the power_state
> of t7XX.
> (for example: /sys/bus/pci/devices/0000\:72\:00.0/power_state)
> 
> * If Auto suspend is 20 seconds,
>    test script show power_state have 0~5% of the time was in D3 state
>    when host don't have data packet transmission.
> 
> * Changed auto suspend time to 5 seconds,
>    test script show power_state have 50%~80% of the time was in D3 state
>    when host don't have data packet transmission.
> 
> We tested Fibocom FM350 and our products using the t7xx and they all
> benefited from this.

Possible negative outcomes for data transmission still need 
clarification. Let me repeat it here.

On 06.11.2024 13:10, 吳逼逼 wrote:
> Receiving or sending data will cause PCIE to change D3 Cold to D0 state.

Am I understand it correctly that receiving IP packets on downlink will 
cause PCIe link re-activation?


I am concerned about a TCP connection that can be idle for a long period 
of time. For example, an established SSH connection can stay idle for 
minutes. If I connected to a server and execute something like this:

user@host$ sleep 20 && echo "Done"

Will I eventually see the "Done" message or will the autosuspended modem 
effectively block any incoming traffic? And how long does it take for 
the modem to wake up and deliver a downlink packet to the host? Have you 
measured StDev change?

> Signed-off-by: Jack Wu <wojackbb@gmail.com>
> ---
> V2:
>   * supplementary commit information
> ---
> ---
>   drivers/net/wwan/t7xx/t7xx_pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
> index e556e5bd49ab..dcadd615a025 100644
> --- a/drivers/net/wwan/t7xx/t7xx_pci.c
> +++ b/drivers/net/wwan/t7xx/t7xx_pci.c
> @@ -48,7 +48,7 @@
>   #define T7XX_INIT_TIMEOUT		20
>   #define PM_SLEEP_DIS_TIMEOUT_MS		20
>   #define PM_ACK_TIMEOUT_MS		1500
> -#define PM_AUTOSUSPEND_MS		20000
> +#define PM_AUTOSUSPEND_MS		5000
>   #define PM_RESOURCE_POLL_TIMEOUT_US	10000
>   #define PM_RESOURCE_POLL_STEP_US	100
>   

Re: [net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[Jakub Kicinski]

On Thu, 14 Nov 2024 20:54:20 +0200 Sergey Ryazanov wrote:
> > We tested Fibocom FM350 and our products using the t7xx and they all
> > benefited from this.  
> 
> Possible negative outcomes for data transmission still need 
> clarification. Let me repeat it here.
> 
> On 06.11.2024 13:10, 吳逼逼 wrote:
> > Receiving or sending data will cause PCIE to change D3 Cold to D0 state.  
> 
> Am I understand it correctly that receiving IP packets on downlink will 
> cause PCIe link re-activation?
> 
> 
> I am concerned about a TCP connection that can be idle for a long period 
> of time. For example, an established SSH connection can stay idle for 
> minutes. If I connected to a server and execute something like this:
> 
> user@host$ sleep 20 && echo "Done"
> 
> Will I eventually see the "Done" message or will the autosuspended modem 
> effectively block any incoming traffic? And how long does it take for 
> the modem to wake up and deliver a downlink packet to the host? Have you 
> measured StDev change?

He's decreasing the sleep timer from 20 to 5 sec, both of which 
are very high for networking, anyway. You appear to be questioning
autosuspend itself but it seems to have been added 2 years ago already.

What am I missing?

Re: [net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[Sergey Ryazanov]

On 16.11.2024 01:21, Jakub Kicinski wrote:
> On Thu, 14 Nov 2024 20:54:20 +0200 Sergey Ryazanov wrote:
>>> We tested Fibocom FM350 and our products using the t7xx and they all
>>> benefited from this.
>>
>> Possible negative outcomes for data transmission still need
>> clarification. Let me repeat it here.
>>
>> On 06.11.2024 13:10, 吳逼逼 wrote:
>>> Receiving or sending data will cause PCIE to change D3 Cold to D0 state.
>>
>> Am I understand it correctly that receiving IP packets on downlink will
>> cause PCIe link re-activation?
>>
>>
>> I am concerned about a TCP connection that can be idle for a long period
>> of time. For example, an established SSH connection can stay idle for
>> minutes. If I connected to a server and execute something like this:
>>
>> user@host$ sleep 20 && echo "Done"
>>
>> Will I eventually see the "Done" message or will the autosuspended modem
>> effectively block any incoming traffic? And how long does it take for
>> the modem to wake up and deliver a downlink packet to the host? Have you
>> measured StDev change?
> 
> He's decreasing the sleep timer from 20 to 5 sec, both of which
> are very high for networking, anyway. You appear to be questioning
> autosuspend itself but it seems to have been added 2 years ago already.
> 
> What am I missing?

Some possible funny side-effect of sleeping with this chipset. Like 
loosing network connection and dropping TCP sessions. I hope that 20 
seconds was putted on purpose.

Suddenly, I don't have this modem at hand and want to be sure that we 
are not going to receive a stream of bug reports.

--
Sergey

Re: [net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[Jakub Kicinski]

On Tue, 19 Nov 2024 03:01:47 +0200 Sergey Ryazanov wrote:
> > He's decreasing the sleep timer from 20 to 5 sec, both of which
> > are very high for networking, anyway. You appear to be questioning
> > autosuspend itself but it seems to have been added 2 years ago already.
> > 
> > What am I missing?  
> 
> Some possible funny side-effect of sleeping with this chipset. Like 
> loosing network connection and dropping TCP sessions. I hope that 20 
> seconds was putted on purpose.
> 
> Suddenly, I don't have this modem at hand and want to be sure that we 
> are not going to receive a stream of bug reports.

Power saving is always tricky, but they say they tested. I think we
should give it a go, worst case - it will be an easy revert.

Re: [net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[Sergey Ryazanov]

On 19.11.2024 03:44, Jakub Kicinski wrote:
> On Tue, 19 Nov 2024 03:01:47 +0200 Sergey Ryazanov wrote:
>>> He's decreasing the sleep timer from 20 to 5 sec, both of which
>>> are very high for networking, anyway. You appear to be questioning
>>> autosuspend itself but it seems to have been added 2 years ago already.
>>>
>>> What am I missing?
>>
>> Some possible funny side-effect of sleeping with this chipset. Like
>> loosing network connection and dropping TCP sessions. I hope that 20
>> seconds was putted on purpose.
>>
>> Suddenly, I don't have this modem at hand and want to be sure that we
>> are not going to receive a stream of bug reports.
> 
> Power saving is always tricky, but they say they tested. I think we
> should give it a go, worst case - it will be an easy revert.

Make sense. Then let's merge it.

Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Re: [net-next,v2] [PATCH net-next v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 14 Nov 2024 18:20:02 +0800 you wrote:
> From: Jack Wu <wojackbb@gmail.com>
> 
> Because optimizing the power consumption of t7XX,
> change auto suspend time to 5000.
> 
> The Tests uses a script to loop through the power_state
> of t7XX.
> (for example: /sys/bus/pci/devices/0000\:72\:00.0/power_state)
> 
> [...]

Here is the summary with links:
  - [net-next,v2,net-next,v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000
    https://git.kernel.org/netdev/net-next/c/a0c80d5108ab

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html