Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net: mvneta: only do WoL speed down if the PHY is valid
@ 2020-05-21 14:19 Daniel González Cabanelas
  2020-05-21 15:19 ` Andrew Lunn
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel González Cabanelas @ 2020-05-21 14:19 UTC (permalink / raw)
  To: netdev; +Cc: davem, thomas.petazzoni, andrew

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





^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid
  2020-05-21 14:19 [PATCH] net: mvneta: only do WoL speed down if the PHY is valid Daniel González Cabanelas
@ 2020-05-21 15:19 ` Andrew Lunn
  2020-05-21 15:26   ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2020-05-21 15:19 UTC (permalink / raw)
  To: Daniel González Cabanelas
  Cc: netdev, davem, thomas.petazzoni, Russell King

>  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid
  2020-05-21 15:19 ` Andrew Lunn
@ 2020-05-21 15:26   ` Russell King - ARM Linux admin
  2020-05-21 15:55     ` Andrew Lunn
                       ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Russell King - ARM Linux admin @ 2020-05-21 15:26 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Daniel González Cabanelas, netdev, davem, thomas.petazzoni

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid
  2020-05-21 15:26   ` Russell King - ARM Linux admin
@ 2020-05-21 15:55     ` Andrew Lunn
  2020-05-21 18:20       ` Russell King - ARM Linux admin
  2020-05-21 15:55     ` Daniel González Cabanelas
  2020-05-21 15:55     ` Florian Fainelli
  2 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2020-05-21 15:55 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Daniel González Cabanelas, netdev, davem, thomas.petazzoni

> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid
  2020-05-21 15:26   ` Russell King - ARM Linux admin
  2020-05-21 15:55     ` Andrew Lunn
@ 2020-05-21 15:55     ` Daniel González Cabanelas
  2020-05-21 15:55     ` Florian Fainelli
  2 siblings, 0 replies; 8+ messages in thread
From: Daniel González Cabanelas @ 2020-05-21 15:55 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, netdev, davem, thomas.petazzoni

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid
  2020-05-21 15:26   ` Russell King - ARM Linux admin
  2020-05-21 15:55     ` Andrew Lunn
  2020-05-21 15:55     ` Daniel González Cabanelas
@ 2020-05-21 15:55     ` Florian Fainelli
  2 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2020-05-21 15:55 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn
  Cc: Daniel González Cabanelas, netdev, davem, thomas.petazzoni



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid
  2020-05-21 15:55     ` Andrew Lunn
@ 2020-05-21 18:20       ` Russell King - ARM Linux admin
  2020-05-22 23:10         ` David Miller
  0 siblings, 1 reply; 8+ messages in thread
From: Russell King - ARM Linux admin @ 2020-05-21 18:20 UTC (permalink / raw)
  To: Andrew Lunn, davem
  Cc: Daniel González Cabanelas, netdev, thomas.petazzoni

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: mvneta: only do WoL speed down if the PHY is valid
  2020-05-21 18:20       ` Russell King - ARM Linux admin
@ 2020-05-22 23:10         ` David Miller
  0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2020-05-22 23:10 UTC (permalink / raw)
  To: linux; +Cc: andrew, dgcbueu, netdev, thomas.petazzoni

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-21 14:19 [PATCH] net: mvneta: only do WoL speed down if the PHY is valid Daniel González Cabanelas
2020-05-21 15:19 ` Andrew Lunn
2020-05-21 15:26   ` Russell King - ARM Linux admin
2020-05-21 15:55     ` Andrew Lunn
2020-05-21 18:20       ` Russell King - ARM Linux admin
2020-05-22 23:10         ` David Miller
2020-05-21 15:55     ` Daniel González Cabanelas
2020-05-21 15:55     ` Florian Fainelli

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git