| Message ID | 1578967276-55956-2-git-send-email-boon.leong.ong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: general fixes for Ethernet functionality | expand |
From: Ong Boon Leong <boon.leong.ong@intel.com> Date: Jan/14/2020, 02:01:10 (UTC+00:00) > DMA_CH(#i)_RxDesc_Tail_Pointer points to an offset from the base and > indicates the location of the last valid descriptor. > > The change introduced by "net: stmmac: Update RX Tail Pointer to last > free entry" incorrectly updates the RxDesc_Tail_Pointer and causess > Rx operation to freeze in corner case. The issue is explained as > follow:- > > Say, cur_rx=1 and dirty_rx=0, then we have dirty=1 and entry=0 before > the while (dirty-- > 0) loop of stmmac_rx_refill() is entered. When > the while loop is 1st entered, Rx buffer[entry=0] is refilled and after > entry++, then, entry=1. Now, the while loop condition check "dirty-- > 0" > and the while loop bails out because dirty=0. Up to this point, the > driver code works correctly. > > However, the current implementation sets the Rx Tail Pointer to the > location pointed by dirty_rx, just updated to the value of entry(=1). > This is incorrect because the last Rx buffer that is refileld with empty > buffer is with entry=0. In another words, the current logics always sets > Rx Tail Pointer to the next Rx buffer to be refilled (too early). > > So, we fix this by tracking the index of the most recently refilled Rx > buffer by using "last_refill" and use "last_refill" to update the Rx Tail > Pointer instead of using "entry" which points to the next dirty_rx to be > refilled in future. I'm not sure about this ... RX Tail points to last valid descriptor but it doesn't point to the base address of that one, it points to the end address. Let's say we have a ring buffer with just 1 descriptor. With your new logic then: RX base == RX tail (== RX base), so the IP will not see any descriptor. But with old logic: RX base == (RX base + 1), which causes the IP to correctly see the descriptor. Can you provide more information on the Rx operation freeze you mentioned ? Can it be another issue ? --- Thanks, Jose Miguel Abreu
>From: Ong Boon Leong <boon.leong.ong@intel.com> >Date: Jan/14/2020, 02:01:10 (UTC+00:00) > >> DMA_CH(#i)_RxDesc_Tail_Pointer points to an offset from the base and >> indicates the location of the last valid descriptor. >> >> The change introduced by "net: stmmac: Update RX Tail Pointer to last >> free entry" incorrectly updates the RxDesc_Tail_Pointer and causess >> Rx operation to freeze in corner case. The issue is explained as >> follow:- >> >> Say, cur_rx=1 and dirty_rx=0, then we have dirty=1 and entry=0 before >> the while (dirty-- > 0) loop of stmmac_rx_refill() is entered. When >> the while loop is 1st entered, Rx buffer[entry=0] is refilled and after >> entry++, then, entry=1. Now, the while loop condition check "dirty-- > 0" >> and the while loop bails out because dirty=0. Up to this point, the >> driver code works correctly. >> >> However, the current implementation sets the Rx Tail Pointer to the >> location pointed by dirty_rx, just updated to the value of entry(=1). >> This is incorrect because the last Rx buffer that is refileld with empty >> buffer is with entry=0. In another words, the current logics always sets >> Rx Tail Pointer to the next Rx buffer to be refilled (too early). >> >> So, we fix this by tracking the index of the most recently refilled Rx >> buffer by using "last_refill" and use "last_refill" to update the Rx Tail >> Pointer instead of using "entry" which points to the next dirty_rx to be >> refilled in future. > >I'm not sure about this ... > >RX Tail points to last valid descriptor but it doesn't point to the base >address of that one, it points to the end address. > >Let's say we have a ring buffer with just 1 descriptor. With your new >logic then: RX base == RX tail (== RX base), so the IP will not see any >descriptor. But with old logic: RX base == (RX base + 1), which causes >the IP to correctly see the descriptor. > >Can you provide more information on the Rx operation freeze you >mentioned ? Can it be another issue ? I recheck on my side, it seems like the fix needed for simics model at my end and not needed for actual SoC. This is strange but I can check internal team. I also read through the databook which says that for 40-bit or 48-bit addressing mode, the tail pointer must be advanced to the location immediately after the descriptors are set, for the DMA to know that additional descriptors are available. Now, relooking at the current logic which sets the rx tail pointer according to the value of "dirty_rx" which can be "zero" as it takes value from entry that is incremented through STMMAC_GET_ENTRY(entry, DMA_RX_SIZE). This too can give a situation that the base and tail registers is pointing to the same location. According to SNPS databook, the DMA engine goes into SUSPEND state if the Rx descriptors are not OWN=1. The operation can be resumed by ensuring that the descriptors are owned by the DMA and then update the tail pointer. What is your opinion here if we always update the Rx tail pointer to pointer the boundary of the DMA size as follow without depending on dirty_rx. rx_q->rx_tail_addr = rx_q->dma_rx_phy + (DMA_RX_SIZE * sizeof(struct dma_desc)) Thanks Boon Leong
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 80d59b7..a317f67 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3417,6 +3417,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue) struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue]; int len, dirty = stmmac_rx_dirty(priv, queue); unsigned int entry = rx_q->dirty_rx; + unsigned int last_refill = entry; len = DIV_ROUND_UP(priv->dma_buf_sz, PAGE_SIZE) * PAGE_SIZE; @@ -3471,12 +3472,13 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue) dma_wmb(); stmmac_set_rx_owner(priv, p, use_rx_wd); - + last_refill = entry; entry = STMMAC_GET_ENTRY(entry, DMA_RX_SIZE); } + rx_q->dirty_rx = entry; rx_q->rx_tail_addr = rx_q->dma_rx_phy + - (rx_q->dirty_rx * sizeof(struct dma_desc)); + (last_refill * sizeof(struct dma_desc)); stmmac_set_rx_tail_ptr(priv, priv->ioaddr, rx_q->rx_tail_addr, queue); }
DMA_CH(#i)_RxDesc_Tail_Pointer points to an offset from the base and indicates the location of the last valid descriptor. The change introduced by "net: stmmac: Update RX Tail Pointer to last free entry" incorrectly updates the RxDesc_Tail_Pointer and causess Rx operation to freeze in corner case. The issue is explained as follow:- Say, cur_rx=1 and dirty_rx=0, then we have dirty=1 and entry=0 before the while (dirty-- > 0) loop of stmmac_rx_refill() is entered. When the while loop is 1st entered, Rx buffer[entry=0] is refilled and after entry++, then, entry=1. Now, the while loop condition check "dirty-- > 0" and the while loop bails out because dirty=0. Up to this point, the driver code works correctly. However, the current implementation sets the Rx Tail Pointer to the location pointed by dirty_rx, just updated to the value of entry(=1). This is incorrect because the last Rx buffer that is refileld with empty buffer is with entry=0. In another words, the current logics always sets Rx Tail Pointer to the next Rx buffer to be refilled (too early). So, we fix this by tracking the index of the most recently refilled Rx buffer by using "last_refill" and use "last_refill" to update the Rx Tail Pointer instead of using "entry" which points to the next dirty_rx to be refilled in future. Fixes: 858a31ffc3d9 ("net: stmmac: Update RX Tail Pointer to last free entry") Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)