[PATCH net-next] net: dsa: remove obsolete phylink dsa_switch operations

[PATCH net-next] net: dsa: remove obsolete phylink dsa_switch operations

[Russell King (Oracle)]

No driver now uses the DSA switch phylink members, so we can now remove
the shim functions and method pointers. Arrange to print an error
message and fail registration if a DSA driver does not provide the
phylink MAC operations structure.

Signed-off-by: Russell King (oracle) <rmk+kernel@armlinux.org.uk>
---
 include/net/dsa.h | 15 ----------
 net/dsa/dsa.c     |  9 ++----
 net/dsa/port.c    | 74 +----------------------------------------------
 3 files changed, 4 insertions(+), 94 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f9ae3ca66b6f..d64bbd1f9f29 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -879,21 +879,6 @@ struct dsa_switch_ops {
 	 */
 	void	(*phylink_get_caps)(struct dsa_switch *ds, int port,
 				    struct phylink_config *config);
-	struct phylink_pcs *(*phylink_mac_select_pcs)(struct dsa_switch *ds,
-						      int port,
-						      phy_interface_t iface);
-	void	(*phylink_mac_config)(struct dsa_switch *ds, int port,
-				      unsigned int mode,
-				      const struct phylink_link_state *state);
-	void	(*phylink_mac_link_down)(struct dsa_switch *ds, int port,
-					 unsigned int mode,
-					 phy_interface_t interface);
-	void	(*phylink_mac_link_up)(struct dsa_switch *ds, int port,
-				       unsigned int mode,
-				       phy_interface_t interface,
-				       struct phy_device *phydev,
-				       int speed, int duplex,
-				       bool tx_pause, bool rx_pause);
 	void	(*phylink_fixed_state)(struct dsa_switch *ds, int port,
 				       struct phylink_link_state *state);
 	/*
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 668c729946ea..ceeadb52d1cc 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -1505,12 +1505,9 @@ static int dsa_switch_probe(struct dsa_switch *ds)
 	if (!ds->num_ports)
 		return -EINVAL;
 
-	if (ds->phylink_mac_ops) {
-		if (ds->ops->phylink_mac_select_pcs ||
-		    ds->ops->phylink_mac_config ||
-		    ds->ops->phylink_mac_link_down ||
-		    ds->ops->phylink_mac_link_up)
-			return -EINVAL;
+	if (!ds->phylink_mac_ops) {
+		dev_err(ds->dev, "DSA switch driver does not provide phylink MAC operations");
+		return -EINVAL;
 	}
 
 	if (np) {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index e23db9507546..a31a5517a12f 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1535,73 +1535,8 @@ 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)
-{
-	struct dsa_port *dp = dsa_phylink_to_port(config);
-	struct phylink_pcs *pcs = ERR_PTR(-EOPNOTSUPP);
-	struct dsa_switch *ds = dp->ds;
-
-	if (ds->ops->phylink_mac_select_pcs)
-		pcs = ds->ops->phylink_mac_select_pcs(ds, dp->index, interface);
-
-	return pcs;
-}
-
-static void dsa_port_phylink_mac_config(struct phylink_config *config,
-					unsigned int mode,
-					const struct phylink_link_state *state)
-{
-	struct dsa_port *dp = dsa_phylink_to_port(config);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!ds->ops->phylink_mac_config)
-		return;
-
-	ds->ops->phylink_mac_config(ds, dp->index, mode, state);
-}
-
-static void dsa_port_phylink_mac_link_down(struct phylink_config *config,
-					   unsigned int mode,
-					   phy_interface_t interface)
-{
-	struct dsa_port *dp = dsa_phylink_to_port(config);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!ds->ops->phylink_mac_link_down)
-		return;
-
-	ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface);
-}
-
-static void dsa_port_phylink_mac_link_up(struct phylink_config *config,
-					 struct phy_device *phydev,
-					 unsigned int mode,
-					 phy_interface_t interface,
-					 int speed, int duplex,
-					 bool tx_pause, bool rx_pause)
-{
-	struct dsa_port *dp = dsa_phylink_to_port(config);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!ds->ops->phylink_mac_link_up)
-		return;
-
-	ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev,
-				     speed, duplex, tx_pause, rx_pause);
-}
-
-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,
-};
-
 int dsa_port_phylink_create(struct dsa_port *dp)
 {
-	const struct phylink_mac_ops *mac_ops;
 	struct dsa_switch *ds = dp->ds;
 	phy_interface_t mode;
 	struct phylink *pl;
@@ -1625,12 +1560,8 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 		}
 	}
 
-	mac_ops = &dsa_port_phylink_mac_ops;
-	if (ds->phylink_mac_ops)
-		mac_ops = ds->phylink_mac_ops;
-
 	pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), mode,
-			    mac_ops);
+			    ds->phylink_mac_ops);
 	if (IS_ERR(pl)) {
 		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(pl));
 		return PTR_ERR(pl);
@@ -1831,9 +1762,6 @@ static void dsa_shared_port_link_down(struct dsa_port *dp)
 	if (ds->phylink_mac_ops && ds->phylink_mac_ops->mac_link_down)
 		ds->phylink_mac_ops->mac_link_down(&dp->pl_config, MLO_AN_FIXED,
 						   PHY_INTERFACE_MODE_NA);
-	else if (ds->ops->phylink_mac_link_down)
-		ds->ops->phylink_mac_link_down(ds, dp->index, MLO_AN_FIXED,
-					       PHY_INTERFACE_MODE_NA);
 }
 
 int dsa_shared_port_link_register_of(struct dsa_port *dp)
-- 
2.30.2

Re: [PATCH net-next] net: dsa: remove obsolete phylink dsa_switch operations

[Andrew Lunn]

On Fri, May 31, 2024 at 09:21:29AM +0100, Russell King (Oracle) wrote:
> No driver now uses the DSA switch phylink members, so we can now remove
> the shim functions and method pointers. Arrange to print an error
> message and fail registration if a DSA driver does not provide the
> phylink MAC operations structure.
> 
> Signed-off-by: Russell King (oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: dsa: remove obsolete phylink dsa_switch operations

[Vladimir Oltean]

On Fri, May 31, 2024 at 09:21:29AM +0100, Russell King (Oracle) wrote:
> No driver now uses the DSA switch phylink members, so we can now remove
> the shim functions and method pointers. Arrange to print an error
> message and fail registration if a DSA driver does not provide the
> phylink MAC operations structure.
> 
> Signed-off-by: Russell King (oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  include/net/dsa.h | 15 ----------
>  net/dsa/dsa.c     |  9 ++----
>  net/dsa/port.c    | 74 +----------------------------------------------
>  3 files changed, 4 insertions(+), 94 deletions(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 668c729946ea..ceeadb52d1cc 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -1505,12 +1505,9 @@ static int dsa_switch_probe(struct dsa_switch *ds)
>  	if (!ds->num_ports)
>  		return -EINVAL;
>  
> -	if (ds->phylink_mac_ops) {
> -		if (ds->ops->phylink_mac_select_pcs ||
> -		    ds->ops->phylink_mac_config ||
> -		    ds->ops->phylink_mac_link_down ||
> -		    ds->ops->phylink_mac_link_up)
> -			return -EINVAL;
> +	if (!ds->phylink_mac_ops) {
> +		dev_err(ds->dev, "DSA switch driver does not provide phylink MAC operations");
> +		return -EINVAL;
>  	}
>  
>  	if (np) {
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index e23db9507546..a31a5517a12f 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -1625,12 +1560,8 @@ int dsa_port_phylink_create(struct dsa_port *dp)
>  		}
>  	}
>  
> -	mac_ops = &dsa_port_phylink_mac_ops;
> -	if (ds->phylink_mac_ops)
> -		mac_ops = ds->phylink_mac_ops;
> -
>  	pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), mode,
> -			    mac_ops);
> +			    ds->phylink_mac_ops);
>  	if (IS_ERR(pl)) {
>  		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(pl));
>  		return PTR_ERR(pl);

Nack. The highlighted portions break dsa_loop, hellcreek and mv88e6060 [1],
which are currently trivially integrated with phylink through the
default dsa_port_phylink_mac_ops, but have no implementations of
ds->ops->phylink_mac_* of their own.

What I'm trying to point out is that we are not at the stage yet where
we can enforce all drivers to populate ds->phylink_mac_ops.

> @@ -1831,9 +1762,6 @@ static void dsa_shared_port_link_down(struct dsa_port *dp)
>  	if (ds->phylink_mac_ops && ds->phylink_mac_ops->mac_link_down)

Mostly irrelevant, but I'll point out an issue with the patch logic's
consistency anyway: there is no need to check "if (ds->phylink_mac_ops)"
more than once. The earlier probe failure is sufficient (although, as
mentioned, breaking for 3 drivers).

>  		ds->phylink_mac_ops->mac_link_down(&dp->pl_config, MLO_AN_FIXED,
>  						   PHY_INTERFACE_MODE_NA);
> -	else if (ds->ops->phylink_mac_link_down)
> -		ds->ops->phylink_mac_link_down(ds, dp->index, MLO_AN_FIXED,
> -					       PHY_INTERFACE_MODE_NA);
>  }
>  
>  int dsa_shared_port_link_register_of(struct dsa_port *dp)
> -- 
> 2.30.2
> 

[1] Quick way to check: compare the outputs of these 2 commands, and see
which drivers from the first category are absent from the second:
$ grep -r dsa_register_switch drivers/net/dsa/ | cut -d: -f1 | sort | uniq
$ grep -r phylink_mac_ops drivers/net/dsa/ | cut -d: -f1 | sort | uniq