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

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

[Sean Anderson]

There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.

This attempts to address these by reusing the same conditions used to
decide whether to return early when setting mode bits. The logic is
pretty messy, but this preserves the existing logic where possible.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v4:
- Drop cleanup patch

Changes in v3:
- Order bugfix patch first

Changes in v2:
- New

 drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 309371abfe23..40bd5a069368 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config,
 			  unsigned long *supported,
 			  struct phylink_link_state *state)
 {
+	bool have_1g = true, have_10g = true;
 	struct net_device *ndev = to_net_dev(config->dev);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct macb *bp = netdev_priv(ndev);
 
-	/* We only support MII, RMII, GMII, RGMII & SGMII. */
+	/* There are three major types of interfaces we support:
+	 * - (R)MII supporting 10/100 Mbit/s
+	 * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
+	 * - 10GBASER supporting 10 Gbit/s only
+	 */
 	if (state->interface != PHY_INTERFACE_MODE_NA &&
 	    state->interface != PHY_INTERFACE_MODE_MII &&
 	    state->interface != PHY_INTERFACE_MODE_RMII &&
@@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config,
 		return;
 	}
 
-	if (!macb_is_gem(bp) &&
-	    (state->interface == PHY_INTERFACE_MODE_GMII ||
-	     phy_interface_mode_is_rgmii(state->interface))) {
-		linkmode_zero(supported);
-		return;
+	/* For 1G and up we must have both have a GEM and GIGABIT_MODE */
+	if (!macb_is_gem(bp) ||
+	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
+		if (state->interface == PHY_INTERFACE_MODE_GMII ||
+		    phy_interface_mode_is_rgmii(state->interface) ||
+		    state->interface == PHY_INTERFACE_MODE_SGMII ||
+		    state->interface == PHY_INTERFACE_MODE_10GBASER) {
+			linkmode_zero(supported);
+			return;
+		} else if (state->interface == PHY_INTERFACE_MODE_NA) {
+			have_1g = false;
+			have_10g = false;
+		}
 	}
 
-	if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
-	    !(bp->caps & MACB_CAPS_HIGH_SPEED &&
-	      bp->caps & MACB_CAPS_PCS)) {
-		linkmode_zero(supported);
-		return;
+	/* We need a PCS for SGMII and 10GBASER */
+	if (!(bp->caps & MACB_CAPS_PCS)) {
+		if (state->interface == PHY_INTERFACE_MODE_SGMII ||
+		    state->interface == PHY_INTERFACE_MODE_10GBASER) {
+			linkmode_zero(supported);
+			return;
+		} else if (state->interface == PHY_INTERFACE_MODE_NA) {
+			have_10g = false;
+		}
+	}
+
+	/* And for 10G that PCS must be high-speed */
+	if (!(bp->caps & MACB_CAPS_HIGH_SPEED)) {
+		if (state->interface == PHY_INTERFACE_MODE_10GBASER) {
+			linkmode_zero(supported);
+			return;
+		} else if (state->interface == PHY_INTERFACE_MODE_NA) {
+			have_10g = false;
+		}
 	}
 
 	phylink_set_port_modes(mask);
 	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)) {
+	if ((state->interface == PHY_INTERFACE_MODE_NA ||
+	     state->interface == PHY_INTERFACE_MODE_10GBASER) && have_10g) {
 		phylink_set_10g_modes(mask);
 		phylink_set(mask, 10000baseKR_Full);
 		if (state->interface != PHY_INTERFACE_MODE_NA)
@@ -558,11 +584,10 @@ static void macb_validate(struct phylink_config *config,
 	phylink_set(mask, 100baseT_Half);
 	phylink_set(mask, 100baseT_Full);
 
-	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
-	    (state->interface == PHY_INTERFACE_MODE_NA ||
+	if ((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))) {
+	     phy_interface_mode_is_rgmii(state->interface)) && have_1g) {
 		phylink_set(mask, 1000baseT_Full);
 		phylink_set(mask, 1000baseX_Full);
 
-- 
2.25.1

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

[Russell King (Oracle)]

On Mon, Oct 25, 2021 at 01:24:05PM -0400, Sean Anderson wrote:
> There were several cases where validate() would return bogus supported
> modes with unusual combinations of interfaces and capabilities. For
> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
> another case, SGMII could be enabled even if the mac was not a GEM
> (despite this being checked for later on in mac_config()). These
> inconsistencies make it difficult to refactor this function cleanly.
> 
> This attempts to address these by reusing the same conditions used to
> decide whether to return early when setting mode bits. The logic is
> pretty messy, but this preserves the existing logic where possible.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v4:
> - Drop cleanup patch
> 
> Changes in v3:
> - Order bugfix patch first
> 
> Changes in v2:
> - New
> 
>  drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 309371abfe23..40bd5a069368 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config,
>  			  unsigned long *supported,
>  			  struct phylink_link_state *state)
>  {
> +	bool have_1g = true, have_10g = true;
>  	struct net_device *ndev = to_net_dev(config->dev);
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };

I think DaveM would ask for this to be reverse-christmas-tree, so the
new bool should be here.

>  	struct macb *bp = netdev_priv(ndev);
>  
> -	/* We only support MII, RMII, GMII, RGMII & SGMII. */
> +	/* There are three major types of interfaces we support:
> +	 * - (R)MII supporting 10/100 Mbit/s
> +	 * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
> +	 * - 10GBASER supporting 10 Gbit/s only
> +	 */
>  	if (state->interface != PHY_INTERFACE_MODE_NA &&
>  	    state->interface != PHY_INTERFACE_MODE_MII &&
>  	    state->interface != PHY_INTERFACE_MODE_RMII &&
> @@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config,
>  		return;
>  	}
>  
> -	if (!macb_is_gem(bp) &&
> -	    (state->interface == PHY_INTERFACE_MODE_GMII ||
> -	     phy_interface_mode_is_rgmii(state->interface))) {
> -		linkmode_zero(supported);
> -		return;
> +	/* For 1G and up we must have both have a GEM and GIGABIT_MODE */
> +	if (!macb_is_gem(bp) ||
> +	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
> +		if (state->interface == PHY_INTERFACE_MODE_GMII ||
> +		    phy_interface_mode_is_rgmii(state->interface) ||
> +		    state->interface == PHY_INTERFACE_MODE_SGMII ||
> +		    state->interface == PHY_INTERFACE_MODE_10GBASER) {
> +			linkmode_zero(supported);
> +			return;
> +		} else if (state->interface == PHY_INTERFACE_MODE_NA) {
> +			have_1g = false;
> +			have_10g = false;
> +		}
>  	}

Would it make more sense to do:

	bool have_1g = false, have_10g = false;

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

This uses positive logic to derive have_1g and have_10g, and then uses
the switch statement to validate against those. Would the above result
in more understandable code?

Thanks.

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

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

[Sean Anderson]

On 10/25/21 5:19 PM, Russell King (Oracle) wrote:
> On Mon, Oct 25, 2021 at 01:24:05PM -0400, Sean Anderson wrote:
>> There were several cases where validate() would return bogus supported
>> modes with unusual combinations of interfaces and capabilities. For
>> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
>> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
>> another case, SGMII could be enabled even if the mac was not a GEM
>> (despite this being checked for later on in mac_config()). These
>> inconsistencies make it difficult to refactor this function cleanly.
>>
>> This attempts to address these by reusing the same conditions used to
>> decide whether to return early when setting mode bits. The logic is
>> pretty messy, but this preserves the existing logic where possible.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>> Changes in v4:
>> - Drop cleanup patch
>>
>> Changes in v3:
>> - Order bugfix patch first
>>
>> Changes in v2:
>> - New
>>
>>  drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
>>  1 file changed, 42 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 309371abfe23..40bd5a069368 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config,
>>  			  unsigned long *supported,
>>  			  struct phylink_link_state *state)
>>  {
>> +	bool have_1g = true, have_10g = true;
>>  	struct net_device *ndev = to_net_dev(config->dev);
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>
> I think DaveM would ask for this to be reverse-christmas-tree, so the
> new bool should be here.

Ah, I wasn't aware that there was another variable-ordering style in use for net.

>>  	struct macb *bp = netdev_priv(ndev);
>>
>> -	/* We only support MII, RMII, GMII, RGMII & SGMII. */
>> +	/* There are three major types of interfaces we support:
>> +	 * - (R)MII supporting 10/100 Mbit/s
>> +	 * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
>> +	 * - 10GBASER supporting 10 Gbit/s only
>> +	 */
>>  	if (state->interface != PHY_INTERFACE_MODE_NA &&
>>  	    state->interface != PHY_INTERFACE_MODE_MII &&
>>  	    state->interface != PHY_INTERFACE_MODE_RMII &&
>> @@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config,
>>  		return;
>>  	}
>>
>> -	if (!macb_is_gem(bp) &&
>> -	    (state->interface == PHY_INTERFACE_MODE_GMII ||
>> -	     phy_interface_mode_is_rgmii(state->interface))) {
>> -		linkmode_zero(supported);
>> -		return;
>> +	/* For 1G and up we must have both have a GEM and GIGABIT_MODE */
>> +	if (!macb_is_gem(bp) ||
>> +	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
>> +		if (state->interface == PHY_INTERFACE_MODE_GMII ||
>> +		    phy_interface_mode_is_rgmii(state->interface) ||
>> +		    state->interface == PHY_INTERFACE_MODE_SGMII ||
>> +		    state->interface == PHY_INTERFACE_MODE_10GBASER) {
>> +			linkmode_zero(supported);
>> +			return;
>> +		} else if (state->interface == PHY_INTERFACE_MODE_NA) {
>> +			have_1g = false;
>> +			have_10g = false;
>> +		}
>>  	}
>
> Would it make more sense to do:
>
> 	bool have_1g = false, have_10g = false;
>
> 	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;
> 	}
>
> This uses positive logic to derive have_1g and have_10g, and then uses
> the switch statement to validate against those. Would the above result
> in more understandable code?

I experimented with something like the above, but I wasn't able to
express it cleanly. I think what you have would work nicely.

--Sean

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

[Jakub Kicinski]

On Mon, 25 Oct 2021 13:24:05 -0400 Sean Anderson wrote:
> There were several cases where validate() would return bogus supported
> modes with unusual combinations of interfaces and capabilities. For
> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
> another case, SGMII could be enabled even if the mac was not a GEM
> (despite this being checked for later on in mac_config()). These
> inconsistencies make it difficult to refactor this function cleanly.

Since you're respinning anyway (AFAIU) would you mind clarifying 
the fix vs refactoring question? Sounds like it could be a fix for 
the right (wrong?) PHY/MAC combination, but I don't think you're
intending it to be treated as a fix.

If it's a fix it needs [PATCH net] in the subject and a Fixes tag,
if it's not a fix it needs [PATCH net-next] in the subject.

This will make the lifes of maintainers and backporters easier, 
thanks :)

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

[Sean Anderson]

Hi Jakub,

On 10/25/21 8:44 PM, Jakub Kicinski wrote:
> On Mon, 25 Oct 2021 13:24:05 -0400 Sean Anderson wrote:
>> There were several cases where validate() would return bogus supported
>> modes with unusual combinations of interfaces and capabilities. For
>> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
>> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
>> another case, SGMII could be enabled even if the mac was not a GEM
>> (despite this being checked for later on in mac_config()). These
>> inconsistencies make it difficult to refactor this function cleanly.
>
> Since you're respinning anyway (AFAIU) would you mind clarifying
> the fix vs refactoring question? Sounds like it could be a fix for
> the right (wrong?) PHY/MAC combination, but I don't think you're
> intending it to be treated as a fix.
>
> If it's a fix it needs [PATCH net] in the subject and a Fixes tag,
> if it's not a fix it needs [PATCH net-next] in the subject.
>
> This will make the lifes of maintainers and backporters easier,
> thanks :)

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.

If you think this fixes a bug, then the appropriate tag is

Fixes: 7897b071ac3b ("net: macb: convert to phylink")

--Sean

[1] https://lore.kernel.org/netdev/YXaIWFB8Kx9rm%2Fj9@shell.armlinux.org.uk/

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

[Jakub Kicinski]

On Tue, 26 Oct 2021 11:30:08 -0400 Sean Anderson wrote:
> Hi Jakub,
> 
> On 10/25/21 8:44 PM, Jakub Kicinski wrote:
> > On Mon, 25 Oct 2021 13:24:05 -0400 Sean Anderson wrote:  
> >> There were several cases where validate() would return bogus supported
> >> modes with unusual combinations of interfaces and capabilities. For
> >> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
> >> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
> >> another case, SGMII could be enabled even if the mac was not a GEM
> >> (despite this being checked for later on in mac_config()). These
> >> inconsistencies make it difficult to refactor this function cleanly.  
> >
> > Since you're respinning anyway (AFAIU) would you mind clarifying
> > the fix vs refactoring question? Sounds like it could be a fix for
> > the right (wrong?) PHY/MAC combination, but I don't think you're
> > intending it to be treated as a fix.
> >
> > If it's a fix it needs [PATCH net] in the subject and a Fixes tag,
> > if it's not a fix it needs [PATCH net-next] in the subject.
> >
> > This will make the lifes of maintainers and backporters easier,
> > thanks :)  
> 
> 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.

Ok, unless one of the PHY experts can help us make a call let's go 
for net-next and no Fixes tag. 

> 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.
> 
> If you think this fixes a bug, then the appropriate tag is
> 
> Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> 
> --Sean
> 
> [1] https://lore.kernel.org/netdev/YXaIWFB8Kx9rm%2Fj9@shell.armlinux.org.uk/

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

[Russell King (Oracle)]

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!

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

[Nicolas Ferre]

On 26/10/2021 at 17:39, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, 26 Oct 2021 11:30:08 -0400 Sean Anderson wrote:
>> Hi Jakub,
>>
>> On 10/25/21 8:44 PM, Jakub Kicinski wrote:
>>> On Mon, 25 Oct 2021 13:24:05 -0400 Sean Anderson wrote:
>>>> There were several cases where validate() would return bogus supported
>>>> modes with unusual combinations of interfaces and capabilities. For
>>>> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
>>>> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
>>>> another case, SGMII could be enabled even if the mac was not a GEM
>>>> (despite this being checked for later on in mac_config()). These
>>>> inconsistencies make it difficult to refactor this function cleanly.
>>>
>>> Since you're respinning anyway (AFAIU) would you mind clarifying
>>> the fix vs refactoring question? Sounds like it could be a fix for
>>> the right (wrong?) PHY/MAC combination, but I don't think you're
>>> intending it to be treated as a fix.
>>>
>>> If it's a fix it needs [PATCH net] in the subject and a Fixes tag,
>>> if it's not a fix it needs [PATCH net-next] in the subject.
>>>
>>> This will make the lifes of maintainers and backporters easier,
>>> thanks :)
>>
>> 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.
> 
> Ok, unless one of the PHY experts can help us make a call let's go
> for net-next and no Fixes tag.

Agreed.

Regards,
   Nicolas

>> 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.
>>
>> If you think this fixes a bug, then the appropriate tag is
>>
>> Fixes: 7897b071ac3b ("net: macb: convert to phylink")
>>
>> --Sean
>>
>> [1] https://lore.kernel.org/netdev/YXaIWFB8Kx9rm%2Fj9@shell.armlinux.org.uk/
> 


-- 
Nicolas Ferre

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

[Nicolas Ferre]

On 25/10/2021 at 23:35, Sean Anderson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 10/25/21 5:19 PM, Russell King (Oracle) wrote:
>> On Mon, Oct 25, 2021 at 01:24:05PM -0400, Sean Anderson wrote:
>>> There were several cases where validate() would return bogus supported
>>> modes with unusual combinations of interfaces and capabilities. For
>>> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
>>> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
>>> another case, SGMII could be enabled even if the mac was not a GEM
>>> (despite this being checked for later on in mac_config()). These
>>> inconsistencies make it difficult to refactor this function cleanly.
>>>
>>> This attempts to address these by reusing the same conditions used to
>>> decide whether to return early when setting mode bits. The logic is
>>> pretty messy, but this preserves the existing logic where possible.
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>>
>>> Changes in v4:
>>> - Drop cleanup patch
>>>
>>> Changes in v3:
>>> - Order bugfix patch first
>>>
>>> Changes in v2:
>>> - New
>>>
>>>   drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
>>>   1 file changed, 42 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>> index 309371abfe23..40bd5a069368 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>> @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config,
>>>                         unsigned long *supported,
>>>                         struct phylink_link_state *state)
>>>   {
>>> +    bool have_1g = true, have_10g = true;
>>>       struct net_device *ndev = to_net_dev(config->dev);
>>>       __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>>
>> I think DaveM would ask for this to be reverse-christmas-tree, so the
>> new bool should be here.
> 
> Ah, I wasn't aware that there was another variable-ordering style in use for net.
> 
>>>       struct macb *bp = netdev_priv(ndev);
>>>
>>> -    /* We only support MII, RMII, GMII, RGMII & SGMII. */
>>> +    /* There are three major types of interfaces we support:
>>> +     * - (R)MII supporting 10/100 Mbit/s
>>> +     * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
>>> +     * - 10GBASER supporting 10 Gbit/s only
>>> +     */
>>>       if (state->interface != PHY_INTERFACE_MODE_NA &&
>>>           state->interface != PHY_INTERFACE_MODE_MII &&
>>>           state->interface != PHY_INTERFACE_MODE_RMII &&
>>> @@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config,
>>>               return;
>>>       }
>>>
>>> -    if (!macb_is_gem(bp) &&
>>> -        (state->interface == PHY_INTERFACE_MODE_GMII ||
>>> -         phy_interface_mode_is_rgmii(state->interface))) {
>>> -            linkmode_zero(supported);
>>> -            return;
>>> +    /* For 1G and up we must have both have a GEM and GIGABIT_MODE */
>>> +    if (!macb_is_gem(bp) ||
>>> +        (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
>>> +            if (state->interface == PHY_INTERFACE_MODE_GMII ||
>>> +                phy_interface_mode_is_rgmii(state->interface) ||
>>> +                state->interface == PHY_INTERFACE_MODE_SGMII ||
>>> +                state->interface == PHY_INTERFACE_MODE_10GBASER) {
>>> +                    linkmode_zero(supported);
>>> +                    return;
>>> +            } else if (state->interface == PHY_INTERFACE_MODE_NA) {
>>> +                    have_1g = false;
>>> +                    have_10g = false;
>>> +            }
>>>       }
>>
>> Would it make more sense to do:
>>
>>        bool have_1g = false, have_10g = false;
>>
>>        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;
>>        }
>>
>> This uses positive logic to derive have_1g and have_10g, and then uses
>> the switch statement to validate against those. Would the above result
>> in more understandable code?
> 
> I experimented with something like the above, but I wasn't able to
> express it cleanly. I think what you have would work nicely.

I like it as well. Thanks a lot for these enhancements.

Regards,
   Nicolas


-- 
Nicolas Ferre

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

[Russell King (Oracle)]

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!

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

[Sean Anderson]

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

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

[Russell King (Oracle)]

On Tue, Oct 26, 2021 at 01:28:15PM -0400, Sean Anderson wrote:
> 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

Hmm. That brings with it fundamental question: if the PCS supports 1G
only, does it _actually_ support Cisco SGMII, or does it only support
1000base-X?

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

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

[Sean Anderson]

On 10/26/21 1:46 PM, Russell King (Oracle) wrote:
> On Tue, Oct 26, 2021 at 01:28:15PM -0400, Sean Anderson wrote:
>> 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
> 
> Hmm. That brings with it fundamental question: if the PCS supports 1G
> only, does it _actually_ support Cisco SGMII, or does it only support
> 1000base-X?
> 

Of course, in the technical reference manual [1], they say

 > The line rate is 1 Gb/s as SGMII hardwired to function at 1 Gb/s only.
 > However, the data transfer rate can be forced down to 100 Mb/s or 10
 > Mb/s if the link partner is not capable.

which sounds like the normal byte-repetition of SGMII...

And they also talk about how the autonegotiation timeout and control words are different.

--Sean

[1] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

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

[Russell King (Oracle)]

On Tue, Oct 26, 2021 at 01:49:03PM -0400, Sean Anderson wrote:
> On 10/26/21 1:46 PM, Russell King (Oracle) wrote:
> > On Tue, Oct 26, 2021 at 01:28:15PM -0400, Sean Anderson wrote:
> > > 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
> > 
> > Hmm. That brings with it fundamental question: if the PCS supports 1G
> > only, does it _actually_ support Cisco SGMII, or does it only support
> > 1000base-X?
> > 
> 
> Of course, in the technical reference manual [1], they say
> 
> > The line rate is 1 Gb/s as SGMII hardwired to function at 1 Gb/s only.
> > However, the data transfer rate can be forced down to 100 Mb/s or 10
> > Mb/s if the link partner is not capable.
> 
> which sounds like the normal byte-repetition of SGMII...
> 
> And they also talk about how the autonegotiation timeout and control words are different.

This document looks a little comical. "GEM supports the serial gigabit
media-independent interface (SGMII, 1000BASE-SX and 1000BASE-LX) at
1000 Gb/s using the PS-GTR interface."

So:
a) it supports terabyte speeds?
b) it provides an optical connection direct from the SoC?
   (the L and S in 1000BASE-X refer to the laser wavelength!)

They really should just be saying "1000BASE-X" rather than specifying
an optical standard, but lets ignore that fundamental mistake for now.

In the section "SGMII, 1000BASE-SX, or 1000BASE-LX" it says:

"When bit [27] (SGMII mode) in the network configuration register
(GEM(0:3).network_config[sgmii_mode_enable]) is set, it changes the
behavior of the auto-negotiation advertisement and link partner ability
registers to meet the requirements of SGMII. Additionally, the time
duration of the link timer is reduced from 10ms to 1.6ms."

That bodes well for Cisco SGMII support, but it says nothing about how
that affects the PCS registers themselves.

As you say above, it goes on to say:

"The line rate is 1 Gb/s as SGMII hardwired to function at 1 GB/s
only."

That statement is confused. Cisco SGMII and 1000Base-X actually operate
at 1.25Gbaud line rate due to the 4B5B encoding on the Serdes. However,
the underlying data rate is 1Gbps, with 100 and 10Mbps achieved by
symbol replication of only the data portions of the packet transfer.
This replication does not, however, apply to non-data symbols though.

I suppose they _could_ have implemented Cisco SGMII by having the PCS
fixed in 1G mode, and then replicate the data prior to the PCS. That
would be rather peculiar though, and I'm not sure whether that could
work for the non-data symbols. I suppose it depends whether they just
slow down the transmission rate into the PCS, or do only data portion
replication before the PCS.


I've also just found the register listing in HTML form (so less
searchable than a PDF), and the PCS register listing only shows
1000base-X layout for the advertisement and link partner registers.
It seems to make no mention of "SGMII" mode.

So we have an open question: do 10 and 100M speeds actually work on
GEM, and if they do, how does one program it to operate at these
speeds. Currently, the driver seems to change bits in NCFGR to change
speed, and also reconfigure the transmit clock rate.

Going back to the first point I mentioned above, how much should we
take from these documents as actually being correct? Should we not
assume anything, but instead just experiment with the hardware and
see what works.

For example, are the two speed bits in the PCS control register
really read-only when in Cisco SGMII mode, or can they be changed -
and if they can be changed, does that have an effect on the ethernet
link?

Hmm, this seems to have uncovered more questions...

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

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

[Sean Anderson]

On 10/26/21 2:28 PM, Russell King (Oracle) wrote:
> On Tue, Oct 26, 2021 at 01:49:03PM -0400, Sean Anderson wrote:
>> On 10/26/21 1:46 PM, Russell King (Oracle) wrote:
>> > On Tue, Oct 26, 2021 at 01:28:15PM -0400, Sean Anderson wrote:
>> > > 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
>> >
>> > Hmm. That brings with it fundamental question: if the PCS supports 1G
>> > only, does it _actually_ support Cisco SGMII, or does it only support
>> > 1000base-X?
>> >
>>
>> Of course, in the technical reference manual [1], they say
>>
>> > The line rate is 1 Gb/s as SGMII hardwired to function at 1 Gb/s only.
>> > However, the data transfer rate can be forced down to 100 Mb/s or 10
>> > Mb/s if the link partner is not capable.
>>
>> which sounds like the normal byte-repetition of SGMII...
>>
>> And they also talk about how the autonegotiation timeout and control words are different.
>
> This document looks a little comical. "GEM supports the serial gigabit
> media-independent interface (SGMII, 1000BASE-SX and 1000BASE-LX) at
> 1000 Gb/s using the PS-GTR interface."
>
> So:
> a) it supports terabyte speeds?
> b) it provides an optical connection direct from the SoC?
>     (the L and S in 1000BASE-X refer to the laser wavelength!)
>
> They really should just be saying "1000BASE-X" rather than specifying
> an optical standard, but lets ignore that fundamental mistake for now.
>
> In the section "SGMII, 1000BASE-SX, or 1000BASE-LX" it says:
>
> "When bit [27] (SGMII mode) in the network configuration register
> (GEM(0:3).network_config[sgmii_mode_enable]) is set, it changes the
> behavior of the auto-negotiation advertisement and link partner ability
> registers to meet the requirements of SGMII. Additionally, the time
> duration of the link timer is reduced from 10ms to 1.6ms."
>
> That bodes well for Cisco SGMII support, but it says nothing about how
> that affects the PCS registers themselves.
>
> As you say above, it goes on to say:
>
> "The line rate is 1 Gb/s as SGMII hardwired to function at 1 GB/s
> only."
>
> That statement is confused. Cisco SGMII and 1000Base-X actually operate
> at 1.25Gbaud line rate due to the 4B5B encoding on the Serdes. However,
> the underlying data rate is 1Gbps, with 100 and 10Mbps achieved by
> symbol replication of only the data portions of the packet transfer.
> This replication does not, however, apply to non-data symbols though.
>
> I suppose they _could_ have implemented Cisco SGMII by having the PCS
> fixed in 1G mode, and then replicate the data prior to the PCS. That
> would be rather peculiar though, and I'm not sure whether that could
> work for the non-data symbols. I suppose it depends whether they just
> slow down the transmission rate into the PCS, or do only data portion
> replication before the PCS.
>
> I've also just found the register listing in HTML form (so less
> searchable than a PDF),

Unfortunately, AFAIK this is the only listing they have :l

(OTOH it is easy to link to)

> and the PCS register listing only shows
> 1000base-X layout for the advertisement and link partner registers.
> It seems to make no mention of "SGMII" mode.

The autonegotiation registers [2, 3] both mention SGMII "non SGMII".

The mux to turn on SGMII is in the network_config register [3].

[1] https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_an_adv.html
[2] https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_an_lp_base.html
[3] https://www.xilinx.com/html_docs/registers/ug1087/gem___network_config.html

> So we have an open question: do 10 and 100M speeds actually work on
> GEM, and if they do, how does one program it to operate at these
> speeds. Currently, the driver seems to change bits in NCFGR to change
> speed, and also reconfigure the transmit clock rate.
>
> Going back to the first point I mentioned above, how much should we
> take from these documents as actually being correct? Should we not
> assume anything, but instead just experiment with the hardware and
> see what works.

> For example, are the two speed bits in the PCS control register
> really read-only when in Cisco SGMII mode, or can they be changed -
> and if they can be changed, does that have an effect on the ethernet
> link?

Keep in mind that it is not only Zynq(MP) parts with have GEMs, but
several other SoCs as well. I have not reviewed their datasheets (except
for SiFive's which just say "go read the Linux driver"). It is possible
that other SoCs may not have these limitations. So any experimental
program will need to also experiment with e.g. sama.

Perhaps someone from cadence could comment on what is actually supported
by gem/macb?

--Sean

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

[Nicolas.Ferre]

Sean, Russell,

On 26/10/2021 at 20:52, Sean Anderson wrote:
>>
>> Going back to the first point I mentioned above, how much should we
>> take from these documents as actually being correct? Should we not
>> assume anything, but instead just experiment with the hardware and
>> see what works.
>> For example, are the two speed bits in the PCS control register
>> really read-only when in Cisco SGMII mode, or can they be changed -
>> and if they can be changed, does that have an effect on the ethernet
>> link?
> Keep in mind that it is not only Zynq(MP) parts with have GEMs, but
> several other SoCs as well. I have not reviewed their datasheets (except
> for SiFive's which just say "go read the Linux driver"). It is possible
> that other SoCs may not have these limitations. So any experimental
> program will need to also experiment with e.g. sama.

Claudiu and myself can certainly help in reviewing and testing on 
Microchip devices. The limitation that I see is that we only have 10/100 
and 1000 speeds implemented in our SoC with limited number of link types 
covered (MII, RMII, GMII and RGMII).

Our datasheets including different variants of MACB and GEM can give 
some information:
MACB example in: 
http://ww1.microchip.com/downloads/en/DeviceDoc/SAM9X60-Data-Sheet-DS60001579A.pdf

GEM example at 10/100 in: 
https://www.microchip.com/content/dam/mchp/documents/MPU32/ProductDocuments/DataSheets/SAMA5D2-Series-Data-sheet-ds60001476G.pdf

GEM example with 10/100/1000 in: 
https://www.microchip.com/content/dam/mchp/documents/MPU32/ProductDocuments/DataSheets/SAMA5D3-Series-Data-sheet-DS60001609b.pdf

We can also explore some of the design and configuration registers (DCFGn).

> Perhaps someone from cadence could comment on what is actually supported
> by gem/macb?

That could be good as well.

Best regards,
   Nicolas

-- 
Nicolas Ferre