From patchwork Wed Aug 31 15:07:53 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Vladislav Zolotarov X-Patchwork-Id: 112568 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 93E6AB6F18 for ; Thu, 1 Sep 2011 01:08:45 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756211Ab1HaPIj (ORCPT ); Wed, 31 Aug 2011 11:08:39 -0400 Received: from mms3.broadcom.com ([216.31.210.19]:1216 "EHLO MMS3.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756064Ab1HaPIi convert rfc822-to-8bit (ORCPT ); Wed, 31 Aug 2011 11:08:38 -0400 Received: from [10.9.200.131] by MMS3.broadcom.com with ESMTP (Broadcom SMTP Relay (Email Firewall v6.3.2)); Wed, 31 Aug 2011 08:13:52 -0700 X-Server-Uuid: B55A25B1-5D7D-41F8-BC53-C57E7AD3C201 Received: from mail-irva-13.broadcom.com (10.11.16.103) by IRVEXCHHUB01.corp.ad.broadcom.com (10.9.200.131) with Microsoft SMTP Server id 8.2.247.2; Wed, 31 Aug 2011 08:08:20 -0700 Received: from lb-tlvb-vladz.localnet (lb-tlvb-vladz.il.broadcom.com [10.185.6.94]) by mail-irva-13.broadcom.com (Postfix) with ESMTP id D8C1574D03; Wed, 31 Aug 2011 08:08:19 -0700 (PDT) From: "Vlad Zolotarov" Organization: Broadcom To: "Michal Schmidt" Subject: Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle Date: Wed, 31 Aug 2011 18:07:53 +0300 User-Agent: KMail/1.13.6 (Linux/2.6.38-11-generic; KDE/4.6.2; x86_64; ; ) cc: =?utf-8?q?Micha=C5=82_Miros=C5=82aw?= , "netdev@vger.kernel.org" , "Dmitry Kravkov" , "Eilon Greenstein" References: <1314714646-3642-1-git-send-email-mschmidt@redhat.com> <201108311501.39936.vladz@broadcom.com> <20110831155324.7554d035@alice> In-Reply-To: <20110831155324.7554d035@alice> MIME-Version: 1.0 Message-ID: <201108311807.53767.vladz@broadcom.com> X-WSS-ID: 6240903A3KO11868524-01-01 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 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; }