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: <aIB0VYLqcBKVtAmU@pengutronix.de>
Date: Wed, 23 Jul 2025 07:34:13 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Horatiu Vultur <horatiu.vultur@...rochip.com>
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

Hi Horatiu,

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.

> +	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.

> +
> +	/* 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.

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 |