linux-kernel - Re: [PATCH net-next v2 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] [day] [month] [year] [list]

Message-ID: <aSGioGmSP9rtSLpB@horms.kernel.org>
Date: Sat, 22 Nov 2025 11:46:40 +0000
From: Simon Horman <horms@...nel.org>
To: Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>
Cc: piergiorgio.beruto@...il.com, 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 v2 1/2] net: phy: phy-c45: add SQI and SQI+
 support for OATC14 10Base-T1S PHYs

On Tue, Nov 18, 2025 at 10:29:45AM +0530, Parthiban Veerasooran wrote:

...

> @@ -1695,3 +1695,89 @@ int genphy_c45_oatc14_cable_test_start(struct phy_device *phydev)
>  				OATC14_HDD_START_CONTROL);
>  }
>  EXPORT_SYMBOL(genphy_c45_oatc14_cable_test_start);
> +
> +/**
> + * genphy_c45_oatc14_get_sqi_max - Get maximum supported SQI or SQI+ level of
> + *				   OATC14 10Base-T1S PHY
> + * @phydev: pointer to the PHY device structure
> + *
> + * This function reads the advanced capability register to determine the maximum
> + * supported Signal Quality Indicator (SQI) or SQI+ level
> + *
> + * Return:
> + * * Maximum SQI/SQI+ level (≥0)
> + * * -EOPNOTSUPP if not supported
> + * * Negative errno on read failure
> + */
> +int genphy_c45_oatc14_get_sqi_max(struct phy_device *phydev)
> +{
> +	u8 bits;
> +	int ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_ADFCAP);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Check for SQI+ capability
> +	 * 0 - SQI+ is not supported
> +	 * (3-8) bits for (8-256) SQI+ levels supported
> +	 */
> +	bits = FIELD_GET(OATC14_ADFCAP_SQIPLUS_CAPABILITY, ret);
> +	if (bits) {
> +		phydev->oatc14_sqiplus_bits = bits;
> +		/* Max sqi+ level supported: (2 ^ bits) - 1 */
> +		return BIT(bits) - 1;
> +	}
> +
> +	/* Check for SQI capability
> +	 * 0 - SQI is not supported
> +	 * 1 - SQI is supported (0-7 levels)
> +	 */
> +	if (ret & OATC14_ADFCAP_SQI_CAPABILITY)
> +		return OATC14_SQI_MAX_LEVEL;
> +
> +	return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL(genphy_c45_oatc14_get_sqi_max);
> +
> +/**
> + * genphy_c45_oatc14_get_sqi - Get Signal Quality Indicator (SQI) from an OATC14
> + *			       10Base-T1S PHY
> + * @phydev: pointer to the PHY device structure
> + *
> + * This function 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:
> + * * SQI/SQI+ value on success
> + * * Negative errno on read failure
> + */
> +int genphy_c45_oatc14_get_sqi(struct phy_device *phydev)
> +{
> +	u8 shift;
> +	int ret;
> +
> +	/* Calculate and return SQI+ value if supported */
> +	if (phydev->oatc14_sqiplus_bits) {

Hi Parthiban,

AFAICT oatc14_sqiplus_bits will always be 0 until
genphy_c45_oatc14_get_sqi_max() is called, after which
it may be some other value.

But according to the flow of linkstate_prepare_data()
it seems that genphy_c45_oatc14_get_sqi_max()
will be called after genphy_c45_oatc14_get_sqi().

In which case the condition above will be false
(unless genphy_c45_oatc14_get_sqi_max was somehow already
called by some other means.)

This doesn't seem to be in line with the intention of this code.

Flagged by Claude Code with https://github.com/masoncl/review-prompts/

> +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> +				   MDIO_OATC14_DCQ_SQIPLUS);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* SQI+ uses N MSBs out of 8 bits, left-aligned with padding 1's
> +		 * Calculate the right-shift needed to isolate the N bits.
> +		 */
> +		shift = 8 - phydev->oatc14_sqiplus_bits;
> +
> +		return (ret & OATC14_DCQ_SQIPLUS_VALUE) >> shift;
> +	}
> +
> +	/* Read and return SQI value if SQI+ capability is not supported */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_DCQ_SQI);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret & OATC14_DCQ_SQI_VALUE;
> +}
> +EXPORT_SYMBOL(genphy_c45_oatc14_get_sqi);

...