diff mbox

[net-next] netxen: write IP address to firmware when using bonding

Message ID 1322073791-15223-1-git-send-email-andy@greyhouse.net
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Andy Gospodarek Nov. 23, 2011, 6:43 p.m. UTC
The following patch was added to enable NX3031 devices to properly
aggregate frames for LRO:

	commit 6598b169b856793f8f9b80a3f3c5a48f5eaf40e3
	Author: Dhananjay Phadke <dhananjay@netxen.com>
	Date:   Sun Jul 26 20:07:37 2009 +0000

	    netxen: enable ip addr hashing

This patch is a followup to that fix as it allows LRO aggregation on
bonded devices that contain an NX3031 device.  This was tested on an
older distro and modified slightly to the latest upstream.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
---
 .../net/ethernet/qlogic/netxen/netxen_nic_main.c   |  119 +++++++++++++++-----
 1 files changed, 92 insertions(+), 27 deletions(-)

Comments

David Miller Nov. 23, 2011, 9:09 p.m. UTC | #1
From: Andy Gospodarek <andy@greyhouse.net>
Date: Wed, 23 Nov 2011 13:43:11 -0500

> The following patch was added to enable NX3031 devices to properly
> aggregate frames for LRO:
> 
> 	commit 6598b169b856793f8f9b80a3f3c5a48f5eaf40e3
> 	Author: Dhananjay Phadke <dhananjay@netxen.com>
> 	Date:   Sun Jul 26 20:07:37 2009 +0000
> 
> 	    netxen: enable ip addr hashing
> 
> This patch is a followup to that fix as it allows LRO aggregation on
> bonded devices that contain an NX3031 device.  This was tested on an
> older distro and modified slightly to the latest upstream.
> 
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>

Let's put a helper function somewhere instead of expanding this sequence
in every driver that might want to determine if it is a bonding slave.
--
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
Andy Gospodarek Nov. 24, 2011, 3:24 a.m. UTC | #2
On Wed, Nov 23, 2011 at 04:09:22PM -0500, David Miller wrote:
> From: Andy Gospodarek <andy@greyhouse.net>
> Date: Wed, 23 Nov 2011 13:43:11 -0500
> 
> > The following patch was added to enable NX3031 devices to properly
> > aggregate frames for LRO:
> > 
> > 	commit 6598b169b856793f8f9b80a3f3c5a48f5eaf40e3
> > 	Author: Dhananjay Phadke <dhananjay@netxen.com>
> > 	Date:   Sun Jul 26 20:07:37 2009 +0000
> > 
> > 	    netxen: enable ip addr hashing
> > 
> > This patch is a followup to that fix as it allows LRO aggregation on
> > bonded devices that contain an NX3031 device.  This was tested on an
> > older distro and modified slightly to the latest upstream.
> > 
> > Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
> 
> Let's put a helper function somewhere instead of expanding this sequence
> in every driver that might want to determine if it is a bonding slave.

Dave,

Are you talking about adding a macro like this:

	for_each_dev_in_bond(bond,slave) {
		[...]
	}

to replace the statements I added that were like this:

	for_each_netdev_rcu(&init_net, slave) {
		if (slave->master == dev) {
			[...]
		}
	}

If so, that totally seems reasonable.  If were requesting something
else, please let me know.

Thanks,

-andy

--
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 Nov. 24, 2011, 5:38 a.m. UTC | #3
From: Andy Gospodarek <andy@greyhouse.net>
Date: Wed, 23 Nov 2011 22:24:27 -0500

> Are you talking about adding a macro like this:
> 
> 	for_each_dev_in_bond(bond,slave) {
> 		[...]
> 	}
> 
> to replace the statements I added that were like this:
> 
> 	for_each_netdev_rcu(&init_net, slave) {
> 		if (slave->master == dev) {
> 			[...]
> 		}
> 	}
> 
> If so, that totally seems reasonable.  If were requesting something
> else, please let me know.

Yes, some helper that walks the device list and tries to find
a device whose ->master matches 'dev'.
--
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
Rajesh Borundia Nov. 28, 2011, 6:02 a.m. UTC | #4
Hi Andy,

We need to restore the ip address after the adapter reset.
netxen_restore_indev_addr is the function that restores normal ip addresses
and vlan ip addresses. If we could find bond device directly from slave then 
we can use netxen_config_indev_addr to add the ip address of the bond device.
Otherwise we may need to cache the bond ip address in function netxen_list_config_vlan_ip
and change the condition from

if (!is_vlan_dev(dev))
                return;

to

if (!is_vlan_dev(dev) && !is_bond_dev(dev))
                return;


Some of the code in if and else part is repeated.
If possible can we have small functions for that ?
eg:
if (!is_netxen_netdev(dev))
+                       goto done;
+
+               adapter = netdev_priv(dev);
+               if (!adapter)
+                       goto done;
+
+               if (adapter->is_up != NETXEN_ADAPTER_UP_MAGIC)
+                       goto done;
+
+               netxen_config_indev_addr(adapter, orig_dev, event);


Rajesh
Andy Gospodarek Nov. 28, 2011, 8:43 p.m. UTC | #5
On Mon, Nov 28, 2011 at 12:02:28AM -0600, Rajesh Borundia wrote:
> Hi Andy,
> 
> We need to restore the ip address after the adapter reset.
> netxen_restore_indev_addr is the function that restores normal ip addresses
> and vlan ip addresses. If we could find bond device directly from slave then 
> we can use netxen_config_indev_addr to add the ip address of the bond device.
> Otherwise we may need to cache the bond ip address in function netxen_list_config_vlan_ip
> and change the condition from
> 
> if (!is_vlan_dev(dev))
>                 return;
> 
> to
> 
> if (!is_vlan_dev(dev) && !is_bond_dev(dev))
>                 return;
> 

I don't think this was what you were originally intending, but what
about changing the name of the vlan_ip_list (and the vlan functions that
modify/read it) to something different and use it for all virtual or
stacked devices?

Something like 'virtual' or 'virt' might work instead of 'vlan.'
Something else that denotes that a device at a higher layer than just
the physical device has an IP address that we need to know about, but
that the device is not specific to VLANs.

> Some of the code in if and else part is repeated.
> If possible can we have small functions for that ?
> eg:
> if (!is_netxen_netdev(dev))
> +                       goto done;
> +
> +               adapter = netdev_priv(dev);
> +               if (!adapter)
> +                       goto done;
> +
> +               if (adapter->is_up != NETXEN_ADAPTER_UP_MAGIC)
> +                       goto done;
> +
> +               netxen_config_indev_addr(adapter, orig_dev, event);
> 
> 

I could change that.  I started to look at sharing some code, but
decided against too much initial code churn.  Since the fundamental idea
behind this chance seems acceptable, I can take a look at it more
closely.

I'll take a look at this and send a v2 of this patch.


> Rajesh
> ________________________________________
> From: David Miller [davem@davemloft.net]
> Sent: Thursday, November 24, 2011 11:08 AM
> To: andy@greyhouse.net
> Cc: netdev; Sony Chacko; Rajesh Borundia
> Subject: Re: [PATCH net-next] netxen: write IP address to firmware when using bonding
> 
> From: Andy Gospodarek <andy@greyhouse.net>
> Date: Wed, 23 Nov 2011 22:24:27 -0500
> 
> > Are you talking about adding a macro like this:
> >
> >       for_each_dev_in_bond(bond,slave) {
> >               [...]
> >       }
> >
> > to replace the statements I added that were like this:
> >
> >       for_each_netdev_rcu(&init_net, slave) {
> >               if (slave->master == dev) {
> >                       [...]
> >               }
> >       }
> >
> > If so, that totally seems reasonable.  If were requesting something
> > else, please let me know.
> 
> Yes, some helper that walks the device list and tries to find
> a device whose ->master matches 'dev'.
> 
> 
> --
> 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
--
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
Rajesh Borundia Nov. 29, 2011, 11:58 a.m. UTC | #6
Yes virt would be fine.

Thanks,
Rajesh
diff mbox

Patch

diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
index 7dd9a4b..64eb618 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -3038,18 +3038,45 @@  recheck:
 		goto recheck;
 	}
 
-	if (!is_netxen_netdev(dev))
-		goto done;
+	/* If this is a bonding device, look for netxen-based slaves*/
+	if (dev->priv_flags & IFF_BONDING) {
+		struct net_device *slave;
 
-	adapter = netdev_priv(dev);
+		rcu_read_lock();
+		for_each_netdev_rcu(&init_net, slave) {
+			/* check to see if the device is in the bond */
+			if (slave->master == dev) {
 
-	if (!adapter)
-		goto done;
+				if (!is_netxen_netdev(slave))
+					continue;
 
-	if (adapter->is_up != NETXEN_ADAPTER_UP_MAGIC)
-		goto done;
+				adapter = netdev_priv(slave);
+
+				if (!adapter)
+					continue;
+
+				if (adapter->is_up != NETXEN_ADAPTER_UP_MAGIC)
+					continue;
+
+				netxen_config_indev_addr(adapter, orig_dev, event);
+			}
+		}
+		rcu_read_unlock();
+
+	} else {
+		if (!is_netxen_netdev(dev))
+			goto done;
+
+		adapter = netdev_priv(dev);
 
-	netxen_config_indev_addr(adapter, orig_dev, event);
+		if (!adapter)
+			goto done;
+
+		if (adapter->is_up != NETXEN_ADAPTER_UP_MAGIC)
+			goto done;
+
+		netxen_config_indev_addr(adapter, orig_dev, event);
+	}
 done:
 	return NOTIFY_DONE;
 }
@@ -3074,30 +3101,68 @@  recheck:
 		goto recheck;
 	}
 
-	if (!is_netxen_netdev(dev))
-		goto done;
+	/* If this is a bonding device, look for netxen-based slaves*/
+	if (dev->priv_flags & IFF_BONDING) {
+		struct net_device *slave;
 
-	adapter = netdev_priv(dev);
+		rcu_read_lock();
+		for_each_netdev_rcu(&init_net, slave) {
+			/* check to see if the device is in the bond */
+			if (slave->master == dev) {
 
-	if (!adapter || !netxen_destip_supported(adapter))
-		goto done;
+				if (!is_netxen_netdev(slave))
+					continue;
 
-	if (adapter->is_up != NETXEN_ADAPTER_UP_MAGIC)
-		goto done;
+				adapter = netdev_priv(slave);
 
-	switch (event) {
-	case NETDEV_UP:
-		netxen_config_ipaddr(adapter, ifa->ifa_address, NX_IP_UP);
-		netxen_list_config_vlan_ip(adapter, ifa, NX_IP_UP);
-		break;
-	case NETDEV_DOWN:
-		netxen_config_ipaddr(adapter, ifa->ifa_address, NX_IP_DOWN);
-		netxen_list_config_vlan_ip(adapter, ifa, NX_IP_DOWN);
-		break;
-	default:
-		break;
-	}
+				if (!adapter || !netxen_destip_supported(adapter))
+					continue;
+
+				if (adapter->is_up != NETXEN_ADAPTER_UP_MAGIC)
+					continue;
+
+				switch (event) {
+				case NETDEV_UP:
+					netxen_config_ipaddr(adapter, ifa->ifa_address, NX_IP_UP);
+					netxen_list_config_vlan_ip(adapter, ifa, NX_IP_UP);
+					break;
+				case NETDEV_DOWN:
+					netxen_config_ipaddr(adapter, ifa->ifa_address, NX_IP_DOWN);
+					netxen_list_config_vlan_ip(adapter, ifa, NX_IP_DOWN);
+					break;
+				default:
+					break;
+				}
+			}
+		}
+		rcu_read_unlock();
+
+	} else {
+
+		if (!is_netxen_netdev(dev))
+			goto done;
+
+		adapter = netdev_priv(dev);
+
+		if (!adapter || !netxen_destip_supported(adapter))
+			goto done;
 
+		if (adapter->is_up != NETXEN_ADAPTER_UP_MAGIC)
+			goto done;
+
+		switch (event) {
+		case NETDEV_UP:
+			netxen_config_ipaddr(adapter, ifa->ifa_address, NX_IP_UP);
+			netxen_list_config_vlan_ip(adapter, ifa, NX_IP_UP);
+			break;
+		case NETDEV_DOWN:
+			netxen_config_ipaddr(adapter, ifa->ifa_address, NX_IP_DOWN);
+			netxen_list_config_vlan_ip(adapter, ifa, NX_IP_DOWN);
+			break;
+		default:
+			break;
+		}
+	}
 done:
 	return NOTIFY_DONE;
 }