[PATCH net-next v2 0/5] Removing more phylink cruft

[PATCH net-next v2 0/5] Removing more phylink cruft

[Russell King (Oracle)]

Hi,

Continuing on with the cleanups, this patch series removes
dsa_port_phylink_mac_select_pcs() which is no longer required. This
will have no functional effect as phylink does this:

        bool using_mac_select_pcs = false;

        if (mac_ops->mac_select_pcs &&
            mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
              ERR_PTR(-EOPNOTSUPP))
                using_mac_select_pcs = true;

and no mac_select_pcs() method is equivalent to a mac_select_pcs()
that returns -EOPNOTSUPP.

We then make mv88e6xxx_mac_select_pcs() return NULL, as we don't want
to invoke this old behaviour anymore - mv88e6xxx doesn't require it.

Then, allow phylink to remove PCS, which has been a long standing
behavioural oddity.

Remove the use of pl->pcs when validating as this will never be
non-NULL unless "using_mac_select_pcs" was set.

This then clears the way to removing using_mac_select_pcs from phylink
and the check.

 drivers/net/dsa/mv88e6xxx/chip.c |  2 +-
 drivers/net/phy/phylink.c        | 18 +++-----------
 net/dsa/port.c                   |  8 --------
 3 files changed, 5 insertions(+), 23 deletions(-)

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

[PATCH net-next v2 1/5] net: dsa: remove dsa_port_phylink_mac_select_pcs()

[Russell King (Oracle)]

There is no longer any reason to implement the mac_select_pcs()
callback in DSA. Returning ERR_PTR(-EOPNOTSUPP) is functionally
equivalent to not providing the function.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 net/dsa/port.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index f1e96706a701..ee0aaec4c8e0 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1575,13 +1575,6 @@ void dsa_port_set_tag_protocol(struct dsa_port *cpu_dp,
 	cpu_dp->tag_ops = tag_ops;
 }
 
-static struct phylink_pcs *
-dsa_port_phylink_mac_select_pcs(struct phylink_config *config,
-				phy_interface_t interface)
-{
-	return ERR_PTR(-EOPNOTSUPP);
-}
-
 static void dsa_port_phylink_mac_config(struct phylink_config *config,
 					unsigned int mode,
 					const struct phylink_link_state *state)
@@ -1604,7 +1597,6 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config,
 }
 
 static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
-	.mac_select_pcs = dsa_port_phylink_mac_select_pcs,
 	.mac_config = dsa_port_phylink_mac_config,
 	.mac_link_down = dsa_port_phylink_mac_link_down,
 	.mac_link_up = dsa_port_phylink_mac_link_up,
-- 
2.30.2

[PATCH net-next v2 2/5] net: dsa: mv88e6xxx: return NULL when no PCS is present

[Russell King (Oracle)]

Rather than returning an EOPNOTSUPP error pointer when the switch
has no support for PCS, return NULL to indicate that no PCS is
required.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index f68233d24f32..da19a3b05549 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -868,7 +868,7 @@ mv88e6xxx_mac_select_pcs(struct phylink_config *config,
 {
 	struct dsa_port *dp = dsa_phylink_to_port(config);
 	struct mv88e6xxx_chip *chip = dp->ds->priv;
-	struct phylink_pcs *pcs = ERR_PTR(-EOPNOTSUPP);
+	struct phylink_pcs *pcs = NULL;
 
 	if (chip->info->ops->pcs_ops)
 		pcs = chip->info->ops->pcs_ops->pcs_select(chip, dp->index,
-- 
2.30.2

[PATCH net-next v2 3/5] net: phylink: allow mac_select_pcs() to remove a PCS

[Russell King (Oracle)]

phylink has historically not permitted a PCS to be removed. An attempt
to permit this with phylink_set_pcs() resulted in comments indicating
that there was no need for this. This behaviour has been propagated
forward to the mac_select_pcs() approach as it was believed from these
comments that changing this would be NAK'd.

However, with mac_select_pcs(), it takes more code and thus complexity
to maintain this behaviour, which can - and in this case has - resulted
in a bug. If mac_select_pcs() returns NULL for a particular interface
type, but there is already a PCS in-use, then we skip the pcs_validate()
method, but continue using the old PCS. Also, it wouldn't be expected
behaviour by implementers of mac_select_pcs().

Allow this by removing this old unnecessary restriction.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 24a3144e870a..aa1139efc7e4 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1184,7 +1184,7 @@ static void phylink_major_config(struct phylink *pl, bool restart,
 			return;
 		}
 
-		pcs_changed = pcs && pl->pcs != pcs;
+		pcs_changed = pl->pcs != pcs;
 	}
 
 	phylink_pcs_poll_stop(pl);
-- 
2.30.2

[PATCH net-next v2 4/5] net: phylink: remove use of pl->pcs in phylink_validate_mac_and_pcs()

[Russell King (Oracle)]

When the mac_select_pcs() method is not implemented, there is no way
for pl->pcs to be set to a non-NULL value. This was here to support
the old phylink_set_pcs() method which has been removed a few years
ago. Simplify the code in phylink_validate_mac_and_pcs().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index aa1139efc7e4..94f3c5fd09ed 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -649,8 +649,8 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
 					unsigned long *supported,
 					struct phylink_link_state *state)
 {
+	struct phylink_pcs *pcs = NULL;
 	unsigned long capabilities;
-	struct phylink_pcs *pcs;
 	int ret;
 
 	/* Get the PCS for this interface mode */
@@ -658,8 +658,6 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
 		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
 		if (IS_ERR(pcs))
 			return PTR_ERR(pcs);
-	} else {
-		pcs = pl->pcs;
 	}
 
 	if (pcs) {
-- 
2.30.2

[PATCH net-next v2 5/5] net: phylink: remove "using_mac_select_pcs"

[Russell King (Oracle)]

With DSA's implementation of the mac_select_pcs() method removed, we
can now remove the detection of mac_select_pcs() implementation.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 94f3c5fd09ed..b5870f8666ac 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -79,7 +79,6 @@ struct phylink {
 	unsigned int pcs_state;
 
 	bool mac_link_dropped;
-	bool using_mac_select_pcs;
 
 	struct sfp_bus *sfp_bus;
 	bool sfp_may_have_phy;
@@ -654,7 +653,7 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
 	int ret;
 
 	/* Get the PCS for this interface mode */
-	if (pl->using_mac_select_pcs) {
+	if (pl->mac_ops->mac_select_pcs) {
 		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
 		if (IS_ERR(pcs))
 			return PTR_ERR(pcs);
@@ -1173,7 +1172,7 @@ static void phylink_major_config(struct phylink *pl, bool restart,
 						state->interface,
 						state->advertising);
 
-	if (pl->using_mac_select_pcs) {
+	if (pl->mac_ops->mac_select_pcs) {
 		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
 		if (IS_ERR(pcs)) {
 			phylink_err(pl,
@@ -1689,7 +1688,6 @@ struct phylink *phylink_create(struct phylink_config *config,
 			       phy_interface_t iface,
 			       const struct phylink_mac_ops *mac_ops)
 {
-	bool using_mac_select_pcs = false;
 	struct phylink *pl;
 	int ret;
 
@@ -1700,11 +1698,6 @@ struct phylink *phylink_create(struct phylink_config *config,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (mac_ops->mac_select_pcs &&
-	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
-	      ERR_PTR(-EOPNOTSUPP))
-		using_mac_select_pcs = true;
-
 	pl = kzalloc(sizeof(*pl), GFP_KERNEL);
 	if (!pl)
 		return ERR_PTR(-ENOMEM);
@@ -1723,7 +1716,6 @@ struct phylink *phylink_create(struct phylink_config *config,
 		return ERR_PTR(-EINVAL);
 	}
 
-	pl->using_mac_select_pcs = using_mac_select_pcs;
 	pl->phy_state.interface = iface;
 	pl->link_interface = iface;
 	if (iface == PHY_INTERFACE_MODE_MOCA)
-- 
2.30.2

Re: [PATCH net-next v2 0/5] Removing more phylink cruft

[Maxime Chevallier]

Hi,

On Wed, 16 Oct 2024 10:57:29 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> Hi,
> 
> Continuing on with the cleanups, this patch series removes
> dsa_port_phylink_mac_select_pcs() which is no longer required. This
> will have no functional effect as phylink does this:
> 
>         bool using_mac_select_pcs = false;
> 
>         if (mac_ops->mac_select_pcs &&
>             mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
>               ERR_PTR(-EOPNOTSUPP))
>                 using_mac_select_pcs = true;
> 
> and no mac_select_pcs() method is equivalent to a mac_select_pcs()
> that returns -EOPNOTSUPP.
> 
> We then make mv88e6xxx_mac_select_pcs() return NULL, as we don't want
> to invoke this old behaviour anymore - mv88e6xxx doesn't require it.
> 
> Then, allow phylink to remove PCS, which has been a long standing
> behavioural oddity.
> 
> Remove the use of pl->pcs when validating as this will never be
> non-NULL unless "using_mac_select_pcs" was set.
> 
> This then clears the way to removing using_mac_select_pcs from phylink
> and the check.

I don't have any setup with such a switch or that could trigger the
PCS removal, so I couldn't fully test it. However it does look good to
me :)

For the series,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net-next v2 3/5] net: phylink: allow mac_select_pcs() to remove a PCS

[Vladimir Oltean]

On Wed, Oct 16, 2024 at 10:58:34AM +0100, Russell King (Oracle) wrote:
> phylink has historically not permitted a PCS to be removed. An attempt
> to permit this with phylink_set_pcs() resulted in comments indicating
> that there was no need for this. This behaviour has been propagated
> forward to the mac_select_pcs() approach as it was believed from these
> comments that changing this would be NAK'd.
> 
> However, with mac_select_pcs(), it takes more code and thus complexity
> to maintain this behaviour, which can - and in this case has - resulted
> in a bug. If mac_select_pcs() returns NULL for a particular interface
> type, but there is already a PCS in-use, then we skip the pcs_validate()
> method, but continue using the old PCS. Also, it wouldn't be expected
> behaviour by implementers of mac_select_pcs().
> 
> Allow this by removing this old unnecessary restriction.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v2 4/5] net: phylink: remove use of pl->pcs in phylink_validate_mac_and_pcs()

[Vladimir Oltean]

On Wed, Oct 16, 2024 at 10:58:39AM +0100, Russell King (Oracle) wrote:
> When the mac_select_pcs() method is not implemented, there is no way
> for pl->pcs to be set to a non-NULL value. This was here to support
> the old phylink_set_pcs() method which has been removed a few years
> ago. Simplify the code in phylink_validate_mac_and_pcs().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v2 5/5] net: phylink: remove "using_mac_select_pcs"

[Vladimir Oltean]

On Wed, Oct 16, 2024 at 10:58:44AM +0100, Russell King (Oracle) wrote:
> With DSA's implementation of the mac_select_pcs() method removed, we
> can now remove the detection of mac_select_pcs() implementation.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v2 0/5] Removing more phylink cruft

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Andrew Lunn <andrew@lunn.ch>:

On Wed, 16 Oct 2024 10:57:29 +0100 you wrote:
> Hi,
> 
> Continuing on with the cleanups, this patch series removes
> dsa_port_phylink_mac_select_pcs() which is no longer required. This
> will have no functional effect as phylink does this:
> 
>         bool using_mac_select_pcs = false;
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/5] net: dsa: remove dsa_port_phylink_mac_select_pcs()
    https://git.kernel.org/netdev/net-next/c/ecb595ebba0e
  - [net-next,v2,2/5] net: dsa: mv88e6xxx: return NULL when no PCS is present
    https://git.kernel.org/netdev/net-next/c/14ca726ada7f
  - [net-next,v2,3/5] net: phylink: allow mac_select_pcs() to remove a PCS
    https://git.kernel.org/netdev/net-next/c/486dc391ef43
  - [net-next,v2,4/5] net: phylink: remove use of pl->pcs in phylink_validate_mac_and_pcs()
    https://git.kernel.org/netdev/net-next/c/6c48cd044cc8
  - [net-next,v2,5/5] net: phylink: remove "using_mac_select_pcs"
    https://git.kernel.org/netdev/net-next/c/7530ea26c810

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html