netdev - Re: [PATCH net-next] net: phy: aquantia: Add mdix config and reporting

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: <ZxD69GqiPcqOZK2w@makrotopia.org>
Date: Thu, 17 Oct 2024 12:54:28 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: Paul Davey <paul.davey@...iedtelesis.co.nz>
Cc: Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: aquantia: Add mdix config and
 reporting

On Thu, Oct 17, 2024 at 02:54:07PM +1300, Paul Davey wrote:
> Add support for configuring MDI-X state of PHY.
> Add reporting of resolved MDI-X state in status information.

> [...]
> +static int aqr_set_polarity(struct phy_device *phydev, int polarity)

"polarity" is not the right term here. This is not about the polarity
of copper pairs, but rather about pairs being swapped.
Please name the function accordingly, eg. aqr_set_mdix().

> +{
> +	u16 val = 0;
> +
> +	switch (polarity) {
> +	case ETH_TP_MDI:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_MDI;
> +		break;
> +	case ETH_TP_MDI_X:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_MDIX;
> +		break;
> +	case ETH_TP_MDI_AUTO:
> +	case ETH_TP_MDI_INVALID:
> +	default:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_AUTO;
> +		break;
> +	}
> +
> +	return phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_RESVD_VEND_PROV,
> +				      MDIO_AN_RESVD_VEND_PROV_MDIX_MASK, val);
> +}
> +
>  static int aqr_config_aneg(struct phy_device *phydev)
>  {
>  	bool changed = false;
>  	u16 reg;
>  	int ret;
>  
> +	ret = aqr_set_polarity(phydev, phydev->mdix_ctrl);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
>  	if (phydev->autoneg == AUTONEG_DISABLE)
>  		return genphy_c45_pma_setup_forced(phydev);
>  
> @@ -278,6 +315,14 @@ static int aqr_read_status(struct phy_device *phydev)
>  				 val & MDIO_AN_RX_LP_STAT1_1000BASET_HALF);
>  	}
>  
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RESVD_VEND_STATUS1);

According to the datasheet the MDI/MDI-X indication should only be
interpreted when autonegotiation has completed.
Hence this call should be protected by genphy_c45_aneg_done(phydev) and
phydev->mdix set to ETH_TP_MDI_INVALID in case auto-negotiation hasn't
completed.

> +	if (val < 0)
> +		return val;
> +	if (val & MDIO_AN_RESVD_VEND_STATUS1_MDIX)
> +		phydev->mdix = ETH_TP_MDI_X;
> +	else
> +		phydev->mdix = ETH_TP_MDI;
> +
>  	return genphy_c45_read_status(phydev);
>  }
>  
> -- 
> 2.47.0
> 
>