Re: [PATCH net-next v1 1/3] r8169: make PHY OCP access safe in atomic context

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] When the PHY OCP write handshake never completes, the CPU now
  busy-spins with local IRQs disabled for the full timeout, and the real…

--

r8169: make PHY OCP access safe in atomic context

This commit replaces the sleeping rtl_loop_wait() handshake in
r8168_phy_ocp_read() and r8168_phy_ocp_write() with a busy-wait based on
readl_poll_timeout_atomic(). It also serializes GPHY_OCP accesses with a
new phy_ocp_lock spinlock, so that the upcoming RTL8127A PTP code can
access PHY OCP registers from atomic context.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index cd4ebc654dd0b..96d3a2bd70947 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -1234,30 +1236,52 @@ static bool rtl_ocp_reg_failure(u32 reg)
>  	return WARN_ONCE(reg & 0xffff0001, "Invalid ocp reg %x!\n", reg);
>  }
>  
> -DECLARE_RTL_COND(rtl_ocp_gphy_cond)
> +/* PHY OCP registers may also be accessed from atomic context (e.g. by the
> + * PTP code), therefore busy-wait for completion instead of sleeping.
> + */
> +static int rtl_phy_ocp_wait(struct rtl8169_private *tp, bool high)
>  {
> -	return RTL_R32(tp, GPHY_OCP) & OCPAR_FLAG;
> +	u32 val;
> +	int ret;
> +
> +	ret = readl_poll_timeout_atomic(tp->mmio_addr + GPHY_OCP, val,
> +					!!(val & OCPAR_FLAG) == high, 1, 250);
> +	if (ret && net_ratelimit())
> +		netdev_err(tp->dev, "PHY OCP access timeout\n");
> +
> +	return ret;
>  }
>  
>  static void r8168_phy_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
>  {
> +	unsigned long flags;
> +
>  	if (rtl_ocp_reg_failure(reg))
>  		return;
>  
> +	spin_lock_irqsave(&tp->phy_ocp_lock, flags);
>  	RTL_W32(tp, GPHY_OCP, OCPAR_FLAG | (reg << 15) | data);
> -
> -	rtl_loop_wait_low(tp, &rtl_ocp_gphy_cond, 25, 10);
> +	rtl_phy_ocp_wait(tp, false);
> +	spin_unlock_irqrestore(&tp->phy_ocp_lock, flags);
>  }

[Severity: Low]
What happens here when the device stops responding and MMIO reads return
all-ones, for example after surprise removal, a PCIe link down or during
AER recovery?

In that case OCPAR_FLAG always reads as set. rtl_phy_ocp_wait(tp, false)
never sees its condition met and spins until the timeout, with local
interrupts disabled by spin_lock_irqsave().

Is the timeout actually 250us? poll_timeout_us_atomic() in
include/linux/iopoll.h only subtracts the udelay() time, plus 1ns per
iteration, from the budget:

		if (__delay_us) { \
			udelay(__delay_us); \
			if (__timeout_us) \
				__left_ns -= __delay_ns; \
		} \
		cpu_relax(); \
		if (__timeout_us) \
			__left_ns--; \

The time spent in the non-posted readl() is not counted. So each write
that times out would keep interrupts off for about 250 x (1us + readl
latency). That is longer than the commit message says:

  "keeping the previous maximum wait time of 250us"

Long write sequences would hit one such window per write. Examples are
the rtl_fw PHY firmware download, the r8169_phy_config.c tables via
rtl_writephy()->r8168g_mdio_write(), phylib MDIO ops such as
r8169_mdio_write_reg_c45(), and the r8127 SFP init.

Before this patch, the same failure path slept in rtl_loop_wait() via
fsleep(25) and left interrupts enabled.

The read path in r8168_phy_ocp_read() doesn't seem to have this problem,
because an all-ones value meets the "flag high" condition right away.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109023022.770-1-javen_xu%40realsil.com.cn