RE: [PATCH v3 06/10] net/macb: clean up ring buffer logic

2012-10-31 Thread David Laight 
>   return (TX_RING_SIZE - (bp->tx_head - bp->tx_tail) & (TX_RING_SIZE - 
> 1));

Is equivalent to:

return (bp->tx_tail - bp->tx_head) & (TX_RING_SIZE - 1));

David



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 06/10] net/macb: clean up ring buffer logic

2012-10-31 Thread Nicolas Ferre 
On 10/31/2012 10:59 AM, Nicolas Ferre :
> On 10/30/2012 07:22 PM, Håvard Skinnemoen :
>> On Tue, Oct 30, 2012 at 4:12 AM, David Laight  
>> wrote:
 Instead of masking head and tail every time we increment them, just let 
 them
 wrap through UINT_MAX and mask them when subscripting. Add simple accessor
 functions to do the subscripting properly to minimize the chances of 
 messing
 this up.
>>> ...
 +static unsigned int macb_tx_ring_avail(struct macb *bp)
 +{
 + return TX_RING_SIZE - (bp->tx_head - bp->tx_tail);
 +}
>>>
>>> That one doesn't look quite right to me.
>>> Surely it should be masking with 'TX_RING_SIZE - 1'
>>
>> Why is that? head and tail can never be more than TX_RING_SIZE apart,
>> so it shouldn't make any difference.
> 
> Absolutely.

Well not so absolute, after having thinking twice ;-)

We should move to:

static unsigned int macb_tx_ring_avail(struct macb *bp)
{
return (TX_RING_SIZE - (bp->tx_head - bp->tx_tail) & (TX_RING_SIZE - 
1));
}

Thanks David!

(sorry for the noise) Bye,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 06/10] net/macb: clean up ring buffer logic

2012-10-31 Thread David Laight 
> On Tue, Oct 30, 2012 at 4:12 AM, David Laight  wrote:
> >> Instead of masking head and tail every time we increment them, just let 
> >> them
> >> wrap through UINT_MAX and mask them when subscripting. Add simple accessor
> >> functions to do the subscripting properly to minimize the chances of 
> >> messing
> >> this up.
> > ...
> >> +static unsigned int macb_tx_ring_avail(struct macb *bp)
> >> +{
> >> + return TX_RING_SIZE - (bp->tx_head - bp->tx_tail);
> >> +}
> >
> > That one doesn't look quite right to me.
> > Surely it should be masking with 'TX_RING_SIZE - 1'
> 
> Why is that? head and tail can never be more than TX_RING_SIZE apart,
> so it shouldn't make any difference.

It's a ring buffer (I presume) the pointers can be in either order.

David



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 06/10] net/macb: clean up ring buffer logic

2012-10-31 Thread Nicolas Ferre 
On 10/30/2012 07:22 PM, Håvard Skinnemoen :
> On Tue, Oct 30, 2012 at 4:12 AM, David Laight  wrote:
>>> Instead of masking head and tail every time we increment them, just let them
>>> wrap through UINT_MAX and mask them when subscripting. Add simple accessor
>>> functions to do the subscripting properly to minimize the chances of messing
>>> this up.
>> ...
>>> +static unsigned int macb_tx_ring_avail(struct macb *bp)
>>> +{
>>> + return TX_RING_SIZE - (bp->tx_head - bp->tx_tail);
>>> +}
>>
>> That one doesn't look quite right to me.
>> Surely it should be masking with 'TX_RING_SIZE - 1'
> 
> Why is that? head and tail can never be more than TX_RING_SIZE apart,
> so it shouldn't make any difference.

Absolutely.

Best regards,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 06/10] net/macb: clean up ring buffer logic

2012-10-30 Thread Håvard Skinnemoen 
On Tue, Oct 30, 2012 at 4:12 AM, David Laight  wrote:
>> Instead of masking head and tail every time we increment them, just let them
>> wrap through UINT_MAX and mask them when subscripting. Add simple accessor
>> functions to do the subscripting properly to minimize the chances of messing
>> this up.
> ...
>> +static unsigned int macb_tx_ring_avail(struct macb *bp)
>> +{
>> + return TX_RING_SIZE - (bp->tx_head - bp->tx_tail);
>> +}
>
> That one doesn't look quite right to me.
> Surely it should be masking with 'TX_RING_SIZE - 1'

Why is that? head and tail can never be more than TX_RING_SIZE apart,
so it shouldn't make any difference.

Havard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 06/10] net/macb: clean up ring buffer logic

2012-10-30 Thread David Laight 
> Instead of masking head and tail every time we increment them, just let them
> wrap through UINT_MAX and mask them when subscripting. Add simple accessor
> functions to do the subscripting properly to minimize the chances of messing
> this up.
...
> +static unsigned int macb_tx_ring_avail(struct macb *bp)
> +{
> + return TX_RING_SIZE - (bp->tx_head - bp->tx_tail);
> +}

That one doesn't look quite right to me.
Surely it should be masking with 'TX_RING_SIZE - 1'

David



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3 06/10] net/macb: clean up ring buffer logic

2012-10-30 Thread Nicolas Ferre 
From: Havard Skinnemoen 

Instead of masking head and tail every time we increment them, just let them
wrap through UINT_MAX and mask them when subscripting. Add simple accessor
functions to do the subscripting properly to minimize the chances of messing
this up.

This makes the code slightly smaller, and hopefully faster as well.  Also,
doing the ring buffer management this way will simplify things a lot when
making the ring sizes configurable in the future.

Signed-off-by: Havard Skinnemoen 
[nicolas.fe...@atmel.com: split patch in topics, adapt to newer kernel]
Signed-off-by: Nicolas Ferre 
---
 drivers/net/ethernet/cadence/macb.c | 168 +++-
 drivers/net/ethernet/cadence/macb.h |  22 +++--
 2 files changed, 122 insertions(+), 68 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index cd6d431..dc03b36 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -31,24 +31,13 @@
 
 #define RX_BUFFER_SIZE 128
 #define RX_RING_SIZE   512
-#define RX_RING_BYTES  (sizeof(struct dma_desc) * RX_RING_SIZE)
+#define RX_RING_BYTES  (sizeof(struct macb_dma_desc) * RX_RING_SIZE)
 
 /* Make the IP header word-aligned (the ethernet header is 14 bytes) */
 #define RX_OFFSET  2
 
 #define TX_RING_SIZE   128
-#define DEF_TX_RING_PENDING(TX_RING_SIZE - 1)
-#define TX_RING_BYTES  (sizeof(struct dma_desc) * TX_RING_SIZE)
-
-#define TX_RING_GAP(bp)\
-   (TX_RING_SIZE - (bp)->tx_pending)
-#define TX_BUFFS_AVAIL(bp) \
-   (((bp)->tx_tail <= (bp)->tx_head) ? \
-(bp)->tx_tail + (bp)->tx_pending - (bp)->tx_head : \
-(bp)->tx_tail - (bp)->tx_head - TX_RING_GAP(bp))
-#define NEXT_TX(n) (((n) + 1) & (TX_RING_SIZE - 1))
-
-#define NEXT_RX(n) (((n) + 1) & (RX_RING_SIZE - 1))
+#define TX_RING_BYTES  (sizeof(struct macb_dma_desc) * TX_RING_SIZE)
 
 /* minimum number of free TX descriptors before waking up TX process */
 #define MACB_TX_WAKEUP_THRESH  (TX_RING_SIZE / 4)
@@ -56,6 +45,51 @@
 #define MACB_RX_INT_FLAGS  (MACB_BIT(RCOMP) | MACB_BIT(RXUBR)  \
 | MACB_BIT(ISR_ROVR))
 
+/* Ring buffer accessors */
+static unsigned int macb_tx_ring_wrap(unsigned int index)
+{
+   return index & (TX_RING_SIZE - 1);
+}
+
+static unsigned int macb_tx_ring_avail(struct macb *bp)
+{
+   return TX_RING_SIZE - (bp->tx_head - bp->tx_tail);
+}
+
+static struct macb_dma_desc *macb_tx_desc(struct macb *bp, unsigned int index)
+{
+   return &bp->tx_ring[macb_tx_ring_wrap(index)];
+}
+
+static struct macb_tx_skb *macb_tx_skb(struct macb *bp, unsigned int index)
+{
+   return &bp->tx_skb[macb_tx_ring_wrap(index)];
+}
+
+static dma_addr_t macb_tx_dma(struct macb *bp, unsigned int index)
+{
+   dma_addr_t offset;
+
+   offset = macb_tx_ring_wrap(index) * sizeof(struct macb_dma_desc);
+
+   return bp->tx_ring_dma + offset;
+}
+
+static unsigned int macb_rx_ring_wrap(unsigned int index)
+{
+   return index & (RX_RING_SIZE - 1);
+}
+
+static struct macb_dma_desc *macb_rx_desc(struct macb *bp, unsigned int index)
+{
+   return &bp->rx_ring[macb_rx_ring_wrap(index)];
+}
+
+static void *macb_rx_buffer(struct macb *bp, unsigned int index)
+{
+   return bp->rx_buffers + RX_BUFFER_SIZE * macb_rx_ring_wrap(index);
+}
+
 static void __macb_set_hwaddr(struct macb *bp)
 {
u32 bottom;
@@ -336,17 +370,18 @@ static void macb_tx(struct macb *bp)
bp->tx_ring[TX_RING_SIZE - 1].ctrl |= MACB_BIT(TX_WRAP);
 
/* free transmit buffer in upper layer*/
-   for (tail = bp->tx_tail; tail != head; tail = NEXT_TX(tail)) {
-   struct ring_info *rp = &bp->tx_skb[tail];
-   struct sk_buff *skb = rp->skb;
-
-   BUG_ON(skb == NULL);
+   for (tail = bp->tx_tail; tail != head; tail++) {
+   struct macb_tx_skb  *tx_skb;
+   struct sk_buff  *skb;
 
rmb();
 
-   dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len,
-DMA_TO_DEVICE);
-   rp->skb = NULL;
+   tx_skb = macb_tx_skb(bp, tail);
+   skb = tx_skb->skb;
+
+   dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
+   skb->len, DMA_TO_DEVICE);
+   tx_skb->skb = NULL;
dev_kfree_skb_irq(skb);
}
 
@@ -366,34 +401,38 @@ static void macb_tx(struct macb *bp)
return;
 
head = bp->tx_head;
-   for (tail = bp->tx_tail; tail != head; tail = NEXT_TX(tail)) {
-