netdev - Re: [PATCH net-next v2 2/3] net: dsa: mxl-gsw1xx: configure PCS polarities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20260131143904.ux4h43nmbxp5bn2b@skbuf>
Date: Sat, 31 Jan 2026 16:39:04 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Daniel Golle <daniel@...rotopia.org>
Cc: Hauke Mehrtens <hauke@...ke-m.de>, Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/3] net: dsa: mxl-gsw1xx: configure PCS
 polarities

On Fri, Jan 30, 2026 at 12:49:55AM +0000, Daniel Golle wrote:
> Configure SerDes PCS RX and TX polarities using the newly
> introduced generic properties.
> 
> Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> ---
> @@ -260,15 +268,17 @@ static int gsw1xx_pcs_reset(struct gsw1xx_priv *priv)
>  	      FIELD_PREP(GSW1XX_SGMII_PHY_RX0_CFG2_FILT_CNT,
>  			 GSW1XX_SGMII_PHY_RX0_CFG2_FILT_CNT_DEF);
>  
> +	ret = phy_get_manual_rx_polarity(of_fwnode_handle(pcs_port->dn),
> +					 phy_modes(interface), &pol);
> +	if (ret)
> +		return ret;
> +
>  	/* RX lane seems to be inverted internally, so bit
>  	 * GSW1XX_SGMII_PHY_RX0_CFG2_INVERT needs to be set for normal
>  	 * (ie. non-inverted) operation.

Sorry, I just noticed this existing comment now.

I think this patch set has the right solution with the wrong explanation,
and as someone who cares about logical consistency as much as about
functionality, I think it's worth resending to clarify something.

There exists a problem which, at a high level, is that the signal can
be inverted at multiple levels in its path. When multiple levels are
configurable, it becomes important to know "which level does this DT
property correspond to".  Otherwise it's not clear how inversions at
multiple layers relate to each other.

For someone who doesn't study the problem deeply, it will be confusing
that this approach is inconsistent with what I intend to do for XPCS on
SJA1105:
https://lore.kernel.org/netdev/20260122105654.105600-16-vladimir.oltean@nxp.com/

That is, if one needs to invert the TX lane signal in the PCS to achieve
normal polarity at the pins (to compensate for internal inversion in the
SerDes/PMA), then the PCS node needs to have "tx-polarity = <PHY_POL_INVERT>".

In Documentation/devicetree/bindings/phy/phy-common-props.yaml it
explicitly says that "If the property is absent, the default value is
undefined." in order to permit drivers to call phy_get_rx_polarity()
with a custom default_val, rather than phy_get_manual_rx_polarity()
which implies a predefined default_val of PHY_POL_NORMAL.

This is the opposite of what you're doing. You need to invert the RX
signal in the SerDes for what can be assumed to be a hidden inversion in
the PCS, but you make that invisible in the device tree, interpreting
PHY_POL_NORMAL as "inverted" when programming the SerDes.

Only difference, which also makes your approach self-consistent, is that
the rx-polarity property goes neither in the PCS nor the SerDes OF nodes
(which don't exist), but in the port node. If we interpret the port node
as what happens at the pins, the totality of the internal data path
layers with no further insight into sub-components, then it's OK and is
the simplest solution to the problem.

But it needs to be presented 100% clearly, with a very clear distinction
between the PCS, the SerDes PHY and the port as a whole.  Patches 1 and
2 use these 3 concepts very interchangeably, ranging from comments/commit
messages ("Reference the common PHY properties so RX and TX SerDes lane
polarity of the SGMII/1000Base-X/2500Base-X PCS can be configured") to
code placement (SerDes configuration done in gsw1xx_pcs_reset()) to
variable names ("pcs_port" could lose the "pcs" portion, to avoid somebody
unfamiliar with the HW doing some refactoring where they take the port
OF node as the PCS OF node, for other purposes as well, perhaps not applicable).

> -	 *
> -	 * TODO: Take care of inverted RX pair once generic property is
> -	 *       available
>  	 */
> -
> -	val |= GSW1XX_SGMII_PHY_RX0_CFG2_INVERT;
> +	if (pol == PHY_POL_NORMAL)
> +		val |= GSW1XX_SGMII_PHY_RX0_CFG2_INVERT;
>  
>  	ret = regmap_write(priv->sgmii, GSW1XX_SGMII_PHY_RX0_CFG2, val);
>  	if (ret < 0)
> @@ -277,9 +287,13 @@ static int gsw1xx_pcs_reset(struct gsw1xx_priv *priv)
>  	val = FIELD_PREP(GSW1XX_SGMII_PHY_TX0_CFG3_VBOOST_LEVEL,
>  			 GSW1XX_SGMII_PHY_TX0_CFG3_VBOOST_LEVEL_DEF);
>  
> -	/* TODO: Take care of inverted TX pair once generic property is
> -	 *       available
> -	 */
> +	ret = phy_get_manual_tx_polarity(of_fwnode_handle(pcs_port->dn),
> +					 phy_modes(interface), &pol);
> +	if (ret)
> +		return ret;
> +
> +	if (pol == PHY_POL_INVERT)
> +		val |= GSW1XX_SGMII_PHY_TX0_CFG3_INVERT;
>  
>  	ret = regmap_write(priv->sgmii, GSW1XX_SGMII_PHY_TX0_CFG3, val);
>  	if (ret < 0)
> @@ -336,7 +350,7 @@ static int gsw1xx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  	priv->tbi_interface = PHY_INTERFACE_MODE_NA;
>  
>  	if (!reconf)
> -		ret = gsw1xx_pcs_reset(priv);
> +		ret = gsw1xx_pcs_reset(priv, interface);
>  
>  	if (ret)
>  		return ret;
> -- 
> 2.52.0