linux-kernel - Re: [PATCH net-next v0] net: phy: aquantia: poll status register

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: <5f4a8026-0057-48dd-b51e-6888d79c3d76@lunn.ch>
Date: Tue, 8 Oct 2024 23:40:40 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Aryan Srivastava <aryan.srivastava@...iedtelesis.co.nz>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v0] net: phy: aquantia: poll status register

On Mon, Oct 07, 2024 at 10:35:36AM +1300, Aryan Srivastava wrote:
> The system interface connection status register is not immediately
> correct upon line side link up. This results in the status being read as
> OFF and then transitioning to the correct host side link mode with a
> short delay. This results in the phylink framework passing the OFF
> status down to all MAC config drivers, resulting in the host side link
> being misconfigured, which in turn can lead to link flapping or complete
> packet loss in some cases.
> 
> Mitigate this by periodically polling the register until it not showing
> the OFF state. This will be done every 1ms for 10ms, using the same
> poll/timeout as the processor intensive operation reads.

Does the datasheet say anything about when MDIO_PHYXS_VEND_IF_STATUS
is valid?

>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_XAUI	4
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII	6
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI	7
> +#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OFF	9
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII	10
>  
>  #define MDIO_AN_VEND_PROV			0xc400
> @@ -342,9 +343,18 @@ static int aqr107_read_status(struct phy_device *phydev)
>  	if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
>  		return 0;
>  
> -	val = phy_read_mmd(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS);
> -	if (val < 0)
> -		return val;
> +	/**
> +	 * The status register is not immediately correct on line side link up.
> +	 * Poll periodically until it reflects the correct ON state.
> +	 */
> +	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
> +					MDIO_PHYXS_VEND_IF_STATUS, val,
> +					(FIELD_GET(MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK, val) !=
> +					MDIO_PHYXS_VEND_IF_STATUS_TYPE_OFF),
> +					AQR107_OP_IN_PROG_SLEEP,
> +					AQR107_OP_IN_PROG_TIMEOUT, false);
> +	if (ret)
> +		return ret;

I don't know if returning ETIMEDOUT is the correct thing to do
here. It might be better to set phydev->link to false, since there is
no end to end link yet.

	Andrew