Re: [PATCH net v3] ice: fix Tx hang dump using stale tx_buf[0].skb

["Choinière, Vincent" <choiniere.vincent@hydroquebec.com>]

Hi,

I posted v4 here:

https://lore.kernel.org/all/20261112-ice-tx-hang-dump-v4-1-9125f687669b@hydroquebec.com/

> Is pointer equality enough to show that this is still the hung ring?

Pointer equality alone is not sufficient. In v4, the saved ring is used
only if the timeout was on the PF VSI, that VSI is up, and
vsi->tx_rings[queue] is the saved ring, with the queue recorded at timeout
time. PF VSI rings have q_index == i, so a freed ring reused for another
queue after ethtool -L is rejected and the dump falls back to the
timeout-time values.

A reset of the same queue in place is still not detected: ethtool -G
replaces the ring contents, and ice_down() on an MTU change clears the
descriptors and indices without changing desc. Comparing desc would
miss the second case and is subject to address reuse. As before this
patch, a successful snapshot shows the queue's ring when the report is
generated. The v4 commit message and kernel-doc say so and no longer
imply that ring replacement is detected.

> Could the success path of ice_tx_hang_snapshot() at least set
> ev->queue = tx_ring->q_index, the same way it updates next_to_clean
> and next_to_use?

With the check above, a successful snapshot comes from
vsi->tx_rings[ev->queue], so there is no need to update ev->queue on the
success path.

In v4, I also read the saved ring pointer before taking RTNL,
alongside queue, head and intr. This avoids reading the ring pointer
after waiting for RTNL while using event values read before the wait.
It does not make the event snapshot atomic; the handoff remains
unsynchronized.

> Could the commit message say whether removing the unused export is
> planned as a net-next follow-up?

Yes. The v4 commit message says so. I plan to send the removal of
devlink_fmsg_dump_skb() to net-next once this fix is applied.

> [High] This problem wasn't introduced by this patch, but the service
> task can still run ice_report_tx_hang() during driver removal

> [High] This problem wasn't introduced by this patch, but devlink reload
> with DRIVER_REINIT can deadlock

Both issues predate this patch and are documented under Limitations
in the v4 notes. 
I suggest addressing them in separate fixes to keep this patch focused. If 
someone more familiar with these lifecycle and locking paths would like 
to take them on, that would be welcome.

Thanks,
Vincent

pw-bot: cr