| Message ID | 20250219054247.733243-2-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: enetc: fix some known issues | expand |
> -----Original Message----- > From: Wei Fang <wei.fang@nxp.com> > Sent: Wednesday, February 19, 2025 7:43 AM [...] > Subject: [PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in > enetc_map_tx_buffs() > > 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> Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>
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 :)
> > 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? > 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 :)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 6a6fc819dfde..01c09fd26f9f 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -372,13 +372,13 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) dma_err: dev_err(tx_ring->dev, "DMA map error"); - do { + while (count--) { tx_swbd = &tx_ring->tx_swbd[i]; enetc_free_tx_frame(tx_ring, tx_swbd); if (i == 0) i = tx_ring->bd_count; i--; - } while (count--); + } return 0; }
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> --- drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)