netdev - Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180921081358.k3ubz73klvylc4yx@verge.net.au>
Date:   Fri, 21 Sep 2018 10:13:59 +0200
From:   Simon Horman <horms@...ge.net.au>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a
 supported link mode

On Thu, Sep 20, 2018 at 03:25:30PM +0200, Andrew Lunn wrote:
> > 1. net-next: cf7d97e1e54d ("net: mdio: remove duplicated include from mdio_bus.c")
> > 
> > # mii-tool -vv eth0
> > Using SIOCGMIIPHY=0x8947
> > eth0: no link
> >   registers for MII PHY 0: 
> >     1140 7949 0022 1622 0d81 c1e1 000f 0000
> >     0000 0300 0000 0000 0000 0000 0000 3000
> >     0000 0000 0000 0078 7002 0000 0000 0200
> >     0000 0000 0000 0528 0000 0000 0000 0000
> >   product info: vendor 00:08:85, model 34 rev 2
> >   basic mode:   autonegotiation enabled
> >   basic status: no link
> >   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> >   advertising:  100baseTx-FD 100baseTx-HD flow-control
> >   link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> > 
> > 2. net-next with this patch reverted
> > 
> > # mii-tool -vv eth0
> > Using SIOCGMIIPHY=0x8947
> > eth0: negotiated 100baseTx-FD, link ok
> >   registers for MII PHY 0: 
> >     1140 796d 0022 1622 0181 c1e1 000f 0000
> 
> Hi Simon
> 
> Word 5 is what we are advertising. Bits 10 and 11 are Pause and Asym
> Pause. In the good case here, neither are set. In this bad case above,
> both bits are set.
> 
> The patch i asked you to try only cleared the Pause bit, not the
> Asymmetric Pause bit. mii-tool only saying 'flow-control' did not
> help.
> 
> Word 6 is what the partner is advertising. c1e1 indicates the partner
> does not support flow control, both bits are 0. I don't see why this
> is preventing auto-net though. But in the bad case, the status
> register indicates auto-neg has not completed.
> 
> Anyway, please can you try this patch, which also removes Aysm Pause.

Thanks Andrew,

it seems that removing Aysm Pause does the trick.


* net-next [5678cb3c96ee ("net-next: mscc: remove unused ocelot_dev_gmii.h")]
+ Your patch to disable Pause and Asym_Pause (and 10baseT)
  => Success!

# ip link set up dev eth0; sleep 10; mii-tool -vv eth0
[   13.418522] Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=204)
[   16.399410] ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
Using SIOCGMIIPHY=0x8947
eth0: negotiated 100baseTx-FD, link ok
  registers for MII PHY 0: 
    1140 796d 0022 1622 0181 c1e1 000f 0000
    0000 0300 3800 0000 0000 0000 0000 3000
    0000 0000 0000 0c7e 54fe 0000 0000 0200
    0000 0000 0000 0500 0000 0000 0000 0000
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: autonegotiation complete, link ok
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD
  link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

* net-next [5678cb3c96ee ("net-next: mscc: remove unused ocelot_dev_gmii.h")]
+ Your patch modified to only disable Asym_Pause (and 10baseT)
  => Success!

# ip link set up dev eth0; sleep 10; mii-tool -vv eth0
[   86.414460] Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=204)
[   89.414651] ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
Using SIOCGMIIPHY=0x8947
eth0: negotiated 100baseTx-FD, link ok
  registers for MII PHY 0: 
    1140 796d 0022 1622 0581 c1e1 000f 0000
    0000 0300 3800 0000 0000 0000 0000 3000
    0000 0000 0000 087e 44fe 0000 0000 0200
    0000 0000 0000 0500 0000 0000 0000 0000
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: autonegotiation complete, link ok
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD flow-control
  link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

* net-next [5678cb3c96ee ("net-next: mscc: remove unused ocelot_dev_gmii.h")]
  + Your patch modified to only disable Pause (and 10baseT)
  => Fail (same test and result as yesterday)

# ip link set up dev eth0; sleep 10; mii-tool -vv eth0
[   52.518742] Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=204)
Using SIOCGMIIPHY=0x8947
eth0: no link
  registers for MII PHY 0: 
    1140 7949 0022 1622 0981 c1e1 000f 0000
    0000 0300 0000 0000 0000 0000 0000 3000
    0000 0000 0000 0878 7002 0000 0000 0200
    0000 0000 0000 0528 0000 0000 0000 0000
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: no link
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD
  link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

> 
> Thanks
> 	Andrew
> 
> >From 00a061304af51831ca1dc86bf6ce23d01f724229 Mon Sep 17 00:00:00 2001
> From: Andrew Lunn <andrew@...n.ch>
> Date: Tue, 18 Sep 2018 18:12:54 -0500
> Subject: [PATCH] ravb: Disable Pause Advertisement
> 
> The previous commit to ravb had the side effect of making the PHY
> advertise Pause. This previously did not happen, and it appears the
> MAC does not support Pause. By default, phydev->supported has Pause
> enabled, but phydev->advertising does not. Rather than rely on this,
> be explicit, and remove the Pause link mode.
> 
> Reported-by: Simon Horman <horms@...ge.net.au>
> Fixes: 41124fa64d4b ("net: ethernet: Add helper to remove a supported link mode")
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index fb2a1125780d..b0f2612ad226 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1073,9 +1073,11 @@ static int ravb_phy_init(struct net_device *ndev)
>  		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
>  	}
>  
> -	/* 10BASE is not supported */
> +	/* 10BASE, Pause and Asym Pause is not supported */
>  	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
>  	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
>  
>  	phy_attached_info(phydev);
>  
> -- 
> 2.19.0.rc1
>