[PATCH] phy: Elimination the forced speed reduction algorithm.

[PATCH] phy: Elimination the forced speed reduction algorithm.

[Kirill Kapranov]

In case of fixed speed set up for a NIC (e.g. ethtool -s eth0 autoneg off speed
100 duplex full) with an ethernet cable plugged off, the mentioned algorithm
slows down a NIC speed, so further cable hook-up leads to nonoperable link state.

Signed-off-by: Kirill Kapranov <kapranoff@inbox.ru>
---
 drivers/net/phy/phy.c |   50 +------------------------------------------------
 1 files changed, 1 insertions(+), 49 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ef9ea92..828d4e6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -463,33 +463,6 @@ void phy_stop_machine(struct phy_device *phydev)
 }
 
 /**
- * phy_force_reduction - reduce PHY speed/duplex settings by one step
- * @phydev: target phy_device struct
- *
- * Description: Reduces the speed/duplex settings by one notch,
- *   in this order--
- *   1000/FULL, 1000/HALF, 100/FULL, 100/HALF, 10/FULL, 10/HALF.
- *   The function bottoms out at 10/HALF.
- */
-static void phy_force_reduction(struct phy_device *phydev)
-{
-	int idx;
-
-	idx = phy_find_setting(phydev->speed, phydev->duplex);
-	
-	idx++;
-
-	idx = phy_find_valid(idx, phydev->supported);
-
-	phydev->speed = settings[idx].speed;
-	phydev->duplex = settings[idx].duplex;
-
-	pr_info("Trying %d/%s\n",
-		phydev->speed, DUPLEX_FULL == phydev->duplex ? "FULL" : "HALF");
-}
-
-
-/**
  * phy_error - enter HALTED state for this PHY device
  * @phydev: target phy_device struct
  *
@@ -818,30 +791,11 @@ void phy_state_machine(struct work_struct *work)
 				phydev->adjust_link(phydev->attached_dev);
 
 			} else if (0 == phydev->link_timeout--) {
-				int idx;
-
 				needs_aneg = 1;
 				/* If we have the magic_aneg bit,
 				 * we try again */
 				if (phydev->drv->flags & PHY_HAS_MAGICANEG)
 					break;
-
-				/* The timer expired, and we still
-				 * don't have a setting, so we try
-				 * forcing it until we find one that
-				 * works, starting from the fastest speed,
-				 * and working our way down */
-				idx = phy_find_valid(0, phydev->supported);
-
-				phydev->speed = settings[idx].speed;
-				phydev->duplex = settings[idx].duplex;
-
-				phydev->autoneg = AUTONEG_DISABLE;
-
-				pr_info("Trying %d/%s\n",
-					phydev->speed,
-					DUPLEX_FULL == phydev->duplex ?
-					"FULL" : "HALF");
 			}
 			break;
 		case PHY_NOLINK:
@@ -866,10 +820,8 @@ void phy_state_machine(struct work_struct *work)
 				phydev->state = PHY_RUNNING;
 				netif_carrier_on(phydev->attached_dev);
 			} else {
-				if (0 == phydev->link_timeout--) {
-					phy_force_reduction(phydev);
+				if (0 == phydev->link_timeout--)
 					needs_aneg = 1;
-				}
 			}
 
 			phydev->adjust_link(phydev->attached_dev);
-- 
1.7.2.5

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[David Miller]

From: Kirill Kapranov <kapranoff@inbox.ru>
Date: Wed, 27 Mar 2013 15:16:13 +0400

> In case of fixed speed set up for a NIC (e.g. ethtool -s eth0 autoneg off speed
> 100 duplex full) with an ethernet cable plugged off, the mentioned algorithm
> slows down a NIC speed, so further cable hook-up leads to nonoperable link state.
> 
> Signed-off-by: Kirill Kapranov <kapranoff@inbox.ru>

Applied.

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[Kirill Kapranov]

Fri, 29/03/2013 14:38 -0400, David Miller пишет:
> Why are you posting this again?  I've applied your patch 3 days
> ago.

oops...
The last letter I sent was a coding style correction -- removing
trailing whitespace in response to remark of Tue, 26 Mar 2013 12:59:13
-0400 (EDT)  by David Miller:
> 
> 
> > -                                     phy_force_reduction(phydev);
> > +                             if (0 == phydev->link_timeout--) 
>                                                                 ^^^^
> Trailing whitespace, please remove.

I sent it  Wed, 27 Mar 2013 15:16:13 +0400

After receiving confirmation (Wed, 27 Mar 2013 13:11:02 -0400 (EDT))

> Applied.

from David Miller, I haven't sent any post.

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[David Miller]

From: Kirill Kapranov <kapranoff@inbox.ru>
Date: Tue, 26 Mar 2013 17:34:32 +0400

> In case of fixed speed set up for a NIC (e.g. ethtool -s eth0 autoneg off speed
> 100 duplex full) with an ethernet cable plugged off, the mentioned algorithm
> slows down a NIC speed, so further cable hook-up leads to nonoperable link state.
> 
> Signed-off-by: Kirill Kapranov <kapranoff@inbox.ru>

Why are you posting this again?  I've applied your patch 3 days
ago.

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[David Miller]

From: Kirill Kapranov <kapranoff@inbox.ru>
Date: Tue, 26 Mar 2013 17:34:32 +0400

> -					phy_force_reduction(phydev);
> +				if (0 == phydev->link_timeout--) 
                                                                ^^^^

Trailing whitespace, please remove.

Also, for whatever reason this patch didn't make it to the mailing
list, that's critical because we automate the tracking of patches by
catching the patch postings made to the list.

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[David Miller]

From: Kirill Kapranov <kapranoff@inbox.ru>
Date: Tue, 26 Mar 2013 18:56:32 +0400

> Does the absence of criticism mean that this patch is ready for
> upstream?

You were given feedback on coding style, did you address and fix these
issues in the submission you made today?

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[Kirill Kapranov]

Does the absence of criticism mean that this patch is ready for
upstream?
Maybe I shall write more verbose explanation additionally?

[PATCH] phy: Elimination the forced speed reduction algorithm.

[Kirill Kapranov]

In case of fixed speed set up for a NIC (e.g. ethtool -s eth0 autoneg off speed
100 duplex full) with an ethernet cable plugged off, the mentioned algorithm
slows down a NIC speed, so further cable hook-up leads to nonoperable link state.

Signed-off-by: Kirill Kapranov <kapranoff@inbox.ru>
---
 drivers/net/phy/phy.c |   50 +------------------------------------------------
 1 files changed, 1 insertions(+), 49 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ef9ea92..fc2dd94 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -463,33 +463,6 @@ void phy_stop_machine(struct phy_device *phydev)
 }
 
 /**
- * phy_force_reduction - reduce PHY speed/duplex settings by one step
- * @phydev: target phy_device struct
- *
- * Description: Reduces the speed/duplex settings by one notch,
- *   in this order--
- *   1000/FULL, 1000/HALF, 100/FULL, 100/HALF, 10/FULL, 10/HALF.
- *   The function bottoms out at 10/HALF.
- */
-static void phy_force_reduction(struct phy_device *phydev)
-{
-	int idx;
-
-	idx = phy_find_setting(phydev->speed, phydev->duplex);
-	
-	idx++;
-
-	idx = phy_find_valid(idx, phydev->supported);
-
-	phydev->speed = settings[idx].speed;
-	phydev->duplex = settings[idx].duplex;
-
-	pr_info("Trying %d/%s\n",
-		phydev->speed, DUPLEX_FULL == phydev->duplex ? "FULL" : "HALF");
-}
-
-
-/**
  * phy_error - enter HALTED state for this PHY device
  * @phydev: target phy_device struct
  *
@@ -818,30 +791,11 @@ void phy_state_machine(struct work_struct *work)
 				phydev->adjust_link(phydev->attached_dev);
 
 			} else if (0 == phydev->link_timeout--) {
-				int idx;
-
 				needs_aneg = 1;
 				/* If we have the magic_aneg bit,
 				 * we try again */
 				if (phydev->drv->flags & PHY_HAS_MAGICANEG)
 					break;
-
-				/* The timer expired, and we still
-				 * don't have a setting, so we try
-				 * forcing it until we find one that
-				 * works, starting from the fastest speed,
-				 * and working our way down */
-				idx = phy_find_valid(0, phydev->supported);
-
-				phydev->speed = settings[idx].speed;
-				phydev->duplex = settings[idx].duplex;
-
-				phydev->autoneg = AUTONEG_DISABLE;
-
-				pr_info("Trying %d/%s\n",
-					phydev->speed,
-					DUPLEX_FULL == phydev->duplex ?
-					"FULL" : "HALF");
 			}
 			break;
 		case PHY_NOLINK:
@@ -866,10 +820,8 @@ void phy_state_machine(struct work_struct *work)
 				phydev->state = PHY_RUNNING;
 				netif_carrier_on(phydev->attached_dev);
 			} else {
-				if (0 == phydev->link_timeout--) {
-					phy_force_reduction(phydev);
+				if (0 == phydev->link_timeout--) 
 					needs_aneg = 1;
-				}
 			}
 
 			phydev->adjust_link(phydev->attached_dev);
-- 
1.7.2.5

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[Kirill Kapranov]

15/03/2013 08:35 -0400, David Miller wrote:
> From: Kirill Kapranov <kapranoff@inbox.ru>
> Date: Thu, 14 Mar 2013 14:40:52 +0400
> 
> > @@ -867,7 +821,6 @@ void phy_state_machine(struct work_struct *work)
> >  				netif_carrier_on(phydev->attached_dev);
> >  			} else {
> >  				if (0 == phydev->link_timeout--) {
> > -					phy_force_reduction(phydev);
> >  					needs_aneg = 1;
> >  				}
> 
> This is not a single-statement basic block, and therefore you
> should remove the surrounding braces.

Thank You, the excess braces will be removed.

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Fri, 15 Mar 2013 05:48:49 -0700

> The phy code also uses non standard kernel style tests like
> 	if (CONSTANT == variable)
> instead of
> 	if (variable == CONSTANT)

Right, to trigger compile time errors when "=" is accidently used
instead of "==".

But the whole driver is like this already.

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[Joe Perches]

On Fri, 2013-03-15 at 08:35 -0400, David Miller wrote:
> From: Kirill Kapranov <kapranoff@inbox.ru>
> Date: Thu, 14 Mar 2013 14:40:52 +0400
> 
> > @@ -867,7 +821,6 @@ void phy_state_machine(struct work_struct *work)
> >                               netif_carrier_on(phydev->attached_dev);
> >                       } else {
> >                               if (0 == phydev->link_timeout--) {
> > -                                     phy_force_reduction(phydev);
> >                                       needs_aneg = 1;
> >                               }
> 
> This is not a single-statement basic block, and therefore you
> should remove the surrounding braces.

s/not/now/

The phy code also uses non standard kernel style tests like
	if (CONSTANT == variable)
instead of
	if (variable == CONSTANT)

Re: [PATCH] phy: Elimination the forced speed reduction algorithm.

[David Miller]

From: Kirill Kapranov <kapranoff@inbox.ru>
Date: Thu, 14 Mar 2013 14:40:52 +0400

> @@ -867,7 +821,6 @@ void phy_state_machine(struct work_struct *work)
>  				netif_carrier_on(phydev->attached_dev);
>  			} else {
>  				if (0 == phydev->link_timeout--) {
> -					phy_force_reduction(phydev);
>  					needs_aneg = 1;
>  				}

This is not a single-statement basic block, and therefore you
should remove the surrounding braces.

[PATCH] phy: Elimination the forced speed reduction algorithm.

[Kirill Kapranov]

In case of fixed speed set up for a NIC (e.g. ethtool -s eth0 autoneg off speed
100 duplex full) with an ethernet cable plugged off, the mentioned algorithm
slows down a NIC speed, so further cable hook-up leads to nonoperable link state.

Signed-off-by: Kirill Kapranov <kapranoff@inbox.ru>
---
 drivers/net/phy/phy.c |   47 -----------------------------------------------
 1 files changed, 0 insertions(+), 47 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ef9ea92..0e40170 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -463,33 +463,6 @@ void phy_stop_machine(struct phy_device *phydev)
 }
 
 /**
- * phy_force_reduction - reduce PHY speed/duplex settings by one step
- * @phydev: target phy_device struct
- *
- * Description: Reduces the speed/duplex settings by one notch,
- *   in this order--
- *   1000/FULL, 1000/HALF, 100/FULL, 100/HALF, 10/FULL, 10/HALF.
- *   The function bottoms out at 10/HALF.
- */
-static void phy_force_reduction(struct phy_device *phydev)
-{
-	int idx;
-
-	idx = phy_find_setting(phydev->speed, phydev->duplex);
-	
-	idx++;
-
-	idx = phy_find_valid(idx, phydev->supported);
-
-	phydev->speed = settings[idx].speed;
-	phydev->duplex = settings[idx].duplex;
-
-	pr_info("Trying %d/%s\n",
-		phydev->speed, DUPLEX_FULL == phydev->duplex ? "FULL" : "HALF");
-}
-
-
-/**
  * phy_error - enter HALTED state for this PHY device
  * @phydev: target phy_device struct
  *
@@ -818,30 +791,11 @@ void phy_state_machine(struct work_struct *work)
 				phydev->adjust_link(phydev->attached_dev);
 
 			} else if (0 == phydev->link_timeout--) {
-				int idx;
-
 				needs_aneg = 1;
 				/* If we have the magic_aneg bit,
 				 * we try again */
 				if (phydev->drv->flags & PHY_HAS_MAGICANEG)
 					break;
-
-				/* The timer expired, and we still
-				 * don't have a setting, so we try
-				 * forcing it until we find one that
-				 * works, starting from the fastest speed,
-				 * and working our way down */
-				idx = phy_find_valid(0, phydev->supported);
-
-				phydev->speed = settings[idx].speed;
-				phydev->duplex = settings[idx].duplex;
-
-				phydev->autoneg = AUTONEG_DISABLE;
-
-				pr_info("Trying %d/%s\n",
-					phydev->speed,
-					DUPLEX_FULL == phydev->duplex ?
-					"FULL" : "HALF");
 			}
 			break;
 		case PHY_NOLINK:
@@ -867,7 +821,6 @@ void phy_state_machine(struct work_struct *work)
 				netif_carrier_on(phydev->attached_dev);
 			} else {
 				if (0 == phydev->link_timeout--) {
-					phy_force_reduction(phydev);
 					needs_aneg = 1;
 				}
 			}
-- 
1.7.2.5