diff mbox

[v2,1/2] s2io: add dynamic LRO disable support

Message ID 20100609100928.6573.14199.sendpatchset@localhost.localdomain
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Amerigo Wang June 9, 2010, 10:05 a.m. UTC 
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>

---

--
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

Comments

Ben Hutchings June 9, 2010, 2 p.m. UTC | #1 
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.
Ramkrishna Vepa June 9, 2010, 6:52 p.m. UTC | #2 
> 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.
--
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
Amerigo Wang June 15, 2010, 8:26 a.m. UTC | #3 
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.
--
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
Ramkrishna Vepa June 15, 2010, 4:07 p.m. UTC | #4 
> >> 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.
--
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
David Miller June 15, 2010, 5:21 p.m. UTC | #5 
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.
--
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 mbox

Patch

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",