| Message ID | 1342820631-19738-5-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2012-07-20 at 14:43 -0700, Jeff Kirsher wrote: > From: Jesse Brandeburg <jesse.brandeburg@intel.com> > > This is the implementation in e1000 to allow ethtool to force > MDI state, allowing users to work around some improperly > behaving switches. > > Current get_settings behavior slightly changes in that now when link is down > get_settings will return the MDI state of the last link because get_settings > needs to succeed to allow the set to work even when link is down. > > Forcing in this driver is for now only allowed when auto-neg is enabled. > > To use must have the matching version of ethtool app that supports > this functionality. > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > CC: Tushar Dave <tushar.n.dave@intel.com> > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 34 ++++++++++++++++++++++ > drivers/net/ethernet/intel/e1000/e1000_main.c | 4 +++ > 2 files changed, 38 insertions(+) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > index 3103f0b..1d96bda 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > @@ -174,6 +174,15 @@ static int e1000_get_settings(struct net_device *netdev, > > ecmd->autoneg = ((hw->media_type == e1000_media_type_fiber) || > hw->autoneg) ? AUTONEG_ENABLE : AUTONEG_DISABLE; > + > + /* MDI-X => 1; MDI => 0 */ > + if (hw->media_type == e1000_media_type_copper) > + ecmd->eth_tp_mdix = (!!adapter->phy_info.mdix_mode ? > + ETH_TP_MDI_X : > + ETH_TP_MDI); > + else > + ecmd->eth_tp_mdix = ETH_TP_MDI_INVALID; [...] Why don't you set ecmd->eth_tp_mdix_ctrl here? If you also leave it as 0, it's impossible for userland to tell whether the current mode was forced or automatically selected. Ben.
On Fri, 20 Jul 2012, Ben Hutchings wrote: > Why don't you set ecmd->eth_tp_mdix_ctrl here? > > If you also leave it as 0, it's impossible for userland to tell whether > the current mode was forced or automatically selected. Thanks for the review, right now the get interface (and ethtool display) doesn't support any way to report if the setting was forced or not. I didn't think about changing the get because I didn't want to modify the userland reporting (I also figured it was a simple interface right now, and didn't need changing, and was focused on the _set_ which is the part fixing the users' reported bugs.) I think the patches as they currently stand are okay, do you agree? I would be glad to submit a followon to implement the new "get" interface if we can hash out the interface changes, but I see no reason to gate these patches. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2012-07-20 at 18:17 -0700, Brandeburg, Jesse wrote: > > On Fri, 20 Jul 2012, Ben Hutchings wrote: > > Why don't you set ecmd->eth_tp_mdix_ctrl here? > > > > If you also leave it as 0, it's impossible for userland to tell whether > > the current mode was forced or automatically selected. > > Thanks for the review, right now the get interface (and ethtool display) > doesn't support any way to report if the setting was forced or not. I > didn't think about changing the get because I didn't want to modify the > userland reporting (I also figured it was a simple interface right now, > and didn't need changing, and was focused on the _set_ which is the part > fixing the users' reported bugs.) Everything else you can change with ETHTOOL_SSET is also reported by ETHTOOL_GSET; why would this be any different? > I think the patches as they currently stand are okay, do you agree? I > would be glad to submit a followon to implement the new "get" interface if > we can hash out the interface changes, but I see no reason to gate these > patches. You left this for 20 months, what's the hurry now? Ben.
On Sat, 2012-07-21 at 16:37 +0100, Ben Hutchings wrote: > > On Fri, 2012-07-20 at 18:17 -0700, Brandeburg, Jesse wrote: > > > > On Fri, 20 Jul 2012, Ben Hutchings wrote: > > > Why don't you set ecmd->eth_tp_mdix_ctrl here? > > > > > > If you also leave it as 0, it's impossible for userland to tell > whether > > > the current mode was forced or automatically selected. > > > > Thanks for the review, right now the get interface (and ethtool > display) > > doesn't support any way to report if the setting was forced or not. > I > > didn't think about changing the get because I didn't want to modify > the > > userland reporting (I also figured it was a simple interface right > now, > > and didn't need changing, and was focused on the _set_ which is the > part > > fixing the users' reported bugs.) > > Everything else you can change with ETHTOOL_SSET is also reported by > ETHTOOL_GSET; why would this be any different? > > > I think the patches as they currently stand are okay, do you agree? > I > > would be glad to submit a followon to implement the new "get" > interface if > > we can hash out the interface changes, but I see no reason to gate > these > > patches. > > You left this for 20 months, what's the hurry now? Since there some changes that are needed in this patch set, I will drop this series from my tree so that I can continue pushing additional ixgbe/ixgbevf patches.
diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c index 3103f0b..1d96bda 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c @@ -174,6 +174,15 @@ static int e1000_get_settings(struct net_device *netdev, ecmd->autoneg = ((hw->media_type == e1000_media_type_fiber) || hw->autoneg) ? AUTONEG_ENABLE : AUTONEG_DISABLE; + + /* MDI-X => 1; MDI => 0 */ + if (hw->media_type == e1000_media_type_copper) + ecmd->eth_tp_mdix = (!!adapter->phy_info.mdix_mode ? + ETH_TP_MDI_X : + ETH_TP_MDI); + else + ecmd->eth_tp_mdix = ETH_TP_MDI_INVALID; + return 0; } @@ -183,6 +192,22 @@ static int e1000_set_settings(struct net_device *netdev, struct e1000_adapter *adapter = netdev_priv(netdev); struct e1000_hw *hw = &adapter->hw; + /* + * MDI setting is only allowed when autoneg enabled because + * some hardware doesn't allow MDI setting when speed or + * duplex is forced. + */ + if (ecmd->eth_tp_mdix_ctrl) { + if (hw->media_type != e1000_media_type_copper) + return -EOPNOTSUPP; + + if ((ecmd->eth_tp_mdix_ctrl != ETH_TP_MDI_AUTO) && + (ecmd->autoneg != AUTONEG_ENABLE)) { + e_err(drv, "forcing MDI/MDI-X state is not supported when link speed and/or duplex are forced\n"); + return -EINVAL; + } + } + while (test_and_set_bit(__E1000_RESETTING, &adapter->flags)) msleep(1); @@ -199,12 +224,21 @@ static int e1000_set_settings(struct net_device *netdev, ecmd->advertising = hw->autoneg_advertised; } else { u32 speed = ethtool_cmd_speed(ecmd); + /* calling this overrides forced MDI setting */ if (e1000_set_spd_dplx(adapter, speed, ecmd->duplex)) { clear_bit(__E1000_RESETTING, &adapter->flags); return -EINVAL; } } + /* MDI-X => 2; MDI => 1; Auto => 3 */ + if (ecmd->eth_tp_mdix_ctrl) { + if (ecmd->eth_tp_mdix_ctrl == ETH_TP_MDI_AUTO) + hw->mdix = AUTO_ALL_MODES; + else + hw->mdix = ecmd->eth_tp_mdix_ctrl; + } + /* reset the link */ if (netif_running(adapter->netdev)) { diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c index 3bfbb8d..0ae2fcf 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_main.c +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c @@ -4939,6 +4939,10 @@ int e1000_set_spd_dplx(struct e1000_adapter *adapter, u32 spd, u8 dplx) default: goto err_inval; } + + /* clear MDI, MDI(-X) override is only allowed when autoneg enabled */ + hw->mdix = AUTO_ALL_MODES; + return 0; err_inval: