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
|