| Message ID | 1448181858-5935-2-git-send-email-zhengsq@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
From: Shunqian Zheng <zhengsq@rock-chips.com> Date: Sun, 22 Nov 2015 16:44:18 +0800 > From: ZhengShunQian <zhengsq@rock-chips.com> > > The init_dma_desc_rings() may realloc the rx_skbuff[] when > suspend and resume. This patch free the rx_skbuff[] before > reallocing memory. > > Signed-off-by: ZhengShunQian <zhengsq@rock-chips.com> This isn't really the right way to fix this. I see two reasonable approaches: 1) suspend liberates the RX ring, although this approach is less desirable 2) resume doesn't try to allocate already populated RX ring entries Freeing the whole RX ring just to allocate it again immediately makes no sense at all and is wasteful work.
Hello On 11/24/2015 7:09 PM, David Miller wrote: > From: Shunqian Zheng <zhengsq@rock-chips.com> > Date: Sun, 22 Nov 2015 16:44:18 +0800 > >> From: ZhengShunQian <zhengsq@rock-chips.com> >> >> The init_dma_desc_rings() may realloc the rx_skbuff[] when >> suspend and resume. This patch free the rx_skbuff[] before >> reallocing memory. >> >> Signed-off-by: ZhengShunQian <zhengsq@rock-chips.com> > > This isn't really the right way to fix this. > > I see two reasonable approaches: > > 1) suspend liberates the RX ring, although this approach is less > desirable > > 2) resume doesn't try to allocate already populated RX ring > entries > > Freeing the whole RX ring just to allocate it again immediately > makes no sense at all and is wasteful work. This is a bug in this driver version that, to be honest, we fixed with the first approach on STi kernel. The patch just called the dma_free_rx_skbufs(priv) in the suspend. I can give you this patch that is tested on my side too. But! I do think we should move on second approach. Indeed, also on ST platforms, when we play with suspend states the DDR although in self-refresh the data are not lost at all. No reason to free and reallocate all in suspend/resume. I can test that and then provide another patch to this mailing list asap. Let me know. peppe
On 11/25/2015 4:13 PM, Giuseppe CAVALLARO wrote: > Hello > > On 11/24/2015 7:09 PM, David Miller wrote: >> From: Shunqian Zheng <zhengsq@rock-chips.com> >> Date: Sun, 22 Nov 2015 16:44:18 +0800 >> >>> From: ZhengShunQian <zhengsq@rock-chips.com> >>> >>> The init_dma_desc_rings() may realloc the rx_skbuff[] when >>> suspend and resume. This patch free the rx_skbuff[] before >>> reallocing memory. >>> >>> Signed-off-by: ZhengShunQian <zhengsq@rock-chips.com> >> >> This isn't really the right way to fix this. >> >> I see two reasonable approaches: >> >> 1) suspend liberates the RX ring, although this approach is less >> desirable >> >> 2) resume doesn't try to allocate already populated RX ring >> entries >> >> Freeing the whole RX ring just to allocate it again immediately >> makes no sense at all and is wasteful work. > > This is a bug in this driver version that, to be honest, we fixed with > the first approach on STi kernel. > The patch just called the dma_free_rx_skbufs(priv) in the suspend. > I can give you this patch that is tested on my side too. > But! I do think we should move on second approach. > Indeed, also on ST platforms, when we play with suspend states > the DDR although in self-refresh the data are not lost at all. > No reason to free and reallocate all in suspend/resume. > I can test that and then provide another patch to this mailing list > asap. I have just send the patch (directly for approach #2). Peppe > > Let me know. > peppe > > >
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 64d8aa4..2af1ed9 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1022,6 +1022,14 @@ static void stmmac_free_rx_buffers(struct stmmac_priv *priv, int i) priv->rx_skbuff[i] = NULL; } +static void dma_free_rx_skbufs(struct stmmac_priv *priv) +{ + int i; + + for (i = 0; i < priv->dma_rx_size; i++) + stmmac_free_rx_buffers(priv, i); +} + /** * init_dma_desc_rings - init the RX/TX descriptor rings * @dev: net device structure @@ -1058,6 +1066,8 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags) /* RX INITIALIZATION */ pr_debug("\tSKB addresses:\nskb\t\tskb data\tdma data\n"); } + + dma_free_rx_skbufs(priv); for (i = 0; i < rxsize; i++) { struct dma_desc *p; if (priv->extend_desc) @@ -1122,14 +1132,6 @@ err_init_rx_buffers: return ret; } -static void dma_free_rx_skbufs(struct stmmac_priv *priv) -{ - int i; - - for (i = 0; i < priv->dma_rx_size; i++) - stmmac_free_rx_buffers(priv, i); -} - static void dma_free_tx_skbufs(struct stmmac_priv *priv) { int i;