RE: [EXTERNAL] Re: [PATCH net-next v3 11/12] net: atlantic: add AQC113 TX timestamp polling and PTP TX classification

[Sukhdeep Soni [C]]

On 22nd May 2026, Jakub Kicinski wrote:

> net: atlantic: add AQC113 TX timestamp polling and PTP TX classification
>
> This commit adds PTP TX classification for IPv4 and IPv6 packets. It also
> introduces a polling mechanism for hardware TX timestamps in the driver's
> ring cleanup path.
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> index 4ef4fe64b8ac1..1da14786fe5cc 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
[ ... ]
> @@ -68,14 +70,6 @@ int aq_ndev_open(struct net_device *ndev)
>  	if (err < 0)
>  		goto err_exit;
>  
> -	err = aq_reapply_rxnfc_all_rules(aq_nic);
> -	if (err < 0)
> -		goto err_exit;
> -
> -	err = aq_filters_vlans_update(aq_nic);
> -	if (err < 0)
> -		goto err_exit;
>
> [Severity: High]
> The commit message mentions removing a stale PTP call, but doesn't this
> remove the restoration of RX flow rules and VLAN filters when the device is
> brought up? It doesn't look like these were moved to aq_nic_start().

Both aq_reapply_rxnfc_all_rules() and aq_filters_vlans_update() will be
moved to aq_nic_start() (called by aq_ndev_open()), where they already
appear. The removal from aq_ndev_open() eliminates the duplicate
calls, not the calls themselves.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e270327e47fd8..87d680590ef2f 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> [ ... ]
> @@ -311,6 +311,26 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
>  		if (likely(!buff->is_eop))
>  			goto out;
>  
> +		if (unlikely(buff->request_ts) &&
> +		    self->aq_nic->aq_hw_ops->hw_ring_tx_ptp_get_ts) {
> +			u64 ts = self->aq_nic->aq_hw_ops->hw_ring_tx_ptp_get_ts(self);
> +
> +			if (!ts) {
> +				if (time_after(jiffies,
> +					       self->ptp_ts_deadline)) {
>
> [Severity: High]
> Where is self->ptp_ts_deadline initialized?
>
> If it is never initialized and defaults to 0, will this condition always
> evaluate to true on the very first poll since jiffies is typically greater
> than 0, causing all PTP packets to immediately time out and be dropped?

ptp_ts_deadline is declared in patch 11 and initialized to jiffies + HZ in
patch 12, in the same hunk that sets request_ts = 1U. Across the applied
series there is no window where request_ts=1 with ptp_ts_deadline uninitialized.
The field's zero-initialized default is never observed since ptp_ts_deadline is
only read when request_ts is true.

> +					/* Timeout: free skb, unblock ring */
> +					buff->request_ts = 0;
> +					dev_kfree_skb_any(buff->skb);
> +					buff->skb = NULL;
>
> [Severity: High]
> If a timeout occurs, buff->skb is freed and set to NULL, but there is no
> break or continue here. Doesn't execution then fall through to calling
> aq_ptp_tx_hwtstamp() while the SKB is already freed, leading to a
> use-after-free when the PTP logic attempts to access the SKB to report
> the timeout?
>
> Additionally, since buff->skb is now NULL, won't this skip the subsequent
> transmission statistics update block?

Both concerns are valid and will be addressed in the code. A goto out is added immediately
after buff->skb = NULL on the timeout path, so execution never falls through to aq_ptp_tx_hwtstamp().
The stats update block below is also protected by if (buff->skb), so a NULL skb there is safe regardless.
The timeout path now correctly skips both the timestamp reporting and the stats update.

> +				} else {
> +					buff->is_mapped = 0;
> +					buff->pa = 0U;
> +					break;
> +				}
> +			}
> +
> +			aq_ptp_tx_hwtstamp(self->aq_nic, ts);
> +		}