netdev - Re: [PATCH net-next v2 13/13] net: dsa: add driver for MaxLinear GSW1xx switch family

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: <aP6395j1lbzld4U6@makrotopia.org>
Date: Mon, 27 Oct 2025 00:08:23 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Hauke Mehrtens <hauke@...ke-m.de>, Andrew Lunn <andrew@...n.ch>,
	Vladimir Oltean <olteanv@...il.com>,
	"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>, Simon Horman <horms@...nel.org>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Andreas Schirm <andreas.schirm@...mens.com>,
	Lukas Stockmann <lukas.stockmann@...mens.com>,
	Alexander Sverdlin <alexander.sverdlin@...mens.com>,
	Peter Christen <peter.christen@...mens.com>,
	Avinash Jayaraman <ajayaraman@...linear.com>,
	Bing tao Xu <bxu@...linear.com>, Liang Xu <lxu@...linear.com>,
	Juraj Povazanec <jpovazanec@...linear.com>,
	"Fanni (Fang-Yi) Chan" <fchan@...linear.com>,
	"Benny (Ying-Tsan) Weng" <yweng@...linear.com>,
	"Livia M. Rosu" <lrosu@...linear.com>,
	John Crispin <john@...ozen.org>
Subject: Re: [PATCH net-next v2 13/13] net: dsa: add driver for MaxLinear
 GSW1xx switch family

Hi Russell,

thank you for the review and for not getting tired to teach me ;)

On Sat, Oct 25, 2025 at 07:44:11PM +0100, Russell King (Oracle) wrote:
> On Sat, Oct 25, 2025 at 03:51:23PM +0100, Daniel Golle wrote:
> > [...]
> > +	/* Assert and deassert SGMII shell reset */
> > +	ret = regmap_set_bits(priv->shell, GSW1XX_SHELL_RST_REQ,
> > +			      GSW1XX_RST_REQ_SGMII_SHELL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_clear_bits(priv->shell, GSW1XX_SHELL_RST_REQ,
> > +				GSW1XX_RST_REQ_SGMII_SHELL);
> > +	if (ret < 0)
> > +		return ret;
> 
> So this is disruptive. Overall, at this point, having added every other
> comment below, this code has me wondering whether you are aware of the
> documentation I have written in phylink.h for pcs_config(). This code
> goes against this paragraph in that documentation:
> 
> "
>  * pcs_config() will be called when configuration of the PCS is required
>  * or when the advertisement is possibly updated. It must not unnecessarily
>  * disrupt an established link.
> "
> 
> Low quality implementations lead to poor user experiences.

I've improved this in v3 which I have just sent, unless the TBI block came
out of reset or the interface mode had changed since the previous call to
.pcs_config() I'm now avoiding any disruptive operations.

> > [...]
> > +	if (interface == PHY_INTERFACE_MODE_SGMII) {
> > +		txaneg = ADVERTISE_SGMII;
> > +		if (sgmii_mac_mode) {
> > +			txaneg |= BIT(14); /* MAC should always send BIT 14 */
> 
> Bit 14 is ADVERTISE_LPACK.

Thanks for that, always learning ;)

> 
> I think I'd prefer:
> 
> 			txaneg = ADVERTISE_SGMII | ADVERTISE_LPACK;
> 
> and...
> 
> > +			anegctl |= FIELD_PREP(GSW1XX_SGMII_TBI_ANEGCTL_ANMODE,
> > +					      GSW1XX_SGMII_TBI_ANEGCTL_ANMODE_SGMII_MAC);
> > +		} else {
> > +			txaneg |= LPA_SGMII_1000FULL;
> 
> 			txaneg = LPA_SGMII | LPA_SGMII_1000FULL;
> 
> here.

Ack.

> 
> > +			anegctl |= FIELD_PREP(GSW1XX_SGMII_TBI_ANEGCTL_ANMODE,
> > +					      GSW1XX_SGMII_TBI_ANEGCTL_ANMODE_SGMII_PHY);
> 
> So this seems to be yet another case of reverse SGMII. Andrew, please
> can we get to a conclusion on PHY_INTERFACE_MODE_REVSGMII before we
> end up with a crapshow of drivers doing their own stuff *exactly*
> like we see here?

I agree that PHY_INTERFACE_MODE_REVSGMII would make sense, and I have
now at least added a comment indicating that.
As on DSA switches it is very common for the same SerDes interface
being potentially used to connect a PHY or SFP cage, but be used as CPU
port, it will be important to clearly state which end of such links
is described as SGMII or REVSGMII, as one side is always MAC side and
the other side is PHY side, so its a bit ambigous to use the 'foward'
(aka. 'normal') vs. 'reverse' language...

> > [...] (regarding GSW1XX_SGMII_TBI_ANEGCTL_OVRANEG and
> >        GSW1XX_SGMII_TBI_ANEGCTL_OVRABL bits)
> Please add a comment describing what is going on here. What does this
> register bit do...

I've added a comment describing the override bits in detail and it
also turned out that it makes most sense to always set both override
bits.

> 
> > +	} else if (interface == PHY_INTERFACE_MODE_1000BASEX ||
> > +		   interface == PHY_INTERFACE_MODE_2500BASEX) {
> > +		txaneg = BIT(5) | BIT(7);
> 
> ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE ?

Actually phylink_mii_c22_pcs_encode_advertisement() seemed like a good
match here and I've used that for the Base-X modes.

> > [...]
> > +
> > +static const struct phylink_pcs_ops gsw1xx_sgmii_pcs_ops = {
> > +	.pcs_an_restart = gsw1xx_sgmii_pcs_an_restart,
> > +	.pcs_config = gsw1xx_sgmii_pcs_config,
> > +	.pcs_disable = gsw1xx_sgmii_pcs_disable,
> > +	.pcs_enable = gsw1xx_sgmii_pcs_enable,
> > +	.pcs_get_state = gsw1xx_sgmii_pcs_get_state,
> > +	.pcs_link_up = gsw1xx_sgmii_pcs_link_up,
> 
> Please order these in the same order as they appear in the struct, and
> please order your functions above in the same order. This makes it
> easier in future if new methods need to be added.

Ack, done.

> 
> Also, please add the .pcs_inband_caps method to describe the
> capabilities of the PCS.

Ack.

> 
> It seems to me that this is not just a Cisco SGMII PCS, but also
> supports IEEE 802.3 1000BASE-X. "SGMII" is an ambiguous term. Please
> avoid propagating this ambigutiy to the kernel. I think in this case
> merely "gsw1xx_pcs_xyz" will do.

I've renamed all functions according to your suggestions, but kept
register names as-is to still match how they are called in the (btw
public) datasheet.

> > [...]
> > +static struct phylink_pcs *gsw1xx_phylink_mac_select_pcs(struct phylink_config *config,
> > +							 phy_interface_t interface)
> > +{
> > +	struct dsa_port *dp = dsa_phylink_to_port(config);
> > +	struct gswip_priv *gswip_priv = dp->ds->priv;
> > +	struct gsw1xx_priv *gsw1xx_priv = container_of(gswip_priv,
> > +						       struct gsw1xx_priv,
> > +						       gswip);
> 
> Reverse christmas tree?

Not possible as each declaration uses the previously declared
variable in its initializer.


Cheers


Daniel