Re: [PATCH iwl-net] igc: Disable PTM after the last device reset on remove

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Nov 04, 2026 at 02:22:40PM +0000, Li, Yifan2 wrote:
> On Mon, Nov 02, 2026, Bjorn Helgaas wrote:
> > On Sat, Oct 31, 2026 at 10:19:28AM +0800, Li Yifan wrote:
> ...

> > > 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.

I don't think the PCIe spec includes any generic way to trigger PTM
dialogs, and in general, I think devices are permitted to initiate
them at any time, independent of driver or other events.  So I think
igc has device-specific knowledge that the NIC *only* initiates PTM
dialogs when a driver sets the TRIG bit.  If so, I think that's worth
a mention.

I haven't worked through the steps you outlined, but maybe they
explain why there's no PTM Request after the later reset.

> > > 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.

I think that part of the c080fe262f9e commit log is just wrong.  The
fact that i225/i226 can make PTM requests when bus mastering is
disabled is perfectly fine; it's not a hardware issue.  I *suspect*
that only the pci_disable_ptm() in that commit is actually required.

> > > 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/20261104121457.2108515-1-yifan2.li@intel.com

Thanks for that!

Bjorn