[v1] net: phy: micrel: Fix the KSZ9131 MDI-X status issue

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

Posted by Raju Lakkaraju 1 year, 5 months ago

Access information about Auto mdix completion and pair selection from the
KSZ9131's Auto/MDI/MDI-X status register

Fixes: b64e6a8794d9 ("net: phy: micrel: Add PHY Auto/MDI/MDI-X set driver for KSZ9131")
Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.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;
 	} else {
+		ret = phy_read(phydev, MII_KSZ9131_DIG_AXAN_STS);
+		if (ret < 0)
+			return ret;
+
 		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
-	}
 
-	if (ret & MII_KSZ9131_AUTO_MDI_SET)
-		phydev->mdix = ETH_TP_MDI;
-	else
-		phydev->mdix = ETH_TP_MDI_X;
+		if (ret & MII_KSZ9131_DIG_AXAN_STS_LINK_DET) {
+			if (ret & MII_KSZ9131_DIG_AXAN_STS_A_SELECT)
+				phydev->mdix = ETH_TP_MDI;
+			else
+				phydev->mdix = ETH_TP_MDI_X;
+		} else {
+			phydev->mdix = ETH_TP_MDI_INVALID;
+		}
+	}
 
 	return 0;
 }
-- 
2.34.1

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

Posted by Andrew Lunn 1 year, 5 months ago

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@microchip.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

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

Posted by Raju Lakkaraju 1 year, 4 months ago

Hi Andrew,

Thank you for review the patch.

The 07/18/2024 17:04, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?
> 

Ok. I will add broken behaviour details.

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

Ok. I will add "Cc: stable tag"

> > Fixes: b64e6a8794d9 ("net: phy: micrel: Add PHY Auto/MDI/MDI-X set driver for KSZ9131")
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.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.
> 

I agree. I will fix it.

> 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.

Yes. I agree with you.
I will fix it in next version of patch.

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

>        Andrew

-- 
Thanks,                                                                         
Raju

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

Posted by Paolo Abeni 1 year, 5 months ago

On 7/12/24 13:16, Raju Lakkaraju wrote:
> Access information about Auto mdix completion and pair selection from the
> KSZ9131's Auto/MDI/MDI-X status register
> 
> Fixes: b64e6a8794d9 ("net: phy: micrel: Add PHY Auto/MDI/MDI-X set driver for KSZ9131")
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>

@Andrew, @Heiner: the patch LGTM, any feedback on your side?

thanks,

Paolo