[PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Geert Uytterhoeven]

If CONFIG_PM=n:

    drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
    drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no member named ‘runtime_auto’

If PM is disabled, the runtime_auto flag is not available, but auto
suspend is not enabled anyway.  Hence protect the check for runtime_auto
by #ifdef CONFIG_PM to fix this.

Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper to
include/linux/pm.h, which always return false if CONFIG_PM is disabled.

The only other user in non-core code (drivers/usb/core/sysfs.c) has a
big #ifdef CONFIG_PM check around all PM-related code.

Thoughts?
---
 drivers/net/usb/lan78xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d36d5ebf37f355f2..7b9ac47b2ecf9905 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3271,7 +3271,9 @@ struct rtnl_link_stats64 *lan78xx_get_stats64(struct net_device *netdev,
 	 * periodic reading from HW will prevent from entering USB auto suspend.
 	 * if autosuspend is disabled, read from HW.
 	 */
+#ifdef CONFIG_PM
 	if (!dev->udev->dev.power.runtime_auto)
+#endif
 		lan78xx_update_stats(dev);
 
 	mutex_lock(&dev->stats.access_lock);
-- 
1.9.1

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Eric Dumazet]

On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote:
> If CONFIG_PM=n:
> 
>     drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
>     drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no member named ‘runtime_auto’
> 
> If PM is disabled, the runtime_auto flag is not available, but auto
> suspend is not enabled anyway.  Hence protect the check for runtime_auto
> by #ifdef CONFIG_PM to fix this.
> 
> Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper to
> include/linux/pm.h, which always return false if CONFIG_PM is disabled.
> 
> The only other user in non-core code (drivers/usb/core/sysfs.c) has a
> big #ifdef CONFIG_PM check around all PM-related code.
> 
> Thoughts?
> ---
>  drivers/net/usb/lan78xx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d36d5ebf37f355f2..7b9ac47b2ecf9905 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -3271,7 +3271,9 @@ struct rtnl_link_stats64 *lan78xx_get_stats64(struct net_device *netdev,
>  	 * periodic reading from HW will prevent from entering USB auto suspend.
>  	 * if autosuspend is disabled, read from HW.
>  	 */
> +#ifdef CONFIG_PM
>  	if (!dev->udev->dev.power.runtime_auto)
> +#endif
>  		lan78xx_update_stats(dev);
>  
>  	mutex_lock(&dev->stats.access_lock);

Note that a ndo_get_stat64() handler is not allowed to sleep,
so the mutex_lock() is not wise...

Historically /proc/net/dev handler but also bonding ndo_get_stats() used
RCU or a rwlock or a spinlock.

So a complete fix would need to get rid of this mutex as well.

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 20 Mar 2016 09:35:52 -0700

> On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote:
>> If CONFIG_PM=n:
>> 
>>     drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
>>     drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no member named ‘runtime_auto’
>> 
>> If PM is disabled, the runtime_auto flag is not available, but auto
>> suspend is not enabled anyway.  Hence protect the check for runtime_auto
>> by #ifdef CONFIG_PM to fix this.
>> 
>> Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> ---
>> Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper to
>> include/linux/pm.h, which always return false if CONFIG_PM is disabled.
>> 
>> The only other user in non-core code (drivers/usb/core/sysfs.c) has a
>> big #ifdef CONFIG_PM check around all PM-related code.
>> 
>> Thoughts?
>> ---
>>  drivers/net/usb/lan78xx.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>> index d36d5ebf37f355f2..7b9ac47b2ecf9905 100644
>> --- a/drivers/net/usb/lan78xx.c
>> +++ b/drivers/net/usb/lan78xx.c
>> @@ -3271,7 +3271,9 @@ struct rtnl_link_stats64 *lan78xx_get_stats64(struct net_device *netdev,
>>  	 * periodic reading from HW will prevent from entering USB auto suspend.
>>  	 * if autosuspend is disabled, read from HW.
>>  	 */
>> +#ifdef CONFIG_PM
>>  	if (!dev->udev->dev.power.runtime_auto)
>> +#endif
>>  		lan78xx_update_stats(dev);
>>  
>>  	mutex_lock(&dev->stats.access_lock);
> 
> Note that a ndo_get_stat64() handler is not allowed to sleep,
> so the mutex_lock() is not wise...
> 
> Historically /proc/net/dev handler but also bonding ndo_get_stats() used
> RCU or a rwlock or a spinlock.
> 
> So a complete fix would need to get rid of this mutex as well.

This function is also buggy for another reason.  The driver needs to
capture the statistics when the runtime suspend happens.

Since it's much easier to find things wrong rather than right with
this new code, I've decided to completely revert the commit that added
lan78xx_get_stats64().

Once a correct version is implemented we can add it back.

Thanks.

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Guenter Roeck]

On 03/20/2016 03:43 AM, Geert Uytterhoeven wrote:
> If CONFIG_PM=n:
>
>      drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
>      drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no member named ‘runtime_auto’
>
> If PM is disabled, the runtime_auto flag is not available, but auto
> suspend is not enabled anyway.  Hence protect the check for runtime_auto
> by #ifdef CONFIG_PM to fix this.
>
> Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper to
> include/linux/pm.h, which always return false if CONFIG_PM is disabled.
>
> The only other user in non-core code (drivers/usb/core/sysfs.c) has a
> big #ifdef CONFIG_PM check around all PM-related code.
>
> Thoughts?

Not that it matters anymore since David reverted the original patch,
but my reason for not sending a similar patch was that I wasn't sure
if .runtime_auto should be accessed from drivers in the first place,
or if there is some logical problem with the code.

Guenter

> ---
>   drivers/net/usb/lan78xx.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d36d5ebf37f355f2..7b9ac47b2ecf9905 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -3271,7 +3271,9 @@ struct rtnl_link_stats64 *lan78xx_get_stats64(struct net_device *netdev,
>   	 * periodic reading from HW will prevent from entering USB auto suspend.
>   	 * if autosuspend is disabled, read from HW.
>   	 */
> +#ifdef CONFIG_PM
>   	if (!dev->udev->dev.power.runtime_auto)
> +#endif
>   		lan78xx_update_stats(dev);
>
>   	mutex_lock(&dev->stats.access_lock);
>

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> Sent: Sunday, March 20, 2016 6:59 PM
> To: Geert Uytterhoeven; Woojung Huh - C21699; UNGLinuxDriver; David S.
> Miller
> Cc: Rafael J. Wysocki; netdev@vger.kernel.org; linux-usb@vger.kernel.org;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM
>  
> Not that it matters anymore since David reverted the original patch,
> but my reason for not sending a similar patch was that I wasn't sure
> if .runtime_auto should be accessed from drivers in the first place,
> or if there is some logical problem with the code.
> 

Because driver can enable/disable autosuspend by usb_enable_autosuspend() and
usb_disable_autosuspend(), it would be nice to have helper to find out autosuspend status.

The code path was to utilize autosuspend power saving, when it is enabled.

Woojung

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Oliver Neukum]

On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote:
> If CONFIG_PM=n:
> 
>     drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
>     drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no
> member named ‘runtime_auto’
> 
> If PM is disabled, the runtime_auto flag is not available, but auto
> suspend is not enabled anyway.  Hence protect the check for
> runtime_auto
> by #ifdef CONFIG_PM to fix this.
> 
> Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper
> to
> include/linux/pm.h, which always return false if CONFIG_PM is
> disabled.

That is what we do for almost everything else in include/pm_runtime.h
So it is the best solution to add the stub.

	Regards
		Oliver

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.com]
> Sent: Monday, March 21, 2016 4:36 AM
> To: Geert Uytterhoeven
> Cc: Woojung Huh - C21699; UNGLinuxDriver; David S. Miller; Guenter Roeck;
> Rafael J. Wysocki; netdev@vger.kernel.org; linux-usb@vger.kernel.org;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM

Thanks for all comments.
Will look into it and submit new patch.

- Woojung

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Alan Stern]

On Mon, 21 Mar 2016, Oliver Neukum wrote:

> On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote:
> > If CONFIG_PM=n:
> > 
> >     drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
> >     drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no
> > member named ‘runtime_auto’
> > 
> > If PM is disabled, the runtime_auto flag is not available, but auto
> > suspend is not enabled anyway.  Hence protect the check for
> > runtime_auto
> > by #ifdef CONFIG_PM to fix this.
> > 
> > Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper
> > to
> > include/linux/pm.h, which always return false if CONFIG_PM is
> > disabled.
> 
> That is what we do for almost everything else in include/pm_runtime.h
> So it is the best solution to add the stub.

Guenter's question about whether drivers should try to access
runtime_auto in the first place was a good one.  A similar problem
arises in the block layer: When a block device has removable media, the
block layer probes at 1-second intervals looking for media changes.  
This defeats autosuspend in the same way as we're talking about here.

One possible solution is to export a sysfs parameter to prevent 
statistics collection (or more generally, to change the interval at 
which it occurs).

But checking the runtime_auto flag is probably not a great idea.  Even
if it isn't set, collecting statistics is likely to wait up a device
that otherwise would have remained suspended.

Perhaps the best solution is to collect the statistics only when the 
device is not suspended or is about to suspend.

Alan Stern

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Oliver Neukum]

On Mon, 2016-03-21 at 10:57 -0400, Alan Stern wrote:

> One possible solution is to export a sysfs parameter to prevent 
> statistics collection (or more generally, to change the interval at 
> which it occurs).

Surely, not performing a task can hardly be beaten in terms of power
consumption. That is not meant to be flippant, but I think these
issues are orthogonal. The question of how much to do doesn't
solve the question of doing efficiently what we do.

> But checking the runtime_auto flag is probably not a great idea.  Even
> if it isn't set, collecting statistics is likely to wait up a device
> that otherwise would have remained suspended.
> 
> Perhaps the best solution is to collect the statistics only when the 
> device is not suspended or is about to suspend.

If we know when the next activity will come, why not pass this
information down?

	Regards
		Oliver

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Alan Stern]

On Mon, 21 Mar 2016, Oliver Neukum wrote:

> On Mon, 2016-03-21 at 10:57 -0400, Alan Stern wrote:
> 
> > One possible solution is to export a sysfs parameter to prevent 
> > statistics collection (or more generally, to change the interval at 
> > which it occurs).
> 
> Surely, not performing a task can hardly be beaten in terms of power
> consumption. That is not meant to be flippant, but I think these
> issues are orthogonal. The question of how much to do doesn't
> solve the question of doing efficiently what we do.

In other words, what's the best way to collect the statistics without 
interfering with runtime PM, right?

If the device is suspended, presumably we know there's nothing to
collect -- especially if we already collected the statistics at the
time the device got suspended.  Hence my suggestion to avoid querying 
the device while it is suspended.

But this leaves open the issue that querying the device too often will 
prevent it from going into autosuspend.  It seems to me that the best 
way to deal with this is to make sure that the autosuspend timeout is 
shorter than the interal between queries, not to make the querying 
conditional on !runtime_auto.

> > But checking the runtime_auto flag is probably not a great idea.  Even
> > if it isn't set, collecting statistics is likely to wait up a device
> > that otherwise would have remained suspended.
> > 
> > Perhaps the best solution is to collect the statistics only when the 
> > device is not suspended or is about to suspend.
> 
> If we know when the next activity will come, why not pass this
> information down?

I don't follow.

Alan Stern

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> But this leaves open the issue that querying the device too often will
> prevent it from going into autosuspend.  It seems to me that the best
> way to deal with this is to make sure that the autosuspend timeout is
> shorter than the interal between queries, not to make the querying
> conditional on !runtime_auto.

Short autosuspend timeout can affect performance. For instance our experiments showed that
shorter than 10sec timeout made Ethernet performance degrade because of wakeup delays.
So, just putting shorter timeout may have some side effects.

Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Alan Stern]

On Mon, 21 Mar 2016 Woojung.Huh@microchip.com wrote:

> > But this leaves open the issue that querying the device too often will
> > prevent it from going into autosuspend.  It seems to me that the best
> > way to deal with this is to make sure that the autosuspend timeout is
> > shorter than the interal between queries, not to make the querying
> > conditional on !runtime_auto.
> 
> Short autosuspend timeout can affect performance. For instance our experiments showed that
> shorter than 10sec timeout made Ethernet performance degrade because of wakeup delays.
> So, just putting shorter timeout may have some side effects.

Sure.  This just means that you need a long statistics interval --
longer than the autosuspend timeout.  That's why I suggested making the
interval adjustable.

Alternatively, you could automatically set the statistics interval to
the autosuspend timeout (or 0 if autosuspend is disabled) plus some
fixed value.

Alan Stern

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> > > But this leaves open the issue that querying the device too often will
> > > prevent it from going into autosuspend.  It seems to me that the best
> > > way to deal with this is to make sure that the autosuspend timeout is
> > > shorter than the interal between queries, not to make the querying
> > > conditional on !runtime_auto.
> >
> > Short autosuspend timeout can affect performance. For instance our
> experiments showed that
> > shorter than 10sec timeout made Ethernet performance degrade because
> of wakeup delays.
> > So, just putting shorter timeout may have some side effects.
> 
> Sure.  This just means that you need a long statistics interval --
> longer than the autosuspend timeout.  That's why I suggested making the
> interval adjustable.

What do you mean statistics interval?
Interval calling ndo_get_stats64 or another thread/timer or else getting statistics?

Thanks.
Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Alan Stern]

On Mon, 21 Mar 2016 Woojung.Huh@microchip.com wrote:

> > > > But this leaves open the issue that querying the device too often will
> > > > prevent it from going into autosuspend.  It seems to me that the best
> > > > way to deal with this is to make sure that the autosuspend timeout is
> > > > shorter than the interal between queries, not to make the querying
> > > > conditional on !runtime_auto.
> > >
> > > Short autosuspend timeout can affect performance. For instance our
> > experiments showed that
> > > shorter than 10sec timeout made Ethernet performance degrade because
> > of wakeup delays.
> > > So, just putting shorter timeout may have some side effects.
> > 
> > Sure.  This just means that you need a long statistics interval --
> > longer than the autosuspend timeout.  That's why I suggested making the
> > interval adjustable.
> 
> What do you mean statistics interval?
> Interval calling ndo_get_stats64 or another thread/timer or else getting statistics?

The time between calls to ndo_get_stats64, since that's the routine 
which queries the device.

Alan Stern

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Oliver Neukum]

On Mon, 2016-03-21 at 14:24 -0400, Alan Stern wrote:
> On Mon, 21 Mar 2016, Oliver Neukum wrote:
> 
> > On Mon, 2016-03-21 at 10:57 -0400, Alan Stern wrote:
> > 
> > > One possible solution is to export a sysfs parameter to prevent 
> > > statistics collection (or more generally, to change the interval at 
> > > which it occurs).
> > 
> > Surely, not performing a task can hardly be beaten in terms of power
> > consumption. That is not meant to be flippant, but I think these
> > issues are orthogonal. The question of how much to do doesn't
> > solve the question of doing efficiently what we do.
> 
> In other words, what's the best way to collect the statistics without 
> interfering with runtime PM, right?

Yes.

> If the device is suspended, presumably we know there's nothing to
> collect -- especially if we already collected the statistics at the
> time the device got suspended.  Hence my suggestion to avoid querying 
> the device while it is suspended.

That is perfectly alright if we just collect statistics.
As a generic mechanism it is bad. Think about the polling
for media detection.

> But this leaves open the issue that querying the device too often will 
> prevent it from going into autosuspend.  It seems to me that the best 
> way to deal with this is to make sure that the autosuspend timeout is 
> shorter than the interal between queries, not to make the querying 
> conditional on !runtime_auto.
[..]
> > If we know when the next activity will come, why not pass this
> > information down?

We have an autosuspend timeout because we think that IO, if it will
come at all, is likeliest to come soon. If, however, the IO is
periodic that heuristics is false.
To save most power the driver must either decide that the interval
is too short or suspend immediately. So if we are lucky enough
to have the frequency in the kernel, we should use that information.

	Regards
		Oliver

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Alan Stern]

On Mon, 21 Mar 2016, Oliver Neukum wrote:

> On Mon, 2016-03-21 at 14:24 -0400, Alan Stern wrote:
> > On Mon, 21 Mar 2016, Oliver Neukum wrote:
> > 
> > > On Mon, 2016-03-21 at 10:57 -0400, Alan Stern wrote:
> > > 
> > > > One possible solution is to export a sysfs parameter to prevent 
> > > > statistics collection (or more generally, to change the interval at 
> > > > which it occurs).
> > > 
> > > Surely, not performing a task can hardly be beaten in terms of power
> > > consumption. That is not meant to be flippant, but I think these
> > > issues are orthogonal. The question of how much to do doesn't
> > > solve the question of doing efficiently what we do.
> > 
> > In other words, what's the best way to collect the statistics without 
> > interfering with runtime PM, right?
> 
> Yes.
> 
> > If the device is suspended, presumably we know there's nothing to
> > collect -- especially if we already collected the statistics at the
> > time the device got suspended.  Hence my suggestion to avoid querying 
> > the device while it is suspended.
> 
> That is perfectly alright if we just collect statistics.
> As a generic mechanism it is bad. Think about the polling
> for media detection.

True.  Here I'm talking specifically about collecting statistics.  
Media detection has its own requirements.

> > But this leaves open the issue that querying the device too often will 
> > prevent it from going into autosuspend.  It seems to me that the best 
> > way to deal with this is to make sure that the autosuspend timeout is 
> > shorter than the interal between queries, not to make the querying 
> > conditional on !runtime_auto.
> [..]
> > > If we know when the next activity will come, why not pass this
> > > information down?
> 
> We have an autosuspend timeout because we think that IO, if it will
> come at all, is likeliest to come soon. If, however, the IO is
> periodic that heuristics is false.
> To save most power the driver must either decide that the interval
> is too short or suspend immediately. So if we are lucky enough
> to have the frequency in the kernel, we should use that information.

The autosuspend timeout is set by userspace.  The kernel may assign a 
default value, but the user can always override it.  Given this, I 
don't see how the kernel can use frequency information (and I'm not 
sure where that information would come from in the first place).

Alan Stern

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Oliver Neukum]

On Mon, 2016-03-21 at 15:30 -0400, Alan Stern wrote:
> On Mon, 21 Mar 2016, Oliver Neukum wrote:
> 

> > We have an autosuspend timeout because we think that IO, if it will
> > come at all, is likeliest to come soon. If, however, the IO is
> > periodic that heuristics is false.
> > To save most power the driver must either decide that the interval
> > is too short or suspend immediately. So if we are lucky enough
> > to have the frequency in the kernel, we should use that information.
> 
> The autosuspend timeout is set by userspace.  The kernel may assign a

Thus it should apply to all IO originating in user space.
But only to that IO.

> default value, but the user can always override it.  Given this, I 
> don't see how the kernel can use frequency information (and I'm not 
> sure where that information would come from in the first place).

It can ignore internal IO for the purpose of the timeout.
If such IO is performed while the device is active, don't
alter the timer. Otherwise resume the device and look at
the provided hint and suspend again immediately if the period is long
enough.
If IO is generated periodically in the kernel, the kernel must know that
period.

	Regards
		Oliver
 

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Alan Stern]

On Tue, 22 Mar 2016, Oliver Neukum wrote:

> On Mon, 2016-03-21 at 15:30 -0400, Alan Stern wrote:
> > On Mon, 21 Mar 2016, Oliver Neukum wrote:
> > 
> 
> > > We have an autosuspend timeout because we think that IO, if it will
> > > come at all, is likeliest to come soon. If, however, the IO is
> > > periodic that heuristics is false.
> > > To save most power the driver must either decide that the interval
> > > is too short or suspend immediately. So if we are lucky enough
> > > to have the frequency in the kernel, we should use that information.
> > 
> > The autosuspend timeout is set by userspace.  The kernel may assign a
> 
> Thus it should apply to all IO originating in user space.
> But only to that IO.

Fair enough.

> > default value, but the user can always override it.  Given this, I 
> > don't see how the kernel can use frequency information (and I'm not 
> > sure where that information would come from in the first place).
> 
> It can ignore internal IO for the purpose of the timeout.
> If such IO is performed while the device is active, don't
> alter the timer.

Come to think of it, we don't.  If pm_runtime_get_sync() and then
pm_runtime_put() are called while the device is already at full power, 
the PM core doesn't update the last_busy time.  So if the driver 
doesn't update it either, the statistics collection won't interfere 
with autosuspend (except when it races with the autosuspend timer 
expiration).

> Otherwise resume the device and look at
> the provided hint and suspend again immediately if the period is long
> enough.

I don't see any point in resuming the device just in order to collect 
operating statistics.  If it was already suspended then it wasn't 
operating, so there will be no statistics to collect.

> If IO is generated periodically in the kernel, the kernel must know that
> period.

Alan Stern

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Oliver Neukum]

On Tue, 2016-03-22 at 10:21 -0400, Alan Stern wrote:
> I don't see any point in resuming the device just in order to collect 
> operating statistics.  If it was already suspended then it wasn't 
> operating, so there will be no statistics to collect.

Indeed. In that case the point is moot. But it is correct to ask
the core whether the device is autosuspended at that point rather
than keep a private flag if you can.

All that is relevant only if the upper layers ask for information
that the driver cannot provide without resuming the device.
Those are fundamentally different issues.

	Regards
		Oliver

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Alan Stern]

On Tue, 22 Mar 2016, Oliver Neukum wrote:

> On Tue, 2016-03-22 at 10:21 -0400, Alan Stern wrote:
> > I don't see any point in resuming the device just in order to collect 
> > operating statistics.  If it was already suspended then it wasn't 
> > operating, so there will be no statistics to collect.
> 
> Indeed. In that case the point is moot. But it is correct to ask
> the core whether the device is autosuspended at that point rather
> than keep a private flag if you can.

That's why we have pm_runtime_status_suspended().

> All that is relevant only if the upper layers ask for information
> that the driver cannot provide without resuming the device.
> Those are fundamentally different issues.

Of course.

Alan Stern

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Oliver Neukum]

On Tue, 2016-03-22 at 11:13 -0400, Alan Stern wrote:
> > Indeed. In that case the point is moot. But it is correct to ask
> > the core whether the device is autosuspended at that point rather
> > than keep a private flag if you can.
> 
> That's why we have pm_runtime_status_suspended().

I guess we are in violent agreement though we were unaware of being
in that state.

	Regards
		Oliver