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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] rtl_enable_ltr()
  (drivers/net/ethernet/realtek/r8169_main.c:3038-3048) gates all chip…
- [Medium] The new gate only skips *programming* the chip's LTR
  machinery; nothing ever programs it to the off state.

--

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

This adds a check in rtl_enable_ltr() that reads the endpoint's
PCI_EXP_DEVCTL2 register and returns early when PCI_EXP_DEVCTL2_LTR_EN is
clear, so the chip's LTR machinery is not programmed on hierarchies where
LTR was not enabled.

Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2529752

> 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;

[Severity: High]
Is the endpoint's LTR Mechanism Enable bit really what the PCI core uses to
reflect an end-to-end LTR path?

Looking at pci_configure_ltr() in drivers/pci/pcie/aspm.c, when the bit is
already set (for example by firmware), the core leaves it alone and only sets
the path flag if the upstream bridge already has one:

	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;

		return;
	}

So on a machine where firmware set LTR_EN on the r8169 function but the
hierarchy above it has no LTR path, the function returns with LTR_EN still set
and pdev->ltr_path left at 0.

The whole-path result appears to live in pci_dev::ltr_path in
include/linux/pci.h instead:

	unsigned int	ltr_path:1;	/* Latency Tolerance Reporting
					   supported from root to here */

and aspm.c uses that field, not DEVCTL2, when deciding L1.2 is unusable:

	if (!child->ltr_path)
		child_l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;

If that reading is right, then on the platform class described in the changelog
the new test passes, rtl_hw_start() -> rtl_hw_aspm_clkreq_enable(tp, true) ->
rtl_enable_ltr() still programs ALDPS_LTR_EN, LTR_SNOOP_EN,
LINK_SPEED_CHANGE_EN and finally LTR_OBFF_LOCK_EN, and the chip is still let to
trigger L1.2. Does the link-flap case the changelog reports stay reachable
in that configuration?

There is also the CONFIG_PCIEASPM=n build, where pci_configure_ltr() is an
empty stub in drivers/pci/pci.h while pci_disable_link_state() is a stub
returning 0, so tp->aspm_manageable stays 1 and rtl_enable_ltr() still runs.
In that build no path discovery has happened at all - is testing the bit
meaningful there, or is it just leftover firmware state?

Alongside that, could the comment and the commit message be adjusted? The
comment states the core "reflects it in LTR Mechanism Enable", and the changelog
says the core "records the result by setting LTR Mechanism Enable in the
endpoint's Device Control 2 register", which does not hold for the
firmware-already-set case shown above.

[Severity: Medium]
This early return skips the programming, but does anything ever program the
chip's LTR bits to the off state?

Grepping r8169_main.c, ALDPS_LTR_EN, LTR_SNOOP_EN, LTR_OBFF_LOCK_EN,
LINK_SPEED_CHANGE_EN and COMBO_LTR_EXTEND_EN only ever appear in set
operations inside rtl_enable_ltr():

	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);

The disable branch of rtl_hw_aspm_clkreq_enable() clears only 0xe092,
ClkReqEn/INT_CFG0_CLKREQEN and ASPM_en, so those LTR bits stay armed.

If they were armed earlier and rtl_enable_ltr() later takes the new early
return - say a pre-boot UEFI Realtek driver configured ASPM plus LTR, or an
earlier call ran while LTR_EN read as set and the bit was cleared afterwards,
with rtl_reset_work() -> rtl_hw_start() re-entering this path on the same
device - does the chip keep emitting LTR messages with LTR Mechanism Enable
clear and ALDPS still tied to them?

Would writing the off values for those bits when the platform has not enabled
LTR be more robust than skipping the writes, so the driver's view and the
chip's state cannot diverge?

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