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

[sashiko-bot@kernel.org]

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