All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] net: macb: Clean up macb_validate
@ 2021-10-11 16:55 Sean Anderson
  2021-10-11 16:55 ` [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate Sean Anderson
  2021-10-12  8:33 ` [PATCH v2 1/2] net: macb: Clean up macb_validate Antoine Tenart
  0 siblings, 2 replies; 19+ messages in thread
From: Sean Anderson @ 2021-10-11 16:55 UTC (permalink / raw)
  To: netdev, David S . Miller, Jakub Kicinski
  Cc: Russell King, Nicolas Ferre, Claudiu Beznea, 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate
  2021-10-11 16:55 [PATCH v2 1/2] net: macb: Clean up macb_validate Sean Anderson
@ 2021-10-11 16:55 ` Sean Anderson
  2021-10-12  0:17   ` Jakub Kicinski
  2021-10-12  8:33 ` [PATCH v2 1/2] net: macb: Clean up macb_validate Antoine Tenart
  1 sibling, 1 reply; 19+ messages in thread
From: Sean Anderson @ 2021-10-11 16:55 UTC (permalink / raw)
  To: netdev, David S . Miller, Jakub Kicinski
  Cc: Russell King, Nicolas Ferre, Claudiu Beznea, 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate
  2021-10-11 16:55 ` [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate Sean Anderson
@ 2021-10-12  0:17   ` Jakub Kicinski
  2021-10-12  8:27     ` Antoine Tenart
  0 siblings, 1 reply; 19+ messages in thread
From: Jakub Kicinski @ 2021-10-12  0:17 UTC (permalink / raw)
  To: Sean Anderson
  Cc: netdev, David S . Miller, Russell King, Nicolas Ferre,
	Claudiu Beznea, Antoine Tenart

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate
  2021-10-12  0:17   ` Jakub Kicinski
@ 2021-10-12  8:27     ` Antoine Tenart
  2021-10-12 16:20       ` Sean Anderson
  0 siblings, 1 reply; 19+ messages in thread
From: Antoine Tenart @ 2021-10-12  8:27 UTC (permalink / raw)
  To: Jakub Kicinski, Sean Anderson
  Cc: netdev, David S . Miller, Russell King, Nicolas Ferre, Claudiu Beznea

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-11 16:55 [PATCH v2 1/2] net: macb: Clean up macb_validate Sean Anderson
  2021-10-11 16:55 ` [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate Sean Anderson
@ 2021-10-12  8:33 ` Antoine Tenart
  2021-10-12  8:34   ` Antoine Tenart
                     ` (3 more replies)
  1 sibling, 4 replies; 19+ messages in thread
From: Antoine Tenart @ 2021-10-12  8:33 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, Sean Anderson, netdev
  Cc: Russell King, Nicolas Ferre, Claudiu Beznea, Sean Anderson

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-12  8:33 ` [PATCH v2 1/2] net: macb: Clean up macb_validate Antoine Tenart
@ 2021-10-12  8:34   ` Antoine Tenart
  2021-10-12  9:24   ` Nicolas Ferre
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 19+ messages in thread
From: Antoine Tenart @ 2021-10-12  8:34 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, Sean Anderson, netdev
  Cc: Russell King, Nicolas Ferre, Claudiu Beznea, Sean Anderson

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 *

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-12  8:33 ` [PATCH v2 1/2] net: macb: Clean up macb_validate Antoine Tenart
  2021-10-12  8:34   ` Antoine Tenart
@ 2021-10-12  9:24   ` Nicolas Ferre
  2021-10-12 16:34   ` Sean Anderson
  2021-10-14 16:34   ` Russell King (Oracle)
  3 siblings, 0 replies; 19+ messages in thread
From: Nicolas Ferre @ 2021-10-12  9:24 UTC (permalink / raw)
  To: Antoine Tenart, David S . Miller, Jakub Kicinski, Sean Anderson, netdev
  Cc: Russell King, Claudiu Beznea

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate
  2021-10-12  8:27     ` Antoine Tenart
@ 2021-10-12 16:20       ` Sean Anderson
  0 siblings, 0 replies; 19+ messages in thread
From: Sean Anderson @ 2021-10-12 16:20 UTC (permalink / raw)
  To: Antoine Tenart, Jakub Kicinski
  Cc: netdev, David S . Miller, Russell King, Nicolas Ferre, Claudiu Beznea



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-12  8:33 ` [PATCH v2 1/2] net: macb: Clean up macb_validate Antoine Tenart
  2021-10-12  8:34   ` Antoine Tenart
  2021-10-12  9:24   ` Nicolas Ferre
@ 2021-10-12 16:34   ` Sean Anderson
  2021-10-12 16:53     ` Antoine Tenart
  2021-10-14 16:34   ` Russell King (Oracle)
  3 siblings, 1 reply; 19+ messages in thread
From: Sean Anderson @ 2021-10-12 16:34 UTC (permalink / raw)
  To: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev
  Cc: Russell King, Nicolas Ferre, Claudiu Beznea



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-12 16:34   ` Sean Anderson
@ 2021-10-12 16:53     ` Antoine Tenart
  0 siblings, 0 replies; 19+ messages in thread
From: Antoine Tenart @ 2021-10-12 16:53 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, Sean Anderson, netdev
  Cc: Russell King, Nicolas Ferre, Claudiu Beznea

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-12  8:33 ` [PATCH v2 1/2] net: macb: Clean up macb_validate Antoine Tenart
                     ` (2 preceding siblings ...)
  2021-10-12 16:34   ` Sean Anderson
@ 2021-10-14 16:34   ` Russell King (Oracle)
  2021-10-14 17:50     ` Sean Anderson
  3 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2021-10-14 16:34 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: David S . Miller, Jakub Kicinski, Sean Anderson, netdev,
	Nicolas Ferre, Claudiu Beznea

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!

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-14 16:34   ` Russell King (Oracle)
@ 2021-10-14 17:50     ` Sean Anderson
  2021-10-14 23:08       ` Russell King (Oracle)
  0 siblings, 1 reply; 19+ messages in thread
From: Sean Anderson @ 2021-10-14 17:50 UTC (permalink / raw)
  To: Russell King (Oracle), Antoine Tenart
  Cc: David S . Miller, Jakub Kicinski, netdev, Nicolas Ferre, Claudiu Beznea



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-14 17:50     ` Sean Anderson
@ 2021-10-14 23:08       ` Russell King (Oracle)
  2021-10-15 22:28         ` Sean Anderson
  0 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2021-10-14 23:08 UTC (permalink / raw)
  To: Sean Anderson
  Cc: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev,
	Nicolas Ferre, Claudiu Beznea

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!

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-14 23:08       ` Russell King (Oracle)
@ 2021-10-15 22:28         ` Sean Anderson
  2021-10-15 22:47           ` Russell King (Oracle)
  0 siblings, 1 reply; 19+ messages in thread
From: Sean Anderson @ 2021-10-15 22:28 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev,
	Nicolas Ferre, Claudiu Beznea




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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-15 22:28         ` Sean Anderson
@ 2021-10-15 22:47           ` Russell King (Oracle)
  2021-10-19 15:02             ` Russell King (Oracle)
  0 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2021-10-15 22:47 UTC (permalink / raw)
  To: Sean Anderson
  Cc: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev,
	Nicolas Ferre, Claudiu Beznea

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!

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-15 22:47           ` Russell King (Oracle)
@ 2021-10-19 15:02             ` Russell King (Oracle)
  2021-10-22 17:37               ` Sean Anderson
  0 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2021-10-19 15:02 UTC (permalink / raw)
  To: Sean Anderson
  Cc: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev,
	Nicolas Ferre, Claudiu Beznea

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!

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-19 15:02             ` Russell King (Oracle)
@ 2021-10-22 17:37               ` Sean Anderson
  2021-10-25 10:35                 ` Russell King (Oracle)
  0 siblings, 1 reply; 19+ messages in thread
From: Sean Anderson @ 2021-10-22 17:37 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev,
	Nicolas Ferre, Claudiu Beznea

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-22 17:37               ` Sean Anderson
@ 2021-10-25 10:35                 ` Russell King (Oracle)
  2021-10-25 15:26                   ` Sean Anderson
  0 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2021-10-25 10:35 UTC (permalink / raw)
  To: Sean Anderson
  Cc: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev,
	Nicolas Ferre, Claudiu Beznea

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!

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
  2021-10-25 10:35                 ` Russell King (Oracle)
@ 2021-10-25 15:26                   ` Sean Anderson
  0 siblings, 0 replies; 19+ messages in thread
From: Sean Anderson @ 2021-10-25 15:26 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev,
	Nicolas Ferre, Claudiu Beznea



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-10-25 15:26 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-11 16:55 [PATCH v2 1/2] net: macb: Clean up macb_validate Sean Anderson
2021-10-11 16:55 ` [PATCH v2 2/2] net: macb: Allow SGMII only if we are a GEM in mac_validate Sean Anderson
2021-10-12  0:17   ` Jakub Kicinski
2021-10-12  8:27     ` Antoine Tenart
2021-10-12 16:20       ` Sean Anderson
2021-10-12  8:33 ` [PATCH v2 1/2] net: macb: Clean up macb_validate Antoine Tenart
2021-10-12  8:34   ` Antoine Tenart
2021-10-12  9:24   ` Nicolas Ferre
2021-10-12 16:34   ` Sean Anderson
2021-10-12 16:53     ` Antoine Tenart
2021-10-14 16:34   ` Russell King (Oracle)
2021-10-14 17:50     ` Sean Anderson
2021-10-14 23:08       ` Russell King (Oracle)
2021-10-15 22:28         ` Sean Anderson
2021-10-15 22:47           ` Russell King (Oracle)
2021-10-19 15:02             ` Russell King (Oracle)
2021-10-22 17:37               ` Sean Anderson
2021-10-25 10:35                 ` Russell King (Oracle)
2021-10-25 15:26                   ` Sean Anderson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.