On 6/5/2025 11:36 AM, Paul Menzel wrote:
[two additions]
Am 05.06.25 um 10:26 schrieb Paul Menzel:
[Cc: [email protected]]
Remove again, as it does not seem valid anymore.
Dear Vitaly,
Thank you for your patch.
Am 05.06.25 um 09:06 schrieb Vitaly Lifshits:
I226 devices advertise support for the PCI-E link L1.2 substate.
However,
due to a hardware limitation, the exit latency from this low-power state
is longer than the packet buffer can tolerate under high traffic
conditions. This can lead to packet loss and degraded performance.
It’d be great if you could add details about the exit latency times.
This was established during internal validation and, unfortunately, I do
not have public documents
Despite, you referencing some of the problem report in the Link: tag,
that message is badly formatted, as lines are wrapped. As you have the
details about the problem currently in your had, it’d be great if you
added the report details too. Another advantage is, that the commit
message would be self-contained, and people would get more idea about
it without requiring Internet access or loading an HTML page.
In my commit message I referred to the general issue which is the packet
loss. The packet loss exact numbers may vary depending on the system and
its link partner, so they don’t play a significant role. Therefore,
elaborating on a specific setup doesn't play a crucial role here and
other similar reports can be found in the web.
To mitigate this, disable the L1.2 substate during both probe and resume
flows.
Were you able to reproduce the problem or only the reporter?
Yes, we reproduced the issue on different platforms and operating systems.
Also the power usage implications should be documented.
The power consumption is insignificant and this too may vary between
different systems, I’ll add a comment about it in the next revision of
the patch.
Link: https://lore.kernel.org/intel-wired-
lan/[email protected]
Signed-off-by: Vitaly Lifshits <[email protected]>
Fixes: 43546211738e ("igc: Add new device ID's")
---
drivers/net/ethernet/intel/igc/igc_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/
ethernet/intel/igc/igc_main.c
index 27575a1e1777..65ec705eac33 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7074,6 +7074,8 @@ static int igc_probe(struct pci_dev *pdev,
const struct igc_info *ei = igc_info_tbl[ent->driver_data];
int err;
+ pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
+
Aren’t you disabling this unconditionally now? Also, a comment would
be nice, why this is disabled.
Yes, initially I disabled it unconditionally because I225 devices don’t
support this link state anyway, so disabling it won’t have any affect on
them.
Nevertheless I decided to wrap this code to be applied only on I226
devices so that it won't affect any future devices using this code.
err = pci_enable_device_mem(pdev);
if (err)
return err;
@@ -7498,6 +7500,8 @@ static int __igc_resume(struct device *dev,
bool rpm)
pci_enable_wake(pdev, PCI_D3hot, 0);
pci_enable_wake(pdev, PCI_D3cold, 0);ValdikSS <[email protected].
+ pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
+
if (igc_init_interrupt_scheme(adapter, true)) {
netdev_err(netdev, "Unable to allocate memory for queues\n");
return -ENOMEM;
@@ -7623,6 +7627,7 @@ static pci_ers_result_t
igc_io_slot_reset(struct pci_dev *pdev)
pci_enable_wake(pdev, PCI_D3hot, 0);
pci_enable_wake(pdev, PCI_D3cold, 0);
+ pci_disable_link_state_locked(pdev, PCIE_LINK_STATE_L1_2);
/* In case of PCI error, adapter loses its HW address
* so we should re-assign it here.
*/
Kind regards,
Paul
