netdev - Re: [PATCH net-next V6 4/5] net: lan743x: Migrate phylib to phylink

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] [day] [month] [year] [list]

Message-ID: <ZumnRtZYBLIyI/Gm@shell.armlinux.org.uk>
Date: Tue, 17 Sep 2024 16:59:02 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
	andrew@...n.ch, hkallweit1@...il.com, richardcochran@...il.com,
	rdunlap@...radead.org, bryan.whitehead@...rochip.com,
	edumazet@...gle.com, pabeni@...hat.com,
	maxime.chevallier@...tlin.com, linux-kernel@...r.kernel.org,
	horms@...nel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next V6 4/5] net: lan743x: Migrate phylib to phylink

Hi,

A couple of niggles. I know that the patches have already been merged
while I wasn't able to review, but maybe you can address these points.

On Fri, Sep 06, 2024 at 04:05:10PM +0530, Raju Lakkaraju wrote:
> +static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> +					struct phy_device *phydev,
> +					unsigned int link_an_mode,
> +					phy_interface_t interface,
> +					int speed, int duplex,
> +					bool tx_pause, bool rx_pause)
> +{
> +	struct net_device *netdev = to_net_dev(config->dev);
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +	int mac_cr;
> +	u8 cap;
> +
> +	mac_cr = lan743x_csr_read(adapter, MAC_CR);
> +	/* Pre-initialize register bits.
> +	 * Resulting value corresponds to SPEED_10
> +	 */
> +	mac_cr &= ~(MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
> +	if (speed == SPEED_2500)
> +		mac_cr |= MAC_CR_CFG_H_ | MAC_CR_CFG_L_;
> +	else if (speed == SPEED_1000)
> +		mac_cr |= MAC_CR_CFG_H_;
> +	else if (speed == SPEED_100)
> +		mac_cr |= MAC_CR_CFG_L_;
> +
> +	lan743x_csr_write(adapter, MAC_CR, mac_cr);
> +
> +	lan743x_ptp_update_latency(adapter, speed);
> +
> +	/* Flow Control operation */
> +	cap = 0;
> +	if (tx_pause)
> +		cap |= FLOW_CTRL_TX;
> +	if (rx_pause)
> +		cap |= FLOW_CTRL_RX;
> +
> +	lan743x_mac_flow_ctrl_set_enables(adapter,
> +					  cap & FLOW_CTRL_TX,
> +					  cap & FLOW_CTRL_RX);

I'm wondeing about this, which looks to me to be over-complex code.

void lan743x_mac_flow_ctrl_set_enables(struct lan743x_adapter *adapter,
                                       bool tx_enable, bool rx_enable)

This function takes two booleans. You're passing cap & FLOW_CTRL_TX
and cap & FLOW_CTRL_RX to this function for these. However, you are
setting these bits in "cap" immediately above from a pair of booleans
and nowhere else. So why not:

	lan743x_mac_flow_ctrl_set_enables(adapter, tx_pause, rx_pause);

?

> @@ -3115,13 +3217,13 @@ static int lan743x_netdev_open(struct net_device *netdev)
>  	if (ret)
>  		goto close_intr;
>  
> -	ret = lan743x_phy_open(adapter);
> +	ret = lan743x_phylink_connect(adapter);
>  	if (ret)
>  		goto close_mac;
>  
>  	ret = lan743x_ptp_open(adapter);
>  	if (ret)
> -		goto close_phy;
> +		goto close_mac;
>  
>  	lan743x_rfe_open(adapter);
>  
> @@ -3161,9 +3263,8 @@ static int lan743x_netdev_open(struct net_device *netdev)
>  			lan743x_rx_close(&adapter->rx[index]);
>  	}
>  	lan743x_ptp_close(adapter);
> -
> -close_phy:
> -	lan743x_phy_close(adapter);
> +	if (adapter->phylink)
> +		lan743x_phylink_disconnect(adapter);

I'm not sure why this is conditional on adapter->phylink. Surely this
test will always be true?

Thanks.

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