> > After running with some test data, I agree that the bug exists and that > the fix is correct. > > Reviewed-by: Vladimir Oltean <vladimir.oltean@xxxxxxx> > > 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? > Yes, I plan to use a helper function to replace the same code blocks in net-next tree. > 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 :)
