Messages in this thread | | | Date | Mon, 18 Jul 2022 17:14:42 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings based on rate adaptation |
| |
On Sat, Jul 16, 2022 at 06:37:22PM -0400, Sean Anderson wrote: > On 7/16/22 4:17 PM, Andrew Lunn wrote: > > On Fri, Jul 15, 2022 at 05:59:17PM -0400, Sean Anderson wrote: > > > If the phy is configured to use pause-based rate adaptation, ensure that > > > the link is full duplex with pause frame reception enabled. Note that these > > > settings may be overridden by ethtool. > > > > > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > > > --- > > > > > > Changes in v3: > > > - New > > > > > > drivers/net/phy/phylink.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > > index 7fa21941878e..7f65413aa778 100644 > > > --- a/drivers/net/phy/phylink.c > > > +++ b/drivers/net/phy/phylink.c > > > @@ -1445,6 +1445,10 @@ static void phylink_phy_change(struct phy_device *phydev, bool up) > > > pl->phy_state.speed = phy_interface_speed(phydev->interface, > > > phydev->speed); > > > pl->phy_state.duplex = phydev->duplex; > > > + if (phydev->rate_adaptation == RATE_ADAPT_PAUSE) { > > > + pl->phy_state.duplex = DUPLEX_FULL; > > > + rx_pause = true; > > > + } > > > > I would not do this. If the requirements for rate adaptation are not > > fulfilled, you should turn off rate adaptation. > > > > A MAC which knows rate adaptation is going on can help out, by not > > advertising 10Half, 100Half etc. Autoneg will then fail for modes > > where rate adaptation does not work. > > OK, so maybe it is better to phylink_warn here. Something along the > lines of "phy using pause-based rate adaptation, but duplex is %s". > > > The MAC should also be declaring what sort of pause it supports, so > > disable rate adaptation if it does not have async pause. > > That's what we do in the previous patch. > > The problem is that rx_pause and tx_pause are resolved based on our > advertisement and the link partner's advertisement. However, the link > partner may not support pause frames at all. In that case, we will get > rx_pause and tx_pause as false. However, we still want to enable rx_pause, > because we know that the phy will be emitting pause frames. And of course > the user can always force disable pause frames anyway through ethtool.
If you want the MAC to enable rx_pause, that ought to be handled separately in the mac_link_up() method, IMHO.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |