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

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

[Woojung.Huh]

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

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

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

[Woojung.Huh]

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

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

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

On Fri, 2016-02-19 at 00:03 +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.
> 
> 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

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

[Woojung.Huh]

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

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

On Fri, 2016-02-19 at 00:16 +0000, Woojung.Huh@microchip.com wrote:
> > > 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

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

[Woojung.Huh]

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

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

Thanks.

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

[Woojung.Huh]

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

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

[David Miller]

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

>> 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?

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

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

[Woojung.Huh]

> -----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:
> > From: <Woojung.Huh@microchip.com>
> > Date: Mon, 22 Feb 2016 18:28:18 +0000
> >
> > >> 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.

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]

On Mon, 2016-02-22 at 20:28 +0000, Woojung.Huh@microchip.com wrote:
> > -----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:
> > > From: <Woojung.Huh@microchip.com>
> > > Date: Mon, 22 Feb 2016 18:28:18 +0000
> > > 
> > > > > 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.

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]