linux-kernel - Re: [PATCH net-next 1/2] net: phy: phy-c45: add SQI and SQI+ support for OATC14 10Base-T1S PHYs

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: <f6acd8db-4512-4f5d-a8cc-0cc522573db5@lunn.ch>
Date: Thu, 13 Nov 2025 17:15:23 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>
Cc: piergiorgio.beruto@...il.com, 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 1/2] net: phy: phy-c45: add SQI and SQI+ support
 for OATC14 10Base-T1S PHYs

> +/**
> + * genphy_c45_oatc14_get_sqi - Get Signal Quality Indicator (SQI) from an OATC14
> +			       10Base-T1S PHY
> + * @phydev: pointer to the PHY device structure
> + *
> + * Reads the SQI or SQI+ value from an OATC14-compatible 10Base-T1S PHY. If SQI+
> + * capability is supported, the function returns the extended SQI+ value;
> + * otherwise, it returns the basic SQI value.
> + *
> + * Return:
> + * * Positive SQI/SQI+ value on success
> + * * 0 if SQI update is not available

I thought 0 represented a very bad link? How is the call supposed to
know the difference between a bad link, and no new value, try again
later?

I had a very quick look at the standard. All that update seems to
indicate is that value has been updated since the last poll of that
bit. There is no indication you cannot read the 'old' SQI value if
there has not been an update. So i think you should always return an
SQI value, if it is if 'old'. 

> +	/* Read SQI capability */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_ADFCAP);
> +	if (ret < 0)
> +		return ret;

I wounder if this should be cached somewhere. You don't expect it to
change.

	Andrew