| Message ID | 20170413084555.6962-5-alice.michael@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
You probably need to fixup or remove the commit hashes for the mentioned commits, unless you already did that? Thanks, Jake > -----Original Message----- > From: Michael, Alice > Sent: Thursday, April 13, 2017 1:46 AM > To: Michael, Alice <alice.michael@intel.com>; intel-wired-lan@lists.osuosl.org > Cc: Keller, Jacob E <jacob.e.keller@intel.com> > Subject: [next PATCH S70 05/12] i40e: fix CONFIG_BUSY checks in > i40e_set_settings function > > From: Jacob Keller <jacob.e.keller@intel.com> > > The check for I40E_CONFIG_BUSY state bit in the i40e_set_link_ksettings > function is fishy. First we can notice a few things about the check here. > > First a similar check was introduced by commit > 'c7d05ca89f8e ("i40e: driver ethtool core")' > > Later a commit introducing the link settings was added by commit > 'bf9c71417f72 ("i40e: Implement set_settings for ethtool")' > > However, this second check was against vsi->state instead of pf->state, > and also failed to set the bit, it only checks. That indicates the locking > was not quite correct. The only other place that the state bit > in vsi->state gets used is to protect the filter list. > > Since this code does not care about the mac filter list, and seems > clear the original code should have set the pf->state bit. Fix these > issues by using pf->state correctly, and by actually setting the bit > so that we properly lock as expected. > > Since these checks occur while holding the rtnl_lock(), lets also add a > timeout so that we don't potentially softlock the system. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 38 ++++++++++++++++++++--- > --- > 1 file changed, 29 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > index 10325b5..08035c4 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > @@ -698,6 +698,7 @@ static int i40e_set_link_ksettings(struct net_device > *netdev, > struct ethtool_link_ksettings copy_cmd; > i40e_status status = 0; > bool change = false; > + int timeout = 50; > int err = 0; > u32 autoneg; > u32 advertise; > @@ -756,14 +757,20 @@ static int i40e_set_link_ksettings(struct net_device > *netdev, > if (memcmp(©_cmd, &safe_cmd, sizeof(struct > ethtool_link_ksettings))) > return -EOPNOTSUPP; > > - while (test_bit(__I40E_CONFIG_BUSY, &vsi->state)) > + while (test_and_set_bit(__I40E_CONFIG_BUSY, &pf->state)) { > + timeout--; > + if (!timeout) > + return -EBUSY; > usleep_range(1000, 2000); > + } > > /* Get the current phy config */ > status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities, > NULL); > - if (status) > - return -EAGAIN; > + if (status) { > + err = -EAGAIN; > + goto done; > + } > > /* Copy abilities to config in case autoneg is not > * set below > @@ -779,7 +786,8 @@ static int i40e_set_link_ksettings(struct net_device > *netdev, > if (!ethtool_link_ksettings_test_link_mode( > &safe_cmd, supported, Autoneg)) { > netdev_info(netdev, "Autoneg not supported on > this phy\n"); > - return -EINVAL; > + err = -EINVAL; > + goto done; > } > /* Autoneg is allowed to change */ > config.abilities = abilities.abilities | > @@ -797,7 +805,8 @@ static int i40e_set_link_ksettings(struct net_device > *netdev, > hw->phy.link_info.phy_type != > I40E_PHY_TYPE_10GBASE_T) { > netdev_info(netdev, "Autoneg cannot be > disabled on this phy\n"); > - return -EINVAL; > + err = -EINVAL; > + goto done; > } > /* Autoneg is allowed to change */ > config.abilities = abilities.abilities & > @@ -808,8 +817,10 @@ static int i40e_set_link_ksettings(struct net_device > *netdev, > > ethtool_convert_link_mode_to_legacy_u32(&tmp, > > safe_cmd.link_modes.supported); > - if (advertise & ~tmp) > - return -EINVAL; > + if (advertise & ~tmp) { > + err = -EINVAL; > + goto done; > + } > > if (advertise & ADVERTISED_100baseT_Full) > config.link_speed |= I40E_LINK_SPEED_100MB; > @@ -865,7 +876,8 @@ static int i40e_set_link_ksettings(struct net_device > *netdev, > netdev_info(netdev, "Set phy config failed, err %s aq_err > %s\n", > i40e_stat_str(hw, status), > i40e_aq_str(hw, hw->aq.asq_last_status)); > - return -EAGAIN; > + err = -EAGAIN; > + goto done; > } > > status = i40e_update_link_info(hw); > @@ -878,6 +890,9 @@ static int i40e_set_link_ksettings(struct net_device > *netdev, > netdev_info(netdev, "Nothing changed, exiting without setting > anything.\n"); > } > > +done: > + clear_bit(__I40E_CONFIG_BUSY, &pf->state); > + > return err; > } > > @@ -1292,6 +1307,7 @@ static int i40e_set_ringparam(struct net_device > *netdev, > struct i40e_vsi *vsi = np->vsi; > struct i40e_pf *pf = vsi->back; > u32 new_rx_count, new_tx_count; > + int timeout = 50; > int i, err = 0; > > if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending)) > @@ -1316,8 +1332,12 @@ static int i40e_set_ringparam(struct net_device > *netdev, > (new_rx_count == vsi->rx_rings[0]->count)) > return 0; > > - while (test_and_set_bit(__I40E_CONFIG_BUSY, &pf->state)) > + while (test_and_set_bit(__I40E_CONFIG_BUSY, &pf->state)) { > + timeout--; > + if (!timeout) > + return -EBUSY; > usleep_range(1000, 2000); > + } > > if (!netif_running(vsi->netdev)) { > /* simple case - set for the next time the netdev is started */ > -- > 2.9.3
> -----Original Message----- > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On > Behalf Of Alice Michael > Sent: Thursday, April 13, 2017 1:46 AM > To: Michael, Alice <alice.michael@intel.com>; intel-wired- > lan@lists.osuosl.org > Subject: [Intel-wired-lan] [next PATCH S70 05/12] i40e: fix CONFIG_BUSY > checks in i40e_set_settings function > > From: Jacob Keller <jacob.e.keller@intel.com> > > The check for I40E_CONFIG_BUSY state bit in the i40e_set_link_ksettings > function is fishy. First we can notice a few things about the check here. > > First a similar check was introduced by commit 'c7d05ca89f8e ("i40e: driver > ethtool core")' > > Later a commit introducing the link settings was added by commit > 'bf9c71417f72 ("i40e: Implement set_settings for ethtool")' > > However, this second check was against vsi->state instead of pf->state, and > also failed to set the bit, it only checks. That indicates the locking was not > quite correct. The only other place that the state bit in vsi->state gets used is > to protect the filter list. > > Since this code does not care about the mac filter list, and seems clear the > original code should have set the pf->state bit. Fix these issues by using pf- > >state correctly, and by actually setting the bit so that we properly lock as > expected. > > Since these checks occur while holding the rtnl_lock(), lets also add a timeout > so that we don't potentially softlock the system. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 38 > ++++++++++++++++++++------ > 1 file changed, 29 insertions(+), 9 deletions(-) Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c index 10325b5..08035c4 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c @@ -698,6 +698,7 @@ static int i40e_set_link_ksettings(struct net_device *netdev, struct ethtool_link_ksettings copy_cmd; i40e_status status = 0; bool change = false; + int timeout = 50; int err = 0; u32 autoneg; u32 advertise; @@ -756,14 +757,20 @@ static int i40e_set_link_ksettings(struct net_device *netdev, if (memcmp(©_cmd, &safe_cmd, sizeof(struct ethtool_link_ksettings))) return -EOPNOTSUPP; - while (test_bit(__I40E_CONFIG_BUSY, &vsi->state)) + while (test_and_set_bit(__I40E_CONFIG_BUSY, &pf->state)) { + timeout--; + if (!timeout) + return -EBUSY; usleep_range(1000, 2000); + } /* Get the current phy config */ status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities, NULL); - if (status) - return -EAGAIN; + if (status) { + err = -EAGAIN; + goto done; + } /* Copy abilities to config in case autoneg is not * set below @@ -779,7 +786,8 @@ static int i40e_set_link_ksettings(struct net_device *netdev, if (!ethtool_link_ksettings_test_link_mode( &safe_cmd, supported, Autoneg)) { netdev_info(netdev, "Autoneg not supported on this phy\n"); - return -EINVAL; + err = -EINVAL; + goto done; } /* Autoneg is allowed to change */ config.abilities = abilities.abilities | @@ -797,7 +805,8 @@ static int i40e_set_link_ksettings(struct net_device *netdev, hw->phy.link_info.phy_type != I40E_PHY_TYPE_10GBASE_T) { netdev_info(netdev, "Autoneg cannot be disabled on this phy\n"); - return -EINVAL; + err = -EINVAL; + goto done; } /* Autoneg is allowed to change */ config.abilities = abilities.abilities & @@ -808,8 +817,10 @@ static int i40e_set_link_ksettings(struct net_device *netdev, ethtool_convert_link_mode_to_legacy_u32(&tmp, safe_cmd.link_modes.supported); - if (advertise & ~tmp) - return -EINVAL; + if (advertise & ~tmp) { + err = -EINVAL; + goto done; + } if (advertise & ADVERTISED_100baseT_Full) config.link_speed |= I40E_LINK_SPEED_100MB; @@ -865,7 +876,8 @@ static int i40e_set_link_ksettings(struct net_device *netdev, netdev_info(netdev, "Set phy config failed, err %s aq_err %s\n", i40e_stat_str(hw, status), i40e_aq_str(hw, hw->aq.asq_last_status)); - return -EAGAIN; + err = -EAGAIN; + goto done; } status = i40e_update_link_info(hw); @@ -878,6 +890,9 @@ static int i40e_set_link_ksettings(struct net_device *netdev, netdev_info(netdev, "Nothing changed, exiting without setting anything.\n"); } +done: + clear_bit(__I40E_CONFIG_BUSY, &pf->state); + return err; } @@ -1292,6 +1307,7 @@ static int i40e_set_ringparam(struct net_device *netdev, struct i40e_vsi *vsi = np->vsi; struct i40e_pf *pf = vsi->back; u32 new_rx_count, new_tx_count; + int timeout = 50; int i, err = 0; if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending)) @@ -1316,8 +1332,12 @@ static int i40e_set_ringparam(struct net_device *netdev, (new_rx_count == vsi->rx_rings[0]->count)) return 0; - while (test_and_set_bit(__I40E_CONFIG_BUSY, &pf->state)) + while (test_and_set_bit(__I40E_CONFIG_BUSY, &pf->state)) { + timeout--; + if (!timeout) + return -EBUSY; usleep_range(1000, 2000); + } if (!netif_running(vsi->netdev)) { /* simple case - set for the next time the netdev is started */