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

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

[Daniel González Cabanelas]

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 <andrew@lunn.ch>
Signed-off-by: Daniel González Cabanelas <dgcbueu@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 41d2a0eac..f9170bc93 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -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);
-- 
2.26.2

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[Andrew Lunn]

>  drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 41d2a0eac..f9170bc93 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -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

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[Russell King - ARM Linux admin]

On Thu, May 21, 2020 at 05:19:16PM +0200, Andrew Lunn wrote:
> >  drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index 41d2a0eac..f9170bc93 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -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.

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

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[Andrew Lunn]

> 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

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[Russell King - ARM Linux admin]

On Thu, May 21, 2020 at 05:55:13PM +0200, Andrew Lunn wrote:
> > 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.

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

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Thu, 21 May 2020 19:20:10 +0100

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

Done.

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[Daniel González Cabanelas]

Thanks for the comments.

I'll look for a better approach.

Regards
Daniel

El jue., 21 may. 2020 a las 17:27, Russell King - ARM Linux admin
(<linux@armlinux.org.uk>) escribió:
>
> On Thu, May 21, 2020 at 05:19:16PM +0200, Andrew Lunn wrote:
> > >  drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > > index 41d2a0eac..f9170bc93 100644
> > > --- a/drivers/net/ethernet/marvell/mvneta.c
> > > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > > @@ -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.
>
> 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

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[Russell King - ARM Linux admin]

On Thu, May 21, 2020 at 05:55:19PM +0200, Daniel González Cabanelas wrote:
> 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

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

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[Daniel González Cabanelas]

Hi Russell,

El vie., 5 jun. 2020 a las 11:49, Russell King - ARM Linux admin
(<linux@armlinux.org.uk>) escribió:
>
> On Thu, May 21, 2020 at 05:55:19PM +0200, Daniel González Cabanelas wrote:
> > 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
>

Thank you.

Regards
Daniel.

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

Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid

[Russell King - ARM Linux admin]

Hi Daniel,

On Sun, Jun 07, 2020 at 02:25:11AM +0200, Daniel González Cabanelas wrote:
> Hi Russell,
> 
> El vie., 5 jun. 2020 a las 11:49, Russell King - ARM Linux admin
> (<linux@armlinux.org.uk>) escribió:
> >
> > On Thu, May 21, 2020 at 05:55:19PM +0200, Daniel González Cabanelas wrote:
> > > 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
> >
> 
> 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.

-- 
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: mvneta: only do WoL speed down if the PHY is valid

[Florian Fainelli]

On 5/21/2020 8:26 AM, Russell King - ARM Linux admin wrote:
> On Thu, May 21, 2020 at 05:19:16PM +0200, Andrew Lunn wrote:
>>>  drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>>> index 41d2a0eac..f9170bc93 100644
>>> --- a/drivers/net/ethernet/marvell/mvneta.c
>>> +++ b/drivers/net/ethernet/marvell/mvneta.c
>>> @@ -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.
> 
> 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