Re: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Francois Romieu]

Hau  :
[...]
> > Either the driver resumes the device so that it can perform requested
> > operation or it signals .set_wol failure when the device is suspended.
> > 
> > If the driver does something else, "spam removal" translates to "silent
> > failure".
> 
> Because "tp->saved_wolopts" will be used to set hardware wol capability in
> rtl8169_runtime_resume().  So I prefer to keep "wol->wolopts" to
> " tp->saved_wolopts " in runtime suspend state and set this to this
> "wol->wolopts" to hardware in in rtl8169_runtime_resume(). 

It would be fine if it could be proven that rtl8169_runtime_resume() will
always be run before software state is lost.

-- 
Ueimor

RE: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Hau]

[...]
> Nit: you may directly use "struct device *d = >pci_dev->dev;"
> 

I will do that on my next version patch.

Thanks.
--Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Hau]

[...]
> > @@ -1852,12 +1863,17 @@ static int rtl8169_set_wol(struct net_device
> *dev, struct ethtool_wolinfo *wol)
> > tp->features |= RTL_FEATURE_WOL;
> > else
> > tp->features &= ~RTL_FEATURE_WOL;
> > -   __rtl8169_set_wol(tp, wol->wolopts);
> > +   if (pm_runtime_active(>dev))
> > +   __rtl8169_set_wol(tp, wol->wolopts);
> > +   else
> > +   tp->saved_wolopts = wol->wolopts;
> >
> > rtl_unlock_work(tp);
> >
> > device_set_wakeup_enable(>pci_dev->dev, wol->wolopts);
> >
> > +   pm_runtime_put_noidle(>dev);
> > +
> > return 0;
> 
> Either the driver resumes the device so that it can perform requested
> operation or it signals .set_wol failure when the device is suspended.
> 
> If the driver does something else, "spam removal" translates to "silent
> failure".

Because "tp->saved_wolopts" will be used to set hardware wol capability in 
rtl8169_runtime_resume().  So I prefer to keep "wol->wolopts" to " 
tp->saved_wolopts " in runtime suspend state and set this to this 
"wol->wolopts" to hardware in in rtl8169_runtime_resume(). 

Thanks.

--Please consider the environment before printing this e-mail.

Re: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Francois Romieu]

Chunhao Lin  :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c 
> b/drivers/net/ethernet/realtek/r8169.c
> index 0e62d74..f07604f 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -1749,13 +1749,21 @@ static u32 __rtl8169_get_wol(struct rtl8169_private 
> *tp)
>  static void rtl8169_get_wol(struct net_device *dev, struct ethtool_wolinfo 
> *wol)
>  {
>   struct rtl8169_private *tp = netdev_priv(dev);
> + struct pci_dev *pdev = tp->pci_dev;

Nit: you may directly use "struct device *d = >pci_dev->dev;"

-- 
Ueimor

Re: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Francois Romieu]

Chunhao Lin  :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c 
> b/drivers/net/ethernet/realtek/r8169.c
> index 0e62d74..f07604f 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
[...]
> @@ -1852,12 +1863,17 @@ static int rtl8169_set_wol(struct net_device *dev, 
> struct ethtool_wolinfo *wol)
>   tp->features |= RTL_FEATURE_WOL;
>   else
>   tp->features &= ~RTL_FEATURE_WOL;
> - __rtl8169_set_wol(tp, wol->wolopts);
> + if (pm_runtime_active(>dev))
> + __rtl8169_set_wol(tp, wol->wolopts);
> + else
> + tp->saved_wolopts = wol->wolopts;
>  
>   rtl_unlock_work(tp);
>  
>   device_set_wakeup_enable(>pci_dev->dev, wol->wolopts);
>  
> + pm_runtime_put_noidle(>dev);
> +
>   return 0;

Either the driver resumes the device so that it can perform requested
operation or it signals .set_wol failure when the device is suspended.

If the driver does something else, "spam removal" translates to
"silent failure".

-- 
Ueimor