linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net: mvpp2: fix condition for setting up link interrupt
@ 2019-01-24 13:18 Thomas Bogendoerfer
  2019-01-24 15:51 ` Andrew Lunn
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Bogendoerfer @ 2019-01-24 13:18 UTC (permalink / raw)
  To: David S. Miller, netdev, linux-kernel

Set up link interrupt if connection is handled by phylink otherwise
link state change detection for in-band-status doesn't work.

Fixes: 4bb043262878 ("net: mvpp2: phylink support")
Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
 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 16066c2d5b3a..0fbfe1945a69 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -3405,7 +3405,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 && port->phylink) {
 		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
 				  dev->name, port);
 		if (err) {
-- 
2.16.4


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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-24 13:18 [PATCH net] net: mvpp2: fix condition for setting up link interrupt Thomas Bogendoerfer
@ 2019-01-24 15:51 ` Andrew Lunn
  2019-01-24 16:07   ` Russell King - ARM Linux admin
  2019-01-24 16:15   ` Thomas Bogendoerfer
  0 siblings, 2 replies; 13+ messages in thread
From: Andrew Lunn @ 2019-01-24 15:51 UTC (permalink / raw)
  To: Thomas Bogendoerfer, antoine.tenart, maxime.chevallier
  Cc: David S. Miller, netdev, linux-kernel, Russell King

On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> Set up link interrupt if connection is handled by phylink otherwise
> link state change detection for in-band-status doesn't work.

Hi Thomas

Please ensure the maintainers of the driver are at least on Cc:

Thanks
	Andrew

> 
> Fixes: 4bb043262878 ("net: mvpp2: phylink support")
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
>  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 16066c2d5b3a..0fbfe1945a69 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -3405,7 +3405,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 && port->phylink) {
>  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
>  				  dev->name, port);
>  		if (err) {
> -- 
> 2.16.4
> 

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-24 15:51 ` Andrew Lunn
@ 2019-01-24 16:07   ` Russell King - ARM Linux admin
  2019-01-24 16:15     ` Andrew Lunn
                       ` (2 more replies)
  2019-01-24 16:15   ` Thomas Bogendoerfer
  1 sibling, 3 replies; 13+ messages in thread
From: Russell King - ARM Linux admin @ 2019-01-24 16:07 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Thomas Bogendoerfer, antoine.tenart, maxime.chevallier,
	David S. Miller, netdev, linux-kernel

On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
> On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > Set up link interrupt if connection is handled by phylink otherwise
> > link state change detection for in-band-status doesn't work.
> 
> Hi Thomas
> 
> Please ensure the maintainers of the driver are at least on Cc:

... and other interested parties!

> Thanks
> 	Andrew
> 
> > 
> > Fixes: 4bb043262878 ("net: mvpp2: phylink support")
> > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > ---
> >  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 16066c2d5b3a..0fbfe1945a69 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -3405,7 +3405,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 && port->phylink) {
> >  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
> >  				  dev->name, port);
> >  		if (err) {

This still looks fishy to me.  mvpp2_link_status_isr() has handling in
it that is safe to be called for non-phylink cases, so presumably the
right fix is to drop the "&& !port->phylink" completely?

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

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-24 15:51 ` Andrew Lunn
  2019-01-24 16:07   ` Russell King - ARM Linux admin
@ 2019-01-24 16:15   ` Thomas Bogendoerfer
  1 sibling, 0 replies; 13+ messages in thread
From: Thomas Bogendoerfer @ 2019-01-24 16:15 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: antoine.tenart, maxime.chevallier, David S. Miller, netdev,
	linux-kernel, Russell King

On Thu, 24 Jan 2019 16:51:37 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > Set up link interrupt if connection is handled by phylink otherwise
> > link state change detection for in-band-status doesn't work.
> 
> Hi Thomas
> 
> Please ensure the maintainers of the driver are at least on Cc:

I've hoped my script will get them, but it used --nogit-fallback, which prevented
finding maintainers... Sorry about that

Thomas.

-- 
SUSE Linux GmbH
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-24 16:07   ` Russell King - ARM Linux admin
@ 2019-01-24 16:15     ` Andrew Lunn
  2019-01-24 16:26       ` Russell King - ARM Linux admin
  2019-01-24 16:17     ` Thomas Bogendoerfer
  2019-01-25  8:37     ` Antoine Tenart
  2 siblings, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2019-01-24 16:15 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Thomas Bogendoerfer, antoine.tenart, maxime.chevallier,
	David S. Miller, netdev, linux-kernel

On Thu, Jan 24, 2019 at 04:07:41PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
> > On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > > Set up link interrupt if connection is handled by phylink otherwise
> > > link state change detection for in-band-status doesn't work.
> > 
> > Hi Thomas
> > 
> > Please ensure the maintainers of the driver are at least on Cc:
> 
> ... and other interested parties!

Hi Russell

get_maintainer.pl does not list you, so it can be hard to know who
those other interested parties are. I have local knowledge, so added
you :-)

      Andrew

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-24 16:07   ` Russell King - ARM Linux admin
  2019-01-24 16:15     ` Andrew Lunn
@ 2019-01-24 16:17     ` Thomas Bogendoerfer
  2019-01-25  8:37     ` Antoine Tenart
  2 siblings, 0 replies; 13+ messages in thread
From: Thomas Bogendoerfer @ 2019-01-24 16:17 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, antoine.tenart, maxime.chevallier, David S. Miller,
	netdev, linux-kernel

On Thu, 24 Jan 2019 16:07:41 +0000
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
> > On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > > Set up link interrupt if connection is handled by phylink otherwise
> > > link state change detection for in-band-status doesn't work.
> > 
> > Hi Thomas
> > 
> > Please ensure the maintainers of the driver are at least on Cc:
> 
> ... and other interested parties!

sorry about missing maintainers, but how could I find "other interested
parties" ? /me thought that's handled by sending to mailinglists, which I did...

Thomas.

-- 
SUSE Linux GmbH
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-24 16:15     ` Andrew Lunn
@ 2019-01-24 16:26       ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 13+ messages in thread
From: Russell King - ARM Linux admin @ 2019-01-24 16:26 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Thomas Bogendoerfer, antoine.tenart, maxime.chevallier,
	David S. Miller, netdev, linux-kernel

On Thu, Jan 24, 2019 at 05:15:35PM +0100, Andrew Lunn wrote:
> On Thu, Jan 24, 2019 at 04:07:41PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
> > > On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > > > Set up link interrupt if connection is handled by phylink otherwise
> > > > link state change detection for in-band-status doesn't work.
> > > 
> > > Hi Thomas
> > > 
> > > Please ensure the maintainers of the driver are at least on Cc:
> > 
> > ... and other interested parties!
> 
> Hi Russell
> 
> get_maintainer.pl does not list you, so it can be hard to know who
> those other interested parties are. I have local knowledge, so added
> you :-)

Well, I don't maintain the driver, so get_maintainer.pl wouldn't list
me in any case.

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

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-24 16:07   ` Russell King - ARM Linux admin
  2019-01-24 16:15     ` Andrew Lunn
  2019-01-24 16:17     ` Thomas Bogendoerfer
@ 2019-01-25  8:37     ` Antoine Tenart
  2019-01-25 15:18       ` Thomas Bogendoerfer
  2019-12-08 16:42       ` Russell King - ARM Linux admin
  2 siblings, 2 replies; 13+ messages in thread
From: Antoine Tenart @ 2019-01-25  8:37 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Thomas Bogendoerfer, antoine.tenart,
	maxime.chevallier, David S. Miller, netdev, linux-kernel

Hi,

On Thu, Jan 24, 2019 at 04:07:41PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
> > On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > > 
> > > Fixes: 4bb043262878 ("net: mvpp2: phylink support")
> > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > > ---
> > >  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 16066c2d5b3a..0fbfe1945a69 100644
> > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > @@ -3405,7 +3405,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 && port->phylink) {
> > >  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
> > >  				  dev->name, port);
> > >  		if (err) {
> 
> This still looks fishy to me.  mvpp2_link_status_isr() has handling in
> it that is safe to be called for non-phylink cases, so presumably the
> right fix is to drop the "&& !port->phylink" completely?

That's right, mvpp2_link_status_isr() is safe to be called with or
without phylink being used. This IRQ is reporting the link status, and
is used when using in-band status or when phylink isn't used.

We do have a similar fix locally, which looks like:

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 2bcbf9caaf0d..6bab1824a1e4 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -3406,7 +3406,8 @@ 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 &&
+           (!port->phylink || !port->has_phy)) {
                err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
                                  dev->name, port);
                if (err) {

We haven't submitted it yet, as we saw several issues when a port has no
PHY and is using the XLG MAC (so, not on the mcbin). We currently are
working on them.

I don't like to ask this, as you submitted the fix first, but I do think
we should hold back a bit while we figure out proper solutions. If you
need in-band status to be working properly, we could share out current
local branch so you can test and validate it do solve your issue.

Thanks!
Antoine

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

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-25  8:37     ` Antoine Tenart
@ 2019-01-25 15:18       ` Thomas Bogendoerfer
  2019-12-08 16:42       ` Russell King - ARM Linux admin
  1 sibling, 0 replies; 13+ messages in thread
From: Thomas Bogendoerfer @ 2019-01-25 15:18 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: Russell King - ARM Linux admin, Andrew Lunn, maxime.chevallier,
	David S. Miller, netdev, linux-kernel

On Fri, 25 Jan 2019 09:37:20 +0100
Antoine Tenart <antoine.tenart@bootlin.com> wrote:

> Hi,
> 
> On Thu, Jan 24, 2019 at 04:07:41PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
> > > On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > > > 
> > > > Fixes: 4bb043262878 ("net: mvpp2: phylink support")
> > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > > > ---
> > > >  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 16066c2d5b3a..0fbfe1945a69 100644
> > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > @@ -3405,7 +3405,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 && port->phylink) {
> > > >  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
> > > >  				  dev->name, port);
> > > >  		if (err) {
> > 
> > This still looks fishy to me.  mvpp2_link_status_isr() has handling in
> > it that is safe to be called for non-phylink cases, so presumably the
> > right fix is to drop the "&& !port->phylink" completely?
> 
> That's right, mvpp2_link_status_isr() is safe to be called with or
> without phylink being used. This IRQ is reporting the link status, and
> is used when using in-band status or when phylink isn't used.
> 
> We do have a similar fix locally, which looks like:
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 2bcbf9caaf0d..6bab1824a1e4 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -3406,7 +3406,8 @@ 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 &&
> +           (!port->phylink || !port->has_phy)) {
>                 err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
>                                   dev->name, port);
>                 if (err) {
> 
> We haven't submitted it yet, as we saw several issues when a port has no
> PHY and is using the XLG MAC (so, not on the mcbin). We currently are
> working on them.
> 
> I don't like to ask this, as you submitted the fix first, but I do think
> we should hold back a bit while we figure out proper solutions. If you
> need in-band status to be working properly, we could share out current
> local branch so you can test and validate it do solve your issue.

I also thought about it, and your patch makes more sense. It's more about
having no ethernet phy and therefore looking at the serdes for link status.
The system I'm running on has a few internal nodes behind a switch. The
switch propagates the external link state to the nodes via serdes lines.

I've tested your patch and it works in my setup.

Thomas.

-- 
SUSE Linux GmbH
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-01-25  8:37     ` Antoine Tenart
  2019-01-25 15:18       ` Thomas Bogendoerfer
@ 2019-12-08 16:42       ` Russell King - ARM Linux admin
  2019-12-08 17:14         ` Willy Tarreau
  2019-12-10 14:53         ` Antoine Tenart
  1 sibling, 2 replies; 13+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-08 16:42 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: Willy Tarreau, Andrew Lunn, Thomas Bogendoerfer,
	maxime.chevallier, David S. Miller, netdev, linux-kernel

On Fri, Jan 25, 2019 at 09:37:20AM +0100, Antoine Tenart wrote:
> Hi,
> 
> On Thu, Jan 24, 2019 at 04:07:41PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
> > > On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
> > > > 
> > > > Fixes: 4bb043262878 ("net: mvpp2: phylink support")
> > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > > > ---
> > > >  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 16066c2d5b3a..0fbfe1945a69 100644
> > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > @@ -3405,7 +3405,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 && port->phylink) {
> > > >  		err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
> > > >  				  dev->name, port);
> > > >  		if (err) {
> > 
> > This still looks fishy to me.  mvpp2_link_status_isr() has handling in
> > it that is safe to be called for non-phylink cases, so presumably the
> > right fix is to drop the "&& !port->phylink" completely?
> 
> That's right, mvpp2_link_status_isr() is safe to be called with or
> without phylink being used. This IRQ is reporting the link status, and
> is used when using in-band status or when phylink isn't used.
> 
> We do have a similar fix locally, which looks like:
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 2bcbf9caaf0d..6bab1824a1e4 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -3406,7 +3406,8 @@ 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 &&
> +           (!port->phylink || !port->has_phy)) {
>                 err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
>                                   dev->name, port);
>                 if (err) {
> 
> We haven't submitted it yet, as we saw several issues when a port has no
> PHY and is using the XLG MAC (so, not on the mcbin). We currently are
> working on them.
> 
> I don't like to ask this, as you submitted the fix first, but I do think
> we should hold back a bit while we figure out proper solutions. If you
> need in-band status to be working properly, we could share out current
> local branch so you can test and validate it do solve your issue.

Hi Antoine,

Today, I received an email from Willy Tarreau about this issue which
persists to this day with mainline kernels.

Willy reminded me that I've been carrying a fix for this, but because
of your concerns as stated above, I haven't bothered submitting it
through fear of causing regressions (which you seem to know about):

   http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=mvpp2&id=67ef3bff255b26cc0d6def8ca99c4e8ae9937727

Just like Thomas' case, the current code is broken for phylink when
in-band negotiation is being used - such as with the 1G/2.5G SFP
slot on the Macchiatobin.

It seems that resolving the issue has stalled.  Can I merge my patch,
or could you state exactly what the problems are with it so that
someone else can look into the issues please?

Thanks.

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

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-12-08 16:42       ` Russell King - ARM Linux admin
@ 2019-12-08 17:14         ` Willy Tarreau
  2019-12-10 14:53         ` Antoine Tenart
  1 sibling, 0 replies; 13+ messages in thread
From: Willy Tarreau @ 2019-12-08 17:14 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Antoine Tenart, Andrew Lunn, Thomas Bogendoerfer,
	maxime.chevallier, David S. Miller, netdev, linux-kernel

On Sun, Dec 08, 2019 at 04:42:36PM +0000, Russell King - ARM Linux admin wrote:
> Today, I received an email from Willy Tarreau about this issue which
> persists to this day with mainline kernels.
> 
> Willy reminded me that I've been carrying a fix for this, but because
> of your concerns as stated above, I haven't bothered submitting it
> through fear of causing regressions (which you seem to know about):
> 
>    http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=mvpp2&id=67ef3bff255b26cc0d6def8ca99c4e8ae9937727
> 
> Just like Thomas' case, the current code is broken for phylink when
> in-band negotiation is being used - such as with the 1G/2.5G SFP
> slot on the Macchiatobin.

Just to provide some context, I upgraded my mcbin-single-shot from 5.1.5
to 5.4.2 with everything working except that the SFP links would never get
up anymore (either in ethtool or ip link). I've been digging quite a bit,
looking at any DT change or out-of-tree patches and found the one above
with the exact description of my issue, which appeared to totally make
sense. Applying it alone fixed the issue, so unless there are more
serious downsides than unusable ethernet ports, it would be nice to have
it merged.

Thanks!
Willy

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-12-08 16:42       ` Russell King - ARM Linux admin
  2019-12-08 17:14         ` Willy Tarreau
@ 2019-12-10 14:53         ` Antoine Tenart
  2019-12-10 14:58           ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 13+ messages in thread
From: Antoine Tenart @ 2019-12-10 14:53 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Antoine Tenart, Willy Tarreau, Andrew Lunn, Thomas Bogendoerfer,
	maxime.chevallier, David S. Miller, netdev, linux-kernel

Hi Russell,

On Sun, Dec 08, 2019 at 04:42:36PM +0000, Russell King - ARM Linux admin wrote:
> 
> Today, I received an email from Willy Tarreau about this issue which
> persists to this day with mainline kernels.
> 
> Willy reminded me that I've been carrying a fix for this, but because
> of your concerns as stated above, I haven't bothered submitting it
> through fear of causing regressions (which you seem to know about):
> 
>    http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=mvpp2&id=67ef3bff255b26cc0d6def8ca99c4e8ae9937727
> 
> Just like Thomas' case, the current code is broken for phylink when
> in-band negotiation is being used - such as with the 1G/2.5G SFP
> slot on the Macchiatobin.
> 
> It seems that resolving the issue has stalled.  Can I merge my patch,
> or could you state exactly what the problems are with it so that
> someone else can look into the issues please?

Yes, please merge your patch (the one dropping the check on
'!port->phylink'), I've been using it for months. I answered that patch
submission back then[1] but it seems it was lost somehow :)

Thanks!
Antoine

[1] https://www.spinics.net/lists/netdev/msg555697.html

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

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

* Re: [PATCH net] net: mvpp2: fix condition for setting up link interrupt
  2019-12-10 14:53         ` Antoine Tenart
@ 2019-12-10 14:58           ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 13+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-10 14:58 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: Willy Tarreau, Andrew Lunn, Thomas Bogendoerfer,
	maxime.chevallier, David S. Miller, netdev, linux-kernel

On Tue, Dec 10, 2019 at 03:53:59PM +0100, Antoine Tenart wrote:
> Hi Russell,
> 
> On Sun, Dec 08, 2019 at 04:42:36PM +0000, Russell King - ARM Linux admin wrote:
> > 
> > Today, I received an email from Willy Tarreau about this issue which
> > persists to this day with mainline kernels.
> > 
> > Willy reminded me that I've been carrying a fix for this, but because
> > of your concerns as stated above, I haven't bothered submitting it
> > through fear of causing regressions (which you seem to know about):
> > 
> >    http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=mvpp2&id=67ef3bff255b26cc0d6def8ca99c4e8ae9937727
> > 
> > Just like Thomas' case, the current code is broken for phylink when
> > in-band negotiation is being used - such as with the 1G/2.5G SFP
> > slot on the Macchiatobin.
> > 
> > It seems that resolving the issue has stalled.  Can I merge my patch,
> > or could you state exactly what the problems are with it so that
> > someone else can look into the issues please?
> 
> Yes, please merge your patch (the one dropping the check on
> '!port->phylink'), I've been using it for months. I answered that patch
> submission back then[1] but it seems it was lost somehow :)
> 
> Thanks!
> Antoine
> 
> [1] https://www.spinics.net/lists/netdev/msg555697.html

Thanks.  Looks like I never read your reply (probably got buried.)

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

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

end of thread, other threads:[~2019-12-10 14:58 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-24 13:18 [PATCH net] net: mvpp2: fix condition for setting up link interrupt Thomas Bogendoerfer
2019-01-24 15:51 ` Andrew Lunn
2019-01-24 16:07   ` Russell King - ARM Linux admin
2019-01-24 16:15     ` Andrew Lunn
2019-01-24 16:26       ` Russell King - ARM Linux admin
2019-01-24 16:17     ` Thomas Bogendoerfer
2019-01-25  8:37     ` Antoine Tenart
2019-01-25 15:18       ` Thomas Bogendoerfer
2019-12-08 16:42       ` Russell King - ARM Linux admin
2019-12-08 17:14         ` Willy Tarreau
2019-12-10 14:53         ` Antoine Tenart
2019-12-10 14:58           ` Russell King - ARM Linux admin
2019-01-24 16:15   ` Thomas Bogendoerfer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).