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

[David Miller]

From: Eric Dumazet <eric.dumazet@xxxxxxxxx>
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@xxxxxxxxxxxx>
>> Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>> ---
>> 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.