Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/810349/?format=api
{ "id": 810349, "url": "http://patchwork.ozlabs.org/api/patches/810349/?format=api", "web_url": "http://patchwork.ozlabs.org/project/netdev/patch/20170906010418.39007-2-jeffrey.t.kirsher@intel.com/", "project": { "id": 7, "url": "http://patchwork.ozlabs.org/api/projects/7/?format=api", "name": "Linux network development", "link_name": "netdev", "list_id": "netdev.vger.kernel.org", "list_email": "netdev@vger.kernel.org", "web_url": null, "scm_url": null, "webscm_url": null, "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20170906010418.39007-2-jeffrey.t.kirsher@intel.com>", "list_archive_url": null, "date": "2017-09-06T01:04:17", "name": "[net,1/2] i40e: avoid NVM acquire deadlock during NVM update", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": true, "hash": "298cdd6ad295bb933cac29fb556b22f35915f44a", "submitter": { "id": 473, "url": "http://patchwork.ozlabs.org/api/people/473/?format=api", "name": "Kirsher, Jeffrey T", "email": "jeffrey.t.kirsher@intel.com" }, "delegate": { "id": 34, "url": "http://patchwork.ozlabs.org/api/users/34/?format=api", "username": "davem", "first_name": "David", "last_name": "Miller", "email": "davem@davemloft.net" }, "mbox": "http://patchwork.ozlabs.org/project/netdev/patch/20170906010418.39007-2-jeffrey.t.kirsher@intel.com/mbox/", "series": [ { "id": 1677, "url": "http://patchwork.ozlabs.org/api/series/1677/?format=api", "web_url": "http://patchwork.ozlabs.org/project/netdev/list/?series=1677", "date": "2017-09-06T01:04:16", "name": "Intel Wired LAN Driver Updates 2017-09-05", "version": 1, "mbox": "http://patchwork.ozlabs.org/series/1677/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/810349/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/810349/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<netdev-owner@vger.kernel.org>", "X-Original-To": "patchwork-incoming@ozlabs.org", "Delivered-To": "patchwork-incoming@ozlabs.org", "Authentication-Results": "ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)", "Received": [ "from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xn53Y63Vmz9s7h\n\tfor <patchwork-incoming@ozlabs.org>;\n\tWed, 6 Sep 2017 11:05:13 +1000 (AEST)", "(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753900AbdIFBE3 (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tTue, 5 Sep 2017 21:04:29 -0400", "from mga01.intel.com ([192.55.52.88]:63879 \"EHLO mga01.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753635AbdIFBEZ (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tTue, 5 Sep 2017 21:04:25 -0400", "from fmsmga004.fm.intel.com ([10.253.24.48])\n\tby fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t05 Sep 2017 18:04:24 -0700", "from nurashix-mobl1.gar.corp.intel.com (HELO\n\tjtkirshe-DESK.amr.corp.intel.com.com) ([10.254.78.137])\n\tby fmsmga004.fm.intel.com with ESMTP; 05 Sep 2017 18:04:23 -0700" ], "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.41,481,1498546800\"; d=\"scan'208\";a=\"308353595\"", "From": "Jeff Kirsher <jeffrey.t.kirsher@intel.com>", "To": "davem@davemloft.net", "Cc": "Anjali Singhai Jain <anjali.singhai@intel.com>,\n\tnetdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,\n\tjogreene@redhat.com, Jacob Keller <jacob.e.keller@intel.com>,\n\tJeff Kirsher <jeffrey.t.kirsher@intel.com>", "Subject": "[net 1/2] i40e: avoid NVM acquire deadlock during NVM update", "Date": "Tue, 5 Sep 2017 18:04:17 -0700", "Message-Id": "<20170906010418.39007-2-jeffrey.t.kirsher@intel.com>", "X-Mailer": "git-send-email 2.14.1", "In-Reply-To": "<20170906010418.39007-1-jeffrey.t.kirsher@intel.com>", "References": "<20170906010418.39007-1-jeffrey.t.kirsher@intel.com>", "Sender": "netdev-owner@vger.kernel.org", "Precedence": "bulk", "List-ID": "<netdev.vger.kernel.org>", "X-Mailing-List": "netdev@vger.kernel.org" }, "content": "From: Anjali Singhai Jain <anjali.singhai@intel.com>\n\nX722 devices use the AdminQ to access the NVM, and this requires taking\nthe AdminQ lock. Because of this, we lock the AdminQ during\ni40e_read_nvm(), which is also called in places where the lock is\nalready held, such as the firmware update path which wants to lock once\nand then unlock when finished after performing several tasks.\n\nAlthough this should have only affected X722 devices, commit\n96a39aed25e6 (\"i40e: Acquire NVM lock before reads on all devices\",\n2016-12-02) added locking for all NVM reads, regardless of device\nfamily.\n\nThis resulted in us accidentally causing NVM acquire timeouts on all\ndevices, causing failed firmware updates which left the eeprom in\na corrupt state.\n\nCreate unsafe non-locked variants of i40e_read_nvm_word and\ni40e_read_nvm_buffer, __i40e_read_nvm_word and __i40e_read_nvm_buffer\nrespectively. These variants will not take the NVM lock and are expected\nto only be called in places where the NVM lock is already held if\nneeded.\n\nSince the only caller of i40e_read_nvm_buffer() was in such a path,\nremove it entirely in favor of the unsafe version. If necessary we can\nalways add it back in the future.\n\nAdditionally, we now need to hold the NVM lock in i40e_validate_checksum\nbecause the call to i40e_calc_nvm_checksum now assumes that the NVM lock\nis held. We can further move the call to read I40E_SR_SW_CHECKSUM_WORD\nup a bit so that we do not need to acquire the NVM lock twice.\n\nThis should resolve firmware updates and also fix potential raise that\ncould have caused the driver to report an invalid NVM checksum upon\ndriver load.\n\nReported-by: Stefan Assmann <sassmann@kpanic.de>\nFixes: 96a39aed25e6 (\"i40e: Acquire NVM lock before reads on all devices\", 2016-12-02)\nSigned-off-by: Anjali Singhai Jain <anjali.singhai@intel.com>\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\nTested-by: Andrew Bowers <andrewx.bowers@intel.com>\nSigned-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>\n---\n drivers/net/ethernet/intel/i40e/i40e_nvm.c | 98 +++++++++++++++---------\n drivers/net/ethernet/intel/i40e/i40e_prototype.h | 2 -\n 2 files changed, 60 insertions(+), 40 deletions(-)", "diff": "diff --git a/drivers/net/ethernet/intel/i40e/i40e_nvm.c b/drivers/net/ethernet/intel/i40e/i40e_nvm.c\nindex 800bd55d0159..154ba49c2c6f 100644\n--- a/drivers/net/ethernet/intel/i40e/i40e_nvm.c\n+++ b/drivers/net/ethernet/intel/i40e/i40e_nvm.c\n@@ -266,7 +266,7 @@ static i40e_status i40e_read_nvm_aq(struct i40e_hw *hw, u8 module_pointer,\n * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)\n * @data: word read from the Shadow RAM\n *\n- * Reads one 16 bit word from the Shadow RAM using the GLNVM_SRCTL register.\n+ * Reads one 16 bit word from the Shadow RAM using the AdminQ\n **/\n static i40e_status i40e_read_nvm_word_aq(struct i40e_hw *hw, u16 offset,\n \t\t\t\t\t u16 *data)\n@@ -280,27 +280,49 @@ static i40e_status i40e_read_nvm_word_aq(struct i40e_hw *hw, u16 offset,\n }\n \n /**\n- * i40e_read_nvm_word - Reads Shadow RAM\n+ * __i40e_read_nvm_word - Reads nvm word, assumes called does the locking\n * @hw: pointer to the HW structure\n * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)\n * @data: word read from the Shadow RAM\n *\n- * Reads one 16 bit word from the Shadow RAM using the GLNVM_SRCTL register.\n+ * Reads one 16 bit word from the Shadow RAM.\n+ *\n+ * Do not use this function except in cases where the nvm lock is already\n+ * taken via i40e_acquire_nvm().\n+ **/\n+static i40e_status __i40e_read_nvm_word(struct i40e_hw *hw,\n+\t\t\t\t\tu16 offset, u16 *data)\n+{\n+\ti40e_status ret_code = 0;\n+\n+\tif (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE)\n+\t\tret_code = i40e_read_nvm_word_aq(hw, offset, data);\n+\telse\n+\t\tret_code = i40e_read_nvm_word_srctl(hw, offset, data);\n+\treturn ret_code;\n+}\n+\n+/**\n+ * i40e_read_nvm_word - Reads nvm word and acquire lock if necessary\n+ * @hw: pointer to the HW structure\n+ * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)\n+ * @data: word read from the Shadow RAM\n+ *\n+ * Reads one 16 bit word from the Shadow RAM.\n **/\n i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,\n \t\t\t u16 *data)\n {\n-\tenum i40e_status_code ret_code = 0;\n+\ti40e_status ret_code = 0;\n \n \tret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);\n-\tif (!ret_code) {\n-\t\tif (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE) {\n-\t\t\tret_code = i40e_read_nvm_word_aq(hw, offset, data);\n-\t\t} else {\n-\t\t\tret_code = i40e_read_nvm_word_srctl(hw, offset, data);\n-\t\t}\n-\t\ti40e_release_nvm(hw);\n-\t}\n+\tif (ret_code)\n+\t\treturn ret_code;\n+\n+\tret_code = __i40e_read_nvm_word(hw, offset, data);\n+\n+\ti40e_release_nvm(hw);\n+\n \treturn ret_code;\n }\n \n@@ -393,31 +415,25 @@ static i40e_status i40e_read_nvm_buffer_aq(struct i40e_hw *hw, u16 offset,\n }\n \n /**\n- * i40e_read_nvm_buffer - Reads Shadow RAM buffer\n+ * __i40e_read_nvm_buffer - Reads nvm buffer, caller must acquire lock\n * @hw: pointer to the HW structure\n * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF).\n * @words: (in) number of words to read; (out) number of words actually read\n * @data: words read from the Shadow RAM\n *\n * Reads 16 bit words (data buffer) from the SR using the i40e_read_nvm_srrd()\n- * method. The buffer read is preceded by the NVM ownership take\n- * and followed by the release.\n+ * method.\n **/\n-i40e_status i40e_read_nvm_buffer(struct i40e_hw *hw, u16 offset,\n-\t\t\t\t u16 *words, u16 *data)\n+static i40e_status __i40e_read_nvm_buffer(struct i40e_hw *hw,\n+\t\t\t\t\t u16 offset, u16 *words,\n+\t\t\t\t\t u16 *data)\n {\n-\tenum i40e_status_code ret_code = 0;\n+\ti40e_status ret_code = 0;\n \n-\tif (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE) {\n-\t\tret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);\n-\t\tif (!ret_code) {\n-\t\t\tret_code = i40e_read_nvm_buffer_aq(hw, offset, words,\n-\t\t\t\t\t\t\t data);\n-\t\t\ti40e_release_nvm(hw);\n-\t\t}\n-\t} else {\n+\tif (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE)\n+\t\tret_code = i40e_read_nvm_buffer_aq(hw, offset, words, data);\n+\telse\n \t\tret_code = i40e_read_nvm_buffer_srctl(hw, offset, words, data);\n-\t}\n \treturn ret_code;\n }\n \n@@ -499,15 +515,15 @@ static i40e_status i40e_calc_nvm_checksum(struct i40e_hw *hw,\n \tdata = (u16 *)vmem.va;\n \n \t/* read pointer to VPD area */\n-\tret_code = i40e_read_nvm_word(hw, I40E_SR_VPD_PTR, &vpd_module);\n+\tret_code = __i40e_read_nvm_word(hw, I40E_SR_VPD_PTR, &vpd_module);\n \tif (ret_code) {\n \t\tret_code = I40E_ERR_NVM_CHECKSUM;\n \t\tgoto i40e_calc_nvm_checksum_exit;\n \t}\n \n \t/* read pointer to PCIe Alt Auto-load module */\n-\tret_code = i40e_read_nvm_word(hw, I40E_SR_PCIE_ALT_AUTO_LOAD_PTR,\n-\t\t\t\t &pcie_alt_module);\n+\tret_code = __i40e_read_nvm_word(hw, I40E_SR_PCIE_ALT_AUTO_LOAD_PTR,\n+\t\t\t\t\t&pcie_alt_module);\n \tif (ret_code) {\n \t\tret_code = I40E_ERR_NVM_CHECKSUM;\n \t\tgoto i40e_calc_nvm_checksum_exit;\n@@ -521,7 +537,7 @@ static i40e_status i40e_calc_nvm_checksum(struct i40e_hw *hw,\n \t\tif ((i % I40E_SR_SECTOR_SIZE_IN_WORDS) == 0) {\n \t\t\tu16 words = I40E_SR_SECTOR_SIZE_IN_WORDS;\n \n-\t\t\tret_code = i40e_read_nvm_buffer(hw, i, &words, data);\n+\t\t\tret_code = __i40e_read_nvm_buffer(hw, i, &words, data);\n \t\t\tif (ret_code) {\n \t\t\t\tret_code = I40E_ERR_NVM_CHECKSUM;\n \t\t\t\tgoto i40e_calc_nvm_checksum_exit;\n@@ -593,14 +609,19 @@ i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,\n \tu16 checksum_sr = 0;\n \tu16 checksum_local = 0;\n \n+\t/* We must acquire the NVM lock in order to correctly synchronize the\n+\t * NVM accesses across multiple PFs. Without doing so it is possible\n+\t * for one of the PFs to read invalid data potentially indicating that\n+\t * the checksum is invalid.\n+\t */\n+\tret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);\n+\tif (ret_code)\n+\t\treturn ret_code;\n \tret_code = i40e_calc_nvm_checksum(hw, &checksum_local);\n+\t__i40e_read_nvm_word(hw, I40E_SR_SW_CHECKSUM_WORD, &checksum_sr);\n+\ti40e_release_nvm(hw);\n \tif (ret_code)\n-\t\tgoto i40e_validate_nvm_checksum_exit;\n-\n-\t/* Do not use i40e_read_nvm_word() because we do not want to take\n-\t * the synchronization semaphores twice here.\n-\t */\n-\ti40e_read_nvm_word(hw, I40E_SR_SW_CHECKSUM_WORD, &checksum_sr);\n+\t\treturn ret_code;\n \n \t/* Verify read checksum from EEPROM is the same as\n \t * calculated checksum\n@@ -612,7 +633,6 @@ i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,\n \tif (checksum)\n \t\t*checksum = checksum_local;\n \n-i40e_validate_nvm_checksum_exit:\n \treturn ret_code;\n }\n \n@@ -997,6 +1017,7 @@ static i40e_status i40e_nvmupd_state_writing(struct i40e_hw *hw,\n \t\tbreak;\n \n \tcase I40E_NVMUPD_CSUM_CON:\n+\t\t/* Assumes the caller has acquired the nvm */\n \t\tstatus = i40e_update_nvm_checksum(hw);\n \t\tif (status) {\n \t\t\t*perrno = hw->aq.asq_last_status ?\n@@ -1011,6 +1032,7 @@ static i40e_status i40e_nvmupd_state_writing(struct i40e_hw *hw,\n \t\tbreak;\n \n \tcase I40E_NVMUPD_CSUM_LCB:\n+\t\t/* Assumes the caller has acquired the nvm */\n \t\tstatus = i40e_update_nvm_checksum(hw);\n \t\tif (status) {\n \t\t\t*perrno = hw->aq.asq_last_status ?\ndiff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h\nindex df613ea40313..a39b13197891 100644\n--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h\n+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h\n@@ -311,8 +311,6 @@ i40e_status i40e_acquire_nvm(struct i40e_hw *hw,\n void i40e_release_nvm(struct i40e_hw *hw);\n i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,\n \t\t\t\t\t u16 *data);\n-i40e_status i40e_read_nvm_buffer(struct i40e_hw *hw, u16 offset,\n-\t\t\t\t\t u16 *words, u16 *data);\n i40e_status i40e_update_nvm_checksum(struct i40e_hw *hw);\n i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,\n \t\t\t\t\t\t u16 *checksum);\n", "prefixes": [ "net", "1/2" ] }