Re: [PATCH] net: phy: marvell: Fix pause frame negotiation

[Russell King - ARM Linux admin]

On Sat, Apr 11, 2020 at 03:24:01PM +0200, Clemens Gruber wrote:
> 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@xxxxxxxxxxxxxxx # v5.6+
> > > 
> > > nit: please don't CC stable on networking patches
> > > 
> > > > Signed-off-by: Clemens Gruber <clemens.gruber@xxxxxxxxxxxx>
> > > > ---
> > > >  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. (?)

The fiber code is IMHO very suspect; the decoding of the pause status
seems to be completely broken. However, I'm not sure whether anyone
actually uses that or not, so I've been trying not to touch it.

> (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)

The resolved bit indicates whether the resolution data is valid, which
will be set when autoneg is complete or autoneg is disabled.  However,
the timing of the bit compared to the link status is not defined in the
datasheet - and that's the problem.  If the link status bits report that
the link is up but the resolved bit is indicating that the resolution
is not valid, what do we do?  Report potential garbage but link up to
the higher layers, or pretend that the link is down?

> 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?

I'm not sure what you're suggesting.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up