netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* r8169: remove "PHY reset until link up" log spam
@ 2013-07-23  9:55 Peter Wu
  2013-07-23 12:22 ` Sergei Shtylyov
  2013-07-23 15:54 ` Stephen Hemminger
  0 siblings, 2 replies; 13+ messages in thread
From: Peter Wu @ 2013-07-23  9:55 UTC (permalink / raw)
  To: Francois Romieu; +Cc: David Miller, netdev, nic_swsd

This message was added in commit a7154cb8 (June 2004) and is printed
every ten seconds when no cable is connected.

Signed-off-by: Peter Wu <lekensteyn@gmail.com>
---
Using ethtool to silence *all* link warnings is still a manual
operation, in my opinion not acceptable so let's remove this message.

The r8169 constantly resets the device when no link is connected,
contrary the r8168 vendor driver which only resets the link when some
PCI config fields have been modified. As the current reset logic in r8168
seems to work for broken device (which I do not have have and therefore
cannot test), I did not attempt to "clean this up".
---
 drivers/net/ethernet/realtek/r8169.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 880015c..63f04af 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -3689,8 +3689,6 @@ static void rtl_phy_work(struct rtl8169_private *tp)
 	if (tp->link_ok(ioaddr))
 		return;
 
-	netif_warn(tp, link, tp->dev, "PHY reset until link up\n");
-
 	tp->phy_reset_enable(tp);
 
 out_mod_timer:
-- 
1.8.3.3

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: r8169: remove "PHY reset until link up" log spam
  2013-07-23  9:55 r8169: remove "PHY reset until link up" log spam Peter Wu
@ 2013-07-23 12:22 ` Sergei Shtylyov
  2013-07-23 12:38   ` [PATCH v2] " Peter Wu
  2013-07-23 15:54 ` Stephen Hemminger
  1 sibling, 1 reply; 13+ messages in thread
From: Sergei Shtylyov @ 2013-07-23 12:22 UTC (permalink / raw)
  To: Peter Wu; +Cc: Francois Romieu, David Miller, netdev, nic_swsd

Hello.

On 23-07-2013 13:55, Peter Wu wrote:

> This message was added in commit a7154cb8 (June 2004) and is printed

    Please also specify that commit's summary line in parens.

> every ten seconds when no cable is connected.

> Signed-off-by: Peter Wu <lekensteyn@gmail.com>

WBR, Sergei

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] r8169: remove "PHY reset until link up" log spam
  2013-07-23 12:22 ` Sergei Shtylyov
@ 2013-07-23 12:38   ` Peter Wu
  2013-07-23 20:23     ` Francois Romieu
  0 siblings, 1 reply; 13+ messages in thread
From: Peter Wu @ 2013-07-23 12:38 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: Francois Romieu, David Miller, netdev, nic_swsd

This message was added in commit a7154cb8 (June 2004, [PATCH] r8169:
link handling and phy reset rework) and is printed every ten seconds
when no cable is connected. (Before that commit, "Reset RTL8169s PHY"
would be printed instead.)

Signed-off-by: Peter Wu <lekensteyn@gmail.com>
---

On Tuesday 23 July 2013 16:22:57 Sergei Shtylyov wrote:
> On 23-07-2013 13:55, Peter Wu wrote:
> > This message was added in commit a7154cb8 (June 2004) and is printed
> 
>     Please also specify that commit's summary line in parens.
Done, while at it I have also noted that an other spam message would be
logged.
---
 drivers/net/ethernet/realtek/r8169.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 880015c..63f04af 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -3689,8 +3689,6 @@ static void rtl_phy_work(struct rtl8169_private *tp)
 	if (tp->link_ok(ioaddr))
 		return;
 
-	netif_warn(tp, link, tp->dev, "PHY reset until link up\n");
-
 	tp->phy_reset_enable(tp);
 
 out_mod_timer:
-- 
1.8.3.3

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: r8169: remove "PHY reset until link up" log spam
  2013-07-23  9:55 r8169: remove "PHY reset until link up" log spam Peter Wu
  2013-07-23 12:22 ` Sergei Shtylyov
@ 2013-07-23 15:54 ` Stephen Hemminger
  2013-07-23 16:07   ` Peter Wu
  1 sibling, 1 reply; 13+ messages in thread
From: Stephen Hemminger @ 2013-07-23 15:54 UTC (permalink / raw)
  To: Peter Wu; +Cc: Francois Romieu, David Miller, netdev, nic_swsd

On Tue, 23 Jul 2013 11:55:57 +0200
Peter Wu <lekensteyn@gmail.com> wrote:

> This message was added in commit a7154cb8 (June 2004) and is printed
> every ten seconds when no cable is connected.
> 
> Signed-off-by: Peter Wu <lekensteyn@gmail.com>
> ---
> Using ethtool to silence *all* link warnings is still a manual
> operation, in my opinion not acceptable so let's remove this message.
> 
> The r8169 constantly resets the device when no link is connected,
> contrary the r8168 vendor driver which only resets the link when some
> PCI config fields have been modified. As the current reset logic in r8168
> seems to work for broken device (which I do not have have and therefore
> cannot test), I did not attempt to "clean this up".
> ---
>  drivers/net/ethernet/realtek/r8169.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 880015c..63f04af 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -3689,8 +3689,6 @@ static void rtl_phy_work(struct rtl8169_private *tp)
>  	if (tp->link_ok(ioaddr))
>  		return;
>  
> -	netif_warn(tp, link, tp->dev, "PHY reset until link up\n");
> -
>  	tp->phy_reset_enable(tp);
>  
>  out_mod_timer:

Why not implement netif msg flag to allow user to control this?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: r8169: remove "PHY reset until link up" log spam
  2013-07-23 15:54 ` Stephen Hemminger
@ 2013-07-23 16:07   ` Peter Wu
  2013-07-23 16:17     ` Stephen Hemminger
  2013-07-23 20:23     ` Francois Romieu
  0 siblings, 2 replies; 13+ messages in thread
From: Peter Wu @ 2013-07-23 16:07 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Francois Romieu, David Miller, netdev, nic_swsd

On Tuesday 23 July 2013 08:54:36 Stephen Hemminger wrote:
> On Tue, 23 Jul 2013 11:55:57 +0200
> 
> Peter Wu <lekensteyn@gmail.com> wrote:
> > This message was added in commit a7154cb8 (June 2004) and is printed
> > every ten seconds when no cable is connected.
> > 
> > Signed-off-by: Peter Wu <lekensteyn@gmail.com>
> > ---
> > Using ethtool to silence *all* link warnings is still a manual
> > operation, in my opinion not acceptable so let's remove this message.
> > 
> > The r8169 constantly resets the device when no link is connected,
> > contrary the r8168 vendor driver which only resets the link when some
> > PCI config fields have been modified. As the current reset logic in r8168
> > seems to work for broken device (which I do not have have and therefore
> > cannot test), I did not attempt to "clean this up".
> > ---
> > 
> >  drivers/net/ethernet/realtek/r8169.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/realtek/r8169.c
> > b/drivers/net/ethernet/realtek/r8169.c index 880015c..63f04af 100644
> > --- a/drivers/net/ethernet/realtek/r8169.c
> > +++ b/drivers/net/ethernet/realtek/r8169.c
> > @@ -3689,8 +3689,6 @@ static void rtl_phy_work(struct rtl8169_private *tp)
> > 
> >  	if (tp->link_ok(ioaddr))
> >  	
> >  		return;
> > 
> > -	netif_warn(tp, link, tp->dev, "PHY reset until link up\n");
> > -
> > 
> >  	tp->phy_reset_enable(tp);
> >  
> >  out_mod_timer:
> Why not implement netif msg flag to allow user to control this?

Which user wants to get log spam every ten seconds? I see no purpose except 
when a developer asks a user to turn this on, or except if you are a 
developer. This message was added almost ten years ago, Realtek must certainly 
have improved their hardware not to be so crappy? 

See also <http://marc.info/?l=linux-netdev&m=137452974325684&w=2> where David 
Miller wrote:
> In every case where this issue has popped up, we have decided that
> printing messages when the cable is simply unplugged is very
> undesirable.

Regards,
Peter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: r8169: remove "PHY reset until link up" log spam
  2013-07-23 16:07   ` Peter Wu
@ 2013-07-23 16:17     ` Stephen Hemminger
  2013-07-23 21:18       ` Stephen Hemminger
  2013-07-23 20:23     ` Francois Romieu
  1 sibling, 1 reply; 13+ messages in thread
From: Stephen Hemminger @ 2013-07-23 16:17 UTC (permalink / raw)
  To: Peter Wu; +Cc: Francois Romieu, David Miller, netdev, nic_swsd

On Tue, 23 Jul 2013 18:07:15 +0200
Peter Wu <lekensteyn@gmail.com> wrote:

> On Tuesday 23 July 2013 08:54:36 Stephen Hemminger wrote:
> > On Tue, 23 Jul 2013 11:55:57 +0200
> > 
> > Peter Wu <lekensteyn@gmail.com> wrote:
> > > This message was added in commit a7154cb8 (June 2004) and is printed
> > > every ten seconds when no cable is connected.
> > > 
> > > Signed-off-by: Peter Wu <lekensteyn@gmail.com>
> > > ---
> > > Using ethtool to silence *all* link warnings is still a manual
> > > operation, in my opinion not acceptable so let's remove this message.
> > > 
> > > The r8169 constantly resets the device when no link is connected,
> > > contrary the r8168 vendor driver which only resets the link when some
> > > PCI config fields have been modified. As the current reset logic in r8168
> > > seems to work for broken device (which I do not have have and therefore
> > > cannot test), I did not attempt to "clean this up".
> > > ---
> > > 
> > >  drivers/net/ethernet/realtek/r8169.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/realtek/r8169.c
> > > b/drivers/net/ethernet/realtek/r8169.c index 880015c..63f04af 100644
> > > --- a/drivers/net/ethernet/realtek/r8169.c
> > > +++ b/drivers/net/ethernet/realtek/r8169.c
> > > @@ -3689,8 +3689,6 @@ static void rtl_phy_work(struct rtl8169_private *tp)
> > > 
> > >  	if (tp->link_ok(ioaddr))
> > >  	
> > >  		return;
> > > 
> > > -	netif_warn(tp, link, tp->dev, "PHY reset until link up\n");
> > > -
> > > 
> > >  	tp->phy_reset_enable(tp);
> > >  
> > >  out_mod_timer:
> > Why not implement netif msg flag to allow user to control this?
> 
> Which user wants to get log spam every ten seconds? I see no purpose except 
> when a developer asks a user to turn this on, or except if you are a 
> developer. This message was added almost ten years ago, Realtek must certainly 
> have improved their hardware not to be so crappy? 
> 
> See also <http://marc.info/?l=linux-netdev&m=137452974325684&w=2> where David 
> Miller wrote:
> > In every case where this issue has popped up, we have decided that
> > printing messages when the cable is simply unplugged is very
> > undesirable.
> 
> Regards,
> Peter

I meant the general link up/down message, not the silly polling message

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] r8169: remove "PHY reset until link up" log spam
  2013-07-23 12:38   ` [PATCH v2] " Peter Wu
@ 2013-07-23 20:23     ` Francois Romieu
  2013-07-23 20:48       ` Peter Wu
  0 siblings, 1 reply; 13+ messages in thread
From: Francois Romieu @ 2013-07-23 20:23 UTC (permalink / raw)
  To: Peter Wu; +Cc: Sergei Shtylyov, David Miller, netdev, nic_swsd

Peter Wu <lekensteyn@gmail.com> :
> This message was added in commit a7154cb8 (June 2004, [PATCH] r8169:
> link handling and phy reset rework) and is printed every ten seconds
> when no cable is connected.
+                            and runtime power management is disabled.

It isn't completely academic: Fedora has it enabled.

You have the implicit ack of David, so please keep the facts right.

-- 
Ueimor

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: r8169: remove "PHY reset until link up" log spam
  2013-07-23 16:07   ` Peter Wu
  2013-07-23 16:17     ` Stephen Hemminger
@ 2013-07-23 20:23     ` Francois Romieu
  2013-07-23 20:59       ` Peter Wu
  1 sibling, 1 reply; 13+ messages in thread
From: Francois Romieu @ 2013-07-23 20:23 UTC (permalink / raw)
  To: Peter Wu; +Cc: Stephen Hemminger, David Miller, netdev, nic_swsd

Peter Wu <lekensteyn@gmail.com> :
[...]
> Which user wants to get log spam every ten seconds ?

In my world users - be they big workstations or runtime pm enabled
laptops - may not even care about log [1].

> I see no purpose except when a developer asks a user to turn this on,
> or except if you are a developer.

Exactly. You'll easily guess which side I'm on with my load of problem
reports that have no simple solution and slowly ages while bugfixes
flow through -stable.

> This message was added almost ten years ago, Realtek must
> certainly have improved their hardware not to be so crappy ?

With due respect to Realtek, git log or Realtek's own Release.txt file
do not exactly paint the world as a faery of unicorns.

(Realtek is not alone: manufacturers can be quite creative)

[1] Linus is not a real user.

-- 
Ueimor

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] r8169: remove "PHY reset until link up" log spam
  2013-07-23 20:23     ` Francois Romieu
@ 2013-07-23 20:48       ` Peter Wu
  2013-07-23 21:50         ` Francois Romieu
  0 siblings, 1 reply; 13+ messages in thread
From: Peter Wu @ 2013-07-23 20:48 UTC (permalink / raw)
  To: Francois Romieu; +Cc: Sergei Shtylyov, David Miller, netdev, nic_swsd

On Tuesday 23 July 2013 22:23:12 Francois Romieu wrote:
> Peter Wu <lekensteyn@gmail.com> :
> > This message was added in commit a7154cb8 (June 2004, [PATCH] r8169:
> > link handling and phy reset rework) and is printed every ten seconds
> > when no cable is connected.
> 
> +                            and runtime power management is disabled.
> It isn't completely academic: Fedora has it enabled.
I have not really considered runtime PM before, aren't the ethtool operations 
actually broken with runtime PM enabled? Or does the ethtool core take care of 
waking devices (which I doubt)? I don't see any attempt to wake devices in the 
r8169 ethtool operations.

> You have the implicit ack of David, so please keep the facts right.
I am not trying to hide details, before I send the third revision of third 
commit message, what other details may I have overseen? If you want to, you 
can also create a patch with an appropriate commit message, I don't mind.

Regards,
Peter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: r8169: remove "PHY reset until link up" log spam
  2013-07-23 20:23     ` Francois Romieu
@ 2013-07-23 20:59       ` Peter Wu
  2013-07-23 21:51         ` Francois Romieu
  0 siblings, 1 reply; 13+ messages in thread
From: Peter Wu @ 2013-07-23 20:59 UTC (permalink / raw)
  To: Francois Romieu; +Cc: Stephen Hemminger, David Miller, netdev, nic_swsd

On Tuesday 23 July 2013 22:23:40 Francois Romieu wrote:
> Peter Wu <lekensteyn@gmail.com> :
> [...]
> 
> > Which user wants to get log spam every ten seconds ?
> 
> In my world users - be they big workstations or runtime pm enabled
> laptops - may not even care about log [1].
So I am not a real user either because I keep journalctl open / tailf log 
files? I see what you are trying to note, but I doubt the usefulness of this 
specific message. This message appears if:
 (1) The device has just been brought up.
 (2) Has not been connected before while being up.
 (3) Is enabled all the time (runtime PM?)
 (4) No reset it pending.

Only the last point cannot easily be determine from a log context, but these 
do not seem to appear on recent devices? (I've never experienced link issues 
with Realtek Ethernet cards, wireless is another thing)

> > This message was added almost ten years ago, Realtek must
> > certainly have improved their hardware not to be so crappy ?
> 
> With due respect to Realtek, git log or Realtek's own Release.txt file
> do not exactly paint the world as a faery of unicorns.
Or even using the file header as revision log... anyway, you cannot make money 
by selling just crap right? There is enough competition on this market.

Regards,
Peter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: r8169: remove "PHY reset until link up" log spam
  2013-07-23 16:17     ` Stephen Hemminger
@ 2013-07-23 21:18       ` Stephen Hemminger
  0 siblings, 0 replies; 13+ messages in thread
From: Stephen Hemminger @ 2013-07-23 21:18 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Peter Wu, Francois Romieu, David Miller, netdev, nic_swsd

This is what I was talking about, some patch like this.


--- a/drivers/net/ethernet/realtek/r8169.c	2013-07-19 09:12:37.121530393 -0700
+++ b/drivers/net/ethernet/realtek/r8169.c	2013-07-23 14:16:20.239516611 -0700
@@ -1496,11 +1496,12 @@ static void __rtl8169_check_link_status(
 		if (pm)
 			pm_request_resume(&tp->pci_dev->dev);
 		netif_carrier_on(dev);
-		if (net_ratelimit())
+		if (netif_msg_ifup(tp))
 			netif_info(tp, ifup, dev, "link up\n");
 	} else {
 		netif_carrier_off(dev);
-		netif_info(tp, ifdown, dev, "link down\n");
+		if (netif_msg_ifdown(tp))
+			netif_info(tp, ifdown, dev, "link down\n");
 		if (pm)
 			pm_schedule_suspend(&tp->pci_dev->dev, 5000);
 	}

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] r8169: remove "PHY reset until link up" log spam
  2013-07-23 20:48       ` Peter Wu
@ 2013-07-23 21:50         ` Francois Romieu
  0 siblings, 0 replies; 13+ messages in thread
From: Francois Romieu @ 2013-07-23 21:50 UTC (permalink / raw)
  To: Peter Wu; +Cc: Sergei Shtylyov, David Miller, netdev, nic_swsd

Peter Wu <lekensteyn@gmail.com> :
[...]
> I have not really considered runtime PM before, aren't the ethtool
> operations actually broken with runtime PM enabled ? Or does the ethtool
> core take care of waking devices (which I doubt) ? I don't see any attempt
> to wake devices in the r8169 ethtool operations.

ethtool core bails out.

See dev_ethtool, netif_device_present, rtl8169_net_suspend and
netif_device_detach.

[...]
> > You have the implicit ack of David, so please keep the facts right.
> I am not trying to hide details, before I send the third revision of third 
> commit message, what other details may I have overseen ?

Use netif_dbg instead of removing the message ?

> If you want to, you can also create a patch with an appropriate commit
> message, I don't mind.

[...]
- when no cable is connected.
+ when no cable is connected and runtime power management is disabled.

That's it. Your patch, your credit.

-- 
Ueimor

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: r8169: remove "PHY reset until link up" log spam
  2013-07-23 20:59       ` Peter Wu
@ 2013-07-23 21:51         ` Francois Romieu
  0 siblings, 0 replies; 13+ messages in thread
From: Francois Romieu @ 2013-07-23 21:51 UTC (permalink / raw)
  To: Peter Wu; +Cc: Stephen Hemminger, David Miller, netdev, nic_swsd

Peter Wu <lekensteyn@gmail.com> :
[...]
> Or even using the file header as revision log... anyway, you cannot make
> money by selling just crap right ?

Let's stop discussing this topic before it gets depressing, ok ?

And, no, you are not a real user anymore. Get over it.

-- 
Ueimor

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2013-07-23 21:51 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-23  9:55 r8169: remove "PHY reset until link up" log spam Peter Wu
2013-07-23 12:22 ` Sergei Shtylyov
2013-07-23 12:38   ` [PATCH v2] " Peter Wu
2013-07-23 20:23     ` Francois Romieu
2013-07-23 20:48       ` Peter Wu
2013-07-23 21:50         ` Francois Romieu
2013-07-23 15:54 ` Stephen Hemminger
2013-07-23 16:07   ` Peter Wu
2013-07-23 16:17     ` Stephen Hemminger
2013-07-23 21:18       ` Stephen Hemminger
2013-07-23 20:23     ` Francois Romieu
2013-07-23 20:59       ` Peter Wu
2013-07-23 21:51         ` Francois Romieu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).