netdev - Re: [PATCH net-next 4/4] enetc: Migrate to PHYLINK and PCS

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: <20201006060859.hapkjgcv4pwzhkrv@skbuf>
Date:   Tue, 6 Oct 2020 06:09:00 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Claudiu Manoil <claudiu.manoil@....com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next 4/4] enetc: Migrate to PHYLINK and PCS_LYNX

On Mon, Oct 05, 2020 at 05:28:18PM +0300, Claudiu Manoil wrote:
> This is a methodical transition of the driver from phylib
> to phylink, following the guidelines from sfp-phylink.rst.
> The MAC register configurations based on interface mode
> were moved from the probing path to the mac_config() hook.
> MAC enable and disable commands (enabling Rx and Tx paths
> at MAC level) were also extracted and assigned to their
> corresponding phylink hooks.
> As part of the migration to phylink, the serdes configuration
> from the driver was offloaded to the PCS_LYNX module,
> introduced in commit 0da4c3d393e4 ("net: phy: add Lynx PCS module"),
> the PCS_LYNX module being a mandatory component required to
> make the enetc driver work with phylink.
> 
> Signed-off-by: Claudiu Manoil <claudiu.manoil@....com>
> ---
>  drivers/net/ethernet/freescale/enetc/Kconfig  |   5 +-
>  drivers/net/ethernet/freescale/enetc/enetc.c  |  53 ++--
>  drivers/net/ethernet/freescale/enetc/enetc.h  |   9 +-
>  .../ethernet/freescale/enetc/enetc_ethtool.c  |  26 +-
>  .../net/ethernet/freescale/enetc/enetc_pf.c   | 246 +++++++++---------
>  .../net/ethernet/freescale/enetc/enetc_pf.h   |   8 +-
>  .../net/ethernet/freescale/enetc/enetc_qos.c  |   9 +-
>  7 files changed, 190 insertions(+), 166 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
> index 37b804f8bd76..0fa18b00c49b 100644
> --- a/drivers/net/ethernet/freescale/enetc/Kconfig
> +++ b/drivers/net/ethernet/freescale/enetc/Kconfig

(...)

> @@ -922,61 +889,118 @@ static void enetc_mdiobus_destroy(struct enetc_pf *pf)
>  	enetc_imdio_remove(pf);
>  }
>  
> -static void enetc_configure_sgmii(struct phy_device *pcs)
> +static void enetc_pl_mac_validate(struct phylink_config *config,
> +				  unsigned long *supported,
> +				  struct phylink_link_state *state)
>  {
> -	/* SGMII spec requires tx_config_Reg[15:0] to be exactly 0x4001
> -	 * for the MAC PCS in order to acknowledge the AN.
> -	 */
> -	phy_write(pcs, MII_ADVERTISE, ADVERTISE_SGMII | ADVERTISE_LPACK);
> +	struct enetc_pf *pf = phylink_to_enetc_pf(config);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	if (state->interface != PHY_INTERFACE_MODE_NA &&
> +	    state->interface != pf->if_mode) {
> +		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +		return;
> +	}
>  
> -	phy_write(pcs, ENETC_PCS_IF_MODE,
> -		  ENETC_PCS_IF_MODE_SGMII_EN |
> -		  ENETC_PCS_IF_MODE_USE_SGMII_AN);
> +	phylink_set_port_modes(mask);
> +	phylink_set(mask, Autoneg);
> +	phylink_set(mask, Pause);
> +	phylink_set(mask, Asym_Pause);
> +	phylink_set(mask, 10baseT_Half);
> +	phylink_set(mask, 10baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 100baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 1000baseT_Half);
> +	phylink_set(mask, 1000baseT_Full);
> +
> +	if (state->interface == PHY_INTERFACE_MODE_INTERNAL ||
> +	    state->interface == PHY_INTERFACE_MODE_2500BASEX ||
> +	    state->interface == PHY_INTERFACE_MODE_USXGMII) {
> +		phylink_set(mask, 2500baseT_Full);
> +		phylink_set(mask, 2500baseX_Full);
> +	}

Shouldn't the driver reject any interface mode which it does not
support? Either here in the validate callback or directly
where the pf->if_mode is set.

>  
> -	/* Adjust link timer for SGMII */
> -	phy_write(pcs, ENETC_PCS_LINK_TIMER1, ENETC_PCS_LINK_TIMER1_VAL);
> -	phy_write(pcs, ENETC_PCS_LINK_TIMER2, ENETC_PCS_LINK_TIMER2_VAL);
> +	bitmap_and(supported, supported, mask,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> +	bitmap_and(state->advertising, state->advertising, mask,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> +}
>  

(...)

>  
> -static void enetc_configure_serdes(struct enetc_pf *pf)
> +static const struct phylink_mac_ops enetc_mac_phylink_ops = {
> +	.validate = enetc_pl_mac_validate,
> +	.mac_config = enetc_pl_mac_config,
> +	.mac_link_up = enetc_pl_mac_link_up,
> +	.mac_link_down = enetc_pl_mac_link_down,
> +};
> +
> +static int enetc_phylink_create(struct enetc_ndev_priv *priv)
>  {
> -	switch (pf->if_mode) {
> -	case PHY_INTERFACE_MODE_SGMII:
> -		enetc_configure_sgmii(pf->pcs);
> -		break;
> -	case PHY_INTERFACE_MODE_2500BASEX:
> -		enetc_configure_2500basex(pf->pcs);
> -		break;
> -	case PHY_INTERFACE_MODE_USXGMII:
> -		enetc_configure_usxgmii(pf->pcs);
> -		break;
> -	default:
> -		dev_dbg(&pf->si->pdev->dev, "Unsupported link mode %s\n",
> -			phy_modes(pf->if_mode));
> +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> +	struct device *dev = &pf->si->pdev->dev;
> +	struct phylink *phylink;
> +	int err;
> +
> +	pf->phylink_config.dev = &priv->ndev->dev;
> +	pf->phylink_config.type = PHYLINK_NETDEV;
> +	if (enetc_port_has_pcs(pf))
> +		pf->phylink_config.pcs_poll = true;
> +

There is no need for the pcs_poll to be set by the driver, the lynx PCS
module will set it up for you.

Ioana