| Message ID | 20110412193823.0823213A65@rere.qmqm.pl |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: Tue, 12 Apr 2011 21:38:23 +0200 (CEST) > Since ndo_fix_features callback is postponing features change when > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features() > has to be called again when this condition changes. Previously, > ethtool_ops->set_flags callback returned -EBUSY in that case > (it's not possible in the new model). > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > v5: - don't delay set_features, as it's rtnl_locked - same as recovery process > v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion > - add check for failed ndo_set_features in ndo_open callback > v3: - include NETIF_F_LRO in hw_features > - don't call netdev_update_features() if bnx2x_nic_load() failed > v2: - comment in ndo_fix_features callback I've applied this. From what I can tell the is basic agreement from the Broadcom folks, and if any fixups are needed that can be done with follow-up patches. Thanks. -- 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
Le mardi 12 avril 2011 à 21:38 +0200, Michał Mirosław a écrit : > Since ndo_fix_features callback is postponing features change when > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features() > has to be called again when this condition changes. Previously, > ethtool_ops->set_flags callback returned -EBUSY in that case > (it's not possible in the new model). > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > v5: - don't delay set_features, as it's rtnl_locked - same as recovery process > v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion > - add check for failed ndo_set_features in ndo_open callback > v3: - include NETIF_F_LRO in hw_features > - don't call netdev_update_features() if bnx2x_nic_load() failed > v2: - comment in ndo_fix_features callback > --- Hi guys I am not sure its related to these changes, but I now have in net-next-2.6 : [ 23.674263] ------------[ cut here ]------------ [ 23.674266] WARNING: at net/core/dev.c:1318 dev_disable_lro+0x83/0x90() [ 23.674270] Hardware name: ProLiant BL460c G6 [ 23.674273] Modules linked in: tg3 libphy sg [ 23.674280] Pid: 3070, comm: sysctl Tainted: G W 2.6.39-rc2-01242-g3ef22b9-dirty #669 [ 23.674282] Call Trace: [ 23.674285] [<ffffffff813b94f3>] ? dev_disable_lro+0x83/0x90 [ 23.674291] [<ffffffff81042c9b>] warn_slowpath_common+0x8b/0xc0 [ 23.674298] [<ffffffff81042ce5>] warn_slowpath_null+0x15/0x20 [ 23.674304] [<ffffffff813b94f3>] dev_disable_lro+0x83/0x90 [ 23.674309] [<ffffffff81429789>] devinet_sysctl_forward+0x199/0x210 [ 23.674313] [<ffffffff814296e4>] ? devinet_sysctl_forward+0xf4/0x210 [ 23.674318] [<ffffffff8104e712>] ? capable+0x12/0x20 [ 23.674324] [<ffffffff81168f45>] proc_sys_call_handler+0xb5/0xd0 [ 23.674328] [<ffffffff81168f6f>] proc_sys_write+0xf/0x20 [ 23.674334] [<ffffffff81105f39>] vfs_write+0xc9/0x170 [ 23.674339] [<ffffffff81106550>] sys_write+0x50/0x90 [ 23.674345] [<ffffffff814b95a0>] sysenter_dispatch+0x7/0x33 [ 23.674350] ---[ end trace 051ec497c66b228e ]--- Thanks -- 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, Apr 21, 2011 at 04:52:11PM +0200, Eric Dumazet wrote: > Le mardi 12 avril 2011 à 21:38 +0200, Michał Mirosław a écrit : > > Since ndo_fix_features callback is postponing features change when > > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features() > > has to be called again when this condition changes. Previously, > > ethtool_ops->set_flags callback returned -EBUSY in that case > > (it's not possible in the new model). > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > > > v5: - don't delay set_features, as it's rtnl_locked - same as recovery process > > v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion > > - add check for failed ndo_set_features in ndo_open callback > > v3: - include NETIF_F_LRO in hw_features > > - don't call netdev_update_features() if bnx2x_nic_load() failed > > v2: - comment in ndo_fix_features callback > > --- > I am not sure its related to these changes, but I now have in > net-next-2.6 : > [ 23.674263] ------------[ cut here ]------------ > [ 23.674266] WARNING: at net/core/dev.c:1318 dev_disable_lro+0x83/0x90() > [ 23.674270] Hardware name: ProLiant BL460c G6 > [ 23.674273] Modules linked in: tg3 libphy sg > [ 23.674280] Pid: 3070, comm: sysctl Tainted: G W 2.6.39-rc2-01242-g3ef22b9-dirty #669 > [ 23.674282] Call Trace: > [ 23.674285] [<ffffffff813b94f3>] ? dev_disable_lro+0x83/0x90 > [ 23.674291] [<ffffffff81042c9b>] warn_slowpath_common+0x8b/0xc0 > [ 23.674298] [<ffffffff81042ce5>] warn_slowpath_null+0x15/0x20 > [ 23.674304] [<ffffffff813b94f3>] dev_disable_lro+0x83/0x90 > [ 23.674309] [<ffffffff81429789>] devinet_sysctl_forward+0x199/0x210 [...] Hmm. Looks like something is not allowing to disable LRO. Please check with following patch so we can be sure which driver causes this. 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
Le vendredi 22 avril 2011 à 00:41 +0200, Michał Mirosław a écrit : > On Thu, Apr 21, 2011 at 04:52:11PM +0200, Eric Dumazet wrote: > > Le mardi 12 avril 2011 à 21:38 +0200, Michał Mirosław a écrit : > > > Since ndo_fix_features callback is postponing features change when > > > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features() > > > has to be called again when this condition changes. Previously, > > > ethtool_ops->set_flags callback returned -EBUSY in that case > > > (it's not possible in the new model). > > > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > > > > > v5: - don't delay set_features, as it's rtnl_locked - same as recovery process > > > v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion > > > - add check for failed ndo_set_features in ndo_open callback > > > v3: - include NETIF_F_LRO in hw_features > > > - don't call netdev_update_features() if bnx2x_nic_load() failed > > > v2: - comment in ndo_fix_features callback > > > --- > > I am not sure its related to these changes, but I now have in > > net-next-2.6 : > > > [ 23.674263] ------------[ cut here ]------------ > > [ 23.674266] WARNING: at net/core/dev.c:1318 dev_disable_lro+0x83/0x90() > > [ 23.674270] Hardware name: ProLiant BL460c G6 > > [ 23.674273] Modules linked in: tg3 libphy sg > > [ 23.674280] Pid: 3070, comm: sysctl Tainted: G W 2.6.39-rc2-01242-g3ef22b9-dirty #669 > > [ 23.674282] Call Trace: > > [ 23.674285] [<ffffffff813b94f3>] ? dev_disable_lro+0x83/0x90 > > [ 23.674291] [<ffffffff81042c9b>] warn_slowpath_common+0x8b/0xc0 > > [ 23.674298] [<ffffffff81042ce5>] warn_slowpath_null+0x15/0x20 > > [ 23.674304] [<ffffffff813b94f3>] dev_disable_lro+0x83/0x90 > > [ 23.674309] [<ffffffff81429789>] devinet_sysctl_forward+0x199/0x210 > [...] > > Hmm. Looks like something is not allowing to disable LRO. Please check with > following patch so we can be sure which driver causes this. > > Best Regards, > Michał Mirosław Yes, obviously, and I suggested roughly the same patch some time ago. -- 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/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h index e0fca70..9e87417 100644 --- a/drivers/net/bnx2x/bnx2x.h +++ b/drivers/net/bnx2x/bnx2x.h @@ -918,7 +918,6 @@ struct bnx2x { int tx_ring_size; - u32 rx_csum; /* L2 header size + 2*VLANs (8 bytes) + LLC SNAP (8 bytes) */ #define ETH_OVREHEAD (ETH_HLEN + 8 + 8) #define ETH_MIN_PACKET_SIZE 60 diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c index e83ac6d..bec33a8 100644 --- a/drivers/net/bnx2x/bnx2x_cmn.c +++ b/drivers/net/bnx2x/bnx2x_cmn.c @@ -640,7 +640,7 @@ reuse_rx: skb_checksum_none_assert(skb); - if (bp->rx_csum) { + if (bp->dev->features & NETIF_F_RXCSUM) { if (likely(BNX2X_RX_CSUM_OK(cqe))) skb->ip_summed = CHECKSUM_UNNECESSARY; else @@ -2443,11 +2443,21 @@ alloc_err: } +static int bnx2x_reload_if_running(struct net_device *dev) +{ + struct bnx2x *bp = netdev_priv(dev); + + if (unlikely(!netif_running(dev))) + return 0; + + bnx2x_nic_unload(bp, UNLOAD_NORMAL); + return bnx2x_nic_load(bp, LOAD_NORMAL); +} + /* called with rtnl_lock */ int bnx2x_change_mtu(struct net_device *dev, int new_mtu) { struct bnx2x *bp = netdev_priv(dev); - int rc = 0; if (bp->recovery_state != BNX2X_RECOVERY_DONE) { printk(KERN_ERR "Handling parity error recovery. Try again later\n"); @@ -2464,12 +2474,39 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu) */ dev->mtu = new_mtu; - if (netif_running(dev)) { - bnx2x_nic_unload(bp, UNLOAD_NORMAL); - rc = bnx2x_nic_load(bp, LOAD_NORMAL); + return bnx2x_reload_if_running(dev); +} + +u32 bnx2x_fix_features(struct net_device *dev, u32 features) +{ + struct bnx2x *bp = netdev_priv(dev); + + /* TPA requires Rx CSUM offloading */ + if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa) + features &= ~NETIF_F_LRO; + + return features; +} + +int bnx2x_set_features(struct net_device *dev, u32 features) +{ + struct bnx2x *bp = netdev_priv(dev); + u32 flags = bp->flags; + + if (features & NETIF_F_LRO) + flags |= TPA_ENABLE_FLAG; + else + flags &= ~TPA_ENABLE_FLAG; + + if (flags ^ bp->flags) { + bp->flags = flags; + + if (bp->recovery_state == BNX2X_RECOVERY_DONE) + return bnx2x_reload_if_running(dev); + /* else: bnx2x_nic_load() will be called at end of recovery */ } - return rc; + return 0; } void bnx2x_tx_timeout(struct net_device *dev) diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h index 775fef0..1cdab69 100644 --- a/drivers/net/bnx2x/bnx2x_cmn.h +++ b/drivers/net/bnx2x/bnx2x_cmn.h @@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp); */ int bnx2x_change_mtu(struct net_device *dev, int new_mtu); +u32 bnx2x_fix_features(struct net_device *dev, u32 features); +int bnx2x_set_features(struct net_device *dev, u32 features); + /** * tx timeout netdev callback * diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c index 1479994..ad7d91e 100644 --- a/drivers/net/bnx2x/bnx2x_ethtool.c +++ b/drivers/net/bnx2x/bnx2x_ethtool.c @@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev, return 0; } -static int bnx2x_set_flags(struct net_device *dev, u32 data) -{ - struct bnx2x *bp = netdev_priv(dev); - int changed = 0; - int rc = 0; - - if (bp->recovery_state != BNX2X_RECOVERY_DONE) { - printk(KERN_ERR "Handling parity error recovery. Try again later\n"); - return -EAGAIN; - } - - if (!(data & ETH_FLAG_RXVLAN)) - return -EINVAL; - - if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa) - return -EINVAL; - - rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN | - ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH); - if (rc) - return rc; - - /* TPA requires Rx CSUM offloading */ - if ((data & ETH_FLAG_LRO) && bp->rx_csum) { - if (!(bp->flags & TPA_ENABLE_FLAG)) { - bp->flags |= TPA_ENABLE_FLAG; - changed = 1; - } - } else if (bp->flags & TPA_ENABLE_FLAG) { - dev->features &= ~NETIF_F_LRO; - bp->flags &= ~TPA_ENABLE_FLAG; - changed = 1; - } - - if (changed && netif_running(dev)) { - bnx2x_nic_unload(bp, UNLOAD_NORMAL); - rc = bnx2x_nic_load(bp, LOAD_NORMAL); - } - - return rc; -} - -static u32 bnx2x_get_rx_csum(struct net_device *dev) -{ - struct bnx2x *bp = netdev_priv(dev); - - return bp->rx_csum; -} - -static int bnx2x_set_rx_csum(struct net_device *dev, u32 data) -{ - struct bnx2x *bp = netdev_priv(dev); - int rc = 0; - - if (bp->recovery_state != BNX2X_RECOVERY_DONE) { - printk(KERN_ERR "Handling parity error recovery. Try again later\n"); - return -EAGAIN; - } - - bp->rx_csum = data; - - /* Disable TPA, when Rx CSUM is disabled. Otherwise all - TPA'ed packets will be discarded due to wrong TCP CSUM */ - if (!data) { - u32 flags = ethtool_op_get_flags(dev); - - rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO)); - } - - return rc; -} - -static int bnx2x_set_tso(struct net_device *dev, u32 data) -{ - if (data) { - dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN); - dev->features |= NETIF_F_TSO6; - } else { - dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN); - dev->features &= ~NETIF_F_TSO6; - } - - return 0; -} - static const struct { char string[ETH_GSTRING_LEN]; } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = { @@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = { .set_ringparam = bnx2x_set_ringparam, .get_pauseparam = bnx2x_get_pauseparam, .set_pauseparam = bnx2x_set_pauseparam, - .get_rx_csum = bnx2x_get_rx_csum, - .set_rx_csum = bnx2x_set_rx_csum, - .get_tx_csum = ethtool_op_get_tx_csum, - .set_tx_csum = ethtool_op_set_tx_hw_csum, - .set_flags = bnx2x_set_flags, - .get_flags = ethtool_op_get_flags, - .get_sg = ethtool_op_get_sg, - .set_sg = ethtool_op_set_sg, - .get_tso = ethtool_op_get_tso, - .set_tso = bnx2x_set_tso, .self_test = bnx2x_self_test, .get_sset_count = bnx2x_get_sset_count, .get_strings = bnx2x_get_strings, diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c index a6915aa..696e84a 100644 --- a/drivers/net/bnx2x/bnx2x_main.c +++ b/drivers/net/bnx2x/bnx2x_main.c @@ -8904,8 +8904,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp) bp->multi_mode = multi_mode; bp->int_mode = int_mode; - bp->dev->features |= NETIF_F_GRO; - /* Set TPA flags */ if (disable_tpa) { bp->flags &= ~TPA_ENABLE_FLAG; @@ -8925,8 +8923,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp) bp->tx_ring_size = MAX_TX_AVAIL; - bp->rx_csum = 1; - /* make sure that the numbers are in the right granularity */ bp->tx_ticks = (50 / BNX2X_BTR) * BNX2X_BTR; bp->rx_ticks = (25 / BNX2X_BTR) * BNX2X_BTR; @@ -9304,6 +9300,8 @@ static const struct net_device_ops bnx2x_netdev_ops = { .ndo_validate_addr = eth_validate_addr, .ndo_do_ioctl = bnx2x_ioctl, .ndo_change_mtu = bnx2x_change_mtu, + .ndo_fix_features = bnx2x_fix_features, + .ndo_set_features = bnx2x_set_features, .ndo_tx_timeout = bnx2x_tx_timeout, #ifdef CONFIG_NET_POLL_CONTROLLER .ndo_poll_controller = poll_bnx2x, @@ -9430,20 +9428,17 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev, dev->netdev_ops = &bnx2x_netdev_ops; bnx2x_set_ethtool_ops(dev); - dev->features |= NETIF_F_SG; - dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; + + dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | + NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_HW_VLAN_TX; + + dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA; + + dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX; if (bp->flags & USING_DAC_FLAG) dev->features |= NETIF_F_HIGHDMA; - dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN); - dev->features |= NETIF_F_TSO6; - dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX); - - dev->vlan_features |= NETIF_F_SG; - dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; - if (bp->flags & USING_DAC_FLAG) - dev->vlan_features |= NETIF_F_HIGHDMA; - dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN); - dev->vlan_features |= NETIF_F_TSO6; #ifdef BCM_DCBNL dev->dcbnl_ops = &bnx2x_dcbnl_ops;
Since ndo_fix_features callback is postponing features change when bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features() has to be called again when this condition changes. Previously, ethtool_ops->set_flags callback returned -EBUSY in that case (it's not possible in the new model). Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> v5: - don't delay set_features, as it's rtnl_locked - same as recovery process v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion - add check for failed ndo_set_features in ndo_open callback v3: - include NETIF_F_LRO in hw_features - don't call netdev_update_features() if bnx2x_nic_load() failed v2: - comment in ndo_fix_features callback --- drivers/net/bnx2x/bnx2x.h | 1 - drivers/net/bnx2x/bnx2x_cmn.c | 49 +++++++++++++++++-- drivers/net/bnx2x/bnx2x_cmn.h | 3 + drivers/net/bnx2x/bnx2x_ethtool.c | 95 ------------------------------------- drivers/net/bnx2x/bnx2x_main.c | 27 ++++------ 5 files changed, 57 insertions(+), 118 deletions(-)