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 <[email protected]>
---
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 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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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
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:51:37 +0100
Andrew Lunn <[email protected]> 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)
On Thu, 24 Jan 2019 16:07:41 +0000
Russell King - ARM Linux admin <[email protected]> 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)
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
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 <[email protected]>
> > > ---
> > > 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
On Fri, 25 Jan 2019 09:37:20 +0100
Antoine Tenart <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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)
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 <[email protected]>
> > > > ---
> > > > 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
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
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
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