[PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Catalin(ux aka Dino) BOIE]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 394 bytes --]

Hello!

The attached patch, makes the bridge to select features (almost all) only 
if _all_ devices supports them.
The patch was tested (create a bridge, add an interface, remove the 
interface and always check features flags).

What do you think?

Signed-off-by: Catalin(ux aka Dino) BOIE <catab at umbrella.ro>
---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2195 bytes --]

--- bridge1/net/bridge/br_private.h	2005-03-02 09:37:50.000000000 +0200
+++ linux/net/bridge/br_private.h	2005-05-19 18:00:05.000000000 +0300
@@ -27,6 +27,10 @@
 #define BR_PORT_BITS	10
 #define BR_MAX_PORTS	(1<<BR_PORT_BITS)
 
+#define BR_FEAT_MASK	(NETIF_F_HW_CSUM | NETIF_F_SG \
+			| NETIF_F_FRAGLIST | NETIF_F_IP_CSUM \
+			| NETIF_F_HIGHDMA | NETIF_F_TSO)
+
 typedef struct bridge_id bridge_id;
 typedef struct mac_addr mac_addr;
 typedef __u16 port_id;
--- bridge1/net/bridge/br_device.c	2005-03-02 09:37:30.000000000 +0200
+++ linux/net/bridge/br_device.c	2005-05-19 17:14:21.000000000 +0300
@@ -107,4 +107,5 @@ void br_dev_setup(struct net_device *dev
 	dev->tx_queue_len = 0;
 	dev->set_mac_address = NULL;
 	dev->priv_flags = IFF_EBRIDGE;
+	dev->features = BR_FEAT_MASK;
 }
--- bridge1/net/bridge/br_if.c	2005-03-02 09:38:33.000000000 +0200
+++ linux/net/bridge/br_if.c	2005-05-19 18:21:39.000000000 +0300
@@ -314,6 +314,27 @@ int br_min_mtu(const struct net_bridge *
 	return mtu;
 }
 
+/*
+ * If slave device (@dev) doesn't support special features,
+ * turn them off globally.
+ */
+static inline void br_features_change(struct net_bridge *br, struct net_device *dev)
+{
+	br->dev->features &= dev->features | ~BR_FEAT_MASK;
+}
+
+/*
+ * Recomputes features using slave's features
+ */
+static void br_features_recompute(struct net_bridge *br)
+{
+	struct net_bridge_port *p;
+
+	br->dev->features |= BR_FEAT_MASK;
+	list_for_each_entry(p, &br->port_list, list)
+		br_features_change(br, p->dev);
+}
+
 /* called with RTNL */
 int br_add_if(struct net_bridge *br, struct net_device *dev)
 {
@@ -332,9 +353,10 @@ int br_add_if(struct net_bridge *br, str
 	if (IS_ERR(p = new_nbp(br, dev, br_initial_port_cost(dev))))
 		return PTR_ERR(p);
 
+	br_features_change(br, dev);
+
  	if ((err = br_fdb_insert(br, p, dev->dev_addr, 1)))
 		destroy_nbp(p);
- 
 	else if ((err = br_sysfs_addif(p)))
 		del_nbp(p);
 	else {
@@ -368,6 +390,7 @@ int br_del_if(struct net_bridge *br, str
 
 	spin_lock_bh(&br->lock);
 	br_stp_recalculate_bridge_id(br);
+	br_features_recompute(br);
 	spin_unlock_bh(&br->lock);
 
 	return 0;

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Nivedita Singhvi]

Catalin(ux aka Dino) BOIE wrote:

> Hello!
> 
> The attached patch, makes the bridge to select features (almost all) 
> only if _all_ devices supports them.
> The patch was tested (create a bridge, add an interface, remove the 
> interface and always check features flags).
> 
> What do you think?

While this is generally a cleaner thing to do - it breaks
what we need - Jon's  patch adds the checksum offload/SG
unconditionally precisely because we don't want it to be
an all or nothing - we'd like to utilize offload on those
devices that do support it..

thanks,
Nivedita


> ------------------------------------------------------------------------
> 
> --- bridge1/net/bridge/br_private.h	2005-03-02 09:37:50.000000000 +0200
> +++ linux/net/bridge/br_private.h	2005-05-19 18:00:05.000000000 +0300
> @@ -27,6 +27,10 @@
>  #define BR_PORT_BITS	10
>  #define BR_MAX_PORTS	(1<<BR_PORT_BITS)
>  
> +#define BR_FEAT_MASK	(NETIF_F_HW_CSUM | NETIF_F_SG \
> +			| NETIF_F_FRAGLIST | NETIF_F_IP_CSUM \
> +			| NETIF_F_HIGHDMA | NETIF_F_TSO)
> +
>  typedef struct bridge_id bridge_id;
>  typedef struct mac_addr mac_addr;
>  typedef __u16 port_id;
> --- bridge1/net/bridge/br_device.c	2005-03-02 09:37:30.000000000 +0200
> +++ linux/net/bridge/br_device.c	2005-05-19 17:14:21.000000000 +0300
> @@ -107,4 +107,5 @@ void br_dev_setup(struct net_device *dev
>  	dev->tx_queue_len = 0;
>  	dev->set_mac_address = NULL;
>  	dev->priv_flags = IFF_EBRIDGE;
> +	dev->features = BR_FEAT_MASK;
>  }
> --- bridge1/net/bridge/br_if.c	2005-03-02 09:38:33.000000000 +0200
> +++ linux/net/bridge/br_if.c	2005-05-19 18:21:39.000000000 +0300
> @@ -314,6 +314,27 @@ int br_min_mtu(const struct net_bridge *
>  	return mtu;
>  }
>  
> +/*
> + * If slave device (@dev) doesn't support special features,
> + * turn them off globally.
> + */
> +static inline void br_features_change(struct net_bridge *br, struct net_device *dev)
> +{
> +	br->dev->features &= dev->features | ~BR_FEAT_MASK;
> +}
> +
> +/*
> + * Recomputes features using slave's features
> + */
> +static void br_features_recompute(struct net_bridge *br)
> +{
> +	struct net_bridge_port *p;
> +
> +	br->dev->features |= BR_FEAT_MASK;
> +	list_for_each_entry(p, &br->port_list, list)
> +		br_features_change(br, p->dev);
> +}
> +
>  /* called with RTNL */
>  int br_add_if(struct net_bridge *br, struct net_device *dev)
>  {
> @@ -332,9 +353,10 @@ int br_add_if(struct net_bridge *br, str
>  	if (IS_ERR(p = new_nbp(br, dev, br_initial_port_cost(dev))))
>  		return PTR_ERR(p);
>  
> +	br_features_change(br, dev);
> +
>   	if ((err = br_fdb_insert(br, p, dev->dev_addr, 1)))
>  		destroy_nbp(p);
> - 
>  	else if ((err = br_sysfs_addif(p)))
>  		del_nbp(p);
>  	else {
> @@ -368,6 +390,7 @@ int br_del_if(struct net_bridge *br, str
>  
>  	spin_lock_bh(&br->lock);
>  	br_stp_recalculate_bridge_id(br);
> +	br_features_recompute(br);
>  	spin_unlock_bh(&br->lock);
>  
>  	return 0;

Re: [PATCH] [BRIDGE] Set features based on slave's ones

[David S. Miller]

From: Nivedita Singhvi <niv@us.ibm.com>
Date: Thu, 19 May 2005 08:59:24 -0700

> While this is generally a cleaner thing to do - it breaks
> what we need - Jon's  patch adds the checksum offload/SG
> unconditionally precisely because we don't want it to be
> an all or nothing - we'd like to utilize offload on those
> devices that do support it..

No, I think Catalin's patch is exactly how this should
be implemented.

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Jon Mason]

This patch only enables the features which are common between all of the 
bridged devices (at the time of their addition to the bridge).  It overlooks 
the cases where not all of the adapters have the same features, or where the 
user has changed the enablement of a certain feature (via ethtool).  

Thanks,
Jon

On Thursday 19 May 2005 10:28 am, Catalin(ux aka Dino) BOIE wrote:
> Hello!
>
> The attached patch, makes the bridge to select features (almost all) only
> if _all_ devices supports them.
> The patch was tested (create a bridge, add an interface, remove the
> interface and always check features flags).
>
> What do you think?
>
> Signed-off-by: Catalin(ux aka Dino) BOIE <catab at umbrella.ro>
> ---
> Catalin(ux aka Dino) BOIE
> catab at deuroconsult.ro
> http://kernel.umbrella.ro/

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Catalin(ux aka Dino) BOIE]

On Thu, 19 May 2005, Jon Mason wrote:

> This patch only enables the features which are common between all of the
> bridged devices (at the time of their addition to the bridge).  It overlooks
> the cases where not all of the adapters have the same features, or where the
> user has changed the enablement of a certain feature (via ethtool).
>
> Thanks,
> Jon

I agree that the patch doesn't take care of ethtool oprations.
I will correct this.

But, it doesn't overlook the case when "not all of the adapters have the 
same features". If all devices have a feature, it is enabled on the bridge 
interface. Else, it is cleared. Am I missing your point?

Thank you very much for looking over the patch!

> On Thursday 19 May 2005 10:28 am, Catalin(ux aka Dino) BOIE wrote:
>> Hello!
>>
>> The attached patch, makes the bridge to select features (almost all) only
>> if _all_ devices supports them.
>> The patch was tested (create a bridge, add an interface, remove the
>> interface and always check features flags).
>>
>> What do you think?
>>
>> Signed-off-by: Catalin(ux aka Dino) BOIE <catab at umbrella.ro>
>> ---
>> Catalin(ux aka Dino) BOIE
>> catab at deuroconsult.ro
>> http://kernel.umbrella.ro/
>

---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Jon Mason]

On Thursday 19 May 2005 12:22 pm, Catalin(ux aka Dino) BOIE wrote:
> On Thu, 19 May 2005, Jon Mason wrote:
> > This patch only enables the features which are common between all of the
> > bridged devices (at the time of their addition to the bridge).  It
> > overlooks the cases where not all of the adapters have the same features,
> > or where the user has changed the enablement of a certain feature (via
> > ethtool).
> >
> > Thanks,
> > Jon
>
> I agree that the patch doesn't take care of ethtool oprations.
> I will correct this.

I think trying to correct this could be a bear of a problem.  Good luck.

> But, it doesn't overlook the case when "not all of the adapters have the
> same features". If all devices have a feature, it is enabled on the bridge
> interface. Else, it is cleared. Am I missing your point?

My point is that some features the user might want enabled regardless of 
whether all devices support them.  An example of this is where a system has 
hardware checksum support for all devices except one.  In this case, it would 
be benefitial to have this device do the checksum in software (via 
skb_checksum_help() call in dev_queue_xmit()).

> Thank you very much for looking over the patch!

I really like your patch, and I'm working on combining the two so that we have 
the best of both worlds.  I'll submit it once I have tested it, and we'll see 
who likes it.

Thanks,
Jon

Re: [PATCH] [BRIDGE] Set features based on slave's ones

[David S. Miller]

From: Jon Mason <jdmason@us.ibm.com>
Date: Thu, 19 May 2005 12:47:55 -0500

> My point is that some features the user might want enabled regardless of 
> whether all devices support them.  An example of this is where a system has 
> hardware checksum support for all devices except one.  In this case, it would 
> be benefitial to have this device do the checksum in software (via 
> skb_checksum_help() call in dev_queue_xmit()).

If "SG and checksumming is so common these days" as others have
stated, what you are describing is a totally uncommon scenerio.  What
Catalin is proposing is infinitely better than what we have today.

Re: [PATCH] [BRIDGE] Set features based on slave's ones

[Jon Mason]

On Thursday 19 May 2005 01:58 pm, David S. Miller wrote:
> From: Jon Mason <jdmason@us.ibm.com>
> Date: Thu, 19 May 2005 12:47:55 -0500
>
> > My point is that some features the user might want enabled regardless of
> > whether all devices support them.  An example of this is where a system
> > has hardware checksum support for all devices except one.  In this case,
> > it would be benefitial to have this device do the checksum in software
> > (via skb_checksum_help() call in dev_queue_xmit()).
>
> If "SG and checksumming is so common these days" as others have
> stated, what you are describing is a totally uncommon scenerio.  

Uncommon, yes, but very possible and I am trying to mitigate the performance 
effects of this case.

> What 
> Catalin is proposing is infinitely better than what we have today.

I completely agree Catalin's patch is better than what we have now.  My only 
arguement is that there are some cases where it should be augmented to 
include additional features.

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Jon Mason]

I combined my previous patch with your patch, and came up with the
following.  I have tested it on my system, and it works exactly the way
I wanted.  What do you think?

Thanks,
Jon

Signed-off-by: Catalin(ux aka Dino) BOIE <catab at umbrella.ro>
Signed-off-by: Jon Mason <jdmason@us.ibm.com>


--- linux-2.6.11-xenU/net/bridge/br_private.h	2005-03-02 01:37:50.000000000 -0600
+++ linux-2.6.11-xen0/net/bridge/br_private.h	2005-05-19 08:12:23.000000000 -0500
@@ -27,6 +27,10 @@
 #define BR_PORT_BITS	10
 #define BR_MAX_PORTS	(1<<BR_PORT_BITS)
 
+#define BR_FEAT_MASK   (NETIF_F_HW_CSUM | NETIF_F_SG \
+		| NETIF_F_FRAGLIST | NETIF_F_IP_CSUM \
+		| NETIF_F_HIGHDMA | NETIF_F_TSO)
+
 typedef struct bridge_id bridge_id;
 typedef struct mac_addr mac_addr;
 typedef __u16 port_id;
--- linux-2.6.11-xenU/net/bridge/br_device.c	2005-05-19 06:19:42.000000000 -0500
+++ linux-2.6.11-xen0/net/bridge/br_device.c	2005-05-19 08:11:19.000000000 -0500
@@ -107,4 +107,5 @@ void br_dev_setup(struct net_device *dev
 	dev->tx_queue_len = 0;
 	dev->set_mac_address = NULL;
 	dev->priv_flags = IFF_EBRIDGE;
+	dev->features = BR_FEAT_MASK;
 }
--- linux-2.6.11-xenU/net/bridge/br_if.c	2005-05-19 08:43:03.000000000 -0500
+++ linux-2.6.11-xen0/net/bridge/br_if.c	2005-05-19 08:43:58.000000000 -0500
@@ -314,6 +314,27 @@ int br_min_mtu(const struct net_bridge *
 	return mtu;
 }
 
+/*
+ * If slave device (@dev) doesn't support special features,
+ * turn them off globally.
+ */
+static inline void br_features_change(struct net_bridge *br, struct net_device *dev)
+{
+	br->dev->features &= dev->features | NETIF_F_HW_CSUM | NETIF_F_SG;
+}
+
+/*
+ * Recomputes features using slave's features
+ */
+static void br_features_recompute(struct net_bridge *br)
+{
+	struct net_bridge_port *p;
+
+	br->dev->features = BR_FEAT_MASK;
+	list_for_each_entry(p, &br->port_list, list)
+		br_features_change(br, p->dev);
+}
+
 /* called with RTNL */
 int br_add_if(struct net_bridge *br, struct net_device *dev)
 {
@@ -332,6 +353,8 @@ int br_add_if(struct net_bridge *br, str
 	if (IS_ERR(p = new_nbp(br, dev, br_initial_port_cost(dev))))
 		return PTR_ERR(p);
 
+	br_features_change(br, dev);
+
  	if ((err = br_fdb_insert(br, p, dev->dev_addr, 1)))
 		destroy_nbp(p);
  
@@ -368,6 +391,7 @@ int br_del_if(struct net_bridge *br, str
 
 	spin_lock_bh(&br->lock);
 	br_stp_recalculate_bridge_id(br);
+	br_features_recompute(br);
 	spin_unlock_bh(&br->lock);
 
 	return 0;

Re: [PATCH] [BRIDGE] Set features based on slave's ones

[David S. Miller]

From: Jon Mason <jdmason@us.ibm.com>
Date: Thu, 19 May 2005 14:00:32 -0500

> I combined my previous patch with your patch, and came up with the
> following.  I have tested it on my system, and it works exactly the way
> I wanted.  What do you think?

Look, let's do this.  Do an initial patch that only does
enables features possible on all devices.  Because we
all agree that is a definite improvement from the current
situation.

Then we can discuss the relative merits of enabling features
available on only some devices.

Ok?

Re: [PATCH] [BRIDGE] Set features based on slave's ones

[Jon Mason]

On Thursday 19 May 2005 02:21 pm, David S. Miller wrote:
> From: Jon Mason <jdmason@us.ibm.com>
> Date: Thu, 19 May 2005 14:00:32 -0500
>
> > I combined my previous patch with your patch, and came up with the
> > following.  I have tested it on my system, and it works exactly the way
> > I wanted.  What do you think?
>
> Look, let's do this.  Do an initial patch that only does
> enables features possible on all devices.  Because we
> all agree that is a definite improvement from the current
> situation.
>
> Then we can discuss the relative merits of enabling features
> available on only some devices.
>
> Ok?

That is acceptable to me.

Re: [PATCH] [BRIDGE] Set features based on slave's ones

[David S. Miller]

From: Jon Mason <jdmason@us.ibm.com>
Date: Thu, 19 May 2005 11:06:53 -0500

> This patch only enables the features which are common between all of the 
> bridged devices (at the time of their addition to the bridge).  It overlooks 
> the cases where not all of the adapters have the same features, or where the 
> user has changed the enablement of a certain feature (via ethtool).  

There is not valid setting other than the subset of
what the devices all support.

Yes, the bridging code should listen for network device
events when netdev->flag settings change, but otherwise
this is the way to implement this.  Just like bonding
does.

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Stephen Hemminger]

On Thu, 19 May 2005 18:28:26 +0300 (EEST)
"Catalin(ux aka Dino) BOIE" <util@deuroconsult.ro> wrote:

> Hello!
> 
> The attached patch, makes the bridge to select features (almost all) only 
> if _all_ devices supports them.
> The patch was tested (create a bridge, add an interface, remove the 
> interface and always check features flags).
> 
> What do you think?
> 
> Signed-off-by: Catalin(ux aka Dino) BOIE <catab at umbrella.ro>

Okay, but just open code this, don't need BR_FEAT_MASK
and br_features_change.

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Stephen Hemminger]

Also NETIF_F_LLTX should always be set on the bridge, independent of what devices are in it.

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Jon Mason]

On Thu, May 19, 2005 at 06:28:26PM +0300, Catalin(ux aka Dino) BOIE wrote:
> Hello!
> 
> The attached patch, makes the bridge to select features (almost all) only 
> if _all_ devices supports them.
> The patch was tested (create a bridge, add an interface, remove the 
> interface and always check features flags).
> 
> What do you think?
> 
> Signed-off-by: Catalin(ux aka Dino) BOIE <catab at umbrella.ro>
> ---
> Catalin(ux aka Dino) BOIE
> catab at deuroconsult.ro
> http://kernel.umbrella.ro/

> --- bridge1/net/bridge/br_private.h	2005-03-02 09:37:50.000000000 +0200
> +++ linux/net/bridge/br_private.h	2005-05-19 18:00:05.000000000 +0300
> @@ -27,6 +27,10 @@
>  #define BR_PORT_BITS	10
>  #define BR_MAX_PORTS	(1<<BR_PORT_BITS)
>  
> +#define BR_FEAT_MASK	(NETIF_F_HW_CSUM | NETIF_F_SG \
> +			| NETIF_F_FRAGLIST | NETIF_F_IP_CSUM \
> +			| NETIF_F_HIGHDMA | NETIF_F_TSO)
> +
>  typedef struct bridge_id bridge_id;
>  typedef struct mac_addr mac_addr;
>  typedef __u16 port_id;
> --- bridge1/net/bridge/br_device.c	2005-03-02 09:37:30.000000000 +0200
> +++ linux/net/bridge/br_device.c	2005-05-19 17:14:21.000000000 +0300
> @@ -107,4 +107,5 @@ void br_dev_setup(struct net_device *dev
>  	dev->tx_queue_len = 0;
>  	dev->set_mac_address = NULL;
>  	dev->priv_flags = IFF_EBRIDGE;
> +	dev->features = BR_FEAT_MASK;
>  }
> --- bridge1/net/bridge/br_if.c	2005-03-02 09:38:33.000000000 +0200
> +++ linux/net/bridge/br_if.c	2005-05-19 18:21:39.000000000 +0300
> @@ -314,6 +314,27 @@ int br_min_mtu(const struct net_bridge *
>  	return mtu;
>  }
>  
> +/*
> + * If slave device (@dev) doesn't support special features,
> + * turn them off globally.
> + */
> +static inline void br_features_change(struct net_bridge *br, struct net_device *dev)
> +{
> +	br->dev->features &= dev->features | ~BR_FEAT_MASK;

~BR_FEAT_MASK isn't necessary. Either you wanted to do an '&' just
before it, so that BR_FEAT_MASK features are the only ones that you want
or you are enabling features that aren't necessarily supported by the
adapter/driver (like LLTX and HW_VLAN stuff).

> +}
> +
> +/*
> + * Recomputes features using slave's features
> + */
> +static void br_features_recompute(struct net_bridge *br)
> +{
> +	struct net_bridge_port *p;
> +
> +	br->dev->features |= BR_FEAT_MASK;

The OR is not needed.  Just re-initialize features to BR_FEAT_MASK and
everything will take case of itself.

> +	list_for_each_entry(p, &br->port_list, list)
> +		br_features_change(br, p->dev);
> +}
> +
>  /* called with RTNL */
>  int br_add_if(struct net_bridge *br, struct net_device *dev)
>  {
> @@ -332,9 +353,10 @@ int br_add_if(struct net_bridge *br, str
>  	if (IS_ERR(p = new_nbp(br, dev, br_initial_port_cost(dev))))
>  		return PTR_ERR(p);
>  
> +	br_features_change(br, dev);
> +
>   	if ((err = br_fdb_insert(br, p, dev->dev_addr, 1)))
>  		destroy_nbp(p);
> - 
>  	else if ((err = br_sysfs_addif(p)))
>  		del_nbp(p);
>  	else {
> @@ -368,6 +390,7 @@ int br_del_if(struct net_bridge *br, str
>  
>  	spin_lock_bh(&br->lock);
>  	br_stp_recalculate_bridge_id(br);
> +	br_features_recompute(br);
>  	spin_unlock_bh(&br->lock);
>  
>  	return 0;

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Catalin(ux aka Dino) BOIE]

>> +static inline void br_features_change(struct net_bridge *br, struct net_device *dev)
>> +{
>> +	br->dev->features &= dev->features | ~BR_FEAT_MASK;
>
> ~BR_FEAT_MASK isn't necessary. Either you wanted to do an '&' just
> before it, so that BR_FEAT_MASK features are the only ones that you want
> or you are enabling features that aren't necessarily supported by the
> adapter/driver (like LLTX and HW_VLAN stuff).

So, I invert BR_FEAT_MASK so I can, by default clear special features.
Then I ored with dev->features, so I can enable special features if the 
slave device supports it. Then, I do & so I can clear stuff that is not 
supported by device, but the rest of br->dev features remains untouched.

Let's test: if dev hash SG the code is like this:
BR_FEAT_MASK = 1001 (only test SG and HW_CSUM)
# br is inited
br |= 1001
br &= 0001 | ~1001
br &= 0001 | 0110
br &= 0111
br = 1001 & 0111 = 0001 - so bit SG is set also in br. But not HW_CSUM 
because it's not in device.

I can't see the code is wrong. Can you give me an example when it fails, 
please?

>> +}
>> +
>> +/*
>> + * Recomputes features using slave's features
>> + */
>> +static void br_features_recompute(struct net_bridge *br)
>> +{
>> +	struct net_bridge_port *p;
>> +
>> +	br->dev->features |= BR_FEAT_MASK;
>
> The OR is not needed.  Just re-initialize features to BR_FEAT_MASK and
> everything will take case of itself.

Nope. Not correct. Because we might enable LLTX on the bridge, but not in 
BR_FEAT_MASK.
I will post a patch in few hours with all stuff updated.

---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Jon Mason]

On Friday 20 May 2005 01:07 am, Catalin(ux aka Dino) BOIE wrote:
> >> +static inline void br_features_change(struct net_bridge *br, struct
> >> net_device *dev) +{
> >> +	br->dev->features &= dev->features | ~BR_FEAT_MASK;
> >
> > ~BR_FEAT_MASK isn't necessary. Either you wanted to do an '&' just
> > before it, so that BR_FEAT_MASK features are the only ones that you want
> > or you are enabling features that aren't necessarily supported by the
> > adapter/driver (like LLTX and HW_VLAN stuff).
>
> So, I invert BR_FEAT_MASK so I can, by default clear special features.
> Then I ored with dev->features, so I can enable special features if the
> slave device supports it. Then, I do & so I can clear stuff that is not
> supported by device, but the rest of br->dev features remains untouched.
>
> Let's test: if dev hash SG the code is like this:
> BR_FEAT_MASK = 1001 (only test SG and HW_CSUM)
> # br is inited
> br |= 1001
> br &= 0001 | ~1001
> br &= 0001 | 0110
> br &= 0111
> br = 1001 & 0111 = 0001 - so bit SG is set also in br. But not HW_CSUM
> because it's not in device.
>
> I can't see the code is wrong. Can you give me an example when it fails,
> please?

Let's use the full BR_FEAT_MASK of NETIF_F_HW_CSUM | NETIF_F_SG |                       
| NETIF_F_FRAGLIST | NETIF_F_IP_CSUM | NETIF_F_HIGHDMA | NETIF_F_TSO).
That is 0011 0000 0110 1011

Now, let's assume that the NIC only has NETIF_F_TSO, 
NETIF_F_IP_CSUM, and NETIF_F_SG enabled.
So, that is 0000 1000 0000 0011

So, we have the following:
For the first adapter added:
br = 0001 1000 0110 1011
br = 0001 1000 0110 1011 & 0000 1000 0000 0011 | ~0001 1000 0110 1011
br = 0001 1000 0110 1011 & 0000 1000 0000 0011 | 1110 0111 1001 0100
br = 0000 1000 0000 0011 | 1110 0111 1001 0100
br = 1110 1111 1001 0111
Note - this breaks down to the following being enabled:
NETIF_F_SG
NETIF_F_IP_CSUM  
NETIF_F_NO_CSUM
UNDEFINED BIT
NETIF_F_HW_VLAN_RX
NETIF_F_HW_VLAN_FILTER 
NETIF_F_VLAN_CHALLENGED
NETIF_F_TSO 
NETIF_F_LLTX 

For a second adapter with the same features added:
br = 1110 1111 1001 0111
br = 1110 1111 1001 0111 & 0000 1000 0000 0011 | 1110 0111 1001 0100
br = 1110 1111 1001 0111 | 1110 0111 1001 0100
br = 1110 1111 1001 0111

So, NETIF_F_HW_VLAN_RX, NETIF_F_HW_VLAN_FILTER, NETIF_F_VLAN_CHALLENGED, and 
NETIF_F_LLTX will be enabled, and they might not be enabled/supported.  If 
you take out the ~BR_FEAT_MASK then it will behave as I would expect 
(verified with printks on the last patch).

On a side note:  Wouldn't it make more sense to have NETIF_F_TSO defined as 16 
in include/linux/netdevice.h (as it would be grouped with the other hardware 
offload bits and the bit is free).

> >> +}
> >> +
> >> +/*
> >> + * Recomputes features using slave's features
> >> + */
> >> +static void br_features_recompute(struct net_bridge *br)
> >> +{
> >> +	struct net_bridge_port *p;
> >> +
> >> +	br->dev->features |= BR_FEAT_MASK;
> >
> > The OR is not needed.  Just re-initialize features to BR_FEAT_MASK and
> > everything will take case of itself.
>
> Nope. Not correct. Because we might enable LLTX on the bridge, but not in
> BR_FEAT_MASK.

This is not the case.  This is the same as re-doing the entire feature list 
(as if it was being done initially).  All that needs to be done is for 
br->dev->features to have a initial value.

> I will post a patch in few hours with all stuff updated.

Great.  Thanks.

Thanks,
Jon

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Catalin(ux aka Dino) BOIE]

>>>> +/*
>>>> + * Recomputes features using slave's features
>>>> + */
>>>> +static void br_features_recompute(struct net_bridge *br)
>>>> +{
>>>> +	struct net_bridge_port *p;
>>>> +
>>>> +	br->dev->features |= BR_FEAT_MASK;
>>>
>>> The OR is not needed.  Just re-initialize features to BR_FEAT_MASK and
>>> everything will take case of itself.
>>
>> Nope. Not correct. Because we might enable LLTX on the bridge, but not in
>> BR_FEAT_MASK.
>
> This is not the case.  This is the same as re-doing the entire feature list
> (as if it was being done initially).  All that needs to be done is for
> br->dev->features to have a initial value.
>
>> I will post a patch in few hours with all stuff updated.
>
> Great.  Thanks.
>
> Thanks,
> Jon

I will post a patch with the initial features as:

dev->features = BR_FEAT_MASK | NETIF_F_NO_CSUM | NETIF_F_LLTX

at Stephen's suggestion.

But, BR_FEAT_MASK will not have NETIF_F_NO_CSUM and NETIF_F_LLTX.

I think the code is correct as it is.

---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Catalin(ux aka Dino) BOIE]

> Let's use the full BR_FEAT_MASK of NETIF_F_HW_CSUM | NETIF_F_SG |
> | NETIF_F_FRAGLIST | NETIF_F_IP_CSUM | NETIF_F_HIGHDMA | NETIF_F_TSO).
> That is 0011 0000 0110 1011
>
> Now, let's assume that the NIC only has NETIF_F_TSO,
> NETIF_F_IP_CSUM, and NETIF_F_SG enabled.
> So, that is 0000 1000 0000 0011
>
> So, we have the following:
> For the first adapter added:
> br = 0001 1000 0110 1011
> br = 0001 1000 0110 1011 & 0000 1000 0000 0011 | ~0001 1000 0110 1011
> br = 0001 1000 0110 1011 & 0000 1000 0000 0011 | 1110 0111 1001 0100
> br = 0000 1000 0000 0011 | 1110 0111 1001 0100
> br = 1110 1111 1001 0111
> Note - this breaks down to the following being enabled:
> NETIF_F_SG
> NETIF_F_IP_CSUM
> NETIF_F_NO_CSUM
> UNDEFINED BIT
> NETIF_F_HW_VLAN_RX
> NETIF_F_HW_VLAN_FILTER
> NETIF_F_VLAN_CHALLENGED
> NETIF_F_TSO
> NETIF_F_LLTX
[...]
> Thanks,
> Jon

The code applies the ~ first, then the | and _then_ the &.
If we have:
a &= a | ~b;
it is equivalent with:
a = a & (a | ~b);

For your example:
br   = 0001 1000 0110 1011 = 0x186B
feat = 0001 1000 0110 1011 = 0x186B
nic  = 0000 1000 0000 0011 = 0x0803
br &= nic | ~feat
br = 0x803 - I say this is correct!

With the second nic:

br = 0x0803
feat = 0x186B
nic2 = 0x0803
br &= nic2 | ~feat;
br = 0x0803 - I say it's correct

Am I missing something?
---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/

Re: [PATCH] [BRIDGE] Set features based on slave's ones (was Ethernet Bridging: Enable Hardware Checksumming)

[Jon Mason]

> The code applies the ~ first, then the | and _then_ the &.
> If we have:
> a &= a | ~b;
> it is equivalent with:
> a = a & (a | ~b);

 Ah, that is what I was missing.  Sorry.