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

- From: Sean Anderson \<hidden\>
- Date: 2021-10-26 17:28:35 
- List: netdev
- Page: https://ratatoskr.run/netdev/2021/10/11214680
- Thread: https://ratatoskr.run/netdev/2021/10/11214465/t.md

```text
On 10/26/21 1:04 PM, Russell King (Oracle) wrote:
> 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;
> 	}

This is incorrect. (R)GMII does not need a PCS. Only SGMII needs one. So it really should be

 	if (macb_is_gem(bp) &&
 	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
 	 	have_1g = true;
 		if (bp->caps & MACB_CAPS_PCS)
 			have_sgmii = 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:

And then SGMII needs its own case here.

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

Actually, according to the Zynq UltraScale+ Devices Register Reference [1], the PCS does not support 10/100. So should SGMII even fall through here?

[1] https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_control.html

>
> 	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);
>   }
>
```