Re: [PATCH] tg3: ethtool phys_id default (rev2)

Re: [PATCH] tg3: ethtool phys_id default (rev2)

[Michael Chan]

On Fri, 2008-02-22 at 10:24 -0800, Stephen Hemminger wrote:
> When asked to blink LEDs the tg3 driver behaves when using:
> 	ethtool -p ethX
> The default value for data is zero, and other drivers interpret this
> as blink forever (or at least a really long time).  The tg3 driver
> interprets this as blink once.  All drivers should have the same
> behaviour.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Acked-by: Michael Chan <mchan@broadcom.com>

We should do this across the board for bnx2, bnx2x, and niu as well.

Re: [PATCH] tg3: ethtool phys_id default (rev2)

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 22 Feb 2008 11:16:42 -0800

> On Fri, 2008-02-22 at 10:24 -0800, Stephen Hemminger wrote:
> > When asked to blink LEDs the tg3 driver behaves when using:
> > 	ethtool -p ethX
> > The default value for data is zero, and other drivers interpret this
> > as blink forever (or at least a really long time).  The tg3 driver
> > interprets this as blink once.  All drivers should have the same
> > behaviour.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> Acked-by: Michael Chan <mchan@broadcom.com>

Applied.

> We should do this across the board for bnx2, bnx2x, and niu as well.

Agreed.

Re: [PATCH] tg3: ethtool phys_id default (rev2)

[Eliezer Tamir]

On Sat, 2008-02-23 at 19:52 -0800, David Miller wrote:
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Fri, 22 Feb 2008 11:16:42 -0800
> 
> > On Fri, 2008-02-22 at 10:24 -0800, Stephen Hemminger wrote:
> > > When asked to blink LEDs the tg3 driver behaves when using:
> > > 	ethtool -p ethX
> > > The default value for data is zero, and other drivers interpret this
> > > as blink forever (or at least a really long time).  The tg3 driver
> > > interprets this as blink once.  All drivers should have the same
> > > behaviour.
...
> > We should do this across the board for bnx2, bnx2x, and niu as well.
> 
> Agreed.

Doesn't this mean that ethtool -p will hold the RTNL lock forever?
Is this a good idea?

For example on the Red Hat machine I have here if you do:

ethtool -p eth2 100000 &
reboot

Various things the shutdown scripts try to do will fail because of the
held RTNL lock. in the end the script dies and the machine does not
reboot.

Eliezer

Re: [PATCH] tg3: ethtool phys_id default (rev2)

[Stephen Hemminger]

On Sun, 24 Feb 2008 07:42:06 +0200
"Eliezer Tamir" <eliezert@broadcom.com> wrote:

> 
> On Sat, 2008-02-23 at 19:52 -0800, David Miller wrote:
> > From: "Michael Chan" <mchan@broadcom.com>
> > Date: Fri, 22 Feb 2008 11:16:42 -0800
> > 
> > > On Fri, 2008-02-22 at 10:24 -0800, Stephen Hemminger wrote:
> > > > When asked to blink LEDs the tg3 driver behaves when using:
> > > > 	ethtool -p ethX
> > > > The default value for data is zero, and other drivers interpret this
> > > > as blink forever (or at least a really long time).  The tg3 driver
> > > > interprets this as blink once.  All drivers should have the same
> > > > behaviour.
> ...
> > > We should do this across the board for bnx2, bnx2x, and niu as well.
> > 
> > Agreed.
> 
> Doesn't this mean that ethtool -p will hold the RTNL lock forever?
> Is this a good idea?
> 
> For example on the Red Hat machine I have here if you do:
> 
> ethtool -p eth2 100000 &
> reboot
> 
> Various things the shutdown scripts try to do will fail because of the
> held RTNL lock. in the end the script dies and the machine does not
> reboot.

1. ethtool -p is only used by root to identify interfaces, so in practice
   this is not a real problem.

2. ethtool -p is interruptible, and the reboot process sends a SIGTERM to
   all processes.

Re: [PATCH] tg3: ethtool phys_id default (rev2)

[Eliezer Tamir]

> > Doesn't this mean that ethtool -p will hold the RTNL lock forever?
> > Is this a good idea?
> > 
> > For example on the Red Hat machine I have here if you do:
> > 
> > ethtool -p eth2 100000 &
> > reboot
> > 
> > Various things the shutdown scripts try to do will fail because of the
> > held RTNL lock. in the end the script dies and the machine does not
> > reboot.
> 
> 1. ethtool -p is only used by root to identify interfaces, so in practice
>    this is not a real problem.
> 
> 2. ethtool -p is interruptible, and the reboot process sends a SIGTERM to
>    all processes.


The simple use-case of running ethtool -p eth0 10000 on an ssh console
and them going to the rack and pressing the power button fails.

It seems like the shutdown script dies a long way before it usually
kills all the processes. 

Bringing down NFS, SNMP and several other services fails.
Then things die for being blocked more then 120 seconds and the system
is stuck.