From: Thomas Bogendoerfer <hidden> Date: 2019-01-24 13:18:15
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 <redacted>
---
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -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
From: Thomas Bogendoerfer <hidden> Date: 2019-01-24 16:15:12
On Thu, 24 Jan 2019 16:51:37 +0100
Andrew Lunn [off-list ref] wrote:
On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
quoted
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)
From: Andrew Lunn <andrew@lunn.ch> Date: 2019-01-24 16:15:42
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:
quoted
On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
quoted
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
From: Thomas Bogendoerfer <hidden> Date: 2019-01-24 16:17:40
On Thu, 24 Jan 2019 16:07:41 +0000
Russell King - ARM Linux admin [off-list ref] wrote:
On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
quoted
On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
quoted
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)
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2019-01-24 16:26:59
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:
quoted
On Thu, Jan 24, 2019 at 04:51:37PM +0100, Andrew Lunn wrote:
quoted
On Thu, Jan 24, 2019 at 02:18:03PM +0100, Thomas Bogendoerfer wrote:
quoted
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
@@ -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:
@@ -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
@@ -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:
@@ -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)
@@ -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:
@@ -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
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
From: Antoine Tenart <hidden> Date: 2019-12-10 14:54:04
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
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2019-12-10 14:58:20
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:
quoted
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