| Message ID | E1pxWYJ-002QsU-IT@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: xpcs: cleanups for clause 73 support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 21 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, May 12, 2023 at 06:27:35PM +0100, Russell King (Oracle) wrote: > xpcs was indicating symmetric pause should be enabled regardless of > the advertisements by either party. Fix this to use > linkmode_resolve_pause() now that we're no longer obliterating the > link partner's advertisement by logically anding it with our own. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/pcs/pcs-xpcs.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > index 43115d04c01a..beed799a69a7 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -538,11 +538,20 @@ static void xpcs_resolve_lpa_c73(struct dw_xpcs *xpcs, > struct phylink_link_state *state) > { > __ETHTOOL_DECLARE_LINK_MODE_MASK(res); > + bool tx_pause, rx_pause; > > /* Calculate the union of the advertising masks */ > linkmode_and(res, state->lp_advertising, state->advertising); > > - state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; > + /* Resolve pause modes */ > + linkmode_resolve_pause(state->advertising, state->lp_advertising, > + &tx_pause, &rx_pause); > + > + if (tx_pause) > + state->pause |= MLO_PAUSE_TX; > + if (rx_pause) > + state->pause |= MLO_PAUSE_RX; > + Hi Russell I must be missing something. Why not use phylink_resolve_an_pause()? Andrew
On Sat, May 13, 2023 at 07:47:35PM +0200, Andrew Lunn wrote: > On Fri, May 12, 2023 at 06:27:35PM +0100, Russell King (Oracle) wrote: > > xpcs was indicating symmetric pause should be enabled regardless of > > the advertisements by either party. Fix this to use > > linkmode_resolve_pause() now that we're no longer obliterating the > > link partner's advertisement by logically anding it with our own. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/pcs/pcs-xpcs.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > index 43115d04c01a..beed799a69a7 100644 > > --- a/drivers/net/pcs/pcs-xpcs.c > > +++ b/drivers/net/pcs/pcs-xpcs.c > > @@ -538,11 +538,20 @@ static void xpcs_resolve_lpa_c73(struct dw_xpcs *xpcs, > > struct phylink_link_state *state) > > { > > __ETHTOOL_DECLARE_LINK_MODE_MASK(res); > > + bool tx_pause, rx_pause; > > > > /* Calculate the union of the advertising masks */ > > linkmode_and(res, state->lp_advertising, state->advertising); > > > > - state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; > > + /* Resolve pause modes */ > > + linkmode_resolve_pause(state->advertising, state->lp_advertising, > > + &tx_pause, &rx_pause); > > + > > + if (tx_pause) > > + state->pause |= MLO_PAUSE_TX; > > + if (rx_pause) > > + state->pause |= MLO_PAUSE_RX; > > + > > Hi Russell > > I must be missing something. Why not use phylink_resolve_an_pause()? Check the next few patches... it eventually gets to using the c73 helper, entirely eliminating this function. This is a staged conversion, so that its easier to bisect down to the change that caused the breakage. Converting straight to the c73 helper would be a big change - not only fixing the pause resolution mechanism but also how we do the c73 priority resolution. Moreover, the above patch can be backported to stable kernels without too much effort if there's a desire to do so.
> > Hi Russell > > > > I must be missing something. Why not use phylink_resolve_an_pause()? > > Check the next few patches... it eventually gets to using the c73 > helper, entirely eliminating this function. Yes, i got to that eventually. What might of helped would of been to include something like: This is a staged conversion, so that its easier to bisect down to the change that caused anye breakage. The aim is to replace this code with the c73 helper. in the commit message. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index 43115d04c01a..beed799a69a7 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -538,11 +538,20 @@ static void xpcs_resolve_lpa_c73(struct dw_xpcs *xpcs, struct phylink_link_state *state) { __ETHTOOL_DECLARE_LINK_MODE_MASK(res); + bool tx_pause, rx_pause; /* Calculate the union of the advertising masks */ linkmode_and(res, state->lp_advertising, state->advertising); - state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; + /* Resolve pause modes */ + linkmode_resolve_pause(state->advertising, state->lp_advertising, + &tx_pause, &rx_pause); + + if (tx_pause) + state->pause |= MLO_PAUSE_TX; + if (rx_pause) + state->pause |= MLO_PAUSE_RX; + state->speed = xpcs_get_max_usxgmii_speed(res); state->duplex = DUPLEX_FULL; }
xpcs was indicating symmetric pause should be enabled regardless of the advertisements by either party. Fix this to use linkmode_resolve_pause() now that we're no longer obliterating the link partner's advertisement by logically anding it with our own. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/pcs/pcs-xpcs.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)