diff mbox series

[REPOST] usbnet: asix: leave the carrier control to phylink

Message ID m35xjgdvih.fsf@t19.piap.pl (mailing list archive)
State New
Headers show
Series [REPOST] usbnet: asix: leave the carrier control to phylink | expand

Commit Message

Krzysztof Hałasa April 7, 2025, 12:08 p.m. UTC 
[added Oleksij - the author of the phylink code for this driver]

ASIX AX88772B based USB 10/100 Ethernet adapter doesn't come
up ("carrier off"), despite the built-in 100BASE-FX PHY positive link
indication. The internal PHY is configured (using EEPROM) in fixed
100 Mbps full duplex mode.

The primary problem appears to be using carrier_netif_{on,off}() while,
at the same time, delegating carrier management to phylink. Use only the
latter and remove "manual control" in the asix driver.

I don't have any other AX88772 board here, but the problem doesn't seem
specific to a particular board or settings - it's probably
timing-dependent.

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

Comments

Oleksij Rempel April 7, 2025, 1:38 p.m. UTC | #1 
Hi Krzysztof,

On Mon, Apr 07, 2025 at 02:08:22PM +0200, Krzysztof Hałasa wrote:
> [added Oleksij - the author of the phylink code for this driver]
> 
> ASIX AX88772B based USB 10/100 Ethernet adapter doesn't come
> up ("carrier off"), despite the built-in 100BASE-FX PHY positive link
> indication. The internal PHY is configured (using EEPROM) in fixed
> 100 Mbps full duplex mode.
>
> The primary problem appears to be using carrier_netif_{on,off}() while,
> at the same time, delegating carrier management to phylink. Use only the
> latter and remove "manual control" in the asix driver.

Good point, this artifact should be partially removed, but not for all
devices.  Only ax88772 are converted to PHYlink. ax88178 are not
converted.

> I don't have any other AX88772 board here, but the problem doesn't seem
> specific to a particular board or settings - it's probably
> timing-dependent.

The AX88772 portion of the driver, is not forwarding the interrupt to
the PHY driver. It means, PHY is in polling mode. As long as PHY
provides proper information, it will work.

On other hand, you seems to use AX88772B in 100BASE-FX mode. I'm sure,
current PHY driver for this device do not know anything about FX mode:
drivers/net/phy/ax88796b.c

Which 100BASE-FX PHY  capable device do you use? Is it possible to buy
it some where?

> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> 
> diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
> index 74162190bccc..8531b804021a 100644
> --- a/drivers/net/usb/asix.h
> +++ b/drivers/net/usb/asix.h
> @@ -224,7 +224,6 @@ int asix_write_rx_ctl(struct usbnet *dev, u16 mode, int in_pm);
>  
>  u16 asix_read_medium_status(struct usbnet *dev, int in_pm);
>  int asix_write_medium_mode(struct usbnet *dev, u16 mode, int in_pm);
> -void asix_adjust_link(struct net_device *netdev);
>  
>  int asix_write_gpio(struct usbnet *dev, u16 value, int sleep, int in_pm);
>  
> diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> index 72ffc89b477a..7fd763917ae2 100644
> --- a/drivers/net/usb/asix_common.c
> +++ b/drivers/net/usb/asix_common.c
> @@ -414,28 +414,6 @@ int asix_write_medium_mode(struct usbnet *dev, u16 mode, int in_pm)
>  	return ret;
>  }
>  
> -/* set MAC link settings according to information from phylib */
> -void asix_adjust_link(struct net_device *netdev)
> -{
> -	struct phy_device *phydev = netdev->phydev;
> -	struct usbnet *dev = netdev_priv(netdev);
> -	u16 mode = 0;
> -
> -	if (phydev->link) {
> -		mode = AX88772_MEDIUM_DEFAULT;
> -
> -		if (phydev->duplex == DUPLEX_HALF)
> -			mode &= ~AX_MEDIUM_FD;
> -
> -		if (phydev->speed != SPEED_100)
> -			mode &= ~AX_MEDIUM_PS;
> -	}
> -
> -	asix_write_medium_mode(dev, mode, 0);
> -	phy_print_status(phydev);
> -	usbnet_link_change(dev, phydev->link, 0);
> -}
> -
>  int asix_write_gpio(struct usbnet *dev, u16 value, int sleep, int in_pm)
>  {
>  	int ret;
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 57d6e5abc30e..af91fc947f40 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -40,22 +40,6 @@ struct ax88172_int_data {
>  	__le16 res3;
>  } __packed;
>  
> -static void asix_status(struct usbnet *dev, struct urb *urb)
> -{
> -	struct ax88172_int_data *event;
> -	int link;
> -
> -	if (urb->actual_length < 8)
> -		return;
> -
> -	event = urb->transfer_buffer;
> -	link = event->link & 0x01;
> -	if (netif_carrier_ok(dev->net) != link) {
> -		usbnet_link_change(dev, link, 1);
> -		netdev_dbg(dev->net, "Link Status is: %d\n", link);
> -	}
> -}
> -
>  static void asix_set_netdev_dev_addr(struct usbnet *dev, u8 *addr)
>  {
>  	if (is_valid_ether_addr(addr)) {
> @@ -752,7 +736,6 @@ static void ax88772_mac_link_down(struct phylink_config *config,
>  	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,
> @@ -783,7 +766,6 @@ static void ax88772_mac_link_up(struct phylink_config *config,
>  		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 = {
> @@ -1309,40 +1291,36 @@ static int ax88178_bind(struct usbnet *dev, struct usb_interface *intf)
>  static const struct driver_info ax8817x_info = {
>  	.description = "ASIX AX8817x USB 2.0 Ethernet",
>  	.bind = ax88172_bind,
> -	.status = asix_status,
>  	.link_reset = ax88172_link_reset,
>  	.reset = ax88172_link_reset,
> -	.flags =  FLAG_ETHER | FLAG_LINK_INTR,
> +	.flags =  FLAG_ETHER,
>  	.data = 0x00130103,
>  };
>  
>  static const struct driver_info dlink_dub_e100_info = {
>  	.description = "DLink DUB-E100 USB Ethernet",
>  	.bind = ax88172_bind,
> -	.status = asix_status,
>  	.link_reset = ax88172_link_reset,
>  	.reset = ax88172_link_reset,
> -	.flags =  FLAG_ETHER | FLAG_LINK_INTR,
> +	.flags =  FLAG_ETHER,
>  	.data = 0x009f9d9f,
>  };
>  
>  static const struct driver_info netgear_fa120_info = {
>  	.description = "Netgear FA-120 USB Ethernet",
>  	.bind = ax88172_bind,
> -	.status = asix_status,
>  	.link_reset = ax88172_link_reset,
>  	.reset = ax88172_link_reset,
> -	.flags =  FLAG_ETHER | FLAG_LINK_INTR,
> +	.flags =  FLAG_ETHER,
>  	.data = 0x00130103,
>  };
>  
>  static const struct driver_info hawking_uf200_info = {
>  	.description = "Hawking UF200 USB Ethernet",
>  	.bind = ax88172_bind,
> -	.status = asix_status,
>  	.link_reset = ax88172_link_reset,
>  	.reset = ax88172_link_reset,
> -	.flags =  FLAG_ETHER | FLAG_LINK_INTR,
> +	.flags =  FLAG_ETHER,
>  	.data = 0x001f1d1f,
>  };
>  
> @@ -1350,10 +1328,9 @@ static const struct driver_info ax88772_info = {
>  	.description = "ASIX AX88772 USB 2.0 Ethernet",
>  	.bind = ax88772_bind,
>  	.unbind = ax88772_unbind,
> -	.status = asix_status,
>  	.reset = ax88772_reset,
>  	.stop = ax88772_stop,
> -	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR | FLAG_MULTI_PACKET,
> +	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
>  	.rx_fixup = asix_rx_fixup_common,
>  	.tx_fixup = asix_tx_fixup,
>  };
> @@ -1362,11 +1339,9 @@ static const struct driver_info ax88772b_info = {
>  	.description = "ASIX AX88772B USB 2.0 Ethernet",
>  	.bind = ax88772_bind,
>  	.unbind = ax88772_unbind,
> -	.status = asix_status,
>  	.reset = ax88772_reset,
>  	.stop = ax88772_stop,
> -	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
> -	         FLAG_MULTI_PACKET,
> +	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
>  	.rx_fixup = asix_rx_fixup_common,
>  	.tx_fixup = asix_tx_fixup,
>  	.data = FLAG_EEPROM_MAC,
> @@ -1376,11 +1351,9 @@ static const struct driver_info lxausb_t1l_info = {
>  	.description = "Linux Automation GmbH USB 10Base-T1L",
>  	.bind = ax88772_bind,
>  	.unbind = ax88772_unbind,
> -	.status = asix_status,
>  	.reset = ax88772_reset,
>  	.stop = ax88772_stop,
> -	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
> -		 FLAG_MULTI_PACKET,
> +	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
>  	.rx_fixup = asix_rx_fixup_common,
>  	.tx_fixup = asix_tx_fixup,
>  	.data = FLAG_EEPROM_MAC,
> @@ -1390,11 +1363,9 @@ static const struct driver_info ax88178_info = {
>  	.description = "ASIX AX88178 USB 2.0 Ethernet",
>  	.bind = ax88178_bind,
>  	.unbind = ax88178_unbind,
> -	.status = asix_status,
>  	.link_reset = ax88178_link_reset,
>  	.reset = ax88178_reset,
> -	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
> -		 FLAG_MULTI_PACKET,
> +	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
>  	.rx_fixup = asix_rx_fixup_common,
>  	.tx_fixup = asix_tx_fixup,
>  };
> @@ -1412,10 +1383,8 @@ static const struct driver_info hg20f9_info = {
>  	.description = "HG20F9 USB 2.0 Ethernet",
>  	.bind = ax88772_bind,
>  	.unbind = ax88772_unbind,
> -	.status = asix_status,
>  	.reset = ax88772_reset,
> -	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
> -	         FLAG_MULTI_PACKET,
> +	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
>  	.rx_fixup = asix_rx_fixup_common,
>  	.tx_fixup = asix_tx_fixup,
>  	.data = FLAG_EEPROM_MAC,
> 
> -- 
> Krzysztof "Chris" Hałasa
> 
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
> 
>
Krzysztof Hałasa April 8, 2025, 11:55 a.m. UTC | #2 
Oleksij,

thanks for your fast response.

Oleksij Rempel <o.rempel@pengutronix.de> writes:

> Good point, this artifact should be partially removed, but not for all
> devices.  Only ax88772 are converted to PHYlink. ax88178 are not
> converted.

There is also AX88172. I assume the situation with 172 and 178 is
similar.

> The AX88772 portion of the driver, is not forwarding the interrupt to
> the PHY driver. It means, PHY is in polling mode. As long as PHY
> provides proper information, it will work.

It does, yes.

> On other hand, you seems to use AX88772B in 100BASE-FX mode. I'm sure,
> current PHY driver for this device do not know anything about FX mode:
> drivers/net/phy/ax88796b.c
>
> Which 100BASE-FX PHY  capable device do you use? Is it possible to buy
> it some where?

No, it's a part of custom hw, but the carrier problem seems to be
independent of the actual PHY type. The PHY code needs a bit of fixing
as well, though (one can't really enable autoneg with 100BASE-FX).

Will attach a version with 8817x parts removed.
Paolo Abeni April 8, 2025, 1:29 p.m. UTC | #3 
On 4/7/25 2:08 PM, Krzysztof Hałasa wrote:
> [added Oleksij - the author of the phylink code for this driver]
> 
> ASIX AX88772B based USB 10/100 Ethernet adapter doesn't come
> up ("carrier off"), despite the built-in 100BASE-FX PHY positive link
> indication. The internal PHY is configured (using EEPROM) in fixed
> 100 Mbps full duplex mode.
> 
> The primary problem appears to be using carrier_netif_{on,off}() while,
> at the same time, delegating carrier management to phylink. Use only the
> latter and remove "manual control" in the asix driver.
> 
> I don't have any other AX88772 board here, but the problem doesn't seem
> specific to a particular board or settings - it's probably
> timing-dependent.
> 
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

Does not build:

../drivers/net/usb/asix_devices.c:1396:19: error: ‘asix_status’
undeclared here (not in a function); did you mean ‘si_status’?
 1396 |         .status = asix_status,
      |                   ^~~~~~~~~~~
      |                   si_status

/P
Krzysztof Hałasa April 9, 2025, 9:07 a.m. UTC | #4 
Paolo Abeni <pabeni@redhat.com> writes:

> Does not build:
>
> ../drivers/net/usb/asix_devices.c:1396:19: error: ‘asix_status’
> undeclared here (not in a function); did you mean ‘si_status’?
>  1396 |         .status = asix_status,

Right, thanks - somehow I didn't realize the recent addition in net-next
will break it. Should have been obvious.

Not a factor in v2, though.
diff mbox series

Patch

diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
index 74162190bccc..8531b804021a 100644
--- a/drivers/net/usb/asix.h
+++ b/drivers/net/usb/asix.h
@@ -224,7 +224,6 @@  int asix_write_rx_ctl(struct usbnet *dev, u16 mode, int in_pm);
 
 u16 asix_read_medium_status(struct usbnet *dev, int in_pm);
 int asix_write_medium_mode(struct usbnet *dev, u16 mode, int in_pm);
-void asix_adjust_link(struct net_device *netdev);
 
 int asix_write_gpio(struct usbnet *dev, u16 value, int sleep, int in_pm);
 
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 72ffc89b477a..7fd763917ae2 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -414,28 +414,6 @@  int asix_write_medium_mode(struct usbnet *dev, u16 mode, int in_pm)
 	return ret;
 }
 
-/* set MAC link settings according to information from phylib */
-void asix_adjust_link(struct net_device *netdev)
-{
-	struct phy_device *phydev = netdev->phydev;
-	struct usbnet *dev = netdev_priv(netdev);
-	u16 mode = 0;
-
-	if (phydev->link) {
-		mode = AX88772_MEDIUM_DEFAULT;
-
-		if (phydev->duplex == DUPLEX_HALF)
-			mode &= ~AX_MEDIUM_FD;
-
-		if (phydev->speed != SPEED_100)
-			mode &= ~AX_MEDIUM_PS;
-	}
-
-	asix_write_medium_mode(dev, mode, 0);
-	phy_print_status(phydev);
-	usbnet_link_change(dev, phydev->link, 0);
-}
-
 int asix_write_gpio(struct usbnet *dev, u16 value, int sleep, int in_pm)
 {
 	int ret;
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 57d6e5abc30e..af91fc947f40 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -40,22 +40,6 @@  struct ax88172_int_data {
 	__le16 res3;
 } __packed;
 
-static void asix_status(struct usbnet *dev, struct urb *urb)
-{
-	struct ax88172_int_data *event;
-	int link;
-
-	if (urb->actual_length < 8)
-		return;
-
-	event = urb->transfer_buffer;
-	link = event->link & 0x01;
-	if (netif_carrier_ok(dev->net) != link) {
-		usbnet_link_change(dev, link, 1);
-		netdev_dbg(dev->net, "Link Status is: %d\n", link);
-	}
-}
-
 static void asix_set_netdev_dev_addr(struct usbnet *dev, u8 *addr)
 {
 	if (is_valid_ether_addr(addr)) {
@@ -752,7 +736,6 @@  static void ax88772_mac_link_down(struct phylink_config *config,
 	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,
@@ -783,7 +766,6 @@  static void ax88772_mac_link_up(struct phylink_config *config,
 		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 = {
@@ -1309,40 +1291,36 @@  static int ax88178_bind(struct usbnet *dev, struct usb_interface *intf)
 static const struct driver_info ax8817x_info = {
 	.description = "ASIX AX8817x USB 2.0 Ethernet",
 	.bind = ax88172_bind,
-	.status = asix_status,
 	.link_reset = ax88172_link_reset,
 	.reset = ax88172_link_reset,
-	.flags =  FLAG_ETHER | FLAG_LINK_INTR,
+	.flags =  FLAG_ETHER,
 	.data = 0x00130103,
 };
 
 static const struct driver_info dlink_dub_e100_info = {
 	.description = "DLink DUB-E100 USB Ethernet",
 	.bind = ax88172_bind,
-	.status = asix_status,
 	.link_reset = ax88172_link_reset,
 	.reset = ax88172_link_reset,
-	.flags =  FLAG_ETHER | FLAG_LINK_INTR,
+	.flags =  FLAG_ETHER,
 	.data = 0x009f9d9f,
 };
 
 static const struct driver_info netgear_fa120_info = {
 	.description = "Netgear FA-120 USB Ethernet",
 	.bind = ax88172_bind,
-	.status = asix_status,
 	.link_reset = ax88172_link_reset,
 	.reset = ax88172_link_reset,
-	.flags =  FLAG_ETHER | FLAG_LINK_INTR,
+	.flags =  FLAG_ETHER,
 	.data = 0x00130103,
 };
 
 static const struct driver_info hawking_uf200_info = {
 	.description = "Hawking UF200 USB Ethernet",
 	.bind = ax88172_bind,
-	.status = asix_status,
 	.link_reset = ax88172_link_reset,
 	.reset = ax88172_link_reset,
-	.flags =  FLAG_ETHER | FLAG_LINK_INTR,
+	.flags =  FLAG_ETHER,
 	.data = 0x001f1d1f,
 };
 
@@ -1350,10 +1328,9 @@  static const struct driver_info ax88772_info = {
 	.description = "ASIX AX88772 USB 2.0 Ethernet",
 	.bind = ax88772_bind,
 	.unbind = ax88772_unbind,
-	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
-	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR | FLAG_MULTI_PACKET,
+	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
 	.rx_fixup = asix_rx_fixup_common,
 	.tx_fixup = asix_tx_fixup,
 };
@@ -1362,11 +1339,9 @@  static const struct driver_info ax88772b_info = {
 	.description = "ASIX AX88772B USB 2.0 Ethernet",
 	.bind = ax88772_bind,
 	.unbind = ax88772_unbind,
-	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
-	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
-	         FLAG_MULTI_PACKET,
+	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
 	.rx_fixup = asix_rx_fixup_common,
 	.tx_fixup = asix_tx_fixup,
 	.data = FLAG_EEPROM_MAC,
@@ -1376,11 +1351,9 @@  static const struct driver_info lxausb_t1l_info = {
 	.description = "Linux Automation GmbH USB 10Base-T1L",
 	.bind = ax88772_bind,
 	.unbind = ax88772_unbind,
-	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
-	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
-		 FLAG_MULTI_PACKET,
+	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
 	.rx_fixup = asix_rx_fixup_common,
 	.tx_fixup = asix_tx_fixup,
 	.data = FLAG_EEPROM_MAC,
@@ -1390,11 +1363,9 @@  static const struct driver_info ax88178_info = {
 	.description = "ASIX AX88178 USB 2.0 Ethernet",
 	.bind = ax88178_bind,
 	.unbind = ax88178_unbind,
-	.status = asix_status,
 	.link_reset = ax88178_link_reset,
 	.reset = ax88178_reset,
-	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
-		 FLAG_MULTI_PACKET,
+	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
 	.rx_fixup = asix_rx_fixup_common,
 	.tx_fixup = asix_tx_fixup,
 };
@@ -1412,10 +1383,8 @@  static const struct driver_info hg20f9_info = {
 	.description = "HG20F9 USB 2.0 Ethernet",
 	.bind = ax88772_bind,
 	.unbind = ax88772_unbind,
-	.status = asix_status,
 	.reset = ax88772_reset,
-	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
-	         FLAG_MULTI_PACKET,
+	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET,
 	.rx_fixup = asix_rx_fixup_common,
 	.tx_fixup = asix_tx_fixup,
 	.data = FLAG_EEPROM_MAC,