Re: [PATCH net] igc: Disable PCIe L1.2 substate on I225-LMvP and I226-LMvP devices

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The widened condition in `__igc_resume()` (igc_main.c:7562-7563)
  makes I225-LMvP and I226-LMvP call the non-locked…
- [Medium] Adding `IGC_DEV_ID_I226_LMVP` to `igc_is_device_id_i226()`
  (drivers/net/ethernet/intel/igc/igc_base.c:450) changes more than the…
- [Low] In `igc_probe()`, `__igc_resume()` and `igc_io_slot_reset()`, the
  return value of…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch now
  exposes I226-LMvP to it.

--

igc: Disable PCIe L1.2 substate on I225-LMvP and I226-LMvP devices

This commit adds IGC_DEV_ID_I226_LMVP to igc_is_device_id_i226(). It also
extends the ASPM L1.2 disable in probe, resume and PCI error recovery to
IGC_DEV_ID_I225_LMVP. The motivation is BadDLLP errors, link timeouts and
shutdown hangs seen when I225-LMvP negotiates L1.2 behind Thunderbolt/USB4
docks.

Link: https://lore.kernel.org/lkml/eb5a9805-3e53-ec22-696e-21c6b8cf0bfc@molgen.mpg.de/T/

> diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
> index ab9120a3127f2..6865fef4049ff 100644
> --- a/drivers/net/ethernet/intel/igc/igc_base.c
> +++ b/drivers/net/ethernet/intel/igc/igc_base.c
> @@ -447,6 +447,7 @@ bool igc_is_device_id_i226(struct igc_hw *hw)
>  	case IGC_DEV_ID_I226_V:
>  	case IGC_DEV_ID_I226_K:
>  	case IGC_DEV_ID_I226_IT:
> +	case IGC_DEV_ID_I226_LMVP:
>  		return true;
>  	default:
>  		return false;

[Severity: Medium]
Does this change do more than the L1.2 handling the commit message
describes? igc_is_device_id_i226() also selects I226-specific TSN/Qbv
register programming in igc_tsn.c. I226-LMvP now takes these paths:

drivers/net/ethernet/intel/igc/igc_tsn.c:igc_tsn_disable_offload() {
    ...
	if (igc_is_device_id_i226(hw))
		igc_tsn_restore_retx_default(adapter);
    ...
}

drivers/net/ethernet/intel/igc/igc_tsn.c:igc_tsn_enable_offload() {
    ...
	if (igc_is_device_id_i226(hw))
		igc_tsn_set_retx_qbvfullthreshold(adapter);
    ...
		if (igc_is_device_id_i226(hw)) {
			...
				tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS;
			...
			hrtimer_start(&adapter->hrtimer, expires_time, HRTIMER_MODE_REL);
		}
    ...
}

Before this patch, 0x5503 matched neither igc_is_device_id_i225() nor
igc_is_device_id_i226(). So none of the following ran on that SKU: the
RETX_CTL QBVFULLTH/QBVFULLEN workaround, the FutScdDis programming, the
Qbv hrtimer and the double BASET_L write.

The new behaviour matches the other I226 SKUs, but the subject and body
only describe an ASPM L1.2 fix. Because of the Fixes: 0325143b59c6 tag,
these TSN register changes would go into stable trees without any
mention.

The TSN part also seems to fix an omission from when
igc_is_device_id_i226() was introduced, not one from 0325143b59c6.

Could the commit message describe the TSN side effects? Alternatively,
could that part be split into a separate patch with its own Fixes: tag?

[Severity: High]
This isn't a bug introduced by this patch. However, classifying 0x5503 as
I226 also exposes I226-LMvP to an existing problem with adapter->hrtimer.

igc_tsn_enable_offload() arms adapter->hrtimer when the taprio base time
is in the future. The only hrtimer_cancel() for it is in igc_remove().

__igc_shutdown() (system and runtime suspend) and igc_reinit_queues() call
igc_clear_interrupt_scheme() without cancelling the timer:

igc_clear_interrupt_scheme()
  igc_free_q_vectors()
    adapter->num_tx_queues = 0;
    igc_reset_q_vector()
      adapter->tx_ring[...] = NULL;

The timer is not cancelled across suspend, so it can still be pending
when resume runs. On resume or reinit, the queue count is restored before
the rings exist again:

igc_init_interrupt_scheme()
  igc_set_interrupt_capability()
    adapter->num_tx_queues = adapter->rss_queues;
    pci_enable_msix_range()
  igc_alloc_q_vectors()
    /* tx_ring[] repopulated here */

The timer callback loops over num_tx_queues without a NULL check:

drivers/net/ethernet/intel/igc/igc_main.c:igc_qbv_scheduling_timer() {
    ...
	for (i = 0; i < adapter->num_tx_queues; i++) {
		struct igc_ring *tx_ring = adapter->tx_ring[i];

		if (tx_ring->admin_gate_closed) {
    ...
}

The callback takes qbv_tx_lock, but the teardown and setup paths don't.
Suppose the timer expires between igc_set_interrupt_capability() and
igc_alloc_q_vectors(), or runs at the same time as igc_free_q_vectors().
Can it then dereference a NULL tx_ring in hardirq context?

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 1fb5f3cbe93c5..6c4e046610689 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -7172,8 +7172,8 @@ static int igc_probe(struct pci_dev *pdev,
>  	hw->subsystem_vendor_id = pdev->subsystem_vendor;
>  	hw->subsystem_device_id = pdev->subsystem_device;
>  
> -	/* Disable ASPM L1.2 on I226 devices to avoid packet loss */
> -	if (igc_is_device_id_i226(hw))
> +	/* Disable ASPM L1.2 on I226 and I225-LMvP devices to avoid packet loss */
> +	if (igc_is_device_id_i226(hw) || hw->device_id == IGC_DEV_ID_I225_LMVP)
>  		pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);

[Severity: Low]
This probably isn't a bug, but the return value of pci_disable_link_state()
is ignored here. The same is true in __igc_resume(), and for
pci_disable_link_state_locked() in igc_io_slot_reset().

__pci_disable_link_state() can return -EINVAL when there is no link state
(for example with pcie_aspm=off). It can also return -EPERM when the OS
doesn't have ASPM control:

drivers/pci/pcie/aspm.c:__pci_disable_link_state() {
    ...
	if (aspm_disabled) {
		pci_warn(pdev, "can't disable ASPM; OS doesn't have ASPM control\n");
		return -EPERM;
	}
    ...
}

In those cases L1.2 stays as firmware configured it on the newly covered
I225-LMvP and I226-LMvP devices.

The PCI core already warns on the -EPERM path. Failing probe, resume or
recovery would leave the NIC unusable, and the existing I226 handling
does the same thing. Is continuing silently here the intended behaviour?

> @@ -7559,7 +7559,7 @@ static int __igc_resume(struct device *dev, bool rpm)
>  	pci_enable_wake(pdev, PCI_D3hot, 0);
>  	pci_enable_wake(pdev, PCI_D3cold, 0);
>  
> -	if (igc_is_device_id_i226(hw))
> +	if (igc_is_device_id_i226(hw) || hw->device_id == IGC_DEV_ID_I225_LMVP)
>  		pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);

[Severity: High]
Can this take pci_bus_sem recursively during AER recovery?

__igc_resume() is also the runtime resume path, and igc runtime suspends
after link loss via igc_runtime_idle(). pcie_do_recovery() runtime
resumes devices from inside a bus walk that holds pci_bus_sem for read:

pcie_do_recovery()
  pci_walk_bridge(bridge, pci_pm_runtime_get_sync, NULL)
    pci_walk_bus()
      down_read(&pci_bus_sem);
      pci_pm_runtime_get_sync()
        pm_runtime_get_sync()
          igc_runtime_resume()
            __igc_resume(dev, true)
              pci_disable_link_state()
                __pci_disable_link_state()
                  down_read(&pci_bus_sem);

A writer could queue between the two down_read() calls, for example
pci_device_add() from enumeration or hotplug, which is plausible behind a
Thunderbolt dock. The nested reader would then block behind the writer,
while the writer waits for the outer reader. Would that deadlock AER
recovery and PCI enumeration? Lockdep would also be expected to report
recursive locking here.

igc_io_slot_reset() already uses pci_disable_link_state_locked() for this
reason. The runtime resume step earlier in recovery isn't covered,
though.

The other I226 IDs already have the same pattern, and this patch extends
it to both LMvP devices. Could it be fixed for all of them at the same
time?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925112850.189302-1-danividanivi%40gmail.com