netdev - Re: Help on PHY not supporting autoneg

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y4fgT1kjX9LTULOi@gvm01>
Date:   Wed, 30 Nov 2022 23:59:27 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org
Subject: Re: Help on PHY not supporting autoneg

> > [   27.049388] socfpga-dwmac ff700000.ethernet eth0: configuring for phy/mii link mode
> > [   27.057155] ncn_resume  // I don't fully understand what happened since the link up, but it seems the MAC is doing more initialization
> 
> This looks odd. I would not expect a resume here. Add a WARN_ON(1) to
> get a stack trace and see if that helps explain why. My guess would
> be, you somehow end up in socfpga_dwmac_resume().
Ok, I can see the MAC driver gets there. I'll try to debug this later
(not my focus at the moment).

> > [   27.061251] ncn_handle_interrupt 8001
> > [   27.065100] link = 0, ret = 0809 // here I get a link down (???)
> > 
> > >From there on, if I read the LINK_CONTROL bit, someone set it to 0 (???)
> 
> You can add a WARN_ON(regnum==0) in phy_write() to get a stack trace.
Indeed, it was genphy_setup_forced setting this bit to 0.

> 
> > I've also tried a completely different approach, that is "emulating"
> > autoneg by implementing the config_aneg, aneg_done and read_status
> > callbacks. If I do this, then my driver works just fine and nobody seems
> > to be overwriting register 0.
> 
> That is not emulating. The config_aneg() just has a bad name.
> 
> If you have not provided a config_aneg() the default implementation is
> used, __genphy_config_aneg(), which will call genphy_setup_forced():
> 
> int genphy_setup_forced(struct phy_device *phydev)
> {
> 	u16 ctl;
> 
> 	phydev->pause = 0;
> 	phydev->asym_pause = 0;
> 
> 	ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
> 
> 	return phy_modify(phydev, MII_BMCR,
> 			  ~(BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN), ctl);
> 
Ok, there is where my problem was. If I supply a config_aneg()
implementation that sets the bits I want, then everything works just
fine, and ethtool seems to report the correct information.

[   17.061733] dwmac1000: Master AXI performs any burst length
[   17.061766] socfpga-dwmac ff700000.ethernet eth0: No Safety Features support found
[   17.061799] socfpga-dwmac ff700000.ethernet eth0: IEEE 1588-2008 Advanced Timestamp supported
[   17.062957] socfpga-dwmac ff700000.ethernet eth0: registered PTP clock
[   17.066366] socfpga-dwmac ff700000.ethernet eth0: configuring for phy/mii link mode
[   17.067170] socfpga-dwmac ff700000.ethernet eth0: No phy led trigger registered for speed(10)
[   17.067404] socfpga-dwmac ff700000.ethernet eth0: Link is Up - 10Mbps/Half - flow control off
/root #
/root # ethtool eth0
Settings for eth0:
        Supported ports: [ MII ]
        Supported link modes:   10baseT1S_P2MP/Half
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: No
        Supported FEC modes: Not reported
        Advertised link modes:  10baseT1S_P2MP/Half
        Advertised pause frame use: Symmetric Receive-only
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: 10Mb/s
        Duplex: Half
        Auto-negotiation: off
        Port: Twisted Pair
        PHYAD: 8
        Transceiver: external
        MDI-X: off (auto)
        Supports Wake-on: d
        Wake-on: d
        Current message level: 0x0000003f (63)
                               drv probe link timer ifdown ifup
        Link detected: yes


> What exactly is LINK_CONTROL. It is not one of the Linux names for a
> bit in BMCR.
The 802.3cg standard define link_control as a varibale set by autoneg.
In factm it is tied to the BMCR_ANENABLE bit. The standard further
specifies that when AN is not supported, this bit can be supplied in a
vendor-specific way. A common thing to do is to just leave it tied to
the BMCR_ANENABLE bit.

So, the "problem" seems to lie in the genphy_setup_forced() function.
More precisely, where you pointed me at: 
>       return phy_modify(phydev, MII_BMCR,
>                         ~(BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN), ctl);
> 

In my view we have two choices to handle LINK_CONTROL.

1. Just let the PHY driver override the config_aneg() callback as I did.
This may be a bit counter-intuitive because of the function name, but it
works.

2. in phylib, distinguish the case of a PHY having aneg disabled from a
PHY NOT supporting aneg. In the latter case, don't touch the control
register and/or provide a separate callback for "starting" the PHY
(e.g., set_link_ctrl)

Thoughts?

Thank you again for your kind answer!
Piergiorgio