Re: [PATCH net-next v2 1/2] net: phy: realtek: add PTP clock for RTL8127a

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] rtl8127a_ptp_remove()
  (drivers/net/phy/realtek/realtek_ptp.c:365-374) unregisters the clock…
- [Medium] The patch adds a PHC whose callbacks
  (gettimex64/settime64/adjtime/adjfine) can reach the PHY's GPHY_OCP…
- [Medium] The PHC is registered at PHY probe and stays reachable from
  userspace for the whole binding.
- [Medium] The PHC is seeded in .config_init, which runs from
  phy_init_hw() before the rest of r8169's PHY bring-up.
- [Low] rtl8127a_ptp_hwtstamp_enable() ends with
  `rtl8127a_ptp_modify(ptp, 0xa640, 0, BIT(15))` (realtek_ptp.c:216),…
- [Low] In rtl8127a_ptp_gettimex64() (realtek_ptp.c:290-293),
  ptp_read_system_prets() and ptp_read_system_postts() bracket the whole…

--

net: phy: realtek: add PTP clock for RTL8127a

This patch registers a PTP hardware clock for the internal PHY of RTL8127A
(RTL8261C, PHY ID 0x001cc890). The PHY's free-running clock is read, set,
stepped and frequency-adjusted through vendor MMD registers, and a driver
mutex serializes multi-register sequences. The clock is seeded from system
time in config_init each time the PHY is initialized.

> diff --git a/drivers/net/phy/realtek/realtek_ptp.c b/drivers/net/phy/realtek/realtek_ptp.c
> new file mode 100644
> index 0000000000000..bd5c0d6988dae
> --- /dev/null
> +++ b/drivers/net/phy/realtek/realtek_ptp.c
> @@ -0,0 +1,374 @@

[ ... ]

> +static int rtl8127a_ptp_read(struct rtl8127a_ptp *ptp, u16 reg)
> +{
> +	return phy_read_mmd(ptp->phydev, MDIO_MMD_VEND2, reg);
> +}
> +
> +static int rtl8127a_ptp_write(struct rtl8127a_ptp *ptp, u16 reg, u16 val)
> +{
> +	return phy_write_mmd(ptp->phydev, MDIO_MMD_VEND2, reg, val);
> +}

[Severity: Medium]
Do these accesses need more than the MDIO bus lock to stay serialized
against r8169? The PTP accessors end up on the MAC's GPHY_OCP register:

phy_read_mmd()
  rtl822x_read_mmd()
    rtlgen_read_mmd()
      rtlgen_read_vend2()
        __mdiobus_c45_read()
          r8169_mdio_read_reg_c45()
            r8168_phy_ocp_read()

r8169 loads the RTL8127A PHY firmware through the same register, and that
path does not take the MDIO bus lock:

rtl8169_up()
  rtl8169_init_phy()
    r8169_hw_phy_config()
      rtl8127a_1_hw_phy_config()
        r8169_apply_firmware()
          rtl_fw_write_firmware()
            rtl_writephy()
              r8168g_mdio_write()
                r8168_phy_ocp_write()
                  RTL_W32(tp, GPHY_OCP, OCPAR_FLAG | (reg << 15) | data);

/dev/ptpN exists from PHY probe onward. A gettimex64, settime64, adjtime
or adjfine call could arrive from phc2sys or a chrony PHC refclock during
interface open, the reset worker, or runtime/system resume. Its GPHY_OCP
transactions could then interleave with the firmware patch writes.

Could that lose or redirect a firmware write, or hand the other
transaction's data back to the PTP code?

The unlocked firmware loader in r8169 predates this patch. This patch adds
a path that userspace polls continuously.

[ ... ]

> +static int rtl8127a_ptp_hwtstamp_enable(struct rtl8127a_ptp *ptp, bool enable)
> +{

[ ... ]

> +	if (ret)
> +		goto out;
> +
> +	ret = rtl8127a_ptp_modify(ptp, 0xa640, 0, BIT(15));

[Severity: Low]
This isn't a bug, but what does BIT(15) of 0xa640 control? The register is
outside the 0xe4xx PTP block and has no define or comment.

Both the enable and the disable branch reach this write. Probe and remove
only call rtl8127a_ptp_hwtstamp_enable(ptp, false), but they still set the
bit, and nothing ever clears it.

If the bit turns on some PHY-core timestamping function, does it stay on
after disable, remove() and driver unbind? The later hardware timestamping
patch in the series keeps the same unconditional write.

> +out:
> +	mutex_unlock(&ptp->lock);

[ ... ]

> +static int rtl8127a_ptp_gettimex64(struct ptp_clock_info *info,
> +				   struct timespec64 *ts,
> +				   struct ptp_system_timestamp *sts)
> +{
> +	struct rtl8127a_ptp *ptp = container_of(info, struct rtl8127a_ptp, info);
> +	int ret;
> +
> +	mutex_lock(&ptp->lock);
> +	ptp_read_system_prets(sts);
> +	ret = rtl8127a_ptp_clkadj(ptp, RTL8127A_PTP_DIRECT_READ);
> +	ptp_read_system_postts(sts);

[Severity: Low]
Should prets/postts surround only the EXEC write that latches the counter?
rtl8127a_ptp_clkadj() first does an MDIO read of CLK_CFG inside
rtl8127a_ptp_modify(), then the EXEC write, then a read_poll_timeout()
(1us sleep, 1ms timeout) until EXEC clears.

All of that falls inside the system-time window. The window is wider than
it needs to be, and its midpoint is shifted away from the actual latch
point.

Taking prets just before the EXEC write and postts just after it, then
polling, would avoid this.

> +	if (!ret)
> +		ret = rtl8127a_ptp_read_time(ptp, RTL8127A_PTP_CFG_TIME, ts);
> +	mutex_unlock(&ptp->lock);

[ ... ]

> +int rtl8127a_ptp_config_init(struct phy_device *phydev)
> +{
> +	struct rtl8127a_ptp *ptp = phydev->priv;
> +	struct timespec64 ts;
> +
> +	if (!ptp)
> +		return 0;
> +
> +	/* (Re)start the clock from the system time whenever the PHY is
> +	 * (re)initialized, e.g. each time the MAC driver brings it up.
> +	 */
> +	ktime_get_real_ts64(&ts);
> +
> +	return rtl8127a_ptp_settime(ptp, &ts);

[Severity: Medium]
Does the time written here survive the rest of the r8169 PHY bring-up?
config_init runs from phy_init_hw(), before the firmware load and the soft
reset:

drivers/net/ethernet/realtek/r8169_main.c:rtl8169_init_phy() {
	phy_init_hw(tp->phydev);
	phy_resume(tp->phydev);
	r8169_hw_phy_config(tp, tp->phydev, tp->mac_version);
	...
	genphy_soft_reset(tp->phydev);
}

rtl8127a_1_hw_phy_config() starts with r8169_apply_firmware(), which loads
the PHY MCU patch and polls for BMCR_RESET. It then writes a number of PHY
parameters.

If the firmware load or the BMCR soft reset resets the PTP block, the
commit message would no longer hold:

  "The clock is started from the system time each time the PHY is
  initialized."

The probe-time programming in rtl8127a_ptp_hwtstamp_enable() (SYNCE_CTL,
INER, CTL, 0xa640) would be lost as well.

With the later hardware timestamping patch, the PTP_CTL/INER/GEN_CFG setup
written by rtl8127a_ptp_hwtstamp_set() is never re-applied after
rtl8169_up() either (open, reset worker, resume). hwtstamp_set() only calls
rtl8127a_ptp_hwtstamp_enable() when tx_type or rx_filter change, so
re-issuing the same configuration would not restore it.

Could hardware timestamping then stop silently after a MAC reset or
resume? The code alone doesn't show whether these registers are actually
cleared; that depends on the hardware.

> +}
> +
> +int rtl8127a_ptp_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct rtl8127a_ptp *ptp;
> +	int ret;
> +
> +	ptp = devm_kzalloc(dev, sizeof(*ptp), GFP_KERNEL);
> +	if (!ptp)
> +		return -ENOMEM;

[ ... ]

> +	ptp->clock = ptp_clock_register(&ptp->info, dev);
> +	if (IS_ERR(ptp->clock))
> +		return PTR_ERR(ptp->clock);

[Severity: Medium]
What happens to PHC users while the r8169 PCI function is
runtime-suspended? The clock stays registered for the whole PHY binding.
r8169 schedules a runtime suspend 10s after the interface goes down or
loses carrier:

drivers/net/ethernet/realtek/r8169_main.c:rtl8169_runtime_idle() {
	...
	if (!netif_running(tp->dev) || !netif_carrier_ok(tp->dev))
		pm_schedule_suspend(device, 10000);
	...
}

In D3hot, r8168_phy_ocp_read() returns 0xffff. r8168_phy_ocp_write() times
out and logs a rtl_loop_wait error, but r8169_mdio_write_reg_c45() still
returns 0.

Every PHC operation (clock_gettime, settime, adjtime, adjfine, and
PTP_SYS_OFFSET_EXTENDED with up to 25 samples) would then send MMIO to a
suspended device. It would busy-wait through the write timeouts and the
1ms EXEC poll and then fail with -ETIMEDOUT.

Other userspace-reachable r8169 paths guard against this: the LED ops use
pm_runtime_resume_and_get() and get_stats64 checks pm_runtime_active().
This PHY-side path has no such guard.

When the link comes back, the resume path re-seeds the clock:

rtl8169_runtime_resume()
  rtl8169_up()
    rtl8169_init_phy()
      phy_init_hw()
        rtl8127a_ptp_config_init()

That steps the PHC to CLOCK_REALTIME. A ptp4l/phc2sys session would see
errors while the cable is unplugged and then a clock jump. This is still
the case at the end of the series.

> +
> +	phydev->priv = ptp;
> +
> +	return 0;
> +}
> +
> +void rtl8127a_ptp_remove(struct phy_device *phydev)
> +{
> +	struct rtl8127a_ptp *ptp = phydev->priv;
> +
> +	if (!ptp)
> +		return;
> +
> +	rtl8127a_ptp_hwtstamp_enable(ptp, false);
> +	ptp_clock_unregister(ptp->clock);
> +}

[Severity: Medium]
Should phydev->priv be cleared here? rtl8127a_ptp_probe() allocates ptp
with devm_kzalloc() on &phydev->mdio.dev. Devres frees it right after
remove() returns, but phydev->priv still points at it.

rtl8127a_ptp_config_init() and rtl8127a_ptp_remove() treat any non-NULL
priv as a live object. Consider this sequence:

  - the realtek PHY driver is unbound through sysfs while r8169 stays
    attached
  - it is rebound, and the probe fails before phydev->priv = ptp
    (devm_kzalloc() failure, an MDIO timeout in
    rtl8127a_ptp_hwtstamp_enable(), a ptp_clock_register() failure, or a
    hwmon init failure in rtl822x_probe())

phy_probe() sets phydev->drv before calling drv->probe and does not clear
it on the out_reset path. The next rtl8169_up() would then do:

rtl8169_init_phy()
  phy_init_hw()
    rtl8127a_ptp_config_init()
      rtl8127a_ptp_settime()
        mutex_lock(&ptp->lock);

Wouldn't that be a use-after-free on the devres memory? The later hardware
timestamping patch keeps the same remove() and only clears mii_ts and
default_timestamp, so the problem remains at the end of the series.

Setting phydev->priv = NULL in rtl8127a_ptp_remove() would avoid it.

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