diff mbox

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

Message ID 201108311807.53767.vladz@broadcom.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Vladislav Zolotarov Aug. 31, 2011, 3:07 p.m. UTC 
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:

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

Pls., comment.

thanks,
vlad

> 
> Thanks,
> Michal

--
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

Comments

Michal Schmidt Aug. 31, 2011, 3:37 p.m. UTC | #1 
On Wed, 31 Aug 2011 18:07:53 +0300 Vlad Zolotarov wrote:
> 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/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);

Drivers want to know which features changed. They compare
the features argument with dev->features.
Perhaps we could pass old_features as the argument. Then we'd better
change the name of the callback to avoid confusion.

I think it's not worth changing. I could restore dev->features before
returning if bnx2x_reload_if_running() fails.

Michal
--
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
Michal Schmidt Aug. 31, 2011, 3:51 p.m. UTC | #2 
On Wed, 31 Aug 2011 17:37:49 +0200 Michal Schmidt wrote:
> I could restore dev->features before
> returning if bnx2x_reload_if_running() fails.

Or even safer - restore them always:
	...
	u32 orig_features = dev->features;
	dev->features = features;
        ret = bnx2x_reload_if_running(dev);
        dev->features = orig_features;
        return ret;
	...
This way we don't have to assume anything about
__netdev_update_features().

Michal
--
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
Vladislav Zolotarov Aug. 31, 2011, 4:16 p.m. UTC | #3 
On Wednesday 31 August 2011 18:51:20 Michal Schmidt wrote:
> On Wed, 31 Aug 2011 17:37:49 +0200 Michal Schmidt wrote:
> > I could restore dev->features before
> > returning if bnx2x_reload_if_running() fails.
> 
> Or even safer - restore them always:
> 	...
> 	u32 orig_features = dev->features;
> 	dev->features = features;
>         ret = bnx2x_reload_if_running(dev);
>         dev->features = orig_features;
>         return ret;
> 	...
> This way we don't have to assume anything about
> __netdev_update_features().

I agree - it's the best choice if we go for a bnx2x-only solution.

thanks,
vlad

> 
> Michal
> --
> 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
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= Aug. 31, 2011, 6:11 p.m. UTC | #4 
2011/8/31 Michal Schmidt <mschmidt@redhat.com>:
> On Wed, 31 Aug 2011 17:37:49 +0200 Michal Schmidt wrote:
>> I could restore dev->features before
>> returning if bnx2x_reload_if_running() fails.
>
> Or even safer - restore them always:
>        ...
>        u32 orig_features = dev->features;
>        dev->features = features;
>        ret = bnx2x_reload_if_running(dev);
>        dev->features = orig_features;
>        return ret;
>        ...
> This way we don't have to assume anything about
> __netdev_update_features().

The assignment in __netdev_update_features() is just to save copying
that assignment all over the drivers' code. This won't change unless
someone wants to break^Wchange the design.

Best Regards,
Michał Mirosław
--
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
diff mbox

Patch

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;
 }