Re: [PATCH RFC] Providing a helper for PCS inband negotiation

On Tue, May 16, 2023 at 12:00:09PM +0300, Vladimir Oltean wrote:

On Tue, May 16, 2023 at 12:36:33AM +0100, Russell King (Oracle) wrote:

quoted

quoted

Clause 73 negotiates the actual use of 10GBase-KR as a SERDES protocol
through the copper backplane in favor of other "Base-K*" alternative
link modes, so it's not quite proper to say that 10GBase-KR is a clause
73 using protocol.

I believe it is correct to state it as such, because:

72.1: Table 72–1—Physical Layer clauses associated with the 10GBASE-KR PMD

	73—Auto-Negotiation for Backplane Ethernet              Required

Essentially, 802.3 doesn't permit 10GBASE-KR without hardware support
for Clause 73 AN (but that AN doesn't have to be enabled by management
software.)

Just like clause 40 (PCS and PMA for 1000BASE-T) requires clause 28 AN
to be supported. But, when the autoneg process begins, the use of
10GBase-KR as a protocol over the backplane link hasn't even been yet
established, so I find it unnatural to speak of clause 73 autoneg as
something that 10GBase-KR has.

The reason why I'm insisting on this is because to me, to treat clause
73 as an in-band autoneg process of 10GBase-KR sounds like a reversal of
causality. The clause 73 link codeword has a Technology Ability field
through which 10GBase-KR, 1GBase-KX etc are advertised as supported
protocols. If C73 is an inband protocol of 10GBase-KR, what should the
local PCS advertise for its Technology Ability? Only 10GBase-KR, because
this is what is implied by treating it as an attribute of 10GBase-KR, no?

I'm not going to get hung up on this, because I don't regard the point
as being particularly important to this discussion. You are right that
Clause 73 AN selects whether e.g. 10GBASE-KR gets used, and I don't
disagree with that. We're just entering into what seems like a pointless
debate that eats up email bandwidth.

But that would be a denatured way of negotiating - advertise a single
link mode, take it or leave it. And what other inband autoneg protocols
permit, say, starting from SGMII and ending in 1000Base-X? Clause 73
can't be directly compared to what we currently mean by managed =
"in-band-status".

Not only is C37 autoneg not directly comparable to C73, but they are not
mutually exclusive, either. I would say they are more or less orthogonal.
More below.

I don't believe that toggling clause 73 autoneg based on phylink_pcs_neg_mode()
makes much sense.

I agree, which means phylink_pcs_neg_mode() needs to document this so
that people don't think it does - and that solves both of the issues
I was bringing up in my original email.

quoted

However, if we did want to extend this topic, then there are a number
of questions that really need to be asked is about the XPCS driver.
Such as - what does 1000BASE-KX, 10000BASE-KX4, 10000BASE-KR and
2500BASE-X have to do with USXGMII, and why are there no copper
ethtool modes listed when a USXGMII link can definitely support
being connected to a copper PHY? (See xpcs_usxgmii_features[]).

Why does XPCS think that USXGMII uses Clause 73 AN? (see the first
entry in synopsys_xpcs_compat[].)

First, in principle USXGMII and clause 73 are not mutually exclusive.

It is possible to use clause 73 to advertise 10GBase-KR as a link mode,
and that will give you link training for proper 3-tap electrical
equalization over the copper backplane.

Then, once C73 AN/LT ended and 10GBase-KR has been established, is
possible to configure the 10GBase-R PCS to enable C37 USXGMII to select
the actual data rate via symbol replication, while the SERDES lane
remains at 10GBaud. At least, the XPCS seems to permit enabling symbol
replication in conjunction with 10GBase-KR.

My comments are against the driver as it stands today, not some
theoretical case that the hardware may support.

What I'm getting at is if the interface mode is
PHY_INTERFACE_MODE_USXGMII, then... okay... we _may_ wish to do
clause 73 negotiation advertising 10GBASE-KR and then do clause 73
for the USXGMII control word - but the driver doesn't do this as far
as I can see. If C73 AN is being used, it merely reads the C73
state and returns the resolution from that. Any speed information that
a USXGMII PHY passes back over the C37 inband signalling would be
ignored because there seems to be no provision for the USXGMII
inband signalling.

So I'm confused what the xpcs driver _actually_ does when USXGMII
mode is selected by PHY_INTERFACE_MODE_USXGMII, because looking at
the driver, it doesn't look like it's USXGMII at all.

Then, there's the entire issue that the code, as it was originally
introduced, is not the same as it is now. For example, this bit in
xpcs_do_config():

	switch (compat->an_mode) {
	case DW_AN_C73:
		if (phylink_autoneg_inband(mode)) {
			ret = xpcs_config_aneg_c73(xpcs, compat);
			if (ret)
				return ret;
		}
		break;

used to look at state->an_enabled rather than phylink_autoneg_inband().
Through my idiocy, I inadvertently converted that in commit 11059740e616
("net: pcs: xpcs: convert to phylink_pcs_ops").

If we want to change that back to the old behaviour, that needs to
be:
		if (test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising)) {
			...
		}
		break;

but that wouldn't ever have been sufficient, even when the code was
using an_enabled, because both of these reflect the user configuration.
(an_enabled was just a proxy for this Autoneg bit). I'm going to call
both of these an "AN indicator" in the question below.

Isn't it rather perverse that the driver configures AN if this AN
indicator is set, but then does nothing if it isn't?

quoted

xpcs_sgmii_features[] only mentions copper linkmodes, but as we know,
it's common for copper PHYs to also support fibre with an auto-
selection mechanism. So, 1000BASE-X is definitely possible if SGMII
is supported, so why isn't it listed.

Most likely explanation is that XPCS has never been paired up until now
to such a PHY.

So it's probably safe to add ETHTOOL_LINK_MODE_1000baseX_Full_BIT
there - thanks.

quoted

As previously said, 1000BASE-X can be connected to a PHY that does
1000BASE-T, so why does xpcs_1000basex_features[] not mention
1000baseT_Full... there's probably more here as well.

Interestingly, xpcs_2500basex_features[] _does_ mention both
2500BASE-X and 2500BASE-T, but I think that only does because I
happened to comment on it during a review.

I think xpcs is another can of worms, but is an easier can of worms
to solve than trying to sort out that "what's an ethernet PHY"
question we seem to be heading towards (which I think would be a
mammoth task, even back when phylink didn't exist, to sort out.)

I wasn't necessarily going to go all the way into "what's a PHY?".
I just want to clarify some terms such that we can agree what is correct
and what is not. I believe that much of what's currently in XPCS w.r.t.
C73 is not correct, partly through initial intention and partly through
blind conversions such as mine.

Right, that's probably why I'm having a hard time interpreting what
this driver is doing when it comes to these modes that makes use
clause 73.

As this is the only phylink-using implementation that involves clause
73 at present, I would like to ensure that there's a clear resolution
of the expected behaviour before we get further implementations, and
preferably document what's expected.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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