[PATCH stable 4.9] net: dsa: bcm_sf2: force pause link settings

[PATCH stable 4.9] net: dsa: bcm_sf2: force pause link settings

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

commit 7c97bc0128b2eecc703106112679a69d446d1a12 upstream

The pause settings reported by the PHY should also be applied to the GMII port
status override otherwise the switch will not generate pause frames towards the
link partner despite the advertisement saying otherwise.

Fixes: 246d7f773c13 ("net: dsa: add Broadcom SF2 switch driver")
Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 40b3adf7ad99..03f38c36e188 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -671,6 +671,11 @@ static void bcm_sf2_sw_adjust_link(struct dsa_switch *ds, int port,
 		reg |= LINK_STS;
 	if (phydev->duplex == DUPLEX_FULL)
 		reg |= DUPLX_MODE;
+	if (phydev->pause) {
+		if (phydev->asym_pause)
+			reg |= TXFLOW_CNTL;
+		reg |= RXFLOW_CNTL;
+	}
 
 	core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
 
-- 
2.25.1

[PATCH stable 4.14] net: dsa: bcm_sf2: force pause link settings

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

commit 7c97bc0128b2eecc703106112679a69d446d1a12 upstream

The pause settings reported by the PHY should also be applied to the GMII port
status override otherwise the switch will not generate pause frames towards the
link partner despite the advertisement saying otherwise.

Fixes: 246d7f773c13 ("net: dsa: add Broadcom SF2 switch driver")
Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 11a72c4cbb92..b838d6bf3165 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -701,6 +701,11 @@ static void bcm_sf2_sw_adjust_link(struct dsa_switch *ds, int port,
 		reg |= LINK_STS;
 	if (phydev->duplex == DUPLEX_FULL)
 		reg |= DUPLX_MODE;
+	if (phydev->pause) {
+		if (phydev->asym_pause)
+			reg |= TXFLOW_CNTL;
+		reg |= RXFLOW_CNTL;
+	}
 
 	core_writel(priv, reg, offset);
 
-- 
2.25.1

[PATCH stable 4.19] net: dsa: bcm_sf2: force pause link settings

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

commit 7c97bc0128b2eecc703106112679a69d446d1a12 upstream

The pause settings reported by the PHY should also be applied to the GMII port
status override otherwise the switch will not generate pause frames towards the
link partner despite the advertisement saying otherwise.

Fixes: 246d7f773c13 ("net: dsa: add Broadcom SF2 switch driver")
Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 3deda0321c00..8a67c6f76e3b 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -599,6 +599,11 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
 		reg |= LINK_STS;
 	if (state->duplex == DUPLEX_FULL)
 		reg |= DUPLX_MODE;
+	if (state->pause & MLO_PAUSE_TXRX_MASK) {
+		if (state->pause & MLO_PAUSE_TX)
+			reg |= TXFLOW_CNTL;
+		reg |= RXFLOW_CNTL;
+	}
 
 	core_writel(priv, reg, offset);
 }
-- 
2.25.1

[PATCH stable 5.4] net: dsa: bcm_sf2: force pause link settings

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

commit 7c97bc0128b2eecc703106112679a69d446d1a12 upstream

The pause settings reported by the PHY should also be applied to the GMII port
status override otherwise the switch will not generate pause frames towards the
link partner despite the advertisement saying otherwise.

Fixes: 246d7f773c13 ("net: dsa: add Broadcom SF2 switch driver")
Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 0ee1c0a7b165..282e9e2a50d9 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -602,6 +602,11 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
 		reg |= LINK_STS;
 	if (state->duplex == DUPLEX_FULL)
 		reg |= DUPLX_MODE;
+	if (state->pause & MLO_PAUSE_TXRX_MASK) {
+		if (state->pause & MLO_PAUSE_TX)
+			reg |= TXFLOW_CNTL;
+		reg |= RXFLOW_CNTL;
+	}
 
 	core_writel(priv, reg, offset);
 }
-- 
2.25.1

Re: [PATCH stable 4.9] net: dsa: bcm_sf2: force pause link settings

[Vladimir Oltean]

Hi Florian,

On Mon, Jul 04, 2022 at 08:35:07AM -0700, Florian Fainelli wrote:
> From: Doug Berger <opendmb@gmail.com>
> 
> commit 7c97bc0128b2eecc703106112679a69d446d1a12 upstream
> 
> The pause settings reported by the PHY should also be applied to the GMII port
> status override otherwise the switch will not generate pause frames towards the
> link partner despite the advertisement saying otherwise.
> 
> Fixes: 246d7f773c13 ("net: dsa: add Broadcom SF2 switch driver")
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/dsa/bcm_sf2.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index 40b3adf7ad99..03f38c36e188 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -671,6 +671,11 @@ static void bcm_sf2_sw_adjust_link(struct dsa_switch *ds, int port,
>  		reg |= LINK_STS;
>  	if (phydev->duplex == DUPLEX_FULL)
>  		reg |= DUPLX_MODE;
> +	if (phydev->pause) {
> +		if (phydev->asym_pause)
> +			reg |= TXFLOW_CNTL;
> +		reg |= RXFLOW_CNTL;
> +	}

Is this correct? phydev->pause and phydev->asym_pause keep the Pause and
Asym_Pause bits advertised by the link partner. In other words, in this
manual resolution you are ignoring what the local switch port has
advertised.

To give you an example (I'm looking at the infamous Table 28B–3—Pause
resolution from IEEE 802.3).

Your logic, simplified, says: enable TXFLOW_CNTL as local device
resolution iff the link partner advertised PAUSE=1 && ASM_DIR=1, and
RXFLOW_CNTL if the link partner advertised PAUSE=1 at all.

The most trivial counter-example is if we (local device) advertise
PAUSE=0 ASM_DIR=0 (this can be achieved with ethtool rx off tx off).
That is one way to rig the negotiated flow control to off/off regardless
of what the link partner has advertised. But your logic would enable
local capabilities just because the link partner said so, ignoring what
was advertised here.

Another example is if the local station advertises PAUSE=1, ASM_DIR=1,
and remote PAUSE=0, ASM_DIR=1. According to IEEE, the link partners
should agree on "rx on tx off" for the local system, and "tx on rx off"
for the link partner. But your logic does not even enter the first "if"
condition, because phydev->pause (link partner's PAUSE advertisement) is
0. So neither TXFLOW_CNTL nor RXFLOW_CNTL will be enabled locally,
despite the expectation and what is printed by phy_print_status().

>  
>  	core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
>  
> -- 
> 2.25.1
> 

Re: [PATCH stable 4.9] net: dsa: bcm_sf2: force pause link settings

[Andrew Lunn]

On Tue, Jul 05, 2022 at 02:34:57AM +0300, Vladimir Oltean wrote:
> Hi Florian,
> 
> On Mon, Jul 04, 2022 at 08:35:07AM -0700, Florian Fainelli wrote:
> > From: Doug Berger <opendmb@gmail.com>
> > 
> > commit 7c97bc0128b2eecc703106112679a69d446d1a12 upstream
> > 
> > The pause settings reported by the PHY should also be applied to the GMII port
> > status override otherwise the switch will not generate pause frames towards the
> > link partner despite the advertisement saying otherwise.
> > 
> > Fixes: 246d7f773c13 ("net: dsa: add Broadcom SF2 switch driver")
> > Signed-off-by: Doug Berger <opendmb@gmail.com>
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> >  drivers/net/dsa/bcm_sf2.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> > index 40b3adf7ad99..03f38c36e188 100644
> > --- a/drivers/net/dsa/bcm_sf2.c
> > +++ b/drivers/net/dsa/bcm_sf2.c
> > @@ -671,6 +671,11 @@ static void bcm_sf2_sw_adjust_link(struct dsa_switch *ds, int port,
> >  		reg |= LINK_STS;
> >  	if (phydev->duplex == DUPLEX_FULL)
> >  		reg |= DUPLX_MODE;
> > +	if (phydev->pause) {
> > +		if (phydev->asym_pause)
> > +			reg |= TXFLOW_CNTL;
> > +		reg |= RXFLOW_CNTL;
> > +	}
> 
> Is this correct? phydev->pause and phydev->asym_pause keep the Pause and
> Asym_Pause bits advertised by the link partner. In other words, in this
> manual resolution you are ignoring what the local switch port has
> advertised.

linkmode_resolve_pause() is not used yet outside of phylink, but
should help here.

       Andrew

Re: [PATCH stable 4.9] net: dsa: bcm_sf2: force pause link settings

[Vladimir Oltean]

On Tue, Jul 05, 2022 at 08:56:16AM +0200, Andrew Lunn wrote:
> linkmode_resolve_pause() is not used yet outside of phylink, but
> should help here.

I don't think linkmode_resolve_pause() was present in 4.9.
I'm looking at what gianfar was doing at the time (gfar_get_flowctrl_cfg)
and I see mii_resolve_flowctrl_fdx() as a potentially relevant helper.