Skip to contentSkip to conversation

s2io: add dynamic LRO disable support

PATCH Superseded Amerigo Wang 2 participants 6 messages Jun 18, 2010 – Jun 22, 2010

Subsystems (2)
Networking Network drivers

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Amerigo Wang

author
#0 lore raw

PATCH v3 1/2 s2io: add dynamic LRO disable support

This patch adds dynamic LRO diable support for s2io net driver.

I don't have s2io card, so only did compiling test. Anyone who wants to test this is more than welcome.

This is based on Neil's initial work.

Signed-off-by WANG Cong <[email protected]>
Signed-off-by Neil Horman <[email protected]>
Acked-by Neil Horman <[email protected]>
Reviewed-by Stanislaw Gruszka <[email protected]>
Cc Ramkrishna Vepa <[email protected]>

---

drivers/net/s2io.c +38 -1
index 668327c..e380f37 100644
@@ -795,7 +795,6 @@ static int init_shared_mem(struct s2io_nic *nic)
 		ring->rx_curr_put_info.ring_len = rx_cfg->num_rxd - 1;
 		ring->nic = nic;
 		ring->ring_no = i;
-		ring->lro = lro_enable;
 
 		blk_cnt = rx_cfg->num_rxd / (rxd_count[nic->rxd_mode] + 1);
 		/*  Allocating all the Rx blocks */

@@ -6684,6 +6683,41 @@ static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data)
 
 	return 0;
 }
+static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
+{
+	struct s2io_nic *sp = netdev_priv(dev);
+	int rc = 0;
+	int changed = 0;
+
+	if (data & (ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH))
+		return -EOPNOTSUPP;
+
+	if (data & ETH_FLAG_LRO) {
+		if (lro_enable) {
+			if (!(dev->features & NETIF_F_LRO)) {
+				dev->features |= NETIF_F_LRO;
+				changed = 1;
+			}
+		} else
+			rc = -EINVAL;
+	} else if (dev->features & NETIF_F_LRO) {
+		dev->features &= ~NETIF_F_LRO;
+		changed = 1;
+	}
+
+	if (changed && netif_running(dev)) {
+		s2io_stop_all_tx_queue(sp);
+		s2io_card_down(sp);
+		sp->lro = dev->features & NETIF_F_LRO;
+		rc = s2io_card_up(sp);
+		if (rc)
+			s2io_reset(sp);
+		else
+			s2io_start_all_tx_queue(sp);
+	}
+
+	return rc;
+}
 
 static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_settings = s2io_ethtool_gset,

@@ -6701,6 +6735,8 @@ static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_rx_csum = s2io_ethtool_get_rx_csum,
 	.set_rx_csum = s2io_ethtool_set_rx_csum,
 	.set_tx_csum = s2io_ethtool_op_set_tx_csum,
+	.set_flags = s2io_ethtool_set_flags,
+	.get_flags = ethtool_op_get_flags,
 	.set_sg = ethtool_op_set_sg,
 	.get_tso = s2io_ethtool_op_get_tso,
 	.set_tso = s2io_ethtool_op_set_tso,

@@ -7229,6 +7265,7 @@ static int s2io_card_up(struct s2io_nic *sp)
 		struct ring_info *ring = &mac_control->rings[i];
 
 		ring->mtu = dev->mtu;
+		ring->lro = sp->lro;
 		ret = fill_rx_buffers(sp, ring, 1);
 		if (ret) {
 			DBG_PRINT(ERR_DBG, "%s: Out of memory in Open\n",
author
#1 lore raw

PATCH v3 2/2 mlx4: add dynamic LRO disable support

This patch adds dynamic LRO diable support for mlx4 net driver. It also fixes a bug of mlx4, which checks NETIF_F_LRO flag in rx path without rtnl lock.

I don't have mlx4 card, so only did compiling test. Anyone who wants to test this is more than welcome.

This is based on Neil's initial work too.

Signed-off-by WANG Cong <[email protected]>
Signed-off-by Neil Horman <[email protected]>
Acked-by Neil Horman <[email protected]>
Reviewed-by Stanislaw Gruszka <[email protected]>
Cc Ben Hutchings <[email protected]>

---

drivers/net/mlx4/en_ethtool.c +36 -1
index d5afd03..d68b796 100644
@@ -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,
 };
#2 lore raw

Re: [v3 Patch 2/2] mlx4: add dynamic LRO disable support

Quoted 6 lines · Amerigo Wang
drivers/net/mlx4/en_ethtool.c +6 -0
+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;

drivers/net/mlx4/en_ethtool.c static void mlx4_en_get_ringparam(struct net_device *dev, · line 405

Quoted 10 lines
drivers/net/mlx4/en_ethtool.c +10 -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

author
#3 lore raw

Re: [v3 Patch 2/2] mlx4: add dynamic LRO disable support

Quoted 20 lines · Stanislaw Gruszka
Quoted 6 lines · Amerigo Wang
drivers/net/mlx4/en_ethtool.c +6 -0
+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;

drivers/net/mlx4/en_ethtool.c static void mlx4_en_get_ringparam(struct net_device *dev, · line 405

Quoted 10 lines
drivers/net/mlx4/en_ethtool.c +10 -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?

Quoted 5 lines

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?

#4 lore raw

Re: [v3 Patch 2/2] mlx4: add dynamic LRO disable support

Quoted 11 lines · Cong Wang
Quoted 10 lines · Stanislaw Gruszka

drivers/net/mlx4/en_ethtool.c static void mlx4_en_get_ringparam(struct net_device *dev, · line 398

Quoted 9 lines · Amerigo Wang
drivers/net/mlx4/en_ethtool.c +9 -0
+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.

Quoted 10 lines
Quoted 10 lines

drivers/net/mlx4/en_ethtool.c static void mlx4_en_get_ringparam(struct net_device *dev, · line 405

Quoted 7 lines
drivers/net/mlx4/en_ethtool.c +7 -0
+
+	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?

Quoted 7 lines
Quoted 5 lines

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

author
#5 lore raw

Re: [v3 Patch 2/2] mlx4: add dynamic LRO disable support

Quoted 14 lines · Stanislaw Gruszka
Quoted 11 lines · Cong Wang
Quoted 10 lines · Stanislaw Gruszka

drivers/net/mlx4/en_ethtool.c static void mlx4_en_get_ringparam(struct net_device *dev, · line 398

Quoted 9 lines · Amerigo Wang
drivers/net/mlx4/en_ethtool.c +9 -0
+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

Quoted 3 lines
	if (data&  ~ETH_FLAG_LRO)
		return -EOPNOTSUPP;

code will persist correct when someone add new flags.

Yeah, better.

Quoted 17 lines
Quoted 17 lines
Quoted 16 lines

drivers/net/mlx4/en_ethtool.c static void mlx4_en_get_ringparam(struct net_device *dev, · line 405

Quoted 8 lines
drivers/net/mlx4/en_ethtool.c +7 -0
+
+	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.

Quoted 9 lines
Quoted 1 line

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!