RE: netdev carrier changes is one even after ethernet link up.

RE: netdev carrier changes is one even after ethernet link up.

[Bhadram Varka]

+ netdev

From: Bhadram Varka 
Sent: Thursday, August 31, 2017 11:24 AM
To: 'andrew@lunn.ch' <andrew@lunn.ch>; 'f.fainelli@gmail.com' <f.fainelli@gmail.com>
Cc: linux-netdev <netdev@vger.kernel.org>
Subject: netdev carrier changes is one even after ethernet link up.

Hi,

I have observed that carrier_changes is one even in case of the ethernet link is up.
 
After investigating the code below is my observation -

ethernet_driver_probe()
+--->phy_connect()
|     +--->phy_attach_direct()
|           +---> netif_carrier_off()    : which increments carrier_changes to one.
+--->register_netdevice() : will the carrier_changes becomes zero here ?
+--->netif_carrier_off(): not increment the carrier_changes since __LINK_STATE_NOCARRIER already set.
 
>From ethernet driver open will start the PHY and trigger the phy_state_machine. 
Phy_state_machine workqueue calling netif_carrier_on() once the link is UP.
netif_carrier_on() increments the carrier_changes by one.
 
After link is UP if we check the carrier_changes sysfs node - it will be one only.
 
$ cat /sys/class/net/eth0/carrier_changes
1
 
After reverting the change - https://lkml.org/lkml/2016/1/9/173 (net: phy: turn carrier off on phy attach) then I could see the carrier changes incremented to 2 after Link UP.
$ cat /sys/class/net/eth0/carrier_changes
2

Thanks,
Bhadram.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: netdev carrier changes is one even after ethernet link up.

[Florian Fainelli]

On 08/30/2017 10:53 PM, Bhadram Varka wrote:
> Hi,
> 
>  
> 
> I have observed that carrier_changes is one even in case of the ethernet
> link is up.
> 
>  
> 
> After investigating the code below is my observation –
> 
>  
> 
> ethernet_driver_probe()
> 
> +--->phy_connect()
> 
> |     +--->phy_attach_direct()
> 
> |           +---> netif_carrier_off()    : which increments
> carrier_changes to one.
> 
> +--->register_netdevice() : will the carrier_changes becomes zero here ?
> 
> +--->netif_carrier_off(): not increment the carrier_changes since
> __LINK_STATE_NOCARRIER already set.
> 
>  
> 
> From ethernet driver open will start the PHY and trigger the
> phy_state_machine.
> 
> Phy_state_machine workqueue calling netif_carrier_on() once the link is UP.
> 
> netif_carrier_on() increments the carrier_changes by one.

If the call trace is correct, then there is at least two problems here:

- phy_connect() does start the PHY machine which means that as soon as
it detects a link state of any kind (up or down) it can call
netif_carrier_off() respectively netif_carrier_on()

- as soon as you call register_netdevice() notifiers run and other parts
of the kernel or user-space programs can see an inconsistent link state

I would suggest doing the following sequence instead:

netif_carrier_off()
register_netdevice()
phy_connect()

Which should result in a consistent link state and carrier value.

> 
>  
> 
> After link is UP if we check the carrier_changes sysfs node - it will be
> one only.
> 
>  
> 
> $ cat /sys/class/net/eth0/carrier_changes
> 
> 1
> 
>  
> 
> After reverting the change - https://lkml.org/lkml/2016/1/9/173 (net:
> phy: turn carrier off on phy attach) then I could see the carrier
> changes incremented to 2 after Link UP.
> 
> $ cat /sys/class/net/eth0/carrier_changes
> 
> 2
> 
>  
> 
> Thanks,
> 
> Bhadram.
> 
> ------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and
> may contain confidential information.  Any unauthorized review, use,
> disclosure or distribution is prohibited.  If you are not the intended
> recipient, please contact the sender by reply email and destroy all
> copies of the original message.
> ------------------------------------------------------------------------


-- 
Florian

RE: netdev carrier changes is one even after ethernet link up.

[Bhadram Varka]

Thanks for responding.

-----Original Message-----
From: Florian Fainelli [mailto:f.fainelli@gmail.com] 
Sent: Friday, September 01, 2017 5:53 AM
To: Bhadram Varka <vbhadram@nvidia.com>; andrew@lunn.ch
Cc: linux-netdev <netdev@vger.kernel.org>
Subject: Re: netdev carrier changes is one even after ethernet link up.

On 08/30/2017 10:53 PM, Bhadram Varka wrote:
> Hi,
> 
>  
> 
> I have observed that carrier_changes is one even in case of the 
> ethernet link is up.
> 
>  
> 
> After investigating the code below is my observation –
> 
>  
> 
> ethernet_driver_probe()
> 
> +--->phy_connect()
> 
> |     +--->phy_attach_direct()
> 
> |           +---> netif_carrier_off()    : which increments
> carrier_changes to one.
> 
> +--->register_netdevice() : will the carrier_changes becomes zero here ?
> 
> +--->netif_carrier_off(): not increment the carrier_changes since
> __LINK_STATE_NOCARRIER already set.
> 
>  
> 
> From ethernet driver open will start the PHY and trigger the 
> phy_state_machine.
> 
> Phy_state_machine workqueue calling netif_carrier_on() once the link is UP.
> 
> netif_carrier_on() increments the carrier_changes by one.

If the call trace is correct, then there is at least two problems here:

- phy_connect() does start the PHY machine which means that as soon as it detects a link state of any kind (up or down) it can call
netif_carrier_off() respectively netif_carrier_on()

- as soon as you call register_netdevice() notifiers run and other parts of the kernel or user-space programs can see an inconsistent link state

I would suggest doing the following sequence instead:

netif_carrier_off()
register_netdevice()
phy_connect()

Which should result in a consistent link state and carrier value.

Yes, It will address the issue. 

If we did the phy_conect in ndo_open it will make the carrier changes as one. But if we did in probe function then it's not working.

In ethernet driver probe - (below sequence is not working)
phy_connect()
register_netdevice()
netif_carrier_off()

working sequence:
In probe():
register_netdevice()
ndo_open:
           phy_connect()

Thanks,
Bhadram.
> 
>  
> 
> After link is UP if we check the carrier_changes sysfs node - it will 
> be one only.
> 
>  
> 
> $ cat /sys/class/net/eth0/carrier_changes
> 
> 1
> 
>  
> 
> After reverting the change - https://lkml.org/lkml/2016/1/9/173 (net:
> phy: turn carrier off on phy attach) then I could see the carrier 
> changes incremented to 2 after Link UP.
> 
> $ cat /sys/class/net/eth0/carrier_changes
> 
> 2
> 
>  
> 
> Thanks,
> 
> Bhadram.
> 
> ----------------------------------------------------------------------
> -- This email message is for the sole use of the intended recipient(s) 
> and may contain confidential information.  Any unauthorized review, 
> use, disclosure or distribution is prohibited.  If you are not the 
> intended recipient, please contact the sender by reply email and 
> destroy all copies of the original message.
> ----------------------------------------------------------------------
> --


--
Florian

RE: netdev carrier changes is one even after ethernet link up.

[Bhadram Varka]

Thanks for responding. Now responding inline

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Friday, September 01, 2017 5:53 AM
> To: Bhadram Varka <vbhadram@nvidia.com>; andrew@lunn.ch
> Cc: linux-netdev <netdev@vger.kernel.org>
> Subject: Re: netdev carrier changes is one even after ethernet link up.
> 
> On 08/30/2017 10:53 PM, Bhadram Varka wrote:
> > Hi,
> >
> >
> >
> > I have observed that carrier_changes is one even in case of the
> > ethernet link is up.
> >
> >
> >
> > After investigating the code below is my observation –
> >
> >
> >
> > ethernet_driver_probe()
> >
> > +--->phy_connect()
> >
> > |     +--->phy_attach_direct()
> >
> > |           +---> netif_carrier_off()    : which increments
> > carrier_changes to one.
> >
> > +--->register_netdevice() : will the carrier_changes becomes zero here ?
> >
> > +--->netif_carrier_off(): not increment the carrier_changes since
> > __LINK_STATE_NOCARRIER already set.
> >
> >
> >
> > From ethernet driver open will start the PHY and trigger the
> > phy_state_machine.
> >
> > Phy_state_machine workqueue calling netif_carrier_on() once the link is
> UP.
> >
> > netif_carrier_on() increments the carrier_changes by one.
> 
> If the call trace is correct, then there is at least two problems here:
> 
> - phy_connect() does start the PHY machine which means that as soon as it
> detects a link state of any kind (up or down) it can call
> netif_carrier_off() respectively netif_carrier_on()
> 
> - as soon as you call register_netdevice() notifiers run and other parts of the
> kernel or user-space programs can see an inconsistent link state
> 
> I would suggest doing the following sequence instead:
> 
> netif_carrier_off()
> register_netdevice()
> phy_connect()
> 
> Which should result in a consistent link state and carrier value.
> 
Yes, It will address the issue. 

If we did the phy_conect in ndo_open it will make the carrier changes as two. But if we did in probe function then it's not working.

In ethernet driver probe - (below sequence is not working)
phy_connect()
register_netdevice()
netif_carrier_off()

working sequence:
In probe():
register_netdevice()
ndo_open:
           phy_connect()

After reverting - https://lkml.org/lkml/2016/1/9/173 this works if we do phy_connect in probe as well.

Thanks,
Bhadram.
> >
> >
> >
> > After link is UP if we check the carrier_changes sysfs node - it will
> > be one only.
> >
> >
> >
> > $ cat /sys/class/net/eth0/carrier_changes
> >
> > 1
> >
> >
> >
> > After reverting the change - https://lkml.org/lkml/2016/1/9/173 (net:
> > phy: turn carrier off on phy attach) then I could see the carrier
> > changes incremented to 2 after Link UP.
> >
> > $ cat /sys/class/net/eth0/carrier_changes
> >
> > 2
> >
> >
> >
> > Thanks,
> >
> > Bhadram.
> >
> > ----------------------------------------------------------------------
> > -- This email message is for the sole use of the intended recipient(s)
> > and may contain confidential information.  Any unauthorized review,
> > use, disclosure or distribution is prohibited.  If you are not the
> > intended recipient, please contact the sender by reply email and
> > destroy all copies of the original message.
> > ----------------------------------------------------------------------
> > --
> 
> 
> --
> Florian

Re: netdev carrier changes is one even after ethernet link up.

[Florian Fainelli]

On 08/31/2017 10:49 PM, Bhadram Varka wrote:
> Thanks for responding. Now responding inline
> 
>> -----Original Message-----
>> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
>> Sent: Friday, September 01, 2017 5:53 AM
>> To: Bhadram Varka <vbhadram@nvidia.com>; andrew@lunn.ch
>> Cc: linux-netdev <netdev@vger.kernel.org>
>> Subject: Re: netdev carrier changes is one even after ethernet link up.
>>
>> On 08/30/2017 10:53 PM, Bhadram Varka wrote:
>>> Hi,
>>>
>>>
>>>
>>> I have observed that carrier_changes is one even in case of the
>>> ethernet link is up.
>>>
>>>
>>>
>>> After investigating the code below is my observation –
>>>
>>>
>>>
>>> ethernet_driver_probe()
>>>
>>> +--->phy_connect()
>>>
>>> |     +--->phy_attach_direct()
>>>
>>> |           +---> netif_carrier_off()    : which increments
>>> carrier_changes to one.
>>>
>>> +--->register_netdevice() : will the carrier_changes becomes zero here ?
>>>
>>> +--->netif_carrier_off(): not increment the carrier_changes since
>>> __LINK_STATE_NOCARRIER already set.
>>>
>>>
>>>
>>> From ethernet driver open will start the PHY and trigger the
>>> phy_state_machine.
>>>
>>> Phy_state_machine workqueue calling netif_carrier_on() once the link is
>> UP.
>>>
>>> netif_carrier_on() increments the carrier_changes by one.
>>
>> If the call trace is correct, then there is at least two problems here:
>>
>> - phy_connect() does start the PHY machine which means that as soon as it
>> detects a link state of any kind (up or down) it can call
>> netif_carrier_off() respectively netif_carrier_on()
>>
>> - as soon as you call register_netdevice() notifiers run and other parts of the
>> kernel or user-space programs can see an inconsistent link state
>>
>> I would suggest doing the following sequence instead:
>>
>> netif_carrier_off()
>> register_netdevice()
>> phy_connect()
>>
>> Which should result in a consistent link state and carrier value.
>>
> Yes, It will address the issue. 
> 
> If we did the phy_conect in ndo_open it will make the carrier changes as two. But if we did in probe function then it's not working.
> 
> In ethernet driver probe - (below sequence is not working)
> phy_connect()
> register_netdevice()
> netif_carrier_off()
> 
> working sequence:
> In probe():
> register_netdevice()
> ndo_open:
>            phy_connect()
> 
> After reverting - https://lkml.org/lkml/2016/1/9/173 this works if we do phy_connect in probe as well.

But as mentioned before you should not be doing the PHY probe in your
driver's probe function for different reasons:

- the probe function's responsibility is to initialize the driver and
the HW to a state where they both have everything needed but it should
be in quiesced state. There is no guarantee that your network device may
ever be used after probe unless something calls ndo_open(), you should
therefore keep all resources to a minimum: memory allocated, HW powered
down etc.

- there is a race condition between the PHY state machine started in
phy_connect(), and when register_netdevice() is called and notifiers
running which can lead to an inconsistent state for the carrier

So considering that your driver does not do that, I am not sure what you
are expecting...
-- 
Florian

RE: netdev carrier changes is one even after ethernet link up.

[Bhadram Varka]

Hi Florian,

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Saturday, September 02, 2017 7:25 AM
> To: Bhadram Varka <vbhadram@nvidia.com>; andrew@lunn.ch
> Cc: linux-netdev <netdev@vger.kernel.org>
> Subject: Re: netdev carrier changes is one even after ethernet link up.
> 
> On 08/31/2017 10:49 PM, Bhadram Varka wrote:
> > Thanks for responding. Now responding inline
> >
> >> -----Original Message-----
> >> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> >> Sent: Friday, September 01, 2017 5:53 AM
> >> To: Bhadram Varka <vbhadram@nvidia.com>; andrew@lunn.ch
> >> Cc: linux-netdev <netdev@vger.kernel.org>
> >> Subject: Re: netdev carrier changes is one even after ethernet link up.
> >>
> >> On 08/30/2017 10:53 PM, Bhadram Varka wrote:
> >>> Hi,
> >>>
> >>>
> >>>
> >>> I have observed that carrier_changes is one even in case of the
> >>> ethernet link is up.
> >>>
> >>>
> >>>
> >>> After investigating the code below is my observation -
> >>>
> >>>
> >>>
> >>> ethernet_driver_probe()
> >>>
> >>> +--->phy_connect()
> >>>
> >>> |     +--->phy_attach_direct()
> >>>
> >>> |           +---> netif_carrier_off()    : which increments
> >>> carrier_changes to one.
> >>>
> >>> +--->register_netdevice() : will the carrier_changes becomes zero here ?
> >>>
> >>> +--->netif_carrier_off(): not increment the carrier_changes since
> >>> __LINK_STATE_NOCARRIER already set.
> >>>
> >>>
> >>>
> >>> From ethernet driver open will start the PHY and trigger the
> >>> phy_state_machine.
> >>>
> >>> Phy_state_machine workqueue calling netif_carrier_on() once the link
> >>> is
> >> UP.
> >>>
> >>> netif_carrier_on() increments the carrier_changes by one.
> >>
> >> If the call trace is correct, then there is at least two problems here:
> >>
> >> - phy_connect() does start the PHY machine which means that as soon
> >> as it detects a link state of any kind (up or down) it can call
> >> netif_carrier_off() respectively netif_carrier_on()
> >>
> >> - as soon as you call register_netdevice() notifiers run and other
> >> parts of the kernel or user-space programs can see an inconsistent
> >> link state
> >>
> >> I would suggest doing the following sequence instead:
> >>
> >> netif_carrier_off()
> >> register_netdevice()
> >> phy_connect()
> >>
> >> Which should result in a consistent link state and carrier value.
> >>
> > Yes, It will address the issue.
> >
> > If we did the phy_conect in ndo_open it will make the carrier changes as
> two. But if we did in probe function then it's not working.
> >
> > In ethernet driver probe - (below sequence is not working)
> > phy_connect()
> > register_netdevice()
> > netif_carrier_off()
> >
> > working sequence:
> > In probe():
> > register_netdevice()
> > ndo_open:
> >            phy_connect()
> >
> > After reverting - https://lkml.org/lkml/2016/1/9/173 this works if we do
> phy_connect in probe as well.
> 
> But as mentioned before you should not be doing the PHY probe in your
> driver's probe function for different reasons:
> 
> - the probe function's responsibility is to initialize the driver and the HW to a
> state where they both have everything needed but it should be in quiesced
> state. There is no guarantee that your network device may ever be used
> after probe unless something calls ndo_open(), you should therefore keep
> all resources to a minimum: memory allocated, HW powered down etc.
> 
> - there is a race condition between the PHY state machine started in
> phy_connect(), and when register_netdevice() is called and notifiers running
> which can lead to an inconsistent state for the carrier
> 
Agreed. But I could see lot of drivers performing the phy_connect() in driver_probe() function itself. 
Also observed that these drivers are performing netif_carrier_off() after register_netdevice().

My point is that we do we need to do netif_carrier_off() in phy_connect since we already doing the same after register_netdevice() ?

Thanks!
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

RE: netdev carrier changes is one even after ethernet link up.

[Bhadram Varka]

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Saturday, September 02, 2017 7:25 AM
> To: Bhadram Varka <vbhadram@nvidia.com>; andrew@lunn.ch
> Cc: linux-netdev <netdev@vger.kernel.org>
> Subject: Re: netdev carrier changes is one even after ethernet link up.
> 
> On 08/31/2017 10:49 PM, Bhadram Varka wrote:
> > Thanks for responding. Now responding inline
> >
> >> -----Original Message-----
> >> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> >> Sent: Friday, September 01, 2017 5:53 AM
> >> To: Bhadram Varka <vbhadram@nvidia.com>; andrew@lunn.ch
> >> Cc: linux-netdev <netdev@vger.kernel.org>
> >> Subject: Re: netdev carrier changes is one even after ethernet link up.
> >>
> >> On 08/30/2017 10:53 PM, Bhadram Varka wrote:
> >>> Hi,
> >>>
> >>>
> >>>
> >>> I have observed that carrier_changes is one even in case of the
> >>> ethernet link is up.
> >>>
> >>>
> >>>
> >>> After investigating the code below is my observation -
> >>>
> >>>
> >>>
> >>> ethernet_driver_probe()
> >>>
> >>> +--->phy_connect()
> >>>
> >>> |     +--->phy_attach_direct()
> >>>
> >>> |           +---> netif_carrier_off()    : which increments
> >>> carrier_changes to one.
> >>>
> >>> +--->register_netdevice() : will the carrier_changes becomes zero here ?
> >>>
> >>> +--->netif_carrier_off(): not increment the carrier_changes since
> >>> __LINK_STATE_NOCARRIER already set.
> >>>
> >>>
> >>>
> >>> From ethernet driver open will start the PHY and trigger the
> >>> phy_state_machine.
> >>>
> >>> Phy_state_machine workqueue calling netif_carrier_on() once the link
> >>> is
> >> UP.
> >>>
> >>> netif_carrier_on() increments the carrier_changes by one.
> >>
> >> If the call trace is correct, then there is at least two problems here:
> >>
> >> - phy_connect() does start the PHY machine which means that as soon
> >> as it detects a link state of any kind (up or down) it can call
> >> netif_carrier_off() respectively netif_carrier_on()
> >>
> >> - as soon as you call register_netdevice() notifiers run and other
> >> parts of the kernel or user-space programs can see an inconsistent
> >> link state
> >>
> >> I would suggest doing the following sequence instead:
> >>
> >> netif_carrier_off()
> >> register_netdevice()
> >> phy_connect()
> >>
> >> Which should result in a consistent link state and carrier value.
> >>
> > Yes, It will address the issue.
> >
> > If we did the phy_conect in ndo_open it will make the carrier changes as
> two. But if we did in probe function then it's not working.
> >
> > In ethernet driver probe - (below sequence is not working)
> > phy_connect()
> > register_netdevice()
> > netif_carrier_off()
> >
> > working sequence:
> > In probe():
> > register_netdevice()
> > ndo_open:
> >            phy_connect()
> >
> > After reverting - https://lkml.org/lkml/2016/1/9/173 this works if we do
> phy_connect in probe as well.
> 
> But as mentioned before you should not be doing the PHY probe in your
> driver's probe function for different reasons:
> 
> - the probe function's responsibility is to initialize the driver and the HW to a
> state where they both have everything needed but it should be in quiesced
> state. There is no guarantee that your network device may ever be used
> after probe unless something calls ndo_open(), you should therefore keep
> all resources to a minimum: memory allocated, HW powered down etc.
> 
> - there is a race condition between the PHY state machine started in
> phy_connect(), and when register_netdevice() is called and notifiers running
> which can lead to an inconsistent state for the carrier

Agreed. But I could see lot of drivers performing the phy_connect() in driver_probe() function itself.
Also observed that these drivers are performing netif_carrier_off() after register_netdevice().

My point is that why do we need to do netif_carrier_off() in phy_connect() since we already doing the same after register_netdevice() in MAC driver ?

Thanks!
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------