netdev - Re: [PATCH net-next v4 6/8] net: pcs: Add 10GBASE-R mode for Synopsys Designware XPCS

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: <ZFDDWrkiGzho1fA9@shell.armlinux.org.uk>
Date:   Tue, 2 May 2023 09:01:30 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Jiawen Wu <jiawenwu@...stnetic.com>
Cc:     netdev@...r.kernel.org, andrew@...n.ch,
        jarkko.nikula@...ux.intel.com, olteanv@...il.com,
        andriy.shevchenko@...ux.intel.com, hkallweit1@...il.com,
        linux-i2c@...r.kernel.org, linux-gpio@...r.kernel.org,
        mengyuanlou@...-swift.com, Jose Abreu <Jose.Abreu@...opsys.com>
Subject: Re: [PATCH net-next v4 6/8] net: pcs: Add 10GBASE-R mode for
 Synopsys Designware XPCS

On Sat, Apr 22, 2023 at 12:56:19PM +0800, Jiawen Wu wrote:
> Add basic support for XPCS using 10GBASE-R interface. This mode will
> be extended to use interrupt, so set pcs.poll false. And avoid soft
> reset so that the device using this mode is in the default configuration.
> 
> Cc: Jose Abreu <Jose.Abreu@...opsys.com>
> 
> Signed-off-by: Jiawen Wu <jiawenwu@...stnetic.com>
> ---
>  drivers/net/pcs/pcs-xpcs.c   | 56 ++++++++++++++++++++++++++++++++++++
>  include/linux/pcs/pcs-xpcs.h |  1 +
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 539cd43eae8d..9ddaceda1fe9 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -64,6 +64,16 @@ static const int xpcs_xlgmii_features[] = {
>  	__ETHTOOL_LINK_MODE_MASK_NBITS,
>  };
>  
> +static const int xpcs_10gbaser_features[] = {
> +	ETHTOOL_LINK_MODE_Pause_BIT,
> +	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +	ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseLR_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseER_Full_BIT,
> +	__ETHTOOL_LINK_MODE_MASK_NBITS,
> +};
> +
>  static const int xpcs_sgmii_features[] = {
>  	ETHTOOL_LINK_MODE_Pause_BIT,
>  	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> @@ -106,6 +116,10 @@ static const phy_interface_t xpcs_xlgmii_interfaces[] = {
>  	PHY_INTERFACE_MODE_XLGMII,
>  };
>  
> +static const phy_interface_t xpcs_10gbaser_interfaces[] = {
> +	PHY_INTERFACE_MODE_10GBASER,
> +};
> +
>  static const phy_interface_t xpcs_sgmii_interfaces[] = {
>  	PHY_INTERFACE_MODE_SGMII,
>  };
> @@ -123,6 +137,7 @@ enum {
>  	DW_XPCS_USXGMII,
>  	DW_XPCS_10GKR,
>  	DW_XPCS_XLGMII,
> +	DW_XPCS_10GBASER,
>  	DW_XPCS_SGMII,
>  	DW_XPCS_1000BASEX,
>  	DW_XPCS_2500BASEX,
> @@ -246,6 +261,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs,
>  
>  	switch (compat->an_mode) {
>  	case DW_AN_C73:
> +	case DW_10GBASER:
>  		dev = MDIO_MMD_PCS;
>  		break;
>  	case DW_AN_C37_SGMII:
> @@ -872,6 +888,8 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
>  		return -ENODEV;
>  
>  	switch (compat->an_mode) {
> +	case DW_10GBASER:
> +		break;
>  	case DW_AN_C73:
>  		if (phylink_autoneg_inband(mode)) {
>  			ret = xpcs_config_aneg_c73(xpcs, compat);
> @@ -919,6 +937,27 @@ static int xpcs_config(struct phylink_pcs *pcs, unsigned int mode,
>  	return xpcs_do_config(xpcs, interface, mode, advertising);
>  }
>  
> +static int xpcs_get_state_10gbaser(struct dw_xpcs *xpcs,
> +				   struct phylink_link_state *state)
> +{
> +	int ret;
> +
> +	state->link = false;
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & MDIO_STAT1_LSTATUS) {
> +		state->link = true;
> +		state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
> +		state->duplex = DUPLEX_FULL;
> +		state->speed = SPEED_10000;
> +	}
> +
> +	return 0;

This looks to me to be an almost duplicate of
phylink_mii_c45_pcs_get_state().

> +}
> +
>  static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
>  			      struct phylink_link_state *state,
>  			      const struct xpcs_compat *compat)
> @@ -1033,6 +1072,14 @@ static void xpcs_get_state(struct phylink_pcs *pcs,
>  		return;
>  
>  	switch (compat->an_mode) {
> +	case DW_10GBASER:
> +		ret = xpcs_get_state_10gbaser(xpcs, state);

This could then be simply:

		phylink_mii_c45_pcs_get_state(xpcs->mdiodev, state);

> +		if (ret) {
> +			pr_err("xpcs_get_state_10gbaser returned %pe\n",
> +			       ERR_PTR(ret));

Please avoid printing errors like this if we can't read the state. If we
become unable to read the state, then this message will flood the log at
the polling rate (if polling is enabled.)

I know the other cases here do, but they shouldn't.

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