[PATCH net v2] r8169: don't enable chip LTR when the platform has not enabled LTR

[PATCH net v2] r8169: don't enable chip LTR when the platform has not enabled LTR

[Yogesh Gaur]

rtl_enable_ltr() programs the MAC to generate LTR messages - ALDPS_LTR_EN,
LTR_SNOOP_EN, LTR_OBFF_LOCK_EN, plus LINK_SPEED_CHANGE_EN on
RTL8125/RTL8126/RTL8127 - and rtl_hw_aspm_clkreq_enable() calls it on
every ASPM enable, then goes on to let the chip trigger L1.2.

The only gate is tp->aspm_manageable, which records that the OS is allowed
to control ASPM. It says nothing about LTR. LTR is a separate PCIe
capability that only works if every device on the path to the root port
supports it. The PCI core works that out in pci_configure_ltr() and
records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
function must not issue LTR messages while LTR Mechanism Enable is clear.

So on a platform whose hierarchy has no LTR path, the driver tells the
chip to start sending LTR messages nothing will honour, and ties ALDPS -
the PHY's link-down power saving - to them. A report against RTL8125B
(rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
link down/up transitions in one eight-hour boot, with repeated downshifts
to 100Mbps, against four transitions at boot and then a stable link on
the kernel before the LTR change.

Gate the chip's LTR programming on pci_dev->ltr_path. That field lives
inside CONFIG_PCIEASPM, and without ASPM support there is nothing here to
enable LTR for, so compile the function out in that configuration.

Note this does change CONFIG_PCIEASPM=n builds, which used to program
chip LTR unconditionally: pci_disable_link_state() is a stub that returns
success there, so tp->aspm_manageable ends up set. The PCI core does not
configure LTR in that configuration either.

Fixes: 9ab94a32af70 ("r8169: enable LTR support")
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2529752
Assisted-by: LLM
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
v2:
 - Gate on pci_dev->ltr_path instead of reading PCI_EXP_DEVCTL2 directly,
   and compile rtl_enable_ltr() out for CONFIG_PCIEASPM=n, where that
   field does not exist and there is nothing to enable LTR for.
   Suggested by Heiner Kallweit.
 - Commit message notes the resulting change for CONFIG_PCIEASPM=n builds.
 - No change to the RTL8125 register programming itself.

v1: https://lore.kernel.org/all/20260909110554.1977-1-yogeshgaur.83@gmail.com/

 drivers/net/ethernet/realtek/r8169_main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index ec4fc21fa21f..05f7a44aff01 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -3035,8 +3035,16 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
 	}
 }
 
+#ifdef CONFIG_PCIEASPM
 static void rtl_enable_ltr(struct rtl8169_private *tp)
 {
+	/* The chip must not issue LTR messages unless LTR is enabled on the
+	 * whole path up to the root port. pci_configure_ltr() works that out
+	 * and records the result in pci_dev->ltr_path.
+	 */
+	if (!tp->pci_dev->ltr_path)
+		return;
+
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_80:
 		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
@@ -3120,6 +3128,11 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
 	/* chip can trigger LTR */
 	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);
 }
+#else
+static void rtl_enable_ltr(struct rtl8169_private *tp)
+{
+}
+#endif
 
 static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 {
-- 
2.34.1

Re: [PATCH net v2] r8169: don't enable chip LTR when the platform has not enabled LTR

[Heiner Kallweit]

On 10.09.2026 15:01, Yogesh Gaur wrote:
> rtl_enable_ltr() programs the MAC to generate LTR messages - ALDPS_LTR_EN,
> 

Empty lines, mail formatting issue?

> LTR_SNOOP_EN, LTR_OBFF_LOCK_EN, plus LINK_SPEED_CHANGE_EN on
> 
> RTL8125/RTL8126/RTL8127 - and rtl_hw_aspm_clkreq_enable() calls it on
> 
> every ASPM enable, then goes on to let the chip trigger L1.2.
> 
> 
> 
> The only gate is tp->aspm_manageable, which records that the OS is allowed
> 
> to control ASPM. It says nothing about LTR. LTR is a separate PCIe
> 
> capability that only works if every device on the path to the root port
> 
> supports it. The PCI core works that out in pci_configure_ltr() and
> 
> records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
> 
> function must not issue LTR messages while LTR Mechanism Enable is clear.
> 
> 
> 
> So on a platform whose hierarchy has no LTR path, the driver tells the
> 
> chip to start sending LTR messages nothing will honour, and ties ALDPS -
> 
> the PHY's link-down power saving - to them. A report against RTL8125B
> 
> (rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291

Did you verify that this is caused by LTR messages, and not by any other
root cause?

> 
> link down/up transitions in one eight-hour boot, with repeated downshifts
> 
> to 100Mbps, against four transitions at boot and then a stable link on
> 
> the kernel before the LTR change.
> 
> 
> 
> Gate the chip's LTR programming on pci_dev->ltr_path. That field lives
> 
> inside CONFIG_PCIEASPM, and without ASPM support there is nothing here to
> 
> enable LTR for, so compile the function out in that configuration.
> 
> 
> 
> Note this does change CONFIG_PCIEASPM=n builds, which used to program
> 
> chip LTR unconditionally: pci_disable_link_state() is a stub that returns
> 
> success there, so tp->aspm_manageable ends up set. The PCI core does not
> 
> configure LTR in that configuration either.
> 
> 
> 
> Fixes: 9ab94a32af70 ("r8169: enable LTR support")
> 
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2529752
 
Did you verify that your patch fixes the reported issue? Or do you
just assume this? Because in the bug comment history I don't see
a statement that issue has been resolved.

> Assisted-by: LLM
> 
> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
> 
> ---
> 
> v2:
> 
>  - Gate on pci_dev->ltr_path instead of reading PCI_EXP_DEVCTL2 directly,
> 
>    and compile rtl_enable_ltr() out for CONFIG_PCIEASPM=n, where that
> 
>    field does not exist and there is nothing to enable LTR for.
> 
>    Suggested by Heiner Kallweit.
> 
>  - Commit message notes the resulting change for CONFIG_PCIEASPM=n builds.
> 
>  - No change to the RTL8125 register programming itself.
> 
> 
> 
> v1: https://lore.kernel.org/all/20260909110554.1977-1-yogeshgaur.83@gmail.com/
> 
> 
> 
>  drivers/net/ethernet/realtek/r8169_main.c | 13 +++++++++++++
> 
>  1 file changed, 13 insertions(+)
> 
> 
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> 
> index ec4fc21fa21f..05f7a44aff01 100644
> 
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> 
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> 
> @@ -3035,8 +3035,16 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
> 
>  	}
> 
>  }
> 
>  
> 
> +#ifdef CONFIG_PCIEASPM
> 
>  static void rtl_enable_ltr(struct rtl8169_private *tp)
> 
>  {
> 
> +	/* The chip must not issue LTR messages unless LTR is enabled on the
> 
> +	 * whole path up to the root port. pci_configure_ltr() works that out
> 
> +	 * and records the result in pci_dev->ltr_path.
> 
> +	 */
> 
> +	if (!tp->pci_dev->ltr_path)

Simpler would be:
if (!IS_ENABLED(CONFIG_PCIEASPM) || !tp->pci_dev->ltr_path)

Then you don't need the stub function.

> 
> +		return;
> 
> +
> 
>  	switch (tp->mac_version) {
> 
>  	case RTL_GIGA_MAC_VER_80:
> 
>  		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> 
> @@ -3120,6 +3128,11 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
> 
>  	/* chip can trigger LTR */
> 
>  	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);
> 
>  }
> 
> +#else
> 
> +static void rtl_enable_ltr(struct rtl8169_private *tp)
> 
> +{
> 
> +}
> 
> +#endif
> 
>  
> 
>  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> 
>  {
> 

Re: [PATCH net v2] r8169: don't enable chip LTR when the platform has not enabled LTR

[Yogesh Gaur]

On Fri, Sep 11, 2026 at 1:03 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 10.09.2026 15:01, Yogesh Gaur wrote:
> > rtl_enable_ltr() programs the MAC to generate LTR messages - ALDPS_LTR_EN,
> >
>
> Empty lines, mail formatting issue?
Yes, I would correct it in v3.

>
> > LTR_SNOOP_EN, LTR_OBFF_LOCK_EN, plus LINK_SPEED_CHANGE_EN on
> >
> > RTL8125/RTL8126/RTL8127 - and rtl_hw_aspm_clkreq_enable() calls it on
> >
> > every ASPM enable, then goes on to let the chip trigger L1.2.
> >
> >
> >
> > The only gate is tp->aspm_manageable, which records that the OS is allowed
> >
> > to control ASPM. It says nothing about LTR. LTR is a separate PCIe
> >
> > capability that only works if every device on the path to the root port
> >
> > supports it. The PCI core works that out in pci_configure_ltr() and
> >
> > records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
> >
> > function must not issue LTR messages while LTR Mechanism Enable is clear.
> >
> >
> >
> > So on a platform whose hierarchy has no LTR path, the driver tells the
> >
> > chip to start sending LTR messages nothing will honour, and ties ALDPS -
> >
> > the PHY's link-down power saving - to them. A report against RTL8125B
> >
> > (rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
>
> Did you verify that this is caused by LTR messages, and not by any other
> root cause?
>
> >
> > link down/up transitions in one eight-hour boot, with repeated downshifts
> >
> > to 100Mbps, against four transitions at boot and then a stable link on
> >
> > the kernel before the LTR change.
> >
> >
> >
> > Gate the chip's LTR programming on pci_dev->ltr_path. That field lives
> >
> > inside CONFIG_PCIEASPM, and without ASPM support there is nothing here to
> >
> > enable LTR for, so compile the function out in that configuration.
> >
> >
> >
> > Note this does change CONFIG_PCIEASPM=n builds, which used to program
> >
> > chip LTR unconditionally: pci_disable_link_state() is a stub that returns
> >
> > success there, so tp->aspm_manageable ends up set. The PCI core does not
> >
> > configure LTR in that configuration either.
> >
> >
> >
> > Fixes: 9ab94a32af70 ("r8169: enable LTR support")
> >
> > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2529752
>
> Did you verify that your patch fixes the reported issue? Or do you
> just assume this? Because in the bug comment history I don't see
> a statement that issue has been resolved.
I don't have a board with me, hence have not verified it. I did a
compile check only.
>
> > Assisted-by: LLM
> >
> > Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
> >
> > ---
> >
> > v2:
> >
> >  - Gate on pci_dev->ltr_path instead of reading PCI_EXP_DEVCTL2 directly,
> >
> >    and compile rtl_enable_ltr() out for CONFIG_PCIEASPM=n, where that
> >
> >    field does not exist and there is nothing to enable LTR for.
> >
> >    Suggested by Heiner Kallweit.
> >
> >  - Commit message notes the resulting change for CONFIG_PCIEASPM=n builds.
> >
> >  - No change to the RTL8125 register programming itself.
> >
> >
> >
> > v1: https://lore.kernel.org/all/20260909110554.1977-1-yogeshgaur.83@gmail.com/
> >
> >
> >
> >  drivers/net/ethernet/realtek/r8169_main.c | 13 +++++++++++++
> >
> >  1 file changed, 13 insertions(+)
> >
> >
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> >
> > index ec4fc21fa21f..05f7a44aff01 100644
> >
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> >
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >
> > @@ -3035,8 +3035,16 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
> >
> >       }
> >
> >  }
> >
> >
> >
> > +#ifdef CONFIG_PCIEASPM
> >
> >  static void rtl_enable_ltr(struct rtl8169_private *tp)
> >
> >  {
> >
> > +     /* The chip must not issue LTR messages unless LTR is enabled on the
> >
> > +      * whole path up to the root port. pci_configure_ltr() works that out
> >
> > +      * and records the result in pci_dev->ltr_path.
> >
> > +      */
> >
> > +     if (!tp->pci_dev->ltr_path)
>
> Simpler would be:
> if (!IS_ENABLED(CONFIG_PCIEASPM) || !tp->pci_dev->ltr_path)
>
I tried that and it does not build, because ltr_path itself is declared
inside the #ifdef:

  include/linux/pci.h:443
    #ifdef CONFIG_PCIEASPM
            struct pcie_link_state  *link_state;
            unsigned int    aspm_l0s_support:1;
            unsigned int    aspm_l1_support:1;
            unsigned int    ltr_path:1;
    #endif

IS_ENABLED() is a C expression, so the member reference still has to
compile even when the left operand is 0:

  drivers/net/ethernet/realtek/r8169_main.c: In function 'rtl_enable_ltr':
  error: 'struct pci_dev' has no member named 'ltr_path'
       if (!IS_ENABLED(CONFIG_PCIEASPM) || !tp->pci_dev->ltr_path)

> Then you don't need the stub function.
>
You are right that the stub function should go, though. v3 drops it and
keeps a single rtl_enable_ltr() with the guard inline:

        static void rtl_enable_ltr(struct rtl8169_private *tp)
        {
                /* comment */
        #ifdef CONFIG_PCIEASPM
                if (!tp->pci_dev->ltr_path)
                        return;
        #else
                return;
        #endif
> >
> > +             return;
> >
> > +
> >
> >       switch (tp->mac_version) {
> >
> >       case RTL_GIGA_MAC_VER_80:
> >
> >               r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> >
> > @@ -3120,6 +3128,11 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
> >
> >       /* chip can trigger LTR */
> >
> >       r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);
> >
> >  }
> >
> > +#else
> >
> > +static void rtl_enable_ltr(struct rtl8169_private *tp)
> >
> > +{
> >
> > +}
> >
> > +#endif
> >
> >
> >
> >  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> >
> >  {
> >
>

[PATCH net v3] r8169: don't enable chip LTR when the platform has not enabled LTR

[Yogesh Gaur]

rtl_enable_ltr() programs the MAC to generate LTR messages - ALDPS_LTR_EN,
LTR_SNOOP_EN, LTR_OBFF_LOCK_EN, plus LINK_SPEED_CHANGE_EN on
RTL8125/RTL8126/RTL8127 - and rtl_hw_aspm_clkreq_enable() calls it on
every ASPM enable, then goes on to let the chip trigger L1.2.

The only gate is tp->aspm_manageable, which records that the OS is allowed
to control ASPM. It says nothing about LTR. LTR is a separate PCIe
capability that only works if every device on the path to the root port
supports it. The PCI core works that out in pci_configure_ltr() and
records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
function must not issue LTR messages while LTR Mechanism Enable is clear.

So on a platform whose hierarchy has no LTR path, the driver tells the
chip to start sending LTR messages nothing will honour, and ties ALDPS -
the PHY's link-down power saving - to them. A report against RTL8125B
(rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
link down/up transitions in one eight-hour boot, with repeated downshifts
to 100Mbps, against four transitions at boot and then a stable link on
the kernel before the LTR change.

Gate the chip's LTR programming on pci_dev->ltr_path. That field exists
only when CONFIG_PCIEASPM is enabled, so the test needs a preprocessor
guard rather than IS_ENABLED(). With CONFIG_PCIEASPM=n the PCI core's
pci_configure_ltr() is a stub and LTR is enabled nowhere, so the chip
must not issue LTR messages in that configuration either.

Note this does change CONFIG_PCIEASPM=n builds, which used to program
chip LTR unconditionally: pci_disable_link_state() is a stub that returns
success there, so tp->aspm_manageable ends up set.

Fixes: 9ab94a32af70 ("r8169: enable LTR support")
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2529752
Assisted-by: LLM
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
v3:
 - Drop the #ifdef/#else pair of rtl_enable_ltr() definitions and keep a
   single function with the guard inline, so there is no stub function.
   Suggested by Heiner Kallweit.
 - IS_ENABLED(CONFIG_PCIEASPM) cannot replace the #ifdef here: ltr_path is
   declared inside #ifdef CONFIG_PCIEASPM in include/linux/pci.h, so the
   member reference still has to compile in a CONFIG_PCIEASPM=n build:
     error: 'struct pci_dev' has no member named 'ltr_path'
   (x86_64 defconfig + EXPERT=y + PCIEASPM=n + R8169=m). Happy to send a
   PCI patch first that either moves the field out of the #ifdef or adds a
   pcie_ltr_path() accessor, and then drop the #ifdef from here - please
   say if you would prefer that, it is a different tree.
 - Added testing disclosure below.

v2: https://lore.kernel.org/all/20260910130140.910-1-yogeshgaur.83@gmail.com/
v1: https://lore.kernel.org/all/20260909110554.1977-1-yogeshgaur.83@gmail.com/

NOT VERIFIED ON HARDWARE. I have no RTL8125 here. The causal claim above is
reasoned from the register writes.

Compile-tested only: drivers/net/ethernet/realtek/r8169_main.o, x86_64
defconfig + R8169=m, built W=1 clean with both CONFIG_PCIEASPM=y and
CONFIG_PCIEASPM=n.

 drivers/net/ethernet/realtek/r8169_main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index ec4fc21fa21f..c305a8f19551 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -3037,6 +3037,18 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
 
 static void rtl_enable_ltr(struct rtl8169_private *tp)
 {
+	/* The chip must not issue LTR messages unless LTR is enabled for the
+	 * whole path up to the root port. pci_configure_ltr() works that out
+	 * and records the result in pci_dev->ltr_path, which exists only with
+	 * CONFIG_PCIEASPM; without it the PCI core never enables LTR anywhere.
+	 */
+#ifdef CONFIG_PCIEASPM
+	if (!tp->pci_dev->ltr_path)
+		return;
+#else
+	return;
+#endif
+
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_80:
 		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
-- 
2.55.0.windows.5

Re: [PATCH net v2] r8169: don't enable chip LTR when the platform has not enabled LTR

[Heiner Kallweit]

On 11.09.2026 15:00, Yogesh Gaur wrote:
> On Fri, Sep 11, 2026 at 1:03 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 10.09.2026 15:01, Yogesh Gaur wrote:
>>> rtl_enable_ltr() programs the MAC to generate LTR messages - ALDPS_LTR_EN,
>>>
>>
>> Empty lines, mail formatting issue?
> Yes, I would correct it in v3.
> 
>>
>>> LTR_SNOOP_EN, LTR_OBFF_LOCK_EN, plus LINK_SPEED_CHANGE_EN on
>>>
>>> RTL8125/RTL8126/RTL8127 - and rtl_hw_aspm_clkreq_enable() calls it on
>>>
>>> every ASPM enable, then goes on to let the chip trigger L1.2.
>>>
>>>
>>>
>>> The only gate is tp->aspm_manageable, which records that the OS is allowed
>>>
>>> to control ASPM. It says nothing about LTR. LTR is a separate PCIe
>>>
>>> capability that only works if every device on the path to the root port
>>>
>>> supports it. The PCI core works that out in pci_configure_ltr() and
>>>
>>> records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
>>>
>>> function must not issue LTR messages while LTR Mechanism Enable is clear.
>>>
>>>
>>>
>>> So on a platform whose hierarchy has no LTR path, the driver tells the
>>>
>>> chip to start sending LTR messages nothing will honour, and ties ALDPS -
>>>
>>> the PHY's link-down power saving - to them. A report against RTL8125B
>>>
>>> (rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
>>
>> Did you verify that this is caused by LTR messages, and not by any other
>> root cause?
>>
>>>
>>> link down/up transitions in one eight-hour boot, with repeated downshifts
>>>
>>> to 100Mbps, against four transitions at boot and then a stable link on
>>>
>>> the kernel before the LTR change.
>>>
>>>
>>>
>>> Gate the chip's LTR programming on pci_dev->ltr_path. That field lives
>>>
>>> inside CONFIG_PCIEASPM, and without ASPM support there is nothing here to
>>>
>>> enable LTR for, so compile the function out in that configuration.
>>>
>>>
>>>
>>> Note this does change CONFIG_PCIEASPM=n builds, which used to program
>>>
>>> chip LTR unconditionally: pci_disable_link_state() is a stub that returns
>>>
>>> success there, so tp->aspm_manageable ends up set. The PCI core does not
>>>
>>> configure LTR in that configuration either.
>>>
>>>
>>>
>>> Fixes: 9ab94a32af70 ("r8169: enable LTR support")
>>>
>>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2529752
>>
>> Did you verify that your patch fixes the reported issue? Or do you
>> just assume this? Because in the bug comment history I don't see
>> a statement that issue has been resolved.
> I don't have a board with me, hence have not verified it. I did a
> compile check only.

If it's not verified that your patch fixes the reported issue, better
remove the Closes line.

>>
>>> Assisted-by: LLM
>>>
>>> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
>>>
>>> ---
>>>
>>> v2:
>>>
>>>  - Gate on pci_dev->ltr_path instead of reading PCI_EXP_DEVCTL2 directly,
>>>
>>>    and compile rtl_enable_ltr() out for CONFIG_PCIEASPM=n, where that
>>>
>>>    field does not exist and there is nothing to enable LTR for.
>>>
>>>    Suggested by Heiner Kallweit.
>>>
>>>  - Commit message notes the resulting change for CONFIG_PCIEASPM=n builds.
>>>
>>>  - No change to the RTL8125 register programming itself.
>>>
>>>
>>>
>>> v1: https://lore.kernel.org/all/20260909110554.1977-1-yogeshgaur.83@gmail.com/
>>>
>>>
>>>
>>>  drivers/net/ethernet/realtek/r8169_main.c | 13 +++++++++++++
>>>
>>>  1 file changed, 13 insertions(+)
>>>
>>>
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>>>
>>> index ec4fc21fa21f..05f7a44aff01 100644
>>>
>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>>
>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>>
>>> @@ -3035,8 +3035,16 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
>>>
>>>       }
>>>
>>>  }
>>>
>>>
>>>
>>> +#ifdef CONFIG_PCIEASPM
>>>
>>>  static void rtl_enable_ltr(struct rtl8169_private *tp)
>>>
>>>  {
>>>
>>> +     /* The chip must not issue LTR messages unless LTR is enabled on the
>>>
>>> +      * whole path up to the root port. pci_configure_ltr() works that out
>>>
>>> +      * and records the result in pci_dev->ltr_path.
>>>
>>> +      */
>>>
>>> +     if (!tp->pci_dev->ltr_path)
>>
>> Simpler would be:
>> if (!IS_ENABLED(CONFIG_PCIEASPM) || !tp->pci_dev->ltr_path)
>>
> I tried that and it does not build, because ltr_path itself is declared
> inside the #ifdef:
> 
>   include/linux/pci.h:443
>     #ifdef CONFIG_PCIEASPM
>             struct pcie_link_state  *link_state;
>             unsigned int    aspm_l0s_support:1;
>             unsigned int    aspm_l1_support:1;
>             unsigned int    ltr_path:1;
>     #endif
> 
> IS_ENABLED() is a C expression, so the member reference still has to
> compile even when the left operand is 0:
> 
>   drivers/net/ethernet/realtek/r8169_main.c: In function 'rtl_enable_ltr':
>   error: 'struct pci_dev' has no member named 'ltr_path'
>        if (!IS_ENABLED(CONFIG_PCIEASPM) || !tp->pci_dev->ltr_path)
> 
Right, ofc ..

>> Then you don't need the stub function.
>>
> You are right that the stub function should go, though. v3 drops it and
> keeps a single rtl_enable_ltr() with the guard inline:
> 
>         static void rtl_enable_ltr(struct rtl8169_private *tp)
>         {
>                 /* comment */
>         #ifdef CONFIG_PCIEASPM
>                 if (!tp->pci_dev->ltr_path)
>                         return;
>         #else
>                 return;
>         #endif

LGTM

>>>
>>> +             return;
>>>
>>> +
>>>
>>>       switch (tp->mac_version) {
>>>
>>>       case RTL_GIGA_MAC_VER_80:
>>>
>>>               r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
>>>
>>> @@ -3120,6 +3128,11 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
>>>
>>>       /* chip can trigger LTR */
>>>
>>>       r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);
>>>
>>>  }
>>>
>>> +#else
>>>
>>> +static void rtl_enable_ltr(struct rtl8169_private *tp)
>>>
>>> +{
>>>
>>> +}
>>>
>>> +#endif
>>>
>>>
>>>
>>>  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>>>
>>>  {
>>>
>>

Re: [PATCH net v2] r8169: don't enable chip LTR when the platform has not enabled LTR

[Yogesh Gaur]

On Fri, Sep 11, 2026 at 6:44 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 11.09.2026 15:00, Yogesh Gaur wrote:
> > On Fri, Sep 11, 2026 at 1:03 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>
> >> On 10.09.2026 15:01, Yogesh Gaur wrote:
> >>> rtl_enable_ltr() programs the MAC to generate LTR messages - ALDPS_LTR_EN,
> >>>
> >>
> >> Empty lines, mail formatting issue?
> > Yes, I would correct it in v3.
> >
> >>
> >>> LTR_SNOOP_EN, LTR_OBFF_LOCK_EN, plus LINK_SPEED_CHANGE_EN on
> >>>
> >>> RTL8125/RTL8126/RTL8127 - and rtl_hw_aspm_clkreq_enable() calls it on
> >>>
> >>> every ASPM enable, then goes on to let the chip trigger L1.2.
> >>>
> >>>
> >>>
> >>> The only gate is tp->aspm_manageable, which records that the OS is allowed
> >>>
> >>> to control ASPM. It says nothing about LTR. LTR is a separate PCIe
> >>>
> >>> capability that only works if every device on the path to the root port
> >>>
> >>> supports it. The PCI core works that out in pci_configure_ltr() and
> >>>
> >>> records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
> >>>
> >>> function must not issue LTR messages while LTR Mechanism Enable is clear.
> >>>
> >>>
> >>>
> >>> So on a platform whose hierarchy has no LTR path, the driver tells the
> >>>
> >>> chip to start sending LTR messages nothing will honour, and ties ALDPS -
> >>>
> >>> the PHY's link-down power saving - to them. A report against RTL8125B
> >>>
> >>> (rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
> >>
> >> Did you verify that this is caused by LTR messages, and not by any other
> >> root cause?
> >>
> >>>
> >>> link down/up transitions in one eight-hour boot, with repeated downshifts
> >>>
> >>> to 100Mbps, against four transitions at boot and then a stable link on
> >>>
> >>> the kernel before the LTR change.
> >>>
> >>>
> >>>
> >>> Gate the chip's LTR programming on pci_dev->ltr_path. That field lives
> >>>
> >>> inside CONFIG_PCIEASPM, and without ASPM support there is nothing here to
> >>>
> >>> enable LTR for, so compile the function out in that configuration.
> >>>
> >>>
> >>>
> >>> Note this does change CONFIG_PCIEASPM=n builds, which used to program
> >>>
> >>> chip LTR unconditionally: pci_disable_link_state() is a stub that returns
> >>>
> >>> success there, so tp->aspm_manageable ends up set. The PCI core does not
> >>>
> >>> configure LTR in that configuration either.
> >>>
> >>>
> >>>
> >>> Fixes: 9ab94a32af70 ("r8169: enable LTR support")
> >>>
> >>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2529752
> >>
> >> Did you verify that your patch fixes the reported issue? Or do you
> >> just assume this? Because in the bug comment history I don't see
> >> a statement that issue has been resolved.
> > I don't have a board with me, hence have not verified it. I did a
> > compile check only.
>
> If it's not verified that your patch fixes the reported issue, better
> remove the Closes line.
>
Sure, would remove it in v4.

Thanks.

> >>
> >>> Assisted-by: LLM
> >>>
> >>> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
> >>>
> >>> ---
> >>>
> >>> v2:
> >>>
> >>>  - Gate on pci_dev->ltr_path instead of reading PCI_EXP_DEVCTL2 directly,
> >>>
> >>>    and compile rtl_enable_ltr() out for CONFIG_PCIEASPM=n, where that
> >>>
> >>>    field does not exist and there is nothing to enable LTR for.
> >>>
> >>>    Suggested by Heiner Kallweit.
> >>>
> >>>  - Commit message notes the resulting change for CONFIG_PCIEASPM=n builds.
> >>>
> >>>  - No change to the RTL8125 register programming itself.
> >>>
> >>>
> >>>
> >>> v1: https://lore.kernel.org/all/20260909110554.1977-1-yogeshgaur.83@gmail.com/
> >>>
> >>>
> >>>
> >>>  drivers/net/ethernet/realtek/r8169_main.c | 13 +++++++++++++
> >>>
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>>
> >>>
> >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> >>>
> >>> index ec4fc21fa21f..05f7a44aff01 100644
> >>>
> >>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>>
> >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>>
> >>> @@ -3035,8 +3035,16 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
> >>>
> >>>       }
> >>>
> >>>  }
> >>>
> >>>
> >>>
> >>> +#ifdef CONFIG_PCIEASPM
> >>>
> >>>  static void rtl_enable_ltr(struct rtl8169_private *tp)
> >>>
> >>>  {
> >>>
> >>> +     /* The chip must not issue LTR messages unless LTR is enabled on the
> >>>
> >>> +      * whole path up to the root port. pci_configure_ltr() works that out
> >>>
> >>> +      * and records the result in pci_dev->ltr_path.
> >>>
> >>> +      */
> >>>
> >>> +     if (!tp->pci_dev->ltr_path)
> >>
> >> Simpler would be:
> >> if (!IS_ENABLED(CONFIG_PCIEASPM) || !tp->pci_dev->ltr_path)
> >>
> > I tried that and it does not build, because ltr_path itself is declared
> > inside the #ifdef:
> >
> >   include/linux/pci.h:443
> >     #ifdef CONFIG_PCIEASPM
> >             struct pcie_link_state  *link_state;
> >             unsigned int    aspm_l0s_support:1;
> >             unsigned int    aspm_l1_support:1;
> >             unsigned int    ltr_path:1;
> >     #endif
> >
> > IS_ENABLED() is a C expression, so the member reference still has to
> > compile even when the left operand is 0:
> >
> >   drivers/net/ethernet/realtek/r8169_main.c: In function 'rtl_enable_ltr':
> >   error: 'struct pci_dev' has no member named 'ltr_path'
> >        if (!IS_ENABLED(CONFIG_PCIEASPM) || !tp->pci_dev->ltr_path)
> >
> Right, ofc ..
>
> >> Then you don't need the stub function.
> >>
> > You are right that the stub function should go, though. v3 drops it and
> > keeps a single rtl_enable_ltr() with the guard inline:
> >
> >         static void rtl_enable_ltr(struct rtl8169_private *tp)
> >         {
> >                 /* comment */
> >         #ifdef CONFIG_PCIEASPM
> >                 if (!tp->pci_dev->ltr_path)
> >                         return;
> >         #else
> >                 return;
> >         #endif
>
> LGTM
>
> >>>
> >>> +             return;
> >>>
> >>> +
> >>>
> >>>       switch (tp->mac_version) {
> >>>
> >>>       case RTL_GIGA_MAC_VER_80:
> >>>
> >>>               r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> >>>
> >>> @@ -3120,6 +3128,11 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
> >>>
> >>>       /* chip can trigger LTR */
> >>>
> >>>       r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);
> >>>
> >>>  }
> >>>
> >>> +#else
> >>>
> >>> +static void rtl_enable_ltr(struct rtl8169_private *tp)
> >>>
> >>> +{
> >>>
> >>> +}
> >>>
> >>> +#endif
> >>>
> >>>
> >>>
> >>>  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> >>>
> >>>  {
> >>>
> >>
>

[PATCH net v4] r8169: don't enable chip LTR when the platform has not enabled LTR

[Yogesh Gaur]

rtl_enable_ltr() programs the MAC to generate LTR messages - ALDPS_LTR_EN,
LTR_SNOOP_EN, LTR_OBFF_LOCK_EN, plus LINK_SPEED_CHANGE_EN on
RTL8125/RTL8126/RTL8127 - and rtl_hw_aspm_clkreq_enable() calls it on
every ASPM enable, then goes on to let the chip trigger L1.2.

The only gate is tp->aspm_manageable, which records that the OS is allowed
to control ASPM. It says nothing about LTR. LTR is a separate PCIe
capability that only works if every device on the path to the root port
supports it. The PCI core works that out in pci_configure_ltr() and
records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
function must not issue LTR messages while LTR Mechanism Enable is clear.

So on a platform whose hierarchy has no LTR path, the driver tells the
chip to start sending LTR messages nothing will honour, and ties ALDPS -
the PHY's link-down power saving - to them. A report against RTL8125B
(rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
link down/up transitions in one eight-hour boot, with repeated downshifts
to 100Mbps, against four transitions at boot and then a stable link on
the kernel before the LTR change.

Gate the chip's LTR programming on pci_dev->ltr_path. That field exists
only when CONFIG_PCIEASPM is enabled, so the test needs a preprocessor
guard rather than IS_ENABLED(). With CONFIG_PCIEASPM=n the PCI core's
pci_configure_ltr() is a stub and LTR is enabled nowhere, so the chip
must not issue LTR messages in that configuration either.

Note this does change CONFIG_PCIEASPM=n builds, which used to program
chip LTR unconditionally: pci_disable_link_state() is a stub that returns
success there, so tp->aspm_manageable ends up set.

Fixes: 9ab94a32af70 ("r8169: enable LTR support")
Assisted-by: LLM
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
v4:
 - Drop closes line.
v3: https://lore.kernel.org/all/20260911131352.2036-1-yogeshgaur.83@gmail.com/
v2: https://lore.kernel.org/all/20260910130140.910-1-yogeshgaur.83@gmail.com/
v1: https://lore.kernel.org/all/20260909110554.1977-1-yogeshgaur.83@gmail.com/

NOT VERIFIED ON HARDWARE. The causal claim above is
reasoned from the register writes.

Compile-tested only: drivers/net/ethernet/realtek/r8169_main.o, x86_64
defconfig + R8169=m, built W=1 clean with both CONFIG_PCIEASPM=y and
CONFIG_PCIEASPM=n.

 drivers/net/ethernet/realtek/r8169_main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index ec4fc21fa21f..c305a8f19551 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -3037,6 +3037,18 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
 
 static void rtl_enable_ltr(struct rtl8169_private *tp)
 {
+	/* The chip must not issue LTR messages unless LTR is enabled for the
+	 * whole path up to the root port. pci_configure_ltr() works that out
+	 * and records the result in pci_dev->ltr_path, which exists only with
+	 * CONFIG_PCIEASPM; without it the PCI core never enables LTR anywhere.
+	 */
+#ifdef CONFIG_PCIEASPM
+	if (!tp->pci_dev->ltr_path)
+		return;
+#else
+	return;
+#endif
+
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_80:
 		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
-- 
2.55.0.windows.5

Re: [PATCH net v4] r8169: don't enable chip LTR when the platform has not enabled LTR

[Heiner Kallweit]

On 14.09.2026 15:00, Yogesh Gaur wrote:
> rtl_enable_ltr() programs the MAC to generate LTR messages - ALDPS_LTR_EN,
> LTR_SNOOP_EN, LTR_OBFF_LOCK_EN, plus LINK_SPEED_CHANGE_EN on
> RTL8125/RTL8126/RTL8127 - and rtl_hw_aspm_clkreq_enable() calls it on
> every ASPM enable, then goes on to let the chip trigger L1.2.
> 
> The only gate is tp->aspm_manageable, which records that the OS is allowed
> to control ASPM. It says nothing about LTR. LTR is a separate PCIe
> capability that only works if every device on the path to the root port
> supports it. The PCI core works that out in pci_configure_ltr() and
> records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
> function must not issue LTR messages while LTR Mechanism Enable is clear.
> 
> So on a platform whose hierarchy has no LTR path, the driver tells the
> chip to start sending LTR messages nothing will honour, and ties ALDPS -
> the PHY's link-down power saving - to them. A report against RTL8125B
> (rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
> link down/up transitions in one eight-hour boot, with repeated downshifts
> to 100Mbps, against four transitions at boot and then a stable link on
> the kernel before the LTR change.
> 
> Gate the chip's LTR programming on pci_dev->ltr_path. That field exists
> only when CONFIG_PCIEASPM is enabled, so the test needs a preprocessor
> guard rather than IS_ENABLED(). With CONFIG_PCIEASPM=n the PCI core's
> pci_configure_ltr() is a stub and LTR is enabled nowhere, so the chip
> must not issue LTR messages in that configuration either.
> 
> Note this does change CONFIG_PCIEASPM=n builds, which used to program
> chip LTR unconditionally: pci_disable_link_state() is a stub that returns
> success there, so tp->aspm_manageable ends up set.
> 
> Fixes: 9ab94a32af70 ("r8169: enable LTR support")
> Assisted-by: LLM
> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
> ---
> v4:
>  - Drop closes line.
> v3: https://lore.kernel.org/all/20260911131352.2036-1-yogeshgaur.83@gmail.com/
> v2: https://lore.kernel.org/all/20260910130140.910-1-yogeshgaur.83@gmail.com/
> v1: https://lore.kernel.org/all/20260909110554.1977-1-yogeshgaur.83@gmail.com/
> 
> NOT VERIFIED ON HARDWARE. The causal claim above is
> reasoned from the register writes.
> 
> Compile-tested only: drivers/net/ethernet/realtek/r8169_main.o, x86_64
> defconfig + R8169=m, built W=1 clean with both CONFIG_PCIEASPM=y and
> CONFIG_PCIEASPM=n.
> 
>  drivers/net/ethernet/realtek/r8169_main.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21f..c305a8f19551 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -3037,6 +3037,18 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
>  
>  static void rtl_enable_ltr(struct rtl8169_private *tp)
>  {
> +	/* The chip must not issue LTR messages unless LTR is enabled for the
> +	 * whole path up to the root port. pci_configure_ltr() works that out
> +	 * and records the result in pci_dev->ltr_path, which exists only with
> +	 * CONFIG_PCIEASPM; without it the PCI core never enables LTR anywhere.
> +	 */
> +#ifdef CONFIG_PCIEASPM
> +	if (!tp->pci_dev->ltr_path)
> +		return;
> +#else
> +	return;
> +#endif
> +
>  	switch (tp->mac_version) {
>  	case RTL_GIGA_MAC_VER_80:
>  		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);

Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH net v4] r8169: don't enable chip LTR when the platform has not enabled LTR

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 0

- [Medium] rtl_enable_ltr()
  (drivers/net/ethernet/realtek/r8169_main.c:3038-3050) gates all chip-…
- [Medium] The permanent commit message presents the RTL8125B link-flap
  report as demonstrating the effect of chip LTR programming ("A report…
- [Medium] The patch only *skips* the chip's LTR programming when
  pci_dev->ltr_path is clear (or CONFIG_PCIEASPM=n); it never clears the…

--

r8169: don't enable chip LTR when the platform has not enabled LTR

This makes rtl_enable_ltr() return early unless pci_dev->ltr_path is set,
and return unconditionally when CONFIG_PCIEASPM=n, so the MAC's LTR
generation bits are no longer programmed on hierarchies where the PCI core
did not record an LTR path.

A couple of questions on the commit message and on the shape of the gate.

> The only gate is tp->aspm_manageable, which records that the OS is allowed
> to control ASPM. It says nothing about LTR. LTR is a separate PCIe
> capability that only works if every device on the path to the root port
> supports it. The PCI core works that out in pci_configure_ltr() and
> records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
> function must not issue LTR messages while LTR Mechanism Enable is clear.
>
> So on a platform whose hierarchy has no LTR path, the driver tells the
> chip to start sending LTR messages nothing will honour, and ties ALDPS -
> the PHY's link-down power saving - to them. A report against RTL8125B
> (rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
> link down/up transitions in one eight-hour boot, with repeated downshifts
> to 100Mbps, against four transitions at boot and then a stable link on
> the kernel before the LTR change.

[Severity: Medium]
Should the durable commit text be this definite about causation?  The notes
below the --- separator say "NOT VERIFIED ON HARDWARE. The causal claim
above is reasoned from the register writes" and "Compile-tested only", and
those notes are dropped by git am, so what remains in history is a
confirmed-regression narrative plus a Fixes: tag that invites stable
backporting.

rtl_enable_ltr() programs several things together:

	r8168_mac_ocp_modify(tp, LTR_SNOOP, 0x0000, LTR_SNOOP_EN);
	r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
	...
	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0000, LINK_SPEED_CHANGE_EN);

and its caller rtl_hw_aspm_clkreq_enable() also sets ASPM_en, ClkReqEn and
the "chip can trigger L1.2" bit, none of which this patch touches.  Can the
register writes alone distinguish LTR message generation from ALDPS or chip
ASPM as the source of the observed flapping?

Also, the patch only changes behaviour where pci_dev->ltr_path is 0.  Is it
established that the reporter's mini PC is such a platform?

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21fb..c305a8f19551d 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -3037,6 +3037,18 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
>  
>  static void rtl_enable_ltr(struct rtl8169_private *tp)
>  {
> +	/* The chip must not issue LTR messages unless LTR is enabled for the
> +	 * whole path up to the root port. pci_configure_ltr() works that out
> +	 * and records the result in pci_dev->ltr_path, which exists only with
> +	 * CONFIG_PCIEASPM; without it the PCI core never enables LTR anywhere.
> +	 */
> +#ifdef CONFIG_PCIEASPM
> +	if (!tp->pci_dev->ltr_path)
> +		return;
> +#else
> +	return;
> +#endif

[Severity: Medium]
Is the #else return; branch correct for platforms where firmware already
enabled LTR?  With CONFIG_PCIEASPM=n, pci_configure_ltr() is the stub in
drivers/pci/pci.h, so firmware-set PCI_EXP_DEVCTL2_LTR_EN bits along the
path are simply left alone.  The CONFIG_PCIEASPM=y implementation treats
that as a supported configuration:

drivers/pci/pcie/aspm.c:pci_configure_ltr() {
	pcie_capability_read_dword(pdev, PCI_EXP_DEVCTL2, &ctl);
	if (ctl & PCI_EXP_DEVCTL2_LTR_EN) {
		if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) {
			pdev->ltr_path = 1;
			return;
		}

		bridge = pci_upstream_bridge(pdev);
		if (bridge && bridge->ltr_path)
			pdev->ltr_path = 1;
	...
}

So does the commit message claim "with CONFIG_PCIEASPM=n the PCI core's
pci_configure_ltr() is a stub and LTR is enabled nowhere" conflate "the
kernel does not enable LTR" with "LTR is not enabled"?

There is a second consequence in the same configuration.  pci_disable_link_state()
is a stub returning 0 in include/linux/pci.h, so tp->aspm_manageable ends up
set and rtl_hw_aspm_clkreq_enable() still takes the enable branch:

	rtl_enable_ltr(tp);		/* now returns immediately */
	switch (tp->mac_version) {
	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_LAST:
		/* reset ephy tx/rx disable timer */
		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
		/* chip can trigger L1.2 */
		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));

Does this leave the MAC armed for L1.2 with the LTR machinery deliberately
left unconfigured on every CONFIG_PCIEASPM=n build?

Would reading the endpoint's own LTR Mechanism Enable bit, which is
available in any Kconfig setting via
pcie_capability_read_dword(tp->pci_dev, PCI_EXP_DEVCTL2, ...), avoid the
need for the blanket #else return; here?

One more note on the comment wording: ltr_path is also left clear when
DEVCAP2 has no LTR bit, when there is no upstream bridge (root-bus or
RC-integrated endpoint), and when firmware owns LTR (!host->native_ltr).
Is "LTR is enabled for the whole path up to the root port" a stronger
statement than what the core actually computes?

[Severity: Medium]
Does this need to clear the chip's LTR bits rather than just skip
programming them?  Grepping r8169_main.c, ALDPS_LTR/ALDPS_LTR_EN,
LTR_SNOOP/LTR_SNOOP_EN, LTR_OBFF_LOCK/LTR_OBFF_LOCK_EN,
LINK_SPEED_CHANGE_EN and COMBO_LTR_EXTEND/COMBO_LTR_EXTEND_EN appear only
in their register definitions and inside rtl_enable_ltr(), and every access
there is a set/OR:

	r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
	RTL_W8(tp, COMBO_LTR_EXTEND, RTL_R8(tp, COMBO_LTR_EXTEND) | COMBO_LTR_EXTEND_EN);
	...
	/* chip can trigger LTR */
	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);

There is no rtl_disable_ltr(), and the disable branch of
rtl_hw_aspm_clkreq_enable() clears only 0xe092, ClkReqEn/INT_CFG0_CLKREQEN
and ASPM_en:

		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
		...
		rtl_mod_config5(tp, ASPM_en, 0);

If any of those LTR enables is already set when the new early return is
taken (power-on or warm-reboot residue, a boot firmware Realtek driver, or
kexec from a kernel that programmed them), does the MAC keep issuing LTR
messages with LTR Mechanism Enable clear, which is the exact state the
commit message says must not happen?  rtl_hw_reset() only writes ChipCmd
CmdReset:

	static void rtl_hw_reset(struct rtl8169_private *tp)
	{
		RTL_W8(tp, ChipCmd, CmdReset);

		rtl_loop_wait_low(tp, &rtl_chipcmd_cond, 100, 100);
	}

so it is not obvious from the source that those MAC OCP bits come back as 0.

The existing in-tree consumer of ltr_path takes the other approach in
drivers/gpu/drm/amd/amdgpu/nbio_v4_3.c:nbio_v4_3_program_ltr():

	if (adev->pdev->ltr_path)
		data |= BIF_CFG_DEV0_EPF0_DEVICE_CNTL2__LTR_EN_MASK;
	else
		data &= ~BIF_CFG_DEV0_EPF0_DEVICE_CNTL2__LTR_EN_MASK;

Would an explicit clear of the LTR enables on the !ltr_path path, matching
the driver's own ASPM disable convention, be the more robust shape here?

>  	switch (tp->mac_version) {
>  	case RTL_GIGA_MAC_VER_80:
>  		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914130050.304-1-yogeshgaur.83%40gmail.com

Re: [PATCH net v4] r8169: don't enable chip LTR when the platform has not enabled LTR

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
r8169: don't enable chip LTR when the platform has not enabled LTR

This makes rtl_enable_ltr() return early unless pci_dev->ltr_path is set,
and return unconditionally when CONFIG_PCIEASPM=n, so the MAC's LTR
generation bits are no longer programmed on hierarchies where the PCI core
did not record an LTR path.

A couple of questions on the commit message and on the shape of the gate.

> The only gate is tp->aspm_manageable, which records that the OS is allowed
> to control ASPM. It says nothing about LTR. LTR is a separate PCIe
> capability that only works if every device on the path to the root port
> supports it. The PCI core works that out in pci_configure_ltr() and
> records the result in pci_dev->ltr_path; per PCIe r6.0 sec 7.5.3.16 a
> function must not issue LTR messages while LTR Mechanism Enable is clear.
>
> So on a platform whose hierarchy has no LTR path, the driver tells the
> chip to start sending LTR messages nothing will honour, and ties ALDPS -
> the PHY's link-down power saving - to them. A report against RTL8125B
> (rev 05, firmware rtl8125b-2_0.0.2) in a mini PC shows the effect: 291
> link down/up transitions in one eight-hour boot, with repeated downshifts
> to 100Mbps, against four transitions at boot and then a stable link on
> the kernel before the LTR change.

[Severity: Medium]
Should the durable commit text be this definite about causation?  The notes
below the --- separator say "NOT VERIFIED ON HARDWARE. The causal claim
above is reasoned from the register writes" and "Compile-tested only", and
those notes are dropped by git am, so what remains in history is a
confirmed-regression narrative plus a Fixes: tag that invites stable
backporting.

rtl_enable_ltr() programs several things together:

	r8168_mac_ocp_modify(tp, LTR_SNOOP, 0x0000, LTR_SNOOP_EN);
	r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
	...
	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0000, LINK_SPEED_CHANGE_EN);

and its caller rtl_hw_aspm_clkreq_enable() also sets ASPM_en, ClkReqEn and
the "chip can trigger L1.2" bit, none of which this patch touches.  Can the
register writes alone distinguish LTR message generation from ALDPS or chip
ASPM as the source of the observed flapping?

Also, the patch only changes behaviour where pci_dev->ltr_path is 0.  Is it
established that the reporter's mini PC is such a platform?

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21fb..c305a8f19551d 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -3037,6 +3037,18 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
>  
>  static void rtl_enable_ltr(struct rtl8169_private *tp)
>  {
> +	/* The chip must not issue LTR messages unless LTR is enabled for the
> +	 * whole path up to the root port. pci_configure_ltr() works that out
> +	 * and records the result in pci_dev->ltr_path, which exists only with
> +	 * CONFIG_PCIEASPM; without it the PCI core never enables LTR anywhere.
> +	 */
> +#ifdef CONFIG_PCIEASPM
> +	if (!tp->pci_dev->ltr_path)
> +		return;
> +#else
> +	return;
> +#endif

[Severity: Medium]
Is the #else return; branch correct for platforms where firmware already
enabled LTR?  With CONFIG_PCIEASPM=n, pci_configure_ltr() is the stub in
drivers/pci/pci.h, so firmware-set PCI_EXP_DEVCTL2_LTR_EN bits along the
path are simply left alone.  The CONFIG_PCIEASPM=y implementation treats
that as a supported configuration:

drivers/pci/pcie/aspm.c:pci_configure_ltr() {
	pcie_capability_read_dword(pdev, PCI_EXP_DEVCTL2, &ctl);
	if (ctl & PCI_EXP_DEVCTL2_LTR_EN) {
		if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) {
			pdev->ltr_path = 1;
			return;
		}

		bridge = pci_upstream_bridge(pdev);
		if (bridge && bridge->ltr_path)
			pdev->ltr_path = 1;
	...
}

So does the commit message claim "with CONFIG_PCIEASPM=n the PCI core's
pci_configure_ltr() is a stub and LTR is enabled nowhere" conflate "the
kernel does not enable LTR" with "LTR is not enabled"?

There is a second consequence in the same configuration.  pci_disable_link_state()
is a stub returning 0 in include/linux/pci.h, so tp->aspm_manageable ends up
set and rtl_hw_aspm_clkreq_enable() still takes the enable branch:

	rtl_enable_ltr(tp);		/* now returns immediately */
	switch (tp->mac_version) {
	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_LAST:
		/* reset ephy tx/rx disable timer */
		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
		/* chip can trigger L1.2 */
		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));

Does this leave the MAC armed for L1.2 with the LTR machinery deliberately
left unconfigured on every CONFIG_PCIEASPM=n build?

Would reading the endpoint's own LTR Mechanism Enable bit, which is
available in any Kconfig setting via
pcie_capability_read_dword(tp->pci_dev, PCI_EXP_DEVCTL2, ...), avoid the
need for the blanket #else return; here?

One more note on the comment wording: ltr_path is also left clear when
DEVCAP2 has no LTR bit, when there is no upstream bridge (root-bus or
RC-integrated endpoint), and when firmware owns LTR (!host->native_ltr).
Is "LTR is enabled for the whole path up to the root port" a stronger
statement than what the core actually computes?

[Severity: Medium]
Does this need to clear the chip's LTR bits rather than just skip
programming them?  Grepping r8169_main.c, ALDPS_LTR/ALDPS_LTR_EN,
LTR_SNOOP/LTR_SNOOP_EN, LTR_OBFF_LOCK/LTR_OBFF_LOCK_EN,
LINK_SPEED_CHANGE_EN and COMBO_LTR_EXTEND/COMBO_LTR_EXTEND_EN appear only
in their register definitions and inside rtl_enable_ltr(), and every access
there is a set/OR:

	r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
	RTL_W8(tp, COMBO_LTR_EXTEND, RTL_R8(tp, COMBO_LTR_EXTEND) | COMBO_LTR_EXTEND_EN);
	...
	/* chip can trigger LTR */
	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);

There is no rtl_disable_ltr(), and the disable branch of
rtl_hw_aspm_clkreq_enable() clears only 0xe092, ClkReqEn/INT_CFG0_CLKREQEN
and ASPM_en:

		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
		...
		rtl_mod_config5(tp, ASPM_en, 0);

If any of those LTR enables is already set when the new early return is
taken (power-on or warm-reboot residue, a boot firmware Realtek driver, or
kexec from a kernel that programmed them), does the MAC keep issuing LTR
messages with LTR Mechanism Enable clear, which is the exact state the
commit message says must not happen?  rtl_hw_reset() only writes ChipCmd
CmdReset:

	static void rtl_hw_reset(struct rtl8169_private *tp)
	{
		RTL_W8(tp, ChipCmd, CmdReset);

		rtl_loop_wait_low(tp, &rtl_chipcmd_cond, 100, 100);
	}

so it is not obvious from the source that those MAC OCP bits come back as 0.

The existing in-tree consumer of ltr_path takes the other approach in
drivers/gpu/drm/amd/amdgpu/nbio_v4_3.c:nbio_v4_3_program_ltr():

	if (adev->pdev->ltr_path)
		data |= BIF_CFG_DEV0_EPF0_DEVICE_CNTL2__LTR_EN_MASK;
	else
		data &= ~BIF_CFG_DEV0_EPF0_DEVICE_CNTL2__LTR_EN_MASK;

Would an explicit clear of the LTR enables on the !ltr_path path, matching
the driver's own ASPM disable convention, be the more robust shape here?

>  	switch (tp->mac_version) {
>  	case RTL_GIGA_MAC_VER_80:
>  		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
-- 
This is an AI-generated review.