Messages in this thread | | | Subject | Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings based on rate adaptation | From | Sean Anderson <> | Date | Mon, 18 Jul 2022 12:45:01 -0400 |
| |
On 7/18/22 12:12 PM, Russell King (Oracle) 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;
Just form context, as discussed with Andrew, this should never change anything. That is, it could be replaced with
WARN_ON_ONCE(pl->phy_state.duplex != DUPLEX_FULL);
Since the phy should never report that it is using rate_adaptation unless it is using full duplex.
>> + rx_pause = true; >> + } > > I really don't like this - as I've pointed out in my previous email, the > reporting in the kernel message log for "Link is Up" will be incorrect > if you force the phy_state here like this. If the media side link has > been negotiated to be half duplex, we should state that in the "Link is > Up" message.
So I guess the question here is whether there are phys which do duplex adaptation. There definitely are phys which support a half-duplex interface mode and a full duplex link mode (such as discussed in patch 08/47). If it's important to get this right, I can do the same treatment for duplex as I did for speed.
> It's only the PCS and MAC that care about this, so this should be dealt > with when calling into the PCS and MAC's link_up() method. > > The problem we have are the legacy drivers (of which mv88e6xxx and > mtk_eth_soc are the only two I'm aware of) that make use of the > state->speed and state->duplex when configuring stuff. We could've been > down to just mv88e6xxx had the DSA and mv88e6xxx patches been sorted > out, but sadly that's now going to be some time off due to reviewer > failure. >
--Sean
| |