Re: [net PATCH 3/5] fbnic: Disable PTP completions during the Tx force-flush

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates what a leaked PTP completion can
  do.

--

fbnic: Disable PTP completions during the Tx force-flush

When the Tx queues fail to go idle, fbnic_tx_flush() sets only
FBNIC_TMI_DROP_CTRL_EN. That drops the packets but still forwards their
PTP completions to the host. This patch also sets
FBNIC_TMI_DROP_CTRL_CMPL_DIS during the flush, so those completions are
drained, and clears both bits in fbnic_tx_flush_off().

> The PTP completion path is governed by a separate bit in the same
> register. With only the drop enable set, a timestamped frame has its
> packet discarded while the matching completion is still forwarded to
> the host. fbnic_clean_tsq() then walks the ring with no packet left to
> match it against and reports "Tx timestamp without matching packet".

[Severity: Low]
The code change looks right. Is the impact described here complete,
though?

fbnic_clean_tsq() never checks which packet a type 1 completion belongs
to. It starts at ring->head (or *ts_head) and gives the timestamp to the
first skb that has FBNIC_XMIT_CB_TS set:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:fbnic_clean_tsq() {
    ...
	do {
		...
		if (head == ring->tail) {
			if (unlikely(net_ratelimit()))
				netdev_err(nv->napi.dev,
					   "Tx timestamp without matching packet\n");
			return;
		}
		...
	} while (!(FBNIC_XMIT_CB(skb)->flags & FBNIC_XMIT_CB_TS));
    ...
	skb_tstamp_tx(skb, &hwtstamp);
    ...
}

The head == ring->tail check is the only safeguard, and it only covers
an empty ring.

Before this patch, a completion left over from the flush could be
handled while a new timestamped skb was already pending. One possible
path:

fbnic_down()
  fbnic_wait_all_queues_idle() times out
    fbnic_tx_flush()   /* only DROP_CTRL_EN set */
  fbnic_flush()        /* Tx skbs freed */
fbnic_up()
  fbnic_enable()       /* TCQ enabled */
  fbnic_napi_enable()
  netif_tx_wake_all_queues()
    new TS skb posted
fbnic_clean_tcq()
  fbnic_clean_tsq()    /* stale completion */

In that case the new skb would get the stale hardware timestamp with no
warning, and ts_packets would still be incremented. Its real completion
would then take the empty-ring path and print the error.

Whether this happens depends on when the TMI completion FIFO releases
the stale entry, and the driver code doesn't show that.

Could the commit message also mention that SO_TIMESTAMPING users might
get a wrong Tx timestamp, not just a ratelimited error message? That
would help anyone judging this fix for stable backports.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179381496288.9644.9775319757787890210.stgit%40ahduyck-xeon-server.home.arpa