| Message ID | 1304559247-16111-1-git-send-email-maheshb@google.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
2011/5/5 Mahesh Bandewar <maheshb@google.com>: > This patch adds tg3_set_features() to handle loopback mode. Currently the > capability is added for the devices which support internal MAC loopback mode. > So when enabled, it enables internal-MAC loopback. > > Signed-off-by: Mahesh Bandewar <maheshb@google.com> > --- > Changes since v3: > (a) Corrected the condition (|| => &&) where loopback capability is added. > (b) set_features() always returns 0. > (c) Clear the loopback bit in ndo_open callback to avoid discrepancy. > > Changes since v2: > Implemtned Joe Perches's style change. > > Changes since v1: > Implemented Matt Carlson's suggestions. > (a) Enable this capability on the devices which are capable of MAC-loopback > mode. > (b) check if the device is running before making changes. > (c) check bits before making changes. > > drivers/net/tg3.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 69 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c > index 7c7c9a8..7f7a290 100644 > --- a/drivers/net/tg3.c > +++ b/drivers/net/tg3.c > @@ -6309,6 +6309,44 @@ dma_error: > return NETDEV_TX_OK; > } > > +static int tg3_set_loopback(struct net_device *dev, u32 features) > +{ > + struct tg3 *tp = netdev_priv(dev); > + u32 cur_mode = 0; > + int retval = 0; void? You always return zero and don't check it in callers. [...] > @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev) > > netif_tx_start_all_queues(dev); > > + /* > + * Reset loopback feature if it was turned on while the device was down > + * to avoid and any discrepancy in features reporting. > + */ > + if (dev->features & NETIF_F_LOOPBACK) { > + dev->features &= ~NETIF_F_LOOPBACK; > + dev->wanted_features &= ~NETIF_F_LOOPBACK; > + } > + if (dev->features & NETIF_F_LOOPBACK) tg3_set_loopback(dev, dev->features); Whatever you do, don't modify wanted_features in drivers. BTW, similar problems (also like in previous versions) are in forcedeth implementation. 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 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote: > 2011/5/5 Mahesh Bandewar <maheshb@google.com>: >> This patch adds tg3_set_features() to handle loopback mode. Currently the >> capability is added for the devices which support internal MAC loopback mode. >> So when enabled, it enables internal-MAC loopback. >> >> Signed-off-by: Mahesh Bandewar <maheshb@google.com> >> --- >> Changes since v3: >> (a) Corrected the condition (|| => &&) where loopback capability is added. >> (b) set_features() always returns 0. >> (c) Clear the loopback bit in ndo_open callback to avoid discrepancy. >> >> Changes since v2: >> Implemtned Joe Perches's style change. >> >> Changes since v1: >> Implemented Matt Carlson's suggestions. >> (a) Enable this capability on the devices which are capable of MAC-loopback >> mode. >> (b) check if the device is running before making changes. >> (c) check bits before making changes. >> >> drivers/net/tg3.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 69 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c >> index 7c7c9a8..7f7a290 100644 >> --- a/drivers/net/tg3.c >> +++ b/drivers/net/tg3.c >> @@ -6309,6 +6309,44 @@ dma_error: >> return NETDEV_TX_OK; >> } >> >> +static int tg3_set_loopback(struct net_device *dev, u32 features) >> +{ >> + struct tg3 *tp = netdev_priv(dev); >> + u32 cur_mode = 0; >> + int retval = 0; > > void? You always return zero and don't check it in callers. > > [...] >> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev) >> >> netif_tx_start_all_queues(dev); >> >> + /* >> + * Reset loopback feature if it was turned on while the device was down >> + * to avoid and any discrepancy in features reporting. >> + */ >> + if (dev->features & NETIF_F_LOOPBACK) { >> + dev->features &= ~NETIF_F_LOOPBACK; >> + dev->wanted_features &= ~NETIF_F_LOOPBACK; >> + } >> + > > if (dev->features & NETIF_F_LOOPBACK) > tg3_set_loopback(dev, dev->features); > Unfortunately at this stage device will not be able to set-loopback so I resorted to clearing the bit(s). > Whatever you do, don't modify wanted_features in drivers. > Since just clearing the 'features' would leave wanted_features in discrepant state, I thought this will bring it to a sane state. So what is a preferred way? > BTW, similar problems (also like in previous versions) are in > forcedeth implementation. Yes, whatever we decide about the state of the wanted_features, I'll implement similarly for forcedeth. Which previous problem you are referring to? Is it the return value? There is a different kind of failure (error while writing the register). Since update_features() cant handle return value, I'm ignoring the return value. As far as the correct return code is concerned, I wasn't sure what is appropriate return code here (may be PHY_ERROR would be appropriate there) but again I could ignore it just the way rest of the code is ignoring. > > 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 > -- 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 5 maja 2011 19:47 użytkownik Mahesh Bandewar <maheshb@google.com> napisał: > On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote: >> 2011/5/5 Mahesh Bandewar <maheshb@google.com>: >>> This patch adds tg3_set_features() to handle loopback mode. Currently the >>> capability is added for the devices which support internal MAC loopback mode. >>> So when enabled, it enables internal-MAC loopback. [...] >>> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev) >>> >>> netif_tx_start_all_queues(dev); >>> >>> + /* >>> + * Reset loopback feature if it was turned on while the device was down >>> + * to avoid and any discrepancy in features reporting. >>> + */ >>> + if (dev->features & NETIF_F_LOOPBACK) { >>> + dev->features &= ~NETIF_F_LOOPBACK; >>> + dev->wanted_features &= ~NETIF_F_LOOPBACK; >>> + } >>> + >> >> if (dev->features & NETIF_F_LOOPBACK) >> tg3_set_loopback(dev, dev->features); >> > Unfortunately at this stage device will not be able to set-loopback so > I resorted to clearing the bit(s). ndo_fix_features+ndo_set_features might be caled before ndo_open - the first might be just from register_netdev() so even before ndo_open callback. >> Whatever you do, don't modify wanted_features in drivers. > Since just clearing the 'features' would leave wanted_features in > discrepant state, I thought this will bring it to a sane state. So > what is a preferred way? If you really can't do other way, driver should keep additional state that is checked in ndo_fix_features callback. >> BTW, similar problems (also like in previous versions) are in >> forcedeth implementation. > Yes, whatever we decide about the state of the wanted_features, I'll > implement similarly for forcedeth. Which previous problem you are > referring to? Is it the return value? There is a different kind of > failure (error while writing the register). Since update_features() > cant handle return value, I'm ignoring the return value. As far as the > correct return code is concerned, I wasn't sure what is appropriate > return code here (may be PHY_ERROR would be appropriate there) but > again I could ignore it just the way rest of the code is ignoring. Let's wait for this threads points to be resolved. You will probably change the forcedeth's implementation then anyway. :-) 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 Thu, May 5, 2011 at 11:35 AM, Michał Mirosław <mirqus@gmail.com> wrote: > W dniu 5 maja 2011 19:47 użytkownik Mahesh Bandewar > <maheshb@google.com> napisał: >> On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote: >>> 2011/5/5 Mahesh Bandewar <maheshb@google.com>: >>>> This patch adds tg3_set_features() to handle loopback mode. Currently the >>>> capability is added for the devices which support internal MAC loopback mode. >>>> So when enabled, it enables internal-MAC loopback. > [...] >>>> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev) >>>> >>>> netif_tx_start_all_queues(dev); >>>> >>>> + /* >>>> + * Reset loopback feature if it was turned on while the device was down >>>> + * to avoid and any discrepancy in features reporting. >>>> + */ >>>> + if (dev->features & NETIF_F_LOOPBACK) { >>>> + dev->features &= ~NETIF_F_LOOPBACK; >>>> + dev->wanted_features &= ~NETIF_F_LOOPBACK; >>>> + } >>>> + >>> >>> if (dev->features & NETIF_F_LOOPBACK) >>> tg3_set_loopback(dev, dev->features); >>> >> Unfortunately at this stage device will not be able to set-loopback so >> I resorted to clearing the bit(s). > > ndo_fix_features+ndo_set_features might be caled before ndo_open - the > first might be just from register_netdev() so even before ndo_open > callback. > ndo_open is the triggering event, so anything before that point does not really help. >>> Whatever you do, don't modify wanted_features in drivers. >> Since just clearing the 'features' would leave wanted_features in >> discrepant state, I thought this will bring it to a sane state. So >> what is a preferred way? > > If you really can't do other way, driver should keep additional state > that is checked in ndo_fix_features callback. > ndo_fix_features callback is called just before calling ndo_set_features callback. Also this is called during update_features, so having the state maintained is not really going to help in this scenario since that is not happening when device reaches ready state (to remove this discrepancy in the feature-set). There must be a reason (that you did not explain!) why driver code should not alter wanted_feature set. So in this scenario, just leaving the wanted_features as it is and clearing the features bit is correct / enough? >>> BTW, similar problems (also like in previous versions) are in >>> forcedeth implementation. >> Yes, whatever we decide about the state of the wanted_features, I'll >> implement similarly for forcedeth. Which previous problem you are >> referring to? Is it the return value? There is a different kind of >> failure (error while writing the register). Since update_features() >> cant handle return value, I'm ignoring the return value. As far as the >> correct return code is concerned, I wasn't sure what is appropriate >> return code here (may be PHY_ERROR would be appropriate there) but >> again I could ignore it just the way rest of the code is ignoring. > > Let's wait for this threads points to be resolved. You will probably > change the forcedeth's implementation then anyway. :-) > agreed. 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 6 maja 2011 01:16 użytkownik Mahesh Bandewar <maheshb@google.com> napisał: > On Thu, May 5, 2011 at 11:35 AM, Michał Mirosław <mirqus@gmail.com> wrote: >> W dniu 5 maja 2011 19:47 użytkownik Mahesh Bandewar >> <maheshb@google.com> napisał: >>> On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote: >>>> 2011/5/5 Mahesh Bandewar <maheshb@google.com>: >>>>> This patch adds tg3_set_features() to handle loopback mode. Currently the >>>>> capability is added for the devices which support internal MAC loopback mode. >>>>> So when enabled, it enables internal-MAC loopback. >> [...] >>>>> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev) >>>>> >>>>> netif_tx_start_all_queues(dev); >>>>> >>>>> + /* >>>>> + * Reset loopback feature if it was turned on while the device was down >>>>> + * to avoid and any discrepancy in features reporting. >>>>> + */ >>>>> + if (dev->features & NETIF_F_LOOPBACK) { >>>>> + dev->features &= ~NETIF_F_LOOPBACK; >>>>> + dev->wanted_features &= ~NETIF_F_LOOPBACK; >>>>> + } >>>>> + >>>> >>>> if (dev->features & NETIF_F_LOOPBACK) >>>> tg3_set_loopback(dev, dev->features); >>>> >>> Unfortunately at this stage device will not be able to set-loopback so >>> I resorted to clearing the bit(s). >> ndo_fix_features+ndo_set_features might be caled before ndo_open - the >> first might be just from register_netdev() so even before ndo_open >> callback. > ndo_open is the triggering event, so anything before that point does > not really help. You're right - I forgot that netif_running() becomes true just before ndo_open is called. I have a hard time to convince myself that you can't program the hardware for loopback just at the end of ndo_open callback. >>>> Whatever you do, don't modify wanted_features in drivers. >>> Since just clearing the 'features' would leave wanted_features in >>> discrepant state, I thought this will bring it to a sane state. So >>> what is a preferred way? >> If you really can't do other way, driver should keep additional state >> that is checked in ndo_fix_features callback. > ndo_fix_features callback is called just before calling > ndo_set_features callback. Also this is called during update_features, > so having the state maintained is not really going to help in this > scenario since that is not happening when device reaches ready state > (to remove this discrepancy in the feature-set). You can call netdev_update_features() at the end of ndo_open. But that would be the same in the end as just enabling the loopback there. > There must be a reason (that you did not explain!) why driver code > should not alter wanted_feature set. So in this scenario, just leaving > the wanted_features as it is and clearing the features bit is correct > / enough? wanted_features reflects a set of features the user wants. I assume that no code can change the user himself, so changing wanted_features instead would break this connection. You can view wanted_features as a cache of user's intention. ;-) 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/tg3.c b/drivers/net/tg3.c index 7c7c9a8..7f7a290 100644 --- a/drivers/net/tg3.c +++ b/drivers/net/tg3.c @@ -6309,6 +6309,44 @@ dma_error: return NETDEV_TX_OK; } +static int tg3_set_loopback(struct net_device *dev, u32 features) +{ + struct tg3 *tp = netdev_priv(dev); + u32 cur_mode = 0; + int retval = 0; + + spin_lock_bh(&tp->lock); + cur_mode = tr32(MAC_MODE); + if (features & NETIF_F_LOOPBACK) { + if (cur_mode & MAC_MODE_PORT_INT_LPBACK) + goto sfeatures_unlock; + + /* + * Clear MAC_MODE_HALF_DUPLEX or you won't get packets back in + * loopback mode if Half-Duplex mode was negotiated earlier. + */ + cur_mode &= ~MAC_MODE_HALF_DUPLEX; + + /* Enable internal MAC loopback mode */ + tw32(MAC_MODE, cur_mode | MAC_MODE_PORT_INT_LPBACK); + netif_carrier_on(tp->dev); + netdev_info(dev, "Internal MAC loopback mode enabled.\n"); + } else { + if (!(cur_mode & MAC_MODE_PORT_INT_LPBACK)) + goto sfeatures_unlock; + + /* Disable internal MAC loopback mode */ + tw32(MAC_MODE, cur_mode & ~MAC_MODE_PORT_INT_LPBACK); + /* Force link status check */ + tg3_setup_phy(tp, 1); + netdev_info(dev, "Internal MAC loopback mode disabled.\n"); + } + +sfeatures_unlock: + spin_unlock_bh(&tp->lock); + return retval; +} + static u32 tg3_fix_features(struct net_device *dev, u32 features) { struct tg3 *tp = netdev_priv(dev); @@ -6319,6 +6357,16 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features) return features; } +static int tg3_set_features(struct net_device *dev, u32 features) +{ + u32 changed = dev->features ^ features; + + if ((changed & NETIF_F_LOOPBACK) && netif_running(dev)) + tg3_set_loopback(dev, features); + + return 0; +} + static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp, int new_mtu) { @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev) netif_tx_start_all_queues(dev); + /* + * Reset loopback feature if it was turned on while the device was down + * to avoid and any discrepancy in features reporting. + */ + if (dev->features & NETIF_F_LOOPBACK) { + dev->features &= ~NETIF_F_LOOPBACK; + dev->wanted_features &= ~NETIF_F_LOOPBACK; + } + return 0; err_out3: @@ -15029,6 +15086,7 @@ static const struct net_device_ops tg3_netdev_ops = { .ndo_tx_timeout = tg3_tx_timeout, .ndo_change_mtu = tg3_change_mtu, .ndo_fix_features = tg3_fix_features, + .ndo_set_features = tg3_set_features, #ifdef CONFIG_NET_POLL_CONTROLLER .ndo_poll_controller = tg3_poll_controller, #endif @@ -15045,6 +15103,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = { .ndo_do_ioctl = tg3_ioctl, .ndo_tx_timeout = tg3_tx_timeout, .ndo_change_mtu = tg3_change_mtu, + .ndo_set_features = tg3_set_features, #ifdef CONFIG_NET_POLL_CONTROLLER .ndo_poll_controller = tg3_poll_controller, #endif @@ -15242,6 +15301,16 @@ static int __devinit tg3_init_one(struct pci_dev *pdev, dev->features |= hw_features; dev->vlan_features |= hw_features; + /* + * Add loopback capability only for a subset of devices that support + * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY + * loopback for the remaining devices. + */ + if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5780 && + !tg3_flag(tp, CPMU_PRESENT)) + /* Add the loopback capability */ + dev->hw_features |= NETIF_F_LOOPBACK; + if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 && !tg3_flag(tp, TSO_CAPABLE) && !(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH)) {
This patch adds tg3_set_features() to handle loopback mode. Currently the capability is added for the devices which support internal MAC loopback mode. So when enabled, it enables internal-MAC loopback. Signed-off-by: Mahesh Bandewar <maheshb@google.com> --- Changes since v3: (a) Corrected the condition (|| => &&) where loopback capability is added. (b) set_features() always returns 0. (c) Clear the loopback bit in ndo_open callback to avoid discrepancy. Changes since v2: Implemtned Joe Perches's style change. Changes since v1: Implemented Matt Carlson's suggestions. (a) Enable this capability on the devices which are capable of MAC-loopback mode. (b) check if the device is running before making changes. (c) check bits before making changes. drivers/net/tg3.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 69 insertions(+), 0 deletions(-)