Hi Bjorn,
Thanks for the review.
On Mon, Nov 02, 2026, Bjorn Helgaas wrote:
>
> On Sat, Oct 31, 2026 at 10:19:28AM +0800, Li Yifan wrote:
> > I225/I226 have a hardware issue: after a PTM dialog, the next device
> > reset through the RST bit of the Device Control register (CTRL.RST)
>
> I assume this Device Control is a device-specific register, not the Device
> Control
> in the PCIe Capability, right?
Yes. It is the device-specific CTRL register in MMIO space (IGC_CTRL,
offset 0x0, RST is bit 26). I took the name "Device Control" from the
comment for IGC_CTRL in igc_regs.h. v2 will call it "the
device-specific CTRL register".
>
> > can make the device send a PTM Request. The device sends it even if
> > PTM is disabled in its PTM Control register (PCI config space), the EN
> > bit of its PTM_CTRL register (MMIO) is clear
>
> And I guess PTM_CTRL is another device-specific register. The architected PTM
> Enable bit in the PTM Capability in config space is supposed to be enough to
> prevent the device from issuing PTM Requests, so I guess the device-specific
> control is only relevant because it's not enough to work around this hardware
> defect.
Yes. PTM_CTRL is also in MMIO space (offset 0x12540, EN is bit 30).
The defect is that the device sends a PTM Request while the PTM Enable
bit in its PTM Capability is clear. When the request comes,
pci_disable_ptm() has already cleared that bit. I mentioned
PTM_CTRL.EN only to show that the driver has no other control that
stops the request. v2 will say this directly.
>
> > and Bus Master Enable is
> > clear, so the driver cannot stop it. Commit c080fe262f9e ("igc: Fix
> > possible system crash when loading module") disables PTM before bus
> > mastering because of a related issue (PTM Requests with bus mastering
> > disabled), but that does not stop the request at CTRL.RST.
>
> c080fe262f9e and this both mention Bus Master Enable, but I don't think it's
> relevant to either change. Bus Master Enable controls ability to issue Memory
> and I/O Read/Write Requests. PTM Requests are PCIe Messages (not
> Read/Write Requests) so Bus Master Enable should not affect them.
Agreed. Our data confirms that the I226 sent the request with Bus Master
Enable clear. v2 will remove Bus Master Enable from the description.
>
> If this is a documented erratum, can we include an identifier or a pointer to
> it?
I am working with the I225/I226 hardware team to find out if there is an
erratum for this. If there is a public one, I will cite it in v2.
>
> > The request is harmless while the upstream port has PTM enabled. Since
> > commit e1092d5e15e6 ("PCI/PTM: Do not enable PTM automatically for
> > Root and Switch Upstream Ports"), pci_disable_ptm() also disables PTM
> > on the upstream port when the last user goes away. igc_remove() calls
> > pci_disable_ptm() before unregister_netdev(). If the interface is up,
> > unregister_netdev() then resets the device through igc_close() ->
> > igc_down() -> igc_reset(). The upstream port now handles the PTM
> > Request as an Unsupported Request. On an Intel Nova Lake-S system with
> > an I226-LM, this triggers DPC on "modprobe -r igc". Recovery fails
> > because the driver is unbinding, and the next probe fails:
> >
> > pcieport 0000:80:1d.0: DPC: containment event, status:0x1f01: unmasked
> uncorrectable error detected
> > pcieport 0000:80:1d.0: [20] UnsupReq (First)
> > pcieport 0000:80:1d.0: AER: TLP Header: 0x34000000 0x81000052
> 0x00000000 0x00000000
> >
> > Message code 0x52 in the TLP header is a PTM Request from 81:00.0.
> >
> > Reset the device while the upstream port still has PTM enabled, so
> > that the PTM Request gets a response. Then disable PTM and bus
> > mastering. With this order, a later reset sends no PTM Request (tested
> > on I226-LM). igc_ptp_stop() has already cleared IGC_PTP_ENABLED, so no
> > new PTM dialog starts after this reset.
>
> IGC_PTP_ENABLED is just a software flag, and it appears to be related to PTP,
> not directly related to PTM, so there are some dots missing between it and
> hardware PTM messages.
Sorry, I skipped steps. In igc, PTM is used only for PTP cross
timestamps. The driver starts PTM dialogs only from the PTP code, by
setting the TRIG bit in PTM_CTRL:
- igc_ptp_reset() sets PTM_CTRL.EN and triggers the first dialog.
- igc_ptp_getcrosststamp() triggers dialogs for each cross timestamp
request.
igc_ptp_stop() runs before the reset and stops both:
1. igc_ptp_suspend() -> igc_ptm_stop() clears PTM_CTRL.EN.
2. ptp_clock_unregister() removes the PHC and waits for the
getcrosststamp() calls in progress, so no more calls come.
3. It clears IGC_PTP_ENABLED. igc_reset() calls igc_ptp_reset(),
which returns early when this flag is clear, so the reset path does
not set PTM_CTRL.EN again.
So IGC_PTP_ENABLED does not control the hardware directly. It only
stops the driver from setting PTM_CTRL.EN again. v2 will describe
these steps.
>
> > If the interface is down,
> > unregister_netdev() does not reset the device, so reset it explicitly.
> > PTM is still disabled before bus mastering, as the related fix above
> > requires.
>
> Which fix requires PTM disable before bus mastering disable? And why is that
> required?
c080fe262f9e. Its commit message says: "Avoid an i225/i226 hardware
issue that PTM requests can be made even though PCI bus mastering is
not enabled. These unexpected PTM requests can crash some systems."
The order is a workaround for that I225/I226 hardware issue, not a
PCIe spec requirement. The commit message does not say more about
why the order matters, but when I checked with the hardware team, I
found an internal erratum for that issue.
I found c080fe262f9e when I searched the igc history for this PTM
problem. Our issue is different: after the device has done a PTM
dialog, the next CTRL.RST can make it send a PTM Request, even when
PTM Enable is clear.
>
> > Skip the reset if the device cannot respond: error recovery is in
> > progress or has failed (pci_channel_offline()), the device is gone
> > (pci_device_is_present()), or igc_rd32() has already reported "PCIe
> > link lost" (IGC_REMOVED()). Otherwise the reset accesses a device in
> > error recovery, or polls all-ones register values for about two
> > seconds until igc_disable_pcie_master() times out.
> >
> > Call pci_disable_ptm() only if PTM is enabled. pci_enable_ptm() can
> > fail in probe, for example if the upstream port does not support PTM,
> > and probe continues without PTM. The PCI/PTM change above also made
> > pci_disable_ptm() use a reference count: it decrements
> > dev->ptm_enable_cnt without a check, and then does the same for the
> > upstream port. Before, it did nothing if PTM was not enabled. After a
> > failed pci_enable_ptm(), the count of the device becomes negative and
> > stays negative after the driver unbinds. pcie_ptm_enabled() returns
> > true for a negative count, so after the next probe
> > igc_is_crosststamp_supported() reports PTM support and igc_ptp_reset()
> > starts PTM cycles on a device that does not have PTM enabled. Commit
> > 650f197d8ea6 ("idpf: disable PTM on probe failure and on remove") adds
> > the same check to idpf.
>
> This sounds like a bug in the PCI core PTM support, i.e., maybe
> pci_disable_ptm() should be checking for PTM being enabled?
> Otherwise I'm afraid lots of drivers might trip over this.
Agree and thanks! I sent a PCI core fix:
https://lore.kernel.org/r/[email protected]
v2 will replace this paragraph with a short link to the core fix, and will drop
the pcie_ptm_enabled() check.
>
> > Fixes: e1092d5e15e6 ("PCI/PTM: Do not enable PTM automatically for
> > Root and Switch Upstream Ports")
> > Suggested-by: Mika Westerberg <[email protected]>
> > Co-developed-by: Li Lili <[email protected]>
> > Signed-off-by: Li Lili <[email protected]>
> > Signed-off-by: Li Yifan <[email protected]>
> > Tested-by: Wang Jie <[email protected]>
> > ---
> > drivers/net/ethernet/intel/igc/igc_main.c | 19 ++++++++++++++++---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> > b/drivers/net/ethernet/intel/igc/igc_main.c
> > index b8ac2f4fea5a..3ee8b05f0515 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -7406,9 +7406,6 @@ static void igc_remove(struct pci_dev *pdev)
> >
> > igc_ptp_stop(adapter);
> >
> > - pci_disable_ptm(pdev);
> > - pci_clear_master(pdev);
> > -
> > set_bit(__IGC_DOWN, &adapter->state);
> >
> > timer_delete_sync(&adapter->watchdog_timer);
> > @@ -7427,6 +7424,22 @@ static void igc_remove(struct pci_dev *pdev)
> > igc_release_hw_control(adapter);
> > unregister_netdev(netdev);
> >
> > + /* After a PTM dialog, I225/I226 can send a PTM Request at the next
> > + * CTRL.RST, even with PTM and bus mastering disabled on the device.
> > + * Reset the device while PTM is still enabled on the upstream port,
> > + * so that the request gets a response. unregister_netdev() resets the
> > + * device only if the interface was up. Skip the reset if the device
> > + * cannot respond (error recovery, surprise removal or an earlier
> > + * "PCIe link lost"). Then disable PTM before bus mastering.
> > + */
> > + if (!pci_channel_offline(pdev) && pci_device_is_present(pdev) &&
> > + !IGC_REMOVED(adapter->hw.hw_addr))
>
> All these tests look racy. .reset_hw() really should be able to detect and
> deal
> with any errors that happen if it tries to access a device that is no longer
> responding.
Yes, the current checks are racy and too complicated. v2 removes
pci_device_is_present() and IGC_REMOVED() and keeps only
!pci_channel_offline(), the same check that igc_down() does before
igc_reset(). It only skips a reset that cannot work.
>
> > + adapter->hw.mac.ops.reset_hw(&adapter->hw);
> > +
> > + if (pcie_ptm_enabled(pdev))
> > + pci_disable_ptm(pdev);
> > + pci_clear_master(pdev);
>
> I don't think bus mastering has any connection to PTM, so why are we moving
> pci_clear_master()?
You are right. v2 leaves pci_clear_master() where it is and moves only
pci_disable_ptm(). I will test this order on the I226 before I send v2.
>
> > igc_clear_interrupt_scheme(adapter);
> > pci_iounmap(pdev, adapter->io_addr);
> > pci_release_mem_regions(pdev);
> >
> > base-commit: c550c1c9727726dcae121221c252013153fa5945
> > --
> > 2.43.0
> >
