| Message ID | 20210204112144.24163-6-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethernet: fixes for stmmac driver | expand |
On Thu, 4 Feb 2021 19:21:44 +0800 Joakim Zhang wrote: > +err_reinit_rx_buffers: > + while (queue >= 0) { > + while (--i >= 0) > + stmmac_free_rx_buffer(priv, queue, i); > + > + if (queue == 0) > + break; > + > + i = priv->dma_rx_size; > + queue--; > + } nit: do { ... } while (queue-- > 0); > + > + return -ENOMEM; the caller ignores the return value anyway, so you make make this function void. I'm not sure why you recycle and reallocate every buffer. Isn't it enough to reinitialize the descriptors with the buffers which are already allocated?
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月7日 4:38 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V4 net 5/5] net: stmmac: re-init rx buffers when mac > resume back > > On Thu, 4 Feb 2021 19:21:44 +0800 Joakim Zhang wrote: > > +err_reinit_rx_buffers: > > + while (queue >= 0) { > > + while (--i >= 0) > > + stmmac_free_rx_buffer(priv, queue, i); > > + > > + if (queue == 0) > > + break; > > + > > + i = priv->dma_rx_size; > > + queue--; > > + } > > nit: > > do { > ... > } while (queue-- > 0); OK, will change it. > > + > > + return -ENOMEM; > > the caller ignores the return value anyway, so you make make this function > void. OK. > I'm not sure why you recycle and reallocate every buffer. Isn't it enough to > reinitialize the descriptors with the buffers which are already allocated? As I know, the receive buffer address is not fixed after allocated, it will recycle and re-allocate in stmmac_rx(), where to handle the receive buffers. It should be enough to re-initialize the descriptors with the buffers if it is possible. Could you point me how to do it? Best Regards, Joakim Zhang
On Sat, 20 Feb 2021 07:52:46 +0000 Joakim Zhang wrote: > > I'm not sure why you recycle and reallocate every buffer. Isn't it enough to > > reinitialize the descriptors with the buffers which are already allocated? > > As I know, the receive buffer address is not fixed after allocated, > it will recycle and re-allocate in stmmac_rx(), where to handle the > receive buffers. Not sure what you mean by that. The driver must know the addresses of the memory it allocated and handed over to the device. > It should be enough to re-initialize the descriptors with the buffers > if it is possible. Could you point me how to do it?
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月23日 3:48 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V4 net 5/5] net: stmmac: re-init rx buffers when mac > resume back > > On Sat, 20 Feb 2021 07:52:46 +0000 Joakim Zhang wrote: > > > I'm not sure why you recycle and reallocate every buffer. Isn't it > > > enough to reinitialize the descriptors with the buffers which are already > allocated? > > > > As I know, the receive buffer address is not fixed after allocated, it > > will recycle and re-allocate in stmmac_rx(), where to handle the > > receive buffers. > > Not sure what you mean by that. The driver must know the addresses of the > memory it allocated and handed over to the device. What I mean is that, in stmmac driver, it creates a page pool for rx patch, it will always recycle and re-allocate pages to fill descriptors' dma address. Best Regards, Joakim Zhang > > It should be enough to re-initialize the descriptors with the buffers > > if it is possible. Could you point me how to do it?
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 04ba77775e52..e8e9e5b8c62d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1379,6 +1379,91 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv, u32 queue, int i) } } +/** + * stmmac_reinit_rx_buffers - reinit the RX descriptor buffer. + * @priv: driver private structure + * Description: this function is called to re-allocate a receive buffer, perform + * the DMA mapping and init the descriptor. + */ +static int stmmac_reinit_rx_buffers(struct stmmac_priv *priv) +{ + u32 rx_count = priv->plat->rx_queues_to_use; + u32 queue; + int i; + + for (queue = 0; queue < rx_count; queue++) { + struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue]; + + for (i = 0; i < priv->dma_rx_size; i++) { + struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i]; + + if (buf->page) { + page_pool_recycle_direct(rx_q->page_pool, buf->page); + buf->page = NULL; + } + + if (priv->sph && buf->sec_page) { + page_pool_recycle_direct(rx_q->page_pool, buf->sec_page); + buf->sec_page = NULL; + } + } + } + + for (queue = 0; queue < rx_count; queue++) { + struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue]; + + for (i = 0; i < priv->dma_rx_size; i++) { + struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i]; + struct dma_desc *p; + + if (priv->extend_desc) + p = &((rx_q->dma_erx + i)->basic); + else + p = rx_q->dma_rx + i; + + if (!buf->page) { + buf->page = page_pool_dev_alloc_pages(rx_q->page_pool); + if (!buf->page) + goto err_reinit_rx_buffers; + + buf->addr = page_pool_get_dma_addr(buf->page); + } + + if (priv->sph && !buf->sec_page) { + buf->sec_page = page_pool_dev_alloc_pages(rx_q->page_pool); + if (!buf->sec_page) + goto err_reinit_rx_buffers; + + buf->sec_addr = page_pool_get_dma_addr(buf->sec_page); + } + + stmmac_set_desc_addr(priv, p, buf->addr); + if (priv->sph) + stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true); + else + stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false); + if (priv->dma_buf_sz == BUF_SIZE_16KiB) + stmmac_init_desc3(priv, p); + } + } + + return 0; + +err_reinit_rx_buffers: + while (queue >= 0) { + while (--i >= 0) + stmmac_free_rx_buffer(priv, queue, i); + + if (queue == 0) + break; + + i = priv->dma_rx_size; + queue--; + } + + return -ENOMEM; +} + /** * init_dma_rx_desc_rings - init the RX descriptor rings * @dev: net device structure @@ -5340,7 +5425,7 @@ int stmmac_resume(struct device *dev) mutex_lock(&priv->lock); stmmac_reset_queues_param(priv); - + stmmac_reinit_rx_buffers(priv); stmmac_free_tx_skbufs(priv); stmmac_clear_descriptors(priv);
During suspend/resume stress test, we found descriptor write back by DMA could exhibit unusual behavior, e.g.: 003 [0xc4310030]: 0x0 0x40 0x0 0xb5010040 We can see that desc3 write back is 0xb5010040, it is still ownd by DMA, so application would not recycle this buffer. It will trigger fatal bus error when DMA try to use this descriptor again. To fix this issue, we should re-init all rx buffers when mac resume back. Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 87 ++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-)