On Tue, 2016-04-05 at 03:36 -0700, Michael Chan wrote:
> On Tue, Apr 5, 2016 at 3:07 AM, Ben Hutchings <b...@decadent.org.uk> wrote:
[...]
> > > +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. > > > > > > 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
signature.asc
Description: This is a digitally signed message part
