On Fri, Apr 10, 2020 at 05:43:04PM -0700, Jakub Kicinski wrote:
On Wed, 8 Apr 2020 23:43:26 +0200 Clemens Gruber wrote:
The negotiation of flow control / pause frame modes was broken since commit fcf1f59afc67 ("net: phy: marvell: rearrange to use genphy_read_lpa()") moved the setting of phydev->duplex below the phy_resolve_aneg_pause call. Due to a check of DUPLEX_FULL in that function, phydev->pause was no longer set.
Fix it by moving the parsing of the status variable before the blocks dealing with the pause frames.
Fixes: fcf1f59afc67 ("net: phy: marvell: rearrange to use genphy_read_lpa()") Cc: stable@vger.kernel.org # v5.6+
nit: please don't CC stable on networking patches
Signed-off-by: Clemens Gruber clemens.gruber@pqgruber.com
drivers/net/phy/marvell.c | 44 +++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 4714ca0e0d4b..02cde4c0668c 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -1263,6 +1263,28 @@ static int marvell_read_status_page_an(struct phy_device *phydev, int lpa; int err;
- if (!(status & MII_M1011_PHY_STATUS_RESOLVED))
return 0;If we return early here won't we miss updating the advertising bits? We will no longer call e.g. fiber_lpa_mod_linkmode_lpa_t().
Perhaps extracting info from status should be moved to a helper so we can return early without affecting the rest of the flow?
Is my understanding correct? Russell?
You are correct - and yes, there is also a problem here.
It is not clear whether the resolved bit is set before or after the link status reports that link is up - however, the resolved bit indicates whether the speed and duplex are valid.
What I've done elsewhere is if the resolved bit is not set, then we force phydev->link to be false, so we don't attempt to process a link-up status until we can read the link parameters. I think that's what needs to happen here, i.o.w.:
if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) { phydev->link = 0; return 0; }
especially as we're not reading the LPA.