Re: [PATCH net-next v8 05/11] net: ethernet: oa_tc6: Support for hardware timestamp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] net: oa_tc6: permanent PTP failure and memory leak on dropped TX packets
- [High] net: oa_tc6: out-of-bounds shift and unbounded SKB leak for forwarded packets
- [Medium] net: oa_tc6: disabling HW timestamps causes SKB and timestamp slot leak
- [Medium] net: oa_tc6: 64-bit timestamp statistics are susceptible to torn reads
- [Medium] net: oa_tc6: oa_tc6_get_ts_info drops software timestamping capability flags
- [Medium] net: oa_tc6: unnecessary bulk SPI read on extended status events
--

commit daeff7785bfd3c494b303f6929065e7237a23200
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Support for hardware timestamp

This commit implements PTP hardware timestamping for the OA TC6 framework.
It introduces functions to handle TX and RX timestamps, registers the PTP
clock, and provides ethtool callbacks for timestamp configurations.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index f8e9309a72f11..5cbe049f76efd 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
[ ... ]
> @@ -50,6 +61,195 @@ static int oa_tc6_get_parity(u32 p)
>  	return !((p >> 28) & 1);
>  }
[ ... ]
> +static void oa_tc6_events_handle(struct oa_tc6 *tc6, u32 val, const u32 *ts)
> +{
> +	struct skb_shared_hwtstamps tstamp;
> +	struct oa_tc6_ts_info_tx *ski;
> +	struct sk_buff *skb, *tmp;
> +	LIST_HEAD(deliver);
> +	LIST_HEAD(drop);
> +	u8 tsc, bit_num;
> +	u32 handled = 0;
> +	u32 event;
> +
> +	spin_lock_bh(&tc6->tx_skb_lock);
> +	list_for_each_entry_safe(skb, tmp, &tc6->tx_ts_skb_q, list) {
> +		ski = oa_tc6_tsinfo_tx(skb);
> +		tsc = ski->tsc;
> +		bit_num = tsc - OA_TC6_TTSCA_REG_ID;
> +		event = OA_TC6_STATUS0_TTSCAA << bit_num;

[Severity: High]
If an SKB is forwarded or tc-mirrored to this interface with the
SKBTX_IN_PROGRESS flag already set, its tsc will be 0. When tsc is 0,
bit_num becomes 255. Does this out-of-bounds shift result in undefined
behavior where event evaluates to 0, causing the SKB to evade the 3-slot
cap and leak indefinitely?

> +		if (!(val & event))
> +			continue;

[Severity: High]
Does this logic only handle successful events and lack a timeout mechanism?
If the MAC-PHY drops an outgoing packet or fails to transmit, it will not
generate a timestamp capture event. Since there are only 3 slots available,
would dropping 3 packets permanently exhaust the slots and break PTP until
a device reset?

[ ... ]
> +	if (val & ~handled) {
> +		tc6->tx_hwtstamp_err += hweight32(val & ~handled);

[Severity: Medium]
This increment occurs outside of spin_lock_bh(&tc6->tx_skb_lock). Can this
lead to torn reads when ethtool reads the 64-bit tx_hwtstamp_err on 32-bit
systems without u64_stats_sync protection?

[ ... ]
> @@ -686,15 +900,28 @@ static void oa_tc6_disable_traffic(struct oa_tc6 *tc6)
>  
>  static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
>  {
> +	u32 regs[OA_TC6_STATUS0_TTSC_REG_COUNT];
> +	bool ts_valid = !!tc6->ptp_clock;
>  	u32 value;
>  	int ret;
>  
> -	ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, &value);
> +	/* When timestamp is disabled, there is no behavior change
> +	 * as it reads only STATUS0 register. When enabled,
> +	 * TTSCA_HIGH..TTSCC_LOW are fetched together with STATUS0
> +	 * to avoid having to make second SPI transaction. Reading few
> +	 * extra registers, even it may not be needed every time this
> +	 * function is called, it is more efficient than making second
> +	 * SPI transaction, when needed.
> +	 */
> +	ret = oa_tc6_read_registers(tc6, OA_TC6_REG_STATUS0, regs,
> +				    ts_valid ?
> +				    OA_TC6_STATUS0_TTSC_REG_COUNT : 1);

[Severity: Medium]
Since ptp_clock merely indicates support and is set during probe, does this
cause an unnecessary bulk SPI read on extended status events even when
timestamping is actively disabled? Should this check tc6->hw_tstamp_enabled
instead of !!tc6->ptp_clock?

[ ... ]
> @@ -1081,17 +1332,30 @@ 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);
> +		} else {
> +			spin_lock_bh(&tc6->tx_skb_lock);
> +			list_add_tail(&tc6->ongoing_tx_skb->list,
> +				      &tc6->tx_ts_skb_q);
> +			spin_unlock_bh(&tc6->tx_skb_lock);
> +		}

[Severity: Medium]
If ethtool is used to disable hardware timestamping while a packet with
SKBTX_IN_PROGRESS is in tc6->ongoing_tx_skb, timestamps will be disabled
and masked. Will adding this in-flight SKB to the tx_ts_skb_q queue cause
it to leak permanently since the completion event will never arrive?

[Severity: High]
Does trusting the SKBTX_IN_PROGRESS flag instead of checking if a tsc slot
was actually assigned (tsc != 0) cause forwarded packets with the flag already
set to be enqueued and leaked?

> 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 0000000000000..d83f6ad9373a7
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
[ ... ]
> +static int oa_tc6_set_hwtstamp_settings(struct oa_tc6 *tc6,
> +					const struct hwtstamp_config *ts_cfg)
> +{
[ ... ]
> +	/* The TX timestamp completion interrupt is masked now. Time
> +	 * to release the queued skbs.
> +	 */
> +	if (ts_cfg->tx_type != HWTSTAMP_TX_ON)
> +		oa_tc6_cleanup_tx_tstamp_skbs(tc6);

[Severity: Medium]
This flushes tx_ts_skb_q, but does it fail to handle tc6->ongoing_tx_skb
which might be currently in flight and will later be unconditionally enqueued
to tx_ts_skb_q by oa_tc6_add_tx_skb_to_spi_buf()?

> +out:
> +	return ret;
> +}
[ ... ]
> +void oa_tc6_get_ts_stats(struct oa_tc6 *tc6,
> +			 struct ethtool_ts_stats *stats)
> +{
> +	stats->pkts = tc6->tx_hwtstamp_pkts;
> +	stats->err = tc6->tx_hwtstamp_err;
> +	stats->lost = tc6->tx_hwtstamp_lost;
> +}

[Severity: Medium]
These 64-bit values are updated by the SPI kernel thread and read here
concurrently from process context. Should these be protected with
u64_stats_sync to prevent torn reads on 32-bit architectures?

[ ... ]
> +int oa_tc6_get_ts_info(struct oa_tc6 *tc6,
> +		       struct kernel_ethtool_ts_info *info)
> +{
> +	if (!tc6->ptp_clock)
> +		return ethtool_op_get_ts_info(tc6->netdev, info);
> +
> +	info->so_timestamping = SOF_TIMESTAMPING_RAW_HARDWARE |
> +				SOF_TIMESTAMPING_TX_HARDWARE |
> +				SOF_TIMESTAMPING_RX_HARDWARE;

[Severity: Medium]
By overwriting info->so_timestamping, are the software timestamping flags
(SOF_TIMESTAMPING_SOFTWARE, SOF_TIMESTAMPING_TX_SOFTWARE,
SOF_TIMESTAMPING_RX_SOFTWARE) incorrectly omitted, falsely advertising that
software timestamps are not supported?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-s2500-mac-phy-support-v8-0-7e011aacc309@onsemi.com?part=5