[PATCH v2 1/2] net: macb: Clean up macb_validate

[PATCH v2 1/2] net: macb: Clean up macb_validate

[Sean Anderson]

As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This patch was originally submitted as [1].

[1] https://lore.kernel.org/netdev/20211004191527.1610759-9-sean.anderson@seco.com/

Changes in v2:
- Fix polarity of `one` being inverted
- Only set gigabit modes for NA if macb_is_gem()

 drivers/net/ethernet/cadence/macb_main.c | 94 ++++++++++++------------
 1 file changed, 45 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 683f14665c2c..a9105ec1b885 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -510,32 +510,55 @@ static void macb_validate(struct phylink_config *config,
 			  unsigned long *supported,
 			  struct phylink_link_state *state)
 {
+	bool one = state->interface != PHY_INTERFACE_MODE_NA;
 	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. */
-	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)) {
-		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
-		return;
-	}
-
-	if (!macb_is_gem(bp) &&
-	    (state->interface == PHY_INTERFACE_MODE_GMII ||
-	     phy_interface_mode_is_rgmii(state->interface))) {
-		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
-		return;
-	}
-
-	if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
-	    !(bp->caps & MACB_CAPS_HIGH_SPEED &&
-	      bp->caps & MACB_CAPS_PCS)) {
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_NA:
+	case PHY_INTERFACE_MODE_10GBASER:
+		if (bp->caps & MACB_CAPS_HIGH_SPEED &&
+		    bp->caps & MACB_CAPS_PCS &&
+		    bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+			phylink_set_10g_modes(mask);
+			phylink_set(mask, 10000baseKR_Full);
+		} else if (one) {
+			goto none;
+		}
+		if (one)
+			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:
+		if (!macb_is_gem(bp)) {
+			if (one)
+				goto none;
+			else
+				goto mii;
+		}
+		fallthrough;
+	case PHY_INTERFACE_MODE_SGMII:
+		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+			phylink_set(mask, 1000baseT_Full);
+			phylink_set(mask, 1000baseX_Full);
+			if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
+				phylink_set(mask, 1000baseT_Half);
+		}
+		fallthrough;
+	mii:
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_RMII:
+		phylink_set(mask, 10baseT_Half);
+		phylink_set(mask, 10baseT_Full);
+		phylink_set(mask, 100baseT_Half);
+		phylink_set(mask, 100baseT_Full);
+		break;
+	none:
+	default:
 		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
 		return;
 	}
@@ -543,33 +566,6 @@ static void macb_validate(struct phylink_config *config,
 	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)) {
-		phylink_set_10g_modes(mask);
-		phylink_set(mask, 10000baseKR_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);
-
-	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);
-	}
-out:
 	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
 	bitmap_and(state->advertising, state->advertising, mask,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);
-- 
2.25.1

[PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate

[Sean Anderson]

This aligns mac_validate with mac_config. In mac_config, SGMII is only
enabled if macb_is_gem. Validate should care if the mac is a gem as
well. This also simplifies the logic now that all gigabit modes depend
on the mac being a GEM.

Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- New

 drivers/net/ethernet/cadence/macb_main.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a9105ec1b885..ae8c969a609c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -534,22 +534,18 @@ static void macb_validate(struct phylink_config *config,
 	case PHY_INTERFACE_MODE_RGMII_ID:
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		if (!macb_is_gem(bp)) {
-			if (one)
-				goto none;
-			else
-				goto mii;
-		}
-		fallthrough;
 	case PHY_INTERFACE_MODE_SGMII:
-		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
-			phylink_set(mask, 1000baseT_Full);
-			phylink_set(mask, 1000baseX_Full);
-			if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
-				phylink_set(mask, 1000baseT_Half);
+		if (macb_is_gem(bp)) {
+			if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+				phylink_set(mask, 1000baseT_Full);
+				phylink_set(mask, 1000baseX_Full);
+				if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
+					phylink_set(mask, 1000baseT_Half);
+			}
+		} else if (one) {
+			goto none;
 		}
 		fallthrough;
-	mii:
 	case PHY_INTERFACE_MODE_MII:
 	case PHY_INTERFACE_MODE_RMII:
 		phylink_set(mask, 10baseT_Half);
-- 
2.25.1

Re: [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate

[Jakub Kicinski]

On Mon, 11 Oct 2021 12:55:17 -0400 Sean Anderson wrote:
> This aligns mac_validate with mac_config. In mac_config, SGMII is only
> enabled if macb_is_gem. Validate should care if the mac is a gem as
> well. This also simplifies the logic now that all gigabit modes depend
> on the mac being a GEM.
> 
> Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

Please make sure you CC the author of the original patch, they tend to
be a good person to review the change. Or at the very least validate
the Fixes tag. Adding Antoine.

>  drivers/net/ethernet/cadence/macb_main.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a9105ec1b885..ae8c969a609c 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -534,22 +534,18 @@ static void macb_validate(struct phylink_config *config,
>  	case PHY_INTERFACE_MODE_RGMII_ID:
>  	case PHY_INTERFACE_MODE_RGMII_RXID:
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		if (!macb_is_gem(bp)) {
> -			if (one)
> -				goto none;
> -			else
> -				goto mii;
> -		}
> -		fallthrough;
>  	case PHY_INTERFACE_MODE_SGMII:
> -		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
> -			phylink_set(mask, 1000baseT_Full);
> -			phylink_set(mask, 1000baseX_Full);
> -			if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
> -				phylink_set(mask, 1000baseT_Half);
> +		if (macb_is_gem(bp)) {
> +			if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
> +				phylink_set(mask, 1000baseT_Full);
> +				phylink_set(mask, 1000baseX_Full);
> +				if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
> +					phylink_set(mask, 1000baseT_Half);
> +			}
> +		} else if (one) {
> +			goto none;
>  		}
>  		fallthrough;
> -	mii:
>  	case PHY_INTERFACE_MODE_MII:
>  	case PHY_INTERFACE_MODE_RMII:
>  		phylink_set(mask, 10baseT_Half);

Re: [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate

[Antoine Tenart]

Hello Sean,

Quoting Jakub Kicinski (2021-10-12 02:17:59)
> On Mon, 11 Oct 2021 12:55:17 -0400 Sean Anderson wrote:
> > This aligns mac_validate with mac_config. In mac_config, SGMII is only
> > enabled if macb_is_gem. Validate should care if the mac is a gem as
> > well. This also simplifies the logic now that all gigabit modes depend
> > on the mac being a GEM.

This looks correct.

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

If this is a fix, the patch has to be first in the series as it is
depending on the first one which is not a fix.

Thanks,
Antoine

Re: [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate

[Sean Anderson]

On 10/12/21 4:27 AM, Antoine Tenart wrote:
> Hello Sean,
> 
> Quoting Jakub Kicinski (2021-10-12 02:17:59)
>> On Mon, 11 Oct 2021 12:55:17 -0400 Sean Anderson wrote:
>> > This aligns mac_validate with mac_config. In mac_config, SGMII is only
>> > enabled if macb_is_gem. Validate should care if the mac is a gem as
>> > well. This also simplifies the logic now that all gigabit modes depend
>> > on the mac being a GEM.
> 
> This looks correct.
> 
>> > Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> 
> If this is a fix, the patch has to be first in the series as it is
> depending on the first one which is not a fix.

Ok, I can put this first then.

--Sean

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Antoine Tenart]

Hello Sean,

Quoting Sean Anderson (2021-10-11 18:55:16)
> As the number of interfaces grows, the number of if statements grows
> ever more unweildy. Clean everything up a bit by using a switch
> statement. No functional change intended.

I'm not 100% convinced this makes macb_validate more readable: there are
lots of conditions, and jumps, in the switch.

Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).

(Also having patch 1 first will improve things).

Thanks,
Antoine

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Antoine Tenart]

Quoting Antoine Tenart (2021-10-12 10:33:04)
> Quoting Sean Anderson (2021-10-11 18:55:16)
> > As the number of interfaces grows, the number of if statements grows
> > ever more unweildy. Clean everything up a bit by using a switch
> > statement. No functional change intended.
> 
> I'm not 100% convinced this makes macb_validate more readable: there are
> lots of conditions, and jumps, in the switch.
> 
> Maybe you could try a mixed approach; keeping the invalid modes checks
> (bitmap_zero) at the beginning and once we know the mode is valid using
> a switch statement. That might make it easier to read as this should
> remove lots of conditionals. (We'll still have the one/_NA checks
> though).
> 
> (Also having patch 1 first will improve things).

Patch 2 *

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Nicolas Ferre]

On 12/10/2021 at 10:33, Antoine Tenart wrote:
> Hello Sean,
> 
> Quoting Sean Anderson (2021-10-11 18:55:16)
>> As the number of interfaces grows, the number of if statements grows
>> ever more unweildy. Clean everything up a bit by using a switch
>> statement. No functional change intended.
> 
> I'm not 100% convinced this makes macb_validate more readable: there are
> lots of conditions, and jumps, in the switch.

I agree with Antoine that the result is not much more readable.

Regards,
   Nicolas

> Maybe you could try a mixed approach; keeping the invalid modes checks
> (bitmap_zero) at the beginning and once we know the mode is valid using
> a switch statement. That might make it easier to read as this should
> remove lots of conditionals. (We'll still have the one/_NA checks
> though).
> 
> (Also having patch 1 first will improve things).
> 
> Thanks,
> Antoine
> 


-- 
Nicolas Ferre

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Sean Anderson]

On 10/12/21 4:33 AM, Antoine Tenart wrote:
> Hello Sean,
>
> Quoting Sean Anderson (2021-10-11 18:55:16)
>> As the number of interfaces grows, the number of if statements grows
>> ever more unweildy. Clean everything up a bit by using a switch
>> statement. No functional change intended.
>
> I'm not 100% convinced this makes macb_validate more readable: there are
> lots of conditions, and jumps, in the switch.

The conditions are necessary to determine if the mac actually supports
the mode being requested. The jumps are all forward, and all but one
could be replaced with

	bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
	return;

The idea being that the NA mode goes from top to bottom, and all the
other modes do as well.

> Maybe you could try a mixed approach; keeping the invalid modes checks
> (bitmap_zero) at the beginning and once we know the mode is valid using
> a switch statement. That might make it easier to read as this should
> remove lots of conditionals. (We'll still have the one/_NA checks
> though).

This is actually the issue I wanted to address. The interface checks are
effectively performed twice or sometimes three times. There are also
gotos in the original design to deal with e.g. 10GBASE not having
10/100/1000 modes. This makes it easy to introduce bugs when adding new
modes, such as what happened with SGMII.

> (Also having patch 1 first will improve things).

Yes. Some of the complexity is simply to deal with SGMII being a special
case.

--Sean

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Antoine Tenart]

Quoting Sean Anderson (2021-10-12 18:34:50)
> On 10/12/21 4:33 AM, Antoine Tenart wrote:
> > Quoting Sean Anderson (2021-10-11 18:55:16)
> >> As the number of interfaces grows, the number of if statements grows
> >> ever more unweildy. Clean everything up a bit by using a switch
> >> statement. No functional change intended.
> 
> > Maybe you could try a mixed approach; keeping the invalid modes checks
> > (bitmap_zero) at the beginning and once we know the mode is valid using
> > a switch statement. That might make it easier to read as this should
> > remove lots of conditionals. (We'll still have the one/_NA checks
> > though).
> 
> This is actually the issue I wanted to address. The interface checks are
> effectively performed twice or sometimes three times. There are also
> gotos in the original design to deal with e.g. 10GBASE not having
> 10/100/1000 modes. This makes it easy to introduce bugs when adding new
> modes, such as what happened with SGMII.

I don't think having 1) validity checks 2) availability checks is an
issue. It's a choice between having possible bugs because the two steps
aren't synced vs possible bugs because one of the multiple paths in the
switch gets slightly broken by a patch. IMHO the one easier to read and
follow should win here.

Antoine

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Russell King (Oracle)]

On Tue, Oct 12, 2021 at 10:33:04AM +0200, Antoine Tenart wrote:
> Hello Sean,
> 
> Quoting Sean Anderson (2021-10-11 18:55:16)
> > As the number of interfaces grows, the number of if statements grows
> > ever more unweildy. Clean everything up a bit by using a switch
> > statement. No functional change intended.
> 
> I'm not 100% convinced this makes macb_validate more readable: there are
> lots of conditions, and jumps, in the switch.
> 
> Maybe you could try a mixed approach; keeping the invalid modes checks
> (bitmap_zero) at the beginning and once we know the mode is valid using
> a switch statement. That might make it easier to read as this should
> remove lots of conditionals. (We'll still have the one/_NA checks
> though).

Some of this could be improved if we add the ability for a MAC to
specify the phy_interface_t modes that it supports as a bitmap
before calling phylink_create() - then we can have phylink check
that the mode is supported itself prior to calling the validate
handler.

You can find some patches that add the "supported_interfaces" masks
in git.armlinux.org.uk/linux-arm.git net-queue

and we could add to phylink_validate():

	if (!phy_interface_empty(pl->config->supported_interfaces) &&
	    !test_bit(state->interface, pl->config->supported_interfaces))
		return -EINVAL;

which should go a long way to simplifying a lot of these validation
implementations.

Any thoughts on that?

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

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Sean Anderson]

On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
> On Tue, Oct 12, 2021 at 10:33:04AM +0200, Antoine Tenart wrote:
>> Hello Sean,
>>
>> Quoting Sean Anderson (2021-10-11 18:55:16)
>> > As the number of interfaces grows, the number of if statements grows
>> > ever more unweildy. Clean everything up a bit by using a switch
>> > statement. No functional change intended.
>>
>> I'm not 100% convinced this makes macb_validate more readable: there are
>> lots of conditions, and jumps, in the switch.
>>
>> Maybe you could try a mixed approach; keeping the invalid modes checks
>> (bitmap_zero) at the beginning and once we know the mode is valid using
>> a switch statement. That might make it easier to read as this should
>> remove lots of conditionals. (We'll still have the one/_NA checks
>> though).
>
> Some of this could be improved if we add the ability for a MAC to
> specify the phy_interface_t modes that it supports as a bitmap
> before calling phylink_create() - then we can have phylink check
> that the mode is supported itself prior to calling the validate
> handler.
>
> You can find some patches that add the "supported_interfaces" masks
> in git.armlinux.org.uk/linux-arm.git net-queue
>
> and we could add to phylink_validate():
>
> 	if (!phy_interface_empty(pl->config->supported_interfaces) &&
> 	    !test_bit(state->interface, pl->config->supported_interfaces))
> 		return -EINVAL;
>
> which should go a long way to simplifying a lot of these validation
> implementations.
>
> Any thoughts on that?

IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
tends to add complexity to validate(), because we have a lot of code
like

	if (state->interface == PHY_INTERFACE_MODE_FOO) {
		if (we_support_foo())
			phylink_set(mask, Foo);
		else if (state->interface != PHY_INTERFACE_MODE_NA) {
			linkmode_zero(supported);
			return;
		}
	}

which gets even worse when we want to have different interfaces share
logic. IMO validate() could be much cleaner if we never called it with
NA and instead did something like

	if (state->interface == PHY_INTERFACE_MODE_NA) {
		unsigned long *original;

		linkmode_copy(original, supported);
		for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
			if (test_bit(i, pl->config->supported_interfaces)) {
				unsigned long *iface_mode;

				linkmode_copy(iface_mode, original);
				state->interface = i;
				pl->mac_ops->validate(pl->config, iface_mode, state);
				linkmode_or(supported, supported, iface_mode);
			}
		}
		state->interface = PHY_INTERFACE_MODE_NA;
	}

This of course can be done in addition to/instead of your above
suggestion. I suggested something like this in v3 of this series, but it
would be even better to do this on the phylink level.

--Sean

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Russell King (Oracle)]

On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote:
> On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
> > You can find some patches that add the "supported_interfaces" masks
> > in git.armlinux.org.uk/linux-arm.git net-queue
> > 
> > and we could add to phylink_validate():
> > 
> > 	if (!phy_interface_empty(pl->config->supported_interfaces) &&
> > 	    !test_bit(state->interface, pl->config->supported_interfaces))
> > 		return -EINVAL;
> > 
> > which should go a long way to simplifying a lot of these validation
> > implementations.
> > 
> > Any thoughts on that?
> 
> IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
> tends to add complexity to validate(), because we have a lot of code
> like
> 
> 	if (state->interface == PHY_INTERFACE_MODE_FOO) {
> 		if (we_support_foo())
> 			phylink_set(mask, Foo);
> 		else if (state->interface != PHY_INTERFACE_MODE_NA) {
> 			linkmode_zero(supported);
> 			return;
> 		}
> 	}
> 
> which gets even worse when we want to have different interfaces share
> logic.

There is always the option to use different operations structs if the
properties of the interfaces can be divided up in that way - and that
will probably be more efficient (not that the validate callback is a
performance critical path though.)

> IMO validate() could be much cleaner if we never called it with
> NA and instead did something like
> 
> 	if (state->interface == PHY_INTERFACE_MODE_NA) {
> 		unsigned long *original;
> 
> 		linkmode_copy(original, supported);
> 		for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
> 			if (test_bit(i, pl->config->supported_interfaces)) {
> 				unsigned long *iface_mode;
> 
> 				linkmode_copy(iface_mode, original);
> 				state->interface = i;
> 				pl->mac_ops->validate(pl->config, iface_mode, state);
> 				linkmode_or(supported, supported, iface_mode);
> 			}
> 		}
> 		state->interface = PHY_INTERFACE_MODE_NA;
> 	}
> 
> This of course can be done in addition to/instead of your above
> suggestion. I suggested something like this in v3 of this series, but it
> would be even better to do this on the phylink level.

In addition I think - I think we should use a non-empty
supported_interfaces as an indicator that we use the above, otherwise
we have to loop through all possible interface modes. That also
provides some encouragement to fill out the supported_interfaces
member.

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

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Sean Anderson]

On 10/14/21 7:08 PM, Russell King (Oracle) wrote:
> On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote:
>> On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
>> > You can find some patches that add the "supported_interfaces" masks
>> > in git.armlinux.org.uk/linux-arm.git net-queue
>> >
>> > and we could add to phylink_validate():
>> >
>> > 	if (!phy_interface_empty(pl->config->supported_interfaces) &&
>> > 	    !test_bit(state->interface, pl->config->supported_interfaces))
>> > 		return -EINVAL;
>> >
>> > which should go a long way to simplifying a lot of these validation
>> > implementations.
>> >
>> > Any thoughts on that?
>>
>> IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
>> tends to add complexity to validate(), because we have a lot of code
>> like
>>
>> 	if (state->interface == PHY_INTERFACE_MODE_FOO) {
>> 		if (we_support_foo())
>> 			phylink_set(mask, Foo);
>> 		else if (state->interface != PHY_INTERFACE_MODE_NA) {
>> 			linkmode_zero(supported);
>> 			return;
>> 		}
>> 	}
>>
>> which gets even worse when we want to have different interfaces share
>> logic.
>
> There is always the option to use different operations structs if the
> properties of the interfaces can be divided up in that way - and that
> will probably be more efficient (not that the validate callback is a
> performance critical path though.)
>
>> IMO validate() could be much cleaner if we never called it with
>> NA and instead did something like
>>
>> 	if (state->interface == PHY_INTERFACE_MODE_NA) {
>> 		unsigned long *original;
>>
>> 		linkmode_copy(original, supported);
>> 		for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
>> 			if (test_bit(i, pl->config->supported_interfaces)) {
>> 				unsigned long *iface_mode;
>>
>> 				linkmode_copy(iface_mode, original);
>> 				state->interface = i;
>> 				pl->mac_ops->validate(pl->config, iface_mode, state);
>> 				linkmode_or(supported, supported, iface_mode);
>> 			}
>> 		}
>> 		state->interface = PHY_INTERFACE_MODE_NA;
>> 	}
>>
>> This of course can be done in addition to/instead of your above
>> suggestion. I suggested something like this in v3 of this series, but it
>> would be even better to do this on the phylink level.
>
> In addition I think - I think we should use a non-empty
> supported_interfaces as an indicator that we use the above, otherwise
> we have to loop through all possible interface modes. That also
> provides some encouragement to fill out the supported_interfaces
> member.

I had a stab at this today [1], but it is only compile-tested. In order
to compile "net: phylink: use phy_interface_t bitmaps for optical
modules", I needed to run

	sed -ie 's/sfp_link_an_mode/cfg_link_an_mode/g' drivers/net/phy/phylink.c

Do you plan on making up a series for this? I think the end result is
much nicer that v3 of this series.

--Sean

[1] https://github.com/sean-anderson-seco/linux/commits/supported_interfaces_wip

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Russell King (Oracle)]

On Fri, Oct 15, 2021 at 06:28:10PM -0400, Sean Anderson wrote:
> On 10/14/21 7:08 PM, Russell King (Oracle) wrote:
> > On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote:
> > > On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
> > > > You can find some patches that add the "supported_interfaces" masks
> > > > in git.armlinux.org.uk/linux-arm.git net-queue
> > > >
> > > > and we could add to phylink_validate():
> > > >
> > > > 	if (!phy_interface_empty(pl->config->supported_interfaces) &&
> > > > 	    !test_bit(state->interface, pl->config->supported_interfaces))
> > > > 		return -EINVAL;
> > > >
> > > > which should go a long way to simplifying a lot of these validation
> > > > implementations.
> > > >
> > > > Any thoughts on that?
> > > 
> > > IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
> > > tends to add complexity to validate(), because we have a lot of code
> > > like
> > > 
> > > 	if (state->interface == PHY_INTERFACE_MODE_FOO) {
> > > 		if (we_support_foo())
> > > 			phylink_set(mask, Foo);
> > > 		else if (state->interface != PHY_INTERFACE_MODE_NA) {
> > > 			linkmode_zero(supported);
> > > 			return;
> > > 		}
> > > 	}
> > > 
> > > which gets even worse when we want to have different interfaces share
> > > logic.
> > 
> > There is always the option to use different operations structs if the
> > properties of the interfaces can be divided up in that way - and that
> > will probably be more efficient (not that the validate callback is a
> > performance critical path though.)
> > 
> > > IMO validate() could be much cleaner if we never called it with
> > > NA and instead did something like
> > > 
> > > 	if (state->interface == PHY_INTERFACE_MODE_NA) {
> > > 		unsigned long *original;
> > > 
> > > 		linkmode_copy(original, supported);
> > > 		for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
> > > 			if (test_bit(i, pl->config->supported_interfaces)) {
> > > 				unsigned long *iface_mode;
> > > 
> > > 				linkmode_copy(iface_mode, original);
> > > 				state->interface = i;
> > > 				pl->mac_ops->validate(pl->config, iface_mode, state);
> > > 				linkmode_or(supported, supported, iface_mode);
> > > 			}
> > > 		}
> > > 		state->interface = PHY_INTERFACE_MODE_NA;
> > > 	}
> > > 
> > > This of course can be done in addition to/instead of your above
> > > suggestion. I suggested something like this in v3 of this series, but it
> > > would be even better to do this on the phylink level.
> > 
> > In addition I think - I think we should use a non-empty
> > supported_interfaces as an indicator that we use the above, otherwise
> > we have to loop through all possible interface modes. That also
> > provides some encouragement to fill out the supported_interfaces
> > member.
> 
> I had a stab at this today [1], but it is only compile-tested. In order
> to compile "net: phylink: use phy_interface_t bitmaps for optical
> modules", I needed to run
> 
> 	sed -ie 's/sfp_link_an_mode/cfg_link_an_mode/g' drivers/net/phy/phylink.c
> 
> Do you plan on making up a series for this? I think the end result is
> much nicer that v3 of this series.

I have been working on it but haven't finished the patches yet. There's
a few issues that came up with e.g. DSA and mvneta being able to switch
between different speeds with some SFP modules that have needed other
tweaks.

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

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Russell King (Oracle)]

On Fri, Oct 15, 2021 at 11:47:30PM +0100, Russell King (Oracle) wrote:
> I have been working on it but haven't finished the patches yet. There's
> a few issues that came up with e.g. DSA and mvneta being able to switch
> between different speeds with some SFP modules that have needed other
> tweaks.

Okay, have a look at:

http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue

and the patches from "net: enetc: remove interface checks in
enetc_pl_mac_validate ()" down to the "net-merged" branch label.

That set of patches add the supported_interfaces bitmap, uses it for
validation purposes, converts all but one of the ethernet drivers
over to using it, and then simplifies the validate() implementations.

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

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Sean Anderson]

Hi Russell,

On 10/19/21 11:02 AM, Russell King (Oracle) wrote:
> On Fri, Oct 15, 2021 at 11:47:30PM +0100, Russell King (Oracle) wrote:
>> I have been working on it but haven't finished the patches yet. There's
>> a few issues that came up with e.g. DSA and mvneta being able to switch
>> between different speeds with some SFP modules that have needed other
>> tweaks.
>
> Okay, have a look at:
>
> http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
>
> and the patches from "net: enetc: remove interface checks in
> enetc_pl_mac_validate ()" down to the "net-merged" branch label.
>
> That set of patches add the supported_interfaces bitmap, uses it for
> validation purposes, converts all but one of the ethernet drivers
> over to using it, and then simplifies the validate() implementations.
>

For "net: phy: add phy_interface_t bitmap support", phylink_or would be
nice as well. I use it when implementing NA support for PCSs.

For "net: sfp: augment SFP parsing with phy_interface_t bitmap",
drivers/net/phy/marvell.c also needs to be converted. This is due to
b697d9d38a5a ("net: phy: marvell: add SFP support for 88E1510") being
added to net-next/master.

(I think you have fixed this in your latest revision)

"net: phylink: use supported_interfaces for phylink validation" looks
good. Though the documentation should be updated. Perhaps something
like

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index bc4b866cd99b..a911872c12d8 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -134,11 +134,14 @@ struct phylink_mac_ops {
   * based on @state->advertising and/or @state->speed and update
   * @state->interface accordingly. See phylink_helper_basex_speed().
   *
- * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink expects the
- * MAC driver to return all supported link modes.
+ * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink expects the MAC
+ * driver to return all supported link modes. If @config->supported_interfaces
+ * is populated, phylink will handle this, and it is not necessary for
+ * validate() to support %PHY_INTERFACE_MODE_NA.
   *
- * If the @state->interface mode is not supported, then the @supported
- * mask must be cleared.
+ * If the @state->interface mode is not supported, then the @supported mask
+ * must be cleared. If @config->supported_interfaces is populated, validate()
+ * will only be called with values of @state->interfaces present in the bitmap.
   */
  void validate(struct phylink_config *config, unsigned long *supported,
               struct phylink_link_state *state);
--

I think "net: macb: populate supported_interfaces member" is wrong.
Gigabit modes should be predicated on GIGABIT_MODE_AVAILABLE. I know you
leave the check in validate(), but this is the sort of thing which
should be put in supported interfaces. Additionally, SGMII should
depend on PCS. So something like

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c1f976a79a44..02eff23adcfb 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -880,6 +880,7 @@ static void macb_get_pcs_fixed_state(struct phylink_config *config,
  static int macb_mii_probe(struct net_device *dev)
  {
         struct macb *bp = netdev_priv(dev);
+       unsigned long *supported = bp->phylink_config.supported_interfaces;

         bp->phylink_config.dev = &dev->dev;
         bp->phylink_config.type = PHYLINK_NETDEV;
@@ -889,6 +890,21 @@ static int macb_mii_probe(struct net_device *dev)
                 bp->phylink_config.get_fixed_state = macb_get_pcs_fixed_state;
         }

+       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);
+
         bp->phylink = phylink_create(&bp->phylink_config, bp->pdev->dev.fwnode,
                                      bp->phy_interface, &macb_phylink_ops);
         if (IS_ERR(bp->phylink)) {
--

Other than that, the commits in the range you mentioned above looks good
to me. For reference, my working branch with the above changes applied is [1].

[1] https://github.com/sean-anderson-seco/linux/tree/rking

--Sean

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Russell King (Oracle)]

On Fri, Oct 22, 2021 at 01:37:34PM -0400, Sean Anderson wrote:
> Hi Russell,
> 
> For "net: phy: add phy_interface_t bitmap support", phylink_or would be
> nice as well. I use it when implementing NA support for PCSs.

I think you actually mean phy_interface_or(). Given that we will need
MAC drivers to provide the union of their PCS support, I think that
would be a sensible addition. Thanks.

> For "net: sfp: augment SFP parsing with phy_interface_t bitmap",
> drivers/net/phy/marvell.c also needs to be converted. This is due to
> b697d9d38a5a ("net: phy: marvell: add SFP support for 88E1510") being
> added to net-next/master.
> 
> (I think you have fixed this in your latest revision)

I haven't - but when I move the patch series onto net-next, that will
need to be updated.

> "net: phylink: use supported_interfaces for phylink validation" looks
> good. Though the documentation should be updated. Perhaps something
> like

Yes, I haven't bothered with the doc updates yet... they will need to
be done before the patches are ready. Thanks for the suggestions though.

> I think "net: macb: populate supported_interfaces member" is wrong.
> Gigabit modes should be predicated on GIGABIT_MODE_AVAILABLE.

It is a conversion of what macb_validate() does - if the conversion is
incorrect, then macb_validate() is incorrect.

If MACB_CAPS_GIGABIT_MODE_AVAILABLE isn't set, but MACB_CAPS_HIGH_SPEED
and MACB_CAPS_PCS are both set, macb_validate() will not zero the
supported mask if e.g. PHY_INTERFACE_MODE_10GBASER is requested - it
will indicate 10baseT and 100baseT speeds are supported. So the current
macb_validate() code basically tells phylink that
PHY_INTERFACE_MODE_10GBASER supports 10baseT and 100baseT speeds! 

This probably is not what is intended, but this is what the code does,
and I'm maintaining bug-compatibility with the current macb_validate()
implementation. Any changes to the behaviour should be a separate
patch - either fixing it before this patch, or fixing it afterwards.
As the series is currently based on v5.14, it may be that this has
already been fixed.

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

Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Sean Anderson]

On 10/25/21 6:35 AM, Russell King (Oracle) wrote:
> On Fri, Oct 22, 2021 at 01:37:34PM -0400, Sean Anderson wrote:
>> Hi Russell,
>>
>> For "net: phy: add phy_interface_t bitmap support", phylink_or would be
>> nice as well. I use it when implementing NA support for PCSs.
>
> I think you actually mean phy_interface_or(). Given that we will need
> MAC drivers to provide the union of their PCS support, I think that
> would be a sensible addition. Thanks.
>
>> For "net: sfp: augment SFP parsing with phy_interface_t bitmap",
>> drivers/net/phy/marvell.c also needs to be converted. This is due to
>> b697d9d38a5a ("net: phy: marvell: add SFP support for 88E1510") being
>> added to net-next/master.
>>
>> (I think you have fixed this in your latest revision)
>
> I haven't - but when I move the patch series onto net-next, that will
> need to be updated.
>
>> "net: phylink: use supported_interfaces for phylink validation" looks
>> good. Though the documentation should be updated. Perhaps something
>> like
>
> Yes, I haven't bothered with the doc updates yet... they will need to
> be done before the patches are ready. Thanks for the suggestions though.
>
>> I think "net: macb: populate supported_interfaces member" is wrong.
>> Gigabit modes should be predicated on GIGABIT_MODE_AVAILABLE.
>
> It is a conversion of what macb_validate() does - if the conversion is
> incorrect, then macb_validate() is incorrect.
>
> If MACB_CAPS_GIGABIT_MODE_AVAILABLE isn't set, but MACB_CAPS_HIGH_SPEED
> and MACB_CAPS_PCS are both set, macb_validate() will not zero the
> supported mask if e.g. PHY_INTERFACE_MODE_10GBASER is requested - it
> will indicate 10baseT and 100baseT speeds are supported. So the current
> macb_validate() code basically tells phylink that
> PHY_INTERFACE_MODE_10GBASER supports 10baseT and 100baseT speeds!
>
> This probably is not what is intended, but this is what the code does,
> and I'm maintaining bug-compatibility with the current macb_validate()
> implementation. Any changes to the behaviour should be a separate
> patch - either fixing it before this patch, or fixing it afterwards.
> As the series is currently based on v5.14, it may be that this has
> already been fixed.

Ugh. This sort of thing is what I wanted to address in the first place.
The current logic lends itself well to these sorts of errors.

--Sean