[PATCH RFC net-next 0/8] net: phylink: introduce legacy mode flag

Hi all,

In March 2020, phylink gained support to split the PCS support out of
the MAC callbacks. By doing so, a slight behavioural difference was
introduced when a PCS is present, specifically:

1) the call to mac_config() when the link comes up or advertisement
   changes were eliminated
2) mac_an_restart() will never be called
3) mac_pcs_get_state() will never be called

The intention was to eventually remove this support once all phylink
users were converted. Unfortunately, this still hasn't happened - and
in some cases, it looks like it may never happen.

Through discussion with Sean Anderson, we now need to allow the PCS to
be optional for modern drivers, so we need a different way to identify
these legacy drivers.

In order to do that, this series of patches introduce a
"legacy_pre_march2020" which is used to allow the old behaviour - in
other words, we get the old behaviour only when there is no PCS and
this flag is true. Otherwise, we get the new behaviour.

I decided to use the date of the change in the flag as just using
"legacy" or "legacy_driver" is too non-descript. An alternative could
be to use the git sha1 hash of the set of changes.

As part of this series, I have consolidated DSA's phylink creation, so
only one place needs maintenance. This reduces the size of subsequent
changes, including further changes I have lined up.

I believe I have added the legacy flag to all the drivers which use
legacy mode - that being the ag71xx, mtk_eth_soc and axienet ethernet
drivers, and many DSA drivers - the ones which need the old behaviour
are identified by having non-NULL phylink_mac_link_state or
phylink_mac_an_restart methods in their dsa_switch_ops structure.

 drivers/net/ethernet/atheros/ag71xx.c             |  1 +
 drivers/net/ethernet/mediatek/mtk_eth_soc.c       |  4 ++
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c |  1 +
 drivers/net/phy/phylink.c                         | 32 +++++++++-----
 include/linux/phylink.h                           | 20 +++++++++
 net/dsa/dsa_priv.h                                |  2 +-
 net/dsa/port.c                                    | 51 ++++++++++++++++-------
 net/dsa/slave.c                                   | 19 ++-------
 8 files changed, 86 insertions(+), 44 deletions(-)

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

ag71xx has a PCS, but does not make use of the phylink PCS support.
Mark it was a pre-March 2020 driver.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/ethernet/atheros/ag71xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index ff924f06581e..89b6a8bfee43 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c

@@ -1111,6 +1111,7 @@ static int ag71xx_phylink_setup(struct ag71xx *ag)
 
 	ag->phylink_config.dev = &ag->ndev->dev;
 	ag->phylink_config.type = PHYLINK_NETDEV;
+	ag->phylink_config.legacy_pre_march2020 = true;
 	ag->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
 		MAC_10 | MAC_100 | MAC_1000FD;
 

-- 
2.30.2

axienet has a PCS, but does not make use of the phylink PCS support.
Mark it was a pre-March 2020 driver.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index e39356364f33..23ac353b35fe 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c

@@ -2051,6 +2051,7 @@ static int axienet_probe(struct platform_device *pdev)
 
 	lp->phylink_config.dev = &ndev->dev;
 	lp->phylink_config.type = PHYLINK_NETDEV;
+	lp->phylink_config.legacy_pre_march2020 = true;
 	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
 		MAC_10FD | MAC_100FD | MAC_1000FD;
 

-- 
2.30.2

Use the legacy flag to indicate whether we should operate in legacy
mode. This allows us to stop using the presence of a PCS as an
indicator to the age of the phylink user, and make PCS presence
optional.

Legacy mode involves:
1) calling mac_config() whenever the link comes up
2) calling mac_config() whenever the inband advertisement changes,
   possibly followed by a call to mac_an_restart()
3) making use of mac_an_restart()
4) making use of mac_pcs_get_state()

All the above functionality was moved to a seperate "PCS" block of
operations in March 2020.

Update the documents to indicate that the differences that this flag
makes.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/phy/phylink.c | 12 ++++++------
 include/linux/phylink.h   | 17 +++++++++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 3603c024109a..a935655c39c0 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -742,7 +742,7 @@ static void phylink_mac_pcs_an_restart(struct phylink *pl)
 	    phylink_autoneg_inband(pl->cur_link_an_mode)) {
 		if (pl->pcs_ops)
 			pl->pcs_ops->pcs_an_restart(pl->pcs);
-		else
+		else if (pl->config->legacy_pre_march2020)
 			pl->mac_ops->mac_an_restart(pl->config);
 	}
 }

@@ -803,7 +803,7 @@ static int phylink_change_inband_advert(struct phylink *pl)
 	if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
 		return 0;
 
-	if (!pl->pcs_ops) {
+	if (!pl->pcs_ops && pl->config->legacy_pre_march2020) {
 		/* Legacy method */
 		phylink_mac_config(pl, &pl->link_config);
 		phylink_mac_pcs_an_restart(pl);

@@ -854,7 +854,8 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 
 	if (pl->pcs_ops)
 		pl->pcs_ops->pcs_get_state(pl->pcs, state);
-	else if (pl->mac_ops->mac_pcs_get_state)
+	else if (pl->mac_ops->mac_pcs_get_state &&
+		 pl->config->legacy_pre_march2020)
 		pl->mac_ops->mac_pcs_get_state(pl->config, state);
 	else
 		state->link = 0;

@@ -1024,12 +1025,11 @@ static void phylink_resolve(struct work_struct *w)
 			}
 			phylink_major_config(pl, false, &link_state);
 			pl->link_config.interface = link_state.interface;
-		} else if (!pl->pcs_ops) {
+		} else if (!pl->pcs_ops && pl->config->legacy_pre_march2020) {
 			/* The interface remains unchanged, only the speed,
 			 * duplex or pause settings have changed. Call the
 			 * old mac_config() method to configure the MAC/PCS
-			 * only if we do not have a PCS installed (an
-			 * unconverted user.)
+			 * only if we do not have a legacy MAC driver.
 			 */
 			phylink_mac_config(pl, &link_state);
 		}

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d005b8e36048..a2f266cc3442 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h

@@ -190,6 +190,10 @@ void validate(struct phylink_config *config, unsigned long *supported,
  * negotiation completion state in @state->an_complete, and link up state
  * in @state->link. If possible, @state->lp_advertising should also be
  * populated.
+ *
+ * Note: This is a legacy method. This function will not be called unless
+ * legacy_pre_march2020 is set in &struct phylink_config and there is no
+ * PCS attached.
  */
 void mac_pcs_get_state(struct phylink_config *config,
 		       struct phylink_link_state *state);

@@ -230,6 +234,15 @@ int mac_prepare(struct phylink_config *config, unsigned int mode,
  * guaranteed to be correct, and so any mac_config() implementation must
  * never reference these fields.
  *
+ * Note: For legacy March 2020 drivers (drivers with legacy_pre_march2020 set
+ * in their &phylnk_config and which don't have a PCS), this function will be
+ * called on each link up event, and to also change the in-band advert. For
+ * non-legacy drivers, it will only be called to reconfigure the MAC for a
+ * "major" change in e.g. interface mode. It will not be called for changes
+ * in speed, duplex or pause modes or to change the in-band advertisement.
+ * In any case, it is strongly preferred that speed, duplex and pause settings
+ * are handled in the mac_link_up() method and not in this method.
+ *
  * (this requires a rewrite - please refer to mac_link_up() for situations
  *  where the PCS and MAC are not tightly integrated.)
  *

@@ -314,6 +327,10 @@ int mac_finish(struct phylink_config *config, unsigned int mode,
 /**
  * mac_an_restart() - restart 802.3z BaseX autonegotiation
  * @config: a pointer to a &struct phylink_config.
+ *
+ * Note: This is a legacy method. This function will not be called unless
+ * legacy_pre_march2020 is set in &struct phylink_config and there is no
+ * PCS attached.
  */
 void mac_an_restart(struct phylink_config *config);
 

-- 
2.30.2

Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
the PCS from phylink. This is only supported on non-legacy drivers
where doing so will have no effect on the mac_config() calling
behaviour.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/phy/phylink.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a935655c39c0..9f0f0e0aad55 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
  * in mac_prepare() or mac_config() methods if it is desired to dynamically
  * change the PCS.
  *
- * Please note that there are behavioural changes with the mac_config()
- * callback if a PCS is present (denoting a newer setup) so removing a PCS
- * is not supported, and if a PCS is going to be used, it must be registered
- * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ * Please note that for legacy phylink users, there are behavioural changes
+ * with the mac_config() callback if a PCS is present (denoting a newer setup)
+ * so removing a PCS is not supported. If a PCS is going to be used, it must
+ * be registered by calling phylink_set_pcs() at the latest in the first
+ * mac_config() call.
+ *
+ * For modern drivers, this may be called with a NULL pcs argument to
+ * disconnect the PCS from phylink.
  */
 void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 {
+	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
+		phylink_warn(pl,
+			     "Removing PCS is not supported in a legacy driver");
+		return;
+	}
+
 	pl->pcs = pcs;
-	pl->pcs_ops = pcs->ops;
+	pl->pcs_ops = pcs ? pcs->ops : NULL;
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);
 

-- 
2.30.2

The code in port.c and slave.c creating the phylink instance is very
similar - let's consolidate this into a single function.

Signed-off-by: Russell King (Oracle) <redacted>
---
 net/dsa/dsa_priv.h |  2 +-
 net/dsa/port.c     | 44 ++++++++++++++++++++++++++++----------------
 net/dsa/slave.c    | 19 +++----------------
 3 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index a5c9bc7b66c6..3fb2c37c9b88 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h

@@ -258,13 +258,13 @@ int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
 			       const struct switchdev_obj_ring_role_mrp *mrp);
 int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
 			       const struct switchdev_obj_ring_role_mrp *mrp);
+int dsa_port_phylink_create(struct dsa_port *dp);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
 void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
 int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid, bool broadcast);
 void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid, bool broadcast);
-extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
 
 static inline bool dsa_port_offloads_bridge_port(struct dsa_port *dp,
 						 const struct net_device *dev)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index f6f12ad2b525..eaa66114924b 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c

@@ -1072,7 +1072,7 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config,
 				     speed, duplex, tx_pause, rx_pause);
 }
 
-const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
+static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
 	.validate = dsa_port_phylink_validate,
 	.mac_pcs_get_state = dsa_port_phylink_mac_pcs_get_state,
 	.mac_config = dsa_port_phylink_mac_config,

@@ -1081,6 +1081,30 @@ const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
 	.mac_link_up = dsa_port_phylink_mac_link_up,
 };
 
+int dsa_port_phylink_create(struct dsa_port *dp)
+{
+	struct dsa_switch *ds = dp->ds;
+	phy_interface_t mode;
+	int err;
+
+	err = of_get_phy_mode(dp->dn, &mode);
+	if (err)
+		mode = PHY_INTERFACE_MODE_NA;
+
+	if (ds->ops->phylink_get_interfaces)
+		ds->ops->phylink_get_interfaces(ds, dp->index,
+					dp->pl_config.supported_interfaces);
+
+	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn),
+				mode, &dsa_port_phylink_mac_ops);
+	if (IS_ERR(dp->pl)) {
+		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
+		return PTR_ERR(dp->pl);
+	}
+
+	return 0;
+}
+
 static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
 {
 	struct dsa_switch *ds = dp->ds;

@@ -1157,27 +1181,15 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct device_node *port_dn = dp->dn;
-	phy_interface_t mode;
 	int err;
 
-	err = of_get_phy_mode(port_dn, &mode);
-	if (err)
-		mode = PHY_INTERFACE_MODE_NA;
-
 	dp->pl_config.dev = ds->dev;
 	dp->pl_config.type = PHYLINK_DEV;
 	dp->pl_config.pcs_poll = ds->pcs_poll;
 
-	if (ds->ops->phylink_get_interfaces)
-		ds->ops->phylink_get_interfaces(ds, dp->index,
-					dp->pl_config.supported_interfaces);
-
-	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(port_dn),
-				mode, &dsa_port_phylink_mac_ops);
-	if (IS_ERR(dp->pl)) {
-		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
-		return PTR_ERR(dp->pl);
-	}
+	err = dsa_port_phylink_create(dp);
+	if (err)
+		return err;
 
 	err = phylink_of_phy_connect(dp->pl, port_dn, 0);
 	if (err && err != -ENODEV) {

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ad61f6bc8886..33b54eadc641 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c

@@ -1851,14 +1851,9 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 	struct dsa_port *dp = dsa_slave_to_port(slave_dev);
 	struct device_node *port_dn = dp->dn;
 	struct dsa_switch *ds = dp->ds;
-	phy_interface_t mode;
 	u32 phy_flags = 0;
 	int ret;
 
-	ret = of_get_phy_mode(port_dn, &mode);
-	if (ret)
-		mode = PHY_INTERFACE_MODE_NA;
-
 	dp->pl_config.dev = &slave_dev->dev;
 	dp->pl_config.type = PHYLINK_NETDEV;
 

@@ -1871,17 +1866,9 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 		dp->pl_config.poll_fixed_state = true;
 	}
 
-	if (ds->ops->phylink_get_interfaces)
-		ds->ops->phylink_get_interfaces(ds, dp->index,
-					dp->pl_config.supported_interfaces);
-
-	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(port_dn), mode,
-				&dsa_port_phylink_mac_ops);
-	if (IS_ERR(dp->pl)) {
-		netdev_err(slave_dev,
-			   "error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
-		return PTR_ERR(dp->pl);
-	}
+	ret = dsa_port_phylink_create(dp);
+	if (ret)
+		return ret;
 
 	if (ds->ops->get_phy_flags)
 		phy_flags = ds->ops->get_phy_flags(ds, dp->index);

-- 
2.30.2

Add a boolean to phylink_config to indicate whether a driver has not
been updated for the changes in commit 7cceb599d15d ("net: phylink:
avoid mac_config calls"), and thus are reliant on the old behaviour.

We were keying this behaviour on the presence of a PCS, but this
becomes an unreliable indicator when making PCS optional. Hence, we
use a flag instead.

Signed-off-by: Russell King (Oracle) <redacted>
---
 include/linux/phylink.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 01224235df0f..d005b8e36048 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h

@@ -84,6 +84,8 @@ enum phylink_op_type {
  * struct phylink_config - PHYLINK configuration structure
  * @dev: a pointer to a struct device associated with the MAC
  * @type: operation type of PHYLINK instance
+ * @legacy_pre_march2020: driver has not been updated for March 2020 updates
+ *	(See commit 7cceb599d15d ("net: phylink: avoid mac_config calls")
  * @pcs_poll: MAC PCS cannot provide link change interrupt
  * @poll_fixed_state: if true, starts link_poll,
  *		      if MAC link is at %MLO_AN_FIXED mode.

@@ -97,6 +99,7 @@ enum phylink_op_type {
 struct phylink_config {
 	struct device *dev;
 	enum phylink_op_type type;
+	bool legacy_pre_march2020;
 	bool pcs_poll;
 	bool poll_fixed_state;
 	bool ovr_an_inband;

-- 
2.30.2

As DSA doesn't make use of the PCS support, but it does have PCS, it
must be marked as a pre-March 2020 driver to maintain the old phylink
behaviour.

Signed-off-by: Russell King (Oracle) <redacted>
---
 net/dsa/port.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index eaa66114924b..cfb48dc57f73 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c

@@ -1091,6 +1091,13 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 	if (err)
 		mode = PHY_INTERFACE_MODE_NA;
 
+	/* Presence of phylink_mac_link_state or phylink_mac_an_restart is
+	 * an indicator of a legacy phylink driver.
+	 */
+	if (ds->ops->phylink_mac_link_state ||
+	    ds->ops->phylink_mac_an_restart)
+		dp->pl_config.legacy_pre_march2020 = true;
+
 	if (ds->ops->phylink_get_interfaces)
 		ds->ops->phylink_get_interfaces(ds, dp->index,
 					dp->pl_config.supported_interfaces);

-- 
2.30.2

mtk_eth_soc has not been updated for commit 7cceb599d15d ("net: phylink:
avoid mac_config calls"), so mark it as a legacy driver.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index de4152e2e3e4..a068cf5c970f 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c

@@ -2923,6 +2923,10 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
 
 	mac->phylink_config.dev = &eth->netdev[id]->dev;
 	mac->phylink_config.type = PHYLINK_NETDEV;
+	/* This driver makes use of state->speed/state->duplex in
+	 * mac_config
+	 */
+	mac->phylink_config.legacy_pre_march2020 = true;
 	mac->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
 		MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
 

-- 
2.30.2

On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:

I've read the discussion at
https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
and I still am not sure that I understand what is the use case behind
removing a PCS?

On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:

On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:

quoted

Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
the PCS from phylink. This is only supported on non-legacy drivers
where doing so will have no effect on the mac_config() calling
behaviour.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/phy/phylink.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a935655c39c0..9f0f0e0aad55 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
  * in mac_prepare() or mac_config() methods if it is desired to dynamically
  * change the PCS.
  *
- * Please note that there are behavioural changes with the mac_config()
- * callback if a PCS is present (denoting a newer setup) so removing a PCS
- * is not supported, and if a PCS is going to be used, it must be registered
- * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ * Please note that for legacy phylink users, there are behavioural changes
+ * with the mac_config() callback if a PCS is present (denoting a newer setup)
+ * so removing a PCS is not supported. If a PCS is going to be used, it must
+ * be registered by calling phylink_set_pcs() at the latest in the first
+ * mac_config() call.
+ *
+ * For modern drivers, this may be called with a NULL pcs argument to
+ * disconnect the PCS from phylink.
  */
 void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 {
+	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
+		phylink_warn(pl,
+			     "Removing PCS is not supported in a legacy driver");
+		return;
+	}
+
 	pl->pcs = pcs;
-	pl->pcs_ops = pcs->ops;
+	pl->pcs_ops = pcs ? pcs->ops : NULL;
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);
 

-- 
2.30.2

I've read the discussion at
https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
and I still am not sure that I understand what is the use case behind
removing a PCS?

Passing that to Sean to answer in detail...

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

On 11/23/21 11:08 AM, Russell King (Oracle) wrote:

On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:

quoted

On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:

quoted

Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
the PCS from phylink. This is only supported on non-legacy drivers
where doing so will have no effect on the mac_config() calling
behaviour.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/phy/phylink.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a935655c39c0..9f0f0e0aad55 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
  * in mac_prepare() or mac_config() methods if it is desired to dynamically
  * change the PCS.
  *
- * Please note that there are behavioural changes with the mac_config()
- * callback if a PCS is present (denoting a newer setup) so removing a PCS
- * is not supported, and if a PCS is going to be used, it must be registered
- * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ * Please note that for legacy phylink users, there are behavioural changes
+ * with the mac_config() callback if a PCS is present (denoting a newer setup)
+ * so removing a PCS is not supported. If a PCS is going to be used, it must
+ * be registered by calling phylink_set_pcs() at the latest in the first
+ * mac_config() call.
+ *
+ * For modern drivers, this may be called with a NULL pcs argument to
+ * disconnect the PCS from phylink.
  */
 void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 {
+	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
+		phylink_warn(pl,
+			     "Removing PCS is not supported in a legacy driver");
+		return;
+	}
+
 	pl->pcs = pcs;
-	pl->pcs_ops = pcs->ops;
+	pl->pcs_ops = pcs ? pcs->ops : NULL;
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);

--

2.30.2

I've read the discussion at
https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
and I still am not sure that I understand what is the use case behind
removing a PCS?

Passing that to Sean to answer in detail...

My original feedback was regarding selecting the correct PCS to use. In
response to the question "What PCS do you want to use for this phy
interface mode?" a valid response is "I don't need a PCS," even if for a
different mode a valid response might be "Please use X PCS." Because
this function is used in validate(), it is necessary to evaluate
"what-if" scenarios, even if a scenario requiring a PCS and one
requiring no PCS would never actually be configured.

Typically the PCS is physically attached to the next layer in the link,
even if the hardware could be configured not to use the PCS. So it does
not usually make sense to configure a link to use modes both requiring a
PCS and requiring no PCS. However, it is possible that such a system
could exist. Most systems should use `phy-mode` to restrict the phy
interfaces modes to whatever makes sense for the board. I think Marek's
series (and specifically [1]) is an good step in this regard.

--Sean

[1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/

On Tue, Nov 23, 2021 at 10:00:34AM +0000, Russell King (Oracle) wrote:

ag71xx has a PCS, but does not make use of the phylink PCS support.
Mark it was a pre-March 2020 driver.

Signed-off-by: Russell King (Oracle) <redacted>

Hi,

I've just been looking closer at this driver, and it seems that we can
drop the "legacy_pre_march2020" flag, and in doing so, delete the
ag71xx_mac_pcs_get_state and ag71xx_mac_an_restart functions entirely,
removing them from ag71xx_phylink_mac_ops.

Should this driver need to deal with the PCS - in other words, to
modify the advertisement, then it will need to make use of the
phylink_pcs support.

I'll send a v2 in a day or two.

Thanks!

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

On Tue, Nov 23, 2021 at 12:30:33PM -0500, Sean Anderson wrote:

On 11/23/21 11:08 AM, Russell King (Oracle) wrote:

quoted

On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:

quoted

On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:

quoted

Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
the PCS from phylink. This is only supported on non-legacy drivers
where doing so will have no effect on the mac_config() calling
behaviour.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/phy/phylink.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a935655c39c0..9f0f0e0aad55 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
  * in mac_prepare() or mac_config() methods if it is desired to dynamically
  * change the PCS.
  *
- * Please note that there are behavioural changes with the mac_config()
- * callback if a PCS is present (denoting a newer setup) so removing a PCS
- * is not supported, and if a PCS is going to be used, it must be registered
- * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ * Please note that for legacy phylink users, there are behavioural changes
+ * with the mac_config() callback if a PCS is present (denoting a newer setup)
+ * so removing a PCS is not supported. If a PCS is going to be used, it must
+ * be registered by calling phylink_set_pcs() at the latest in the first
+ * mac_config() call.
+ *
+ * For modern drivers, this may be called with a NULL pcs argument to
+ * disconnect the PCS from phylink.
  */
 void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 {
+	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
+		phylink_warn(pl,
+			     "Removing PCS is not supported in a legacy driver");
+		return;
+	}
+
 	pl->pcs = pcs;
-	pl->pcs_ops = pcs->ops;
+	pl->pcs_ops = pcs ? pcs->ops : NULL;
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);

--

2.30.2

I've read the discussion at
https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
and I still am not sure that I understand what is the use case behind
removing a PCS?

Passing that to Sean to answer in detail...

My original feedback was regarding selecting the correct PCS to use. In
response to the question "What PCS do you want to use for this phy
interface mode?" a valid response is "I don't need a PCS," even if for a
different mode a valid response might be "Please use X PCS."

Yes, but that is not a reason why you'd want to _remove_ one. Just don't
call phylink_set_pcs() in the first place, there you go, no PCS.

Because this function is used in validate(), it is necessary to
evaluate "what-if" scenarios, even if a scenario requiring a PCS and
one requiring no PCS would never actually be configured.

Yes, but on the same port on the same board? The MAC-side PCS is an
integral part of serial Ethernet links, be it modeled as a discrete part
by hardware manufacturers or not. We are effectively talking about a
situation where a serial link would become parallel, or the other way
around. Have you seen such a thing?

Typically the PCS is physically attached to the next layer in the link,
even if the hardware could be configured not to use the PCS. So it does
not usually make sense to configure a link to use modes both requiring a
PCS and requiring no PCS. However, it is possible that such a system
could exist. Most systems should use `phy-mode` to restrict the phy
interfaces modes to whatever makes sense for the board. I think Marek's
series (and specifically [1]) is an good step in this regard.

--Sean

[1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/

Marek's patches are for reconfiguring the SERDES protocol on the same
lanes. But the lanes are still physically there, and you'd need a PCS to
talk to them no matter what you do, they won't magically turn into RGMII.
If you need to switch the MAC PCS you're configuring with another MAC
PCS (within the same hardware block more or less) due to the fact that
the SERDES protocol is changing, that doesn't count as removing the PCS,
does it? Or what are you thinking of when you say PCS? Phylink doesn't
support any other kind of PCS than a MAC-side PCS.

On 11/23/21 1:15 PM, Vladimir Oltean wrote:

On Tue, Nov 23, 2021 at 12:30:33PM -0500, Sean Anderson wrote:

quoted

On 11/23/21 11:08 AM, Russell King (Oracle) wrote:

quoted

On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:

quoted

On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:

quoted

Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
the PCS from phylink. This is only supported on non-legacy drivers
where doing so will have no effect on the mac_config() calling
behaviour.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/phy/phylink.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a935655c39c0..9f0f0e0aad55 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
  * in mac_prepare() or mac_config() methods if it is desired to dynamically
  * change the PCS.
  *
- * Please note that there are behavioural changes with the mac_config()
- * callback if a PCS is present (denoting a newer setup) so removing a PCS
- * is not supported, and if a PCS is going to be used, it must be registered
- * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ * Please note that for legacy phylink users, there are behavioural changes
+ * with the mac_config() callback if a PCS is present (denoting a newer setup)
+ * so removing a PCS is not supported. If a PCS is going to be used, it must
+ * be registered by calling phylink_set_pcs() at the latest in the first
+ * mac_config() call.
+ *
+ * For modern drivers, this may be called with a NULL pcs argument to
+ * disconnect the PCS from phylink.
  */
 void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 {
+	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
+		phylink_warn(pl,
+			     "Removing PCS is not supported in a legacy driver");
+		return;
+	}
+
 	pl->pcs = pcs;
-	pl->pcs_ops = pcs->ops;
+	pl->pcs_ops = pcs ? pcs->ops : NULL;
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);

--

2.30.2

I've read the discussion at
https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
and I still am not sure that I understand what is the use case behind
removing a PCS?

Passing that to Sean to answer in detail...

My original feedback was regarding selecting the correct PCS to use. In
response to the question "What PCS do you want to use for this phy
interface mode?" a valid response is "I don't need a PCS," even if for a
different mode a valid response might be "Please use X PCS."

Yes, but that is not a reason why you'd want to _remove_ one. Just don't
call phylink_set_pcs() in the first place, there you go, no PCS.

Yeah.

quoted

Because this function is used in validate(), it is necessary to
evaluate "what-if" scenarios, even if a scenario requiring a PCS and
one requiring no PCS would never actually be configured.

Yes, but on the same port on the same board? The MAC-side PCS is an
integral part of serial Ethernet links, be it modeled as a discrete part
by hardware manufacturers or not. We are effectively talking about a
situation where a serial link would become parallel, or the other way
around. Have you seen such a thing?

I have not. It's certainly possible to create (since the serial link
often uses different physical pins from the parallel link). I think
we can cross that bridge if/when we ever come to it.

quoted

Typically the PCS is physically attached to the next layer in the link,
even if the hardware could be configured not to use the PCS. So it does
not usually make sense to configure a link to use modes both requiring a
PCS and requiring no PCS. However, it is possible that such a system
could exist. Most systems should use `phy-mode` to restrict the phy
interfaces modes to whatever makes sense for the board. I think Marek's
series (and specifically [1]) is an good step in this regard.

--Sean

[1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/

Marek's patches are for reconfiguring the SERDES protocol on the same
lanes. But the lanes are still physically there, and you'd need a PCS to
talk to them no matter what you do, they won't magically turn into RGMII.
If you need to switch the MAC PCS you're configuring with another MAC
PCS (within the same hardware block more or less) due to the fact that
the SERDES protocol is changing, that doesn't count as removing the PCS,
does it? Or what are you thinking of when you say PCS? Phylink doesn't
support any other kind of PCS than a MAC-side PCS.

I mean that with that patch applied, phylink will no longer try and
validate modes which aren't supported on a particular board (see
phylink_validate_any). Although, it looks like set_pcs never was called
in the validate path in the first place (looks like I misremembered).

--Sean

On Tue, Nov 23, 2021 at 02:04:03PM -0500, Sean Anderson wrote:

On 11/23/21 1:15 PM, Vladimir Oltean wrote:

quoted

On Tue, Nov 23, 2021 at 12:30:33PM -0500, Sean Anderson wrote:

quoted

On 11/23/21 11:08 AM, Russell King (Oracle) wrote:

quoted

On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:

quoted

On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:

quoted

Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
the PCS from phylink. This is only supported on non-legacy drivers
where doing so will have no effect on the mac_config() calling
behaviour.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/phy/phylink.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a935655c39c0..9f0f0e0aad55 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
  * in mac_prepare() or mac_config() methods if it is desired to dynamically
  * change the PCS.
  *
- * Please note that there are behavioural changes with the mac_config()
- * callback if a PCS is present (denoting a newer setup) so removing a PCS
- * is not supported, and if a PCS is going to be used, it must be registered
- * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ * Please note that for legacy phylink users, there are behavioural changes
+ * with the mac_config() callback if a PCS is present (denoting a newer setup)
+ * so removing a PCS is not supported. If a PCS is going to be used, it must
+ * be registered by calling phylink_set_pcs() at the latest in the first
+ * mac_config() call.
+ *
+ * For modern drivers, this may be called with a NULL pcs argument to
+ * disconnect the PCS from phylink.
  */
 void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 {
+	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
+		phylink_warn(pl,
+			     "Removing PCS is not supported in a legacy driver");
+		return;
+	}
+
 	pl->pcs = pcs;
-	pl->pcs_ops = pcs->ops;
+	pl->pcs_ops = pcs ? pcs->ops : NULL;
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);

--

2.30.2

I've read the discussion at
https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
and I still am not sure that I understand what is the use case behind
removing a PCS?

Passing that to Sean to answer in detail...

My original feedback was regarding selecting the correct PCS to use. In
response to the question "What PCS do you want to use for this phy
interface mode?" a valid response is "I don't need a PCS," even if for a
different mode a valid response might be "Please use X PCS."

Yes, but that is not a reason why you'd want to _remove_ one. Just don't
call phylink_set_pcs() in the first place, there you go, no PCS.

Yeah.

quoted

quoted

Because this function is used in validate(), it is necessary to
evaluate "what-if" scenarios, even if a scenario requiring a PCS and
one requiring no PCS would never actually be configured.

Yes, but on the same port on the same board? The MAC-side PCS is an
integral part of serial Ethernet links, be it modeled as a discrete part
by hardware manufacturers or not. We are effectively talking about a
situation where a serial link would become parallel, or the other way
around. Have you seen such a thing?

I have not. It's certainly possible to create (since the serial link
often uses different physical pins from the parallel link). I think
we can cross that bridge if/when we ever come to it.

So what do you mean, something like a MAC which can be routed dynamically
via pinctrl either to RGMII or to a SERDES lane? I would expect that
would require rather intricate interaction with the net device driver.
Can that be done today from user space, even in principle?

quoted

quoted

Typically the PCS is physically attached to the next layer in the link,
even if the hardware could be configured not to use the PCS. So it does
not usually make sense to configure a link to use modes both requiring a
PCS and requiring no PCS. However, it is possible that such a system
could exist. Most systems should use `phy-mode` to restrict the phy
interfaces modes to whatever makes sense for the board. I think Marek's
series (and specifically [1]) is an good step in this regard.

--Sean

[1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/

Marek's patches are for reconfiguring the SERDES protocol on the same
lanes. But the lanes are still physically there, and you'd need a PCS to
talk to them no matter what you do, they won't magically turn into RGMII.
If you need to switch the MAC PCS you're configuring with another MAC
PCS (within the same hardware block more or less) due to the fact that
the SERDES protocol is changing, that doesn't count as removing the PCS,
does it? Or what are you thinking of when you say PCS? Phylink doesn't
support any other kind of PCS than a MAC-side PCS.

I mean that with that patch applied, phylink will no longer try and
validate modes which aren't supported on a particular board (see
phylink_validate_any). Although, it looks like set_pcs never was called
in the validate path in the first place (looks like I misremembered).

Sorry if you feel like I am asking too many questions. I just want to
understand what I'm being asked to review here :)

So going back to the initial question. What use case do these patches
help to make some progress with?

As far as I understand, being able to call phylink_set_pcs(NULL) is
basically the end goal of it. Sorry if I'm misinterpreting, Russell says
in the cover letter that "we now need to allow the PCS to be optional
for modern drivers". I really don't know how to interpret what
"optional" means. Just judging from that phrase, I don't interpret
"optional" as "having the ability to remove it dynamically", but rather
"the same driver can either interact with phylink using a pcs [on some
ports] or without a pcs [on other ports]".  But that deeply confuses me
because that's already supported, and many drivers make use of that
ability already, this is why the pl->pcs_ops checks are there.

On Tue, Nov 23, 2021 at 09:30:17PM +0200, Vladimir Oltean wrote:

Sorry if you feel like I am asking too many questions. I just want to
understand what I'm being asked to review here :)

So going back to the initial question. What use case do these patches
help to make some progress with?

If we exclude patch 8, this series:

1) identifies all those drivers that are reliant on the legacy behaviour
   of phylink, which can then be targetted for modernisation - some of
   which may be trivial to do. ag71xx and axienet have turned out to be
   two drivers that can be trivially converted.

2) hopefully stops the legacy use finding its way into new drivers by
   making it easier to spot in review, but hopefully people will realise
   that setting the legacy flag in their driver to use the old hooks is
   something they probably want to avoid.

3) gives consistent phylink behaviour to modern drivers which may or
   may not decide to register a PCS with phylink.

(3) is probably the most important point for any driver that registers
a PCS conditionally. Right now, any driver that does this gets a
slightly different behaviour from phylink as detailed in patch 7.

I would like to remove the legacy code and old .mac_pcs_get_state and
.mac_an_restart callbacks some day...

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