| Message ID | 1305140625-25888-1-git-send-email-maheshb@google.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
2011/5/11 Mahesh Bandewar <maheshb@google.com>: > This patch adds e1000_set_features() to handle loopback mode. When loopback > is enabled, it enables internal-MAC loopback. Please wait for this driver's conversion to hw_features. One comment below, though. [...] > --- a/drivers/net/e1000e/netdev.c > +++ b/drivers/net/e1000e/netdev.c [...] > +static int e1000_set_features(struct net_device *dev, u32 features) > +{ > + u32 changed = dev->features ^ features; > + > + if ((changed & NETIF_F_LOOPBACK) && netif_running(dev)) > + e1000_set_loopback(dev, features); > + > + return 0; > +} [...] If e1000_set_loopback() fails, this should set dev->features to passed features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to keep the state consistent. Best Regards, Michał Mirosław -- 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 Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote: > 2011/5/11 Mahesh Bandewar <maheshb@google.com>: >> This patch adds e1000_set_features() to handle loopback mode. When loopback >> is enabled, it enables internal-MAC loopback. > > Please wait for this driver's conversion to hw_features. One comment > below, though. > This is not intrusive so should not create problems when that happens. > [...] >> --- a/drivers/net/e1000e/netdev.c >> +++ b/drivers/net/e1000e/netdev.c > [...] >> +static int e1000_set_features(struct net_device *dev, u32 features) >> +{ >> + u32 changed = dev->features ^ features; >> + >> + if ((changed & NETIF_F_LOOPBACK) && netif_running(dev)) >> + e1000_set_loopback(dev, features); >> + >> + return 0; >> +} > [...] > > If e1000_set_loopback() fails, this should set dev->features to passed > features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to > keep the state consistent. > set_features() can return the return code of set_loopback() instead of 0; this way the consistency will be maintained. Thanks, --mahesh.. > Best Regards, > Michał Mirosław > -- 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
From: Mahesh Bandewar <maheshb@google.com> Date: Wed, 11 May 2011 16:11:21 -0700 > On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote: >> 2011/5/11 Mahesh Bandewar <maheshb@google.com>: >>> This patch adds e1000_set_features() to handle loopback mode. When loopback >>> is enabled, it enables internal-MAC loopback. >> >> Please wait for this driver's conversion to hw_features. One comment >> below, though. >> > This is not intrusive so should not create problems when that happens. Right and the Intel maintainer folks can resolve such conflicts anyways. -- 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
W dniu 12 maja 2011 01:11 użytkownik Mahesh Bandewar <maheshb@google.com> napisał: > On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote: >> 2011/5/11 Mahesh Bandewar <maheshb@google.com>: >>> This patch adds e1000_set_features() to handle loopback mode. When loopback >>> is enabled, it enables internal-MAC loopback. >> Please wait for this driver's conversion to hw_features. One comment >> below, though. > This is not intrusive so should not create problems when that happens. Fine by me then. >> [...] >>> --- a/drivers/net/e1000e/netdev.c >>> +++ b/drivers/net/e1000e/netdev.c >> [...] >>> +static int e1000_set_features(struct net_device *dev, u32 features) >>> +{ >>> + u32 changed = dev->features ^ features; >>> + >>> + if ((changed & NETIF_F_LOOPBACK) && netif_running(dev)) >>> + e1000_set_loopback(dev, features); >>> + >>> + return 0; >>> +} >> [...] >> >> If e1000_set_loopback() fails, this should set dev->features to passed >> features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to >> keep the state consistent. > set_features() can return the return code of set_loopback() instead of > 0; this way the consistency will be maintained. Only as long as NETIF_F_LOOPBACK is the only bit set in hw_features. netdev_update_features() can't really know which features were changed and which failed when ndo_set_features callback returns non-zero. Best Regards, Michał Mirosław -- 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 Wed, May 11, 2011 at 10:50 PM, Michał Mirosław <mirqus@gmail.com> wrote: > W dniu 12 maja 2011 01:11 użytkownik Mahesh Bandewar > <maheshb@google.com> napisał: >> On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote: >>> 2011/5/11 Mahesh Bandewar <maheshb@google.com>: >>>> This patch adds e1000_set_features() to handle loopback mode. When loopback >>>> is enabled, it enables internal-MAC loopback. >>> Please wait for this driver's conversion to hw_features. One comment >>> below, though. >> This is not intrusive so should not create problems when that happens. > > Fine by me then. > >>> [...] >>>> --- a/drivers/net/e1000e/netdev.c >>>> +++ b/drivers/net/e1000e/netdev.c >>> [...] >>>> +static int e1000_set_features(struct net_device *dev, u32 features) >>>> +{ >>>> + u32 changed = dev->features ^ features; >>>> + >>>> + if ((changed & NETIF_F_LOOPBACK) && netif_running(dev)) >>>> + e1000_set_loopback(dev, features); >>>> + >>>> + return 0; >>>> +} >>> [...] >>> >>> If e1000_set_loopback() fails, this should set dev->features to passed >>> features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to >>> keep the state consistent. >> set_features() can return the return code of set_loopback() instead of >> 0; this way the consistency will be maintained. > > Only as long as NETIF_F_LOOPBACK is the only bit set in hw_features. > netdev_update_features() can't really know which features were changed > and which failed when ndo_set_features callback returns non-zero. > This is more of an API shortcoming. Callback will have to revert changes made (rollback) before returning non-zero value to keep it consistent. Thanks, --mahesh.. > Best Regards, > Michał Mirosław > -- 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
W dniu 12 maja 2011 23:41 użytkownik Mahesh Bandewar <maheshb@google.com> napisał: > On Wed, May 11, 2011 at 10:50 PM, Michał Mirosław <mirqus@gmail.com> wrote: >> W dniu 12 maja 2011 01:11 użytkownik Mahesh Bandewar >> <maheshb@google.com> napisał: >>> On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote: >>>> If e1000_set_loopback() fails, this should set dev->features to passed >>>> features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to >>>> keep the state consistent. >>> set_features() can return the return code of set_loopback() instead of >>> 0; this way the consistency will be maintained. >> Only as long as NETIF_F_LOOPBACK is the only bit set in hw_features. >> netdev_update_features() can't really know which features were changed >> and which failed when ndo_set_features callback returns non-zero. > This is more of an API shortcoming. Callback will have to revert > changes made (rollback) before returning non-zero value to keep it > consistent. It might just update dev->features to match instead of rollback. It could also start some recovery process that eventually calls netdev_update_features() again to try the change again. IOW, the information what changes failed are returned implicitly in modified dev->features. When callback returns 0, netdev_update_features() assumes that all were set correctly and updates dev->features itself. Best Regards, Michał Mirosław -- 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
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h index 9549879..9db3037 100644 --- a/drivers/net/e1000e/e1000.h +++ b/drivers/net/e1000e/e1000.h @@ -640,6 +640,7 @@ extern s32 e1000_check_polarity_ife(struct e1000_hw *hw); extern s32 e1000_phy_force_speed_duplex_ife(struct e1000_hw *hw); extern s32 e1000_check_polarity_igp(struct e1000_hw *hw); extern bool e1000_check_phy_82574(struct e1000_hw *hw); +extern int e1000_set_loopback(struct net_device *dev, u32 features); static inline s32 e1000_phy_hw_reset(struct e1000_hw *hw) { diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c index 859d0d3..9c26b42 100644 --- a/drivers/net/e1000e/ethtool.c +++ b/drivers/net/e1000e/ethtool.c @@ -2027,6 +2027,40 @@ static int e1000e_set_flags(struct net_device *netdev, u32 data) return 0; } +int e1000_set_loopback(struct net_device *netdev, u32 features) +{ + struct e1000_adapter *adapter = netdev_priv(netdev); + struct e1000_hw *hw = &adapter->hw; + uint16_t reg = 0; + int retval = 0; + + if ((retval = e1e_rphy(hw, PHY_CONTROL, ®)) > 0) + return retval; + + if (features & NETIF_F_LOOPBACK) { + if (reg & MII_CR_LOOPBACK) + return 0; + + retval = e1000_setup_loopback_test(adapter); + if (retval) + return retval; + + netdev_info(netdev, + "e1000e: Internal PHY loopback mode enabled.\n"); + } else { + if (!(reg & MII_CR_LOOPBACK)) + return 0; + + e1000_loopback_cleanup(adapter); + e1000e_down(adapter); + e1000e_up(adapter); + netdev_info(netdev, + "e1000e: Internal PHY loopback mode disabled.\n"); + } + + return retval; +} + static const struct ethtool_ops e1000_ethtool_ops = { .get_settings = e1000_get_settings, .set_settings = e1000_set_settings, diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c index 0939040..6aee9b19 100644 --- a/drivers/net/e1000e/netdev.c +++ b/drivers/net/e1000e/netdev.c @@ -3688,6 +3688,13 @@ static int e1000_open(struct net_device *netdev) else ew32(ICS, E1000_ICS_LSC); + /* + * If loopback was enabled while the device was down, + * make sure it gets installed properly now. + */ + if (netdev->features & NETIF_F_LOOPBACK) + e1000_set_loopback(netdev, netdev->features); + return 0; err_req_irq: @@ -5623,6 +5630,16 @@ static void e1000_netpoll(struct net_device *netdev) } #endif +static int e1000_set_features(struct net_device *dev, u32 features) +{ + u32 changed = dev->features ^ features; + + if ((changed & NETIF_F_LOOPBACK) && netif_running(dev)) + e1000_set_loopback(dev, features); + + return 0; +} + /** * e1000_io_error_detected - called when PCI error is detected * @pdev: Pointer to PCI device @@ -5789,6 +5806,7 @@ static const struct net_device_ops e1000e_netdev_ops = { #ifdef CONFIG_NET_POLL_CONTROLLER .ndo_poll_controller = e1000_netpoll, #endif + .ndo_set_features = e1000_set_features, }; /** @@ -6093,6 +6111,9 @@ static int __devinit e1000_probe(struct pci_dev *pdev, e1000_print_device_info(adapter); + /* Add loopback capability to the device. */ + netdev->hw_features |= NETIF_F_LOOPBACK; + if (pci_dev_run_wake(pdev)) pm_runtime_put_noidle(&pdev->dev); diff --git a/drivers/net/e1000e/phy.c b/drivers/net/e1000e/phy.c index 484774c..9bc5df3 100644 --- a/drivers/net/e1000e/phy.c +++ b/drivers/net/e1000e/phy.c @@ -1758,7 +1758,7 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, u32 usec_interval, bool *success) { s32 ret_val = 0; - u16 i, phy_status; + u16 i, reg; for (i = 0; i < iterations; i++) { /* @@ -1766,7 +1766,7 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, * twice due to the link bit being sticky. No harm doing * it across the board. */ - ret_val = e1e_rphy(hw, PHY_STATUS, &phy_status); + ret_val = e1e_rphy(hw, PHY_STATUS, ®); if (ret_val) /* * If the first read fails, another entity may have @@ -1774,10 +1774,10 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, * see if they have relinquished the resources yet. */ udelay(usec_interval); - ret_val = e1e_rphy(hw, PHY_STATUS, &phy_status); + ret_val = e1e_rphy(hw, PHY_STATUS, ®); if (ret_val) break; - if (phy_status & MII_SR_LINK_STATUS) + if (reg & MII_SR_LINK_STATUS) break; if (usec_interval >= 1000) mdelay(usec_interval/1000); @@ -1787,6 +1787,17 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, *success = (i < iterations); + /* + * If the device is in loopback mode, we have to fake + * link up. Try this only if link isn't present. + */ + if (!(*success) && !e1e_rphy(hw, PHY_CONTROL, ®) && + (reg & MII_CR_LOOPBACK)) { + /* Fake link up. */ + *success = true; + ret_val = 0; + } + return ret_val; }
This patch adds e1000_set_features() to handle loopback mode. When loopback is enabled, it enables internal-MAC loopback. Signed-off-by: Mahesh Bandewar <maheshb@google.com> --- drivers/net/e1000e/e1000.h | 1 + drivers/net/e1000e/ethtool.c | 34 ++++++++++++++++++++++++++++++++++ drivers/net/e1000e/netdev.c | 21 +++++++++++++++++++++ drivers/net/e1000e/phy.c | 19 +++++++++++++++---- 4 files changed, 71 insertions(+), 4 deletions(-)