Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1529971 > unrolled thread

[PATCH 1/1] net: macb: fix the RX queue reset in macb_rx()

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] net: macb: fix the RX queue reset in macb_rx() Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2016-11-25 10:00 +0100
    Re: [PATCH 1/1] net: macb: fix the RX queue reset in macb_rx() Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-11-25 10:20 +0100
    Re: [PATCH 1/1] net: macb: fix the RX queue reset in macb_rx() David Miller <davem@davemloft.net> - 2016-11-28 02:30 +0100
      Re: [PATCH 1/1] net: macb: fix the RX queue reset in macb_rx() Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2016-11-28 14:50 +0100

#1529971 — [PATCH 1/1] net: macb: fix the RX queue reset in macb_rx()

On macb only (not gem), when a RX queue corruption was detected from
macb_rx(), the RX queue was reset: during this process the RX ring
buffer descriptor was initialized by macb_init_rx_ring() but we forgot
to also set bp->rx_tail to 0.

Indeed, when processing the received frames, bp->rx_tail provides the
macb driver with the index in the RX ring buffer of the next buffer to
process. So when the whole ring buffer is reset we must also reset
bp->rx_tail so the driver is synchronized again with the hardware.

Since macb_init_rx_ring() is called from many locations, currently from
macb_rx() and macb_init_rings(), we'd rather add the "bp->rx_tail = 0;"
line inside macb_init_rx_ring() than add the very same line after each
call of this function.

Without this fix, the rx queue is not reset properly to recover from
queue corruption and connection drop may occur.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Fixes: 9ba723b081a2 ("net: macb: remove BUG_ON() and reset the queue to handle RX errors")
---
 drivers/net/ethernet/cadence/macb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 0e489bb82456..8ee303b8da08 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -991,6 +991,7 @@ static inline void macb_init_rx_ring(struct macb *bp)
 		addr += bp->rx_buffer_size;
 	}
 	bp->rx_ring[bp->rx_ring_size - 1].addr |= MACB_BIT(RX_WRAP);
+	bp->rx_tail = 0;
 }
 
 static int macb_rx(struct macb *bp, int budget)
@@ -1736,8 +1737,6 @@ static void macb_init_rings(struct macb *bp)
 	bp->queues[0].tx_head = 0;
 	bp->queues[0].tx_tail = 0;
 	bp->queues[0].tx_ring[bp->tx_ring_size - 1].ctrl |= MACB_BIT(TX_WRAP);
-
-	bp->rx_tail = 0;
 }
 
 static void macb_reset_hw(struct macb *bp)
-- 
2.7.4

[toc] | [next] | [standalone]

#1530047

Le 25/11/2016 à 09:49, Cyrille Pitchen a écrit :
> On macb only (not gem), when a RX queue corruption was detected from
> macb_rx(), the RX queue was reset: during this process the RX ring
> buffer descriptor was initialized by macb_init_rx_ring() but we forgot
> to also set bp->rx_tail to 0.
> 
> Indeed, when processing the received frames, bp->rx_tail provides the
> macb driver with the index in the RX ring buffer of the next buffer to
> process. So when the whole ring buffer is reset we must also reset
> bp->rx_tail so the driver is synchronized again with the hardware.
> 
> Since macb_init_rx_ring() is called from many locations, currently from
> macb_rx() and macb_init_rings(), we'd rather add the "bp->rx_tail = 0;"
> line inside macb_init_rx_ring() than add the very same line after each
> call of this function.
> 
> Without this fix, the rx queue is not reset properly to recover from
> queue corruption and connection drop may occur.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Fixes: 9ba723b081a2 ("net: macb: remove BUG_ON() and reset the queue to handle RX errors")

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Thanks.

Regards,

> ---
>  drivers/net/ethernet/cadence/macb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 0e489bb82456..8ee303b8da08 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -991,6 +991,7 @@ static inline void macb_init_rx_ring(struct macb *bp)
>  		addr += bp->rx_buffer_size;
>  	}
>  	bp->rx_ring[bp->rx_ring_size - 1].addr |= MACB_BIT(RX_WRAP);
> +	bp->rx_tail = 0;
>  }
>  
>  static int macb_rx(struct macb *bp, int budget)
> @@ -1736,8 +1737,6 @@ static void macb_init_rings(struct macb *bp)
>  	bp->queues[0].tx_head = 0;
>  	bp->queues[0].tx_tail = 0;
>  	bp->queues[0].tx_ring[bp->tx_ring_size - 1].ctrl |= MACB_BIT(TX_WRAP);
> -
> -	bp->rx_tail = 0;
>  }
>  
>  static void macb_reset_hw(struct macb *bp)
> 


-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1530980

From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Date: Fri, 25 Nov 2016 09:49:32 +0100

> On macb only (not gem), when a RX queue corruption was detected from
> macb_rx(), the RX queue was reset: during this process the RX ring
> buffer descriptor was initialized by macb_init_rx_ring() but we forgot
> to also set bp->rx_tail to 0.
> 
> Indeed, when processing the received frames, bp->rx_tail provides the
> macb driver with the index in the RX ring buffer of the next buffer to
> process. So when the whole ring buffer is reset we must also reset
> bp->rx_tail so the driver is synchronized again with the hardware.
> 
> Since macb_init_rx_ring() is called from many locations, currently from
> macb_rx() and macb_init_rings(), we'd rather add the "bp->rx_tail = 0;"
> line inside macb_init_rx_ring() than add the very same line after each
> call of this function.
> 
> Without this fix, the rx queue is not reset properly to recover from
> queue corruption and connection drop may occur.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Fixes: 9ba723b081a2 ("net: macb: remove BUG_ON() and reset the queue to handle RX errors")

This doesn't apply cleanly to the 'net' tree, where
RX_RING_SIZE is used instead of bp->rx_ring_size. It seems
you generated this against net-next, however you didn't say
that either in your Subject line nor the commit message.

As a bug fix this should be targetted at 'net'.

[toc] | [prev] | [next] | [standalone]

#1531308

Hi David,

Le 28/11/2016 à 02:25, David Miller a écrit :
> From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Date: Fri, 25 Nov 2016 09:49:32 +0100
> 
>> On macb only (not gem), when a RX queue corruption was detected from
>> macb_rx(), the RX queue was reset: during this process the RX ring
>> buffer descriptor was initialized by macb_init_rx_ring() but we forgot
>> to also set bp->rx_tail to 0.
>>
>> Indeed, when processing the received frames, bp->rx_tail provides the
>> macb driver with the index in the RX ring buffer of the next buffer to
>> process. So when the whole ring buffer is reset we must also reset
>> bp->rx_tail so the driver is synchronized again with the hardware.
>>
>> Since macb_init_rx_ring() is called from many locations, currently from
>> macb_rx() and macb_init_rings(), we'd rather add the "bp->rx_tail = 0;"
>> line inside macb_init_rx_ring() than add the very same line after each
>> call of this function.
>>
>> Without this fix, the rx queue is not reset properly to recover from
>> queue corruption and connection drop may occur.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>> Fixes: 9ba723b081a2 ("net: macb: remove BUG_ON() and reset the queue to handle RX errors")
> 
> This doesn't apply cleanly to the 'net' tree, where
> RX_RING_SIZE is used instead of bp->rx_ring_size. It seems
> you generated this against net-next, however you didn't say
> that either in your Subject line nor the commit message.
> 
> As a bug fix this should be targetted at 'net'.
> 

Indeed, sorry. I've just sent a v2 based on net/master.

Best regards,

Cyrille

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web