netdev - Re: [PATCH net-next v2 1/3] net: phy: bcm54811: Fix the PHY initialization

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] [day] [month] [year] [list]

Message-ID: <fda1e6aa-3b92-423d-b98b-e4b8a39e5d15@axis.com>
Date: Tue, 24 Jun 2025 13:43:16 +0200
From: Kamil Horák (2N) <kamilh@...s.com>
To: netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/3] net: phy: bcm54811: Fix the PHY
 initialization



On 6/23/25 17:56, Russell King (Oracle) wrote:
 > On Mon, Jun 23, 2025 at 05:10:47PM +0200, Kamil Horák - 2N wrote:
 >>       /* With BCM54811, BroadR-Reach implies no autoneg */
 >> -    if (priv->brr_mode)
 >> +    if (priv->brr_mode) {
 >>           phydev->autoneg = 0;
 >
 > This, to me, looks extremely buggy. Setting phydev->autoneg to zero does
 > not prevent userspace enabling autoneg later. It also doesn't report to
 > userspace that autoneg is disabled. Not your problem, but a latent bug
 > in this driver.
So I'll try to do this cleanly. Hope it is possible somehow, because of 
the bcm54811 and bcm54810 properties: the bcm54810 is fully featured PHY 
version which includes auto-negotiation of its own (called LDS or Long 
Distance Signalling) by Broadcom. The bcm54811 is limited, mainly in 
absence of LDS - but the controlling bit is set upon reset, exactly as 
in the bcm54810. The bcm54811 datasheet states "Reserved, reset value 1, 
must be written to zero after every device reset" without describing the 
purpose of the bit. The bcm54810 datasheet describes the bit as "LDS 
Enable" = enable auto-negotiation in BroadR-Reach mode. In other words, 
the LDS control bit exists on the bcm54811 and must be zeroed in order 
to be able to use the PHY in forced speed / master / slave mode. Strange 
enough. Still we need to disable any auto-negotioation if there is 
bcm54811 in BroadR-Reach link mode. For bcm54811 in IEEE mode (using 
"normal" wiring for 1000/100/10MBit, standard IEEE PHY handling would be 
used, including the possibility of half/full duplex.


 >
 >> +        /* Disable Long Distance Signaling, the BRR mode autoneg */
 >> +        err = phy_modify(phydev, MII_BCM54XX_LRECR, LRECR_LDSEN, 0);
 >> +        if (err < 0)
 >> +            return err;
 >> +    }
 >>   -    return bcm5481x_set_brrmode(phydev, priv->brr_mode);
 >> +    if (!phy_interface_is_rgmii(phydev) ||
 >> +        phydev->interface == PHY_INTERFACE_MODE_MII) {
 >
 > Not sure this condition actually reflects what you're trying to
 > achieve, because if we're using PHY_INTERFACE_MODE_MII, then
 > !phy_interface_is_rgmii(phydev) will be true because phydev->interface
 > isn't one of the RGMII modes. So, I think this can be reduced to simply
 >
 >     if (!phy_interface_is_rgmii(phydev)) {
 >
 >> +        /* Misc Control: GMII/MII Mode (not RGMII) */
 >> +        err = bcm54xx_auxctl_write(phydev,
 >> +                       MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
 >> +                       MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN |
 >> +                       MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RSVD
 >> +        );
 >
 > I don't think this addition is described in the commit message.
 >
This will change with introducing of new interface mode for MII-Lite as 
recommended in other answers to this group of patches.


Kamil