Re: [PATCH net-next v7 08/15] net: ethernet: oa_tc6: Support for hardware timestamp

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: ethernet: oa_tc6: Support for hardware timestamp

This adds hardware timestamping support to the OA TC6 MAC-PHY framework,
splitting PTP registration and hwtstamp helpers into new oa_tc6_ptp.c and
oa_tc6_tstamp.c files, and wiring TSC handling into the TX and RX chunk
paths.

Thanks for the patch; a few questions inline.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index bf96e8d1ccb9..939935cb170d 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c

[ ... ]

> +static void oa_tc6_defer_for_hwtstamp(struct oa_tc6 *tc6,
> +				      struct sk_buff *skb)
> +{
> +	if (!tc6->hw_tstamp_enabled)
> +		return;
> +	if (!skb || (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) == 0)
> +		return;
> +	if (tc6->ts_config.tx_type != HWTSTAMP_TX_ON) {
> +		tc6->tx_hwtstamp_lost++;
> +		return;
> +	}
> +
> +	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	u8 ret = tc6->tx_ts_idx++;
> +
> +	if (ret == OA_TC6_TTSCC_REG_ID)
> +		tc6->tx_ts_idx = OA_TC6_TTSCA_REG_ID;
> +	oa_tc6_tsinfo_tx(skb)->tsc = ret;
> +
> +	list_add_tail(&skb->list, &tc6->tx_ts_skb_q);
> +}

[High]
The hardware exposes three timestamp capture slots (TTSCA, TTSCB, TTSCC),
but tx_ts_idx cycles through them unconditionally, with no check that
the previous capture in a given slot has already been drained by the
TTSCA/B/C completion IRQ.

When tx_credits allows more than three timestamped skbs to be pipelined,
the same tsc value is assigned to two skbs.  The earlier skb's TTSCx
register is then overwritten by the later capture before
oa_tc6_process_deferred_skb() reads it, and two skbs end up on
tx_ts_skb_q with an identical tsc.

Should this scheme track per-slot in-use state and either back-pressure
or fall back to software timestamping when all three slots are pending?

[High]
tc6->hw_tstamp_enabled and tc6->ts_config are written from the ioctl
path through oa_tc6_hwtstamp_set()->oa_tc6_set_hwtstamp_settings() and
read here on the threaded IRQ path with no lock, RCU, WRITE_ONCE, or
memory barrier.  Is there some serialization not visible here?

On the disable path, oa_tc6_set_hwtstamp_settings() programs the
hardware first and only afterwards clears hw_tstamp_enabled.  Can this
happen:

  CPU0 (ioctl disable)          CPU1 (threaded IRQ)
  write CONFIG0 (clear FTSE)
  write INT_MASK0 (mask TTSCA)
                                if (!tc6->hw_tstamp_enabled) /* still true */
                                ...
                                skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
                                list_add_tail(&skb->list, &tc6->tx_ts_skb_q);
  tc6->hw_tstamp_enabled = false;

At that point the TTSCA/B/C interrupts are already masked, so
oa_tc6_process_deferred_skb() will never run for that tsc; the skb is
stuck on tx_ts_skb_q, and because SKBTX_IN_PROGRESS was set the socket
TX completion is also blocked.

Would a mutex around the reconfiguration (plus draining tx_ts_skb_q)
be more appropriate than the plain flag read?

> +static int oa_tc6_process_deferred_skb(struct oa_tc6 *tc6, u8 tsc)
> +{
> +	struct skb_shared_hwtstamps tstamp;
> +	struct oa_tc6_ts_info_tx *ski;
> +	struct sk_buff *skb, *tmp;
> +	bool found = false;
> +	int ret = 0;
> +
> +	/* Size of data must match OA_TC6_TSTAMP_SZ */
> +	u32 data[2];
> +
> +	list_for_each_entry_safe(skb, tmp, &tc6->tx_ts_skb_q, list) {
> +		ski = oa_tc6_tsinfo_tx(skb);
> +		if (ski->tsc != tsc)
> +			continue;
> +		if (found) {
> +			dev_warn_ratelimited(&tc6->spi->dev,
> +					     "Multiple skbs. tsc = %d\n",
> +					     tsc);
> +			tc6->tx_hwtstamp_err++;
> +		}
> +		found = true;
> +		list_del(&skb->list);
> +
> +		/* Retrieve the timestamping info */
> +		ret = oa_tc6_read_registers(tc6,
> +					    OA_TC6_REG_TTSCA_HIGH +
> +					    2 * (tsc - 1), &data[0], 2);
> +
> +		if (!ret) {
> +			tstamp.hwtstamp = ktime_set(data[0], data[1]);
> +			skb_tstamp_tx(skb, &tstamp);
> +			tc6->tx_hwtstamp_pkts++;
> +		}
> +
> +		dev_kfree_skb(skb);
> +	}
> +	return ret;
> +}

[Medium]
When more than one skb matches the same tsc, this loop still calls
skb_tstamp_tx() with the identical hardware timestamp for every match
and increments tx_hwtstamp_pkts on each.

Should only the first match report a timestamp, with the remaining
matches freed as errors and neither reporting a bogus duplicate to
userspace nor inflating tx_hwtstamp_pkts?

> +static void oa_tc6_update_ts_in_rx_skb(struct oa_tc6 *tc6)
> +{
> +	struct sk_buff *skb = tc6->rx_skb;
> +	struct oa_tc6_ts_info_rx *ski;
> +	__be32 ts_val[2];
> +	u32 ts[2];
> +
> +	if (!tc6->hw_tstamp_enabled)
> +		return;
> +	ski = oa_tc6_tsinfo_rx(skb);
> +	if (!ski->rtsa)
> +		return;
> +
> +	memcpy(&ts_val[0], skb->data, 4);
> +	memcpy(&ts_val[1], (u32 *)skb->data + 1, 4);
> +
> +	ts[0] = be32_to_cpu(ts_val[0]);
> +	ts[1] = be32_to_cpu(ts_val[1]);
> +
> +	/* Check parity */
> +	if ((oa_tc6_get_parity(ts[0]) ^ oa_tc6_get_parity(ts[1])) ==
> +	    !ski->rtsp) {
> +		struct skb_shared_hwtstamps *hw_ts;
> +
> +		/* Report timestamp to the upper layers */
> +		hw_ts = skb_hwtstamps(skb);
> +		memset(hw_ts, 0, sizeof(*hw_ts));
> +		hw_ts->hwtstamp = ktime_set(ts[0], ts[1]);
> +	}
> +	skb_pull(skb, sizeof(ts));
> +}

[Medium]
The two 4-byte memcpys and the skb_pull() assume the linear region
holds at least 8 bytes, but skb->len is never checked.  If a short
frame arrives with RTSA_VALID set in the footer, the memcpys read past
the received payload (still inside the allocated buffer, so no OOB,
but garbage becomes a "hardware timestamp"), and skb_pull() returns
NULL without advancing skb->data.

The return value is ignored, so eth_type_trans() then parses the
would-be timestamp bytes as the Ethernet header and delivers a corrupt
frame upstream.

Should this bail out early if skb->len < OA_TC6_TSTAMP_SZ, and check
the return of skb_pull()?

[ ... ]

> @@ -887,12 +1060,18 @@ static void oa_tc6_add_tx_skb_to_spi_buf(struct oa_tc6 *tc6)
>  		tc6->tx_skb_offset = 0;
>  		tc6->netdev->stats.tx_bytes += tc6->ongoing_tx_skb->len;
>  		tc6->netdev->stats.tx_packets++;
> -		kfree_skb(tc6->ongoing_tx_skb);
> +
> +		/* Free the ones that are not saved for later processing,
> +		 * like timestamping.
> +		 */
> +		if (!(skb_shinfo(tc6->ongoing_tx_skb)->tx_flags &
> +		    SKBTX_IN_PROGRESS))
> +			kfree_skb(tc6->ongoing_tx_skb);
>  		tc6->ongoing_tx_skb = NULL;
>  	}

[High]
When SKBTX_IN_PROGRESS is set the ongoing_tx_skb reference is dropped
without a free, leaving the skb owned solely by tx_ts_skb_q.  The only
path that removes and frees these skbs is oa_tc6_process_deferred_skb()
driven by the TTSCA/B/C interrupt.

Two lifetime questions:

On module unload/unbind, oa_tc6_free_pending_skbs() only touches
ongoing_tx_skb, ongoing_rx_skb, and waiting_tx_skb.  What frees the
skbs still linked on tx_ts_skb_q?

When hwtstamp is disabled via SIOCSHWTSTAMP,
oa_tc6_set_hwtstamp_settings() masks INT_MASK0_TTSCA_MASK.  Any skb
already queued for a pending capture will not receive its completion
IRQ.  What drains tx_ts_skb_q in that case?

Because SKBTX_IN_PROGRESS is set on these skbs, their socket-level TX
completion is also blocked until skb_tstamp_tx() or free happens, so a
leak here stalls sockets in addition to leaking memory.

[ ... ]

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c b/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c
> new file mode 100644
> index 000000000000..0cde5545ee27
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c

[ ... ]

> +int oa_tc6_ptp_register(struct oa_tc6 *tc6, struct ptp_clock_info *info)
> +{
> +	/* Not supporting hardware timestamp isn't an error */
> +	if (!tc6->hw_tstamp_supported)
> +		return 0;
> +
> +	snprintf(info->name, sizeof(info->name), "%s",
> +		 "OA TC6 PTP clock");

[Low]
This unconditionally overwrites the caller-provided info->name with a
fixed string.  If a board hosts more than one OA TC6 MAC-PHY, all PTP
clocks register under the identical name "OA TC6 PTP clock", making
ptp4l/phc2sys logging and pinning ambiguous.

Could the name be derived from dev_name(&spi->dev) or the netdev name,
or left untouched so the vendor caller can set a unique value?

> +	tc6->ptp_clock = ptp_clock_register(info, &tc6->spi->dev);
> +	if (IS_ERR(tc6->ptp_clock)) {
> +		dev_err(&tc6->spi->dev, "Registration of %s failed",
> +			info->name);
> +		return -EFAULT;
> +	}

[High, Low]
Two questions about this failure path:

On failure, tc6->ptp_clock retains the ERR_PTR value from
ptp_clock_register().  oa_tc6_ptp_unregister() later guards on
`if (tc6->ptp_clock)` and oa_tc6_get_ts_info() on `if (!tc6->ptp_clock)`.
Both checks pass for an ERR_PTR, so ptp_clock_unregister() and
ptp_clock_index() end up dereferencing an ERR_PTR.  Should tc6->ptp_clock
be reset to NULL before returning here?

-EFAULT is documented for bad userspace addresses.  Would returning
PTR_ERR(tc6->ptp_clock) preserve the real errno from ptp_clock_register()
(typically -ENOMEM, -EBUSY, or -EINVAL)?

[ ... ]

> +MODULE_DESCRIPTION("OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface Lib");
> +MODULE_AUTHOR("Selva Rajagopal <selvamani.rajagopal@onsemi.com>");
> +MODULE_LICENSE("GPL");

[Low]
All three .c files (oa_tc6.c, oa_tc6_ptp.c, oa_tc6_tstamp.c) link into
the single oa_tc6_mod.ko per the Makefile change, yet each translation
unit declares its own MODULE_DESCRIPTION/MODULE_AUTHOR/MODULE_LICENSE.
This produces duplicate __UNIQUE_ID_license sections and duplicate
modinfo strings.

Would keeping the module metadata only in oa_tc6.c be cleaner?

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h b/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h

[ ... ]

> @@ -127,6 +147,17 @@ struct oa_tc6 {
>  	bool rx_buf_overflow;
>  	bool int_flag;
>  	bool disable_traffic;
> +	struct ptp_clock_info ptp_clock_info;

[Low]
This ptp_clock_info field does not appear to be assigned or read
anywhere in the series.  oa_tc6_ptp_register() takes a struct
ptp_clock_info * argument and forwards it to ptp_clock_register()
directly, and vendor drivers embed their own ptp_clock_info in their
own state.  Is this field intended for future use, or can it be
dropped?

> +	struct hwtstamp_config ts_config;
> +	struct list_head tx_ts_skb_q;
> +	struct ptp_clock *ptp_clock;
> +	bool hw_tstamp_supported;
> +	bool hw_tstamp_enabled;
> +	u32 tx_hwtstamp_pkts;
> +	u32 tx_hwtstamp_lost;
> +	u32 tx_hwtstamp_err;
> +	int vend1_mms;
> +	u8 tx_ts_idx;
>  };

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
> new file mode 100644
> index 000000000000..f80f1a4916a3
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c

[ ... ]

> +static int oa_tc6_set_hwtstamp_settings(struct oa_tc6 *tc6)
> +{
> +	u32 cfg0, irqm, status0;
> +	int ret;
> +
> +	ret = oa_tc6_read_register(tc6, OA_TC6_REG_CONFIG0, &cfg0);
> +	if (ret) {
> +		dev_err(&tc6->spi->dev, "Failed to read CFG0 register\n");
> +		goto out;
> +	}
[ ... ]
> +	ret = oa_tc6_write_register(tc6, OA_TC6_REG_CONFIG0, cfg0);

[Medium]
oa_tc6_read_register() and oa_tc6_write_register() each take
spi_ctrl_lock individually, but the read-modify-write pair on CONFIG0
(and on INT_MASK0) is not atomic across the two calls.
oa_tc6_zero_align_receive_frame_enable() also RMWs CONFIG0.  Today
that call comes only from vendor probe (single-threaded), but this
function runs at runtime from ioctl.

Would a dedicated register-state mutex covering the whole RMW sequence
protect against a future runtime user of a different CONFIG0 bit
losing an update?

[ ... ]

> +int oa_tc6_hwtstamp_set(struct oa_tc6 *tc6,
> +			struct kernel_hwtstamp_config *cfg)
> +{
[ ... ]
> +	switch (cfg->rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +	case HWTSTAMP_FILTER_ALL:
> +	case HWTSTAMP_FILTER_SOME:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
[ ... every PTP filter accepted ... ]
> +	case HWTSTAMP_FILTER_NTP_ALL:
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +	hwtstamp_config_from_kernel(&tc6->ts_config, cfg);
> +
> +	/* Supports timestamping all traffic */
> +	if (cfg->rx_filter != HWTSTAMP_FILTER_NONE)
> +		tc6->ts_config.rx_filter = HWTSTAMP_FILTER_ALL;
> +	return oa_tc6_set_hwtstamp_settings(tc6);
> +}

[Medium]
The coerced filter is written to tc6->ts_config.rx_filter but not
back into the caller-supplied *cfg.  When this is wired to
ndo_hwtstamp_set(), dev_set_hwtstamp() copies kernel_hwtstamp_config
back to userspace unchanged, so userspace sees its original request
(for example HWTSTAMP_FILTER_PTP_V2_L2_EVENT) reported as accepted
while the hardware silently applies HWTSTAMP_FILTER_ALL.

Should cfg->rx_filter be updated to HWTSTAMP_FILTER_ALL before returning,
so userspace can observe the coercion the way other drivers signal it?

[Medium]
oa_tc6_get_ts_info() advertises rx_filters = BIT(HWTSTAMP_FILTER_ALL)
alone, while this switch accepts every PTP-related filter and
HWTSTAMP_FILTER_NTP_ALL.  ethtool -T therefore describes a different
set of accepted filters than what oa_tc6_hwtstamp_set() actually takes.

Additionally, cfg->flags is not validated on this path; only the ioctl
wrapper oa_tc6_tstamp_ioctl() rejects a non-zero tscfg.flags.  Any
HWTSTAMP_FLAG_* bits arriving via ndo_hwtstamp_set() are silently
swallowed.

Should the advertised rx_filters mask reflect what is actually accepted
here, and should cfg->flags be validated in this function too?
-- 
This is an AI-generated review.