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

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

On Tue, Oct 26, 2021 at 11:30:08AM -0400, Sean Anderson wrote:
> I don't know if it's a "fix" per se. The current logic isn't wrong,
> since I believe that the configurations where the above patch would make
> a difference do not exist. However, as noted in the commit message, this
> makes refactoring difficult. For example, one might want to implement
> supported_interfaces like
> 
>        if (bp->caps & MACB_CAPS_HIGH_SPEED &&
>            bp->caps & MACB_CAPS_PCS)
>                __set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
>        if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
>                __set_bit(PHY_INTERFACE_MODE_GMII, supported);
> 		phy_interface_set_rgmii(supported);
>                if (bp->caps & MACB_CAPS_PCS)
>                        __set_bit(PHY_INTERFACE_MODE_SGMII, supported);
>        }
>        __set_bit(PHY_INTERFACE_MODE_MII, supported);
>        __set_bit(PHY_INTERFACE_MODE_RMII, supported);
> 
> but then you still need to check for GIGABIT_MODE in validate to
> determine whether 10GBASER should "support" 10/100. See [1] for more
> discussion.

However, 10GBASE-R doesn't support anything except 10G speeds, except
if the PHY itself does rate matching to achieve the slower speeds.
Then you need pause frames between the MAC and PHY to control the MAC
sending rate - which isn't something that the phylib model supports
particularly well.

The current code and your patched code conforms to this when
state->interface is 10GBASE-R _and_ MACB_CAPS_GIGABIT_MODE_AVAILABLE
is set, then we only end up with 10G linkmodes being allowed.

The problem case occurs in current code when
MACB_CAPS_GIGABIT_MODE_AVAILABLE is _not_ set, but state->interface
is 10GBASE-R. Current code ends up saying that 10GBASE-R supports
10/100 link modes, which is wrong.

The existing code:

        if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
            (state->interface == PHY_INTERFACE_MODE_NA ||
             state->interface == PHY_INTERFACE_MODE_10GBASER)) {
                phylink_set(mask, 10000baseCR_Full);
                phylink_set(mask, 10000baseER_Full);
                phylink_set(mask, 10000baseKR_Full);
                phylink_set(mask, 10000baseLR_Full);
                phylink_set(mask, 10000baseLRM_Full);
                phylink_set(mask, 10000baseSR_Full);
                phylink_set(mask, 10000baseT_Full);
                if (state->interface != PHY_INTERFACE_MODE_NA)
                        goto out;
        }

        phylink_set(mask, 10baseT_Half);
        phylink_set(mask, 10baseT_Full);
        phylink_set(mask, 100baseT_Half);
        phylink_set(mask, 100baseT_Full);

Would have been better written as:

	if (state->interface == PHY_INTERFACE_MODE_NA ||
	    state->interface == PHY_INTERFACE_MODE_10GBASER) {
		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
	                phylink_set(mask, 10000baseCR_Full);
	                phylink_set(mask, 10000baseER_Full);
	                phylink_set(mask, 10000baseKR_Full);
	                phylink_set(mask, 10000baseLR_Full);
	                phylink_set(mask, 10000baseLRM_Full);
	                phylink_set(mask, 10000baseSR_Full);
	                phylink_set(mask, 10000baseT_Full);
		}
                if (state->interface != PHY_INTERFACE_MODE_NA)
                        goto out;
        }

        phylink_set(mask, 10baseT_Half);
        phylink_set(mask, 10baseT_Full);
        phylink_set(mask, 100baseT_Half);
        phylink_set(mask, 100baseT_Full);

which would have avoided getting to the code that sets 10/100 link
modes when state->interface is 10GBASE-R.

The same transformation is probably applicable to the next if ()
block as well.

If I truely understood exactly what MACB_CAPS_GIGABIT_MODE_AVAILABLE,
MACB_CAPS_HIGH_SPEED, and MACB_CAPS_PCS were indicating and how they
relate to what is supported, I might be tempted to come up with a
better implementation myself. However, every time I look at the
existing code, it just confuses me - it seems to me that the use of
those capability bits is entirely inconsistent in the current
macb_validate() implementation.

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