Skip to contentSkip to conversation

net: Set NOCARRIER bit of etherdev state at initialization

PATCH In review Balakumaran Kannan 3 participants 5 messages May 23, 2014 – May 27, 2014

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Balakumaran Kannan

author
#0 lore raw

PATCH 1/1 net: Set NOCARRIER bit of etherdev state at initialization

For any etherdev 'netif_carrier_ok' should succeed only after phy layer determines carrier state through auto-negotiation or setting it forcibly.

So ethernet drivers should set '__LINK_STATE_NOCARRIER' bit of 'dev->state' using 'netif_carrier_off' call at driver initialization. As it is common for all ethernet devices, 'netif_carrier_off' call could be moved to 'ether_setup'.

Signed-off-by Balakumaran Kannan <[email protected]>

---
'netif_carrier_off' could be called anywhere in between 'alloc_etherdev' and 'phy_start'. For example,
1. e1000 driver calls at the end of 'e1000_probe'
2. intel i40e driver calls at the beginning of 'i40e_open'
3. realtek r8169 driver calls at the end of 'rtl_init_one'

But some driver misses this call. For example smsc911x driver doesn't call 'netif_carrier_off' before calling 'phy_start'. So device starts performing dhcp and IPv6 DAD even if carrier is not ready.

So it is better to move 'netif_carrier_off' call to 'ether_setup'. And drivers can remove this call from their initialization functions.

---
 net/ethernet/eth.c | 3 +++
 1 file changed, 3 insertions(+)
net/ethernet/eth.c +3 -0
index 5dc638c..45f6703 100644
@@ -369,6 +369,9 @@ void ether_setup(struct net_device *dev)
 
 	memset(dev->broadcast, 0xFF, ETH_ALEN);
 
+	/* Carrier state should be off initially */
+	netif_carrier_off(dev);
+
 }
 EXPORT_SYMBOL(ether_setup);
Signature · 1 line
1.8.3.2
#1 lore raw
Quoted 8 lines · Balakumaran Kannan

For any etherdev 'netif_carrier_ok' should succeed only after phy layer determines carrier state through auto-negotiation or setting it forcibly.

So ethernet drivers should set '__LINK_STATE_NOCARRIER' bit of 'dev->state' using 'netif_carrier_off' call at driver initialization. As it is common for all ethernet devices, 'netif_carrier_off' call could be moved to 'ether_setup'.

Unless I misunderstand, wouldn't this turn off carrier for all devices until the driver explicitly sets the carrier up?

The current "carrier on until told otherwise" model is intentional, because not all drivers support carrier detection, and thus we must assume the carrier is on until the driver tells us it is not on.

Would this patch break that model?

Dan

Quoted 32 lines
Signed-off-by Balakumaran Kannan <[email protected]>

---
'netif_carrier_off' could be called anywhere in between 'alloc_etherdev' and 'phy_start'. For example,
1. e1000 driver calls at the end of 'e1000_probe'
2. intel i40e driver calls at the beginning of 'i40e_open'
3. realtek r8169 driver calls at the end of 'rtl_init_one'

But some driver misses this call. For example smsc911x driver doesn't call 'netif_carrier_off' before calling 'phy_start'. So device starts performing dhcp and IPv6 DAD even if carrier is not ready.

So it is better to move 'netif_carrier_off' call to 'ether_setup'. And drivers can remove this call from their initialization functions.

---
 net/ethernet/eth.c | 3 +++
 1 file changed, 3 insertions(+)
net/ethernet/eth.c +3 -0
index 5dc638c..45f6703 100644
@@ -369,6 +369,9 @@ void ether_setup(struct net_device *dev)
 
 	memset(dev->broadcast, 0xFF, ETH_ALEN);
 
+	/* Carrier state should be off initially */
+	netif_carrier_off(dev);
+
 }
 EXPORT_SYMBOL(ether_setup);
maintainer
#2 lore raw

From: Dan Williams <[email protected]>
Date: Fri, 23 May 2014 12:38:01 -0500

Quoted 5 lines

The current "carrier on until told otherwise" model is intentional, because not all drivers support carrier detection, and thus we must assume the carrier is on until the driver tells us it is not on.

Would this patch break that model?

Absolutely correct, this change is completely inappropriate.

author
#3 lore raw
Quoted 10 lines · David Miller

From: Dan Williams <[email protected]>
Date: Fri, 23 May 2014 12:38:01 -0500

Quoted 5 lines

The current "carrier on until told otherwise" model is intentional, because not all drivers support carrier detection, and thus we must assume the carrier is on until the driver tells us it is not on.

Would this patch break that model?

Absolutely correct, this change is completely inappropriate.

So it is driver's responsibility to maintain this flag appropriately. Thus if a driver supports carrier detection, it should set "carrier on" only after determining the carrier state. Then I'll add 'netif_carrier_off' call to smsc911x driver and send a new patch.

Thanks for the explanation.

Regards,
K.Balakumaran

#4 lore raw
Quoted 8 lines · Balakumaran Kannan
Quoted 7 lines · David Miller

From: Dan Williams <[email protected]>
Date: Fri, 23 May 2014 12:38:01 -0500

Quoted 4 lines

The current "carrier on until told otherwise" model is intentional, because not all drivers support carrier detection, and thus we must assume the carrier is on until the driver tells us it is not on.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Would this patch break that model?

Absolutely correct, this change is completely inappropriate.

So it is driver's responsibility to maintain this flag appropriately. Thus if a driver supports carrier detection, it should set "carrier on" only after determining the carrier state. Then I'll add 'netif_carrier_off' call to smsc911x driver and send a new patch.

Correct. Carrier state starts with ON. If the driver knows it can detect carrier state, then the driver should call netif_carrier_off() early in the driver initialization, and then call on/off as appropriate when it gets carrier change events.

Dan

Quoted 8 lines

Thanks for the explanation.

Regards,
K.Balakumaran
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html