Re: [PATCH V2 net-next 2/3] lan78xx: add ethtool set & get pause functions

On Fri, 2016-02-19 at 00:03 +0000, Woojung.Huh@microchip.com wrote:

quoted

quoted

Add ethtool operations of set_pauseram and get_pauseparm.

[...]

quoted

+static void lan78xx_get_pause(struct net_device *net,
+			      struct ethtool_pauseparam *pause)
+{
+	struct lan78xx_net *dev = netdev_priv(net);
+	struct phy_device *phydev = net->phydev;
+	struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET };
+
+	phy_ethtool_gset(phydev, &ecmd);
+
+	pause->autoneg = dev->fc_autoneg;
+
+	if (dev->fc_autoneg) {
+		if (dev->fc_autoneg_control & FLOW_CTRL_TX)
+			pause->tx_pause = 1;
+
+		if (dev->fc_autoneg_control & FLOW_CTRL_RX)
+			pause->rx_pause = 1;

This is incorrect; you should always return the manual settings
(fc_request_control flags) here.  If autonegotiation is enabled then
your get_settings function will return the actual pause flags.

Ben.

Ben, thanks for comments.
How about comment in include/uapi/linux/ethtool.h?
It says 
** struct ethtool_pauseparam - Ethernet pause (flow control) parameters
...
* If @autoneg is non-zero, the MAC is configured to send and/or
* receive pause frames according to the result of autonegotiation.

Doesn't this mean get_pauseparam() returns pause settings based on 
Result of autonegotation? Not manual settings of rx_param & tx_param?

No, get_pauseparam should return the same settings that were passed to
the last set_pauseparam.

Ben.

-- 
Ben Hutchings
Tomorrow will be cancelled due to lack of interest.