mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: netdev-bot+sashiko@kernel.org
To: javen_xu@realsil.com.cn
Cc: hkallweit1@gmail.com, nic_swsd@realtek.com,
	andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com, horms@kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v1 3/3] r8169: add hardware timestamping support for RTL8127a
Date: Thu, 12 Nov 2026 05:33:27 +0000	[thread overview]
Message-ID: <179446160712.2199510.8409224902993653061@kernel.org> (raw)
In-Reply-To: <20261109023022.770-4-javen_xu@realsil.com.cn>

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

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

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] Timestamping is not stopped or restored on suspend, runtime
  suspend, resume or shutdown.
- [High] TX hardware timestamps are not tied to the skb that requested
  them.
- [Medium] The polling scheme for TX and RX timestamps is CPU-intensive
  and runs with IRQs off.
- [Medium] rtl8169_ptp_rx_tstamp() reads the PHY's per-message-type 'last
  received' latch when NAPI processes the skb, which can be well after…
- [Medium] Failed or all-ones register reads are treated as valid
  timestamps.
- [Medium] rtl8169_ptp_get_ts_info() reads ptp->clock and calls
  ptp_clock_index(ptp->clock) without RTNL or any driver lock.
- [Low] rtl8169_ptp_get_ts_info() and rtl8169_ptp_hwtstamp_set() disagree
  on the supported RX filters.
- [Low] The uAPI behaves inconsistently when no PHC is registered.
- [Low] The RX path stamps PTPv1 general messages, even though the driver
  only accepts PTPv2 filters and reports PTP_V2_EVENT.
- [Low] rtl8169_ptp_get_ts_stats() maps the driver counters onto
  ethtool_ts_stats fields in ways that contradict their kernel-doc.
- [Low] In rtl8169_start_xmit(), skb_tx_timestamp(skb) runs before
  rtl8169_ptp_tx_tstamp() sets SKBTX_IN_PROGRESS.
- [Low] rtl8169_ptp_tx_tstamp() runs in ndo_start_xmit with only the TX
  queue lock and reads ptp->config.tx_type with a plain load.
- [Low] ptp->tx_pkts and ptp->tx_timeouts are u64 counters incremented in
  rtl8169_ptp_tx_work(), and ptp->tx_skipped is incremented in…
- [Low] rtl8169_ptp_trx_ts_cmd() does a read-modify-write of
  PTP_TRX_TS_STA through rtl8169_ptp_modify().
- [Low] This patch makes rtl8169_ptp_hwtstamp_enable(true) reachable for
  the first time.

--

r8169: add hardware timestamping support for RTL8127a

This commit adds hardware timestamping of PTPv2 event messages on
RTL8127a. RX timestamps are read from per-message-type PHY latches in the
NAPI path. TX timestamps are handled one packet at a time and polled from
a work item with a 15 second timeout. The commit also wires up
ndo_hwtstamp_get/set, get_ts_info and get_ts_stats.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index fc3cfa09a57e7..2ffb877ac2b49 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -5146,6 +5146,10 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
>  
>  	skb_tx_timestamp(skb);
>  
> +	if (tp->ptp_support &&
> +	    unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> +		rtl8169_ptp_tx_tstamp(&tp->ptp, skb);

[Severity: Low]
Should SKBTX_IN_PROGRESS be set before skb_tx_timestamp() runs here?

rtl8169_ptp_tx_tstamp() sets SKBTX_IN_PROGRESS only after
skb_tx_timestamp() has already produced the software timestamp. Because of
that, this check in __skb_tstamp_tx() can never suppress it:

    if (!hwtstamps && !(tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW) &&
        skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
        return;

A socket that asks for both software and hardware TX timestamps without
SOF_TIMESTAMPING_OPT_TX_SWHW gets an extra software timestamp.
Documentation/networking/timestamping.rst says skb_tx_timestamp() gives a
software timestamp only when SKBTX_IN_PROGRESS is not set.

> +
>  	/* Force memory writes to complete before releasing descriptor */
>  	dma_wmb();

[ ... ]

> diff --git a/drivers/net/ethernet/realtek/r8169_ptp.c b/drivers/net/ethernet/realtek/r8169_ptp.c
> index 6201170e0c83e..d318025837612 100644
> --- a/drivers/net/ethernet/realtek/r8169_ptp.c
> +++ b/drivers/net/ethernet/realtek/r8169_ptp.c

[ ... ]

> @@ -136,6 +155,48 @@ static void rtl8169_ptp_write_time(struct rtl8169_ptp *ptp,
>  	rtl8169_ptp_write(ptp, PTP_CFG_TIME + PTP_TIME_S_HI, sec >> 32);
>  }
>  
> +/* Latch the RX or TX timestamp of the given message type into the
> + * PTP_TRX_TS_TIME registers.
> + */
> +static void rtl8169_ptp_trx_ts_cmd(struct rtl8169_ptp *ptp, u16 cmd)
> +{
> +	rtl8169_ptp_modify(ptp, PTP_TRX_TS_STA,
> +			   PTP_TRX_TS_SEL_RX | PTP_TRX_TS_MSGTYPE,
> +			   PTP_TRX_TS_RD | cmd);

[Severity: Low]
This read-modify-write of PTP_TRX_TS_STA masks only SEL_RX and MSGTYPE.
The TX_RDY status bits 15:12 are written back exactly as they were read.
What is the access type of those bits?

If they are write-1-to-clear, an RX timestamp read from NAPI could clear a
pending TX ready bit. That happens when the read falls between a TX
timestamp event and rtl8169_ptp_tx_work() consuming it.
rtl8169_ptp_tx_work() would then see PTP_INSR.TX_TS with no TX_RDY bit. It
would poll for 1ms with interrupts off and fall back to the SYNC latch.

If they are sticky read/write bits, writing 1 back means they never clear.
Ready bits then pile up and rtl8169_ptp_egress_time() falls into the
multi-bit default case.

> +	rtl8169_ptp_poll(ptp, PTP_TRX_TS_STA, PTP_TRX_TS_RD, false);
> +}
> +
> +static void rtl8169_ptp_egress_time(struct rtl8169_ptp *ptp,
> +				    struct timespec64 *ts)
> +{
> +	u8 msgtype;
> +	int val;
> +
> +	val = rtl8169_ptp_poll(ptp, PTP_TRX_TS_STA, PTP_TRX_TS_TX_RDY, true);
> +	if (val < 0)
> +		val = 0;

[Severity: Medium]
Can a failed or all-ones register read be delivered as a valid timestamp
here?

If the TX_RDY poll times out, val is forced to 0. The switch then falls
through to the SYNC latch, which is read and delivered anyway.

rtl8169_ptp_read() returns u16, so an -ETIMEDOUT from
r8168_phy_ocp_read() becomes 0xff92. Bits 15:12 are set in that value, so
this poll for PTP_TRX_TS_TX_RDY succeeds at once on a failed read.

rtl8169_ptp_trx_ts_cmd() ignores the result of the poll that waits for
PTP_TRX_TS_RD to clear. If the latch command never completes,
PTP_TRX_TS_TIME still holds the old contents. Both
rtl8169_ptp_rx_tstamp() and rtl8169_ptp_tx_work() then report those as
this packet's timestamp, and tx_pkts counts it as a success.

If the device is in D3 or has been removed from the bus,
r8168_phy_ocp_read() returns 0xffff. rtl8169_ptp_read_time() then builds
nsec = 0x3fffffff, which is above NSEC_PER_SEC and never range checked,
and sec = 2^48 - 1. timespec64_to_ktime() turns that into KTIME_MAX.

Should these error paths drop the timestamp and count the request as lost
or err instead?

> +
> +	switch (val & PTP_TRX_TS_TX_RDY) {

[ ... ]

> @@ -243,6 +304,201 @@ static const struct ptp_clock_info rtl8169_ptp_info = {

[ ... ]

> +static void rtl8169_ptp_tx_work(struct work_struct *work)
> +{

[ ... ]

> +	spin_lock_irqsave(&ptp->lock, flags);
> +	ready = rtl8169_ptp_read(ptp, PTP_INSR) & PTP_INT_TX_TS;
> +	if (ready) {
> +		rtl8169_ptp_write(ptp, PTP_INSR, PTP_INT_TX_TS);
> +		rtl8169_ptp_egress_time(ptp, &ts);
> +	}
> +	spin_unlock_irqrestore(&ptp->lock, flags);
> +
> +	if (!ready) {
> +		schedule_work(&ptp->tx_work);
> +		return;
> +	}

[Severity: Medium]
When the timestamp is not ready, this requeues the work at once with no
delay, until tx_start + 15 * HZ. Doesn't that turn every unanswered
request into a 15 second busy loop?

Each pass takes ptp->lock with spin_lock_irqsave() and does a GPHY_OCP
transaction, which can busy-wait up to 250us in rtl_phy_ocp_wait(). Some
requests never get an answer from the hardware, for example a non-PTP skb
or a frame dropped on link down. For those, a kworker keeps spinning for
the whole timeout and floods the OCP mailbox it shares with phylib.

Would a delayed_work, or ptp_schedule_worker() with do_aux_work and a poll
interval, bound this? The commit message only says the timestamp "is
polled from a work item".

The RX side has a similar cost. rtl8169_ptp_rx_tstamp() runs from rtl_rx()
for every frame that ptp_classify_raw() accepts with msgtype <= 3. Under
spin_lock_irqsave() it does:

- a read-modify-write of PTP_TRX_TS_STA
- a read_poll_timeout_atomic() of up to 1000us
- five time register reads

A remote sender decides whether a frame takes this path (L2 0x88F7, or UDP
319/320 with the right messageType). A low rate flood of minimal PTP event
frames can therefore burn a lot of interrupts-off time in softirq context.

[ ... ]

> +void rtl8169_ptp_tx_tstamp(struct rtl8169_ptp *ptp, struct sk_buff *skb)
> +{
> +	if (ptp->config.tx_type != HWTSTAMP_TX_ON ||
> +	    test_and_set_bit_lock(RTL8169_PTP_TX_IN_PROGRESS, &ptp->state)) {
> +		ptp->tx_skipped++;
> +		return;
> +	}

[Severity: Low]
ptp->config.tx_type is read here with a plain load from ndo_start_xmit,
which holds only the TX queue lock. The writers run under RTNL and use
plain stores: rtl8169_ptp_hwtstamp_set() through
hwtstamp_config_from_kernel(), rtl8169_ptp_init() and rtl8169_ptp_stop().

rx_enabled uses READ_ONCE()/WRITE_ONCE() in the same situation. Should
tx_type be annotated the same way, so KCSAN does not report it?

> +
> +	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	ptp->tx_skb = skb_get(skb);
> +	ptp->tx_start = jiffies;
> +	schedule_work(&ptp->tx_work);
> +}

[Severity: High]
Is the TX hardware timestamp actually tied to the skb that asked for it?

This function takes the single slot for any skb with SKBTX_HW_TSTAMP set.
It never calls ptp_classify_raw()/ptp_parse_header() and never records the
message type.

The hardware only has TX latches and ready bits for the four PTP event
types (PTP_TRX_TS_TX_{SYNC,DLYREQ,PDLYREQ,PDLYRSP}_RDY). An skb that is not
a PTP event never gets a timestamp, but it holds the slot for up to 15
seconds. Examples are NTP or any UDP sent with
SOF_TIMESTAMPING_TX_HARDWARE, PTP general messages, and PTPv1.

rtl8169_ptp_tx_work() treats any PTP_INSR.TX_TS as completion of the
tracked skb. So the next PTP event frame that goes out completes the wrong
request. That could be ptp4l's Sync, which was itself counted as
tx_skipped because the slot was busy. Its timestamp goes to the other
socket, and ptp4l never gets one.

rtl8169_ptp_egress_time() also picks the latch from whichever TX_RDY bits
are set, not from the requesting skb. Any value other than exactly one bit
falls into the default case and reads the SYNC latch. TX_RDY is never
cleared before a new request.

On a config change, rtl8169_ptp_hwtstamp_set() ->
rtl8169_ptp_hwtstamp_enable(true) writes 0xffff to PTP_INSR. That can wipe
the status of an in-flight request.

SO_TIMESTAMPING with SOF_TIMESTAMPING_TX_HARDWARE needs no privilege. Once
TX_ON is enabled, any local user can keep the slot busy, so ptp4l times
out waiting for TX timestamps and faults.

Should the request be classified, and matched against the message type the
hardware reports?

> +
> +void rtl8169_ptp_rx_tstamp(struct rtl8169_ptp *ptp, struct sk_buff *skb)
> +{

[ ... ]

> +	/* Only event messages are timestamped by the hardware */
> +	msgtype = ptp_get_msgtype(hdr, ptp_class);
> +	if (msgtype > PTP_MSGTYPE_PDELAY_RESP)
> +		return;

[Severity: Low]
Should this also check ptp_class & PTP_CLASS_V2?

For V1 classes, ptp_get_msgtype() returns hdr->control. V1 Follow_Up
(control = 2) and Delay_Resp (control = 3) are general messages. They
still pass this check and are fed to PTP_TRX_TS_MSGTYPE as PDELAY_REQ and
PDELAY_RESP.

They then get the PDELAY latch of some unrelated v2 frame. That
contradicts both the reported PTP_V2_EVENT filter and the comment above.

> +
> +	spin_lock_irqsave(&ptp->lock, flags);
> +	rtl8169_ptp_trx_ts_cmd(ptp, PTP_TRX_TS_SEL_RX |
> +				    FIELD_PREP(PTP_TRX_TS_MSGTYPE, msgtype) |
> +				    PTP_TRX_TS_EXEC);
> +	rtl8169_ptp_read_time(ptp, PTP_TRX_TS_TIME, &ts);
> +	spin_unlock_irqrestore(&ptp->lock, flags);
> +
> +	skb_hwtstamps(skb)->hwtstamp = timespec64_to_ktime(ts);
> +}

[Severity: Medium]
Can this attach a timestamp that belongs to a different frame?

The latch holds the last received event message of each type. It is read
when NAPI processes the skb, which can be well after the frame arrived.
Nothing checks or acknowledges PTP_INSR.RX_TS. Nothing matches the latch
to this packet either, for example by sequenceId or sourcePortIdentity.

If two event messages of the same type arrive before NAPI handles the
first, the earlier skb gets the later frame's ingress time. That is normal
on a master or boundary clock receiving Delay_Req from several slaves, and
with P2P and several peers.

Frames the classifier accepts but the hardware did not stamp get whatever
stale value is in the latch, or zero right after enable. A network peer
can also send a same-type event message right after a legitimate one and
shift the timestamp attached to the legitimate frame.

The commit message says the PHY "latches the timestamp of the last
received event message per message type", but does not mention this
misattribution.

[ ... ]

> +int rtl8169_ptp_hwtstamp_set(struct rtl8169_ptp *ptp,
> +			     struct kernel_hwtstamp_config *config,
> +			     struct netlink_ext_ack *extack)
> +{
> +	bool enable = false;
> +	bool rx_enabled;
> +
> +	if (!ptp->clock)
> +		return -ENETDOWN;

[Severity: Low]
The PHC only exists between ndo_open and ndo_stop. It is also missing if
ptp_clock_register() failed in rtl8169_ptp_init(). In those cases this
returns -ENETDOWN.

Meanwhile rtl8169_get_ts_info() picks the hardware path based only on
tp->ptp_support. rtl8169_ptp_get_ts_info() then still advertises
SOF_TIMESTAMPING_{TX,RX,RAW}_HARDWARE, tx_types and rx_filters.

If registration failed while the interface is up, would -EOPNOTSUPP be the
better errno here?

rtl8169_ptp_stop() also resets ptp->config to HWTSTAMP_TX_OFF and
HWTSTAMP_FILTER_NONE on every close. That silently drops the timestamping
config across a down/up cycle. Is that intended? The commit message does
not mention it.

[ ... ]

> +	if (ptp->config.tx_type != config->tx_type ||
> +	    ptp->config.rx_filter != config->rx_filter) {
> +		hwtstamp_config_from_kernel(&ptp->config, config);
> +		rtl8169_ptp_hwtstamp_enable(ptp, enable);
> +	}

[Severity: Low]
This makes rtl8169_ptp_hwtstamp_enable(ptp, true) reachable for the first
time. In the parent commit, which added PHC support, it was only ever
called with false.

The enable branch sets PTP_INT_RX_TS and PTP_INT_TX_TS in PTP_INER and
sets PTP_GEN_CFG_INT_CLR_MODE, but nothing services those interrupts. The
MAC interrupt handlers only handle LinkChg -> phy_mac_interrupt().
PTP_INSR.RX_TS is never acknowledged; only TX_TS is ever written back.

If the internal PHY routes PTP_INSR into its interrupt output toward the
MAC, could an RX_TS that is never cleared cause spurious link change
interrupts and phylib state machine runs, or keep the line asserted? Since
the driver only polls, are these interrupt sources needed at all?

> +	WRITE_ONCE(ptp->rx_enabled, rx_enabled);
> +
> +	return 0;
> +}
> +
> +int rtl8169_ptp_get_ts_info(struct rtl8169_ptp *ptp,
> +			    struct kernel_ethtool_ts_info *info)
> +{
> +	info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> +				SOF_TIMESTAMPING_TX_HARDWARE |
> +				SOF_TIMESTAMPING_RX_HARDWARE |
> +				SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +	if (ptp->clock)
> +		info->phc_index = ptp_clock_index(ptp->clock);

[Severity: Medium]
Can this race with rtl8169_ptp_stop() and read a freed ptp_clock?

get_ts_info can be reached without RTNL:

setsockopt(SO_TIMESTAMPING with SOF_TIMESTAMPING_BIND_PHC)
  sock_timestamping_bind_phc()
    ethtool_get_phc_vclocks()
      __ethtool_get_ts_info()
        rtl8169_get_ts_info()
          rtl8169_ptp_get_ts_info()

That path holds only rcu_read_lock() and a device reference. At the same
time, rtl8169_close() under RTNL can run rtl8169_ptp_stop():

	ptp_clock_unregister(ptp->clock);
	ptp->clock = NULL;

ptp_clock_unregister() can drop the last reference and free the ptp_clock
before the pointer is cleared with a plain store. A reader that has
already seen the non-NULL pointer would then read freed memory in
ptp_clock_index().

Before this patch the driver used ethtool_op_get_ts_info(), which never
touched ptp->clock.

> +
> +	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
> +	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 in this mask?

rtl8169_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
upgrades them to PTP_V2_EVENT. None of the three are advertised here, but
PTP_V2_EVENT, which covers both L2 and L4, is.

The RX path does stamp L2 PTP frames, since ptp_parse_header() handles
PTP_CLASS_L2. If PTP_CTL_TYPE_3 is in fact L4 only, should neither the L2
filters nor PTP_V2_EVENT be accepted or advertised?

> +
> +	return 0;
> +}
> +
> +void rtl8169_ptp_get_ts_stats(struct rtl8169_ptp *ptp,
> +			      struct ethtool_ts_stats *stats)
> +{
> +	stats->pkts = ptp->tx_pkts;
> +	stats->lost = ptp->tx_timeouts;
> +	stats->err = ptp->tx_skipped;
> +}

[Severity: Low]
Do these mappings match the ethtool_ts_stats kernel-doc?

@err is documented as timestamp generation error events that the hardware
encountered. But rtl8169_ptp_tx_tstamp() also increments tx_skipped when
ptp->config.tx_type != HWTSTAMP_TX_ON. That is a socket asking for a
hardware timestamp while TX timestamping is off, not a hardware error.

rtl8169_ptp_stop() can discard a pending request through
rtl8169_ptp_tx_complete(ptp, NULL). That fits the @lost definition
(timestamping information from the hardware never arrived), but
tx_timeouts is not incremented.

[Severity: Low]
tx_pkts and tx_timeouts are u64 counters incremented in
rtl8169_ptp_tx_work(). tx_skipped is incremented in
rtl8169_ptp_tx_tstamp() from ndo_start_xmit. All three are read here with
plain loads, without u64_stats_sync or a lock shared with the writers. Can
a reader see torn values on 32-bit architectures?

[ ... ]

> @@ -275,7 +533,13 @@ void rtl8169_ptp_stop(struct rtl8169_ptp *ptp)
>  	if (!ptp->clock)
>  		return;
>  
> +	ptp->config.tx_type = HWTSTAMP_TX_OFF;
> +	ptp->config.rx_filter = HWTSTAMP_FILTER_NONE;
> +	WRITE_ONCE(ptp->rx_enabled, false);
>  	rtl8169_ptp_hwtstamp_enable(ptp, false);
> +	cancel_work_sync(&ptp->tx_work);
> +	if (ptp->tx_skb)
> +		rtl8169_ptp_tx_complete(ptp, NULL);
>  
>  	ptp_clock_unregister(ptp->clock);
>  	ptp->clock = NULL;

[Severity: High]
This is the only place where tx_work is cancelled and a pending tx_skb is
released, and it is only reached from ndo_stop. What happens across
suspend, runtime suspend, resume and shutdown?

rtl8169_suspend(), rtl8169_runtime_suspend() and rtl_shutdown() all go
through rtl8169_net_suspend() -> rtl8169_down(), which does:

	disable_work_sync(&tp->wk.work);
	...
	pci_clear_master(tp->pci_dev);
	...
	rtl_prepare_power_down(tp);

That path never cancels ptp->tx_work and never frees ptp->tx_skb. tx_work
requeues itself on system_percpu_wq for up to 15 seconds, so it keeps
doing GPHY_OCP accesses after the device has been powered down.

With the interface up, carrier loss calls pm_runtime_idle(). Then
rtl8169_runtime_idle() schedules suspend 10 seconds later, which is inside
RTL8169_PTP_TX_TIMEOUT:

	if (!netif_running(tp->dev) || !netif_carrier_ok(tp->dev))
		pm_schedule_suspend(device, 10000);

A request still pending at that point leaves tx_work polling a device in
D3hot. The reads return all-ones, so this sequence follows:

- PTP_INSR & PTP_INT_TX_TS looks ready
- the TX_RDY poll passes
- rtl8169_ptp_read_time() builds a KTIME_MAX timestamp
- skb_tstamp_tx() hands it to user space and tx_pkts counts it

On resume, rtl8169_up() goes through D3 -> D0, rtl8169_init_phy() (which
includes a PHY soft reset) and rtl8169_ptp_set_local_time(). It never
replays rtl8169_ptp_hwtstamp_enable() from ptp->config. If PTP_CTL and
PTP_INER do not survive that, ptp->config and rx_enabled still say
timestamping is on while the hardware has it off.

rtl8169_ptp_hwtstamp_set() only reprograms the hardware when the config
changes:

	if (ptp->config.tx_type != config->tx_type ||
	    ptp->config.rx_filter != config->rx_filter) {

So re-sending the same SIOCSHWTSTAMP config, as ptp4l does during fault
recovery, does nothing. Doesn't timestamping then stay broken until it is
switched off and back on, or the interface is cycled?

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

      reply	other threads:[~2026-11-12  5:33 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-09  2:30 [PATCH net-next v1 0/3] r8169: add PTP " javen
2026-11-09  2:30 ` [PATCH net-next v1 1/3] r8169: make PHY OCP access safe in atomic context javen
2026-11-09 20:47   ` Andrew Lunn
2026-11-10  9:58     ` Javen
2026-11-12  5:33   ` netdev-bot+sashiko
2026-11-09  2:30 ` [PATCH net-next v1 2/3] r8169: add PTP hardware clock support for RTL8127a javen
2026-11-09 20:51   ` Andrew Lunn
2026-11-12  5:33   ` netdev-bot+sashiko
2026-11-09  2:30 ` [PATCH net-next v1 3/3] r8169: add hardware timestamping " javen
2026-11-12  5:33   ` netdev-bot+sashiko [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=179446160712.2199510.8409224902993653061@kernel.org \
    --to=netdev-bot+sashiko@kernel.org \
    --cc=andrew+netdev@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=hkallweit1@gmail.com \
    --cc=horms@kernel.org \
    --cc=javen_xu@realsil.com.cn \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nic_swsd@realtek.com \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®