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

[Bjorn Helgaas <helgaas@kernel.org>]

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?

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

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

If this is a documented erratum, can we include an identifier or a
pointer to it?

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

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

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

> Fixes: e1092d5e15e6 ("PCI/PTM: Do not enable PTM automatically for Root and Switch Upstream Ports")
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Co-developed-by: Li Lili <lili.li@intel.com>
> Signed-off-by: Li Lili <lili.li@intel.com>
> Signed-off-by: Li Yifan <yifan2.li@intel.com>
> Tested-by: Wang Jie <jie.wang@intel.com>
> ---
>  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.

> +		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()?

>  	igc_clear_interrupt_scheme(adapter);
>  	pci_iounmap(pdev, adapter->io_addr);
>  	pci_release_mem_regions(pdev);
> 
> base-commit: c550c1c9727726dcae121221c252013153fa5945
> -- 
> 2.43.0
>