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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 10.09.2026 08:01, Yogesh Gaur wrote:
> On Wed, Sep 9, 2026 at 10:05 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 09.09.2026 13:05, 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 determines that in pci_configure_ltr() and
>>> records the result by setting LTR Mechanism Enable in the endpoint's
>>> Device Control 2 register; per PCIe r6.0 sec 7.5.3.16 a function must not
>>> issue LTR messages while that bit is clear.
>>>
>>> So on a platform whose hierarchy has no LTR path, the driver now 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.
>>>
>>> Read the endpoint's LTR Mechanism Enable bit and leave the chip's LTR
>>> machinery alone when the platform did not enable it.
>>> pcie_capability_read_word() zeroes its output on error, so an unreadable
>>> capability takes the same safe path.
>>>
>>
>> Thanks for the fix!
>>
>>> Fixes: 9ab94a32af70 ("r8169: enable LTR support")
>>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2529752
>>> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
>>> ---
>>>  drivers/net/ethernet/realtek/r8169_main.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>>> index ec4fc21fa21f..c1ff4e898570 100644
>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> @@ -3037,6 +3037,16 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
>>>
>>>  static void rtl_enable_ltr(struct rtl8169_private *tp)
>>>  {
>>> +     u16 ctl2;
>>> +
>>> +     /* The chip must not issue LTR messages unless the platform enabled
>>> +      * LTR on the whole path up to the root port. The PCI core discovers
>>> +      * that in pci_configure_ltr() and reflects it in LTR Mechanism Enable.
>>> +      */
>>> +     pcie_capability_read_word(tp->pci_dev, PCI_EXP_DEVCTL2, &ctl2);
>>> +     if (!(ctl2 & PCI_EXP_DEVCTL2_LTR_EN))
>>> +             return;
>>> +
>>
>> Can't you simply query tp->pci_dev->ltr_path instead of doing this low-level
>> PCI register read? When reading through pci_configure_ltr(), I think this
>> should do the trick.
>>
> Thats was actually my first version, but it does not build: struct
> pci_dev::ltr_path
> is inside #ifdef CONFIG_PCIEASPM (include/linux/pci.h), and r8169 can be built
> with CONFIG_PCIEASPM=n.
> 
However, w/o support for PCIe NIC's you don't need rtl_enable_ltr() at all.
So the complete function could be conditionally compiled.

> I think we should keep DEVCTL2 read. It is what other drivers with this need do
> - rtw89(rtw89_pci_dev_ltr_enabled()), iwlwifi (pcie/gen1_2/trans.c),
> qed_rdma.c,
> rtsx_pcr.c all read PCI_EXP_DEVCTL2 and test PCI_ECP_DEVCTL2_LTR_EN.
> 
I'd not consider these old drivers as role models. As far as possible
I'd like to leave dealing with low-level PCI registers to the PCI subsystem.

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