linux-kernel - Re: [RFC net-next PATCH 07/13] net: pcs: Add Xilinx PCS driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <528bb5d2-e0ce-4125-b11b-8b873230b0fc@linux.dev>
Date: Thu, 3 Apr 2025 16:51:39 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
 Christian Marangi <ansuelsmth@...il.com>, upstream@...oha.com,
 Heiner Kallweit <hkallweit1@...il.com>, Michal Simek <michal.simek@....com>,
 Radhey Shyam Pandey <radhey.shyam.pandey@....com>,
 Robert Hancock <robert.hancock@...ian.com>,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC net-next PATCH 07/13] net: pcs: Add Xilinx PCS driver

On 4/3/25 16:27, Russell King (Oracle) wrote:
> On Thu, Apr 03, 2025 at 02:19:01PM -0400, Sean Anderson wrote:
>> +static int xilinx_pcs_validate(struct phylink_pcs *pcs,
>> +			       unsigned long *supported,
>> +			       const struct phylink_link_state *state)
>> +{
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(xilinx_supported) = { 0 };
>> +
>> +	phylink_set_port_modes(xilinx_supported);
>> +	phylink_set(xilinx_supported, Autoneg);
>> +	phylink_set(xilinx_supported, Pause);
>> +	phylink_set(xilinx_supported, Asym_Pause);
>> +	switch (state->interface) {
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +		/* Half duplex not supported */
>> +		phylink_set(xilinx_supported, 10baseT_Full);
>> +		phylink_set(xilinx_supported, 100baseT_Full);
>> +		phylink_set(xilinx_supported, 1000baseT_Full);
>> +		break;
>> +	case PHY_INTERFACE_MODE_1000BASEX:
>> +		phylink_set(xilinx_supported, 1000baseX_Full);
>> +		break;
>> +	case PHY_INTERFACE_MODE_2500BASEX:
>> +		phylink_set(xilinx_supported, 2500baseX_Full);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	linkmode_and(supported, supported, xilinx_supported);
>> +	return 0;
> 
> You can not assume that an interface mode implies any particular media.
> For example, you can not assume that just because you have SGMII, that
> the only supported media is BaseT. This has been a fundamental principle
> in phylink's validation since day one.
> 
> Phylink documentation for the pcs_validate() callback states:
> 
>  * Validate the interface mode, and advertising's autoneg bit, removing any
>  * media ethtool link modes that would not be supportable from the supported
>  * mask. Phylink will propagate the changes to the advertising mask. See the
>  * &struct phylink_mac_ops validate() method.
> 
> and if we look at the MAC ops validate (before it was removed):
> 
> - * Clear bits in the @supported and @state->advertising masks that
> - * are not supportable by the MAC.
> - *
> - * Note that the PHY may be able to transform from one connection
> - * technology to another, so, eg, don't clear 1000BaseX just
> - * because the MAC is unable to BaseX mode. This is more about
> - * clearing unsupported speeds and duplex settings. The port modes
> - * should not be cleared; phylink_set_port_modes() will help with this.
> 
> PHYs can and do take SGMII and provide both BaseT and BaseX or BaseR
> connections. A PCS that is not directly media facing can not dictate
> the link modes.
> 

OK, how about this:

static int xilinx_pcs_validate(struct phylink_pcs *pcs,
			       unsigned long *supported,
			       const struct phylink_link_state *state)
{
	__ETHTOOL_DECLARE_LINK_MODE_MASK(xilinx_supported) = { 0 };
	unsigned long caps = phy_caps_from_interface(state->interface);

	phylink_set_port_modes(xilinx_supported);
	phylink_set(xilinx_supported, Autoneg);
	phylink_set(xilinx_supported, Pause);
	phylink_set(xilinx_supported, Asym_Pause);
	/* Half duplex not supported */
	caps &= ~(LINK_CAPA_10HD | LINK_CAPA_100HD | LINK_CAPA_1000HD);
	phy_caps_linkmodes(caps, xilinx_supported);
	linkmode_and(supported, supported, xilinx_supported);
	return 0;
}

--Sean