| Message ID | 20240712111648.282897-1-Raju.Lakkaraju@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phy: micrel: Fix the KSZ9131 MDI-X status issue | expand |
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
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
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
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; }
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(-)