Bjorn Helgaas <[email protected]> writes:
> 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.
>
Yeah, most probably I should have written that commit message better :-)
>From what I remember, the issue was on the other side, even if i225/i226
sending PTM requests when bus mastering is disabled is fine, the side
receiving those requests weren't fine with that. And that caused some
stuck systems. (perhaps I should have focused on this when writing the
commit message)
>> > > 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]
>
> Thanks for that!
>
> Bjorn
Cheers,
--
Vinicius
