[PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[Balakumaran Kannan]

As smsc driver supports carrier detection, it should unset NOCARRIER
flag only after carrier state determination. By default that flag
is off so driver should set it before starting auto-negotiation

Signed-off-by: Balakumaran Kannan <Balakumaran.Kannan@ap.sony.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index a0fc151..69016a5 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2525,6 +2525,8 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 
 	netdev_info(dev, "MAC Address: %pM\n", dev->dev_addr);
 
+	netif_carrier_off(dev);
+
 	return 0;
 
 out_unregister_netdev_5:
-- 
1.8.3.2

Re: [PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[David Miller]

From: Balakumaran Kannan <kumaran.4353@gmail.com>
Date: Sun, 25 May 2014 00:24:46 +0530

> @@ -2525,6 +2525,8 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
>  
>  	netdev_info(dev, "MAC Address: %pM\n", dev->dev_addr);
>  
> +	netif_carrier_off(dev);
> +
>  	return 0;
>  

You should do this before the network device is registered.

Re: [PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[Balakumaran Kannan]

Hi David,

On Fri, May 30, 2014 at 3:58 AM, David Miller <davem@davemloft.net> wrote:
> From: Balakumaran Kannan <kumaran.4353@gmail.com>
> Date: Sun, 25 May 2014 00:24:46 +0530
>
>> @@ -2525,6 +2525,8 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
>>
>>       netdev_info(dev, "MAC Address: %pM\n", dev->dev_addr);
>>
>> +     netif_carrier_off(dev);
>> +
>>       return 0;
>>
>
> You should do this before the network device is registered.
IMHO calling netif_carrier_off after registering device will not
affect functionality. Because,

    * Device starts carrier detection once driver's open function
calls phy_start.
    * Kernel starts network layer initialization only after __dev_open
function sets IFF_UP bit of dev->flags.
    And __dev_open sets this bit only after driver's open function returns.
    * So we could place netif_carrier_off call anywhere before
phy_start in driver's open function gets called.

Also I have referred some other drivers.

    * Intel i40e driver calls netif_carrier_off at the beginning of
driver's open (i40e_open) function.
    * e1000 driver places netif_carrier_off at the end of driver probe
(e1000_probe) function.
    * realtek r8169 driver calls netif_carrier_off at the end of
driver init (rtl_init_one) function.

All these drivers call netif_carrier_off function after registering
the network device using register_netdev.

This explanation is based on my understanding. Please explain to me if
I'm wrong or I miss something. In such case I'll modify the patch
accordingly.

Thank you.

Regards,
K.Balakumaran

Re: [PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[David Miller]

From: Balakumaran Kannan <kumaran.4353@gmail.com>
Date: Fri, 30 May 2014 18:35:43 +0530

> All these drivers call netif_carrier_off function after registering
> the network device using register_netdev.

I can quote more counter-examples than your list, starting with the
Broadcom tg3 driver.

Re: [PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[Balakumaran Kannan]

On Sat, May 31, 2014 at 2:12 AM, David Miller <davem@davemloft.net> wrote:
> From: Balakumaran Kannan <kumaran.4353@gmail.com>
> Date: Fri, 30 May 2014 18:35:43 +0530
>
>> All these drivers call netif_carrier_off function after registering
>> the network device using register_netdev.
>
> I can quote more counter-examples than your list, starting with the
> Broadcom tg3 driver.

Thank you David for this information. I'll send updated patch as version-2.

Still I couldn't understand the reason behind this. I should be grateful if you
would explain me little bit.

Regards,
K.Balakumaran

Re: [PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[Florian Fainelli]

2014-06-03 7:15 GMT-07:00 Balakumaran Kannan <kumaran.4353@gmail.com>:
> On Sat, May 31, 2014 at 2:12 AM, David Miller <davem@davemloft.net> wrote:
>> From: Balakumaran Kannan <kumaran.4353@gmail.com>
>> Date: Fri, 30 May 2014 18:35:43 +0530
>>
>>> All these drivers call netif_carrier_off function after registering
>>> the network device using register_netdev.
>>
>> I can quote more counter-examples than your list, starting with the
>> Broadcom tg3 driver.
>
> Thank you David for this information. I'll send updated patch as version-2.
>
> Still I couldn't understand the reason behind this. I should be grateful if you
> would explain me little bit.

I believe one reason is that as soon as you call register_netdev(),
the network device notifiers run as part of call_netdevice_notifiers()
and it is valid for a notified callback to open your network device.
This means that you can call the driver's ndo_open() before you
returned from register_netdev(). If netif_carrier_off() is called
after register_netdev(), there is a risk for ndo_open() not to get a
consistent carrier state, and the actual call to netif_carrier_off()
to mess things up.
-- 
Florian

Re: [PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[David Miller]

From: Balakumaran Kannan <kumaran.4353@gmail.com>
Date: Tue, 3 Jun 2014 19:45:39 +0530

> On Sat, May 31, 2014 at 2:12 AM, David Miller <davem@davemloft.net> wrote:
>> From: Balakumaran Kannan <kumaran.4353@gmail.com>
>> Date: Fri, 30 May 2014 18:35:43 +0530
>>
>>> All these drivers call netif_carrier_off function after registering
>>> the network device using register_netdev.
>>
>> I can quote more counter-examples than your list, starting with the
>> Broadcom tg3 driver.
> 
> Thank you David for this information. I'll send updated patch as version-2.
> 
> Still I couldn't understand the reason behind this. I should be grateful if you
> would explain me little bit.

At the moment you call register_netdev() the device is visible, notifications
are sent to userspace, and userland tools can try to bring the interface up
and see the incorrect link state, before you do the netif_carrier_off().

Said another way, between the register_netdev() and netif_carrier_off() call,
userspace can see the device in an inconsistent state.

Re: [PATCH 1/1] driver: net: smsc911x: set NOCARRIER flag in driver initialization

[Balakumaran Kannan]

Thank you very much Florian and David for great explanation.

Regards,
K.Balakumaran

On Wed, Jun 4, 2014 at 3:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Balakumaran Kannan <kumaran.4353@gmail.com>
> Date: Tue, 3 Jun 2014 19:45:39 +0530
>
>> On Sat, May 31, 2014 at 2:12 AM, David Miller <davem@davemloft.net> wrote:
>>> From: Balakumaran Kannan <kumaran.4353@gmail.com>
>>> Date: Fri, 30 May 2014 18:35:43 +0530
>>>
>>>> All these drivers call netif_carrier_off function after registering
>>>> the network device using register_netdev.
>>>
>>> I can quote more counter-examples than your list, starting with the
>>> Broadcom tg3 driver.
>>
>> Thank you David for this information. I'll send updated patch as version-2.
>>
>> Still I couldn't understand the reason behind this. I should be grateful if you
>> would explain me little bit.
>
> At the moment you call register_netdev() the device is visible, notifications
> are sent to userspace, and userland tools can try to bring the interface up
> and see the incorrect link state, before you do the netif_carrier_off().
>
> Said another way, between the register_netdev() and netif_carrier_off() call,
> userspace can see the device in an inconsistent state.