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

[ValdikSS <iam@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx
> > Fixes: 0325143b59c6 ("igc: disable L1.2 PCI-E link substate to avoid 
> > performance issue")
> > Signed-off-by: ValdikSS <iam@xxxxxxxxxxxxxxx>
> > Reviewed-by: Vitaly Lifshits <vitaly.lifshits@xxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx>
>
>
> Kind regards,
>
> Paul

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature