[PATCH net] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[PATCH net] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[Jonathan McDowell]

A typo in qca808x_read_status means we try to set SMII mode on the port
rather than SGMII when the link speed is not 2.5Gb/s. This results in no
traffic due to the mismatch in configuration between the phy and the
mac.

Fixes: 79c7bc0521545 ("net: phy: add qca8081 read_status")
Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/phy/at803x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 5b6c0d120e09..7077e3a92d31 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1691,7 +1691,7 @@ static int qca808x_read_status(struct phy_device *phydev)
 	if (phydev->link && phydev->speed == SPEED_2500)
 		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
 	else
-		phydev->interface = PHY_INTERFACE_MODE_SMII;
+		phydev->interface = PHY_INTERFACE_MODE_SGMII;
 
 	/* generate seed as a lower random value to make PHY linked as SLAVE easily,
 	 * except for master/slave configuration fault detected.
-- 
2.30.2

Re: [PATCH net] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[Russell King (Oracle)]

On Sun, Jan 30, 2022 at 10:25:28AM +0000, Jonathan McDowell wrote:
> A typo in qca808x_read_status means we try to set SMII mode on the port
> rather than SGMII when the link speed is not 2.5Gb/s. This results in no
> traffic due to the mismatch in configuration between the phy and the
> mac.
> 
> Fixes: 79c7bc0521545 ("net: phy: add qca8081 read_status")
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/net/phy/at803x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 5b6c0d120e09..7077e3a92d31 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -1691,7 +1691,7 @@ static int qca808x_read_status(struct phy_device *phydev)
>  	if (phydev->link && phydev->speed == SPEED_2500)
>  		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
>  	else
> -		phydev->interface = PHY_INTERFACE_MODE_SMII;
> +		phydev->interface = PHY_INTERFACE_MODE_SGMII;

Is it intentional to set the interface to SGMII also when there is no
link?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[Jonathan McDowell]

On Sun, Jan 30, 2022 at 12:40:57PM +0000, Russell King (Oracle) wrote:
> On Sun, Jan 30, 2022 at 10:25:28AM +0000, Jonathan McDowell wrote:
> > A typo in qca808x_read_status means we try to set SMII mode on the port
> > rather than SGMII when the link speed is not 2.5Gb/s. This results in no
> > traffic due to the mismatch in configuration between the phy and the
> > mac.
> > 
> > Fixes: 79c7bc0521545 ("net: phy: add qca8081 read_status")
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> >  drivers/net/phy/at803x.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > index 5b6c0d120e09..7077e3a92d31 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -1691,7 +1691,7 @@ static int qca808x_read_status(struct phy_device *phydev)
> >  	if (phydev->link && phydev->speed == SPEED_2500)
> >  		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
> >  	else
> > -		phydev->interface = PHY_INTERFACE_MODE_SMII;
> > +		phydev->interface = PHY_INTERFACE_MODE_SGMII;
> 
> Is it intentional to set the interface to SGMII also when there is no
> link?

My reading of the code is that if this was just a GigE capable phy the
interface would be set once and never changed/unset. The only reason
it happens here is because the link changes to support the 2.5G mode, so
there's no problem with it defaulting to SGMII even when the external
link isn't actually up. Perhap Luo can confirm if this is the case?

J.

-- 
Web [  101 things you can't have too much of : 30 - Comfy sofas.   ]
site: https:// [                                          ]      Made by
www.earth.li/~noodles/  [                      ]         HuggieTag 0.0.24

Re: [PATCH net] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[Russell King (Oracle)]

On Sun, Jan 30, 2022 at 03:18:10PM +0000, Jonathan McDowell wrote:
> On Sun, Jan 30, 2022 at 12:40:57PM +0000, Russell King (Oracle) wrote:
> > On Sun, Jan 30, 2022 at 10:25:28AM +0000, Jonathan McDowell wrote:
> > > A typo in qca808x_read_status means we try to set SMII mode on the port
> > > rather than SGMII when the link speed is not 2.5Gb/s. This results in no
> > > traffic due to the mismatch in configuration between the phy and the
> > > mac.
> > > 
> > > Fixes: 79c7bc0521545 ("net: phy: add qca8081 read_status")
> > > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > > ---
> > >  drivers/net/phy/at803x.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > > index 5b6c0d120e09..7077e3a92d31 100644
> > > --- a/drivers/net/phy/at803x.c
> > > +++ b/drivers/net/phy/at803x.c
> > > @@ -1691,7 +1691,7 @@ static int qca808x_read_status(struct phy_device *phydev)
> > >  	if (phydev->link && phydev->speed == SPEED_2500)
> > >  		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
> > >  	else
> > > -		phydev->interface = PHY_INTERFACE_MODE_SMII;
> > > +		phydev->interface = PHY_INTERFACE_MODE_SGMII;
> > 
> > Is it intentional to set the interface to SGMII also when there is no
> > link?
> 
> My reading of the code is that if this was just a GigE capable phy the
> interface would be set once and never changed/unset. The only reason
> it happens here is because the link changes to support the 2.5G mode, so
> there's no problem with it defaulting to SGMII even when the external
> link isn't actually up. Perhap Luo can confirm if this is the case?

My point is that other PHY drivers only change the interface mode when
the link comes up, and we ought to have consistency between PHY drivers
rather than each PHY driver deciding on different behaviours - unless
there is a good reason to be different.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[Jonathan McDowell]

On Sun, Jan 30, 2022 at 06:09:37PM +0000, Russell King (Oracle) wrote:
> On Sun, Jan 30, 2022 at 03:18:10PM +0000, Jonathan McDowell wrote:
> > On Sun, Jan 30, 2022 at 12:40:57PM +0000, Russell King (Oracle) wrote:
> > > On Sun, Jan 30, 2022 at 10:25:28AM +0000, Jonathan McDowell wrote:
> > > > A typo in qca808x_read_status means we try to set SMII mode on the port
> > > > rather than SGMII when the link speed is not 2.5Gb/s. This results in no
> > > > traffic due to the mismatch in configuration between the phy and the
> > > > mac.
> > > > 
> > > > Fixes: 79c7bc0521545 ("net: phy: add qca8081 read_status")
> > > > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > > > ---
> > > >  drivers/net/phy/at803x.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > > > index 5b6c0d120e09..7077e3a92d31 100644
> > > > --- a/drivers/net/phy/at803x.c
> > > > +++ b/drivers/net/phy/at803x.c
> > > > @@ -1691,7 +1691,7 @@ static int qca808x_read_status(struct phy_device *phydev)
> > > >  	if (phydev->link && phydev->speed == SPEED_2500)
> > > >  		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
> > > >  	else
> > > > -		phydev->interface = PHY_INTERFACE_MODE_SMII;
> > > > +		phydev->interface = PHY_INTERFACE_MODE_SGMII;
> > > 
> > > Is it intentional to set the interface to SGMII also when there is no
> > > link?
> > 
> > My reading of the code is that if this was just a GigE capable phy the
> > interface would be set once and never changed/unset. The only reason
> > it happens here is because the link changes to support the 2.5G mode, so
> > there's no problem with it defaulting to SGMII even when the external
> > link isn't actually up. Perhap Luo can confirm if this is the case?
> 
> My point is that other PHY drivers only change the interface mode when
> the link comes up, and we ought to have consistency between PHY drivers
> rather than each PHY driver deciding on different behaviours - unless
> there is a good reason to be different.

I'm not aware of any reason to do so. I've rolled a v2 that only changes
the interface mode when the link is up and it behaves as expected in my
testing, so I'll get that sent out later.

J.

-- 
/-\                             | 101 things you can't have too much
|@/  Debian GNU/Linux Developer |           of : 52 - IRC.
\-                              |

[PATCH net v2] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[Jonathan McDowell]

A typo in qca808x_read_status means we try to set SMII mode on the port
rather than SGMII when the link speed is not 2.5Gb/s. This results in no
traffic due to the mismatch in configuration between the phy and the
mac.

v2:
 Only change interface mode when the link is up

Fixes: 79c7bc0521545 ("net: phy: add qca8081 read_status")
Cc: stable@vger.kernel.org
Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/phy/at803x.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 5b6c0d120e09..29aa811af430 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1688,19 +1688,19 @@ static int qca808x_read_status(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	if (phydev->link && phydev->speed == SPEED_2500)
-		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
-	else
-		phydev->interface = PHY_INTERFACE_MODE_SMII;
-
-	/* generate seed as a lower random value to make PHY linked as SLAVE easily,
-	 * except for master/slave configuration fault detected.
-	 * the reason for not putting this code into the function link_change_notify is
-	 * the corner case where the link partner is also the qca8081 PHY and the seed
-	 * value is configured as the same value, the link can't be up and no link change
-	 * occurs.
-	 */
-	if (!phydev->link) {
+	if (phydev->link) {
+		if (phydev->speed == SPEED_2500)
+			phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+		else
+			phydev->interface = PHY_INTERFACE_MODE_SGMII;
+	} else {
+		/* generate seed as a lower random value to make PHY linked as SLAVE easily,
+		 * except for master/slave configuration fault detected.
+		 * the reason for not putting this code into the function link_change_notify is
+		 * the corner case where the link partner is also the qca8081 PHY and the seed
+		 * value is configured as the same value, the link can't be up and no link change
+		 * occurs.
+		 */
 		if (phydev->master_slave_state == MASTER_SLAVE_STATE_ERR) {
 			qca808x_phy_ms_seed_enable(phydev, false);
 		} else {
-- 
2.30.2

Re: [PATCH net v2] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[Russell King (Oracle)]

On Mon, Jan 31, 2022 at 01:56:41PM +0000, Jonathan McDowell wrote:
> A typo in qca808x_read_status means we try to set SMII mode on the port
> rather than SGMII when the link speed is not 2.5Gb/s. This results in no
> traffic due to the mismatch in configuration between the phy and the
> mac.
> 
> v2:
>  Only change interface mode when the link is up
> 
> Fixes: 79c7bc0521545 ("net: phy: add qca8081 read_status")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jonathan McDowell <noodles@earth.li>

Thanks!

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 31 Jan 2022 13:56:41 +0000 you wrote:
> A typo in qca808x_read_status means we try to set SMII mode on the port
> rather than SGMII when the link speed is not 2.5Gb/s. This results in no
> traffic due to the mismatch in configuration between the phy and the
> mac.
> 
> v2:
>  Only change interface mode when the link is up
> 
> [...]

Here is the summary with links:
  - [net,v2] net: phy: Fix qca8081 with speeds lower than 2.5Gb/s
    https://git.kernel.org/netdev/net/c/881cc731df6a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html