On Tue, 2016-04-05 at 03:36 -0700, Michael Chan wrote:

On Tue, Apr 5, 2016 at 3:07 AM, Ben Hutchings [off-list ref] wrote:

[...]

quoted

quoted

+static int bnxt_get_eee(struct net_device *dev, struct ethtool_eee *edata)
+{
+     struct bnxt *bp = netdev_priv(dev);
+
+     if (!(bp->flags & BNXT_FLAG_EEE_CAP))
+             return -EOPNOTSUPP;
+
+     *edata = bp->eee;
+     if (!bp->eee.eee_enabled) {
+             edata->advertised = 0;
+             edata->tx_lpi_enabled = 0;

What about tx_lpi_timer?

We want to keep the tx_lpi_timer value so that it can be used again
when it is turned on again.

The user doesn't have to figure out what value to use if he just wants
to use the default or the last value.

OK, that seems like a good reason.

quoted

And, wouldn't it make more sense to do these fixups to the internal
state in bnxt_set_eee()?

I don't understand.  If the user is enabling EEE, we take all the
parameters.  If he is disabling, we don't take any of the parameters.

Right - it's just a bit weird that you keep the internal state in a
struct ethtool_eee but get_eee() returns a slightly different version
of the state.

Ben.

-- 
Ben Hutchings
No political challenge can be met by shopping. - George Monbiot