Re: [PATCH net-next 2/3] net: dsa: sja1105: Check for PHY mode mismatches with what PHYLINK reports

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Wed, Jun 26, 2019 at 02:20:13PM +0300, Vladimir Oltean wrote:
> PHYLINK being designed with PHYs in mind that can change MII protocol,
> for correct operation it is necessary to ensure that the PHY interface
> mode stays the same (otherwise clear the supported bit mask, as
> required).
> 
> Because this is just a hypothetical situation for now, we don't bother
> to check whether we could actually support the new PHY interface mode.
> Actually we could modify the xMII table, reset the switch and send an
> updated static configuration, but adding that would just be dead code.
> 
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  drivers/net/dsa/sja1105/sja1105_main.c | 47 ++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index da1736093b06..ad4f604590c0 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -766,12 +766,46 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
>  	return sja1105_clocking_setup_port(priv, port);
>  }
>  
> +/* The SJA1105 MAC programming model is through the static config (the xMII
> + * Mode table cannot be dynamically reconfigured), and we have to program
> + * that early (earlier than PHYLINK calls us, anyway).
> + * So just error out in case the connected PHY attempts to change the initial
> + * system interface MII protocol from what is defined in the DT, at least for
> + * now.
> + */
> +static bool sja1105_phy_mode_mismatch(struct sja1105_private *priv, int port,
> +				      phy_interface_t interface)
> +{
> +	struct sja1105_xmii_params_entry *mii;
> +	sja1105_phy_interface_t phy_mode;
> +
> +	mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
> +	phy_mode = mii->xmii_mode[port];
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_MII:
> +		return (phy_mode != XMII_MODE_MII);
> +	case PHY_INTERFACE_MODE_RMII:
> +		return (phy_mode != XMII_MODE_RMII);
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		return (phy_mode != XMII_MODE_RGMII);
> +	default:
> +		return true;
> +	}
> +}
> +
>  static void sja1105_mac_config(struct dsa_switch *ds, int port,
>  			       unsigned int link_an_mode,
>  			       const struct phylink_link_state *state)
>  {
>  	struct sja1105_private *priv = ds->priv;
>  
> +	if (sja1105_phy_mode_mismatch(priv, port, state->interface))
> +		return;
> +
>  	sja1105_adjust_port_config(priv, port, state->speed);
>  }
>  
> @@ -804,6 +838,19 @@ static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
>  
>  	mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
>  
> +	/* include/linux/phylink.h says:
> +	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
> +	 *     expects the MAC driver to return all supported link modes.
> +	 */
> +	if (state->interface != PHY_INTERFACE_MODE_NA &&
> +	    sja1105_phy_mode_mismatch(priv, port, state->interface)) {
> +		dev_warn(ds->dev, "PHY mode mismatch on port %d: "
> +			 "PHYLINK tried to change to %s\n",
> +			 port, phy_modes(state->interface));

Everything's fine except, please don't print to the kernel log for this.
You're just duplicating the prints in phylink.

> +		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +		return;
> +	}
> +
>  	/* The MAC does not support pause frames, and also doesn't
>  	 * support half-duplex traffic modes.
>  	 */
> -- 
> 2.17.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up