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 <[email protected]> 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/[email protected]?part=1
