lkml.org 
[lkml]   [2020]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] net: phy: fix auto-negotiation in case of 'down-shift'
    From
    Date
    On Tue, 2020-11-24 at 14:56 +0000, Russell King - ARM Linux admin wrote:
    > On Tue, Nov 24, 2020 at 03:38:48PM +0100, Antonio Borneo wrote:
    > > If the auto-negotiation fails to establish a gigabit link, the phy
    > > can try to 'down-shift': it resets the bits in MII_CTRL1000 to
    > > stop advertising 1Gbps and retries the negotiation at 100Mbps.
    > >
    > > From commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode
    > > in genphy_read_status") the content of MII_CTRL1000 is not checked
    > > anymore at the end of the negotiation, preventing the detection of
    > > phy 'down-shift'.
    > > In case of 'down-shift' phydev->advertising gets out-of-sync wrt
    > > MII_CTRL1000 and still includes modes that the phy have already
    > > dropped. The link partner could still advertise higher speeds,
    > > while the link is established at one of the common lower speeds.
    > > The logic 'and' in phy_resolve_aneg_linkmode() between
    > > phydev->advertising and phydev->lp_advertising will report an
    > > incorrect mode.
    > >
    > > Issue detected with a local phy rtl8211f connected with a gigabit
    > > capable router through a two-pairs network cable.
    > >
    > > After auto-negotiation, read back MII_CTRL1000 and mask-out from
    > > phydev->advertising the modes that have been eventually discarded
    > > due to the 'down-shift'.
    >
    > Sorry, but no. While your solution will appear to work, in
    > introduces unexpected changes to the user visible APIs.
    >
    > >   if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
    > > + if (phydev->is_gigabit_capable) {
    > > + adv = phy_read(phydev, MII_CTRL1000);
    > > + if (adv < 0)
    > > + return adv;
    > > + /* update advertising in case of 'down-shift' */
    > > + mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
    > > + adv);
    >
    > If a down-shift occurs, this will cause the configured advertising
    > mask to lose the 1G speed, which will be visible to userspace.

    You are right, it gets propagated to user that 1Gbps is not advertised

    > Userspace doesn't expect the advertising mask to change beneath it.
    > Since updates from userspace are done using a read-modify-write of
    > the ksettings, this can have the undesired effect of removing 1G
    > from the configured advertising mask.
    >
    > We've had other PHYs have this behaviour; the correct solution is for
    > the PHY driver to implement reading the resolution from the PHY rather
    > than relying on the generic implementation if it can down-shift

    If it's already upstream, could you please point to one of the phy driver
    that already implements this properly?

    Thanks
    Antonio

    \
     
     \ /
      Last update: 2020-11-24 16:23    [W:4.718 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site