Re: [PATCH] igc: fix disabling L1.2 PCI-E link substate on I226 on init

Re: [PATCH] igc: fix disabling L1.2 PCI-E link substate on I226 on init

[Paul Menzel]

Dear ValdikSS,


Thank you for your patch. Please make sure to use `git format-patch 
-v<N>` (reroll count) to mark the iteration of the patch.

Am 27.07.25 um 22:43 schrieb ValdikSS:
> Device ID comparison in igc_is_device_id_i226 is performed before
> the ID is set, resulting in always failing check on init.
> 
> Before the patch:
> * L1.2 is not disabled on init
> * L1.2 is properly disabled after suspend-resume cycle
> 
> With the patch:
> * L1.2 is properly disabled both on init and after suspend-resume
> 
> How to test:
> Connect to the 1G link with 300+ mbit/s Internet speed, and run
> the download speed test, such as:
> 
>      curl -o /dev/null http://speedtest.selectel.ru/1GB
> 
> Without L1.2 disabled, the speed would be no more than ~200 mbit/s.
> With L1.2 disabled, the speed would reach 1 gbit/s.
> Note: it's required that the latency between your host and the remote
> be around 3-5 ms, the test inside LAN (<1 ms latency) won't trigger the
> issue.

`sudo lspci -vv -s <x>` can be used to check L1.2 enablement under 
`L1SubCtl1`.

> 
> Link: https://lore.kernel.org/intel-wired-lan/15248b4f-3271-42dd-8e35-02bfc92b25e1@intel.com
> Fixes: 0325143b59c6 ("igc: disable L1.2 PCI-E link substate to avoid performance issue")
> Signed-off-by: ValdikSS <iam@valdikss.org.ru>
> Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 031c332f6..1b4465d6b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -7115,6 +7115,13 @@ static int igc_probe(struct pci_dev *pdev,
>   	adapter->port_num = hw->bus.func;
>   	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
>   
> +	/* PCI config space info */
> +	hw->vendor_id = pdev->vendor;
> +	hw->device_id = pdev->device;
> +	hw->revision_id = pdev->revision;
> +	hw->subsystem_vendor_id = pdev->subsystem_vendor;
> +	hw->subsystem_device_id = pdev->subsystem_device;
> +
>   	/* Disable ASPM L1.2 on I226 devices to avoid packet loss */
>   	if (igc_is_device_id_i226(hw))
>   		pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> @@ -7141,13 +7148,6 @@ static int igc_probe(struct pci_dev *pdev,
>   	netdev->mem_start = pci_resource_start(pdev, 0);
>   	netdev->mem_end = pci_resource_end(pdev, 0);
>   
> -	/* PCI config space info */
> -	hw->vendor_id = pdev->vendor;
> -	hw->device_id = pdev->device;
> -	hw->revision_id = pdev->revision;
> -	hw->subsystem_vendor_id = pdev->subsystem_vendor;
> -	hw->subsystem_device_id = pdev->subsystem_device;
> -
>   	/* Copy the default MAC and PHY function pointers */
>   	memcpy(&hw->mac.ops, ei->mac_ops, sizeof(hw->mac.ops));
>   	memcpy(&hw->phy.ops, ei->phy_ops, sizeof(hw->phy.ops));

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [PATCH] igc: fix disabling L1.2 PCI-E link substate on I226 on init

[ValdikSS]

[-- Attachment #1.1: Type: text/plain, Size: 3651 bytes --]

On 28.07.2025 10:03 AM, Paul Menzel wrote:
> Dear ValdikSS,
> 
> 
> Thank you for your patch. Please make sure to use `git format-patch - 
> v<N>` (reroll count) to mark the iteration of the patch.
> 
> Am 27.07.25 um 22:43 schrieb ValdikSS:
>> Device ID comparison in igc_is_device_id_i226 is performed before
>> the ID is set, resulting in always failing check on init.
>>
>> Before the patch:
>> * L1.2 is not disabled on init
>> * L1.2 is properly disabled after suspend-resume cycle
>>
>> With the patch:
>> * L1.2 is properly disabled both on init and after suspend-resume
>>
>> How to test:
>> Connect to the 1G link with 300+ mbit/s Internet speed, and run
>> the download speed test, such as:
>>
>>      curl -o /dev/null http://speedtest.selectel.ru/1GB
>>
>> Without L1.2 disabled, the speed would be no more than ~200 mbit/s.
>> With L1.2 disabled, the speed would reach 1 gbit/s.
>> Note: it's required that the latency between your host and the remote
>> be around 3-5 ms, the test inside LAN (<1 ms latency) won't trigger the
>> issue.
> 
> `sudo lspci -vv -s <x>` can be used to check L1.2 enablement under 
> `L1SubCtl1`.

Is this what you suggest me to include and send the patch again?
I'd prefer not to be involved in the bug fixing process. I just sent a 
patch because it works for me, as the suggestion for the fix. I did not 
know it would require me to be involved in the process.

> 
>>
>> Link: https://lore.kernel.org/intel-wired- 
>> lan/15248b4f-3271-42dd-8e35-02bfc92b25e1@intel.com
>> Fixes: 0325143b59c6 ("igc: disable L1.2 PCI-E link substate to avoid 
>> performance issue")
>> Signed-off-by: ValdikSS <iam@valdikss.org.ru>
>> Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_main.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ 
>> ethernet/intel/igc/igc_main.c
>> index 031c332f6..1b4465d6b 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -7115,6 +7115,13 @@ static int igc_probe(struct pci_dev *pdev,
>>       adapter->port_num = hw->bus.func;
>>       adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
>> +    /* PCI config space info */
>> +    hw->vendor_id = pdev->vendor;
>> +    hw->device_id = pdev->device;
>> +    hw->revision_id = pdev->revision;
>> +    hw->subsystem_vendor_id = pdev->subsystem_vendor;
>> +    hw->subsystem_device_id = pdev->subsystem_device;
>> +
>>       /* Disable ASPM L1.2 on I226 devices to avoid packet loss */
>>       if (igc_is_device_id_i226(hw))
>>           pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>> @@ -7141,13 +7148,6 @@ static int igc_probe(struct pci_dev *pdev,
>>       netdev->mem_start = pci_resource_start(pdev, 0);
>>       netdev->mem_end = pci_resource_end(pdev, 0);
>> -    /* PCI config space info */
>> -    hw->vendor_id = pdev->vendor;
>> -    hw->device_id = pdev->device;
>> -    hw->revision_id = pdev->revision;
>> -    hw->subsystem_vendor_id = pdev->subsystem_vendor;
>> -    hw->subsystem_device_id = pdev->subsystem_device;
>> -
>>       /* Copy the default MAC and PHY function pointers */
>>       memcpy(&hw->mac.ops, ei->mac_ops, sizeof(hw->mac.ops));
>>       memcpy(&hw->phy.ops, ei->phy_ops, sizeof(hw->phy.ops));
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] igc: fix disabling L1.2 PCI-E link substate on I226 on init

[Paul Menzel]

Dear ValdikSS,


Am 28.07.25 um 09:24 schrieb ValdikSS:
> On 28.07.2025 10:03 AM, Paul Menzel wrote:

>> Thank you for your patch. Please make sure to use `git format-patch - 
>> v<N>` (reroll count) to mark the iteration of the patch.
>>
>> Am 27.07.25 um 22:43 schrieb ValdikSS:
>>> Device ID comparison in igc_is_device_id_i226 is performed before
>>> the ID is set, resulting in always failing check on init.
>>>
>>> Before the patch:
>>> * L1.2 is not disabled on init
>>> * L1.2 is properly disabled after suspend-resume cycle
>>>
>>> With the patch:
>>> * L1.2 is properly disabled both on init and after suspend-resume
>>>
>>> How to test:
>>> Connect to the 1G link with 300+ mbit/s Internet speed, and run
>>> the download speed test, such as:
>>>
>>>      curl -o /dev/null http://speedtest.selectel.ru/1GB
>>>
>>> Without L1.2 disabled, the speed would be no more than ~200 mbit/s.
>>> With L1.2 disabled, the speed would reach 1 gbit/s.
>>> Note: it's required that the latency between your host and the remote
>>> be around 3-5 ms, the test inside LAN (<1 ms latency) won't trigger the
>>> issue.
>>
>> `sudo lspci -vv -s <x>` can be used to check L1.2 enablement under 
>> `L1SubCtl1`.
> 
> Is this what you suggest me to include and send the patch again?

I’d have added it, but others might say, it’s common knowledge, so *no 
need* to resend, no that it’s also documented in the list archive.

(Did you check, that L1.2 PCI-E link substate is disabled, or just the 
speed check.)

> I'd prefer not to be involved in the bug fixing process. I just sent a 
> patch because it works for me, as the suggestion for the fix. I did not 
> know it would require me to be involved in the process.

Yeah, it can be tedious. Sorry about that. As you sent the message 
tagged with [PATCH], I’d assumed you want it to be committed. (And it 
looks like it’s going to be as it adheres to all the requirements.) 
Thank you again!

>>> Link: https://lore.kernel.org/intel-wired-lan/15248b4f-3271-42dd-8e35-02bfc92b25e1@intel.com
>>> Fixes: 0325143b59c6 ("igc: disable L1.2 PCI-E link substate to avoid performance issue")
>>> Signed-off-by: ValdikSS <iam@valdikss.org.ru>
>>> Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/igc/igc_main.c | 14 +++++++-------
>>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>>> index 031c332f6..1b4465d6b 100644
>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>>> @@ -7115,6 +7115,13 @@ static int igc_probe(struct pci_dev *pdev,
>>>       adapter->port_num = hw->bus.func;
>>>       adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
>>> +    /* PCI config space info */
>>> +    hw->vendor_id = pdev->vendor;
>>> +    hw->device_id = pdev->device;
>>> +    hw->revision_id = pdev->revision;
>>> +    hw->subsystem_vendor_id = pdev->subsystem_vendor;
>>> +    hw->subsystem_device_id = pdev->subsystem_device;
>>> +
>>>       /* Disable ASPM L1.2 on I226 devices to avoid packet loss */
>>>       if (igc_is_device_id_i226(hw))
>>>           pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>>> @@ -7141,13 +7148,6 @@ static int igc_probe(struct pci_dev *pdev,
>>>       netdev->mem_start = pci_resource_start(pdev, 0);
>>>       netdev->mem_end = pci_resource_end(pdev, 0);
>>> -    /* PCI config space info */
>>> -    hw->vendor_id = pdev->vendor;
>>> -    hw->device_id = pdev->device;
>>> -    hw->revision_id = pdev->revision;
>>> -    hw->subsystem_vendor_id = pdev->subsystem_vendor;
>>> -    hw->subsystem_device_id = pdev->subsystem_device;
>>> -
>>>       /* Copy the default MAC and PHY function pointers */
>>>       memcpy(&hw->mac.ops, ei->mac_ops, sizeof(hw->mac.ops));
>>>       memcpy(&hw->phy.ops, ei->phy_ops, sizeof(hw->phy.ops));
>>
>> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Kind regards,

Paul