Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/780097/?format=api
{ "id": 780097, "url": "http://patchwork.ozlabs.org/api/patches/780097/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20170623082451.32671-9-alice.michael@intel.com/", "project": { "id": 46, "url": "http://patchwork.ozlabs.org/api/projects/46/?format=api", "name": "Intel Wired Ethernet development", "link_name": "intel-wired-lan", "list_id": "intel-wired-lan.osuosl.org", "list_email": "intel-wired-lan@osuosl.org", "web_url": "", "scm_url": "", "webscm_url": "", "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20170623082451.32671-9-alice.michael@intel.com>", "list_archive_url": null, "date": "2017-06-23T08:24:50", "name": "[next,S74-V2,09/10] i40e/i40evf: use cmpxchg64 when updating private flags in ethtool", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "309e9638a212d16be9d4e61eee42a3dbaf3bc915", "submitter": { "id": 71123, "url": "http://patchwork.ozlabs.org/api/people/71123/?format=api", "name": "Michael, Alice", "email": "alice.michael@intel.com" }, "delegate": { "id": 68, "url": "http://patchwork.ozlabs.org/api/users/68/?format=api", "username": "jtkirshe", "first_name": "Jeff", "last_name": "Kirsher", "email": "jeffrey.t.kirsher@intel.com" }, "mbox": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20170623082451.32671-9-alice.michael@intel.com/mbox/", "series": [], "comments": "http://patchwork.ozlabs.org/api/patches/780097/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/780097/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<intel-wired-lan-bounces@osuosl.org>", "X-Original-To": [ "incoming@patchwork.ozlabs.org", "intel-wired-lan@lists.osuosl.org" ], "Delivered-To": [ "patchwork-incoming@bilbo.ozlabs.org", "intel-wired-lan@lists.osuosl.org" ], "Received": [ "from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3wvP6G4j7gz9sNZ\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSat, 24 Jun 2017 02:29:10 +1000 (AEST)", "from localhost (localhost [127.0.0.1])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id 34605877B5;\n\tFri, 23 Jun 2017 16:29:09 +0000 (UTC)", "from fraxinus.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id 64wnUoWkA_8Z; Fri, 23 Jun 2017 16:29:08 +0000 (UTC)", "from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id 50E1C878FA;\n\tFri, 23 Jun 2017 16:29:08 +0000 (UTC)", "from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133])\n\tby ash.osuosl.org (Postfix) with ESMTP id CDAA11C26F5\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 23 Jun 2017 16:29:06 +0000 (UTC)", "from localhost (localhost [127.0.0.1])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id C73C988E18\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 23 Jun 2017 16:29:06 +0000 (UTC)", "from hemlock.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id AHgtei8-BSWx for <intel-wired-lan@lists.osuosl.org>;\n\tFri, 23 Jun 2017 16:29:06 +0000 (UTC)", "from mga07.intel.com (mga07.intel.com [134.134.136.100])\n\tby hemlock.osuosl.org (Postfix) with ESMTPS id 2157088DFC\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 23 Jun 2017 16:29:06 +0000 (UTC)", "from fmsmga001.fm.intel.com ([10.253.24.23])\n\tby orsmga105.jf.intel.com with ESMTP; 23 Jun 2017 09:29:05 -0700", "from unknown (HELO localhost.jf.intel.com) ([10.166.16.121])\n\tby fmsmga001.fm.intel.com with ESMTP; 23 Jun 2017 09:29:05 -0700" ], "X-Virus-Scanned": [ "amavisd-new at osuosl.org", "amavisd-new at osuosl.org" ], "X-Greylist": "domain auto-whitelisted by SQLgrey-1.7.6", "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos; i=\"5.39,379,1493708400\"; d=\"scan'208\";\n\ta=\"1163958948\"", "From": "Alice Michael <alice.michael@intel.com>", "To": "alice.michael@intel.com,\n\tintel-wired-lan@lists.osuosl.org", "Date": "Fri, 23 Jun 2017 04:24:50 -0400", "Message-Id": "<20170623082451.32671-9-alice.michael@intel.com>", "X-Mailer": "git-send-email 2.9.3", "In-Reply-To": "<20170623082451.32671-1-alice.michael@intel.com>", "References": "<20170623082451.32671-1-alice.michael@intel.com>", "Subject": "[Intel-wired-lan] [next PATCH S74-V2 09/10] i40e/i40evf: use\n\tcmpxchg64 when updating private flags in ethtool", "X-BeenThere": "intel-wired-lan@osuosl.org", "X-Mailman-Version": "2.1.18-1", "Precedence": "list", "List-Id": "Intel Wired Ethernet Linux Kernel Driver Development\n\t<intel-wired-lan.osuosl.org>", "List-Unsubscribe": "<https://lists.osuosl.org/mailman/options/intel-wired-lan>, \n\t<mailto:intel-wired-lan-request@osuosl.org?subject=unsubscribe>", "List-Archive": "<http://lists.osuosl.org/pipermail/intel-wired-lan/>", "List-Post": "<mailto:intel-wired-lan@osuosl.org>", "List-Help": "<mailto:intel-wired-lan-request@osuosl.org?subject=help>", "List-Subscribe": "<https://lists.osuosl.org/mailman/listinfo/intel-wired-lan>, \n\t<mailto:intel-wired-lan-request@osuosl.org?subject=subscribe>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "intel-wired-lan-bounces@osuosl.org", "Sender": "\"Intel-wired-lan\" <intel-wired-lan-bounces@osuosl.org>" }, "content": "From: Jacob Keller <jacob.e.keller@intel.com>\n\nWhen a user gives an invalid command to change a private flag which is\nnot supported, either because it is read-only, or the device is not\ncapable of the feature, we simply ignore the request.\n\nA naive solution would simply be to report error codes when one of the\nflags was not supported. However, this causes problems because it makes\nthe operation not atomic. If a user requests multiple private flags\ntogether at once we could end up changing one before failing at the\nsecond flag.\n\nWe can do a bit better if we instead update a temporary copy of the\nflags variable in the loop, and then copy it into place after. If we\naren't careful this has the pitfall of potentially silently overwriting\nany changes caused by other threads.\n\nAvoid this by using cmpxchg64 which will compare and swap the flags\nvariable only if it currently matched the old value. We'll report\n-EAGAIN in the (hopefully rare!) case where the cmpxchg64 fails.\n\nThis ensures that we can properly report when flags are not supported in\nan atomic fashion without the risk of overwriting other threads changes.\n\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\n drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 57 +++++++++++++++-------\n drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c | 41 ++++++++++++----\n 2 files changed, 70 insertions(+), 28 deletions(-)", "diff": "diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c\nindex c76549e..a868c8d 100644\n--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c\n+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c\n@@ -4069,23 +4069,26 @@ static int i40e_set_priv_flags(struct net_device *dev, u32 flags)\n \tstruct i40e_netdev_priv *np = netdev_priv(dev);\n \tstruct i40e_vsi *vsi = np->vsi;\n \tstruct i40e_pf *pf = vsi->back;\n-\tu64 changed_flags;\n+\tu64 orig_flags, new_flags, changed_flags;\n \tu32 i, j;\n \n-\tchanged_flags = pf->flags;\n+\torig_flags = READ_ONCE(pf->flags);\n+\tnew_flags = orig_flags;\n \n \tfor (i = 0; i < I40E_PRIV_FLAGS_STR_LEN; i++) {\n \t\tconst struct i40e_priv_flags *priv_flags;\n \n \t\tpriv_flags = &i40e_gstrings_priv_flags[i];\n \n-\t\tif (priv_flags->read_only)\n-\t\t\tcontinue;\n-\n \t\tif (flags & BIT(i))\n-\t\t\tpf->flags |= priv_flags->flag;\n+\t\t\tnew_flags |= priv_flags->flag;\n \t\telse\n-\t\t\tpf->flags &= ~(priv_flags->flag);\n+\t\t\tnew_flags &= ~(priv_flags->flag);\n+\n+\t\t/* If this is a read-only flag, it can't be changed */\n+\t\tif (priv_flags->read_only &&\n+\t\t ((orig_flags ^ new_flags) & ~BIT(i)))\n+\t\t\treturn -EOPNOTSUPP;\n \t}\n \n \tif (pf->hw.pf_id != 0)\n@@ -4096,18 +4099,40 @@ static int i40e_set_priv_flags(struct net_device *dev, u32 flags)\n \n \t\tpriv_flags = &i40e_gl_gstrings_priv_flags[j];\n \n-\t\tif (priv_flags->read_only)\n-\t\t\tcontinue;\n-\n \t\tif (flags & BIT(i + j))\n-\t\t\tpf->flags |= priv_flags->flag;\n+\t\t\tnew_flags |= priv_flags->flag;\n \t\telse\n-\t\t\tpf->flags &= ~(priv_flags->flag);\n+\t\t\tnew_flags &= ~(priv_flags->flag);\n+\n+\t\t/* If this is a read-only flag, it can't be changed */\n+\t\tif (priv_flags->read_only &&\n+\t\t ((orig_flags ^ new_flags) & ~BIT(i)))\n+\t\t\treturn -EOPNOTSUPP;\n \t}\n \n flags_complete:\n-\t/* check for flags that changed */\n-\tchanged_flags ^= pf->flags;\n+\t/* Before we finalize any flag changes, we need to perform some\n+\t * checks to ensure that the changes are supported and safe.\n+\t */\n+\n+\t/* ATR eviction is not supported on all devices */\n+\tif ((new_flags & I40E_FLAG_HW_ATR_EVICT_ENABLED) &&\n+\t !(pf->hw_features & I40E_HW_ATR_EVICT_CAPABLE))\n+\t\treturn -EOPNOTSUPP;\n+\n+\t/* Compare and exchange the new flags into place. If we failed, that\n+\t * is if cmpxchg64 returns anything but the old value, this means that\n+\t * something else has modified the flags variable since we copied it\n+\t * originally. We'll just punt with an error and log something in the\n+\t * message buffer.\n+\t */\n+\tif (cmpxchg64(&pf->flags, orig_flags, new_flags) != orig_flags) {\n+\t\tdev_warn(&pf->pdev->dev,\n+\t\t\t \"Unable to update pf->flags as it was modified by another thread...\\n\");\n+\t\treturn -EAGAIN;\n+\t}\n+\n+\tchanged_flags = orig_flags ^ new_flags;\n \n \t/* Process any additional changes needed as a result of flag changes.\n \t * The changed_flags value reflects the list of bits that were\n@@ -4121,10 +4146,6 @@ static int i40e_set_priv_flags(struct net_device *dev, u32 flags)\n \t\tset_bit(__I40E_FD_FLUSH_REQUESTED, pf->state);\n \t}\n \n-\t/* Only allow ATR evict on hardware that is capable of handling it */\n-\tif (!(pf->hw_features & I40E_HW_ATR_EVICT_CAPABLE))\n-\t\tpf->flags &= ~I40E_FLAG_HW_ATR_EVICT_ENABLED;\n-\n \tif (changed_flags & I40E_FLAG_TRUE_PROMISC_SUPPORT) {\n \t\tu16 sw_flags = 0, valid_flags = 0;\n \t\tint ret;\ndiff --git a/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c\nindex db79fb3..31fb276 100644\n--- a/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c\n+++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c\n@@ -258,29 +258,50 @@ static u32 i40evf_get_priv_flags(struct net_device *netdev)\n static int i40evf_set_priv_flags(struct net_device *netdev, u32 flags)\n {\n \tstruct i40evf_adapter *adapter = netdev_priv(netdev);\n-\tu64 changed_flags;\n+\tu32 orig_flags, new_flags, changed_flags;\n \tu32 i;\n \n-\tchanged_flags = adapter->flags;\n+\torig_flags = READ_ONCE(adapter->flags);\n+\tnew_flags = orig_flags;\n \n \tfor (i = 0; i < I40EVF_PRIV_FLAGS_STR_LEN; i++) {\n \t\tconst struct i40evf_priv_flags *priv_flags;\n \n \t\tpriv_flags = &i40evf_gstrings_priv_flags[i];\n \n-\t\tif (priv_flags->read_only)\n-\t\t\tcontinue;\n-\n \t\tif (flags & BIT(i))\n-\t\t\tadapter->flags |= priv_flags->flag;\n+\t\t\tnew_flags |= priv_flags->flag;\n \t\telse\n-\t\t\tadapter->flags &= ~(priv_flags->flag);\n+\t\t\tnew_flags &= ~(priv_flags->flag);\n+\n+\t\tif (priv_flags->read_only &&\n+\t\t ((orig_flags ^ new_flags) & ~BIT(i)))\n+\t\t\treturn -EOPNOTSUPP;\n+\t}\n+\n+\t/* Before we finalize any flag changes, any checks which we need to\n+\t * perform to determine if the new flags will be supported should go\n+\t * here...\n+\t */\n+\n+\t/* Compare and exchange the new flags into place. If we failed, that\n+\t * is if cmpxchg returns anything but the old value, this means\n+\t * something else must have modified the flags variable since we\n+\t * copied it. We'll just punt with an error and log something in the\n+\t * message buffer.\n+\t */\n+\tif (cmpxchg(&adapter->flags, orig_flags, new_flags) != orig_flags) {\n+\t\tdev_warn(&adapter->pdev->dev,\n+\t\t\t \"Unable to update adapter->flags as it was modified by another thread...\\n\");\n+\t\treturn -EAGAIN;\n \t}\n \n-\t/* check for flags that changed */\n-\tchanged_flags ^= adapter->flags;\n+\tchanged_flags = orig_flags ^ new_flags;\n \n-\t/* Process any additional changes needed as a result of flag changes. */\n+\t/* Process any additional changes needed as a result of flag changes.\n+\t * The changed_flags value reflects the list of bits that were changed\n+\t * in the code above.\n+\t */\n \n \t/* issue a reset to force legacy-rx change to take effect */\n \tif (changed_flags & I40EVF_FLAG_LEGACY_RX) {\n", "prefixes": [ "next", "S74-V2", "09/10" ] }