bond_compute_features() does not handle devices with different CSUM

8 messages, 3 authors, 2007-05-30 · open the first message on its own page

bond_compute_features() does not handle devices with different CSUM

From: Laurent Chavey <hidden>
Date: 2007-05-30 00:41:59 

kernel version <= 2.6.20.1
file drivers/net/bonding/bonding_main.c
function bond_compute_features()

-----------
  Given a system with two different NIC. One driver sets
        dev->features |=  NETIF_F_HW_CSUM
  the other driver sets
        dev->features |=  NETIF_F_IP_CSUM

  when enslaving the 2 device above, bond_compute_features()
  does not set the intersection of the 2 CSUM
  features (should be NETIF_F_IP_CSUM)
-----------

   should the bond features in the case above include NETIF_F_IP_CSUM

Re: bond_compute_features() does not handle devices with different CSUM

From: Laurent Chavey <hidden>
Date: 2007-05-30 02:47:14 

proposed change.

--- 2.6.20/drivers/net/bonding/bond_main.c      2007-05-29
19:43:39.010565000 -0700
+++ 2.6.20.fix/drivers/net/bonding/bond_main.c  2007-05-29
19:46:12.376980000 -0700
@@ -1227,7 +1227,14 @@
        int i;

        bond_for_each_slave(bond, slave, i) {
-               features &= (slave->dev->features & BOND_INTERSECT_FEATURES);
+               /* NETIF_F_HW_CSUM includes support for NET_IF_IP_CSUM
+                * as such when looking for the intersection we need to
+                * add it to the device supported features
+                */
+               unsigned long dev_features = slave->dev->features;
+               if (slave->dev->features & NETIF_F_HW_CSUM)
+                       dev_features |= NETIF_F_IP_CSUM;
+               features &= (features & BOND_INTERSECT_FEATURES);
                if (slave->dev->hard_header_len > max_hard_header_len)
                        max_hard_header_len = slave->dev->hard_header_len;
        }



On 5/29/07, Laurent Chavey [off-list ref] wrote:
kernel version <= 2.6.20.1
file drivers/net/bonding/bonding_main.c
function bond_compute_features()

-----------
  Given a system with two different NIC. One driver sets
        dev->features |=  NETIF_F_HW_CSUM
  the other driver sets
        dev->features |=  NETIF_F_IP_CSUM

  when enslaving the 2 device above, bond_compute_features()
  does not set the intersection of the 2 CSUM
  features (should be NETIF_F_IP_CSUM)
-----------

   should the bond features in the case above include NETIF_F_IP_CSUM

Re: bond_compute_features() does not handle devices with different CSUM

From: Michael Buesch <hidden>
Date: 2007-05-30 11:03:59 

On Wednesday 30 May 2007 04:47:09 Laurent Chavey wrote:
quoted hunk
proposed change.

--- 2.6.20/drivers/net/bonding/bond_main.c      2007-05-29
19:43:39.010565000 -0700
+++ 2.6.20.fix/drivers/net/bonding/bond_main.c  2007-05-29
19:46:12.376980000 -0700
@@ -1227,7 +1227,14 @@
        int i;

        bond_for_each_slave(bond, slave, i) {
-               features &= (slave->dev->features & BOND_INTERSECT_FEATURES);
+               /* NETIF_F_HW_CSUM includes support for NET_IF_IP_CSUM
+                * as such when looking for the intersection we need to
+                * add it to the device supported features
+                */
+               unsigned long dev_features = slave->dev->features;
+               if (slave->dev->features & NETIF_F_HW_CSUM)
+                       dev_features |= NETIF_F_IP_CSUM;
+               features &= (features & BOND_INTERSECT_FEATURES);
Is this statement correct?
It's the same as
	features = features & (features & BOND_INTERSECT_FEATURES);
which looks strange to me.
Did you mean something like
	features |= (dev_features & BOND_INTERSECT_FEATURES);
?

-- 
Greetings Michael.





  

    
Re: bond_compute_features() does not handle devices with different CSUM

    

      From: Laurent Chavey <hidden>

      Date: 2007-05-30 15:16:22 
    

  

  
features = features & (dev_features & BOND_INTERSECT_FEATURES);

On 5/30/07, Michael Buesch [off-list ref] wrote:
On Wednesday 30 May 2007 04:47:09 Laurent Chavey wrote:
quoted
proposed change.

--- 2.6.20/drivers/net/bonding/bond_main.c      2007-05-29
19:43:39.010565000 -0700
+++ 2.6.20.fix/drivers/net/bonding/bond_main.c  2007-05-29
19:46:12.376980000 -0700
@@ -1227,7 +1227,14 @@
        int i;

        bond_for_each_slave(bond, slave, i) {
-               features &= (slave->dev->features & BOND_INTERSECT_FEATURES);
+               /* NETIF_F_HW_CSUM includes support for NET_IF_IP_CSUM
+                * as such when looking for the intersection we need to
+                * add it to the device supported features
+                */
+               unsigned long dev_features = slave->dev->features;
+               if (slave->dev->features & NETIF_F_HW_CSUM)
+                       dev_features |= NETIF_F_IP_CSUM;
quoted
+               features &= (features & BOND_INTERSECT_FEATURES);
Is this statement correct?
It's the same as
        features = features & (features & BOND_INTERSECT_FEATURES);
which looks strange to me.
Did you mean something like
        features |= (dev_features & BOND_INTERSECT_FEATURES);
?

--
Greetings Michael.





  

    
Re: bond_compute_features() does not handle devices with different CSUM

    

      From: Laurent Chavey <hidden>

      Date: 2007-05-30 15:19:03 
    

  

  
corrected diff.

--- 2.6.20/drivers/net/bonding/bond_main.c      2007-05-29
19:43:39.010565000 -0700
+++ 2.6.20.fix/drivers/net/bonding/bond_main.c  2007-05-30
08:18:24.018322000 -0700
@@ -1227,7 +1227,14 @@
        int i;

        bond_for_each_slave(bond, slave, i) {
-               features &= (slave->dev->features & BOND_INTERSECT_FEATURES);
+               /* NETIF_F_HW_CSUM includes support for NET_IF_IP_CSUM
+                * as such when looking for the intersection we need to
+                * add it to the device supported features
+                */
+               unsigned long dev_features = slave->dev->features;
+               if (slave->dev->features & NETIF_F_HW_CSUM)
+                       dev_features |= NETIF_F_IP_CSUM;
+               features &= (dev_features & BOND_INTERSECT_FEATURES);
                if (slave->dev->hard_header_len > max_hard_header_len)
                        max_hard_header_len = slave->dev->hard_header_len;
        }

On 5/30/07, Laurent Chavey [off-list ref] wrote:
features = features & (dev_features & BOND_INTERSECT_FEATURES);

On 5/30/07, Michael Buesch [off-list ref] wrote:
quoted
On Wednesday 30 May 2007 04:47:09 Laurent Chavey wrote:
quoted
proposed change.

--- 2.6.20/drivers/net/bonding/bond_main.c      2007-05-29
19:43:39.010565000 -0700
+++ 2.6.20.fix/drivers/net/bonding/bond_main.c  2007-05-29
19:46:12.376980000 -0700
@@ -1227,7 +1227,14 @@
        int i;

        bond_for_each_slave(bond, slave, i) {
-               features &= (slave->dev->features & BOND_INTERSECT_FEATURES);
+               /* NETIF_F_HW_CSUM includes support for NET_IF_IP_CSUM
+                * as such when looking for the intersection we need to
+                * add it to the device supported features
+                */
+               unsigned long dev_features = slave->dev->features;
+               if (slave->dev->features & NETIF_F_HW_CSUM)
+                       dev_features |= NETIF_F_IP_CSUM;
quoted
+               features &= (features & BOND_INTERSECT_FEATURES);
Is this statement correct?
It's the same as
        features = features & (features & BOND_INTERSECT_FEATURES);
which looks strange to me.
Did you mean something like
        features |= (dev_features & BOND_INTERSECT_FEATURES);
?

--
Greetings Michael.





  

    
Re: bond_compute_features() does not handle devices with different CSUM

    

      From: Stephen Hemminger <hidden>

      Date: 2007-05-30 16:39:20 
    

  

  
On Tue, 29 May 2007 17:41:45 -0700
"Laurent Chavey" [off-list ref] wrote:

kernel version <= 2.6.20.1
file drivers/net/bonding/bonding_main.c
function bond_compute_features()

-----------
  Given a system with two different NIC. One driver sets
        dev->features |=  NETIF_F_HW_CSUM
  the other driver sets
        dev->features |=  NETIF_F_IP_CSUM

  when enslaving the 2 device above, bond_compute_features()
  does not set the intersection of the 2 CSUM
  features (should be NETIF_F_IP_CSUM)
-----------

   should the bond features in the case above include NETIF_F_IP_CSUM
-
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
The bridge code already has a more complete/complex recompute
routine to handle this.

-- 
Stephen Hemminger [off-list ref]





  

    
Re: bond_compute_features() does not handle devices with different CSUM

    

      From: Laurent Chavey <hidden>

      Date: 2007-05-30 16:58:51 
    

  

  
should we then move/integrate that code as part of the bonding driver ?


On 5/30/07, Stephen Hemminger [off-list ref] wrote:
On Tue, 29 May 2007 17:41:45 -0700
"Laurent Chavey" [off-list ref] wrote:

quoted
kernel version <= 2.6.20.1
file drivers/net/bonding/bonding_main.c
function bond_compute_features()

-----------
  Given a system with two different NIC. One driver sets
        dev->features |=  NETIF_F_HW_CSUM
  the other driver sets
        dev->features |=  NETIF_F_IP_CSUM

  when enslaving the 2 device above, bond_compute_features()
  does not set the intersection of the 2 CSUM
  features (should be NETIF_F_IP_CSUM)
-----------

   should the bond features in the case above include NETIF_F_IP_CSUM
-
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
The bridge code already has a more complete/complex recompute
routine to handle this.

--
Stephen Hemminger [off-list ref]





  

    
Re: bond_compute_features() does not handle devices with different CSUM

    

      From: Stephen Hemminger <hidden>

      Date: 2007-05-30 17:30:30 
    

  

  
On Wed, 30 May 2007 09:58:38 -0700
"Laurent Chavey" [off-list ref] wrote:

should we then move/integrate that code as part of the bonding driver ?


On 5/30/07, Stephen Hemminger [off-list ref] wrote:
quoted
On Tue, 29 May 2007 17:41:45 -0700
"Laurent Chavey" [off-list ref] wrote:

quoted
kernel version <= 2.6.20.1
file drivers/net/bonding/bonding_main.c
function bond_compute_features()

-----------
  Given a system with two different NIC. One driver sets
        dev->features |=  NETIF_F_HW_CSUM
  the other driver sets
        dev->features |=  NETIF_F_IP_CSUM

  when enslaving the 2 device above, bond_compute_features()
  does not set the intersection of the 2 CSUM
  features (should be NETIF_F_IP_CSUM)
-----------

   should the bond features in the case above include NETIF_F_IP_CSUM
-
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
The bridge code already has a more complete/complex recompute
routine to handle this.

--
Stephen Hemminger [off-list ref]
Sure, that would be easier for maintenance.

-- 
Stephen Hemminger [off-list ref]




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help