[PATCH] net/fec: carrier off initially to avoid root mount failure

[PATCH] net/fec: carrier off initially to avoid root mount failure

[Oskar Schirmer]

with hardware slow in negotiation, the system did freeze
while trying to mount root on nfs at boot time.

the link state has not been initialised so network stack
tried to start transmission right away. this caused instant
retries, as the driver solely stated business upon link down,
rendering the system unusable.

notify carrier off initially to prevent transmission until
phylib will report link up.

Signed-off-by: Oskar Schirmer <oskar@linutronix.de>
---
 drivers/net/fec.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 768b840..e83f67d 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -1311,6 +1311,9 @@ fec_probe(struct platform_device *pdev)
 	if (ret)
 		goto failed_mii_init;
 
+	/* Carrier starts down, phylib will bring it up */
+	netif_carrier_off(ndev);
+
 	ret = register_netdev(ndev);
 	if (ret)
 		goto failed_register;
-- 
1.7.0.4

Re: [PATCH] net/fec: carrier off initially to avoid root mount failure

[David Miller]

From: Oskar Schirmer <oskar@linutronix.de>
Date: Thu,  7 Oct 2010 14:30:30 +0200

> with hardware slow in negotiation, the system did freeze
> while trying to mount root on nfs at boot time.
> 
> the link state has not been initialised so network stack
> tried to start transmission right away. this caused instant
> retries, as the driver solely stated business upon link down,
> rendering the system unusable.
> 
> notify carrier off initially to prevent transmission until
> phylib will report link up.
> 
> Signed-off-by: Oskar Schirmer <oskar@linutronix.de>

Maybe fs_enet_open() is a better place for this netif_carrier_off()
call?

Every open the driver probes the PHY and does phy_start().

Re: [PATCH] net/fec: carrier off initially to avoid root mount failure

[Oskar Schirmer]

On Fri, Oct 08, 2010 at 10:31:12 -0700, David Miller wrote:
> From: Oskar Schirmer <oskar@linutronix.de>
> Date: Thu,  7 Oct 2010 14:30:30 +0200
> 
> > with hardware slow in negotiation, the system did freeze
> > while trying to mount root on nfs at boot time.
> > 
> > the link state has not been initialised so network stack
> > tried to start transmission right away. this caused instant
> > retries, as the driver solely stated business upon link down,
> > rendering the system unusable.
> > 
> > notify carrier off initially to prevent transmission until
> > phylib will report link up.
> > 
> > Signed-off-by: Oskar Schirmer <oskar@linutronix.de>
> 
> Maybe fs_enet_open() is a better place for this netif_carrier_off()
> call?

The patch is for fec, so I guess You propose fec_enet_open().

> Every open the driver probes the PHY and does phy_start().

netif_carrier handling is done in phylib properly
except with initialisation. So when asking for the most
correct place to fix this, I'ld propose phylib itself:
Shouldn't the correct carrier state be set with
phy_start_machine or phy_device_create when phy state is
set to PHY_DOWN?

Seen from this point of view, the patch I did propose is
a workaround, as much as doing it in the ndo_open, I guess.

Re: [PATCH] net/fec: carrier off initially to avoid root mount failure

[David Miller]

From: Oskar Schirmer <oskar@linutronix.de>
Date: Fri, 8 Oct 2010 22:35:30 +0200

> So when asking for the most correct place to fix this, I'ld propose
> phylib itself: Shouldn't the correct carrier state be set with
> phy_start_machine or phy_device_create when phy state is set to
> PHY_DOWN?

I agree, phylib is the best place to handle this.

The carrier defaults to "on" for newly created net devices
since that's what software devices and hardware ones which
lack a link want.

phy_device_create() currently lacks access to the network device
struct.

phy_connect_direct() and phy_attach_direct() might be good spots.

Re: [PATCH] net/fec: carrier off initially to avoid root mount failure

[David Miller]

From: Oskar Schirmer <oskar@linutronix.de>
Date: Thu,  7 Oct 2010 14:30:30 +0200

> with hardware slow in negotiation, the system did freeze
> while trying to mount root on nfs at boot time.
> 
> the link state has not been initialised so network stack
> tried to start transmission right away. this caused instant
> retries, as the driver solely stated business upon link down,
> rendering the system unusable.
> 
> notify carrier off initially to prevent transmission until
> phylib will report link up.
> 
> Signed-off-by: Oskar Schirmer <oskar@linutronix.de>

I did some more investigation into this situation, and for now I'm
going to apply your patch.  It is correct, and it also matches what
the only other seemingly correct driver I could find using phylib does
(gianfar) :-) Actually, although I didn't check, bi-modal drivers
(those that only use phylib for some phy types) like tg3 probably do
the right thing here too.

Longer term I think the right thing to do might be:

1) Create some notion of "network device has managed carrier"

   This could simply be a flag bit in the netdev or netdev_ops,
   or some other kind of attribute.

2) Managed carrier devices start with netif_carrier_off(), otherwise
   the device starts with netif_carrier_on().

Then we gut all of the probe time netif_carrir_*() calls in all
of the drivers.

And hopefully it's less error prone than it is right now.

Re: [PATCH] net/fec: carrier off initially to avoid root mount failure

[Oskar Schirmer]

On Sun, Oct 10, 2010 at 21:19:56 -0700, David Miller wrote:
> From: Oskar Schirmer <oskar@linutronix.de>
> Date: Thu,  7 Oct 2010 14:30:30 +0200
> 
> > with hardware slow in negotiation, the system did freeze
> > while trying to mount root on nfs at boot time.
> > 
> > the link state has not been initialised so network stack
> > tried to start transmission right away. this caused instant
> > retries, as the driver solely stated business upon link down,
> > rendering the system unusable.
> > 
> > notify carrier off initially to prevent transmission until
> > phylib will report link up.
> > 
> > Signed-off-by: Oskar Schirmer <oskar@linutronix.de>
> 
> I did some more investigation into this situation, and for now I'm
> going to apply your patch.  It is correct, and it also matches what
> the only other seemingly correct driver I could find using phylib does
> (gianfar) :-) Actually, although I didn't check, bi-modal drivers
> (those that only use phylib for some phy types) like tg3 probably do
> the right thing here too.
> 
> Longer term I think the right thing to do might be:
> 
> 1) Create some notion of "network device has managed carrier"
> 
>    This could simply be a flag bit in the netdev or netdev_ops,
>    or some other kind of attribute.
> 
> 2) Managed carrier devices start with netif_carrier_off(), otherwise
>    the device starts with netif_carrier_on().

This last conditional (managed vs otherwise) would be implicit
with a null PHY driver as Ben Hutchings proposes it to Greg Ungerers
"allow FEC driver to not have attached PHY", 2010/10/07,
with the null PHY simply switching to netif_carrier_on right after
machine start.

Otherwise my patch would need another #ifdef to live in
peace with Gregs patch.

Re: [PATCH] net/fec: carrier off initially to avoid root mount failure

[Greg Ungerer]

On 11/10/10 17:54, Oskar Schirmer wrote:
> On Sun, Oct 10, 2010 at 21:19:56 -0700, David Miller wrote:
>> From: Oskar Schirmer<oskar@linutronix.de>
>> Date: Thu,  7 Oct 2010 14:30:30 +0200
>>
>>> with hardware slow in negotiation, the system did freeze
>>> while trying to mount root on nfs at boot time.
>>>
>>> the link state has not been initialised so network stack
>>> tried to start transmission right away. this caused instant
>>> retries, as the driver solely stated business upon link down,
>>> rendering the system unusable.
>>>
>>> notify carrier off initially to prevent transmission until
>>> phylib will report link up.
>>>
>>> Signed-off-by: Oskar Schirmer<oskar@linutronix.de>
>>
>> I did some more investigation into this situation, and for now I'm
>> going to apply your patch.  It is correct, and it also matches what
>> the only other seemingly correct driver I could find using phylib does
>> (gianfar) :-) Actually, although I didn't check, bi-modal drivers
>> (those that only use phylib for some phy types) like tg3 probably do
>> the right thing here too.
>>
>> Longer term I think the right thing to do might be:
>>
>> 1) Create some notion of "network device has managed carrier"
>>
>>     This could simply be a flag bit in the netdev or netdev_ops,
>>     or some other kind of attribute.
>>
>> 2) Managed carrier devices start with netif_carrier_off(), otherwise
>>     the device starts with netif_carrier_on().
>
> This last conditional (managed vs otherwise) would be implicit
> with a null PHY driver as Ben Hutchings proposes it to Greg Ungerers
> "allow FEC driver to not have attached PHY", 2010/10/07,
> with the null PHY simply switching to netif_carrier_on right after
> machine start.
>
> Otherwise my patch would need another #ifdef to live in
> peace with Gregs patch.

You can ignore my patch for now. I am reworking the it to
use fixed phy. It will look quite different when it is done.

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close,                            FAX:         +61 7 3891 3630
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com