On Wed, Feb 19, 2025 at 01:42:39PM +0800, Wei Fang wrote:
When a DMA mapping error occurs while processing skb frags, it will free one more tx_swbd than expected, so fix this off-by-one issue.
Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers") Cc: stable@vger.kernel.org Signed-off-by: Wei Fang wei.fang@nxp.com
After running with some test data, I agree that the bug exists and that the fix is correct.
Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com
It's just that there's yet one more (correct) dma_err snippet in enetc_lso_hw_offload() which achieves the same thing, but expressed differently, added by you in December 2024.
For fixing a bug from 2019, I agree that you've made the right choice in not creating a dependency on that later code. But I like slightly less the fact that it leaves 2 slightly different, both non-obvious, code paths for unmapping DMA buffers. You could have at least copied the dma_err handling from enetc_lso_hw_offload(), to make it obvious that one is correct and the other is not, and not complicate things with yet a 3rd implementation.
You don't need to change this unless there's some other reason to resend the patch set, but at least, once "net" merges back into "net-next", could you please make a mental note to consolidate the 2 code snippets into a single function?
Also, the first dma_mapping_error() from enetc_map_tx_buffs() does not need to "goto dma_err". It would be dead code. Maybe you could simplify that as well. In general, the convention of naming error path labels is to name them after what they do, rather than after the function that failed when you jump to them. It's easier to manually verify correctness this way.
Also, the dev_err(tx_ring->dev, "DMA map error"); message should be rate limited, since it's called from a fast path and can kill the console if the error is persistent.
A lot of things to improve still, thanks for doing this :)