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

[PATCH RFC] 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>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
This is the final patch that I mentioned in the cover letter
   "[PATCH net-next 0/5] mvpp2 phylink fixes"

 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 93fed4080dbf..0b164b424dca 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -3412,7 +3412,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.7.4

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

[Antoine Tenart]

Hi Russell,

I've been using this patch on my local branch for the past 3 weeks, and
I saw no obvious issue so far. (I also used a similar patch before).

Do you have pending questions regarding this patch? Otherwise I think
you can sent it again without the RFC tag, with my Tested-by and/or
Acked-by tags if you want.

Thanks!
Antoine

On Fri, Feb 08, 2019 at 03:39:10PM +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>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> This is the final patch that I mentioned in the cover letter
>    "[PATCH net-next 0/5] mvpp2 phylink fixes"
> 
>  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 93fed4080dbf..0b164b424dca 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -3412,7 +3412,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.7.4
> 

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com