Date:   Thu, 27 May 2021 13:43:25 +0100
From:   Jon Hunter <jonathanh@...dia.com>
To:     Joakim Zhang <qiangqing.zhang@....com>, <f.fainelli@...il.com>,
        <peppe.cavallaro@...com>, <alexandre.torgue@...s.st.com>,
        <joabreu@...opsys.com>, <davem@...emloft.net>, <kuba@...nel.org>,
        <mcoquelin.stm32@...il.com>, <andrew@...n.ch>, <treding@...dia.com>
CC:     <netdev@...r.kernel.org>, <linux-imx@....com>
Subject: Re: [PATCH V1 net-next] net: stmmac: should not modify RX descriptor
 when STMMAC resume


On 27/05/2021 09:49, Joakim Zhang wrote:
> When system resume back, STMMAC will clear RX descriptors:
> stmmac_resume()
> 	->stmmac_clear_descriptors()
> 		->stmmac_clear_rx_descriptors()
> 			->stmmac_init_rx_desc()
> 				->dwmac4_set_rx_owner()
> 				//p->des3 |= cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR);
> It only asserts OWN and BUF1V bits in desc3 field, doesn't clear desc0/1/2 fields.
> 
> Let's take a case into account, when system suspend, it is possible that
> there are packets have not received yet, so the RX descriptors are wrote
> back by DMA, e.g.
> 008 [0x00000000c4310080]: 0x0 0x40 0x0 0x34010040
> 
> When system resume back, after above process, it became a broken
> descriptor:
> 008 [0x00000000c4310080]: 0x0 0x40 0x0 0xb5010040
> 
> The issue is that it only changes the owner of this descriptor, but do nothing
> about desc0/1/2 fields. The descriptor of STMMAC a bit special, applicaton
> prepares RX descriptors for DMA, after DMA recevie the packets, it will write
> back the descriptors, so the same field of a descriptor have different
> meanings to application and DMA. It should be a software bug there, and may
> not easy to reproduce, but there is a certain probability that it will
> occur.
> 
> i.MX8MP STMMAC DMA width is 34 bits, so desc0/desc1 indicates the buffer
> address, after system resume, the buffer address changes to
> 0x40_00000000. And the correct rx descriptor is 008 [0x00000000c4310080]:
> 0x6511000 0x1 0x0 0x81000000, the valid buffer address is 0x1_6511000.
> So when DMA tried to access the invalid address 0x40_00000000 would
> generate fatal bus error.
> 
> But for other 32 bits width DMA, DMA still can work when this issue happened,
> only desc0 indicates buffer address, so the buffer address is 0x00000000 when
> system resume.
> 
> There is a NOTE in the Guide:
> In the Receive Descriptor (Read Format), if the Buffer Address field is all 0s,
> the module does not transfer data to that buffer and skips to the next buffer
> or next descriptor.
> 
> Also a feedback from SYPS:
> When buffer address field of Rx descriptor is all 0's, DMA skips such descriptor
> means DMA closes Rx descriptor as Intermediate descriptor with OWN bit set to 0,
> indicates that the application owns this descriptor.
> 
> It now appears that this issue seems only can be reproduced on DMA width more
> than 32 bits, this may be why other SoCs which integrated the same STMMAC IP
> can't reproduce it.
> 
> Commit 9c63faaa931e ("net: stmmac: re-init rx buffers when mac resume back") tried
> to re-init desc0/desc1 (buffer address fields) to fix this issue, but it
> is not a proper solution, and made regression on Jetson TX2 boards.
> 
> It is unreasonable to modify RX descriptors outside of stmmac_rx_refill() function,
> where it will clear all desc0/desc1/desc2/desc3 fields together.
> 
> This patch removes RX descriptors modification when STMMAC resume.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> ---
> ChangeLogs:
> 	V1: remove RFC tag, please come here for RFC discussion:
> 	    https://lore.kernel.org/netdev/cec17489-2ef9-7862-94c8-202d31507a0c@nvidia.com/T/
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9fe25fed69..2570d26286ea 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7187,6 +7187,8 @@ static void stmmac_reset_queues_param(struct stmmac_priv *priv)
>  		tx_q->mss = 0;
>  
>  		netdev_tx_reset_queue(netdev_get_tx_queue(priv->dev, queue));
> +
> +		stmmac_clear_tx_descriptors(priv, queue);
>  	}
>  }
>  
> @@ -7251,7 +7253,6 @@ int stmmac_resume(struct device *dev)
>  	stmmac_reset_queues_param(priv);
>  
>  	stmmac_free_tx_skbufs(priv);
> -	stmmac_clear_descriptors(priv);
>  
>  	stmmac_hw_setup(ndev, false);
>  	stmmac_init_coalesce(priv);
> 


So as previously mentioned this still causing a regression when resuming
from suspend on Jetson TX2 platform. I am not sure why you are still
attempting to push this patch as-is when it causes a complete failure
for another platform. I am quite disappointed that you are ignoring the
issue we have reported :-(

To summarise we do not see any issues with suspend on Jetson TX2 without
this patch. I have stressed suspend on this board doing 2000 suspend
iterations and so no issues. However, this patch completely breaks
resuming from suspend for us. Therefore, I don't see how we can merge this.

Given that this fixes a problem, that appears to be specific to your
platform, why do you not implement this in away such that this is only
done for your platform?

Jon

-- 
nvpublic

Powered by blists - more mailing lists