Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings based on rate adaptation

On 7/18/22 12:12 PM, Russell King (Oracle) wrote:

On Fri, Jul 15, 2022 at 05:59:17PM -0400, Sean Anderson wrote:

quoted

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

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.

quoted

+		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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel