Re: [PATCH net-next v2 2/2] net: phy: realtek: add hardware timestamping for RTL8127a

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 · Medium: 7 · 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).

- [High] rtl8127a_ptp_txtstamp()
  (drivers/net/phy/realtek/realtek_ptp.c:508-526) hands the driver's…
- [High] rtl8127a_ptp_remove() (lines 702-719) clears phydev->mii_ts with
  a plain store and immediately tears down: ptp_clock_unregister()…
- [High] rtl8127a_ptp_rxtstamp() (lines 528-553) takes ownership of every
  received PTP event packet and queues it on ptp->rx_queue with…
- [Medium] While a TX timestamp is pending, rtl8127a_ptp_do_aux_work()
  (line 494) returns 0.
- [Medium] rtl8127a_ptp_config_init() (lines 645-659) runs on every
  phy_init_hw(), for example from rtl8169_init_phy()…
- [Medium] rtl8127a_ptp_egress_time() (lines 229-265) treats a failed or
  ambiguous TX-ready poll as if a Sync timestamp were ready.
- [Medium] rtl8127a_ptp_hwtstamp_set() stores the new state
  (`WRITE_ONCE(ptp->tx_type, ...)` and `ptp->rx_filter = ...`) before…
- [Medium] RX timestamps are read late from what the register interface
  shows as one capture per message type (TRX_TS_STA SEL_RX + MSGTYPE ->…
- [Medium] rtl8127a_ptp_hwtstamp_set() reprograms the hardware without
  coordinating with an in-flight TX timestamp.
- [Medium] The driver tracks a single TX timestamp in software, but the
  PHY timestamps every outgoing event message, and nothing ties the…
- [Low] The patch changes REALTEK_PHY_PTP to `depends on PTP_1588_CLOCK
  && NETWORK_PHY_TIMESTAMPING` (drivers/net/phy/realtek/Kconfig:19).
- [Low] rtl8127a_ptp_ts_info() (lines 622-643) advertises rx_filters
  NONE, PTP_V2_EVENT, PTP_V2_L4_EVENT, PTP_V2_SYNC, PTP_V2_L4_SYNC,…

--

net: phy: realtek: add hardware timestamping for RTL8127a

This commit adds mii_timestamper support to the RTL8127A internal PHY so
that the PHY hardware timestamps PTPv2 event messages. The PTP aux worker
reads both TX and RX timestamps over MDIO. Only one TX timestamp can be
pending at a time, and it is dropped after 15 seconds.

> diff --git a/drivers/net/phy/realtek/Kconfig b/drivers/net/phy/realtek/Kconfig
> index 93a8fb552834f..ab375c42a7f22 100644
> --- a/drivers/net/phy/realtek/Kconfig
> +++ b/drivers/net/phy/realtek/Kconfig
> @@ -16,12 +16,12 @@ config REALTEK_PHY_HWMON
>  config REALTEK_PHY_PTP
>  	bool "PTP support for Realtek PHYs"
>  	default y
> -	depends on PTP_1588_CLOCK
> +	depends on PTP_1588_CLOCK && NETWORK_PHY_TIMESTAMPING

[Severity: Low]
Does this remove the existing PHC support on configs that don't enable
NETWORK_PHY_TIMESTAMPING?

NETWORK_PHY_TIMESTAMPING in net/Kconfig is a bool with no default. Its
help text says "If you are unsure how to answer this question, answer N."

Take an existing .config with PTP_1588_CLOCK=y but no
NETWORK_PHY_TIMESTAMPING. olddefconfig will now silently set
REALTEK_PHY_PTP=n, and the RTL8127A PHC goes away even though it only
needed PTP_1588_CLOCK before.

Only the mii_timestamper part seems to need PHY timestamping. Could that
part be made conditional instead? If not, could the commit message
mention that the PHC now needs this option?

[ ... ]

> diff --git a/drivers/net/phy/realtek/realtek_ptp.c b/drivers/net/phy/realtek/realtek_ptp.c
> index bd5c0d6988dae..5e02707620064 100644
> --- a/drivers/net/phy/realtek/realtek_ptp.c
> +++ b/drivers/net/phy/realtek/realtek_ptp.c

[ ... ]

> @@ -171,6 +206,64 @@ static int rtl8127a_ptp_write_time(struct rtl8127a_ptp *ptp,

[ ... ]

> +static int rtl8127a_ptp_trx_ts_cmd(struct rtl8127a_ptp *ptp, u16 cmd)
> +{
> +	int ret;
> +
> +	ret = rtl8127a_ptp_modify(ptp, RTL8127A_PTP_TRX_TS_STA,
> +				  RTL8127A_PTP_TRX_TS_SEL_RX |
> +				  RTL8127A_PTP_TRX_TS_MSGTYPE,
> +				  RTL8127A_PTP_TRX_TS_RD | cmd);

[ ... ]

> +static int rtl8127a_ptp_egress_time(struct rtl8127a_ptp *ptp,
> +				    struct timespec64 *ts)
> +{

[ ... ]

> +	val = rtl8127a_ptp_poll(ptp, RTL8127A_PTP_TRX_TS_STA,
> +				RTL8127A_PTP_TX_RDY, true);
> +	if (val < 0)
> +		val = 0;
> +
> +	switch (val & RTL8127A_PTP_TX_RDY) {

[ ... ]

> +	case RTL8127A_PTP_TX_SYNC_RDY:
> +	default:
> +		msgtype = PTP_MSGTYPE_SYNC;
> +		break;
> +	}

[Severity: Medium]
What happens here when the poll fails, or when more than one ready bit is
set?

If rtl8127a_ptp_poll() gets an MDIO error or times out after 1 ms, val
becomes 0. If several of bits 12-15 are set, none of the single-bit cases
match. Either way the code takes the default branch, latches the Sync
capture, and rtl8127a_ptp_egress_time() returns 0.

rtl8127a_ptp_tx_tstamp() then hands that value to
skb_complete_tx_timestamp() as a valid hardware timestamp, whatever the
pending skb's real message type is. For example, a Delay_Req could get
an earlier, unrelated Sync's egress time.

More than one ready bit can be set because the hardware still
timestamps frames whose clones were dropped while the slot was busy.
Should the poll failure and the ambiguous case return an error instead?
The pending skb's msgtype is also never recorded, so there is nothing to
compare against.

Also, rtl8127a_ptp_trx_ts_cmd() does a read-modify-write of TRX_TS_STA.
That writes the TX_*_RDY status bits back along with the command bits.
If those bits are write-1-to-clear, could an RX latch clear a fresh TX
ready bit?

[ ... ]

> @@ -306,6 +399,101 @@ static int rtl8127a_ptp_settime64(struct ptp_clock_info *info,

[ ... ]

> +/* Returns true if no TX timestamp is pending anymore */
> +static bool rtl8127a_ptp_tx_tstamp(struct rtl8127a_ptp *ptp)
> +{

[ ... ]

> +	if (time_is_before_jiffies(ptp->tx_start + RTL8127A_PTP_TX_TIMEOUT)) {
> +		rtl8127a_ptp_write(ptp, RTL8127A_PTP_INSR,
> +				   RTL8127A_PTP_INT_TX_TS);
> +		mutex_unlock(&ptp->lock);
> +
> +		rtl8127a_ptp_tx_complete(ptp, NULL);
> +		return true;
> +	}
> +
> +	ret = rtl8127a_ptp_read(ptp, RTL8127A_PTP_INSR);
> +	if (ret < 0 || !(ret & RTL8127A_PTP_INT_TX_TS)) {
> +		mutex_unlock(&ptp->lock);
> +		return false;
> +	}
> +
> +	ret = rtl8127a_ptp_write(ptp, RTL8127A_PTP_INSR,
> +				 RTL8127A_PTP_INT_TX_TS);
> +	if (!ret)
> +		ret = rtl8127a_ptp_egress_time(ptp, &ts);

[Severity: Medium]
How is the TX_TS event tied to the skb in ptp->tx_skb?

The driver tracks one TX timestamp in software, but the PHY timestamps
every outgoing event message. When the slot is busy,
rtl8127a_ptp_txtstamp() frees the clone. The original frame is still
sent and timestamped, which sets its TX_*_RDY bit and INSR TX_TS.

Suppose an untracked frame raises TX_TS after the INSR clear above. The
next armed skb can see TX_TS on the worker's first pass, possibly before
its own frame has left the MAC, and get the untracked frame's timestamp.

An untracked frame of the same type can also overwrite the per-type
capture before the pending skb is latched. One of a different type
leaves a stale ready bit, which later sends rtl8127a_ptp_egress_time()
into the SYNC fallback.

The timeout path clears only INSR TX_TS, not the latched ready bits.
Nothing clears stale TX_TS or ready state when rtl8127a_ptp_txtstamp()
arms a new skb, and the skb's msgtype and sequenceId are never recorded
or compared.

Can this attach TX timestamps to the wrong packet when two event
messages go out close together? Examples are multiple PTP instances,
overlapping P2P Pdelay_Req/Resp, or ptp4l moving on after its
tx_timestamp_timeout.

[ ... ]

> +static void rtl8127a_ptp_rx_tstamp(struct rtl8127a_ptp *ptp,
> +				   struct sk_buff *skb)
> +{

[ ... ]

> +	mutex_lock(&ptp->lock);
> +	ret = rtl8127a_ptp_trx_ts_cmd(ptp, RTL8127A_PTP_TRX_TS_SEL_RX |
> +					   FIELD_PREP(RTL8127A_PTP_TRX_TS_MSGTYPE,
> +						      msgtype) |
> +					   RTL8127A_PTP_TRX_TS_EXEC);
> +	if (!ret)
> +		ret = rtl8127a_ptp_read_time(ptp, RTL8127A_PTP_TRX_TS_TIME,
> +					     &ts);
> +	mutex_unlock(&ptp->lock);
> +
> +	if (!ret)
> +		skb_hwtstamps(skb)->hwtstamp = timespec64_to_ktime(ts);

[Severity: Medium]
Can this give the skb another packet's ingress time?

The register interface appears to hold one RX capture per message type,
and the aux worker reads it here some time after the frame arrived. The
code does not check INSR RX_TS or any valid flag. It does not compare the
sequenceId, and it does not acknowledge RX_TS, even though
rtl8127a_ptp_hwtstamp_enable() unmasks it in INER.

If two event messages of the same type arrive before the worker handles
the first one, the first skb gets the second packet's time. A frame the
hardware did not capture gets a stale value, which is still reported as
a valid hardware timestamp.

rtl8127a_ptp_rxtstamp() also does not reject PTPv1 frames. For v1,
ptp_get_msgtype() returns the control field, so v1 frames with control
<= 3 are queued and latched against the v2 per-type captures.

Anyone who can send event messages to UDP port 319 can trigger this.
Could a remote sender make ptp4l's genuine Sync or Delay_Req carry the
arrival time of the sender's own packet?

I could not confirm whether the hardware keeps one register per type or
a FIFO.

> +
> +	netif_rx(skb);
> +}
> +
> +static long rtl8127a_ptp_do_aux_work(struct ptp_clock_info *info)
> +{
> +	struct rtl8127a_ptp *ptp = container_of(info, struct rtl8127a_ptp, info);
> +	struct sk_buff *skb;
> +
> +	while ((skb = skb_dequeue(&ptp->rx_queue)))
> +		rtl8127a_ptp_rx_tstamp(ptp, skb);
> +
> +	/* Keep polling until the pending TX timestamp is available */
> +	return rtl8127a_ptp_tx_tstamp(ptp) ? -1 : 0;
> +}

[Severity: Medium]
Does returning 0 here make the aux worker busy-poll MDIO?

ptp_aux_kworker() requeues the work with the returned delay:

drivers/ptp/ptp_clock.c:ptp_aux_kworker() {
    ...
	delay = info->do_aux_work(info);

	if (delay >= 0)
		kthread_queue_delayed_work(ptp->kworker, &ptp->aux_work, delay);
}

So while a TX timestamp is pending, the worker runs again immediately.
Each pass takes ptp->lock and reads RTL8127A_PTP_INSR over MDIO. That
competes with phylib, and with gettimex64/adjfine/adjtime calls from
phc2sys and ptp4l.

TX_TS may never arrive: the frame was dropped, the link is down, the
packet was not an event message, timestamping was disabled, or
rtl8127a_ptp_hwtstamp_enable() cleared INSR. In that case the polling
lasts the full 15 second RTL8127A_PTP_TX_TIMEOUT, and every other TX
timestamp request is dropped in the meantime.

Would a non-zero delay be more appropriate here? The commit message only
says:

    A pending TX timestamp is dropped after 15 seconds.

It does not mention the busy polling, or that TX timestamping is blocked
during that time.

[ ... ]

> @@ -314,8 +502,146 @@ static const struct ptp_clock_info rtl8127a_ptp_info = {

[ ... ]

> +static void rtl8127a_ptp_txtstamp(struct mii_timestamper *mii_ts,
> +				  struct sk_buff *skb, int type)
> +{
> +	struct rtl8127a_ptp *ptp = container_of(mii_ts, struct rtl8127a_ptp,
> +						mii_ts);
> +
> +	/* Only one TX timestamp can be pending at a time */
> +	if (READ_ONCE(ptp->tx_type) != HWTSTAMP_TX_ON ||
> +	    test_and_set_bit_lock(RTL8127A_PTP_TX_IN_PROGRESS, &ptp->state)) {
> +		kfree_skb(skb);
> +		return;
> +	}

[Severity: High]
Can an unprivileged sender take the only TX timestamp slot here?

skb_clone_tx_timestamp() calls ->txtstamp() for every socket-owned skb
the core classifies as PTP, whether or not a hardware timestamp was
requested. The classifier accepts any UDP datagram to destination port
319 regardless of messageType, and any L2 0x88F7 frame without the
general bit.

This check does not parse the PTP header and does not reject non-event
message types, which rtl8127a_ptp_rxtstamp() does on the RX side. It
also does not check the PTP version or SKBTX_HW_TSTAMP.

So while ptp4l runs with HWTSTAMP_TX_ON, an unprivileged local user can
send a UDP/319 datagram with a non-event messageType:

  sendto() to UDP dport 319
    rtl8169_start_xmit()
      skb_tx_timestamp()
        skb_clone_tx_timestamp()
          rtl8127a_ptp_txtstamp()   <- takes the slot

The hardware does not timestamp that datagram. Every ptp4l Sync or
Delay_Req clone sent afterwards fails test_and_set_bit_lock() and is
freed.

The hardware still timestamps ptp4l's frame and raises INSR TX_TS.
rtl8127a_ptp_tx_tstamp() then attaches that egress time to the other
user's skb and queues it on that socket's error queue.

If no later event message is sent, the slot stays held for the full 15
second timeout while the aux worker polls MDIO. Should this path check
SKBTX_HW_TSTAMP and filter with ptp_parse_header() and
ptp_get_msgtype(), as the RX side does?

[ ... ]

> +	/* Only event messages are timestamped by the hardware */
> +	msgtype = ptp_get_msgtype(hdr, type);
> +	if (msgtype > PTP_MSGTYPE_PDELAY_RESP)
> +		return false;
> +
> +	RTL8127A_PTP_SKB_CB(skb)->msgtype = msgtype;
> +	skb_queue_tail(&ptp->rx_queue, skb);
> +	ptp_schedule_worker(ptp->clock, 0);

[Severity: High]
Is anything limiting the length of ptp->rx_queue?

skb_defer_rx_timestamp() runs in the RX softirq for any frame the
classifier marks as PTP, from any sender: a UDP datagram to port 319 or
a non-general L2 0x88F7 frame. Every event message is queued here with
no length cap, drop policy or expiry.

The only consumer is rtl8127a_ptp_do_aux_work(). It handles one skb at a
time under ptp->lock: a read-modify-write of TRX_TS_STA, a poll with a
timeout of up to 1 ms, and five time register reads. Each of these goes
through r8169's OCP accessors.

While queued, these skbs bypass netdev_max_backlog and socket memory
accounting. Could a remote host flooding UDP/319 over a 10GbE link make
this queue grow without bound, so that the worker's while loop never
exits?

[ ... ]

> +	if (ptp->tx_type != config->tx_type ||
> +	    ptp->rx_filter != config->rx_filter) {
> +		WRITE_ONCE(ptp->tx_type, config->tx_type);
> +		ptp->rx_filter = config->rx_filter;
> +		ret = rtl8127a_ptp_hwtstamp_enable(ptp, enable);
> +	}
> +	WRITE_ONCE(ptp->rx_enabled, rx_enabled);

[Severity: Medium]
What happens to the cached state if rtl8127a_ptp_hwtstamp_enable()
fails?

tx_type and rx_filter are updated before the hardware is programmed.
rx_enabled is updated whatever the return value is.

rtl8127a_ptp_hwtstamp_enable() is a series of separate MDIO
read-modify-writes (SYNCE_CTL, INER, GEN_CFG, INSR, CTL, 0xa640) with no
rollback. An -ETIMEDOUT partway through can leave the block half
configured, for example with INER unmasked but CTL not enabled.

After such an error:

- rtl8127a_ptp_txtstamp() sees HWTSTAMP_TX_ON and arms TX timestamps
  that never arrive.
- rtl8127a_ptp_rxtstamp() sees rx_enabled and attaches stale captures.
- rtl8127a_ptp_hwtstamp_get() reports a configuration the hardware is
  not running.

A retry with the same configuration then matches the cached state,
skips the reprogramming and returns 0. Should the new state be stored
only after the hardware has been programmed successfully?

[Severity: Medium]
Is a pending TX timestamp handled during this reconfiguration?

Any filter change while TX is on calls rtl8127a_ptp_hwtstamp_enable(ptp,
true), which does:

	ret = rtl8127a_ptp_write(ptp, RTL8127A_PTP_INSR, 0xffff);

That clears a TX_TS event latched for ptp->tx_skb before the aux worker
reads it.

tx_type is also set to HWTSTAMP_TX_ON before the hardware is enabled. A
concurrent rtl8127a_ptp_txtstamp() can therefore arm the slot for a
frame that goes out before PTP is enabled.

Switching TX off does not cancel a pending tx_skb either. That includes
one armed by an rtl8127a_ptp_txtstamp() call that read the old tx_type.

In each case RTL8127A_PTP_TX_IN_PROGRESS stays set until the 15 second
timeout. Every new TX timestamp request is freed during that time, and
rtl8127a_ptp_do_aux_work() keeps polling INSR under ptp->lock.

[ ... ]

> +	info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> +			   BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |
> +			   BIT(HWTSTAMP_FILTER_PTP_V2_L4_EVENT) |
> +			   BIT(HWTSTAMP_FILTER_PTP_V2_SYNC) |
> +			   BIT(HWTSTAMP_FILTER_PTP_V2_L4_SYNC) |
> +			   BIT(HWTSTAMP_FILTER_PTP_V2_DELAY_REQ) |
> +			   BIT(HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ);

[Severity: Low]
Should the L2 filters be listed here too?

rtl8127a_ptp_hwtstamp_set() accepts HWTSTAMP_FILTER_PTP_V2_L2_EVENT,
HWTSTAMP_FILTER_PTP_V2_L2_SYNC and HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ,
and rtl8127a_ptp_rxtstamp() handles PTP_CLASS_L2 frames. ethtool -T and
the tsinfo netlink reply will not report the L2 filters as supported.

> +
> +	return 0;
> +}
> +
>  int rtl8127a_ptp_config_init(struct phy_device *phydev)
>  {
>  	struct rtl8127a_ptp *ptp = phydev->priv;

[Severity: Medium]
Should rtl8127a_ptp_config_init() also restore the timestamping
configuration?

It runs on every phy_init_hw(), for example from rtl8169_init_phy() on
open, reset and resume, but it only steps the clock again:

drivers/net/phy/realtek/realtek_ptp.c:rtl8127a_ptp_config_init() {
    ...
	/* (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);
}

That comment suggests the PTP block's state may not survive a PHY
re-init. If CTL and INER are reset as well, ptp->tx_type, ptp->rx_filter
and rtl8127a_ptp_hwtstamp_get() still report timestamping as enabled
while the hardware has it off. TX skbs then wait out the 15 second
timeout with busy polling, and RX skbs get stale captures.

Sending the same SIOCSHWTSTAMP again does not help.
rtl8127a_ptp_hwtstamp_set() only reprograms the hardware when the
requested config differs from the cached one.

Stepping the PHC here while timestamping is active also means captures
pending across the step can mix two timebases.

I could not confirm whether PHY re-init or power-down actually clears
these vendor PTP registers.

[ ... ]

> @@ -369,6 +706,14 @@ void rtl8127a_ptp_remove(struct phy_device *phydev)
>  	if (!ptp)
>  		return;
>  
> +	phydev->mii_ts = NULL;
> +	phydev->default_timestamp = false;
> +
>  	rtl8127a_ptp_hwtstamp_enable(ptp, false);
> +	/* Also stops the aux worker */
>  	ptp_clock_unregister(ptp->clock);
> +
> +	if (ptp->tx_skb)
> +		rtl8127a_ptp_tx_complete(ptp, NULL);
> +	skb_queue_purge(&ptp->rx_queue);
>  }

[Severity: High]
What stops the datapath from still using ptp after this function
returns?

phydev->mii_ts is cleared with a plain store, and teardown follows
straight away. There is no synchronize_net() or RCU grace period, and
rtnl_lock is not taken. devres frees ptp after remove returns.

phy_remove() calls drv->remove() without detaching the netdev. A sysfs
unbind of the PHY driver can therefore run this while traffic is
flowing.

skb_clone_tx_timestamp() and skb_defer_rx_timestamp() read
phydev->mii_ts without locking. A CPU that has already loaded it can
still call rtl8127a_ptp_txtstamp() or rtl8127a_ptp_rxtstamp() after this
point. Those calls could:

- call ptp_schedule_worker() on a destroyed kworker
- touch the devm-freed struct
- leak a TX clone that holds a socket reference, if it is published
  after the ptp->tx_skb check
- queue an RX skb after the purge

rtl8127a_ptp_ts_info(), rtl8127a_ptp_hwtstamp_get() and
rtl8127a_ptp_hwtstamp_set() run under rtnl, which this path does not
take. They can also call ptp_clock_index() on a freed clock.

There is a second case. If the PHY was selected as hwprov through
ethtool tsconfig, dev->hwprov is only cleared in phy_detach(), not on
driver unbind. After this remove, skb_defer_rx_timestamp() takes the
hwprov path, skips phy_is_default_hwtstamp(), and then dereferences the
now NULL mii_ts:

net/core/timestamping.c:skb_defer_rx_timestamp() {
    ...
	mii_ts = phydev->mii_ts;
	if (likely(mii_ts->rxtstamp))
		return mii_ts->rxtstamp(mii_ts, skb, type);
    ...
}

Would any received PTP frame then oops in the RX softirq?

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