[PATCH net-next v1 1/2] net: asix: ax88772: migrate to phylink

[PATCH net-next v1 1/2] net: asix: ax88772: migrate to phylink

[Oleksij Rempel]

There are some exotic ax88772 based devices which may require
functionality provide by the phylink framework. For example:
- US100A20SFP, USB 2.0 auf LWL Converter with SFP Cage
- AX88772B USB to 100Base-TX Ethernet (with RMII) demo board, where it
  is possible to switch between internal PHY and external RMII based
  connection.

So, convert this driver to phylink as soon as possible.

Tested with:
- AX88772A + internal PHY
- AX88772B + external DP83TD510E T1L PHY

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/Kconfig        |   2 +-
 drivers/net/usb/asix.h         |   3 +
 drivers/net/usb/asix_devices.c | 123 ++++++++++++++++++++++++++++-----
 3 files changed, 110 insertions(+), 18 deletions(-)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index e62fc4f2aee0..3d46b5f9287a 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -168,7 +168,7 @@ config USB_NET_AX8817X
 	tristate "ASIX AX88xxx Based USB 2.0 Ethernet Adapters"
 	depends on USB_USBNET
 	select CRC32
-	select PHYLIB
+	select PHYLINK
 	select AX88796B_PHY
 	imply NET_SELFTESTS
 	default y
diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
index 21c1ca275cc4..74162190bccc 100644
--- a/drivers/net/usb/asix.h
+++ b/drivers/net/usb/asix.h
@@ -27,6 +27,7 @@
 #include <linux/if_vlan.h>
 #include <linux/phy.h>
 #include <net/selftests.h>
+#include <linux/phylink.h>
 
 #define DRIVER_VERSION "22-Dec-2011"
 #define DRIVER_NAME "asix"
@@ -185,6 +186,8 @@ struct asix_common_private {
 	struct mii_bus *mdio;
 	struct phy_device *phydev;
 	struct phy_device *phydev_int;
+	struct phylink *phylink;
+	struct phylink_config phylink_config;
 	u16 phy_addr;
 	bool embd_phy;
 	u8 chipcode;
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 5b5eb630c4b7..3f93bc46a7eb 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -327,6 +327,12 @@ static int ax88772_reset(struct usbnet *dev)
 	struct asix_common_private *priv = dev->driver_priv;
 	int ret;
 
+	ret = phylink_connect_phy(priv->phylink, priv->phydev);
+	if (ret) {
+		netdev_err(dev->net, "Could not connect PHY\n");
+		return ret;
+	}
+
 	/* Rewrite MAC address */
 	ether_addr_copy(data->mac_addr, dev->net->dev_addr);
 	ret = asix_write_cmd(dev, AX_CMD_WRITE_NODE_ID, 0, 0,
@@ -343,7 +349,7 @@ static int ax88772_reset(struct usbnet *dev)
 	if (ret < 0)
 		goto out;
 
-	phy_start(priv->phydev);
+	phylink_start(priv->phylink);
 
 	return 0;
 
@@ -590,8 +596,11 @@ static void ax88772_suspend(struct usbnet *dev)
 	struct asix_common_private *priv = dev->driver_priv;
 	u16 medium;
 
-	if (netif_running(dev->net))
-		phy_stop(priv->phydev);
+	if (netif_running(dev->net)) {
+		rtnl_lock();
+		phylink_suspend(priv->phylink, false);
+		rtnl_unlock();
+	}
 
 	/* Stop MAC operation */
 	medium = asix_read_medium_status(dev, 1);
@@ -622,8 +631,11 @@ static void ax88772_resume(struct usbnet *dev)
 		if (!priv->reset(dev, 1))
 			break;
 
-	if (netif_running(dev->net))
-		phy_start(priv->phydev);
+	if (netif_running(dev->net)) {
+		rtnl_lock();
+		phylink_resume(priv->phylink);
+		rtnl_unlock();
+	}
 }
 
 static int asix_resume(struct usb_interface *intf)
@@ -659,7 +671,6 @@ static int ax88772_init_mdio(struct usbnet *dev)
 static int ax88772_init_phy(struct usbnet *dev)
 {
 	struct asix_common_private *priv = dev->driver_priv;
-	int ret;
 
 	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
 	if (!priv->phydev) {
@@ -667,13 +678,6 @@ static int ax88772_init_phy(struct usbnet *dev)
 		return -ENODEV;
 	}
 
-	ret = phy_connect_direct(dev->net, priv->phydev, &asix_adjust_link,
-				 PHY_INTERFACE_MODE_INTERNAL);
-	if (ret) {
-		netdev_err(dev->net, "Could not connect PHY\n");
-		return ret;
-	}
-
 	phy_suspend(priv->phydev);
 	priv->phydev->mac_managed_pm = 1;
 
@@ -698,6 +702,89 @@ static int ax88772_init_phy(struct usbnet *dev)
 	return 0;
 }
 
+static void ax88772_mac_config(struct phylink_config *config, unsigned int mode,
+			      const struct phylink_link_state *state)
+{
+	/* Nothing to do */
+}
+
+static void ax88772_mac_link_down(struct phylink_config *config,
+				 unsigned int mode, phy_interface_t interface)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
+
+	asix_write_medium_mode(dev, 0, 0);
+	usbnet_link_change(dev, false, false);
+}
+
+static void ax88772_mac_link_up(struct phylink_config *config,
+			       struct phy_device *phy,
+			       unsigned int mode, phy_interface_t interface,
+			       int speed, int duplex,
+			       bool tx_pause, bool rx_pause)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
+	u16 m = AX_MEDIUM_AC | AX_MEDIUM_RE;
+
+	m |= duplex ? AX_MEDIUM_FD : 0;
+
+	switch (speed) {
+	case SPEED_100:
+		m |= AX_MEDIUM_PS;
+		break;
+	case SPEED_10:
+		break;
+	default:
+		return;
+	}
+
+	if (tx_pause)
+		m |= AX_MEDIUM_TFC;
+
+	if (rx_pause)
+		m |= AX_MEDIUM_RFC;
+
+	asix_write_medium_mode(dev, m, 0);
+	usbnet_link_change(dev, true, false);
+}
+
+static const struct phylink_mac_ops ax88772_phylink_mac_ops = {
+	.validate = phylink_generic_validate,
+	.mac_config = ax88772_mac_config,
+	.mac_link_down = ax88772_mac_link_down,
+	.mac_link_up = ax88772_mac_link_up,
+};
+
+static int ax88772_phylink_setup(struct usbnet *dev)
+{
+	struct asix_common_private *priv = dev->driver_priv;
+	phy_interface_t phy_if_mode;
+	struct phylink *phylink;
+
+	priv->phylink_config.dev = &dev->net->dev;
+	priv->phylink_config.type = PHYLINK_NETDEV;
+	priv->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
+		MAC_10 | MAC_100;
+
+	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+		  priv->phylink_config.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_RMII,
+		  priv->phylink_config.supported_interfaces);
+
+	if (priv->embd_phy)
+		phy_if_mode = PHY_INTERFACE_MODE_INTERNAL;
+	else
+		phy_if_mode = PHY_INTERFACE_MODE_RMII;
+
+	phylink = phylink_create(&priv->phylink_config, dev->net->dev.fwnode,
+				 phy_if_mode, &ax88772_phylink_mac_ops);
+	if (IS_ERR(phylink))
+		return PTR_ERR(phylink);
+
+	priv->phylink = phylink;
+	return 0;
+}
+
 static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct asix_common_private *priv;
@@ -788,6 +875,10 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
 	if (ret)
 		return ret;
 
+	ret = ax88772_phylink_setup(dev);
+	if (ret)
+		return ret;
+
 	return ax88772_init_phy(dev);
 }
 
@@ -795,16 +886,14 @@ static int ax88772_stop(struct usbnet *dev)
 {
 	struct asix_common_private *priv = dev->driver_priv;
 
-	phy_stop(priv->phydev);
+	phylink_stop(priv->phylink);
+	phylink_disconnect_phy(priv->phylink);
 
 	return 0;
 }
 
 static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
-	struct asix_common_private *priv = dev->driver_priv;
-
-	phy_disconnect(priv->phydev);
 	asix_rx_fixup_common_free(dev->driver_priv);
 }
 
-- 
2.30.2

[PATCH net-next v1 2/2] net: asix: ax88772: add ethtool pause configuration

[Oleksij Rempel]

Add phylink based ethtool pause configuration

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/asix_devices.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 3f93bc46a7eb..4ba4f4251300 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -303,6 +303,24 @@ static int ax88772_ethtool_get_sset_count(struct net_device *ndev, int sset)
 	}
 }
 
+static void ax88772_ethtool_get_pauseparam(struct net_device *ndev,
+					  struct ethtool_pauseparam *pause)
+{
+	struct usbnet *dev = netdev_priv(ndev);
+	struct asix_common_private *priv = dev->driver_priv;
+
+	phylink_ethtool_get_pauseparam(priv->phylink, pause);
+}
+
+static int ax88772_ethtool_set_pauseparam(struct net_device *ndev,
+					 struct ethtool_pauseparam *pause)
+{
+	struct usbnet *dev = netdev_priv(ndev);
+	struct asix_common_private *priv = dev->driver_priv;
+
+	return phylink_ethtool_set_pauseparam(priv->phylink, pause);
+}
+
 static const struct ethtool_ops ax88772_ethtool_ops = {
 	.get_drvinfo		= asix_get_drvinfo,
 	.get_link		= usbnet_get_link,
@@ -319,6 +337,8 @@ static const struct ethtool_ops ax88772_ethtool_ops = {
 	.self_test		= net_selftest,
 	.get_strings		= ax88772_ethtool_get_strings,
 	.get_sset_count		= ax88772_ethtool_get_sset_count,
+	.get_pauseparam		= ax88772_ethtool_get_pauseparam,
+	.set_pauseparam		= ax88772_ethtool_set_pauseparam,
 };
 
 static int ax88772_reset(struct usbnet *dev)
-- 
2.30.2

Re: [PATCH net-next v1 1/2] net: asix: ax88772: migrate to phylink

[Paolo Abeni]

On Sat, 2022-07-23 at 19:47 +0200, Oleksij Rempel wrote:
> There are some exotic ax88772 based devices which may require
> functionality provide by the phylink framework. For example:
> - US100A20SFP, USB 2.0 auf LWL Converter with SFP Cage
> - AX88772B USB to 100Base-TX Ethernet (with RMII) demo board, where it
>   is possible to switch between internal PHY and external RMII based
>   connection.
> 
> So, convert this driver to phylink as soon as possible.
> 
> Tested with:
> - AX88772A + internal PHY
> - AX88772B + external DP83TD510E T1L PHY
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/usb/Kconfig        |   2 +-
>  drivers/net/usb/asix.h         |   3 +
>  drivers/net/usb/asix_devices.c | 123 ++++++++++++++++++++++++++++-----
>  3 files changed, 110 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index e62fc4f2aee0..3d46b5f9287a 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -168,7 +168,7 @@ config USB_NET_AX8817X
>  	tristate "ASIX AX88xxx Based USB 2.0 Ethernet Adapters"
>  	depends on USB_USBNET
>  	select CRC32
> -	select PHYLIB
> +	select PHYLINK
>  	select AX88796B_PHY
>  	imply NET_SELFTESTS
>  	default y
> diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
> index 21c1ca275cc4..74162190bccc 100644
> --- a/drivers/net/usb/asix.h
> +++ b/drivers/net/usb/asix.h
> @@ -27,6 +27,7 @@
>  #include <linux/if_vlan.h>
>  #include <linux/phy.h>
>  #include <net/selftests.h>
> +#include <linux/phylink.h>
>  
>  #define DRIVER_VERSION "22-Dec-2011"
>  #define DRIVER_NAME "asix"
> @@ -185,6 +186,8 @@ struct asix_common_private {
>  	struct mii_bus *mdio;
>  	struct phy_device *phydev;
>  	struct phy_device *phydev_int;
> +	struct phylink *phylink;
> +	struct phylink_config phylink_config;
>  	u16 phy_addr;
>  	bool embd_phy;
>  	u8 chipcode;
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 5b5eb630c4b7..3f93bc46a7eb 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -327,6 +327,12 @@ static int ax88772_reset(struct usbnet *dev)
>  	struct asix_common_private *priv = dev->driver_priv;
>  	int ret;
>  
> +	ret = phylink_connect_phy(priv->phylink, priv->phydev);
> +	if (ret) {
> +		netdev_err(dev->net, "Could not connect PHY\n");
> +		return ret;
> +	}

Don't you need to additionally call phylink_disconnect_phy() in later
error paths? why?

> +
>  	/* Rewrite MAC address */
>  	ether_addr_copy(data->mac_addr, dev->net->dev_addr);
>  	ret = asix_write_cmd(dev, AX_CMD_WRITE_NODE_ID, 0, 0,
> @@ -343,7 +349,7 @@ static int ax88772_reset(struct usbnet *dev)
>  	if (ret < 0)
>  		goto out;
>  
> -	phy_start(priv->phydev);
> +	phylink_start(priv->phylink);
>  
>  	return 0;
>  
> @@ -590,8 +596,11 @@ static void ax88772_suspend(struct usbnet *dev)
>  	struct asix_common_private *priv = dev->driver_priv;
>  	u16 medium;
>  
> -	if (netif_running(dev->net))
> -		phy_stop(priv->phydev);
> +	if (netif_running(dev->net)) {
> +		rtnl_lock();
> +		phylink_suspend(priv->phylink, false);
> +		rtnl_unlock();
> +	}
>  
>  	/* Stop MAC operation */
>  	medium = asix_read_medium_status(dev, 1);
> @@ -622,8 +631,11 @@ static void ax88772_resume(struct usbnet *dev)
>  		if (!priv->reset(dev, 1))
>  			break;
>  
> -	if (netif_running(dev->net))
> -		phy_start(priv->phydev);
> +	if (netif_running(dev->net)) {
> +		rtnl_lock();
> +		phylink_resume(priv->phylink);
> +		rtnl_unlock();
> +	}
>  }
>  
>  static int asix_resume(struct usb_interface *intf)
> @@ -659,7 +671,6 @@ static int ax88772_init_mdio(struct usbnet *dev)
>  static int ax88772_init_phy(struct usbnet *dev)
>  {
>  	struct asix_common_private *priv = dev->driver_priv;
> -	int ret;
>  
>  	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
>  	if (!priv->phydev) {
> @@ -667,13 +678,6 @@ static int ax88772_init_phy(struct usbnet *dev)
>  		return -ENODEV;
>  	}
>  
> -	ret = phy_connect_direct(dev->net, priv->phydev, &asix_adjust_link,
> -				 PHY_INTERFACE_MODE_INTERNAL);
> -	if (ret) {
> -		netdev_err(dev->net, "Could not connect PHY\n");
> -		return ret;
> -	}
> -
>  	phy_suspend(priv->phydev);
>  	priv->phydev->mac_managed_pm = 1;
>  
> @@ -698,6 +702,89 @@ static int ax88772_init_phy(struct usbnet *dev)
>  	return 0;
>  }
>  
> +static void ax88772_mac_config(struct phylink_config *config, unsigned int mode,
> +			      const struct phylink_link_state *state)
> +{
> +	/* Nothing to do */
> +}
> +
> +static void ax88772_mac_link_down(struct phylink_config *config,
> +				 unsigned int mode, phy_interface_t interface)
> +{
> +	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
> +
> +	asix_write_medium_mode(dev, 0, 0);
> +	usbnet_link_change(dev, false, false);
> +}
> +
> +static void ax88772_mac_link_up(struct phylink_config *config,
> +			       struct phy_device *phy,
> +			       unsigned int mode, phy_interface_t interface,
> +			       int speed, int duplex,
> +			       bool tx_pause, bool rx_pause)
> +{
> +	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
> +	u16 m = AX_MEDIUM_AC | AX_MEDIUM_RE;
> +
> +	m |= duplex ? AX_MEDIUM_FD : 0;
> +
> +	switch (speed) {
> +	case SPEED_100:
> +		m |= AX_MEDIUM_PS;
> +		break;
> +	case SPEED_10:
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	if (tx_pause)
> +		m |= AX_MEDIUM_TFC;
> +
> +	if (rx_pause)
> +		m |= AX_MEDIUM_RFC;
> +
> +	asix_write_medium_mode(dev, m, 0);
> +	usbnet_link_change(dev, true, false);
> +}
> +
> +static const struct phylink_mac_ops ax88772_phylink_mac_ops = {
> +	.validate = phylink_generic_validate,
> +	.mac_config = ax88772_mac_config,
> +	.mac_link_down = ax88772_mac_link_down,
> +	.mac_link_up = ax88772_mac_link_up,
> +};
> +
> +static int ax88772_phylink_setup(struct usbnet *dev)
> +{
> +	struct asix_common_private *priv = dev->driver_priv;
> +	phy_interface_t phy_if_mode;
> +	struct phylink *phylink;
> +
> +	priv->phylink_config.dev = &dev->net->dev;
> +	priv->phylink_config.type = PHYLINK_NETDEV;
> +	priv->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> +		MAC_10 | MAC_100;
> +
> +	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +		  priv->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_RMII,
> +		  priv->phylink_config.supported_interfaces);
> +
> +	if (priv->embd_phy)
> +		phy_if_mode = PHY_INTERFACE_MODE_INTERNAL;
> +	else
> +		phy_if_mode = PHY_INTERFACE_MODE_RMII;
> +
> +	phylink = phylink_create(&priv->phylink_config, dev->net->dev.fwnode,
> +				 phy_if_mode, &ax88772_phylink_mac_ops);
> +	if (IS_ERR(phylink))
> +		return PTR_ERR(phylink);
> +
> +	priv->phylink = phylink;

Who will call phylink_destroy() on priv->phylink? It looks like you are
leaking it ?!?

Thanks!

Paolo

Re: [PATCH net-next v1 1/2] net: asix: ax88772: migrate to phylink

[Russell King (Oracle)]

On Sat, Jul 23, 2022 at 07:47:10PM +0200, Oleksij Rempel wrote:
> There are some exotic ax88772 based devices which may require
> functionality provide by the phylink framework. For example:
> - US100A20SFP, USB 2.0 auf LWL Converter with SFP Cage
> - AX88772B USB to 100Base-TX Ethernet (with RMII) demo board, where it
>   is possible to switch between internal PHY and external RMII based
>   connection.
> 
> So, convert this driver to phylink as soon as possible.
> 
> Tested with:
> - AX88772A + internal PHY
> - AX88772B + external DP83TD510E T1L PHY
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Sorry, I missed this.

> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 5b5eb630c4b7..3f93bc46a7eb 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -327,6 +327,12 @@ static int ax88772_reset(struct usbnet *dev)
>  	struct asix_common_private *priv = dev->driver_priv;
>  	int ret;
>  
> +	ret = phylink_connect_phy(priv->phylink, priv->phydev);
> +	if (ret) {
> +		netdev_err(dev->net, "Could not connect PHY\n");
> +		return ret;
> +	}
> +

As Paolo points out, you need to clean this up with a call to
phylink_disconnect_phy() in the error paths in this function. Sorting
that out doesn't look like it would be sufficient, however. Looking at
usbnet_open(), there are a bunch of paths after ->reset() has been
called, where failures can occur and the chip specific driver doesn't
get notified to clean up, so in that case, the PHY would remain
attached.

So, I would say that the usbnet code is not structured to allow a call
to bind the PHY at this point.

However, this is also a functional change to the code - moving the call
to connect the PHY to (presumably) the .ndo_open() stage of the network
device lifetime, rather than the probe stage. In other words, this now
happens when the device is actually opened rather than when the device
is being bound to the driver.

That should have at least been described in the commit description, but
much better would be a separate patch to make that change. However, as
I say above, I think the usbnet code would need additional work to
permit this.

> +static void ax88772_mac_link_up(struct phylink_config *config,
> +			       struct phy_device *phy,
> +			       unsigned int mode, phy_interface_t interface,
> +			       int speed, int duplex,
> +			       bool tx_pause, bool rx_pause)
> +{
> +	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
> +	u16 m = AX_MEDIUM_AC | AX_MEDIUM_RE;
> +
> +	m |= duplex ? AX_MEDIUM_FD : 0;

I'd much rather this was:

	m |= duplex == DUPLEX_FULL ? AX_MEDIUM_FD : 0;

which is much clearer, and doesn't result in other values of duplex
resulting in full duplex. If you do want that behaviour (because the
original code used to), then please use:

	m |= duplex != DUPLEX_HALF ? AX_MEDIUM_FD : 0;

Either of these make the intention here explicit.

Thanks.

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