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

From: Oleksij Rempel <[email-masked]>
Date: Mon, 7 Apr 2025 15:38:01 +0200

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 <[email-masked]>
> 
> 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
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |