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

Add ethtool operations of set_pauseram and get_pauseparm.

Signed-off-by: Woojung Huh <woojung.huh@microchip.com>
---
 drivers/net/usb/lan78xx.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 81 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 4ec25e8..6bcb312 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c

@@ -36,7 +36,7 @@
 #define DRIVER_AUTHOR	"WOOJUNG HUH <woojung.huh@microchip.com>"
 #define DRIVER_DESC	"LAN78XX USB 3.0 Gigabit Ethernet Devices"
 #define DRIVER_NAME	"lan78xx"
-#define DRIVER_VERSION	"1.0.2"
+#define DRIVER_VERSION	"1.0.3"
 
 #define TX_TIMEOUT_JIFFIES		(5 * HZ)
 #define THROTTLE_JIFFIES		(HZ / 8)

@@ -281,6 +281,10 @@ struct lan78xx_net {
 	u32			chipid;
 	u32			chiprev;
 	struct mii_bus		*mdiobus;
+
+	int			fc_autoneg;
+	u8			fc_autoneg_control;
+	u8			fc_request_control;
 };
 
 /* use ethtool to change the level for any given device */

@@ -902,11 +906,17 @@ static int lan78xx_update_flowcontrol(struct lan78xx_net *dev, u8 duplex,
 {
 	u32 flow = 0, fct_flow = 0;
 	int ret;
+	u8 cap;
 
-	u8 cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
+	if (dev->fc_autoneg) {
+		cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
+		dev->fc_autoneg_control = cap;
+	} else {
+		cap = dev->fc_request_control;
+	}
 
 	if (cap & FLOW_CTRL_TX)
-		flow = (FLOW_CR_TX_FCEN_ | 0xFFFF);
+		flow |= (FLOW_CR_TX_FCEN_ | 0xFFFF);
 
 	if (cap & FLOW_CTRL_RX)
 		flow |= FLOW_CR_RX_FCEN_;

@@ -1386,6 +1396,70 @@ static int lan78xx_set_settings(struct net_device *net, struct ethtool_cmd *cmd)
 	return ret;
 }
 
+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;
+	} else {
+		if (dev->fc_request_control & FLOW_CTRL_TX)
+			pause->tx_pause = 1;
+
+		if (dev->fc_request_control & FLOW_CTRL_RX)
+			pause->rx_pause = 1;
+	}
+}
+
+static int lan78xx_set_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 };
+	int ret;
+
+	phy_ethtool_gset(phydev, &ecmd);
+
+	if (pause->autoneg && !ecmd.autoneg) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	dev->fc_request_control = 0;
+	if (pause->rx_pause)
+		dev->fc_request_control |= FLOW_CTRL_RX;
+
+	if (pause->tx_pause)
+		dev->fc_request_control |= FLOW_CTRL_TX;
+
+	if (ecmd.autoneg) {
+		u32 mii_adv;
+
+		ecmd.advertising &= ~(ADVERTISED_Pause | ADVERTISED_Asym_Pause);
+		mii_adv = (u32)mii_advertise_flowctrl(dev->fc_request_control);
+		ecmd.advertising |= mii_adv_to_ethtool_adv_t(mii_adv);
+		phy_ethtool_sset(phydev, &ecmd);
+	}
+
+	dev->fc_autoneg = pause->autoneg;
+
+	ret = 0;
+exit:
+	return ret;
+}
+
 static const struct ethtool_ops lan78xx_ethtool_ops = {
 	.get_link	= lan78xx_get_link,
 	.nway_reset	= lan78xx_nway_reset,

@@ -1404,6 +1478,8 @@ static const struct ethtool_ops lan78xx_ethtool_ops = {
 	.set_wol	= lan78xx_set_wol,
 	.get_eee	= lan78xx_get_eee,
 	.set_eee	= lan78xx_set_eee,
+	.get_pauseparam	= lan78xx_get_pause,
+	.set_pauseparam	= lan78xx_set_pause,
 };
 
 static int lan78xx_ioctl(struct net_device *netdev, struct ifreq *rq, int cmd)

@@ -1631,6 +1707,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 			      SUPPORTED_Pause | SUPPORTED_Asym_Pause);
 	genphy_config_aneg(phydev);
 
+	dev->fc_autoneg = phydev->autoneg;
+
 	phy_start(phydev);
 
 	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");

-- 
2.7.0

On Thu, 2016-02-18 at 22:40 +0000, Woojung.Huh@microchip.com wrote:

Add ethtool operations of set_pauseram and get_pauseparm.

[...]

+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.

+	} else {
+		if (dev->fc_request_control & FLOW_CTRL_TX)
+			pause->tx_pause = 1;
+
+		if (dev->fc_request_control & FLOW_CTRL_RX)
+			pause->rx_pause = 1;
+	}
+}

[...]

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

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?

Woojung

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.

quoted

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.

I used drivers/net/ethernet/intel/e1000e driver as reference.
It's implementation also returns status updated after autonegotiation.
Look into wrong one?

Woojung

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

quoted

quoted

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.

I used drivers/net/ethernet/intel/e1000e driver as reference.
It's implementation also returns status updated after autonegotiation.
Look into wrong one?

Unfortunately the API has not always been clearly defined and there are
lots of bugs (or at least inconsistencies) in drivers.  The comments in
include/uapi/linux/ethtool.h are supposed to be definitive; if they are
not clear then please suggest additional or alternative wording.

Ben.

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

-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Thursday, February 18, 2016 8:21 PM
To: Woojung Huh - C21699; davem@davemloft.net
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH V2 net-next 2/3] lan78xx: add ethtool set & get pause
functions

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

quoted

quoted

quoted

Ben, thanks for comments.
How about comment in include/uapi/linux/ethtool.h?
It says
** struct ethtool_pauseparam - Ethernet pause (flow control)

parameters

quoted

quoted

quoted

...
* 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

quoted

quoted

the last set_pauseparam.

Ben.

I used drivers/net/ethernet/intel/e1000e driver as reference.
It's implementation also returns status updated after autonegotiation.
Look into wrong one?

Unfortunately the API has not always been clearly defined and there are
lots of bugs (or at least inconsistencies) in drivers.  The comments in
include/uapi/linux/ethtool.h are supposed to be definitive; if they are
not clear then please suggest additional or alternative wording.

I will update and repost it.
BTW, do you know which driver is implemented set/get_pauseparam() correctly?

Thanks.

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.

What do you mean "return actual pause flags" when autoneg is enabled?
Pause flags set by (or default) advertise flag?

Thanks.

From: <Woojung.Huh@microchip.com>
Date: Mon, 22 Feb 2016 18:28:18 +0000

quoted

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.

What do you mean "return actual pause flags" when autoneg is enabled?
Pause flags set by (or default) advertise flag?

It means what was negotiated by autonegotiation, and is actually in use.

Otherwise, how can the user find out what was negotiated?

On Mon, 2016-02-22 at 14:10 -0500, David Miller wrote:

From: <Woojung.Huh@microchip.com>
Date: Mon, 22 Feb 2016 18:28:18 +0000

quoted

quoted

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.

What do you mean "return actual pause flags" when autoneg is enabled?
Pause flags set by (or default) advertise flag?

It means what was negotiated by autonegotiation, and is actually in use.

Otherwise, how can the user find out what was negotiated?

Well, actually we don't *directly* report the autonegotiation flow
control mode at present.  We report which pause flags were advertised
and what the link partner advertised, from which you can work it out.

Perhaps ethtool (the utility) should explicitly show what the result
is.

Ben.

-- 
Ben Hutchings
The generation of random numbers is too important to be left to chance.
                                                            - Robert Coveyou

-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Monday, February 22, 2016 3:05 PM
To: David Miller; Woojung Huh - C21699
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH V2 net-next 2/3] lan78xx: add ethtool set & get pause
functions

On Mon, 2016-02-22 at 14:10 -0500, David Miller wrote:

quoted

From: <Woojung.Huh@microchip.com>
Date: Mon, 22 Feb 2016 18:28:18 +0000

quoted

quoted

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.

What do you mean "return actual pause flags" when autoneg is enabled?
Pause flags set by (or default) advertise flag?

It means what was negotiated by autonegotiation, and is actually in use.

Otherwise, how can the user find out what was negotiated?

Well, actually we don't *directly* report the autonegotiation flow
control mode at present.  We report which pause flags were advertised
and what the link partner advertised, from which you can work it out.

NIC advertises pause flags via advertise register (phy reg 0x4) based on Rx & Tx flags,
and get_pauseparam() returns Rx & Tx flags set by set_pauseparm() if autonegotiation is enabled?

Perhaps ethtool (the utility) should explicitly show what the result
is.

Ethtool -a reports Rx/Tx flags and Rx/Tx negotiated flag based on advertising and lp_advertising value.

On Mon, 2016-02-22 at 20:28 +0000, Woojung.Huh@microchip.com wrote:

quoted

-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Monday, February 22, 2016 3:05 PM
To: David Miller; Woojung Huh - C21699
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH V2 net-next 2/3] lan78xx: add ethtool set & get pause
functions

On Mon, 2016-02-22 at 14:10 -0500, David Miller wrote:

quoted

From: <Woojung.Huh@microchip.com>
Date: Mon, 22 Feb 2016 18:28:18 +0000

quoted

quoted

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.

What do you mean "return actual pause flags" when autoneg is enabled?
Pause flags set by (or default) advertise flag?

It means what was negotiated by autonegotiation, and is actually in use.

Otherwise, how can the user find out what was negotiated?

Well, actually we don't *directly* report the autonegotiation flow
control mode at present.  We report which pause flags were advertised
and what the link partner advertised, from which you can work it out.

NIC advertises pause flags via advertise register (phy reg 0x4) based on Rx & Tx flags,
and get_pauseparam() returns Rx & Tx flags set by set_pauseparm() if autonegotiation is enabled?

Yes.

quoted

Perhaps ethtool (the utility) should explicitly show what the result
is.

Ethtool -a reports Rx/Tx flags and Rx/Tx negotiated flag based on advertising and lp_advertising value.

It would probably make sense to include the result of pause frame
autonegotiation in both the 'ethtool -a' and 'ethtool' (no option)
output.

Ben.

-- 
Ben Hutchings
The generation of random numbers is too important to be left to chance.
                                                            - Robert Coveyou