net: mvneta: only do WoL speed down if the PHY is valid

PATCH In review Daniel González Cabanelas 5 participants 11 messages May 21, 2020 – Jun 24, 2020

Subsystems (3)

Networking Network drivers Marvell mvneta Ethernet driver

lore

Share & tools

lore

Daniel González Cabanelas

author

2020-05-21 14:20 UTC #0 lore raw

PATCH 1/1 net: mvneta: only do WoL speed down if the PHY is valid

Previous patch:
"net: mvneta: speed down the PHY, if WoL used, to save energy"

was causing a NULL pointer dereference when ethernet switches are connected to mvneta, because they aren't handled directly as PHYs.

Fix it by restricting the mentioned patch for the PHY detected cases.

Reported-by Andrew Lunn <[email protected]>

Signed-off-by Daniel González Cabanelas <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +4 -3

index 41d2a0eac..f9170bc93 100644
@@ -3567,8 +3567,9 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	phylink_start(pp->phylink);
 
-	/* We may have called phy_speed_down before */
-	phy_speed_up(pp->dev->phydev);
+	if(pp->dev->phydev)
+		/* We may have called phy_speed_down before */
+		phy_speed_up(pp->dev->phydev);
 
 	netif_tx_start_all_queues(pp->dev);
 }

@@ -3577,7 +3578,7 @@ static void mvneta_stop_dev(struct mvneta_port *pp)
 {
 	unsigned int cpu;
 
-	if (device_may_wakeup(&pp->dev->dev))
+	if (pp->dev->phydev && device_may_wakeup(&pp->dev->dev))
 		phy_speed_down(pp->dev->phydev, false);
 
 	phylink_stop(pp->phylink);

Signature · 1 line

2.26.2

Andrew Lunn

2020-05-21 15:19 UTC #1 lore raw

Quoted 8 lines

 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +0 -0

index 41d2a0eac..f9170bc93 100644

drivers/net/ethernet/marvell/mvneta.c static void mvneta_start_dev(struct mvneta_port *pp) · line 3572

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c +3 -2

@@ -3567,8 +3567,9 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	phylink_start(pp->phylink);
 
-	/* We may have called phy_speed_down before */
-	phy_speed_up(pp->dev->phydev);
+	if(pp->dev->phydev)
+		/* We may have called phy_speed_down before */
+		phy_speed_up(pp->dev->phydev);

I don't think it is as simple as this. You should not really be mixing phy_ and phylink_ calls within one driver. You might of noticed there are no other phy_ calls in this driver. So ideally you want to add phylink_ calls which do the right thing.

Andrew

Russell King - ARM Linux admin

2020-05-21 15:27 UTC #2 lore raw

Quoted 11 lines · Andrew Lunn

Quoted 11 lines

 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +0 -0

index 41d2a0eac..f9170bc93 100644
@@ -3567,8 +3567,9 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	phylink_start(pp->phylink);

Quoted 10 lines

drivers/net/ethernet/marvell/mvneta.c static void mvneta_start_dev(struct mvneta_port *pp) · line 3572
Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +3 -2
-	/* We may have called phy_speed_down before */
-	phy_speed_up(pp->dev->phydev);
+	if(pp->dev->phydev)
+		/* We may have called phy_speed_down before */
+		phy_speed_up(pp->dev->phydev);
I don't think it is as simple as this. You should not really be mixing phy_ and phylink_ calls within one driver. You might of noticed there are no other phy_ calls in this driver. So ideally you want to add phylink_ calls which do the right thing.

And... what is mvneta doing getting the phydev? I removed all that code when converting it to phylink, since the idea with phylink is that the PHY is the responsibility of phylink not the network driver.

I hope the patch adding pp->dev->phydev hasn't been merged as it's almost certainly wrong.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Andrew Lunn

2020-05-21 15:55 UTC #3 lore raw

Quoted 2 lines

I hope the patch adding pp->dev->phydev hasn't been merged as it's almost certainly wrong.

Hi Russell

It was merged :-(

And it Oops when used with a switch.

Andrew

Russell King - ARM Linux admin

2020-05-21 18:20 UTC #4 lore raw

Quoted 8 lines · Andrew Lunn

Quoted 2 lines
I hope the patch adding pp->dev->phydev hasn't been merged as it's almost certainly wrong.
Hi Russell
It was merged :-(
And it Oops when used with a switch.

Hmm, now that I have net-next updated, I think the original commit is wrong but not as I thought.

The way this has been added, it means that if we have a PHY on a SFP, we can end up changing the settings on the SFP PHY if there is one present. Do we want to support WoL on SFPs?

David, can you revert 5e3768a436bb70c9c3e27aaba6b73f8ef8f5dcf3 please? It's a layering violation, and as Andrew has found, it causes kernel oopses.

What we need instead is support in phylink for doing this, which isn't going to be a couple of lines change to what was added to mvneta in the referenced commit.

Thanks.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

David Miller

maintainer

2020-05-22 23:10 UTC #5 lore raw

From: Russell King - ARM Linux admin <[email protected]>
Date: Thu, 21 May 2020 19:20:10 +0100

Quoted 3 lines

David, can you revert 5e3768a436bb70c9c3e27aaba6b73f8ef8f5dcf3 please? It's a layering violation, and as Andrew has found, it causes kernel oopses.

Done.

Florian Fainelli

2020-05-21 15:55 UTC #6 lore raw

In reply to Russell King - ARM Linux admin

Quoted 23 lines · Russell King - ARM Linux admin

Quoted 11 lines · Andrew Lunn
Quoted 11 lines
 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +0 -0
index 41d2a0eac..f9170bc93 100644
@@ -3567,8 +3567,9 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	phylink_start(pp->phylink);
 
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c static void mvneta_start_dev(struct mvneta_port *pp) · line 3572
Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +3 -2
-	/* We may have called phy_speed_down before */
-	phy_speed_up(pp->dev->phydev);
+	if(pp->dev->phydev)
+		/* We may have called phy_speed_down before */
+		phy_speed_up(pp->dev->phydev);
I don't think it is as simple as this. You should not really be mixing phy_ and phylink_ calls within one driver. You might of noticed there are no other phy_ calls in this driver. So ideally you want to add phylink_ calls which do the right thing.

Quoted 6 lines

And... what is mvneta doing getting the phydev? I removed all that code when converting it to phylink, since the idea with phylink is that the PHY is the responsibility of phylink not the network driver.
I hope the patch adding pp->dev->phydev hasn't been merged as it's almost certainly wrong.

This is already merged, this is a follow from a bisection that Andrew run.

There should be a phylink_phy_speed_{up,down} to maintain the PHYLINK abstraction, and the functionality is useful beyond PHYLIB. --
Florian

Daniel González Cabanelas

author

2020-05-21 15:55 UTC #7 lore raw

In reply to Russell King - ARM Linux admin

Thanks for the comments.

I'll look for a better approach.

Regards
Daniel

Quoted 34 lines · Russell King - ARM Linux admin

Quoted 11 lines · Andrew Lunn
Quoted 11 lines
 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +0 -0
index 41d2a0eac..f9170bc93 100644
@@ -3567,8 +3567,9 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
    phylink_start(pp->phylink);
 
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c static void mvneta_start_dev(struct mvneta_port *pp) · line 3572
Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +3 -2
-   /* We may have called phy_speed_down before */
-   phy_speed_up(pp->dev->phydev);
+   if(pp->dev->phydev)
+           /* We may have called phy_speed_down before */
+           phy_speed_up(pp->dev->phydev);
I don't think it is as simple as this. You should not really be mixing phy_ and phylink_ calls within one driver. You might of noticed there are no other phy_ calls in this driver. So ideally you want to add phylink_ calls which do the right thing.
And... what is mvneta doing getting the phydev? I removed all that code when converting it to phylink, since the idea with phylink is that the PHY is the responsibility of phylink not the network driver.
I hope the patch adding pp->dev->phydev hasn't been merged as it's almost certainly wrong.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Russell King - ARM Linux admin

2020-06-05 09:49 UTC #8 lore raw

Quoted 3 lines · Daniel González Cabanelas

Thanks for the comments.
I'll look for a better approach.

Hi Daniel,

I've just pushed out phylink a patch adding this functionality. I'm intending to submit it when net-next re-opens. See:

http://git.armlinux.org.uk/cgit/linux-arm.git/patch/?id=58c81223e17e39433895cfaf3dbf401134334779

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Daniel González Cabanelas

author

2020-06-07 00:25 UTC #9 lore raw

Hi Russell,

Quoted 7 lines · Russell King - ARM Linux admin

Quoted 3 lines · Daniel González Cabanelas
Thanks for the comments.
I'll look for a better approach.

Quoted 7 lines

Hi Daniel,
I've just pushed out phylink a patch adding this functionality. I'm intending to submit it when net-next re-opens. See:
http://git.armlinux.org.uk/cgit/linux-arm.git/patch/?id=58c81223e17e39433895cfaf3dbf401134334779

Thank you.

Regards
Daniel.

Quoted 3 lines

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Russell King - ARM Linux admin

2020-06-24 09:29 UTC #10 lore raw

Hi Daniel,

Quoted 13 lines · Daniel González Cabanelas

Hi Russell,
Quoted 9 lines · Russell King - ARM Linux admin
Quoted 3 lines · Daniel González Cabanelas
Thanks for the comments.
I'll look for a better approach.
Hi Daniel,

Quoted 7 lines

Quoted 5 lines
I've just pushed out phylink a patch adding this functionality. I'm intending to submit it when net-next re-opens. See:
http://git.armlinux.org.uk/cgit/linux-arm.git/patch/?id=58c81223e17e39433895cfaf3dbf401134334779
Thank you.

Are you preparing a patch for mvneta, or are you expecting me to do that? I'm not able to test WoL on any of my mvneta platforms.

Thanks.

Signature · 2 lines

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