[PATCH] net: marvell: mvpp2: phylink requires the link interrupt

[PATCH] net: marvell: mvpp2: phylink requires the link interrupt

[Russell King]

phylink requires the MAC to report when its link status changes when
operating in inband modes.  Failure to report link status changes
means that phylink has no idea when the link events happen, which
results in either the network interface's carrier remaining up or
remaining permanently down.

For example, with a fiber module, if the interface is brought up and
link is initially established, taking the link down at the far end
will cut the optical power.  The SFP module's LOS asserts, we
deactivate the link, and the network interface reports no carrier.

When the far end is brought back up, the SFP module's LOS deasserts,
but the MAC may be slower to establish link.  If this happens (which
in my tests is a certainty) then phylink never hears that the MAC
has established link with the far end, and the network interface is
stuck reporting no carrier.  This means the interface is
non-functional.

Avoiding the link interrupt when we have phylink is basically not
an option, so remove the !port->phylink from the test.

Tested-by: Sven Auhagen <sven.auhagen@voleatech.de>
Tested-by: Antoine Tenart <antoine.tenart@bootlin.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 111b3b8239e1..ef44c6979a31 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -3674,7 +3674,7 @@ static int mvpp2_open(struct net_device *dev)
 		valid = true;
 	}
 
-	if (priv->hw_version == MVPP22 && port->link_irq && !port->phylink) {
+	if (priv->hw_version == MVPP22 && port->link_irq) {
 		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
 				  dev->name, port);
 		if (err) {
-- 
2.20.1

Re: [PATCH] net: marvell: mvpp2: phylink requires the link interrupt

[Jakub Kicinski]

On Tue, 10 Dec 2019 22:33:05 +0000, Russell King wrote:
> phylink requires the MAC to report when its link status changes when
> operating in inband modes.  Failure to report link status changes
> means that phylink has no idea when the link events happen, which
> results in either the network interface's carrier remaining up or
> remaining permanently down.
> 
> For example, with a fiber module, if the interface is brought up and
> link is initially established, taking the link down at the far end
> will cut the optical power.  The SFP module's LOS asserts, we
> deactivate the link, and the network interface reports no carrier.
> 
> When the far end is brought back up, the SFP module's LOS deasserts,
> but the MAC may be slower to establish link.  If this happens (which
> in my tests is a certainty) then phylink never hears that the MAC
> has established link with the far end, and the network interface is
> stuck reporting no carrier.  This means the interface is
> non-functional.
> 
> Avoiding the link interrupt when we have phylink is basically not
> an option, so remove the !port->phylink from the test.
> 
> Tested-by: Sven Auhagen <sven.auhagen@voleatech.de>
> Tested-by: Antoine Tenart <antoine.tenart@bootlin.com>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Fixes: 4bb043262878 ("net: mvpp2: phylink support") ?

Seems like you maybe didn't want this backported to stable hence 
no fixes tag?

Please advise :)

> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 111b3b8239e1..ef44c6979a31 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -3674,7 +3674,7 @@ static int mvpp2_open(struct net_device *dev)
>  		valid = true;
>  	}
>  
> -	if (priv->hw_version == MVPP22 && port->link_irq && !port->phylink) {
> +	if (priv->hw_version == MVPP22 && port->link_irq) {
>  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
>  				  dev->name, port);
>  		if (err) {

Re: [PATCH] net: marvell: mvpp2: phylink requires the link interrupt

[Willy Tarreau]

On Fri, Dec 13, 2019 at 04:34:03PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2019 22:33:05 +0000, Russell King wrote:
> > phylink requires the MAC to report when its link status changes when
> > operating in inband modes.  Failure to report link status changes
> > means that phylink has no idea when the link events happen, which
> > results in either the network interface's carrier remaining up or
> > remaining permanently down.
> > 
> > For example, with a fiber module, if the interface is brought up and
> > link is initially established, taking the link down at the far end
> > will cut the optical power.  The SFP module's LOS asserts, we
> > deactivate the link, and the network interface reports no carrier.
> > 
> > When the far end is brought back up, the SFP module's LOS deasserts,
> > but the MAC may be slower to establish link.  If this happens (which
> > in my tests is a certainty) then phylink never hears that the MAC
> > has established link with the far end, and the network interface is
> > stuck reporting no carrier.  This means the interface is
> > non-functional.
> > 
> > Avoiding the link interrupt when we have phylink is basically not
> > an option, so remove the !port->phylink from the test.
> > 
> > Tested-by: Sven Auhagen <sven.auhagen@voleatech.de>
> > Tested-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Fixes: 4bb043262878 ("net: mvpp2: phylink support") ?
> 
> Seems like you maybe didn't want this backported to stable hence 
> no fixes tag?
> 
> Please advise :)

Speaking for myself, I'd very much like to get it into stable in order
to avoid having to keep this local patch on top of latest stable!

Willy

Re: [PATCH] net: marvell: mvpp2: phylink requires the link interrupt

[Russell King - ARM Linux admin]

On Fri, Dec 13, 2019 at 04:34:03PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2019 22:33:05 +0000, Russell King wrote:
> > phylink requires the MAC to report when its link status changes when
> > operating in inband modes.  Failure to report link status changes
> > means that phylink has no idea when the link events happen, which
> > results in either the network interface's carrier remaining up or
> > remaining permanently down.
> > 
> > For example, with a fiber module, if the interface is brought up and
> > link is initially established, taking the link down at the far end
> > will cut the optical power.  The SFP module's LOS asserts, we
> > deactivate the link, and the network interface reports no carrier.
> > 
> > When the far end is brought back up, the SFP module's LOS deasserts,
> > but the MAC may be slower to establish link.  If this happens (which
> > in my tests is a certainty) then phylink never hears that the MAC
> > has established link with the far end, and the network interface is
> > stuck reporting no carrier.  This means the interface is
> > non-functional.
> > 
> > Avoiding the link interrupt when we have phylink is basically not
> > an option, so remove the !port->phylink from the test.
> > 
> > Tested-by: Sven Auhagen <sven.auhagen@voleatech.de>
> > Tested-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Fixes: 4bb043262878 ("net: mvpp2: phylink support") ?
> 
> Seems like you maybe didn't want this backported to stable hence 
> no fixes tag?

Correct, because backporting just this patch will break the
Macchiatobin.

This patch is dependent on the previous two patches, which are more
about correct use of the API.  I suspect if you try to backport the
series, things will get very hairly very quickly.

> 
> Please advise :)
> 
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > index 111b3b8239e1..ef44c6979a31 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -3674,7 +3674,7 @@ static int mvpp2_open(struct net_device *dev)
> >  		valid = true;
> >  	}
> >  
> > -	if (priv->hw_version == MVPP22 && port->link_irq && !port->phylink) {
> > +	if (priv->hw_version == MVPP22 && port->link_irq) {
> >  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
> >  				  dev->name, port);
> >  		if (err) {
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: marvell: mvpp2: phylink requires the link interrupt

[Russell King - ARM Linux admin]

On Sat, Dec 14, 2019 at 07:51:27AM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Dec 13, 2019 at 04:34:03PM -0800, Jakub Kicinski wrote:
> > On Tue, 10 Dec 2019 22:33:05 +0000, Russell King wrote:
> > > phylink requires the MAC to report when its link status changes when
> > > operating in inband modes.  Failure to report link status changes
> > > means that phylink has no idea when the link events happen, which
> > > results in either the network interface's carrier remaining up or
> > > remaining permanently down.
> > > 
> > > For example, with a fiber module, if the interface is brought up and
> > > link is initially established, taking the link down at the far end
> > > will cut the optical power.  The SFP module's LOS asserts, we
> > > deactivate the link, and the network interface reports no carrier.
> > > 
> > > When the far end is brought back up, the SFP module's LOS deasserts,
> > > but the MAC may be slower to establish link.  If this happens (which
> > > in my tests is a certainty) then phylink never hears that the MAC
> > > has established link with the far end, and the network interface is
> > > stuck reporting no carrier.  This means the interface is
> > > non-functional.
> > > 
> > > Avoiding the link interrupt when we have phylink is basically not
> > > an option, so remove the !port->phylink from the test.
> > > 
> > > Tested-by: Sven Auhagen <sven.auhagen@voleatech.de>
> > > Tested-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > Fixes: 4bb043262878 ("net: mvpp2: phylink support") ?
> > 
> > Seems like you maybe didn't want this backported to stable hence 
> > no fixes tag?
> 
> Correct, because backporting just this patch will break the
> Macchiatobin.
> 
> This patch is dependent on the previous two patches, which are more
> about correct use of the API.  I suspect if you try to backport the
> series, things will get very hairly very quickly.

Oh, sorry, too early, wrong patch.  Yes, please add the fixes tag.

> 
> > 
> > Please advise :)
> > 
> > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > index 111b3b8239e1..ef44c6979a31 100644
> > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > @@ -3674,7 +3674,7 @@ static int mvpp2_open(struct net_device *dev)
> > >  		valid = true;
> > >  	}
> > >  
> > > -	if (priv->hw_version == MVPP22 && port->link_irq && !port->phylink) {
> > > +	if (priv->hw_version == MVPP22 && port->link_irq) {
> > >  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
> > >  				  dev->name, port);
> > >  		if (err) {
> > 
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: marvell: mvpp2: phylink requires the link interrupt

[Willy Tarreau]

On Sat, Dec 14, 2019 at 07:56:02AM +0000, Russell King - ARM Linux admin wrote:
> On Sat, Dec 14, 2019 at 07:51:27AM +0000, Russell King - ARM Linux admin wrote:
> > On Fri, Dec 13, 2019 at 04:34:03PM -0800, Jakub Kicinski wrote:
> > > On Tue, 10 Dec 2019 22:33:05 +0000, Russell King wrote:
> > > > phylink requires the MAC to report when its link status changes when
> > > > operating in inband modes.  Failure to report link status changes
> > > > means that phylink has no idea when the link events happen, which
> > > > results in either the network interface's carrier remaining up or
> > > > remaining permanently down.
> > > > 
> > > > For example, with a fiber module, if the interface is brought up and
> > > > link is initially established, taking the link down at the far end
> > > > will cut the optical power.  The SFP module's LOS asserts, we
> > > > deactivate the link, and the network interface reports no carrier.
> > > > 
> > > > When the far end is brought back up, the SFP module's LOS deasserts,
> > > > but the MAC may be slower to establish link.  If this happens (which
> > > > in my tests is a certainty) then phylink never hears that the MAC
> > > > has established link with the far end, and the network interface is
> > > > stuck reporting no carrier.  This means the interface is
> > > > non-functional.
> > > > 
> > > > Avoiding the link interrupt when we have phylink is basically not
> > > > an option, so remove the !port->phylink from the test.
> > > > 
> > > > Tested-by: Sven Auhagen <sven.auhagen@voleatech.de>
> > > > Tested-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > 
> > > Fixes: 4bb043262878 ("net: mvpp2: phylink support") ?
> > > 
> > > Seems like you maybe didn't want this backported to stable hence 
> > > no fixes tag?
> > 
> > Correct, because backporting just this patch will break the
> > Macchiatobin.
> > 
> > This patch is dependent on the previous two patches, which are more
> > about correct use of the API.  I suspect if you try to backport the
> > series, things will get very hairly very quickly.
> 
> Oh, sorry, too early, wrong patch.  Yes, please add the fixes tag.

I prefer :-)  Because indeed I only have this one on top of 5.4.2 which
solved the problem. You can even add my tested-by if you want (though I
don't care).

Thanks,
Willy

Re: [PATCH] net: marvell: mvpp2: phylink requires the link interrupt

[Jakub Kicinski]

On Sat, 14 Dec 2019 09:24:03 +0100, Willy Tarreau wrote:
> > > > Fixes: 4bb043262878 ("net: mvpp2: phylink support") ?
> > Oh, sorry, too early, wrong patch.  Yes, please add the fixes tag.  
> 
> I prefer :-)  Because indeed I only have this one on top of 5.4.2 which
> solved the problem. You can even add my tested-by if you want (though I
> don't care).

OK, applied, and queued for stable, thanks!