[PATCH v2 net-next] net: phy: at803x: remove at803x_aneg_done()

[PATCH v2 net-next] net: phy: at803x: remove at803x_aneg_done()

[Michael Walle]

Here is what Vladimir says about it:

  at803x_aneg_done() keeps the aneg reporting as "not done" even when
  the copper-side link was reported as up, but the in-band autoneg has
  not finished.

  That was the _intended_ behavior when that code was introduced, and
  Heiner have said about it [1]:

  | That's not nice from the PHY:
  | It signals "link up", and if the system asks the PHY for link details,
  | then it sheepishly says "well, link is *almost* up".

  If the specification of phy_aneg_done behavior does not include
  in-band autoneg (and it doesn't), then this piece of code does not
  belong here.

  The fact that we can no longer trigger this code from phylib is yet
  another reason why it fails at its intended (and wrong) purpose and
  should be removed.

Removing the SGMII link check, would just keep the call to
genphy_aneg_done(), which is also the fallback. Thus we can just remove
at803x_aneg_done() altogether.

[1] https://lore.kernel.org/netdev/fdf0074a-2572-5914-6f3e-77202cbf96de@gmail.com/

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/at803x.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c2aa4c92edde..d7799beb811c 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -751,36 +751,6 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	}
 }
 
-static int at803x_aneg_done(struct phy_device *phydev)
-{
-	int ccr;
-
-	int aneg_done = genphy_aneg_done(phydev);
-	if (aneg_done != BMSR_ANEGCOMPLETE)
-		return aneg_done;
-
-	/*
-	 * in SGMII mode, if copper side autoneg is successful,
-	 * also check SGMII side autoneg result
-	 */
-	ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
-	if ((ccr & AT803X_MODE_CFG_MASK) != AT803X_MODE_CFG_SGMII)
-		return aneg_done;
-
-	/* switch to SGMII/fiber page */
-	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
-
-	/* check if the SGMII link is OK. */
-	if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) {
-		phydev_warn(phydev, "803x_aneg_done: SGMII link is not ok\n");
-		aneg_done = 0;
-	}
-	/* switch back to copper page */
-	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);
-
-	return aneg_done;
-}
-
 static int at803x_read_status(struct phy_device *phydev)
 {
 	int ss, err, old_link = phydev->link;
@@ -1198,7 +1168,6 @@ static struct phy_driver at803x_driver[] = {
 	.resume			= at803x_resume,
 	/* PHY_GBIT_FEATURES */
 	.read_status		= at803x_read_status,
-	.aneg_done		= at803x_aneg_done,
 	.config_intr		= &at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
-- 
2.20.1

Re: [PATCH v2 net-next] net: phy: at803x: remove at803x_aneg_done()

[Michael Walle]

Am 2021-03-18 20:44, schrieb Michael Walle:
> Here is what Vladimir says about it:
> 
>   at803x_aneg_done() keeps the aneg reporting as "not done" even when
>   the copper-side link was reported as up, but the in-band autoneg has
>   not finished.
> 
>   That was the _intended_ behavior when that code was introduced, and
>   Heiner have said about it [1]:
> 
>   | That's not nice from the PHY:
>   | It signals "link up", and if the system asks the PHY for link 
> details,
>   | then it sheepishly says "well, link is *almost* up".
> 
>   If the specification of phy_aneg_done behavior does not include
>   in-band autoneg (and it doesn't), then this piece of code does not
>   belong here.
> 
>   The fact that we can no longer trigger this code from phylib is yet
>   another reason why it fails at its intended (and wrong) purpose and
>   should be removed.
> 
> Removing the SGMII link check, would just keep the call to
> genphy_aneg_done(), which is also the fallback. Thus we can just remove
> at803x_aneg_done() altogether.
> 
> [1] 
> https://lore.kernel.org/netdev/fdf0074a-2572-5914-6f3e-77202cbf96de@gmail.com/
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---

Sorry forgot the version history:

Changes since v1:
  - more detailed commit message

-michael

Re: [PATCH v2 net-next] net: phy: at803x: remove at803x_aneg_done()

[Heiner Kallweit]

On 18.03.2021 20:44, Michael Walle wrote:
> Here is what Vladimir says about it:
> 
>   at803x_aneg_done() keeps the aneg reporting as "not done" even when
>   the copper-side link was reported as up, but the in-band autoneg has
>   not finished.
> 
>   That was the _intended_ behavior when that code was introduced, and
>   Heiner have said about it [1]:
> 
>   | That's not nice from the PHY:
>   | It signals "link up", and if the system asks the PHY for link details,
>   | then it sheepishly says "well, link is *almost* up".
> 
>   If the specification of phy_aneg_done behavior does not include
>   in-band autoneg (and it doesn't), then this piece of code does not
>   belong here.
> 
>   The fact that we can no longer trigger this code from phylib is yet
>   another reason why it fails at its intended (and wrong) purpose and
>   should be removed.
> 
> Removing the SGMII link check, would just keep the call to
> genphy_aneg_done(), which is also the fallback. Thus we can just remove
> at803x_aneg_done() altogether.
> 
> [1] https://lore.kernel.org/netdev/fdf0074a-2572-5914-6f3e-77202cbf96de@gmail.com/
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---

Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH v2 net-next] net: phy: at803x: remove at803x_aneg_done()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Thu, 18 Mar 2021 20:44:31 +0100 you wrote:
> Here is what Vladimir says about it:
> 
>   at803x_aneg_done() keeps the aneg reporting as "not done" even when
>   the copper-side link was reported as up, but the in-band autoneg has
>   not finished.
> 
>   That was the _intended_ behavior when that code was introduced, and
>   Heiner have said about it [1]:
> 
> [...]

Here is the summary with links:
  - [v2,net-next] net: phy: at803x: remove at803x_aneg_done()
    https://git.kernel.org/netdev/net-next/c/5b6b827413e8

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html