netdev - Re: [PATCH net-next 1/5] net: wangxun: add flow control support

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: <6218df6e-db11-4640-a296-946088d32916@lunn.ch>
Date: Wed, 22 Nov 2023 17:15:05 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, linux@...linux.org.uk,
	horms@...nel.org, mengyuanlou@...-swift.com
Subject: Re: [PATCH net-next 1/5] net: wangxun: add flow control support

> +	/* Calculate max LAN frame size */
> +	link = dev->mtu + ETH_HLEN + ETH_FCS_LEN + WX_ETH_FRAMING;
> +	tc = link;
> +
> +	/* Calculate delay value for device */
> +	dv_id = WX_DV(link, tc);

That looks odd. tc == link. So why pass both? Or is it a typo?

> +/* BitTimes (BT) conversion */
> +#define WX_BT2KB(BT)         (((BT) + (8 * 1024 - 1)) / (8 * 1024))
> +#define WX_B2BT(BT)          ((BT) * 8)
> +/* Calculate Interface Delay */
> +#define WX_PHY_D     12800
> +#define WX_MAC_D     4096
> +#define WX_XAUI_D    (2 * 1024)
> +#define WX_ID        (WX_MAC_D + WX_XAUI_D + WX_PHY_D)
> +/* Calculate PCI Bus delay for low thresholds */
> +#define WX_PCI_DELAY 10000
> +
> +/* Calculate delay value in bit times */
> +#define WX_DV(_max_frame_link, _max_frame_tc) \
> +	((36 * (WX_B2BT(_max_frame_link) + 672 + (2 * 5556) + (2 * WX_ID) + 6144) / \
> +	  25 + 1) +  2 * WX_B2BT(_max_frame_tc))
> +
> +/* Calculate low threshold delay values */
> +#define WX_LOW_DV(_max_frame_tc) \
> +	(2 * (2 * WX_B2BT(_max_frame_tc) + (36 * WX_PCI_DELAY / 25) + 1))

There is too much magic here. Please make these functions, and add
comments explaining what is going on here.

> +static void ngbe_get_pauseparam(struct net_device *netdev,
> +				struct ethtool_pauseparam *pause)
> +{
> +	struct wx *wx = netdev_priv(netdev);
> +
> +	pause->autoneg = !wx->fc.disable_fc_autoneg;

Maybe call it enable_fs_autoneg, since that is what the kAPI uses?

> +static int ngbe_set_pauseparam(struct net_device *netdev,
> +			       struct ethtool_pauseparam *pause)
> +{
> +	struct wx *wx = netdev_priv(netdev);
> +
> +	if (!wx->phydev)
> +		return -ENODEV;
> +
> +	if (!phy_validate_pause(wx->phydev, pause))
> +		return -EINVAL;
> +
> +	wx->fc.disable_fc_autoneg = !pause->autoneg;
> +	wx->fc.tx_pause = pause->tx_pause;
> +	wx->fc.rx_pause = pause->rx_pause;
> +
> +	phy_set_asym_pause(wx->phydev, pause->rx_pause, pause->tx_pause);

You should only be doing this if pause->autoneg is true. If it is
false, you ignore the results from autoneg, and just configure the
hardware as indicated by pause->{tr}x_pause.

phylink makes this a lot easier, because it hides away all these
details. You might want to convert to phylink. Everybody gets is wrong
with phylib, but correct with phylink.

	 Andrew