linux-kernel - Re: [PATCH net] net: phy: micrel: Fix the KSZ9131 MDI-X status issue

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: <fe873fde-7a41-4a4a-ba9f-41c2ba0ddc02@lunn.ch>
Date: Thu, 18 Jul 2024 17:04:02 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
	hkallweit1@...il.com, linux@...linux.org.uk, edumazet@...gle.com,
	pabeni@...hat.com, horatiu.vultur@...rochip.com,
	linux-kernel@...r.kernel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net] net: phy: micrel: Fix the KSZ9131 MDI-X status issue

On Fri, Jul 12, 2024 at 04:46:48PM +0530, Raju Lakkaraju wrote:
> Access information about Auto mdix completion and pair selection from the
> KSZ9131's Auto/MDI/MDI-X status register

Please explain what the broken behaviour is. How would i know i need
this patch?

You have not included a Cc: stable tag. Does that mean this does not
bother anybody and so does not need backporting?

> Fixes: b64e6a8794d9 ("net: phy: micrel: Add PHY Auto/MDI/MDI-X set driver for KSZ9131")
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
> ---
>  drivers/net/phy/micrel.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index ebafedde0ab7..fddc1b91ba7f 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -1438,6 +1438,9 @@ static int ksz9131_config_init(struct phy_device *phydev)
>  #define MII_KSZ9131_AUTO_MDIX		0x1C
>  #define MII_KSZ9131_AUTO_MDI_SET	BIT(7)
>  #define MII_KSZ9131_AUTO_MDIX_SWAP_OFF	BIT(6)
> +#define MII_KSZ9131_DIG_AXAN_STS	0x14
> +#define MII_KSZ9131_DIG_AXAN_STS_LINK_DET	BIT(14)
> +#define MII_KSZ9131_DIG_AXAN_STS_A_SELECT	BIT(12)
>  
>  static int ksz9131_mdix_update(struct phy_device *phydev)
>  {
> @@ -1452,14 +1455,24 @@ static int ksz9131_mdix_update(struct phy_device *phydev)
>  			phydev->mdix_ctrl = ETH_TP_MDI;
>  		else
>  			phydev->mdix_ctrl = ETH_TP_MDI_X;
> +
> +		phydev->mdix = phydev->mdix_ctrl;

This seems a bit odd. phydev->mdix_ctrl is what the user wants to
happen. This is generally ETH_TP_MDI_AUTO, meaning the PHY should
figure it out. It can be ETH_TP_MDI_X, or ETH_TP_MDI which forces the
configuration. phydev->mdix is what it has ended up using.

So the code above first seems to change what the user asked for. This
is likely to replace ETH_TP_MDI_AUTO with one of the fixed modes,
which will then break when the user replaces a crossed cable with a
straight cable, and the forced mode is then wrong.

Setting mdix to mdix_ctrl then seems wrong. In most cases, you are
going to get ETH_TP_MDI_AUTO, when in fact you should be returning
what the PHY has decided on, ETH_TP_MDI_X, ETH_TP_MDI, or
ETH_TP_MDI_INVALID because the link is down.

Maybe genphy_c45_read_mdix() will help you. It simply reads a PHY
status register, sets phydev->mdix and it is done.

       Andrew