diff mbox series

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

Message ID 20241114102002.481081-1-wojackbb@gmail.com (mailing list archive)
State Accepted
Commit a0c80d5108ab36cf6cc974a24ce9c522f271d754
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2,net-next,v2] net: wwan: t7xx: Change PM_AUTOSUSPEND_MS to 5000 | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: andrew+netdev@lunn.ch
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-11-15--21-00 (tests: 789)

Commit Message

吳逼逼 Nov. 14, 2024, 10:20 a.m. UTC 
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(-)

Comments

Andrew Lunn Nov. 14, 2024, 3:33 p.m. UTC | #1 
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
Sergey Ryazanov Nov. 14, 2024, 6:54 p.m. UTC | #2 
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
>
Jakub Kicinski Nov. 15, 2024, 11:21 p.m. UTC | #3 
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?
Sergey Ryazanov Nov. 19, 2024, 1:01 a.m. UTC | #4 
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
Jakub Kicinski Nov. 19, 2024, 1:44 a.m. UTC | #5 
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.
Sergey Ryazanov Nov. 19, 2024, 1:50 a.m. UTC | #6 
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>
patchwork-bot+netdevbpf@kernel.org Nov. 19, 2024, 2:50 a.m. UTC | #7 
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!
diff mbox series

Patch

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