Phylink flow control support on ports with MLO_AN_FIXED auto negotiation

Phylink flow control support on ports with MLO_AN_FIXED auto negotiation

[Stefan Chulski]

Hi,

Armada has options for 1G/10G ports without PHY's(for example community board Macchiato single shot).
This port doesn't have PHY's and cannot negotiate Flow Control support, but we can for example connect two ports without PHY's and manually(by ethtool) configure FC.
Current phylink return error if I do this on ports with MLO_AN_FIXED(callback phylink_ethtool_set_pauseparam):
if (pl->cur_link_an_mode == MLO_AN_FIXED)
                return -EOPNOTSUPP;

How can we enable FC configurations for these ports? Do you have any suggestions or should I post my proposal as an RFC patch?

Thanks,
Stefan.

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

[Stefan Chulski]

> > Hi,
> >
> > Armada has options for 1G/10G ports without PHY's(for example
> community board Macchiato single shot).
> > This port doesn't have PHY's and cannot negotiate Flow Control support,
> but we can for example connect two ports without PHY's and manually(by
> ethtool) configure FC.
> 
> On the Macchiatobin single shot, you use the existing SFP support rather
> than forcing them to fixed link.
> 
> > Current phylink return error if I do this on ports with
> MLO_AN_FIXED(callback phylink_ethtool_set_pauseparam):
> > if (pl->cur_link_an_mode == MLO_AN_FIXED)
> >                 return -EOPNOTSUPP;
> >
> > How can we enable FC configurations for these ports? Do you have any
> suggestions or should I post my proposal as an RFC patch?
> 
> If you really must use fixed-link, you specify the pause modes via firmware,
> just as you specify the speed and duplex - you specify the link partner's flow
> control abilities.

In this case we cannot change this by ethtool during runtime?

Regards,
Stefan.

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

[Russell King - ARM Linux admin]

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!

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

[Russell King - ARM Linux admin]

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

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

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

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

[Stefan Chulski]

> > 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.
> 
> Did you try this patch? What's the outcome?
> 

Hi,

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

Regards,
Stefan.

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

[Stefan Chulski]

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

> >
> >  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://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.armlinux.org.
> >
> uk_developer_patches_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ
> 3dKwkTIxK
> >
> Al6_Bs7GMx4zhJArrXKN2mDMOXGh7lg&m=bLvAkwDrYioAER_dvXEqutiRiU
> W57bKfscMh
> > 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!

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

[Russell King - ARM Linux admin]

On Mon, Feb 15, 2021 at 04:19:19PM +0000, Stefan Chulski 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.
> > 
> > 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!

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

[Andrew Lunn]

On Mon, Feb 15, 2021 at 04:19:19PM +0000, Stefan Chulski 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.
> > 
> > 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

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

[Stefan Chulski]

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










 

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

[Andrew Lunn]

On Mon, Feb 15, 2021 at 08:33:19PM +0000, Stefan Chulski 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.
> > > >
> > > > 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