Re: [PATCH v4] net: macb: Fix several edge cases in validate

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Oct 26, 2021 at 06:37:18PM +0200, Nicolas Ferre wrote:
> I like it as well. Thanks a lot for these enhancements.

So, would this work - have I understood all these capability flags
correctly? To aid this process, I've included the resulting code
below as well as the diff.

Should we only be setting have_10g if _all_ of MACB_CAPS_HIGH_SPEED,
MACB_CAPS_PCS, and MACB_CAPS_GIGABIT_MODE_AVAILABLE are set and we
have gem, or is what I have below sufficient (does it matter if
MACB_CAPS_PCS is clear?)

static void macb_validate(struct phylink_config *config,
			  unsigned long *supported,
			  struct phylink_link_state *state)
{
	struct net_device *ndev = to_net_dev(config->dev);
	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
	bool have_1g = false, have_10g = false;
	struct macb *bp = netdev_priv(ndev);

	if (macb_is_gem(bp) &&
	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
		if (bp->caps & MACB_CAPS_PCS)
			have_1g = true;
		if (bp->caps & MACB_CAPS_HIGH_SPEED)
			have_10g = true;
	}

	switch (state->interface) {
	case PHY_INTERFACE_MODE_NA:
	case PHY_INTERFACE_MODE_MII:
	case PHY_INTERFACE_MODE_RMII:
		break;

	case PHY_INTERFACE_MODE_GMII:
	case PHY_INTERFACE_MODE_RGMII:
	case PHY_INTERFACE_MODE_RGMII_ID:
	case PHY_INTERFACE_MODE_RGMII_RXID:
	case PHY_INTERFACE_MODE_RGMII_TXID:
	case PHY_INTERFACE_MODE_SGMII:
		if (!have_1g) {
			linkmode_zero(supported);
			return;
		}
		break;

	case PHY_INTERFACE_MODE_10GBASER:
		if (!have_10g) {
			linkmode_zero(supported);
			return;
		}
		break;

	default:
		linkmode_zero(supported);
		return;
	}

	phylink_set_port_modes(mask);
	phylink_set(mask, Autoneg);
	phylink_set(mask, Asym_Pause);

	switch (state->interface) {
	case PHY_INTERFACE_MODE_NA:
	case PHY_INTERFACE_MODE_10GBASER:
		if (have_10g) {
			phylink_set_10g_modes(mask);
			phylink_set(mask, 10000baseKR_Full);
		}
		if (state->interface != PHY_INTERFACE_MODE_NA)
			break;
		fallthrough;

	case PHY_INTERFACE_MODE_GMII:
	case PHY_INTERFACE_MODE_RGMII:
	case PHY_INTERFACE_MODE_RGMII_ID:
	case PHY_INTERFACE_MODE_RGMII_RXID:
	case PHY_INTERFACE_MODE_RGMII_TXID:
	case PHY_INTERFACE_MODE_SGMII:
		if (have_1g) {
			phylink_set(mask, 1000baseT_Full);
			phylink_set(mask, 1000baseX_Full);

			if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
				phylink_set(mask, 1000baseT_Half);
		}
		fallthrough;

	default:
		phylink_set(mask, 10baseT_Half);
		phylink_set(mask, 10baseT_Full);
		phylink_set(mask, 100baseT_Half);
		phylink_set(mask, 100baseT_Full);
		break;
	}

	linkmode_and(supported, supported, mask);
	linkmode_and(state->advertising, state->advertising, mask);
}


diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 309371abfe23..36fcd5563a71 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -512,30 +512,43 @@ static void macb_validate(struct phylink_config *config,
 {
 	struct net_device *ndev = to_net_dev(config->dev);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+	bool have_1g = false, have_10g = false;
 	struct macb *bp = netdev_priv(ndev);
 
-	/* We only support MII, RMII, GMII, RGMII & SGMII. */
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    state->interface != PHY_INTERFACE_MODE_MII &&
-	    state->interface != PHY_INTERFACE_MODE_RMII &&
-	    state->interface != PHY_INTERFACE_MODE_GMII &&
-	    state->interface != PHY_INTERFACE_MODE_SGMII &&
-	    state->interface != PHY_INTERFACE_MODE_10GBASER &&
-	    !phy_interface_mode_is_rgmii(state->interface)) {
-		linkmode_zero(supported);
-		return;
+	if (macb_is_gem(bp) &&
+	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
+		if (bp->caps & MACB_CAPS_PCS)
+			have_1g = true;
+		if (bp->caps & MACB_CAPS_HIGH_SPEED)
+			have_10g = true;
 	}
 
-	if (!macb_is_gem(bp) &&
-	    (state->interface == PHY_INTERFACE_MODE_GMII ||
-	     phy_interface_mode_is_rgmii(state->interface))) {
-		linkmode_zero(supported);
-		return;
-	}
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_NA:
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_RMII:
+		break;
+
+	case PHY_INTERFACE_MODE_GMII:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_SGMII:
+		if (!have_1g) {
+			linkmode_zero(supported);
+			return;
+		}
+		break;
+
+	case PHY_INTERFACE_MODE_10GBASER:
+		if (!have_10g) {
+			linkmode_zero(supported);
+			return;
+		}
+		break;
 
-	if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
-	    !(bp->caps & MACB_CAPS_HIGH_SPEED &&
-	      bp->caps & MACB_CAPS_PCS)) {
+	default:
 		linkmode_zero(supported);
 		return;
 	}
@@ -544,32 +557,40 @@ static void macb_validate(struct phylink_config *config,
 	phylink_set(mask, Autoneg);
 	phylink_set(mask, Asym_Pause);
 
-	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
-	    (state->interface == PHY_INTERFACE_MODE_NA ||
-	     state->interface == PHY_INTERFACE_MODE_10GBASER)) {
-		phylink_set_10g_modes(mask);
-		phylink_set(mask, 10000baseKR_Full);
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_NA:
+	case PHY_INTERFACE_MODE_10GBASER:
+		if (have_10g) {
+			phylink_set_10g_modes(mask);
+			phylink_set(mask, 10000baseKR_Full);
+		}
 		if (state->interface != PHY_INTERFACE_MODE_NA)
-			goto out;
-	}
+			break;
+		fallthrough;
+
+	case PHY_INTERFACE_MODE_GMII:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_SGMII:
+		if (have_1g) {
+			phylink_set(mask, 1000baseT_Full);
+			phylink_set(mask, 1000baseX_Full);
+
+			if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
+				phylink_set(mask, 1000baseT_Half);
+		}
+		fallthrough;
 
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-
-	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
-	    (state->interface == PHY_INTERFACE_MODE_NA ||
-	     state->interface == PHY_INTERFACE_MODE_GMII ||
-	     state->interface == PHY_INTERFACE_MODE_SGMII ||
-	     phy_interface_mode_is_rgmii(state->interface))) {
-		phylink_set(mask, 1000baseT_Full);
-		phylink_set(mask, 1000baseX_Full);
-
-		if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
-			phylink_set(mask, 1000baseT_Half);
+	default:
+		phylink_set(mask, 10baseT_Half);
+		phylink_set(mask, 10baseT_Full);
+		phylink_set(mask, 100baseT_Half);
+		phylink_set(mask, 100baseT_Full);
+		break;
 	}
-out:
+
 	linkmode_and(supported, supported, mask);
 	linkmode_and(state->advertising, state->advertising, mask);
 }

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