Messages in this thread | | | Date | Fri, 10 Apr 2020 17:43:04 -0700 | From | Jakub Kicinski <> | Subject | Re: [PATCH] net: phy: marvell: Fix pause frame negotiation |
| |
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?
> + if (status & MII_M1011_PHY_STATUS_FULLDUPLEX) > + phydev->duplex = DUPLEX_FULL; > + else > + phydev->duplex = DUPLEX_HALF; > + > + switch (status & MII_M1011_PHY_STATUS_SPD_MASK) { > + case MII_M1011_PHY_STATUS_1000: > + phydev->speed = SPEED_1000; > + break; > + > + case MII_M1011_PHY_STATUS_100: > + phydev->speed = SPEED_100; > + break; > + > + default: > + phydev->speed = SPEED_10; > + break; > + } > + > if (!fiber) { > err = genphy_read_lpa(phydev); > if (err < 0) > @@ -1291,28 +1313,6 @@ static int marvell_read_status_page_an(struct phy_device *phydev, > } > } > > - if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) > - return 0; > - > - if (status & MII_M1011_PHY_STATUS_FULLDUPLEX) > - phydev->duplex = DUPLEX_FULL; > - else > - phydev->duplex = DUPLEX_HALF; > - > - switch (status & MII_M1011_PHY_STATUS_SPD_MASK) { > - case MII_M1011_PHY_STATUS_1000: > - phydev->speed = SPEED_1000; > - break; > - > - case MII_M1011_PHY_STATUS_100: > - phydev->speed = SPEED_100; > - break; > - > - default: > - phydev->speed = SPEED_10; > - break; > - } > - > return 0; > } >
| |