Hi Florian,
On Mon, Apr 4, 2016 at 5:58 PM, Florian Fainelli <[email protected]> wrote:
>
> 2016-04-04 17:10 GMT-07:00 Petri Gynther <[email protected]>:
> > dmadesc_set() is used for setting the Tx buffer DMA address, length,
> > and status bits on a Tx ring descriptor when a frame is being Tx'ed.
> >
> > Always set the Tx buffer DMA address first, before updating the length
> > and status bits, i.e. giving the Tx descriptor to the hardware.
>
> Does this fix any real bug you have observed? The hardware won't
> transmit anything until you start writing the correct TDMA producer
> index. Also, dmadesc_set_length_status and dmadesc_set_addr both use
> I/O accessors which use a volatile, so they should not be re-ordered
> relative to each other.
>
> I do agree that the change looks like how it should be done, I am just
> questioning the qualification of this as a fix or not.
>
> Thanks!
You are right. Nothing is transmitted until TDMA producer index is
incremented, and that happens after dmadesc_set() has been called.
So, this is really just a cleanup. Perhaps not worth it, after all.
But I do have a question -- do we need wmb() in bcmgenet_xmit(), just
before we kick the TDMA producer index, to ensure Tx descriptor writes
have been completed?
@@ -1606,6 +1606,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff
*skb, struct net_device *dev)
if (!skb->xmit_more || netif_xmit_stopped(txq)) {
/* Packets are ready, update producer index */
+ wmb();
bcmgenet_tdma_ring_writel(priv, ring->index,
ring->prod_index, TDMA_PROD_INDEX);
>
>
> >
> > Signed-off-by: Petri Gynther <[email protected]>
> > ---
> > drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > index d77cd6d..f7b42b9 100644
> > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > @@ -104,8 +104,8 @@ static inline void dmadesc_set_addr(struct
> > bcmgenet_priv *priv,
> > static inline void dmadesc_set(struct bcmgenet_priv *priv,
> > void __iomem *d, dma_addr_t addr, u32 val)
> > {
> > - dmadesc_set_length_status(priv, d, val);
> > dmadesc_set_addr(priv, d, addr);
> > + dmadesc_set_length_status(priv, d, val);
> > }
> >
> > static inline dma_addr_t dmadesc_get_addr(struct bcmgenet_priv *priv,
> > --
> > 2.8.0.rc3.226.g39d4020
> >
>
>
>
> --
> Florian
