Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

["Vlad Zolotarov" <vladz@broadcom.com>]

On Wednesday 31 August 2011 16:53:24 Michal Schmidt wrote:
> On Wed, 31 Aug 2011 15:01:39 +0300 Vlad Zolotarov wrote:
> > On Tuesday 30 August 2011 22:30:11 Michal Schmidt wrote:
> > > > and then also in fp->flags.  Are the
> > > > fp->flags strictly mirroring hardware state (as in: there is no
> > > > way the states can differ in any point in time where the flags are
> > > > tested)?
> > > 
> > > Yes. This is the purpose of the second mirroring of the flag.
> > 
> > Michal,
> > although the above is true i'd say it's a bit of an overkill:
> > RX VLAN stripping is configured in a function level, so keeping it in
> > a per queue level is not needed.
> > 
> > The problem u were trying to resolve (and u resolved it) was to
> > separate the RX_VLAN_ENABLED flag semantics into two: requested
> > feature and HW configured feature in order to further check the
> > second in the fast path, while setting the first one in the
> > set_features(). Then the second lag is updated according to the first
> > one during the loading of the function (bnx2x_nic_load()).
> > 
> > Therefore it would be enough to just add those two flags in the
> > function (bp) level keeping the rest of your patch as it is. This
> > would also cancel the need for a patch 6.
> 
> Alright, I will remove the per-fp flag and move it to bp.
> 
> I will also apply the cheat suggested by Michał, so that:
>  - dev->features & NETIF_F_HW_VLAN_RX is the requested state
>  - bp->flags & RX_VLAN_STRIP_FLAG is the HW configured state
> => Only one new bp flag needed, not two.
> 
> BTW, Michał's cheat should apply to TPA_ENABLE_FLAG as well - it only
> mirrors NETIF_F_LRO. But for TPA the HW configured state is really
> per-queue (fp->disable_tpa). I will remove TPA_ENABLE_FLAG unless you
> object to Michał's "cheat".

I agree the TPA_ENABLE_FLAG may and should be removed.
However I didn’t like the idea of relying on the current implementation 
of the calling function (__netdev_update_features()). If u want to change 
the implementation the way we rely on the dev->features in the 
ndo_set_features() flow, which is a semantics change, u'd rather change 
the __netdev_update_features() so that it sets the dev->features before 
ndo_set_features() call and restores it in case of a failure. Something like this:

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0a7f619..a5f6d3e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -943,8 +943,7 @@ struct net_device_ops {
                                                 struct net_device *slave_dev);
        u32                     (*ndo_fix_features)(struct net_device *dev,
                                                    u32 features);
-       int                     (*ndo_set_features)(struct net_device *dev,
-                                                   u32 features);
+       int                     (*ndo_set_features)(struct net_device *dev);
 };
 
 /*
diff --git a/net/core/dev.c b/net/core/dev.c
index b2e262e..474e539 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5321,7 +5321,7 @@ static u32 netdev_fix_features(struct net_device *dev, u32 features)
 
 int __netdev_update_features(struct net_device *dev)
 {
-       u32 features;
+       u32 features, old_features;
        int err = 0;
 
        ASSERT_RTNL();
@@ -5340,19 +5340,23 @@ int __netdev_update_features(struct net_device *dev)
        netdev_dbg(dev, "Features changed: 0x%08x -> 0x%08x\n",
                dev->features, features);
 
+       /* Remember the original features and set the new ones */
+       old_features = dev->features;
+       dev->features = features;
+
        if (dev->netdev_ops->ndo_set_features)
-               err = dev->netdev_ops->ndo_set_features(dev, features);
+               err = dev->netdev_ops->ndo_set_features(dev);
 
        if (unlikely(err < 0)) {
+               /* Restore the original features */
+               dev->features = old_features;
+
                netdev_err(dev,
                        "set_features() failed (%d); wanted 0x%08x, left 0x%08x\n",
                        err, features, dev->features);
                return -1;
        }
 
-       if (!err)
-               dev->features = features;
-
        return 1;
 }

However, this would involve updating all other L2 drivers that implement ndo_set_features().

Pls., comment.

thanks,
vlad

> 
> Thanks,
> Michal