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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5c42675c-5143-43dc-94cd-701711ff5fa6@redhat.com>
Date: Fri, 2 May 2025 11:36:08 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Woojung Huh <woojung.huh@...rochip.com>,
 Andrew Lunn <andrew+netdev@...n.ch>,
 Russell King <rmk+kernel@...linux.org.uk>,
 Thangaraj Samynathan <Thangaraj.S@...rochip.com>,
 Rengarajan Sundararajan <Rengarajan.S@...rochip.com>
Cc: kernel@...gutronix.de, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
 Phil Elwell <phil@...pberrypi.org>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>,
 Simon Horman <horms@...nel.org>
Subject: Re: [PATCH net-next v7 08/12] net: usb: lan78xx: Convert to PHYLINK
 for improved PHY and MAC management

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