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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
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?

[ ... ]

> 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;

[ ... ]