netdev - Re: [PATCH net-next] net: phy: micrel: Add support for lan8842

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: <20250723090145.o2kq4vxcjrih54rt@DEN-DL-M31836.microchip.com>
Date: Wed, 23 Jul 2025 11:01:45 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
CC: <andrew@...n.ch>, <hkallweit1@...il.com>, <linux@...linux.org.uk>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: micrel: Add support for lan8842

The 07/23/2025 07:34, Oleksij Rempel wrote:
> 
> Hi Horatiu,

Hi Olekij,

> 
> On Mon, Jul 21, 2025 at 09:14:05AM +0200, Horatiu Vultur wrote:
> 
> > +static int lan8842_config_init(struct phy_device *phydev)
> > +{
> > +     int val;
> > +     int ret;
> > +
> > +     /* Reset the PHY */
> > +     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> 
> It would be good to use defines for MMD pages.

Those are extended pages and not MMD pages. Currently in the entire
source code I can see we used hardcoded values, also in the register
description it looks like all these extended pages do not have really
meaningfull names: Extended Page 0, Extended Page 4, Extended Page 5...

> 
> > +     if (val < 0)
> > +             return val;
> > +     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> > +     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
> 
> Please, do not ignore return values.

Good catch, I will fix that in the next version.
There are few others bellow, I will fix those also.

> 
> > +
> > +     /* Disable ANEG with QSGMII PCS Host side
> > +      * It has the same address as lan8814
> > +      */
> > +     val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
> > +     if (val < 0)
> > +             return val;
> > +     val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
> > +     ret = lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG,
> > +                                 val);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Disable also the SGMII_AUTO_ANEG_ENA, this will determine what is the
> > +      * PHY autoneg with the other end and then will update the host side
> > +      */
> > +     lanphy_write_page_reg(phydev, 4, LAN8842_SGMII_AUTO_ANEG_ENA, 0);
> > +
> > +     /* To allow the PHY to control the LEDs the GPIOs of the PHY should have
> > +      * a function mode and not the GPIO. Apparently by default the value is
> > +      * GPIO and not function even though the datasheet it says that it is
> > +      * function. Therefore set this value.
> > +      */
> > +     lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_EN2, 0);
> > +
> > +     /* Enable the Fast link failure, at the top level, at the bottom level
> > +      * it would be set/cleared inside lan8842_config_intr
> > +      */
> > +     val = lanphy_read_page_reg(phydev, 0, LAN8842_FLF);
> > +     if (val < 0)
> > +             return val;
> > +     val |= LAN8842_FLF_ENA | LAN8842_FLF_ENA_LINK_DOWN;
> 
> If I see it correctly, FLF support will make link fail after ~1ms, while
> IEEE 802.3 recommends 750ms. Since a link recovery of a PHY with autoneg
> support usually takes multiple seconds, I see the benefit for FLF
> support only mostly for SyncE environment at same time it seems to be
> a disadvantage for other environments.

Why would be a disadvantage?
> 
> I would prefer to have IEEE 802.3 recommended link behavior by default
> and have separate Netlink configuration interface for FLF.
> 
> Best Regards,
> Oleksij
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
/Horatiu