* [v2 Patch 1/2] s2io: add dynamic LRO disable support
@ 2010-06-09 10:05 Amerigo Wang
2010-06-09 10:05 ` [v2 Patch 2/2] mlx4: " Amerigo Wang
2010-06-09 14:00 ` [v2 Patch 1/2] s2io: " Ben Hutchings
0 siblings, 2 replies; 11+ messages in thread
From: Amerigo Wang @ 2010-06-09 10:05 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.
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..e558aa7 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,35 @@ 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) {
+ 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);
+ s2io_start_all_tx_queue(sp);
+ }
+
+ return rc;
+}
static const struct ethtool_ops netdev_ethtool_ops = {
.get_settings = s2io_ethtool_gset,
@@ -6701,6 +6729,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 +7259,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
* [v2 Patch 2/2] mlx4: add dynamic LRO disable support
2010-06-09 10:05 [v2 Patch 1/2] s2io: add dynamic LRO disable support Amerigo Wang
@ 2010-06-09 10:05 ` Amerigo Wang
2010-06-09 11:05 ` Stanislaw Gruszka
2010-06-09 14:00 ` [v2 Patch 1/2] s2io: " Ben Hutchings
1 sibling, 1 reply; 11+ messages in thread
From: Amerigo Wang @ 2010-06-09 10:05 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.
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..2c77805 100644
--- a/drivers/net/mlx4/en_ethtool.c
+++ b/drivers/net/mlx4/en_ethtool.c
@@ -387,6 +387,37 @@ 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) {
+ if (!(dev->features & NETIF_F_LRO)) {
+ dev->features |= NETIF_F_LRO;
+ changed = 1;
+ mdev->profile.num_lro = min_t(int, num_lro , MLX4_EN_MAX_LRO_DESCRIPTORS);
+ }
+ } else if (dev->features & NETIF_F_LRO) {
+ dev->features &= ~NETIF_F_LRO;
+ changed = 1;
+ mdev->profile.num_lro = 0;
+ }
+
+ if (changed && netif_running(dev)) {
+ mutex_lock(&mdev->state_lock);
+ mlx4_en_stop_port(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 +446,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,
};
diff --git a/drivers/net/mlx4/en_main.c b/drivers/net/mlx4/en_main.c
index cbabf14..4ef42e2 100644
--- a/drivers/net/mlx4/en_main.c
+++ b/drivers/net/mlx4/en_main.c
@@ -70,8 +70,9 @@ MLX4_EN_PARM_INT(rss_xor, 0, "Use XOR hash function for RSS");
MLX4_EN_PARM_INT(rss_mask, 0xf, "RSS hash type bitmask");
/* Number of LRO sessions per Rx ring (rounded up to a power of two) */
-MLX4_EN_PARM_INT(num_lro, MLX4_EN_MAX_LRO_DESCRIPTORS,
- "Number of LRO sessions per ring or disabled (0)");
+unsigned int num_lro = MLX4_EN_MAX_LRO_DESCRIPTORS;
+module_param(num_lro , uint, 0444);
+MODULE_PARM_DESC(num_lro, "Number of LRO sessions per ring or disabled (0)");
/* Priority pausing */
MLX4_EN_PARM_INT(pfctx, 0, "Priority based Flow Control policy on TX[7:0]."
diff --git a/drivers/net/mlx4/mlx4_en.h b/drivers/net/mlx4/mlx4_en.h
index b55e46c..bbac975 100644
--- a/drivers/net/mlx4/mlx4_en.h
+++ b/drivers/net/mlx4/mlx4_en.h
@@ -568,4 +568,6 @@ int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset);
* Globals
*/
extern const struct ethtool_ops mlx4_en_ethtool_ops;
+
+extern unsigned int num_lro;
#endif
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [v2 Patch 2/2] mlx4: add dynamic LRO disable support
2010-06-09 10:05 ` [v2 Patch 2/2] mlx4: " Amerigo Wang
@ 2010-06-09 11:05 ` Stanislaw Gruszka
2010-06-15 8:35 ` Cong Wang
0 siblings, 1 reply; 11+ messages in thread
From: Stanislaw Gruszka @ 2010-06-09 11:05 UTC (permalink / raw)
To: Amerigo Wang, David Miller
Cc: netdev, nhorman, herbert.xu, bhutchings, Ramkrishna.Vepa, davem
On Wed, Jun 09, 2010 at 06:05:34AM -0400, Amerigo Wang wrote:
> diff --git a/drivers/net/mlx4/en_ethtool.c b/drivers/net/mlx4/en_ethtool.c
> index d5afd03..2c77805 100644
> --- a/drivers/net/mlx4/en_ethtool.c
> +++ b/drivers/net/mlx4/en_ethtool.c
> @@ -387,6 +387,37 @@ 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) {
> + if (!(dev->features & NETIF_F_LRO)) {
> + dev->features |= NETIF_F_LRO;
> + changed = 1;
> + mdev->profile.num_lro = min_t(int, num_lro , MLX4_EN_MAX_LRO_DESCRIPTORS);
I do not understand why you override mdev->profile.num_lro in v2 patch.
If in Rx patch NETIF_F_LRO flag is used we do not need this IMHO.
> + }
> + } else if (dev->features & NETIF_F_LRO) {
> + dev->features &= ~NETIF_F_LRO;
> + changed = 1;
> + mdev->profile.num_lro = 0;
> + }
[snip]
> const struct ethtool_ops mlx4_en_ethtool_ops = {
> .get_drvinfo = mlx4_en_get_drvinfo,
> .get_settings = mlx4_en_get_settings,
> @@ -415,7 +446,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,
> };
Since we modify .set_flags, please assure we return -EOPNOTSUPP
if someone will try to setup ETH_FLAG_NTUPLE and ETH_FLAG_RXHASH.
BTW: seems default ethtool_op_set_flags introduce a bug on many
devices regarding ETH_FLAG_RXHASH. I think default should
be EOPNOTSUPP, and these few devices that actually support RXHASH
should have custom ethtool_ops->set_flags
Stanislaw
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2 Patch 2/2] mlx4: add dynamic LRO disable support
2010-06-09 11:05 ` Stanislaw Gruszka
@ 2010-06-15 8:35 ` Cong Wang
2010-06-15 10:14 ` Stanislaw Gruszka
0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2010-06-15 8:35 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: David Miller, netdev, nhorman, herbert.xu, bhutchings,
Ramkrishna.Vepa
On 06/09/10 19:05, Stanislaw Gruszka wrote:
> On Wed, Jun 09, 2010 at 06:05:34AM -0400, Amerigo Wang wrote:
>> diff --git a/drivers/net/mlx4/en_ethtool.c b/drivers/net/mlx4/en_ethtool.c
>> index d5afd03..2c77805 100644
>> --- a/drivers/net/mlx4/en_ethtool.c
>> +++ b/drivers/net/mlx4/en_ethtool.c
>> @@ -387,6 +387,37 @@ 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) {
>> + if (!(dev->features& NETIF_F_LRO)) {
>> + dev->features |= NETIF_F_LRO;
>> + changed = 1;
>> + mdev->profile.num_lro = min_t(int, num_lro , MLX4_EN_MAX_LRO_DESCRIPTORS);
>
> I do not understand why you override mdev->profile.num_lro in v2 patch.
> If in Rx patch NETIF_F_LRO flag is used we do not need this IMHO.
Oh, I thought you meant this in our previous disccussion. :-/
I will drop this line.
>
>> + }
>> + } else if (dev->features& NETIF_F_LRO) {
>> + dev->features&= ~NETIF_F_LRO;
>> + changed = 1;
>> + mdev->profile.num_lro = 0;
>> + }
> [snip]
>> const struct ethtool_ops mlx4_en_ethtool_ops = {
>> .get_drvinfo = mlx4_en_get_drvinfo,
>> .get_settings = mlx4_en_get_settings,
>> @@ -415,7 +446,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,
>> };
>
> Since we modify .set_flags, please assure we return -EOPNOTSUPP
> if someone will try to setup ETH_FLAG_NTUPLE and ETH_FLAG_RXHASH.
Yeah, good point!
>
> BTW: seems default ethtool_op_set_flags introduce a bug on many
> devices regarding ETH_FLAG_RXHASH. I think default should
> be EOPNOTSUPP, and these few devices that actually support RXHASH
> should have custom ethtool_ops->set_flags
Hmm, you mean this?
if (data & ETH_FLAG_RXHASH)
+ if (!ops->set_flags)
+ return -EOPNOTSUPP;
....
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2 Patch 2/2] mlx4: add dynamic LRO disable support
2010-06-15 8:35 ` Cong Wang
@ 2010-06-15 10:14 ` Stanislaw Gruszka
2010-06-17 10:48 ` Cong Wang
0 siblings, 1 reply; 11+ messages in thread
From: Stanislaw Gruszka @ 2010-06-15 10:14 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, netdev, nhorman, herbert.xu, bhutchings,
Ramkrishna.Vepa
On Tue, 15 Jun 2010 16:35:35 +0800
Cong Wang <amwang@redhat.com> wrote:
> > BTW: seems default ethtool_op_set_flags introduce a bug on many
> > devices regarding ETH_FLAG_RXHASH. I think default should
> > be EOPNOTSUPP, and these few devices that actually support RXHASH
> > should have custom ethtool_ops->set_flags
>
> Hmm, you mean this?
>
> if (data & ETH_FLAG_RXHASH)
> + if (!ops->set_flags)
> + return -EOPNOTSUPP;
> ....
Not really, but I do not have good idea how patch with fix should
looks.
I dislike fact that we setup ->feature that are not in real supported by
particular device instead of returning EOPNOTSUPP. This actually include
both flags NETIF_F_LRO and NETIF_F_RXHASH.
Perhaps ethtool_op_set_flags should be removed and drivers should use
only custom version. In particular seems e1000e and sfc use this
function improperly and should have NULL as .set_flags.
I will think more about that and maybe cook some patches.
Stanislaw
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2 Patch 2/2] mlx4: add dynamic LRO disable support
2010-06-15 10:14 ` Stanislaw Gruszka
@ 2010-06-17 10:48 ` Cong Wang
0 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2010-06-17 10:48 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: David Miller, netdev, nhorman, herbert.xu, bhutchings,
Ramkrishna.Vepa
On 06/15/10 18:14, Stanislaw Gruszka wrote:
> On Tue, 15 Jun 2010 16:35:35 +0800
> Cong Wang<amwang@redhat.com> wrote:
>>> BTW: seems default ethtool_op_set_flags introduce a bug on many
>>> devices regarding ETH_FLAG_RXHASH. I think default should
>>> be EOPNOTSUPP, and these few devices that actually support RXHASH
>>> should have custom ethtool_ops->set_flags
>>
>> Hmm, you mean this?
>>
>> if (data& ETH_FLAG_RXHASH)
>> + if (!ops->set_flags)
>> + return -EOPNOTSUPP;
>> ....
>
> Not really, but I do not have good idea how patch with fix should
> looks.
>
> I dislike fact that we setup ->feature that are not in real supported by
> particular device instead of returning EOPNOTSUPP. This actually include
> both flags NETIF_F_LRO and NETIF_F_RXHASH.
>
> Perhaps ethtool_op_set_flags should be removed and drivers should use
> only custom version. In particular seems e1000e and sfc use this
> function improperly and should have NULL as .set_flags.
This depends on if what ethtool_op_set_flags() does is common for
net drivers.
>
> I will think more about that and maybe cook some patches.
>
Yes, please. Cc me when you post patches.
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-09 10:05 [v2 Patch 1/2] s2io: add dynamic LRO disable support Amerigo Wang
2010-06-09 10:05 ` [v2 Patch 2/2] mlx4: " Amerigo Wang
@ 2010-06-09 14:00 ` Ben Hutchings
2010-06-09 18:52 ` Ramkrishna Vepa
1 sibling, 1 reply; 11+ messages in thread
From: Ben Hutchings @ 2010-06-09 14:00 UTC (permalink / raw)
To: Amerigo Wang
Cc: netdev, nhorman, sgruszka, herbert.xu, Ramkrishna.Vepa, davem
On Wed, 2010-06-09 at 06:05 -0400, Amerigo Wang wrote:
[...]
> +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) {
> + 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);
> + s2io_start_all_tx_queue(sp);
[...]
Is it safe to call s2io_start_all_tx_queue() if s2io_card_up() failed?
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: [v2 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-09 14:00 ` [v2 Patch 1/2] s2io: " Ben Hutchings
@ 2010-06-09 18:52 ` Ramkrishna Vepa
2010-06-15 8:26 ` Cong Wang
0 siblings, 1 reply; 11+ messages in thread
From: Ramkrishna Vepa @ 2010-06-09 18:52 UTC (permalink / raw)
To: Ben Hutchings, Amerigo Wang
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, davem@davemloft.net
> On Wed, 2010-06-09 at 06:05 -0400, Amerigo Wang wrote:
> [...]
> > +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) {
> > + 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);
> > + s2io_start_all_tx_queue(sp);
> [...]
>
> Is it safe to call s2io_start_all_tx_queue() if s2io_card_up() failed?
Ben,
Good point. If s2io_card_up() fails the chip will not be accessed, so it's safe but all transmit skbs will be freed without the user knowing the reason for failing to transmit or receive for that matter. The other option is to return with a failure and get the watchdog timer reset the adapter.
Ram
>
> 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.
The information and any attached documents contained in this message
may be confidential and/or legally privileged. The message is
intended solely for the addressee(s). If you are not the intended
recipient, you are hereby notified that any use, dissemination, or
reproduction is strictly prohibited and may be unlawful. If you are
not the intended recipient, please contact the sender immediately by
return e-mail and destroy all copies of the original message.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-09 18:52 ` Ramkrishna Vepa
@ 2010-06-15 8:26 ` Cong Wang
2010-06-15 16:07 ` Ramkrishna Vepa
0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2010-06-15 8:26 UTC (permalink / raw)
To: Ramkrishna Vepa
Cc: Ben Hutchings, netdev@vger.kernel.org, nhorman@redhat.com,
sgruszka@redhat.com, herbert.xu@redhat.com, davem@davemloft.net
On 06/10/10 02:52, Ramkrishna Vepa wrote:
>> On Wed, 2010-06-09 at 06:05 -0400, Amerigo Wang wrote:
>> [...]
>>> +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) {
>>> + 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);
>>> + s2io_start_all_tx_queue(sp);
>> [...]
>>
>> Is it safe to call s2io_start_all_tx_queue() if s2io_card_up() failed?
> Ben,
> Good point. If s2io_card_up() fails the chip will not be accessed, so it's safe but all transmit skbs will be freed without the user knowing the reason for failing to transmit or receive for that matter. The other option is to return with a failure and get the watchdog timer reset the adapter.
>
(Sorry for the delay, I was on vacation.)
So it seems the latter option is better?
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [v2 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-15 8:26 ` Cong Wang
@ 2010-06-15 16:07 ` Ramkrishna Vepa
2010-06-15 17:21 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Ramkrishna Vepa @ 2010-06-15 16:07 UTC (permalink / raw)
To: Cong Wang
Cc: Ben Hutchings, netdev@vger.kernel.org, nhorman@redhat.com,
sgruszka@redhat.com, herbert.xu@redhat.com, davem@davemloft.net
> >> On Wed, 2010-06-09 at 06:05 -0400, Amerigo Wang wrote:
> >> [...]
> >>> +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) {
> >>> + 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);
> >>> + s2io_start_all_tx_queue(sp);
> >> [...]
> >>
> >> Is it safe to call s2io_start_all_tx_queue() if s2io_card_up() failed?
> > Ben,
> > Good point. If s2io_card_up() fails the chip will not be accessed, so
> it's safe but all transmit skbs will be freed without the user knowing the
> reason for failing to transmit or receive for that matter. The other
> option is to return with a failure and get the watchdog timer reset the
> adapter.
> >
>
> (Sorry for the delay, I was on vacation.)
>
> So it seems the latter option is better?
Yes, this will work.
Thanks,
Ram
The information and any attached documents contained in this message
may be confidential and/or legally privileged. The message is
intended solely for the addressee(s). If you are not the intended
recipient, you are hereby notified that any use, dissemination, or
reproduction is strictly prohibited and may be unlawful. If you are
not the intended recipient, please contact the sender immediately by
return e-mail and destroy all copies of the original message.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v2 Patch 1/2] s2io: add dynamic LRO disable support
2010-06-15 16:07 ` Ramkrishna Vepa
@ 2010-06-15 17:21 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2010-06-15 17:21 UTC (permalink / raw)
To: Ramkrishna.Vepa; +Cc: amwang, bhutchings, netdev, nhorman, sgruszka, herbert.xu
From: Ramkrishna Vepa <Ramkrishna.Vepa@exar.com>
Date: Tue, 15 Jun 2010 09:07:47 -0700
> Yes, this will work.
>
> Thanks,
> Ram
>
> The information and any attached documents contained in this message
> may be confidential and/or legally privileged. The message is
> intended solely for the addressee(s). If you are not the intended
> recipient, you are hereby notified that any use, dissemination, or
> reproduction is strictly prohibited and may be unlawful. If you are
> not the intended recipient, please contact the sender immediately by
> return e-mail and destroy all copies of the original message.
Please, your legal notice is 10 times larger than what you're actually
saying.
That is rediculious. Even more rediculious is using such a notice for
a posting to a public mailing list that is published and duplicated
hundreds and hundreds of times to public archive sites and elsewhere.
Your notice has no meaning in this context, so not only is it wasteful
and annoying, it's useless.
Please us a gmail or yahoo email account to post to these mailing lists
if you can't avoid attaching this legal notice to your postings here.
It's wasting bandwidth and people's time unnecessarily.
Thank you.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-06-17 10:44 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-09 10:05 [v2 Patch 1/2] s2io: add dynamic LRO disable support Amerigo Wang
2010-06-09 10:05 ` [v2 Patch 2/2] mlx4: " Amerigo Wang
2010-06-09 11:05 ` Stanislaw Gruszka
2010-06-15 8:35 ` Cong Wang
2010-06-15 10:14 ` Stanislaw Gruszka
2010-06-17 10:48 ` Cong Wang
2010-06-09 14:00 ` [v2 Patch 1/2] s2io: " Ben Hutchings
2010-06-09 18:52 ` Ramkrishna Vepa
2010-06-15 8:26 ` Cong Wang
2010-06-15 16:07 ` Ramkrishna Vepa
2010-06-15 17:21 ` 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).