Re: [EXT] Re: Phylink flow control support on ports with MLO_AN_FIXED auto negotiation

On Sun, Jan 31, 2021 at 11:12:14AM +0000, Russell King - ARM Linux admin wrote:

I discussed it with Andrew earlier last year, and his response was:

 DT configuration of pause for fixed link probably is sufficient. I don't
 remember it ever been really discussed for DSA. It was a Melanox
 discussion about limiting pause for the CPU. So I think it is safe to
 not implement ethtool -A, at least until somebody has a real use case
 for it.

So I chose not to support it - no point supporting features that people
aren't using. If you have a "real use case" then it can be added.

This patch may be sufficient - I haven't fully considered all the
implications of changing this though.

 drivers/net/phy/phylink.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 7e0fdc17c8ee..2ee0d4dcf9a5 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -673,7 +673,6 @@ static void phylink_resolve(struct work_struct *w)
 		switch (pl->cur_link_an_mode) {
 		case MLO_AN_PHY:
 			link_state = pl->phy_state;
-			phylink_apply_manual_flow(pl, &link_state);
 			mac_config = link_state.link;
 			break;
 

@@ -698,11 +697,12 @@ static void phylink_resolve(struct work_struct *w)
 				link_state.pause = pl->phy_state.pause;
 				mac_config = true;
 			}
-			phylink_apply_manual_flow(pl, &link_state);
 			break;
 		}
 	}
 
+	phylink_apply_manual_flow(pl, &link_state);
+
 	if (mac_config) {
 		if (link_state.interface != pl->link_config.interface) {
 			/* The interface has changed, force the link down and

@@ -1639,9 +1639,6 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 
 	ASSERT_RTNL();
 
-	if (pl->cur_link_an_mode == MLO_AN_FIXED)
-		return -EOPNOTSUPP;
-
 	if (!phylink_test(pl->supported, Pause) &&
 	    !phylink_test(pl->supported, Asym_Pause))
 		return -EOPNOTSUPP;

@@ -1684,7 +1681,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 	/* Update our in-band advertisement, triggering a renegotiation if
 	 * the advertisement changed.
 	 */
-	if (!pl->phydev)
+	if (!pl->phydev && pl->cur_link_an_mode != MLO_AN_FIXED)
 		phylink_change_inband_advert(pl);
 
 	mutex_unlock(&pl->state_mutex);

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On Sun, Jan 31, 2021 at 12:19:50PM +0000, Russell King - ARM Linux admin wrote:

On Sun, Jan 31, 2021 at 11:12:14AM +0000, Russell King - ARM Linux admin wrote:

quoted

I discussed it with Andrew earlier last year, and his response was:

 DT configuration of pause for fixed link probably is sufficient. I don't
 remember it ever been really discussed for DSA. It was a Melanox
 discussion about limiting pause for the CPU. So I think it is safe to
 not implement ethtool -A, at least until somebody has a real use case
 for it.

So I chose not to support it - no point supporting features that people
aren't using. If you have a "real use case" then it can be added.

This patch may be sufficient - I haven't fully considered all the
implications of changing this though.

Did you try this patch? What's the outcome?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

quoted

On Sun, Jan 31, 2021 at 11:12:14AM +0000, Russell King - ARM Linux admin

wrote:

quoted

quoted

I discussed it with Andrew earlier last year, and his response was:

 DT configuration of pause for fixed link probably is sufficient. I
don't  remember it ever been really discussed for DSA. It was a
Melanox  discussion about limiting pause for the CPU. So I think it
is safe to  not implement ethtool -A, at least until somebody has a
real use case  for it.

So I chose not to support it - no point supporting features that
people aren't using. If you have a "real use case" then it can be added.

This patch may be sufficient - I haven't fully considered all the
implications of changing this though.

Did you try this patch? What's the outcome?

Hi,

Didn't tried it yet, its in my TODO list.

Regards,
Stefan.

quoted

quoted

I discussed it with Andrew earlier last year, and his response was:

 DT configuration of pause for fixed link probably is sufficient. I
don't  remember it ever been really discussed for DSA. It was a
Melanox  discussion about limiting pause for the CPU. So I think it
is safe to  not implement ethtool -A, at least until somebody has a
real use case  for it.

So I chose not to support it - no point supporting features that
people aren't using. If you have a "real use case" then it can be added.

This patch may be sufficient - I haven't fully considered all the
implications of changing this though.

Did you try this patch? What's the outcome?

For me patch worked as expected.

Thanks,
Stefan. 

quoted

 drivers/net/phy/phylink.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 7e0fdc17c8ee..2ee0d4dcf9a5 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -673,7 +673,6 @@ static void phylink_resolve(struct work_struct *w)
 		switch (pl->cur_link_an_mode) {
 		case MLO_AN_PHY:
 			link_state = pl->phy_state;
-			phylink_apply_manual_flow(pl, &link_state);
 			mac_config = link_state.link;
 			break;

@@ -698,11 +697,12 @@ static void phylink_resolve(struct work_struct

*w)

quoted

 				link_state.pause = pl->phy_state.pause;
 				mac_config = true;
 			}
-			phylink_apply_manual_flow(pl, &link_state);
 			break;
 		}
 	}

+	phylink_apply_manual_flow(pl, &link_state);
+
 	if (mac_config) {
 		if (link_state.interface != pl->link_config.interface) {
 			/* The interface has changed, force the link down

and @@ -1639,9

quoted

+1639,6 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,

 	ASSERT_RTNL();

-	if (pl->cur_link_an_mode == MLO_AN_FIXED)
-		return -EOPNOTSUPP;
-
 	if (!phylink_test(pl->supported, Pause) &&
 	    !phylink_test(pl->supported, Asym_Pause))
 		return -EOPNOTSUPP;

@@ -1684,7 +1681,7 @@ int phylink_ethtool_set_pauseparam(struct

phylink *pl,

quoted

 	/* Update our in-band advertisement, triggering a renegotiation if
 	 * the advertisement changed.
 	 */
-	if (!pl->phydev)
+	if (!pl->phydev && pl->cur_link_an_mode != MLO_AN_FIXED)
 		phylink_change_inband_advert(pl);

 	mutex_unlock(&pl->state_mutex);

--
RMK's Patch system:
https://urldefense.proofpoint.com/v2/url?u=https-

3A__www.armlinux.org.

quoted

uk_developer_patches_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ
3dKwkTIxK

quoted

Al6_Bs7GMx4zhJArrXKN2mDMOXGh7lg&m=bLvAkwDrYioAER_dvXEqutiRiU
W57bKfscMh

quoted

71TGDxw&s=p5jgFDs7cxtpIE9LZ3ogOahGzpuKjG4PHOcPF6qXnXI&e=
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

--
RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https-
3A__www.armlinux.org.uk_developer_patches_&d=DwIBAg&c=nKjWec2b6
R0mOyPaz7xtfQ&r=DDQ3dKwkTIxKAl6_Bs7GMx4zhJArrXKN2mDMOXGh7lg&
m=bLvAkwDrYioAER_dvXEqutiRiUW57bKfscMh71TGDxw&s=p5jgFDs7cxtpIE9
LZ3ogOahGzpuKjG4PHOcPF6qXnXI&e=
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On Mon, Feb 15, 2021 at 04:19:19PM +0000, Stefan Chulski wrote:

quoted

quoted

quoted

I discussed it with Andrew earlier last year, and his response was:

 DT configuration of pause for fixed link probably is sufficient. I
don't  remember it ever been really discussed for DSA. It was a
Melanox  discussion about limiting pause for the CPU. So I think it
is safe to  not implement ethtool -A, at least until somebody has a
real use case  for it.

So I chose not to support it - no point supporting features that
people aren't using. If you have a "real use case" then it can be added.

This patch may be sufficient - I haven't fully considered all the
implications of changing this though.

Did you try this patch? What's the outcome?

For me patch worked as expected.

Great, thanks. Andrew, do you have any further opinions on this
subject, or shall I sent a proper patch for net-next?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On Mon, Feb 15, 2021 at 04:19:19PM +0000, Stefan Chulski wrote:

quoted

quoted

quoted

I discussed it with Andrew earlier last year, and his response was:

 DT configuration of pause for fixed link probably is sufficient. I
don't  remember it ever been really discussed for DSA. It was a
Melanox  discussion about limiting pause for the CPU. So I think it
is safe to  not implement ethtool -A, at least until somebody has a
real use case  for it.

So I chose not to support it - no point supporting features that
people aren't using. If you have a "real use case" then it can be added.

This patch may be sufficient - I haven't fully considered all the
implications of changing this though.

Did you try this patch? What's the outcome?

For me patch worked as expected.

Hi Stefan

Russell's patch allows it, but i would be interested in knows why you
actually need it. What is your use case for changing this on the fly?

	 Andrew

quoted

quoted

quoted

quoted

I discussed it with Andrew earlier last year, and his response was:

 DT configuration of pause for fixed link probably is
sufficient. I don't  remember it ever been really discussed for
DSA. It was a Melanox  discussion about limiting pause for the
CPU. So I think it is safe to  not implement ethtool -A, at
least until somebody has a real use case  for it.

So I chose not to support it - no point supporting features that
people aren't using. If you have a "real use case" then it can be added.

This patch may be sufficient - I haven't fully considered all the
implications of changing this though.

Did you try this patch? What's the outcome?

For me patch worked as expected.

Hi Stefan

Russell's patch allows it, but i would be interested in knows why you actually
need it. What is your use case for changing this on the fly?

	 Andrew

Usually, user prefer have the capability disable/enable flow control on the fly.
For example:
Armada connected by 10G fixed link to SOHO switch and SOHO has 10 external 1G LAN interfaces.
When we have 2 flows (from Armada to LAN) from two different ports, we have an obvious congestion issue.
Bursts on 10G interface would cause FC frames on Armada<->SOHO 10G port and one flow would affect another.
In some use cases, the user would prefer lossless traffic and keep FC, for another use case (probably UDP streaming) disable FC.

Regards,
Stefan.










 

On Mon, Feb 15, 2021 at 08:33:19PM +0000, Stefan Chulski wrote:

quoted

quoted

quoted

quoted

quoted

I discussed it with Andrew earlier last year, and his response was:

 DT configuration of pause for fixed link probably is
sufficient. I don't  remember it ever been really discussed for
DSA. It was a Melanox  discussion about limiting pause for the
CPU. So I think it is safe to  not implement ethtool -A, at
least until somebody has a real use case  for it.

So I chose not to support it - no point supporting features that
people aren't using. If you have a "real use case" then it can be added.

This patch may be sufficient - I haven't fully considered all the
implications of changing this though.

Did you try this patch? What's the outcome?

For me patch worked as expected.

Hi Stefan

Russell's patch allows it, but i would be interested in knows why you actually
need it. What is your use case for changing this on the fly?

	 Andrew

Usually, user prefer have the capability disable/enable flow control on the fly.
For example:
Armada connected by 10G fixed link to SOHO switch and SOHO has 10 external 1G LAN interfaces.
When we have 2 flows (from Armada to LAN) from two different ports, we have an obvious congestion issue.
Bursts on 10G interface would cause FC frames on Armada<->SOHO 10G port and one flow would affect another.
In some use cases, the user would prefer lossless traffic and keep FC, for another use case (probably UDP streaming) disable FC.

O.K, so you have reasonable uses cases for it. I'm O.K. with this.

     Andrew