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: <Y4gOG5rFwlezsfoD@lunn.ch>
Date:   Thu, 1 Dec 2022 03:14:51 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Piergiorgio Beruto <piergiorgio.beruto@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: Help on PHY not supporting autoneg

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

That looks odd. The PHY should indicate if it supports pause
negotiation. Since this PHY does not support autoneg, it should not be
saying it can negotiate pause. So i'm wondering why it is saying this
here. Same for 'Supported pause'.

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

Given that this is a T1 PHY does MDI-X have any meaning? The (auto)
indicates the PHY is returning mdix_ctrl=ETH_TP_MDI_AUTO, when it
should be returning ETH_TP_MDI_INVALID to indicate it is not
supported.

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

2) sounds wrong. As you said, it is vendor-specific. As such you
cannot trust it to mean anything. The standard has left it undefined,
so you cannot do anything in generic code. I would also worry about
breaking older PHYs who have never had this bit set.

So i would go with 1). As i said, the function name is not ideal, but
it has been like this since forever.

   Andrew