Re: [PATCH v2 03/46] net drivers: remove unnecessary dma_sync_to_device(DMA_FROM_DEVICE)

["Vlad Zolotarov" <vladz@broadcom.com>]

On Monday 11 July 2011 12:29:09 Michał Mirosław wrote:
> On Mon, Jul 11, 2011 at 11:30:39AM +0300, Vlad Zolotarov wrote:
> > >         prod_rx_buf->skb = skb;
> > > 
> > > diff --git a/drivers/net/bnx2x/bnx2x_cmn.h
> > > b/drivers/net/bnx2x/bnx2x_cmn.h index c016e20..c9e49a0 100644
> > > --- a/drivers/net/bnx2x/bnx2x_cmn.h
> > > +++ b/drivers/net/bnx2x/bnx2x_cmn.h
> > > @@ -923,16 +923,11 @@ static inline int bnx2x_alloc_rx_skb(struct bnx2x
> > > *bp, static inline void bnx2x_reuse_rx_skb(struct bnx2x_fastpath *fp,
> > > 
> > >                                       u16 cons, u16 prod)
> > >  
> > >  {
> > > 
> > > -       struct bnx2x *bp = fp->bp;
> > > 
> > >         struct sw_rx_bd *cons_rx_buf = &fp->rx_buf_ring[cons];
> > >         struct sw_rx_bd *prod_rx_buf = &fp->rx_buf_ring[prod];
> > >         struct eth_rx_bd *cons_bd = &fp->rx_desc_ring[cons];
> > >         struct eth_rx_bd *prod_bd = &fp->rx_desc_ring[prod];
> > > 
> > > -       dma_sync_single_for_device(&bp->pdev->dev,
> > > -                                  dma_unmap_addr(cons_rx_buf,
> > > mapping), -                                  RX_COPY_THRESH,
> > > DMA_FROM_DEVICE); -
> > > 
> > >         dma_unmap_addr_set(prod_rx_buf, mapping,
> > >         
> > >                            dma_unmap_addr(cons_rx_buf, mapping));
> > >         
> > >         prod_rx_buf->skb = cons_rx_buf->skb;
> > 
> > Michal, pls., note that this function is only called for buffers which
> > were previously dma_synced towards CPU (your "[PATCH v2 05/46] net:
> > bnx2x: fix DMA sync direction" properly fixes the direction of the first
> > call which was incorrect). Then, according to the 3d edition of the
> > "Linux device drivers" book, chapter 15, "Setting up streaming DMA
> > mappings" article, end of the page 449, when we call for
> > dma_syc_single_for_cpu() the buffer ownership gets to the CPU and CPU
> > may safely access the buffer (in particular, we read it). Then the
> > author says: "Before the device accesses the buffer, however, ownership
> > should be transfered back to it with: dma_sync_single_for_device().
> > 
> > The DMA-API.txt document u've referenced doesn't refer the above
> > function, so, it's unclear how your fix may be based on it. On the other
> > hand it clearly contradicts the "Linux device driver" book.
> 
> DMA-API.txt describes what synchronization points are necessary for what
> DMA mapping types (direction). dma_sync_single_for_cpu/device() are
> functions realising those points. Note that example DMA-API-HOWTO.txt is
> misleading as it has dma_sync_single_for_device() where its not required
> by DMA-API.txt.
> 
> In this case, you don't need to sync to device for mappings that haven't
> been written to by CPU. CPU caches will be invalidated anyway by next
> dma_sync_single_for_cpu() or dma_unmap_single() and the CPU should not
> ever write to cachelines that belong to FROM_DEVICE mappings.

Okay, I see the section in the doc u r talking about... I agree. We may drop 
these sync_single() in the bnx2x_reuse_rx_skb().

> 
> The best source is the code. 

Hmmm... The code is bug prone, so I'd stick to the Doc...;)

Thanks, Michal.
vlad


------------------------------------------------------------------------------
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security 
threats, fraudulent activity, and more. Splunk takes this data and makes 
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired