| Message ID | 20210907112940.967985-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0341d5e3d1ee2a36dd5a49b5bef2ce4ad1cfa6b4 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: renesas: sh_eth: Fix freeing wrong tx descriptor | expand |
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Tue, 7 Sep 2021 20:29:40 +0900 you wrote: > The cur_tx counter must be incremented after TACT bit of > txdesc->status was set. However, a CPU is possible to reorder > instructions and/or memory accesses between cur_tx and > txdesc->status. And then, if TX interrupt happened at such a > timing, the sh_eth_tx_free() may free the descriptor wrongly. > So, add wmb() before cur_tx++. > Otherwise NETDEV WATCHDOG timeout is possible to happen. > > [...] Here is the summary with links: - net: renesas: sh_eth: Fix freeing wrong tx descriptor https://git.kernel.org/netdev/net/c/0341d5e3d1ee You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On 9/7/21 2:29 PM, Yoshihiro Shimoda wrote: > The cur_tx counter must be incremented after TACT bit of > txdesc->status was set. However, a CPU is possible to reorder > instructions and/or memory accesses between cur_tx and > txdesc->status. And then, if TX interrupt happened at such a > timing, the sh_eth_tx_free() may free the descriptor wrongly. > So, add wmb() before cur_tx++. Not dma_wmb()? :-) > Otherwise NETDEV WATCHDOG timeout is possible to happen. > > Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet") > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
Hi Sergey, > From: Sergey Shtylyov, Sent: Wednesday, September 8, 2021 4:30 AM > > On 9/7/21 2:29 PM, Yoshihiro Shimoda wrote: > > > The cur_tx counter must be incremented after TACT bit of > > txdesc->status was set. However, a CPU is possible to reorder > > instructions and/or memory accesses between cur_tx and > > txdesc->status. And then, if TX interrupt happened at such a > > timing, the sh_eth_tx_free() may free the descriptor wrongly. > > So, add wmb() before cur_tx++. > > Not dma_wmb()? :-) On armv8, dma_wmb() is DMB OSHST, and wmb() is DSB ST. IIUC, DMB OSHST is not affected the ordering of instructions. So, we have to use wmb(). > > Otherwise NETDEV WATCHDOG timeout is possible to happen. > > > > Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet") > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> Thank you for your review! Best regards, Yoshihiro Shimoda
On 08.09.2021 8:45, Yoshihiro Shimoda wrote: >>> The cur_tx counter must be incremented after TACT bit of >>> txdesc->status was set. However, a CPU is possible to reorder >>> instructions and/or memory accesses between cur_tx and >>> txdesc->status. And then, if TX interrupt happened at such a >>> timing, the sh_eth_tx_free() may free the descriptor wrongly. >>> So, add wmb() before cur_tx++. >> >> Not dma_wmb()? :-) > > On armv8, dma_wmb() is DMB OSHST, and wmb() is DSB ST. > IIUC, DMB OSHST is not affected the ordering of instructions. > So, we have to use wmb(). I should really read up the ARM manuals on the barrier instructions... :-) >>> Otherwise NETDEV WATCHDOG timeout is possible to happen. >>> >>> Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet") >>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >> >> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > Thank you for your review! Out of curiosity: have you really experienced the bug or found it by review? > Best regards, > Yoshihiro Shimoda MBR, Sergey
Hi Sergey, > From: Sergey Shtylyov, Sent: Wednesday, September 8, 2021 6:46 PM > > On 08.09.2021 8:45, Yoshihiro Shimoda wrote: > > >>> The cur_tx counter must be incremented after TACT bit of > >>> txdesc->status was set. However, a CPU is possible to reorder > >>> instructions and/or memory accesses between cur_tx and > >>> txdesc->status. And then, if TX interrupt happened at such a > >>> timing, the sh_eth_tx_free() may free the descriptor wrongly. > >>> So, add wmb() before cur_tx++. > >> > >> Not dma_wmb()? :-) > > > > On armv8, dma_wmb() is DMB OSHST, and wmb() is DSB ST. > > IIUC, DMB OSHST is not affected the ordering of instructions. > > So, we have to use wmb(). > > I should really read up the ARM manuals on the barrier instructions... :-) :) > >>> Otherwise NETDEV WATCHDOG timeout is possible to happen. > >>> > >>> Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet") > >>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > >> > >> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > > > Thank you for your review! > > Out of curiosity: have you really experienced the bug or found it by > review? Our team has really experienced the bug when iperf3 runs on both side(server and client), and this patch could fix the issue. Best regards, Yoshihiro Shimoda > > Best regards, > > Yoshihiro Shimoda > > MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index 6c8ba916d1a6..1374faa229a2 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -2533,6 +2533,7 @@ static netdev_tx_t sh_eth_start_xmit(struct sk_buff *skb, else txdesc->status |= cpu_to_le32(TD_TACT); + wmb(); /* cur_tx must be incremented after TACT bit was set */ mdp->cur_tx++; if (!(sh_eth_read(ndev, EDTRR) & mdp->cd->edtrr_trns))
The cur_tx counter must be incremented after TACT bit of txdesc->status was set. However, a CPU is possible to reorder instructions and/or memory accesses between cur_tx and txdesc->status. And then, if TX interrupt happened at such a timing, the sh_eth_tx_free() may free the descriptor wrongly. So, add wmb() before cur_tx++. Otherwise NETDEV WATCHDOG timeout is possible to happen. Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet") Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/ethernet/renesas/sh_eth.c | 1 + 1 file changed, 1 insertion(+)