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

[Sukhdeep Soni [C]]

On 4 Jun 2026, Jakub Kicinski wrote:

> net: atlantic: add AQC113 TX timestamp polling and PTP TX classification
>
> This patch wires PTP TX timestamp polling into aq_ring_tx_clean(), adds
> IPv6 PTP packet classification in aq_ndev_start_xmit(), and moves the
> RX-flow and VLAN filter reapply calls from aq_ndev_open() into
> aq_nic_start().
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> index 4ef4fe64b8ac..1da14786fe5c 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;
> -
>  	err = aq_nic_start(aq_nic);
>
> [Low]
> The commit message bullet says:
>
>    Remove stale aq_ndev_open() PTP call that was moved to aq_nic_start().
>
> The lines actually removed from aq_ndev_open() are
> aq_reapply_rxnfc_all_rules() and aq_filters_vlans_update(), which are
> RX-flow-classifier and VLAN-filter helpers rather than PTP calls.  Should
> this bullet be reworded to describe the filter helpers that were actually
> moved?
>
> Also, aq_nic_start() is invoked from atl_resume_common() in aq_pci_func.c
> in addition to aq_ndev_open().  Moving these helpers into aq_nic_start()
> means RX-flow and VLAN filters will now be reapplied on every PM resume,
> which is a behavioral change beyond plain deduplication.  Should the
> changelog call this resume-path side effect out explicitly?
>
> [ ... ]

Both points are already addressed in the commit message:

 - The bullet correctly names aq_reapply_rxnfc_all_rules()
   and aq_filters_vlans_update() as the calls removed from
   aq_ndev_open() â?? no mention of "PTP call".
 - The PM resume side effect is explicitly documented:
   "which also ensures filters are restored correctly after PM resume".

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e270327e47fd..dd1af3deb2d1 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -311,6 +311,27 @@ 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)) {
> +					/* Timeout: free skb, unblock ring */
> +					buff->request_ts = 0;
> +					dev_kfree_skb_any(buff->skb);
> +					buff->skb = NULL;
> +					goto out;
>
> [Medium]
> Can the timeout branch desynchronize aq_ptp->skb_ring from buff_ring?
>
> aq_ptp_xmit() pushes every PTP TX skb into aq_ptp->skb_ring via
> aq_ptp_skb_put() (which holds its own reference via skb_get()), and
> aq_ptp_tx_hwtstamp() consumes them in strict FIFO order:
>
> aq_ptp.c:aq_ptp_tx_hwtstamp() {
>    struct sk_buff *skb = aq_ptp_skb_get(&aq_ptp->skb_ring);
>    ...
>    skb_tstamp_tx(skb, &hwtstamp);
>    dev_kfree_skb_any(skb);
> }
>
> The new timeout branch frees buff->skb and advances sw_head, but does not
> dequeue the matching entry from aq_ptp->skb_ring.  After a timeout, the
> stale skb pointer remains at the head of skb_ring while the buff_ring
> moves on.
>
> When the next PTP TX timestamp arrives, would aq_ptp_tx_hwtstamp() pop
> that stale skb and call skb_tstamp_tx() with the timestamp belonging to
> a later, unrelated packet?  Every subsequent PTP packet would then have
> its TX timestamp misattributed by one slot until aq_ptp_tx_timeout_check()
> calls aq_ptp_skb_ring_clean() to fully drain skb_ring (~10s later).
>
> Should the timeout branch also dequeue and drop the head of skb_ring,
> e.g. via aq_ptp_skb_get() followed by dev_kfree_skb_any() of the returned
> skb, to keep skb_ring and buff_ring in lockstep?
>
> +				} else {
> +					buff->is_mapped = 0;
> +					buff->pa = 0U;
> +					break;
> +				}
> +			}
> +
> +			aq_ptp_tx_hwtstamp(self->aq_nic, ts);
> +		}
>  		if (buff->skb) {
>  			u64_stats_update_begin(&self->stats.tx.syncp);
>  			++self->stats.tx.packets;
>
> [ ... ]

handled - aq_ptp_tx_skb_drop_head() is introduced in this patch precisely
to address this desynchronization. The timeout branch calls it before advancing
sw_head, which calls aq_ptp_skb_get(&aq_ptp->skb_ring) followed by
dev_kfree_skb_any() on the returned skb, keeping skb_ring and buff_ring in
lockstep.