| Message ID | 20100618105945.6496.67648.sendpatchset@localhost.localdomain |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jun 18, 2010 at 06:55:38AM -0400, Amerigo Wang wrote: > +static int mlx4_ethtool_op_set_flags(struct net_device *dev, u32 data) > +{ > + struct mlx4_en_priv *priv = netdev_priv(dev); > + struct mlx4_en_dev *mdev = priv->mdev; > + int rc = 0; > + int changed = 0; > + > + if (data & (ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH)) > + return -EOPNOTSUPP; > + > + if (data & ETH_FLAG_LRO) { > + if (!(dev->features & NETIF_F_LRO)) > + changed = 1; > + } else if (dev->features & NETIF_F_LRO) { > + changed = 1; > + mdev->profile.num_lro = 0; Everything fine except that, what for you zero num_lro value? If we set it to zero it will stay zero and we will not create proper number of lro descriptors in mlx4_en_create_rx_ring() (called from mlx4_en_set_ringparam() -> mlx4_en_alloc_resources()) when someone enable LRO again on. Stanislaw -- 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 06/18/10 19:09, Stanislaw Gruszka wrote: > On Fri, Jun 18, 2010 at 06:55:38AM -0400, Amerigo Wang wrote: >> +static int mlx4_ethtool_op_set_flags(struct net_device *dev, u32 data) >> +{ >> + struct mlx4_en_priv *priv = netdev_priv(dev); >> + struct mlx4_en_dev *mdev = priv->mdev; >> + int rc = 0; >> + int changed = 0; >> + >> + if (data& (ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH)) >> + return -EOPNOTSUPP; >> + >> + if (data& ETH_FLAG_LRO) { >> + if (!(dev->features& NETIF_F_LRO)) >> + changed = 1; >> + } else if (dev->features& NETIF_F_LRO) { >> + changed = 1; >> + mdev->profile.num_lro = 0; > > Everything fine except that, what for you zero num_lro value? > > If we set it to zero it will stay zero and we will not create > proper number of lro descriptors in mlx4_en_create_rx_ring() > (called from mlx4_en_set_ringparam() -> mlx4_en_alloc_resources()) > when someone enable LRO again on. > Huh? Isn't ->num_lro which controls LRO of mlx4 driver? -- 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 Mon, Jun 21, 2010 at 10:23:00AM +0800, Cong Wang wrote: > On 06/18/10 19:09, Stanislaw Gruszka wrote: >> On Fri, Jun 18, 2010 at 06:55:38AM -0400, Amerigo Wang wrote: >>> +static int mlx4_ethtool_op_set_flags(struct net_device *dev, u32 data) >>> +{ >>> + struct mlx4_en_priv *priv = netdev_priv(dev); >>> + struct mlx4_en_dev *mdev = priv->mdev; >>> + int rc = 0; >>> + int changed = 0; >>> + >>> + if (data& (ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH)) >>> + return -EOPNOTSUPP; As we are here, better would be if (data & ~ETH_FLAG_LRO) return -EOPNOTSUPP; code will persist correct when someone add new flags. >>> + >>> + if (data& ETH_FLAG_LRO) { >>> + if (!(dev->features& NETIF_F_LRO)) >>> + changed = 1; >>> + } else if (dev->features& NETIF_F_LRO) { >>> + changed = 1; >>> + mdev->profile.num_lro = 0; >> >> Everything fine except that, what for you zero num_lro value? >> >> If we set it to zero it will stay zero and we will not create >> proper number of lro descriptors in mlx4_en_create_rx_ring() >> (called from mlx4_en_set_ringparam() -> mlx4_en_alloc_resources()) >> when someone enable LRO again on. >> > > Huh? Isn't ->num_lro which controls LRO of mlx4 driver? It is, but only in mlx4_en_add() just before register_netdev(), when we setup default dev->features. Otherwise dev->features tells if LRO is enabled or disabled. This realize me, that we should not dev->features |= NETIF_F_LRO if mdev->profile.num_lro == 0 . Stanislaw -- 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 06/21/10 17:02, Stanislaw Gruszka wrote: > On Mon, Jun 21, 2010 at 10:23:00AM +0800, Cong Wang wrote: >> On 06/18/10 19:09, Stanislaw Gruszka wrote: >>> On Fri, Jun 18, 2010 at 06:55:38AM -0400, Amerigo Wang wrote: >>>> +static int mlx4_ethtool_op_set_flags(struct net_device *dev, u32 data) >>>> +{ >>>> + struct mlx4_en_priv *priv = netdev_priv(dev); >>>> + struct mlx4_en_dev *mdev = priv->mdev; >>>> + int rc = 0; >>>> + int changed = 0; >>>> + >>>> + if (data& (ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH)) >>>> + return -EOPNOTSUPP; > > As we are here, better would be > if (data& ~ETH_FLAG_LRO) > return -EOPNOTSUPP; > code will persist correct when someone add new flags. Yeah, better. > >>>> + >>>> + if (data& ETH_FLAG_LRO) { >>>> + if (!(dev->features& NETIF_F_LRO)) >>>> + changed = 1; >>>> + } else if (dev->features& NETIF_F_LRO) { >>>> + changed = 1; >>>> + mdev->profile.num_lro = 0; >>> >>> Everything fine except that, what for you zero num_lro value? >>> >>> If we set it to zero it will stay zero and we will not create >>> proper number of lro descriptors in mlx4_en_create_rx_ring() >>> (called from mlx4_en_set_ringparam() -> mlx4_en_alloc_resources()) >>> when someone enable LRO again on. >>> >> >> Huh? Isn't ->num_lro which controls LRO of mlx4 driver? > > It is, but only in mlx4_en_add() just before register_netdev(), > when we setup default dev->features. Otherwise dev->features > tells if LRO is enabled or disabled. > > This realize me, that we should not dev->features |= NETIF_F_LRO > if mdev->profile.num_lro == 0 . > Got it! I misunderstood ->num_lro. :( Setting NETIF_F_LRO should be enough for mlx4 driver... I will send the updated version. 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
diff --git a/drivers/net/mlx4/en_ethtool.c b/drivers/net/mlx4/en_ethtool.c index d5afd03..d68b796 100644 --- a/drivers/net/mlx4/en_ethtool.c +++ b/drivers/net/mlx4/en_ethtool.c @@ -387,6 +387,41 @@ static void mlx4_en_get_ringparam(struct net_device *dev, param->tx_pending = mdev->profile.prof[priv->port].tx_ring_size; } +static int mlx4_ethtool_op_set_flags(struct net_device *dev, u32 data) +{ + struct mlx4_en_priv *priv = netdev_priv(dev); + struct mlx4_en_dev *mdev = priv->mdev; + int rc = 0; + int changed = 0; + + if (data & (ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH)) + return -EOPNOTSUPP; + + if (data & ETH_FLAG_LRO) { + if (!(dev->features & NETIF_F_LRO)) + changed = 1; + } else if (dev->features & NETIF_F_LRO) { + changed = 1; + mdev->profile.num_lro = 0; + } + + if (changed) { + if (netif_running(dev)) { + mutex_lock(&mdev->state_lock); + mlx4_en_stop_port(dev); + } + dev->features ^= NETIF_F_LRO; + if (netif_running(dev)) { + rc = mlx4_en_start_port(dev); + if (rc) + en_err(priv, "Failed to restart port\n"); + mutex_unlock(&mdev->state_lock); + } + } + + return rc; +} + const struct ethtool_ops mlx4_en_ethtool_ops = { .get_drvinfo = mlx4_en_get_drvinfo, .get_settings = mlx4_en_get_settings, @@ -415,7 +450,7 @@ const struct ethtool_ops mlx4_en_ethtool_ops = { .get_ringparam = mlx4_en_get_ringparam, .set_ringparam = mlx4_en_set_ringparam, .get_flags = ethtool_op_get_flags, - .set_flags = ethtool_op_set_flags, + .set_flags = mlx4_ethtool_op_set_flags, };