[PATCH net-next 0/3] Better PHYLINK compliance for SJA1105 DSA

[PATCH net-next 0/3] Better PHYLINK compliance for SJA1105 DSA

[Vladimir Oltean]

After discussing with Russell King, it appears this driver is making a
few confusions and not performing some checks for consistent operation.

Vladimir Oltean (3):
  net: dsa: sja1105: Don't check state->link in phylink_mac_config
  net: dsa: sja1105: Check for PHY mode mismatches with what PHYLINK
    reports
  net: dsa: sja1105: Mark in-band AN modes not supported for PHYLINK

 drivers/net/dsa/sja1105/sja1105_main.c | 59 +++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] net: dsa: sja1105: Don't check state->link in phylink_mac_config

[Vladimir Oltean]

It has been pointed out that PHYLINK can call mac_config only to update
the phy_interface_type and without knowing what the AN results are.

Experimentally, when this was observed to happen, state->link was also
unset, and therefore was used as a proxy to ignore this call. However it
is also suggested that state->link is undefined for this callback and
should not be relied upon.

So let the previously-dead codepath for SPEED_UNKNOWN be called, and
update the comment to make sure the MAC's behavior is sane.

Cc: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index caebf76eaa3e..da1736093b06 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -715,7 +715,13 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
 
 	switch (speed_mbps) {
 	case SPEED_UNKNOWN:
-		/* No speed update requested */
+		/* PHYLINK called sja1105_mac_config() to inform us about
+		 * the state->interface, but AN has not completed and the
+		 * speed is not yet valid. UM10944.pdf says that setting
+		 * SJA1105_SPEED_AUTO at runtime disables the port, so that is
+		 * ok for power consumption in case AN will never complete -
+		 * otherwise PHYLINK should come back with a new update.
+		 */
 		speed = SJA1105_SPEED_AUTO;
 		break;
 	case SPEED_10:
@@ -766,9 +772,6 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port,
 {
 	struct sja1105_private *priv = ds->priv;
 
-	if (!state->link)
-		return;
-
 	sja1105_adjust_port_config(priv, port, state->speed);
 }
 
-- 
2.17.1

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

[Vladimir Oltean]

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));
+		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

[PATCH net-next 3/3] net: dsa: sja1105: Mark in-band AN modes not supported for PHYLINK

[Vladimir Oltean]

We need a better way to signal this, perhaps in phylink_validate, but
for now just print this error message as guidance for other people
looking at this driver's code while trying to rework PHYLINK.

Cc: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index ad4f604590c0..d82afb835fb7 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -806,6 +806,11 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port,
 	if (sja1105_phy_mode_mismatch(priv, port, state->interface))
 		return;
 
+	if (link_an_mode == MLO_AN_INBAND) {
+		dev_err(ds->dev, "In-band AN not supported!\n");
+		return;
+	}
+
 	sja1105_adjust_port_config(priv, port, state->speed);
 }
 
-- 
2.17.1

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

[Russell King - ARM Linux admin]

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