[PATCH iwl-net] igb: Retry the D0 write of the i210 PLL workaround

[PATCH iwl-net] igb: Retry the D0 write of the i210 PLL workaround

[Anthony Pighin]

When the PHY PLL of a flashless i210 fails to lock, the existing errata
handler transitions the device through D3hot.

On some platforms, the root port frequently reports link training
while the device is in D3hot. The generic DesignWare link check
then treats the link as down, and the config access is rejected.
If the write that requests D0 state is rejected, igb ignores the
error and the device stays in D3hot. The next MMIO read fails, and
igb reports "PCIe link lost" and detaches the device. Every later
link setting fails.

  igb 0000:01:00.0 0000:01:00.0 (uninitialized): PCIe link lost
  igb 0000:01:00.0: Hardware Error
  ...
  igb 0000:01:00.0: Cannot change link characteristics when SoL/IDER is active.

Retry the D0 request until it is accepted, up to 1000 times. If the
device never reaches D0, stop the workaround.

Observed on LX2160A systems with a flashless i210. To confirm it, a
test build of igb forced the workaround on every probe. Without this
change, igb lost the device in 15 of 20 probes. Reading the root port
registers during the workaround showed link training reported while the
i210 was in D3hot.

With this change, all 500 probes on the same systems reached D0 with
no lost link. About 70% needed a retry, at most 21 D0 requests.
Tested with 6.12.92.

Fixes: 948264879b68 ("igb: Workaround for i210 Errata 25: Slow System Clock")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Pighin <anthony.pighin@nokia.com>
---
 drivers/net/ethernet/intel/igb/e1000_i210.c | 33 +++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_i210.c b/drivers/net/ethernet/intel/igb/e1000_i210.c
index 784f9a7bcbed..fad7cdfea197 100644
--- a/drivers/net/ethernet/intel/igb/e1000_i210.c
+++ b/drivers/net/ethernet/intel/igb/e1000_i210.c
@@ -7,9 +7,11 @@
 
 #include <linux/bitfield.h>
 #include <linux/if_ether.h>
+#include <linux/pci.h>
 #include <linux/types.h>
 #include "e1000_hw.h"
 #include "e1000_i210.h"
+#include "igb.h"
 
 static s32 igb_update_flash_i210(struct e1000_hw *hw);
 
@@ -812,6 +814,33 @@ s32 igb_init_nvm_params_i210(struct e1000_hw *hw)
 	return 0;
 }
 
+static bool igb_set_d0_i210(struct e1000_hw *hw, u16 pci_word)
+{
+	struct igb_adapter *adapter = hw->back;
+	u16 pmcsr;
+	int i;
+
+	pci_word &= ~PCI_PM_CTRL_STATE_MASK;
+	pci_word |= PCI_D0;
+
+	/* Some host controller drivers refuse config accesses while the root
+	 * port reports link training. The root port can report link training
+	 * while the device is in D3hot, so the write that requests D0 can be
+	 * refused.
+	 */
+	for (i = 0; i < 1000; i++) {
+		igb_write_pci_cfg(hw, E1000_PCI_PMCSR, &pci_word);
+		igb_read_pci_cfg(hw, E1000_PCI_PMCSR, &pmcsr);
+		if (!PCI_POSSIBLE_ERROR(pmcsr) &&
+		    (pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D0)
+			return true;
+		usleep_range(50, 100);
+	}
+
+	dev_err(&adapter->pdev->dev, "Device stuck in D3hot\n");
+	return false;
+}
+
 /**
  * igb_pll_workaround_i210
  * @hw: pointer to the HW structure
@@ -866,8 +895,8 @@ s32 igb_pll_workaround_i210(struct e1000_hw *hw)
 		pci_word |= E1000_PCI_PMCSR_D3;
 		igb_write_pci_cfg(hw, E1000_PCI_PMCSR, &pci_word);
 		usleep_range(1000, 2000);
-		pci_word &= ~E1000_PCI_PMCSR_D3;
-		igb_write_pci_cfg(hw, E1000_PCI_PMCSR, &pci_word);
+		if (!igb_set_d0_i210(hw, pci_word))
+			break;
 		reg_val = (E1000_INVM_AUTOLOAD << 4) | (nvm_word << 16);
 		wr32(E1000_EEARBC_I210, reg_val);
 

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

Re: [PATCH iwl-net] igb: Retry the D0 write of the i210 PLL workaround

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] igb: MMIO access while in D3hot after igb_set_d0_i210() fails
- [Low] igb: __bitwise pci_power_t type mismatch in igb_set_d0_i210()
--

commit f07d2c21ba2a41d9be32c940436c29816e2eedd2
Author: Anthony Pighin <anthony.pighin@nokia.com>

igb: Retry the D0 write of the i210 PLL workaround

This commit adds igb_set_d0_i210() to retry transitioning the device from
D3hot back to D0 during the i210 PLL errata workaround if the config write is
rejected while the root port reports link training.

> diff --git a/drivers/net/ethernet/intel/igb/e1000_i210.c b/drivers/net/ethernet/intel/igb/e1000_i210.c
> index 784f9a7bcbed0..fad7cdfea197f 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_i210.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_i210.c
[ ... ]
> @@ -812,6 +814,33 @@ s32 igb_init_nvm_params_i210(struct e1000_hw *hw)
>  	return 0;
>  }
>  
> +static bool igb_set_d0_i210(struct e1000_hw *hw, u16 pci_word)
> +{
> +	struct igb_adapter *adapter = hw->back;
> +	u16 pmcsr;
> +	int i;
> +
> +	pci_word &= ~PCI_PM_CTRL_STATE_MASK;
> +	pci_word |= PCI_D0;
> +
> +	/* Some host controller drivers refuse config accesses while the root
> +	 * port reports link training. The root port can report link training
> +	 * while the device is in D3hot, so the write that requests D0 can be
> +	 * refused.
> +	 */
> +	for (i = 0; i < 1000; i++) {
> +		igb_write_pci_cfg(hw, E1000_PCI_PMCSR, &pci_word);
> +		igb_read_pci_cfg(hw, E1000_PCI_PMCSR, &pmcsr);
> +		if (!PCI_POSSIBLE_ERROR(pmcsr) &&
> +		    (pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D0)
> +			return true;

[Severity: Low]
Does mixing PCI_D0 with u16 pci_word and (pmcsr & PCI_PM_CTRL_STATE_MASK)
trigger Sparse warnings?  PCI_D0 is a restricted __bitwise pci_power_t token
rather than a raw PMCSR register value, so both the bitwise OR and the
comparison degrade pci_power_t to an integer under Sparse.

[ ... ]

> @@ -866,8 +895,8 @@ s32 igb_pll_workaround_i210(struct e1000_hw *hw)
>  		pci_word |= E1000_PCI_PMCSR_D3;
>  		igb_write_pci_cfg(hw, E1000_PCI_PMCSR, &pci_word);
>  		usleep_range(1000, 2000);
> -		pci_word &= ~E1000_PCI_PMCSR_D3;
> -		igb_write_pci_cfg(hw, E1000_PCI_PMCSR, &pci_word);
> +		if (!igb_set_d0_i210(hw, pci_word))
> +			break;

[Severity: Medium]
If igb_set_d0_i210() fails, does breaking out of the loop still perform MMIO
accesses while the device is stuck in D3hot?

After the loop, igb_pll_workaround_i210() runs:

drivers/net/ethernet/intel/igb/e1000_i210.c:igb_pll_workaround_i210() {
    ...
	igb_write_phy_reg_82580(hw, I347AT4_PAGE_SELECT, 0);
	/* restore MDICNFG setting */
	wr32(E1000_MDICNFG, mdicnfg);
	return ret_val;
}

igb_write_phy_reg_82580() acquires the PHY semaphore and reads E1000_SWSM via
igb_rd32(), and igb_reset() also continues issuing MMIO accesses after
init_hw() fails.  Because the device is still in D3hot, readl() returns
0xffffffff, causing igb_rd32() to clear hw->hw_addr, report "PCIe link lost",
and trigger a WARN().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105205325.2624892-1-anthony.pighin@nokia.com?part=1