Re: [PATCH net-next v7 08/12] net: usb: lan78xx: Convert to PHYLINK for improved PHY and MAC management

[Paolo Abeni <pabeni@redhat.com>]

On 4/28/25 3:05 PM, Oleksij Rempel wrote:
[...]
> @@ -2619,28 +2530,100 @@ static int lan78xx_configure_flowcontrol(struct lan78xx_net *dev,
>  	return lan78xx_write_reg(dev, FLOW, flow);
>  }
>  
> +static void lan78xx_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 net_device *net = to_net_dev(config->dev);
> +	struct lan78xx_net *dev = netdev_priv(net);
> +	u32 mac_cr = 0;
> +	int ret;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		mac_cr |= MAC_CR_SPEED_1000_;
> +		break;
> +	case SPEED_100:
> +		mac_cr |= MAC_CR_SPEED_100_;
> +		break;
> +	case SPEED_10:
> +		mac_cr |= MAC_CR_SPEED_10_;
> +		break;
> +	default:
> +		netdev_err(dev->net, "Unsupported speed %d\n", speed);
> +		return;
> +	}
> +
> +	if (duplex == DUPLEX_FULL)
> +		mac_cr |= MAC_CR_FULL_DUPLEX_;
> +
> +	/* make sure TXEN and RXEN are disabled before reconfiguring MAC */
> +	ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_SPEED_MASK_ |
> +				 MAC_CR_FULL_DUPLEX_ | MAC_CR_EEE_EN_, mac_cr);
> +	if (ret < 0)
> +		goto link_up_fail;
> +
> +	ret = lan78xx_configure_flowcontrol(dev, tx_pause, rx_pause);
> +	if (ret < 0)
> +		goto link_up_fail;
> +
> +	ret = lan78xx_configure_usb(dev, speed);
> +	if (ret < 0)
> +		goto link_up_fail;
> +
> +	lan78xx_rx_urb_submit_all(dev);
> +
> +	ret = lan78xx_flush_rx_fifo(dev);
> +	if (ret < 0)
> +		goto link_up_fail;
> +
> +	ret = lan78xx_flush_tx_fifo(dev);
> +	if (ret < 0)
> +		goto link_up_fail;
> +
> +	ret = lan78xx_start_tx_path(dev);
> +	if (ret < 0)
> +		goto link_up_fail;
> +
> +	ret = lan78xx_start_rx_path(dev);
> +	if (ret < 0)
> +		goto link_up_fail;
> +
> +	return;

Minor nit: a blank line here would make IMHO the code more readable.

> +link_up_fail:
> +	netdev_err(dev->net, "Failed to set MAC up with error %pe\n",
> +		   ERR_PTR(ret));
> +}

[...]
>  static int lan78xx_phy_init(struct lan78xx_net *dev)
>  {
> -	__ETHTOOL_DECLARE_LINK_MODE_MASK(fc) = { 0, };
> -	int ret;
> -	u32 mii_adv;
>  	struct phy_device *phydev;
> +	int ret;
>  
>  	phydev = lan78xx_get_phy(dev);
> +	/* phydev can be NULL if no PHY is found and the chip is LAN7801,
> +	 * which will use a fixed link later.
> +	 * If an  error occurs, return the error code immediately.
> +	 */
>  	if (IS_ERR(phydev))
>  		return PTR_ERR(phydev);
>  
> +	ret = lan78xx_phylink_setup(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* If no PHY is found, set up a fixed link. It is very specific to
> +	 * the LAN7801 and is used in special cases like EVB-KSZ9897-1 where
> +	 * LAN7801 acts as a USB-to-Ethernet interface to a switch without
> +	 * a visible PHY.
> +	 */
> +	if (!phydev) {
> +		ret = lan78xx_set_fixed_link(dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	ret = lan78xx_mac_prepare_for_phy(dev);
>  	if (ret < 0)
> -		goto free_phy;
> +		return ret;

lan78xx_phy_init() now does not perform any cleanup on error forcing the
caller to do the phy cleanup when lan78xx_phy_init() fails, which is
imho cunter-intuitive.

> @@ -3449,31 +3435,9 @@ static int lan78xx_open(struct net_device *net)
>  		}
>  	}
>  
> -	ret = lan78xx_flush_rx_fifo(dev);
> -	if (ret < 0)
> -		goto done;
> -	ret = lan78xx_flush_tx_fifo(dev);
> -	if (ret < 0)
> -		goto done;
> -
> -	ret = lan78xx_start_tx_path(dev);
> -	if (ret < 0)
> -		goto done;
> -	ret = lan78xx_start_rx_path(dev);
> -	if (ret < 0)
> -		goto done;
> -
> -	lan78xx_init_stats(dev);
> -
> -	set_bit(EVENT_DEV_OPEN, &dev->flags);
> +	phylink_start(dev->phylink);
>  
>  	netif_start_queue(net);

I guess this should be called after lan78xx_start_tx_path(), which is
not guarateed anymore after this patch

/P