| Message ID | 20190124131803.14038-1-tbogendoerfer@suse.de |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: mvpp2: fix condition for setting up link interrupt | expand |
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 >
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?
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.
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
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.
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.
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
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.
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.
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
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
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.)
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) {
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(-)