* [v4 Patch 1/2] s2io: add dynamic LRO disable support
@ 2010-06-22 8:50 Amerigo Wang
2010-06-22 8:50 ` [v4 Patch 2/2] mlx4: " Amerigo Wang
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Amerigo Wang @ 2010-06-22 8:50 UTC (permalink / raw)
To: netdev
Cc: nhorman, sgruszka, herbert.xu, Amerigo Wang, bhutchings,
Ramkrishna.Vepa, davem
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, and heavily modified
based on Ramkrishna's suggestions.
Signed-off-by: WANG Cong <amwang@redhat.com>
Signed-off-by: Neil Horman <nhorman@redhat.com>
Acked-by: Neil Horman <nhorman@redhat.com>
Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Ramkrishna Vepa <Ramkrishna.Vepa@exar.com>
---
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 668327c..2e6e3b3 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -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_LRO)
+ 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 = -EOPNOTSUPP;
+ } 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",
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [v4 Patch 2/2] mlx4: add dynamic LRO disable support
2010-06-22 8:50 [v4 Patch 1/2] s2io: add dynamic LRO disable support Amerigo Wang
@ 2010-06-22 8:50 ` Amerigo Wang
2010-06-29 6:04 ` David Miller
2010-06-22 11:44 ` [v4 Patch 1/2] s2io: " Ben Hutchings
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Amerigo Wang @ 2010-06-22 8:50 UTC (permalink / raw)
To: netdev
Cc: nhorman, sgruszka, herbert.xu, Amerigo Wang, bhutchings,
Ramkrishna.Vepa, davem
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, and heavily modified based
on Stanislaw's suggestions.
Signed-off-by: WANG Cong <amwang@redhat.com>
Signed-off-by: Neil Horman <nhorman@redhat.com>
Acked-by: Neil Horman <nhorman@redhat.com>
Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
diff --git a/drivers/net/mlx4/en_ethtool.c b/drivers/net/mlx4/en_ethtool.c
index d5afd03..b275238 100644
--- a/drivers/net/mlx4/en_ethtool.c
+++ b/drivers/net/mlx4/en_ethtool.c
@@ -387,6 +387,42 @@ 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_LRO)
+ return -EOPNOTSUPP;
+
+ if (data & ETH_FLAG_LRO) {
+ if (mdev->profile.num_lro == 0)
+ return -EOPNOTSUPP;
+ if (!(dev->features & NETIF_F_LRO))
+ changed = 1;
+ } else if (dev->features & NETIF_F_LRO) {
+ changed = 1;
+ }
+
+ 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 +451,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,
};
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [v4 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-22 8:50 [v4 Patch 1/2] s2io: add dynamic LRO disable support Amerigo Wang
2010-06-22 8:50 ` [v4 Patch 2/2] mlx4: " Amerigo Wang
@ 2010-06-22 11:44 ` Ben Hutchings
2010-06-22 12:31 ` Stanislaw Gruszka
2010-06-25 8:59 ` Cong Wang
2010-06-25 4:33 ` Jon Mason
2010-06-25 4:45 ` [PATCH] " Jon Mason
3 siblings, 2 replies; 11+ messages in thread
From: Ben Hutchings @ 2010-06-22 11:44 UTC (permalink / raw)
To: Amerigo Wang
Cc: netdev, nhorman, sgruszka, herbert.xu, Ramkrishna.Vepa, davem
On Tue, 2010-06-22 at 04:50 -0400, Amerigo Wang wrote:
> 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, and heavily modified
> based on Ramkrishna's suggestions.
[...]
> +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_LRO)
> + 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 = -EOPNOTSUPP;
Should lro_enable=0 really prevent enabling it later? This seems
unusual.
> + } 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;
[...]
This means s2io_nic::lro and ring_info::lro can have the value
NETIF_F_LRO, where previously they would only have the value 0 or 1. I
don't know whether this could be a problem, but the safe thing to do is
to coerce the value by writing !!(dev->features & NETIF_F_LRO).
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v4 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-22 11:44 ` [v4 Patch 1/2] s2io: " Ben Hutchings
@ 2010-06-22 12:31 ` Stanislaw Gruszka
2010-06-25 8:59 ` Cong Wang
1 sibling, 0 replies; 11+ messages in thread
From: Stanislaw Gruszka @ 2010-06-22 12:31 UTC (permalink / raw)
To: Ben Hutchings
Cc: Amerigo Wang, netdev, nhorman, herbert.xu, Ramkrishna.Vepa, davem
On Tue, 22 Jun 2010 12:44:40 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Tue, 2010-06-22 at 04:50 -0400, Amerigo Wang wrote:
> > 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, and heavily modified
> > based on Ramkrishna's suggestions.
> [...]
> > +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_LRO)
> > + 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 = -EOPNOTSUPP;
>
> Should lro_enable=0 really prevent enabling it later? This seems
> unusual.
We are doing this in bnx2x. Current Amerigo patch change to the
same behavior mlx4. For me that have sense - if you want to disallow
LRO - use module option.
In this case however lro_enable variable looks obsolete from times
where there was no ethtool possibility to dynamic set/unset LRO.
Perhaps it should be removed at all, but maybe not as part
of that patch.
Stanislaw
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v4 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-22 8:50 [v4 Patch 1/2] s2io: add dynamic LRO disable support Amerigo Wang
2010-06-22 8:50 ` [v4 Patch 2/2] mlx4: " Amerigo Wang
2010-06-22 11:44 ` [v4 Patch 1/2] s2io: " Ben Hutchings
@ 2010-06-25 4:33 ` Jon Mason
2010-06-25 9:01 ` Cong Wang
2010-06-25 4:45 ` [PATCH] " Jon Mason
3 siblings, 1 reply; 11+ messages in thread
From: Jon Mason @ 2010-06-25 4:33 UTC (permalink / raw)
To: Amerigo Wang
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, bhutchings@solarflare.com, Ramkrishna Vepa,
davem@davemloft.net
On Tue, Jun 22, 2010 at 01:50:07AM -0700, Amerigo Wang wrote:
>
> 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.)
Unfortunately the patch did not quite work, mostly due to NETIF_F_LRO
needing to be added at probe time to the device features. I was able
to modify the patch to get it working, and will respond seperately
with the working patch.
Thanks,
Jon
>
> This is based on Neil's initial work, and heavily modified
> based on Ramkrishna's suggestions.
>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Signed-off-by: Neil Horman <nhorman@redhat.com>
> Acked-by: Neil Horman <nhorman@redhat.com>
> Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
> Cc: Ramkrishna Vepa <Ramkrishna.Vepa@exar.com>
>
> ---
>
> diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
> index 668327c..2e6e3b3 100644
> --- a/drivers/net/s2io.c
> +++ b/drivers/net/s2io.c
> @@ -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_LRO)
> + 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 = -EOPNOTSUPP;
> + } 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",
> --
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] s2io: add dynamic LRO disable support
2010-06-22 8:50 [v4 Patch 1/2] s2io: add dynamic LRO disable support Amerigo Wang
` (2 preceding siblings ...)
2010-06-25 4:33 ` Jon Mason
@ 2010-06-25 4:45 ` Jon Mason
2010-06-25 9:09 ` Cong Wang
2010-06-29 6:04 ` David Miller
3 siblings, 2 replies; 11+ messages in thread
From: Jon Mason @ 2010-06-25 4:45 UTC (permalink / raw)
To: Amerigo Wang
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, bhutchings@solarflare.com, Ramkrishna Vepa,
davem@davemloft.net
This patch adds dynamic LRO disable support for s2io net driver,
enables LRO by default, increases the driver version number, and
corrects the name of the LRO modparm.
This is mostly Wang's patch based on Neil's initial work, heavily
modified based on Ramkrishna's suggestions. This has been tested on
a Neterion Xframe adapter and verified via adapter LRO statistics.
Signed-off-by: Jon Mason <jon.mason@exar.com>
Signed-off-by: WANG Cong <amwang@redhat.com>
Signed-off-by: Neil Horman <nhorman@redhat.com>
Acked-by: Neil Horman <nhorman@redhat.com>
Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Ramkrishna Vepa <Ramkrishna.Vepa@exar.com>
---
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 668327c..a032d72 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -38,7 +38,7 @@
* Tx descriptors that can be associated with each corresponding FIFO.
* intr_type: This defines the type of interrupt. The values can be 0(INTA),
* 2(MSI_X). Default value is '2(MSI_X)'
- * lro_enable: Specifies whether to enable Large Receive Offload (LRO) or not.
+ * lro: Specifies whether to enable Large Receive Offload (LRO) or not.
* Possible values '1' for enable '0' for disable. Default is '0'
* lro_max_pkts: This parameter defines maximum number of packets can be
* aggregated as a single large packet
@@ -90,7 +90,7 @@
#include "s2io.h"
#include "s2io-regs.h"
-#define DRV_VERSION "2.0.26.25"
+#define DRV_VERSION "2.0.26.26"
/* S2io Driver name & version. */
static char s2io_driver_name[] = "Neterion";
@@ -496,7 +496,7 @@ S2IO_PARM_INT(rxsync_frequency, 3);
/* Interrupt type. Values can be 0(INTA), 2(MSI_X) */
S2IO_PARM_INT(intr_type, 2);
/* Large receive offload feature */
-static unsigned int lro_enable;
+static unsigned int lro_enable = 1;
module_param_named(lro, lro_enable, uint, 0);
/* Max pkts to be aggregated by LRO at one time. If not specified,
@@ -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 */
@@ -6675,6 +6674,7 @@ static u32 s2io_ethtool_op_get_tso(struct net_device *dev)
{
return (dev->features & NETIF_F_TSO) != 0;
}
+
static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data)
{
if (data)
@@ -6685,6 +6685,42 @@ 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_LRO)
+ 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,
.set_settings = s2io_ethtool_sset,
@@ -6701,6 +6737,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 +7267,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",
@@ -7974,7 +8013,8 @@ s2io_init_nic(struct pci_dev *pdev, const struct pci_device_id *pre)
dev->netdev_ops = &s2io_netdev_ops;
SET_ETHTOOL_OPS(dev, &netdev_ethtool_ops);
dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
-
+ if (lro_enable)
+ dev->features |= NETIF_F_LRO;
dev->features |= NETIF_F_SG | NETIF_F_IP_CSUM;
if (sp->high_dma_flag == true)
dev->features |= NETIF_F_HIGHDMA;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [v4 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-22 11:44 ` [v4 Patch 1/2] s2io: " Ben Hutchings
2010-06-22 12:31 ` Stanislaw Gruszka
@ 2010-06-25 8:59 ` Cong Wang
1 sibling, 0 replies; 11+ messages in thread
From: Cong Wang @ 2010-06-25 8:59 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev, nhorman, sgruszka, herbert.xu, Ramkrishna.Vepa, davem
On 06/22/10 19:44, Ben Hutchings wrote:
> On Tue, 2010-06-22 at 04:50 -0400, Amerigo Wang wrote:
>> 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, and heavily modified
>> based on Ramkrishna's suggestions.
> [...]
>> +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_LRO)
>> + 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 = -EOPNOTSUPP;
>
> Should lro_enable=0 really prevent enabling it later? This seems
> unusual.
>
I agree with Stanislaw on this point.
>> + } 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;
> [...]
>
> This means s2io_nic::lro and ring_info::lro can have the value
> NETIF_F_LRO, where previously they would only have the value 0 or 1. I
> don't know whether this could be a problem, but the safe thing to do is
> to coerce the value by writing !!(dev->features& NETIF_F_LRO).
>
Yeah, agreed.
It seems that Jon already fixed this when he updated the patch.
Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v4 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-25 4:33 ` Jon Mason
@ 2010-06-25 9:01 ` Cong Wang
0 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2010-06-25 9:01 UTC (permalink / raw)
To: Jon Mason
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, bhutchings@solarflare.com, Ramkrishna Vepa,
davem@davemloft.net
On 06/25/10 12:33, Jon Mason wrote:
> On Tue, Jun 22, 2010 at 01:50:07AM -0700, Amerigo Wang wrote:
>>
>> 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.)
>
> Unfortunately the patch did not quite work, mostly due to NETIF_F_LRO
> needing to be added at probe time to the device features. I was able
> to modify the patch to get it working, and will respond seperately
> with the working patch.
>
Thanks very much, Jon!
I am very pleased to see this patch gets tested. :)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] s2io: add dynamic LRO disable support
2010-06-25 4:45 ` [PATCH] " Jon Mason
@ 2010-06-25 9:09 ` Cong Wang
2010-06-29 6:04 ` David Miller
1 sibling, 0 replies; 11+ messages in thread
From: Cong Wang @ 2010-06-25 9:09 UTC (permalink / raw)
To: Jon Mason
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, bhutchings@solarflare.com, Ramkrishna Vepa,
davem@davemloft.net
On 06/25/10 12:45, Jon Mason wrote:
> This patch adds dynamic LRO disable support for s2io net driver,
> enables LRO by default, increases the driver version number, and
> corrects the name of the LRO modparm.
>
Very good work, Jon! I believe this version is the best we have.
Thanks again!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] s2io: add dynamic LRO disable support
2010-06-25 4:45 ` [PATCH] " Jon Mason
2010-06-25 9:09 ` Cong Wang
@ 2010-06-29 6:04 ` David Miller
1 sibling, 0 replies; 11+ messages in thread
From: David Miller @ 2010-06-29 6:04 UTC (permalink / raw)
To: jon.mason
Cc: amwang, netdev, nhorman, sgruszka, herbert.xu, bhutchings,
Ramkrishna.Vepa
From: Jon Mason <jon.mason@exar.com>
Date: Thu, 24 Jun 2010 23:45:10 -0500
> This patch adds dynamic LRO disable support for s2io net driver,
> enables LRO by default, increases the driver version number, and
> corrects the name of the LRO modparm.
>
> This is mostly Wang's patch based on Neil's initial work, heavily
> modified based on Ramkrishna's suggestions. This has been tested on
> a Neterion Xframe adapter and verified via adapter LRO statistics.
>
> Signed-off-by: Jon Mason <jon.mason@exar.com>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Signed-off-by: Neil Horman <nhorman@redhat.com>
> Acked-by: Neil Horman <nhorman@redhat.com>
> Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
> Cc: Ramkrishna Vepa <Ramkrishna.Vepa@exar.com>
Applied to net-next-2.6
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v4 Patch 2/2] mlx4: add dynamic LRO disable support
2010-06-22 8:50 ` [v4 Patch 2/2] mlx4: " Amerigo Wang
@ 2010-06-29 6:04 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2010-06-29 6:04 UTC (permalink / raw)
To: amwang; +Cc: netdev, nhorman, sgruszka, herbert.xu, bhutchings,
Ramkrishna.Vepa
From: Amerigo Wang <amwang@redhat.com>
Date: Tue, 22 Jun 2010 04:50:17 -0400
>
> 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, and heavily modified based
> on Stanislaw's suggestions.
>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Signed-off-by: Neil Horman <nhorman@redhat.com>
> Acked-by: Neil Horman <nhorman@redhat.com>
> Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
Applied to net-next-2.6
I'd really like the module options to just die as the dynamic
ethtool mechanism should be the only knob for this for consistency
with the rest of the drivers.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-06-29 6:04 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-22 8:50 [v4 Patch 1/2] s2io: add dynamic LRO disable support Amerigo Wang
2010-06-22 8:50 ` [v4 Patch 2/2] mlx4: " Amerigo Wang
2010-06-29 6:04 ` David Miller
2010-06-22 11:44 ` [v4 Patch 1/2] s2io: " Ben Hutchings
2010-06-22 12:31 ` Stanislaw Gruszka
2010-06-25 8:59 ` Cong Wang
2010-06-25 4:33 ` Jon Mason
2010-06-25 9:01 ` Cong Wang
2010-06-25 4:45 ` [PATCH] " Jon Mason
2010-06-25 9:09 ` Cong Wang
2010-06-29 6:04 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).