* [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor
@ 2021-09-07 11:29 Yoshihiro Shimoda
2021-09-07 13:10 ` patchwork-bot+netdevbpf
2021-09-07 19:29 ` Sergey Shtylyov
0 siblings, 2 replies; 6+ messages in thread
From: Yoshihiro Shimoda @ 2021-09-07 11:29 UTC (permalink / raw)
To: s.shtylyov, davem, kuba; +Cc: netdev, linux-renesas-soc, Yoshihiro Shimoda
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(+)
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))
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor
2021-09-07 11:29 [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor Yoshihiro Shimoda
@ 2021-09-07 13:10 ` patchwork-bot+netdevbpf
2021-09-07 19:29 ` Sergey Shtylyov
1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-09-07 13:10 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: s.shtylyov, davem, kuba, netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor
2021-09-07 11:29 [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor Yoshihiro Shimoda
2021-09-07 13:10 ` patchwork-bot+netdevbpf
@ 2021-09-07 19:29 ` Sergey Shtylyov
2021-09-08 5:45 ` Yoshihiro Shimoda
1 sibling, 1 reply; 6+ messages in thread
From: Sergey Shtylyov @ 2021-09-07 19:29 UTC (permalink / raw)
To: Yoshihiro Shimoda, davem, kuba; +Cc: netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor
2021-09-07 19:29 ` Sergey Shtylyov
@ 2021-09-08 5:45 ` Yoshihiro Shimoda
2021-09-08 9:46 ` Sergey Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Yoshihiro Shimoda @ 2021-09-08 5:45 UTC (permalink / raw)
To: Sergey Shtylyov, davem, kuba; +Cc: netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor
2021-09-08 5:45 ` Yoshihiro Shimoda
@ 2021-09-08 9:46 ` Sergey Shtylyov
2021-09-08 11:43 ` Yoshihiro Shimoda
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Shtylyov @ 2021-09-08 9:46 UTC (permalink / raw)
To: Yoshihiro Shimoda, davem, kuba; +Cc: netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor
2021-09-08 9:46 ` Sergey Shtylyov
@ 2021-09-08 11:43 ` Yoshihiro Shimoda
0 siblings, 0 replies; 6+ messages in thread
From: Yoshihiro Shimoda @ 2021-09-08 11:43 UTC (permalink / raw)
To: Sergey Shtylyov, davem, kuba; +Cc: netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-08 11:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07 11:29 [PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor Yoshihiro Shimoda
2021-09-07 13:10 ` patchwork-bot+netdevbpf
2021-09-07 19:29 ` Sergey Shtylyov
2021-09-08 5:45 ` Yoshihiro Shimoda
2021-09-08 9:46 ` Sergey Shtylyov
2021-09-08 11:43 ` Yoshihiro Shimoda
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.