net: phy: Fix overlong PHY timeout

Message ID 20200103220847.17415-1-andre.przywara@arm.com
State Accepted
Commit a44ee246c570deb9190214d0711057a86b0f6a86
Headers show
Series
  • net: phy: Fix overlong PHY timeout
Related show

Commit Message

Andre Przywara Jan. 3, 2020, 10:08 p.m.
Commit 27c3f70f3b50 ("net: phy: Increase link up delay in
genphy_update_link()") increased the per-iteration waiting time from
1ms to 50ms, without adjusting the timeout counter. This lead to the
timeout increasing from the typical 4 seconds to over three minutes.

Adjust the timeout counter evaluation by that factor of 50 to bring the
timeout back to the intended value.

Signed-off-by: Andre Przywara <andre.przywara at arm.com>
---
 drivers/net/phy/phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stefan Roese Jan. 4, 2020, 6:30 a.m. | #1
On 03.01.20 23:08, Andre Przywara wrote:
> Commit 27c3f70f3b50 ("net: phy: Increase link up delay in
> genphy_update_link()") increased the per-iteration waiting time from
> 1ms to 50ms, without adjusting the timeout counter. This lead to the
> timeout increasing from the typical 4 seconds to over three minutes.
> 
> Adjust the timeout counter evaluation by that factor of 50 to bring the
> timeout back to the intended value.
> 
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> ---
>   drivers/net/phy/phy.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 80a7664e49..5cf9c165b6 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -244,7 +244,7 @@ int genphy_update_link(struct phy_device *phydev)
>   			/*
>   			 * Timeout reached ?
>   			 */
> -			if (i > PHY_ANEG_TIMEOUT) {
> +			if (i > (PHY_ANEG_TIMEOUT / 50)) {
>   				printf(" TIMEOUT !\n");
>   				phydev->link = 0;
>   				return -ETIMEDOUT;
> 

Andre, thanks for taking care of this.

Reviewed-by: Stefan Roese <sr at denx.de>

Thanks,
Stefan
Matthias Brugger Jan. 30, 2020, 11 a.m. | #2
On 03/01/2020 23:08, Andre Przywara wrote:
> Commit 27c3f70f3b50 ("net: phy: Increase link up delay in
> genphy_update_link()") increased the per-iteration waiting time from
> 1ms to 50ms, without adjusting the timeout counter. This lead to the
> timeout increasing from the typical 4 seconds to over three minutes.
> 
> Adjust the timeout counter evaluation by that factor of 50 to bring the
> timeout back to the intended value.
> 
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>

I tested this on RPi4 with the genet patches on top. Now the timeout is
reasonable :)

Tested-by: Matthias Brugger <mbrugger at suse.com>

> ---
>  drivers/net/phy/phy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 80a7664e49..5cf9c165b6 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -244,7 +244,7 @@ int genphy_update_link(struct phy_device *phydev)
>  			/*
>  			 * Timeout reached ?
>  			 */
> -			if (i > PHY_ANEG_TIMEOUT) {
> +			if (i > (PHY_ANEG_TIMEOUT / 50)) {
>  				printf(" TIMEOUT !\n");
>  				phydev->link = 0;
>  				return -ETIMEDOUT;
>
Simon Goldschmidt March 5, 2020, 7:25 a.m. | #3
On Fri, Feb 21, 2020 at 5:33 PM Matthias Brugger <matthias.bgg at gmail.com> wrote:
>
> Hi Joe,
>
> On 30/01/2020 12:00, Matthias Brugger wrote:
> >
> >
> > On 03/01/2020 23:08, Andre Przywara wrote:
> >> Commit 27c3f70f3b50 ("net: phy: Increase link up delay in
> >> genphy_update_link()") increased the per-iteration waiting time from
> >> 1ms to 50ms, without adjusting the timeout counter. This lead to the
> >> timeout increasing from the typical 4 seconds to over three minutes.
> >>
> >> Adjust the timeout counter evaluation by that factor of 50 to bring the
> >> timeout back to the intended value.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara at arm.com>

A "Fixes:" tag would have been nice...

Anyway:
Tested-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>

> >
> > I tested this on RPi4 with the genet patches on top. Now the timeout is
> > reasonable :)
> >
> > Tested-by: Matthias Brugger <mbrugger at suse.com>
> >
>
> Friedly reminder, are you planning to take this fix for v2020.04?

Yes, please! This timeout is really annoying right now!

Regards,
Simon

>
> Regards,
> Matthias
>
> >> ---
> >>  drivers/net/phy/phy.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> >> index 80a7664e49..5cf9c165b6 100644
> >> --- a/drivers/net/phy/phy.c
> >> +++ b/drivers/net/phy/phy.c
> >> @@ -244,7 +244,7 @@ int genphy_update_link(struct phy_device *phydev)
> >>                      /*
> >>                       * Timeout reached ?
> >>                       */
> >> -                    if (i > PHY_ANEG_TIMEOUT) {
> >> +                    if (i > (PHY_ANEG_TIMEOUT / 50)) {
> >>                              printf(" TIMEOUT !\n");
> >>                              phydev->link = 0;
> >>                              return -ETIMEDOUT;
> >>
>
Joe Hershberger March 5, 2020, 11:20 p.m. | #4
On Sat, Jan 4, 2020 at 3:56 AM Andre Przywara <andre.przywara at arm.com> wrote:
>
> Commit 27c3f70f3b50 ("net: phy: Increase link up delay in
> genphy_update_link()") increased the per-iteration waiting time from
> 1ms to 50ms, without adjusting the timeout counter. This lead to the
> timeout increasing from the typical 4 seconds to over three minutes.
>
> Adjust the timeout counter evaluation by that factor of 50 to bring the
> timeout back to the intended value.
>
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>

Acked-by: Joe Hershberger <joe.hershberger at ni.com>

Patch

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 80a7664e49..5cf9c165b6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -244,7 +244,7 @@  int genphy_update_link(struct phy_device *phydev)
 			/*
 			 * Timeout reached ?
 			 */
-			if (i > PHY_ANEG_TIMEOUT) {
+			if (i > (PHY_ANEG_TIMEOUT / 50)) {
 				printf(" TIMEOUT !\n");
 				phydev->link = 0;
 				return -ETIMEDOUT;