Messages in this thread | | | Date | Sat, 11 Apr 2020 15:24:01 +0200 | From | Clemens Gruber <> | Subject | Re: [PATCH] net: phy: marvell: Fix pause frame negotiation |
| |
On Sat, Apr 11, 2020 at 10:17:05AM +0100, Russell King - ARM Linux admin wrote: > 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.
I assumed that in the fiber case, the link status register won't be 1 until autonegotiation is complete. There is a part in the 88E1510 datasheet on page 57 [2.6.2], which says so but it's in the Fiber/Copper Auto-Selection chapter and I am not sure if that's true in general. (?) (For copper, we call genphy_update_link, which sets phydev->link to 0 if autoneg is enabled && !completed. And according to the datasheet, the resolved bit is set when autonegotiation is completed || disabled)
TL/DR: It's probably a good idea to force link to 0 to be sure, as you suggested below. I will send a v2 with that change.
Moving the extraction of info to a helper is probably better left to a separate patch?
> 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.
Thanks, Clemens
| |