Message ID | 1459844562-24576-6-git-send-email-michael.chan@broadcom.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 2016-04-05 at 04:22 -0400, Michael Chan wrote: > Allow users to get|set EEE parameters. > > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > --- > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 73 +++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > index 14f0520..3d5c64f 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > @@ -1379,6 +1379,77 @@ static int bnxt_set_eeprom(struct net_device *dev, > eeprom->len); > } > > +static int bnxt_set_eee(struct net_device *dev, struct ethtool_eee *edata) > +{ > + struct bnxt *bp = netdev_priv(dev); > + struct ethtool_eee *eee = &bp->eee; > + struct bnxt_link_info *link_info = &bp->link_info; > + u32 advertising = > + _bnxt_fw_to_ethtool_adv_spds(link_info->advertising, 0); > + int rc = 0; > + > + if (BNXT_VF(bp)) > + return 0; > + > + if (!(bp->flags & BNXT_FLAG_EEE_CAP)) > + return -EOPNOTSUPP; > + > + if (!edata->eee_enabled) > + goto eee_ok; [...] > + eee->advertised = edata->advertised; > + eee->tx_lpi_enabled = edata->tx_lpi_enabled; > + eee->tx_lpi_timer = edata->tx_lpi_timer; > +eee_ok: > + eee->eee_enabled = edata->eee_enabled; > + > + if (netif_running(dev)) > + rc = bnxt_hwrm_set_link_setting(bp, false, true); > + > + return rc; > +} > + > +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? And, wouldn't it make more sense to do these fixups to the internal state in bnxt_set_eee()? Ben. > + } > + > + if (!bp->eee.eee_active) > + edata->lp_advertised = 0; > + > + return 0; > +} [...]
On Tue, Apr 5, 2016 at 3:07 AM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Tue, 2016-04-05 at 04:22 -0400, Michael Chan wrote: > >> Allow users to get|set EEE parameters. >> >> Signed-off-by: Michael Chan <michael.chan@broadcom.com> >> --- >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 73 +++++++++++++++++++++++ >> 1 file changed, 73 insertions(+) >> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c >> index 14f0520..3d5c64f 100644 >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c >> @@ -1379,6 +1379,77 @@ static int bnxt_set_eeprom(struct net_device *dev, >> eeprom->len); >> } >> >> +static int bnxt_set_eee(struct net_device *dev, struct ethtool_eee *edata) >> +{ >> + struct bnxt *bp = netdev_priv(dev); >> + struct ethtool_eee *eee = &bp->eee; >> + struct bnxt_link_info *link_info = &bp->link_info; >> + u32 advertising = >> + _bnxt_fw_to_ethtool_adv_spds(link_info->advertising, 0); >> + int rc = 0; >> + >> + if (BNXT_VF(bp)) >> + return 0; >> + >> + if (!(bp->flags & BNXT_FLAG_EEE_CAP)) >> + return -EOPNOTSUPP; >> + >> + if (!edata->eee_enabled) >> + goto eee_ok; > [...] >> + eee->advertised = edata->advertised; >> + eee->tx_lpi_enabled = edata->tx_lpi_enabled; >> + eee->tx_lpi_timer = edata->tx_lpi_timer; >> +eee_ok: >> + eee->eee_enabled = edata->eee_enabled; >> + >> + if (netif_running(dev)) >> + rc = bnxt_hwrm_set_link_setting(bp, false, true); >> + >> + return rc; >> +} >> + >> +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. > > 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. > > Ben. > >> + } >> + >> + if (!bp->eee.eee_active) >> + edata->lp_advertised = 0; >> + >> + return 0; >> +} > [...] > > -- > Ben Hutchings > No political challenge can be met by shopping. - George Monbiot
On Tue, 2016-04-05 at 03:36 -0700, Michael Chan wrote: > On Tue, Apr 5, 2016 at 3:07 AM, Ben Hutchings <ben@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.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 14f0520..3d5c64f 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -1379,6 +1379,77 @@ static int bnxt_set_eeprom(struct net_device *dev, eeprom->len); } +static int bnxt_set_eee(struct net_device *dev, struct ethtool_eee *edata) +{ + struct bnxt *bp = netdev_priv(dev); + struct ethtool_eee *eee = &bp->eee; + struct bnxt_link_info *link_info = &bp->link_info; + u32 advertising = + _bnxt_fw_to_ethtool_adv_spds(link_info->advertising, 0); + int rc = 0; + + if (BNXT_VF(bp)) + return 0; + + if (!(bp->flags & BNXT_FLAG_EEE_CAP)) + return -EOPNOTSUPP; + + if (!edata->eee_enabled) + goto eee_ok; + + if (!(link_info->autoneg & BNXT_AUTONEG_SPEED)) { + netdev_warn(dev, "EEE requires autoneg\n"); + return -EINVAL; + } + if (edata->tx_lpi_enabled) { + if (bp->lpi_tmr_hi && (edata->tx_lpi_timer > bp->lpi_tmr_hi || + edata->tx_lpi_timer < bp->lpi_tmr_lo)) { + netdev_warn(dev, "Valid LPI timer range is %d and %d microsecs\n", + bp->lpi_tmr_lo, bp->lpi_tmr_hi); + return -EINVAL; + } else if (!bp->lpi_tmr_hi) { + edata->tx_lpi_timer = eee->tx_lpi_timer; + } + } + if (!edata->advertised) { + edata->advertised = advertising & eee->supported; + } else if (edata->advertised & ~advertising) { + netdev_warn(dev, "EEE advertised %x must be a subset of autoneg advertised speeds %x\n", + edata->advertised, advertising); + return -EINVAL; + } + + eee->advertised = edata->advertised; + eee->tx_lpi_enabled = edata->tx_lpi_enabled; + eee->tx_lpi_timer = edata->tx_lpi_timer; +eee_ok: + eee->eee_enabled = edata->eee_enabled; + + if (netif_running(dev)) + rc = bnxt_hwrm_set_link_setting(bp, false, true); + + return rc; +} + +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; + } + + if (!bp->eee.eee_active) + edata->lp_advertised = 0; + + return 0; +} + const struct ethtool_ops bnxt_ethtool_ops = { .get_settings = bnxt_get_settings, .set_settings = bnxt_set_settings, @@ -1407,4 +1478,6 @@ const struct ethtool_ops bnxt_ethtool_ops = { .get_eeprom = bnxt_get_eeprom, .set_eeprom = bnxt_set_eeprom, .get_link = bnxt_get_link, + .get_eee = bnxt_get_eee, + .set_eee = bnxt_set_eee, };
Allow users to get|set EEE parameters. Signed-off-by: Michael Chan <michael.chan@broadcom.com> --- drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 73 +++++++++++++++++++++++ 1 file changed, 73 insertions(+)