[PATCH] net: ti: am65-cpsw-nuss: don't touch DMA after stop

[PATCH] net: ti: am65-cpsw-nuss: don't touch DMA after stop

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Contrary to doc/develop/driver-model/ethernet.rst contract, eth_ops
.free_pkt can be called after .stop, there are several error paths in TFTP,
for instance:

eth_halt() <= tftp_handler() <= net_process_received_packet() <= eth_rx()
...
am65_cpsw_free_pkt() <= eth_rx()

Which results in (deliberately "tftpboot"ing non-existing file):

TFTP error: 'File not found' (1)
Not retrying...
am65_cpsw_nuss_port ethernet@8000000port@1: RX dma free_pkt failed -22

Avoid the error message by checking that the interface is still not stopped
in am65_cpsw_free_pkt().

Fixes: 9d0dca1199d1 ("net: ethernet: ti: Introduce am654 gigabit eth switch subsystem driver")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/net/ti/am65-cpsw-nuss.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index 65ade1afd05..646f618afcf 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -523,6 +523,9 @@ static int am65_cpsw_free_pkt(struct udevice *dev, uchar *packet, int length)
 	struct am65_cpsw_common	*common = priv->cpsw_common;
 	int ret;
 
+	if (!common->started)
+		return -ENETDOWN;
+
 	if (length > 0) {
 		u32 pkt = common->rx_next % UDMA_RX_DESC_NUM;
 
-- 
2.44.0

Re: [PATCH] net: ti: am65-cpsw-nuss: don't touch DMA after stop

[Roger Quadros]

Hi Alexander,

On 08/05/2024 21:36, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> Contrary to doc/develop/driver-model/ethernet.rst contract, eth_ops
> .free_pkt can be called after .stop, there are several error paths in TFTP,
> for instance:

Doesn't this mean we need to fix TFTP instead of patching the Ethernet
driver? I'm sure the issue is present for all Ethernet drivers as none
of them are checking if Ethernet is stopped. Just that most of them
don't print any error message so it goes unnoticed.

> 
> eth_halt() <= tftp_handler() <= net_process_received_packet() <= eth_rx()
> ...
> am65_cpsw_free_pkt() <= eth_rx()> 
> Which results in (deliberately "tftpboot"ing non-existing file):
> 
> TFTP error: 'File not found' (1)
> Not retrying...
> am65_cpsw_nuss_port ethernet@8000000port@1: RX dma free_pkt failed -22
> 
> Avoid the error message by checking that the interface is still not stopped
> in am65_cpsw_free_pkt().
> 
> Fixes: 9d0dca1199d1 ("net: ethernet: ti: Introduce am654 gigabit eth switch subsystem driver")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
>  drivers/net/ti/am65-cpsw-nuss.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
> index 65ade1afd05..646f618afcf 100644
> --- a/drivers/net/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ti/am65-cpsw-nuss.c
> @@ -523,6 +523,9 @@ static int am65_cpsw_free_pkt(struct udevice *dev, uchar *packet, int length)
>  	struct am65_cpsw_common	*common = priv->cpsw_common;
>  	int ret;
>  
> +	if (!common->started)
> +		return -ENETDOWN;
> +
>  	if (length > 0) {
>  		u32 pkt = common->rx_next % UDMA_RX_DESC_NUM;
>  

-- 
cheers,
-roger

Re: [PATCH] net: ti: am65-cpsw-nuss: don't touch DMA after stop

[Sverdlin, Alexander]

Hi Roger!

On Fri, 2024-05-10 at 12:11 +0300, Roger Quadros wrote:
> > Contrary to doc/develop/driver-model/ethernet.rst contract, eth_ops
> > .free_pkt can be called after .stop, there are several error paths in TFTP,
> > for instance:
> 
> Doesn't this mean we need to fix TFTP instead of patching the Ethernet
> driver? I'm sure the issue is present for all Ethernet drivers as none
> of them are checking if Ethernet is stopped. Just that most of them
> don't print any error message so it goes unnoticed.

Thanks for looking into this, your proposal makes sense to me!
I'll rework!

Best regards,
-- 
Alexander Sverdlin
Siemens AG
www.siemens.com