Re: [PATCH net-next v2 6/6] r8169: fix RTL8116af can not enter s0idle and c10

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8169: fix RTL8116af can not enter s0idle and c10

RTL8116AF is a multi-function device where functions 2 to 7 are hidden from
the PCI core. This commit puts these hidden functions into D3hot and enables
ASPM on their PCIe link control register by writing to their configuration
spaces.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index a49e7a8d8f4da..bc00cdc3add65 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -356,6 +356,9 @@ enum rtl_registers {
>  #define LINK_SPEED_CHANGE_EN		BIT(14)
>  #define LTR_SNOOP_EN			GENMASK(15, 14)
>  #define LTR_MSG_EN			BIT(0)
> +#define RTL8116AF_FUNC_PM_CSR		0x80
> +#define RTL8116AF_FUNC_EXP_LNKCTL	0x44

[Severity: High]
Are these offsets swapped?

In typical Realtek PCIe configuration space, the PM capability is at 0x40
(making PM_CSR offset 0x44) and the PCIe capability is at 0x70 (making Link
Control offset 0x80).

Because these definitions might be swapped, the code later reads 0x80 (Link
Control) and applies PCI_PM_CTRL_PME_ENABLE, and reads 0x44 (PM_CSR) and
applies PCI_EXP_LNKCTL_ASPMC. Could this cross-wiring fail to set PME_ENABLE
in the PM_CSR register as intended?

> +#define RTL_PM_D3HOT			GENMASK(1, 0)
>  };
>  
[ ... ]
> @@ -3731,6 +3734,33 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
>  	r8168_mac_ocp_modify(tp, 0xe860, 0x0000, 0x0080);
>  }
>  
> +static void rtl_disable_hidden_function(struct pci_dev *pdev)
> +{
> +	unsigned int slot = PCI_SLOT(pdev->devfn);
> +	struct pci_bus *bus = pdev->bus;
> +	unsigned int devfn;
> +	int func;
> +	int ret;
> +	u32 val;
> +
> +	for (func = 2; func < 8; func++) {
> +		devfn = PCI_DEVFN(slot, func);
> +
> +		ret = pci_bus_read_config_dword(bus, devfn, RTL8116AF_FUNC_PM_CSR, &val);
> +		if (!ret && !PCI_POSSIBLE_ERROR(val)) {

[Severity: High]
Is it possible to verify that the function is the intended hidden device
before proceeding?

The commit message notes that the hidden functions return an all-ones
response when their vendor ID is read. However, this code reads offset 0x80
and checks that it did not return 0xFFFFFFFF. If a legitimate, OS-enumerated
function exists at any of these indices (e.g., a secondary NIC, Wi-Fi, or SD
card reader), the read will succeed with valid register data and the driver
will overwrite its registers at 0x80 and 0x44.

Should the loop read the Vendor ID (offset 0x00) and verify it returns
0xFFFFFFFF to ensure the function is truly hidden before mutating its
configuration space?

> +			val &= ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE);
> +			val |= (RTL_PM_D3HOT | PCI_PM_CTRL_PME_ENABLE);
> +			pci_bus_write_config_dword(bus, devfn, RTL8116AF_FUNC_PM_CSR, val);

[Severity: High]
Can these 32-bit read-modify-write operations on natively 16-bit registers
unintentionally clear adjacent Write-1-to-Clear (W1C) status bits?

Reading a 32-bit value from offset 0x80 fetches both the Link Control (lower
16 bits) and Link Status (upper 16 bits) registers. Link Status contains W1C
bits. Writing the unmodified 32-bit value back writes 1s to any W1C bits
that were set, clearing them.

The same applies to the 16-bit PM_CSR register, where writing back the 32-bit
value might clear the W1C PME_Status bit (bit 15) if it was set, and
needlessly overwrite the adjacent 8-bit PM_Data and PM_Bridge_Ext registers.

Would it be safer to use pci_bus_read_config_word() and
pci_bus_write_config_word(), and explicitly mask out the PME_Status bit when
writing PM_CSR?

> +		}
> +
> +		ret = pci_bus_read_config_dword(bus, devfn, RTL8116AF_FUNC_EXP_LNKCTL, &val);
> +		if (!ret && !PCI_POSSIBLE_ERROR(val)) {
> +			val |= PCI_EXP_LNKCTL_ASPMC;
> +			pci_bus_write_config_dword(bus, devfn, RTL8116AF_FUNC_EXP_LNKCTL, val);
> +		}
> +	}
> +}