David Miller <da...@davemloft.net> :
> From: Shuyu Wei <wsy2...@gmail.com>
> Date: Tue, 17 May 2016 23:25:20 +0800
>
> > diff --git a/drivers/net/ethernet/arc/emac_main.c
> > b/drivers/net/ethernet/arc/emac_main.c
> > index a3a9392..df3dfef 100644
> > --- a/drivers/net/ethernet/arc/emac_main.c
> > +++ b/drivers/net/ethernet/arc/emac_main.c
> > @@ -153,9 +153,8 @@ static void arc_emac_tx_clean(struct net_device *ndev)
> > {
> > struct arc_emac_priv *priv = netdev_priv(ndev);
> > struct net_device_stats *stats = &ndev->stats;
> > - unsigned int i;
> >
> > - for (i = 0; i < TX_BD_NUM; i++) {
> > + while (priv->txbd_dirty != priv->txbd_curr) {
> > unsigned int *txbd_dirty = &priv->txbd_dirty;
> > struct arc_emac_bd *txbd = &priv->txbd[*txbd_dirty];
> > struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty];
> > @@ -685,13 +684,15 @@ static int arc_emac_tx(struct sk_buff *skb, struct
> > net_device *ndev)
> > wmb();
> >
> > skb_tx_timestamp(skb);
> > + priv->tx_buff[*txbd_curr].skb = skb;
> > +
> > + dma_wmb();
> >
> > *info = cpu_to_le32(FOR_EMAC | FIRST_OR_LAST_MASK | len);
> >
> > /* Make sure info word is set */
> > wmb();
> >
> > - priv->tx_buff[*txbd_curr].skb = skb;
> >
> > /* Increment index to point to the next BD */
> > *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
> >
>
> These memory barriers do not look correct to me.
>
> dma_wmb() is about visibility between CPU reads/writes and device
> accesses to a piece of memory. But what you're concerned about wrt.
> the SKB pointer assignment is CPU to CPU accesses. Therefore something
> like smp_wmb() would be appropriate.
Something like:
skb_tx_timestamp(skb);
/* CPU write vs device access. Must be done before releasing control
* of the descriptor (*info).
*/
dma_wmb();
priv->tx_buff[*txbd_curr].skb = skb;
/* CPU arc_emac_tx_clean vs CPU arc_emac_tx. Must be done before
* index (tx_curr) update. Does not necessarily deserves to be done
* before releasing control of the descriptor (*info) due to
* descriptor vs index ordering.
*
* FIXME: missing smp_rmb before the while loop in arc_emac_tx_clean.
*/
smp_wmb();
*info = cpu_to_le32(FOR_EMAC | FIRST_OR_LAST_MASK | len);
/* local descriptor (*info) update vs index (tx_curr) update. */
wmb();
*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
smp_mb(); // The driver alreay contains this one.
The smp_wmb() and wmb() could be made side-by-side once *info is
updated but I don't see the adequate idiom to improve the smp_wmb + wmb
combo. :o/
> And the wmb() looks like it should be a dma_wmb().
I see two points against it:
- it could be too late for skb_tx_timestamp().
- arc_emac_tx_clean must not see an index update before the device
got a chance to acquire the descriptor. arc_emac_tx_clean can't
tell the difference between an about-to-be-released descriptor
and a returned-from-device one.
--
Ueimor
