[PATCH net] phy: avoid unnecessary link-up delay in polling mode

[PATCH net] phy: avoid unnecessary link-up delay in polling mode

[Petr Oros]

commit 93c0970493c71f ("net: phy: consider latched link-down status in
polling mode") removed double-read of latched link-state register for
polling mode from genphy_update_link(). This added extra ~1s delay into
sequence link down->up.
Following scenario:
 - After boot link goes up
 - phy_start() is called triggering an aneg restart, hence link goes
   down and link-down info is latched.
 - After aneg has finished link goes up. In phy_state_machine is checked
   link state but it is latched "link is down". The state machine is
   scheduled after one second and there is detected "link is up". This
   extra delay can be avoided when we keep link-state register double read
   in case when link was down previously.

With this solution we don't miss a link-down event in polling mode and
link-up is faster.

Fixes: 93c0970493c71f ("net: phy: consider latched link-down status in polling mode")
Signed-off-by: Petr Oros <poros@redhat.com>
---
 drivers/net/phy/phy-c45.c    | 5 +++--
 drivers/net/phy/phy_device.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index a1caeee1223617..bceb0dcdecbd61 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -239,9 +239,10 @@ int genphy_c45_read_link(struct phy_device *phydev)
 
 		/* The link state is latched low so that momentary link
 		 * drops can be detected. Do not double-read the status
-		 * in polling mode to detect such short link drops.
+		 * in polling mode to detect such short link drops except
+		 * the link was already down.
 		 */
-		if (!phy_polling_mode(phydev)) {
+		if (!phy_polling_mode(phydev) || !phydev->link) {
 			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
 			if (val < 0)
 				return val;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6a5056e0ae7757..d5f4804c34d597 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
 
 	/* The link state is latched low so that momentary link
 	 * drops can be detected. Do not double-read the status
-	 * in polling mode to detect such short link drops.
+	 * in polling mode to detect such short link drops except
+	 * the link was already down.
 	 */
-	if (!phy_polling_mode(phydev)) {
+	if (!phy_polling_mode(pihydev) || !phydev->link) {
 		status = phy_read(phydev, MII_BMSR);
 		if (status < 0)
 			return status;
-- 
2.24.1

Re: [PATCH net] phy: avoid unnecessary link-up delay in polling mode

[Ivan Vecera]

On Wed, 29 Jan 2020 11:13:08 +0100
Petr Oros <poros@redhat.com> wrote:

> commit 93c0970493c71f ("net: phy: consider latched link-down status in
> polling mode") removed double-read of latched link-state register for
> polling mode from genphy_update_link(). This added extra ~1s delay into
> sequence link down->up.
> Following scenario:
>  - After boot link goes up
>  - phy_start() is called triggering an aneg restart, hence link goes
>    down and link-down info is latched.
>  - After aneg has finished link goes up. In phy_state_machine is checked
>    link state but it is latched "link is down". The state machine is
>    scheduled after one second and there is detected "link is up". This
>    extra delay can be avoided when we keep link-state register double read
>    in case when link was down previously.
> 
> With this solution we don't miss a link-down event in polling mode and
> link-up is faster.
> 
> Fixes: 93c0970493c71f ("net: phy: consider latched link-down status in polling mode")
> Signed-off-by: Petr Oros <poros@redhat.com>
> ---
>  drivers/net/phy/phy-c45.c    | 5 +++--
>  drivers/net/phy/phy_device.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index a1caeee1223617..bceb0dcdecbd61 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -239,9 +239,10 @@ int genphy_c45_read_link(struct phy_device *phydev)
>  
>  		/* The link state is latched low so that momentary link
>  		 * drops can be detected. Do not double-read the status
> -		 * in polling mode to detect such short link drops.
> +		 * in polling mode to detect such short link drops except
> +		 * the link was already down.
>  		 */
> -		if (!phy_polling_mode(phydev)) {
> +		if (!phy_polling_mode(phydev) || !phydev->link) {
>  			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
>  			if (val < 0)
>  				return val;
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 6a5056e0ae7757..d5f4804c34d597 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
>  
>  	/* The link state is latched low so that momentary link
>  	 * drops can be detected. Do not double-read the status
> -	 * in polling mode to detect such short link drops.
> +	 * in polling mode to detect such short link drops except
> +	 * the link was already down.
>  	 */
> -	if (!phy_polling_mode(phydev)) {
> +	if (!phy_polling_mode(pihydev) || !phydev->link) {
>  		status = phy_read(phydev, MII_BMSR);
>  		if (status < 0)
>  			return status;

Reviewed-by: Ivan Vecera <ivecera@redhat.com>

Re: [PATCH net] phy: avoid unnecessary link-up delay in polling mode

[Ivan Vecera]

On Wed, 29 Jan 2020 11:13:08 +0100
Petr Oros <poros@redhat.com> wrote:

> commit 93c0970493c71f ("net: phy: consider latched link-down status in
> polling mode") removed double-read of latched link-state register for
> polling mode from genphy_update_link(). This added extra ~1s delay into
> sequence link down->up.
> Following scenario:
>  - After boot link goes up
>  - phy_start() is called triggering an aneg restart, hence link goes
>    down and link-down info is latched.
>  - After aneg has finished link goes up. In phy_state_machine is checked
>    link state but it is latched "link is down". The state machine is
>    scheduled after one second and there is detected "link is up". This
>    extra delay can be avoided when we keep link-state register double read
>    in case when link was down previously.
> 
> With this solution we don't miss a link-down event in polling mode and
> link-up is faster.
> 
> Fixes: 93c0970493c71f ("net: phy: consider latched link-down status in polling mode")
> Signed-off-by: Petr Oros <poros@redhat.com>
> ---
>  drivers/net/phy/phy-c45.c    | 5 +++--
>  drivers/net/phy/phy_device.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index a1caeee1223617..bceb0dcdecbd61 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -239,9 +239,10 @@ int genphy_c45_read_link(struct phy_device *phydev)
>  
>  		/* The link state is latched low so that momentary link
>  		 * drops can be detected. Do not double-read the status
> -		 * in polling mode to detect such short link drops.
> +		 * in polling mode to detect such short link drops except
> +		 * the link was already down.
>  		 */
> -		if (!phy_polling_mode(phydev)) {
> +		if (!phy_polling_mode(phydev) || !phydev->link) {
>  			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
>  			if (val < 0)
>  				return val;
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 6a5056e0ae7757..d5f4804c34d597 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
>  
>  	/* The link state is latched low so that momentary link
>  	 * drops can be detected. Do not double-read the status
> -	 * in polling mode to detect such short link drops.
> +	 * in polling mode to detect such short link drops except
> +	 * the link was already down.
>  	 */
> -	if (!phy_polling_mode(phydev)) {
> +	if (!phy_polling_mode(pihydev) || !phydev->link) {
                               ^
Please remove the extra 'i' ----

Thx,
Ivan

Re: [PATCH net] phy: avoid unnecessary link-up delay in polling mode

[David Miller]

From: Ivan Vecera <ivecera@redhat.com>
Date: Wed, 29 Jan 2020 13:06:22 +0100

> On Wed, 29 Jan 2020 11:13:08 +0100
> Petr Oros <poros@redhat.com> wrote:
> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 6a5056e0ae7757..d5f4804c34d597 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
>>  
>>  	/* The link state is latched low so that momentary link
>>  	 * drops can be detected. Do not double-read the status
>> -	 * in polling mode to detect such short link drops.
>> +	 * in polling mode to detect such short link drops except
>> +	 * the link was already down.
>>  	 */
>> -	if (!phy_polling_mode(phydev)) {
>> +	if (!phy_polling_mode(pihydev) || !phydev->link) {
>                                ^
> Please remove the extra 'i' ----

How could this have ever been even build tested, let alone functionally
tested? :-/

Re: [PATCH net] phy: avoid unnecessary link-up delay in polling mode

[Petr Oros]

David Miller píše v St 29. 01. 2020 v 13:15 +0100:
> From: Ivan Vecera <ivecera@redhat.com>
> Date: Wed, 29 Jan 2020 13:06:22 +0100
> 
> > On Wed, 29 Jan 2020 11:13:08 +0100
> > Petr Oros <poros@redhat.com> wrote:
> > 
> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> >> index 6a5056e0ae7757..d5f4804c34d597 100644
> >> --- a/drivers/net/phy/phy_device.c
> >> +++ b/drivers/net/phy/phy_device.c
> >> @@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
> >>  
> >>      /* The link state is latched low so that momentary link
> >>       * drops can be detected. Do not double-read the status
> >> -     * in polling mode to detect such short link drops.
> >> +     * in polling mode to detect such short link drops except
> >> +     * the link was already down.
> >>       */
> >> -    if (!phy_polling_mode(phydev)) {
> >> +    if (!phy_polling_mode(pihydev) || !phydev->link) {
> >                                ^
> > Please remove the extra 'i' ----
> 
> How could this have ever been even build tested, let alone functionally
> tested? :-/

Sorry, it was tested on xgene-enet with RTL8211E phy. With patch there was ~4s
delay and without it ~5s. But before send i added "except the link was already
down" comment and by mistake added extra "i" in vim. Again, sorry.

-Petr

> 

[PATCH net v2] phy: avoid unnecessary link-up delay in polling mode

[Petr Oros]

commit 93c0970493c71f ("net: phy: consider latched link-down status in
polling mode") removed double-read of latched link-state register for
polling mode from genphy_update_link(). This added extra ~1s delay into
sequence link down->up.
Following scenario:
 - After boot link goes up
 - phy_start() is called triggering an aneg restart, hence link goes
   down and link-down info is latched.
 - After aneg has finished link goes up. In phy_state_machine is checked
   link state but it is latched "link is down". The state machine is
   scheduled after one second and there is detected "link is up". This
   extra delay can be avoided when we keep link-state register double read
   in case when link was down previously.

With this solution we don't miss a link-down event in polling mode and
link-up is faster.

Changes in v2:
- Fixed typos in phy_polling_mode() argument

Fixes: 93c0970493c71f ("net: phy: consider latched link-down status in polling mode")
Signed-off-by: Petr Oros <poros@redhat.com>
---
 drivers/net/phy/phy-c45.c    | 5 +++--
 drivers/net/phy/phy_device.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index a1caeee1223617..bceb0dcdecbd61 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -239,9 +239,10 @@ int genphy_c45_read_link(struct phy_device *phydev)
 
 		/* The link state is latched low so that momentary link
 		 * drops can be detected. Do not double-read the status
-		 * in polling mode to detect such short link drops.
+		 * in polling mode to detect such short link drops except
+		 * the link was already down.
 		 */
-		if (!phy_polling_mode(phydev)) {
+		if (!phy_polling_mode(phydev) || !phydev->link) {
 			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
 			if (val < 0)
 				return val;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6a5056e0ae7757..05417419c484fa 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
 
 	/* The link state is latched low so that momentary link
 	 * drops can be detected. Do not double-read the status
-	 * in polling mode to detect such short link drops.
+	 * in polling mode to detect such short link drops except
+	 * the link was already down.
 	 */
-	if (!phy_polling_mode(phydev)) {
+	if (!phy_polling_mode(phydev) || !phydev->link) {
 		status = phy_read(phydev, MII_BMSR);
 		if (status < 0)
 			return status;
-- 
2.24.1

Re: [PATCH net v2] phy: avoid unnecessary link-up delay in polling mode

[Heiner Kallweit]

On 29.01.2020 13:19, Petr Oros wrote:
> commit 93c0970493c71f ("net: phy: consider latched link-down status in
> polling mode") removed double-read of latched link-state register for
> polling mode from genphy_update_link(). This added extra ~1s delay into
> sequence link down->up.
> Following scenario:
>  - After boot link goes up
>  - phy_start() is called triggering an aneg restart, hence link goes
>    down and link-down info is latched.
>  - After aneg has finished link goes up. In phy_state_machine is checked
>    link state but it is latched "link is down". The state machine is
>    scheduled after one second and there is detected "link is up". This
>    extra delay can be avoided when we keep link-state register double read
>    in case when link was down previously.
> 
> With this solution we don't miss a link-down event in polling mode and
> link-up is faster.
> 

I have a little problem to understand why it should be faster this way.
Let's take an example: aneg takes 3.5s
Current behavior:

T0: aneg is started, link goes down, link-down status is latched
    (phydev->link is still 1)
T0+1s: state machine runs, latched link-down is read,
       phydev->link goes down, state change PHY_UP to PHY_NOLINK
T0+2s: state machine runs, up-to-date link-down is read
T0+3s: state machine runs, up-to-date link-down is read
T0+4s: state machine runs, aneg is finished, up-to-date link-up is read,
       phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING

Your patch changes the behavior of T0+1s only. So it should make a
difference only if aneg takes less than 1s.
Can you explain, based on the given example, how your change is
supposed to improve this?

And on a side note: I wouldn't consider this change a fix, therefore
it would be material for net-next that is closed at the moment.

Heiner

> Changes in v2:
> - Fixed typos in phy_polling_mode() argument
> 
> Fixes: 93c0970493c71f ("net: phy: consider latched link-down status in polling mode")
> Signed-off-by: Petr Oros <poros@redhat.com>
> ---
>  drivers/net/phy/phy-c45.c    | 5 +++--
>  drivers/net/phy/phy_device.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index a1caeee1223617..bceb0dcdecbd61 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -239,9 +239,10 @@ int genphy_c45_read_link(struct phy_device *phydev)
>  
>  		/* The link state is latched low so that momentary link
>  		 * drops can be detected. Do not double-read the status
> -		 * in polling mode to detect such short link drops.
> +		 * in polling mode to detect such short link drops except
> +		 * the link was already down.
>  		 */
> -		if (!phy_polling_mode(phydev)) {
> +		if (!phy_polling_mode(phydev) || !phydev->link) {
>  			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
>  			if (val < 0)
>  				return val;
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 6a5056e0ae7757..05417419c484fa 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
>  
>  	/* The link state is latched low so that momentary link
>  	 * drops can be detected. Do not double-read the status
> -	 * in polling mode to detect such short link drops.
> +	 * in polling mode to detect such short link drops except
> +	 * the link was already down.
>  	 */
> -	if (!phy_polling_mode(phydev)) {
> +	if (!phy_polling_mode(phydev) || !phydev->link) {
>  		status = phy_read(phydev, MII_BMSR);
>  		if (status < 0)
>  			return status;
> 

Re: [PATCH net v2] phy: avoid unnecessary link-up delay in polling mode

[Petr Oros]

Heiner Kallweit píše v St 29. 01. 2020 v 22:01 +0100:
> On 29.01.2020 13:19, Petr Oros wrote:
> > commit 93c0970493c71f ("net: phy: consider latched link-down status in
> > polling mode") removed double-read of latched link-state register for
> > polling mode from genphy_update_link(). This added extra ~1s delay into
> > sequence link down->up.
> > Following scenario:
> >  - After boot link goes up
> >  - phy_start() is called triggering an aneg restart, hence link goes
> >    down and link-down info is latched.
> >  - After aneg has finished link goes up. In phy_state_machine is checked
> >    link state but it is latched "link is down". The state machine is
> >    scheduled after one second and there is detected "link is up". This
> >    extra delay can be avoided when we keep link-state register double read
> >    in case when link was down previously.
> > 
> > With this solution we don't miss a link-down event in polling mode and
> > link-up is faster.
> > 
> 
> I have a little problem to understand why it should be faster this way.
> Let's take an example: aneg takes 3.5s
> Current behavior:
> 
> T0: aneg is started, link goes down, link-down status is latched
>     (phydev->link is still 1)
> T0+1s: state machine runs, latched link-down is read,
>        phydev->link goes down, state change PHY_UP to PHY_NOLINK
> T0+2s: state machine runs, up-to-date link-down is read
> T0+3s: state machine runs, up-to-date link-down is read
> T0+4s: state machine runs, aneg is finished, up-to-date link-up is read,
>        phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING
> 
> Your patch changes the behavior of T0+1s only. So it should make a
> difference only if aneg takes less than 1s.
> Can you explain, based on the given example, how your change is
> supposed to improve this?
> 


I see this behavior on real hw:
With patch:
T0+3s: state machine runs, up-to-date link-down is read
T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
       first BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==0,
       second BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==1,
       phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING

line: 1917 is first BMSR read
line: 1921 is second BMSR read

[   24.124572] xgene-mii-rgmii:03: genphy_restart_aneg()
[   24.132000] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
[   24.139347] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status:
0x7949
[   24.146783] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
0x7949
[   24.154174] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0

. supressed 3 same messages in T0+1,2,3s

[   28.609822] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
[   28.629906] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status:
0x7969
^^^^^^^^^^^^^^^ detected BMSR_ANEGCOMPLETE but not BMSR_LSTATUS
[   28.644590] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
0x796d
^^^^^^^^^^^^^^^ here is detected BMSR_ANEGCOMPLETE and BMSR_LSTATUS
[   28.658681] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 1

--------------------------------------------------------------------------------
---

Without patch:
T0+3s: state machine runs, up-to-date link-down is read
T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
       here i read link-down (BMSR_LSTATUS==0),
T0+5s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
       up-to-date link-up is read (BMSR_LSTATUS==1),
       phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING

line: 1917 is first BMSR read (status is zero because without patch it is readed
once)
line: 1921 is second BMSR read

[   24.862702] xgene-mii-rgmii:03: 1768: genphy_restart_aneg
[   24.869070] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
[   24.876409] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status: 0x0
[   24.885999] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
0x7949
[   24.893401] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0

. supressed 3 same messages in T0+1,2,3s

[   29.319613] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
[   29.326408] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status: 0x0
[   29.333557] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
0x7969
^^^^^^^^^^^^^^^ detected BMSR_ANEGCOMPLETE but not BMSR_LSTATUS
[   29.340923] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0

[   30.359713] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
[   30.366507] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status: 0x0
[   30.373650] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
0x796d
^^^^^^^^^^^^^^^ here is detected BMSR_ANEGCOMPLETE and BMSR_LSTATUS
[   30.381016] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 1

I tried many variants and it is deterministic behavior. Without patch is delay
one second longer due to later detect link up after aneg finish

-Petr


> And on a side note: I wouldn't consider this change a fix, therefore
> it would be material for net-next that is closed at the moment.
> 
> Heiner
> 
> > Changes in v2:
> > - Fixed typos in phy_polling_mode() argument
> > 
> > Fixes: 93c0970493c71f ("net: phy: consider latched link-down status in polling mode")
> > Signed-off-by: Petr Oros <poros@redhat.com>
> > ---
> >  drivers/net/phy/phy-c45.c    | 5 +++--
> >  drivers/net/phy/phy_device.c | 5 +++--
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > index a1caeee1223617..bceb0dcdecbd61 100644
> > --- a/drivers/net/phy/phy-c45.c
> > +++ b/drivers/net/phy/phy-c45.c
> > @@ -239,9 +239,10 @@ int genphy_c45_read_link(struct phy_device *phydev)
> >  
> >  		/* The link state is latched low so that momentary link
> >  		 * drops can be detected. Do not double-read the status
> > -		 * in polling mode to detect such short link drops.
> > +		 * in polling mode to detect such short link drops except
> > +		 * the link was already down.
> >  		 */
> > -		if (!phy_polling_mode(phydev)) {
> > +		if (!phy_polling_mode(phydev) || !phydev->link) {
> >  			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
> >  			if (val < 0)
> >  				return val;
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 6a5056e0ae7757..05417419c484fa 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
> >  
> >  	/* The link state is latched low so that momentary link
> >  	 * drops can be detected. Do not double-read the status
> > -	 * in polling mode to detect such short link drops.
> > +	 * in polling mode to detect such short link drops except
> > +	 * the link was already down.
> >  	 */
> > -	if (!phy_polling_mode(phydev)) {
> > +	if (!phy_polling_mode(phydev) || !phydev->link) {
> >  		status = phy_read(phydev, MII_BMSR);
> >  		if (status < 0)
> >  			return status;
> > 

Re: [PATCH net v2] phy: avoid unnecessary link-up delay in polling mode

[Heiner Kallweit]

On 31.01.2020 16:09, Petr Oros wrote:
> Heiner Kallweit píše v St 29. 01. 2020 v 22:01 +0100:
>> On 29.01.2020 13:19, Petr Oros wrote:
>>> commit 93c0970493c71f ("net: phy: consider latched link-down status in
>>> polling mode") removed double-read of latched link-state register for
>>> polling mode from genphy_update_link(). This added extra ~1s delay into
>>> sequence link down->up.
>>> Following scenario:
>>>  - After boot link goes up
>>>  - phy_start() is called triggering an aneg restart, hence link goes
>>>    down and link-down info is latched.
>>>  - After aneg has finished link goes up. In phy_state_machine is checked
>>>    link state but it is latched "link is down". The state machine is
>>>    scheduled after one second and there is detected "link is up". This
>>>    extra delay can be avoided when we keep link-state register double read
>>>    in case when link was down previously.
>>>
>>> With this solution we don't miss a link-down event in polling mode and
>>> link-up is faster.
>>>
>>
>> I have a little problem to understand why it should be faster this way.
>> Let's take an example: aneg takes 3.5s
>> Current behavior:
>>
>> T0: aneg is started, link goes down, link-down status is latched
>>     (phydev->link is still 1)
>> T0+1s: state machine runs, latched link-down is read,
>>        phydev->link goes down, state change PHY_UP to PHY_NOLINK
>> T0+2s: state machine runs, up-to-date link-down is read
>> T0+3s: state machine runs, up-to-date link-down is read
>> T0+4s: state machine runs, aneg is finished, up-to-date link-up is read,
>>        phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING
>>
>> Your patch changes the behavior of T0+1s only. So it should make a
>> difference only if aneg takes less than 1s.
>> Can you explain, based on the given example, how your change is
>> supposed to improve this?
>>
> 
> 
> I see this behavior on real hw:
> With patch:
> T0+3s: state machine runs, up-to-date link-down is read
> T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        first BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==0,
>        second BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==1,
>        phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING
> 
> line: 1917 is first BMSR read
> line: 1921 is second BMSR read
> 
> [   24.124572] xgene-mii-rgmii:03: genphy_restart_aneg()
> [   24.132000] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
> [   24.139347] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status:
> 0x7949
> [   24.146783] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
> 0x7949
> [   24.154174] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0
> 
> . supressed 3 same messages in T0+1,2,3s
> 
> [   28.609822] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
> [   28.629906] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status:
> 0x7969
> ^^^^^^^^^^^^^^^ detected BMSR_ANEGCOMPLETE but not BMSR_LSTATUS
> [   28.644590] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
> 0x796d
> ^^^^^^^^^^^^^^^ here is detected BMSR_ANEGCOMPLETE and BMSR_LSTATUS
> [   28.658681] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 1
> 

I see, thanks. Strange behavior of the PHY. Did you test also with other PHY's
whether they behave the same?

> --------------------------------------------------------------------------------
> ---
> 
> Without patch:
> T0+3s: state machine runs, up-to-date link-down is read
> T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        here i read link-down (BMSR_LSTATUS==0),
> T0+5s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        up-to-date link-up is read (BMSR_LSTATUS==1),
>        phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING
> 
> line: 1917 is first BMSR read (status is zero because without patch it is readed
> once)
> line: 1921 is second BMSR read
> 
> [   24.862702] xgene-mii-rgmii:03: 1768: genphy_restart_aneg
> [   24.869070] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
> [   24.876409] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status: 0x0
> [   24.885999] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
> 0x7949
> [   24.893401] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0
> 
> . supressed 3 same messages in T0+1,2,3s
> 
> [   29.319613] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
> [   29.326408] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status: 0x0
> [   29.333557] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
> 0x7969
> ^^^^^^^^^^^^^^^ detected BMSR_ANEGCOMPLETE but not BMSR_LSTATUS
> [   29.340923] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0
> 
> [   30.359713] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
> [   30.366507] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status: 0x0
> [   30.373650] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
> 0x796d
> ^^^^^^^^^^^^^^^ here is detected BMSR_ANEGCOMPLETE and BMSR_LSTATUS
> [   30.381016] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 1
> 
> I tried many variants and it is deterministic behavior. Without patch is delay
> one second longer due to later detect link up after aneg finish
> 
> -Petr
> 
> 
>> And on a side note: I wouldn't consider this change a fix, therefore
>> it would be material for net-next that is closed at the moment.
>>
>> Heiner
>>
>>> Changes in v2:
>>> - Fixed typos in phy_polling_mode() argument
>>>
>>> Fixes: 93c0970493c71f ("net: phy: consider latched link-down status in polling mode")
>>> Signed-off-by: Petr Oros <poros@redhat.com>
>>> ---
>>>  drivers/net/phy/phy-c45.c    | 5 +++--
>>>  drivers/net/phy/phy_device.c | 5 +++--
>>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>>> index a1caeee1223617..bceb0dcdecbd61 100644
>>> --- a/drivers/net/phy/phy-c45.c
>>> +++ b/drivers/net/phy/phy-c45.c
>>> @@ -239,9 +239,10 @@ int genphy_c45_read_link(struct phy_device *phydev)
>>>  
>>>  		/* The link state is latched low so that momentary link
>>>  		 * drops can be detected. Do not double-read the status
>>> -		 * in polling mode to detect such short link drops.
>>> +		 * in polling mode to detect such short link drops except
>>> +		 * the link was already down.
>>>  		 */
>>> -		if (!phy_polling_mode(phydev)) {
>>> +		if (!phy_polling_mode(phydev) || !phydev->link) {
>>>  			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
>>>  			if (val < 0)
>>>  				return val;
>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>> index 6a5056e0ae7757..05417419c484fa 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>>> @@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
>>>  
>>>  	/* The link state is latched low so that momentary link
>>>  	 * drops can be detected. Do not double-read the status
>>> -	 * in polling mode to detect such short link drops.
>>> +	 * in polling mode to detect such short link drops except
>>> +	 * the link was already down.
>>>  	 */
>>> -	if (!phy_polling_mode(phydev)) {
>>> +	if (!phy_polling_mode(phydev) || !phydev->link) {
>>>  		status = phy_read(phydev, MII_BMSR);
>>>  		if (status < 0)
>>>  			return status;
>>>
> 
> 

Re: [PATCH net v2] phy: avoid unnecessary link-up delay in polling mode

[Ivan Vecera]

On Fri, 31 Jan 2020 21:50:48 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> > 0x7949
> > [   24.154174] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0
> > 
> > . supressed 3 same messages in T0+1,2,3s
> > 
> > [   28.609822] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
> > [   28.629906] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status:
> > 0x7969
> > ^^^^^^^^^^^^^^^ detected BMSR_ANEGCOMPLETE but not BMSR_LSTATUS
> > [   28.644590] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
> > 0x796d
> > ^^^^^^^^^^^^^^^ here is detected BMSR_ANEGCOMPLETE and BMSR_LSTATUS
> > [   28.658681] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 1
> >   
> 
> I see, thanks. Strange behavior of the PHY. Did you test also with other PHY's
> whether they behave the same?

Yeah, it's strange... we could try different PHYs but anyway the double read was
removed for polling mode to detect momentary link drops but it make sense only
when phy->link is not 0. Thoughts?

Ivan

Re: [PATCH net v2] phy: avoid unnecessary link-up delay in polling mode

[Heiner Kallweit]

On 01.02.2020 11:25, Ivan Vecera wrote:
> On Fri, 31 Jan 2020 21:50:48 +0100
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>>> 0x7949
>>> [   24.154174] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0
>>>
>>> . supressed 3 same messages in T0+1,2,3s
>>>
>>> [   28.609822] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
>>> [   28.629906] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status:
>>> 0x7969
>>> ^^^^^^^^^^^^^^^ detected BMSR_ANEGCOMPLETE but not BMSR_LSTATUS
>>> [   28.644590] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
>>> 0x796d
>>> ^^^^^^^^^^^^^^^ here is detected BMSR_ANEGCOMPLETE and BMSR_LSTATUS
>>> [   28.658681] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 1
>>>   
>>
>> I see, thanks. Strange behavior of the PHY. Did you test also with other PHY's
>> whether they behave the same?
> 
> Yeah, it's strange... we could try different PHYs but anyway the double read was
> removed for polling mode to detect momentary link drops but it make sense only
> when phy->link is not 0. Thoughts?
> 
> Ivan
> 
I checked with the internal PHY of a Realtek NIC and it showed the same behavior.
So it seems that Realtek PHY's behave like this in general. Therefore I'm fine
with the patch. Just two things:
- Add details about this quirky behavior to the commit description.
- Resubmit annotated as net-next once net-next is open again. It's an improvement,
  not a fix.

Heiner

Re: [PATCH net v2] phy: avoid unnecessary link-up delay in polling mode

[Ivan Vecera]

On Sat, 1 Feb 2020 21:26:54 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 01.02.2020 11:25, Ivan Vecera wrote:
> > On Fri, 31 Jan 2020 21:50:48 +0100
> > Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >   
> >>> 0x7949
> >>> [   24.154174] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 0
> >>>
> >>> . supressed 3 same messages in T0+1,2,3s
> >>>
> >>> [   28.609822] xgene-mii-rgmii:03: genphy_update_link(), line: 1895, link: 0
> >>> [   28.629906] xgene-mii-rgmii:03: genphy_update_link(), line: 1917, status:
> >>> 0x7969
> >>> ^^^^^^^^^^^^^^^ detected BMSR_ANEGCOMPLETE but not BMSR_LSTATUS
> >>> [   28.644590] xgene-mii-rgmii:03: genphy_update_link(), line: 1921, status:
> >>> 0x796d
> >>> ^^^^^^^^^^^^^^^ here is detected BMSR_ANEGCOMPLETE and BMSR_LSTATUS
> >>> [   28.658681] xgene-mii-rgmii:03: genphy_update_link(), line: 1927, link: 1
> >>>     
> >>
> >> I see, thanks. Strange behavior of the PHY. Did you test also with other PHY's
> >> whether they behave the same?  
> > 
> > Yeah, it's strange... we could try different PHYs but anyway the double read was
> > removed for polling mode to detect momentary link drops but it make sense only
> > when phy->link is not 0. Thoughts?
> > 
> > Ivan
> >   
> I checked with the internal PHY of a Realtek NIC and it showed the same behavior.
> So it seems that Realtek PHY's behave like this in general. Therefore I'm fine
> with the patch. Just two things:
> - Add details about this quirky behavior to the commit description.
> - Resubmit annotated as net-next once net-next is open again. It's an improvement,
>   not a fix.

LGTM. Thanks for confirmation.

Ivan

[PATCH net-next] phy: avoid unnecessary link-up delay in polling mode

[Petr Oros]

commit 93c0970493c71f ("net: phy: consider latched link-down status in
polling mode") removed double-read of latched link-state register for
polling mode from genphy_update_link(). This added extra ~1s delay into
sequence link down->up.
Following scenario:
 - After boot link goes up
 - phy_start() is called triggering an aneg restart, hence link goes
   down and link-down info is latched.
 - After aneg has finished link goes up. In phy_state_machine is checked
   link state but it is latched "link is down". The state machine is
   scheduled after one second and there is detected "link is up". This
   extra delay can be avoided when we keep link-state register double read
   in case when link was down previously.

With this solution we don't miss a link-down event in polling mode and
link-up is faster.

Details about this quirky behavior on Realtek phy:
Without patch:
T0:    aneg is started, link goes down, link-down status is latched
T0+3s: state machine runs, up-to-date link-down is read
T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
       here i read link-down (BMSR_LSTATUS==0),
T0+5s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
       up-to-date link-up is read (BMSR_LSTATUS==1),
       phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING

With patch:
T0:    aneg is started, link goes down, link-down status is latched
T0+3s: state machine runs, up-to-date link-down is read
T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
       first BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==0,
       second BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==1,
       phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING

Signed-off-by: Petr Oros <poros@redhat.com>
---
 drivers/net/phy/phy-c45.c    | 5 +++--
 drivers/net/phy/phy_device.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index a1caeee1223617..bceb0dcdecbd61 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -239,9 +239,10 @@ int genphy_c45_read_link(struct phy_device *phydev)
 
 		/* The link state is latched low so that momentary link
 		 * drops can be detected. Do not double-read the status
-		 * in polling mode to detect such short link drops.
+		 * in polling mode to detect such short link drops except
+		 * the link was already down.
 		 */
-		if (!phy_polling_mode(phydev)) {
+		if (!phy_polling_mode(phydev) || !phydev->link) {
 			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
 			if (val < 0)
 				return val;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2a973265de8095..be0129231c2a1c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1930,9 +1930,10 @@ int genphy_update_link(struct phy_device *phydev)
 
 	/* The link state is latched low so that momentary link
 	 * drops can be detected. Do not double-read the status
-	 * in polling mode to detect such short link drops.
+	 * in polling mode to detect such short link drops except
+	 * the link was already down.
 	 */
-	if (!phy_polling_mode(phydev)) {
+	if (!phy_polling_mode(phydev) || !phydev->link) {
 		status = phy_read(phydev, MII_BMSR);
 		if (status < 0)
 			return status;
-- 
2.24.1

Re: [PATCH net-next] phy: avoid unnecessary link-up delay in polling mode

[Heiner Kallweit]

On 18.02.2020 10:35, Petr Oros wrote:
> commit 93c0970493c71f ("net: phy: consider latched link-down status in
> polling mode") removed double-read of latched link-state register for
> polling mode from genphy_update_link(). This added extra ~1s delay into
> sequence link down->up.
> Following scenario:
>  - After boot link goes up
>  - phy_start() is called triggering an aneg restart, hence link goes
>    down and link-down info is latched.
>  - After aneg has finished link goes up. In phy_state_machine is checked
>    link state but it is latched "link is down". The state machine is
>    scheduled after one second and there is detected "link is up". This
>    extra delay can be avoided when we keep link-state register double read
>    in case when link was down previously.
> 
> With this solution we don't miss a link-down event in polling mode and
> link-up is faster.
> 
> Details about this quirky behavior on Realtek phy:
> Without patch:
> T0:    aneg is started, link goes down, link-down status is latched
> T0+3s: state machine runs, up-to-date link-down is read
> T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        here i read link-down (BMSR_LSTATUS==0),
> T0+5s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        up-to-date link-up is read (BMSR_LSTATUS==1),
>        phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING
> 
> With patch:
> T0:    aneg is started, link goes down, link-down status is latched
> T0+3s: state machine runs, up-to-date link-down is read
> T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        first BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==0,
>        second BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==1,
>        phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING
> 
> Signed-off-by: Petr Oros <poros@redhat.com>
> ---
Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH net-next] phy: avoid unnecessary link-up delay in polling mode

[David Miller]

From: Petr Oros <poros@redhat.com>
Date: Tue, 18 Feb 2020 10:35:55 +0100

> commit 93c0970493c71f ("net: phy: consider latched link-down status in
> polling mode") removed double-read of latched link-state register for
> polling mode from genphy_update_link(). This added extra ~1s delay into
> sequence link down->up.
> Following scenario:
>  - After boot link goes up
>  - phy_start() is called triggering an aneg restart, hence link goes
>    down and link-down info is latched.
>  - After aneg has finished link goes up. In phy_state_machine is checked
>    link state but it is latched "link is down". The state machine is
>    scheduled after one second and there is detected "link is up". This
>    extra delay can be avoided when we keep link-state register double read
>    in case when link was down previously.
> 
> With this solution we don't miss a link-down event in polling mode and
> link-up is faster.
> 
> Details about this quirky behavior on Realtek phy:
> Without patch:
> T0:    aneg is started, link goes down, link-down status is latched
> T0+3s: state machine runs, up-to-date link-down is read
> T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        here i read link-down (BMSR_LSTATUS==0),
> T0+5s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        up-to-date link-up is read (BMSR_LSTATUS==1),
>        phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING
> 
> With patch:
> T0:    aneg is started, link goes down, link-down status is latched
> T0+3s: state machine runs, up-to-date link-down is read
> T0+4s: state machine runs, aneg is finished (BMSR_ANEGCOMPLETE==1),
>        first BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==0,
>        second BMSR read: BMSR_ANEGCOMPLETE==1 and BMSR_LSTATUS==1,
>        phydev->link goes up, state change PHY_NOLINK to PHY_RUNNING
> 
> Signed-off-by: Petr Oros <poros@redhat.com>

Applied, thank you.