| Message ID | 20230517161502.1862260-1-shenwei.wang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: fec: add wmb to ensure correct descriptor values | expand |
On 17.05.2023 18:15, Shenwei Wang wrote: > Two wmb() are added in the XDP TX path to ensure proper ordering of > descriptor and buffer updates: > 1. A wmb() is added after updating the last BD to make sure > the updates to rest of the descriptor are visible before > transferring ownership to FEC. > 2. A wmb() is also added after updating the tx_skbuff and bdp > to ensure these updates are visible before updating txq->bd.cur. > 3. Start the xmit of the frame immediately right after configuring the > tx descriptor. > > Fixes: 6d6b39f180b8 ("net: fec: add initial XDP support") > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com> > --- > v2: > - update the inline comments for 2nd wmb per Wei Fang's review. > > drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 6d0b46c76924..d210e67a188b 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -3834,6 +3834,11 @@ static int fec_enet_txq_xmit_frame(struct fec_enet_private *fep, > index = fec_enet_get_bd_index(last_bdp, &txq->bd); > txq->tx_skbuff[index] = NULL; > > + /* Make sure the updates to rest of the descriptor are performed before > + * transferring ownership. > + */ > + wmb(); > + > /* Send it on its way. Tell FEC it's ready, interrupt when done, > * it's the last BD of the frame, and to put the CRC on the end. > */ > @@ -3843,8 +3848,14 @@ static int fec_enet_txq_xmit_frame(struct fec_enet_private *fep, > /* If this was the last BD in the ring, start at the beginning again. */ > bdp = fec_enet_get_nextdesc(last_bdp, &txq->bd); > > + /* Make sure the update to bdp are performed before txq->bd.cur. */ > + wmb(); > + Do you really need wmb() here or would a more light-weight smp_wmb() or dma_wmb() be sufficient? > txq->bd.cur = bdp; > > + /* Trigger transmission start */ > + writel(0, txq->bd.reg_desc_active); > + > return 0; > } > > @@ -3873,12 +3884,6 @@ static int fec_enet_xdp_xmit(struct net_device *dev, > sent_frames++; > } > > - /* Make sure the update to bdp and tx_skbuff are performed. */ > - wmb(); > - > - /* Trigger transmission start */ > - writel(0, txq->bd.reg_desc_active); > - > __netif_tx_unlock(nq); > > return sent_frames; > -- > 2.34.1 > >
> -----Original Message----- > From: Heiner Kallweit <hkallweit1@gmail.com> > Sent: Wednesday, May 17, 2023 2:14 PM > To: Shenwei Wang <shenwei.wang@nxp.com>; Wei Fang <wei.fang@nxp.com>; > David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Simon Horman <horms@kernel.org>; Horatiu Vultur > <horatiu.vultur@microchip.com> > Cc: Clark Wang <xiaoning.wang@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; > netdev@vger.kernel.org; imx@lists.linux.dev > Subject: [EXT] Re: [PATCH v2 net] net: fec: add wmb to ensure correct descriptor > values > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > On 17.05.2023 18:15, Shenwei Wang wrote: > > Two wmb() are added in the XDP TX path to ensure proper ordering of > > descriptor and buffer updates: > > 1. A wmb() is added after updating the last BD to make sure > > the updates to rest of the descriptor are visible before > > transferring ownership to FEC. > > 2. A wmb() is also added after updating the tx_skbuff and bdp > > to ensure these updates are visible before updating txq->bd.cur. > > 3. Start the xmit of the frame immediately right after configuring the > > tx descriptor. > > > > Fixes: 6d6b39f180b8 ("net: fec: add initial XDP support") > > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com> > > --- > > v2: > > - update the inline comments for 2nd wmb per Wei Fang's review. > > > > drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > b/drivers/net/ethernet/freescale/fec_main.c > > index 6d0b46c76924..d210e67a188b 100644 > > --- a/drivers/net/ethernet/freescale/fec_main.c > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > @@ -3834,6 +3834,11 @@ static int fec_enet_txq_xmit_frame(struct > fec_enet_private *fep, > > index = fec_enet_get_bd_index(last_bdp, &txq->bd); > > txq->tx_skbuff[index] = NULL; > > > > + /* Make sure the updates to rest of the descriptor are performed before > > + * transferring ownership. > > + */ > > + wmb(); > > + > > /* Send it on its way. Tell FEC it's ready, interrupt when done, > > * it's the last BD of the frame, and to put the CRC on the end. > > */ > > @@ -3843,8 +3848,14 @@ static int fec_enet_txq_xmit_frame(struct > fec_enet_private *fep, > > /* If this was the last BD in the ring, start at the beginning again. */ > > bdp = fec_enet_get_nextdesc(last_bdp, &txq->bd); > > > > + /* Make sure the update to bdp are performed before txq->bd.cur. */ > > + wmb(); > > + > > Do you really need wmb() here or would a more light-weight smp_wmb() or > dma_wmb() be sufficient? > In this case, both wmb should be changed to dma_wmb. Regards, Shenwei > > txq->bd.cur = bdp; > > > > + /* Trigger transmission start */ > > + writel(0, txq->bd.reg_desc_active); > > + > > return 0; > > } > > > > @@ -3873,12 +3884,6 @@ static int fec_enet_xdp_xmit(struct net_device > *dev, > > sent_frames++; > > } > > > > - /* Make sure the update to bdp and tx_skbuff are performed. */ > > - wmb(); > > - > > - /* Trigger transmission start */ > > - writel(0, txq->bd.reg_desc_active); > > - > > __netif_tx_unlock(nq); > > > > return sent_frames; > > -- > > 2.34.1 > > > >
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 6d0b46c76924..d210e67a188b 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -3834,6 +3834,11 @@ static int fec_enet_txq_xmit_frame(struct fec_enet_private *fep, index = fec_enet_get_bd_index(last_bdp, &txq->bd); txq->tx_skbuff[index] = NULL; + /* Make sure the updates to rest of the descriptor are performed before + * transferring ownership. + */ + wmb(); + /* Send it on its way. Tell FEC it's ready, interrupt when done, * it's the last BD of the frame, and to put the CRC on the end. */ @@ -3843,8 +3848,14 @@ static int fec_enet_txq_xmit_frame(struct fec_enet_private *fep, /* If this was the last BD in the ring, start at the beginning again. */ bdp = fec_enet_get_nextdesc(last_bdp, &txq->bd); + /* Make sure the update to bdp are performed before txq->bd.cur. */ + wmb(); + txq->bd.cur = bdp; + /* Trigger transmission start */ + writel(0, txq->bd.reg_desc_active); + return 0; } @@ -3873,12 +3884,6 @@ static int fec_enet_xdp_xmit(struct net_device *dev, sent_frames++; } - /* Make sure the update to bdp and tx_skbuff are performed. */ - wmb(); - - /* Trigger transmission start */ - writel(0, txq->bd.reg_desc_active); - __netif_tx_unlock(nq); return sent_frames;
Two wmb() are added in the XDP TX path to ensure proper ordering of descriptor and buffer updates: 1. A wmb() is added after updating the last BD to make sure the updates to rest of the descriptor are visible before transferring ownership to FEC. 2. A wmb() is also added after updating the tx_skbuff and bdp to ensure these updates are visible before updating txq->bd.cur. 3. Start the xmit of the frame immediately right after configuring the tx descriptor. Fixes: 6d6b39f180b8 ("net: fec: add initial XDP support") Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com> --- v2: - update the inline comments for 2nd wmb per Wei Fang's review. drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) -- 2.34.1