[RFC][PATCH] net: fs_enet: fix tx error handling

[RFC][PATCH] net: fs_enet: fix tx error handling

[Mans Rullgard]

In some cases, the TXE flag is apparently set without any error
indication in the buffer descriptor status. When this happens, tx
stalls until the tx_restart() function is called via the device
watchdog which can take a long time.

To fix this, check for TXE in the napi poll function and trigger a
tx_restart() call as for errors reported in the buffer descriptor.

This change makes the FCC based Ethernet controller on MPC82xx devices
usable. It probably breaks the other modes (FEC, SCC) which I have no
way of testing.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 .../ethernet/freescale/fs_enet/fs_enet-main.c | 47 +++++++------------
 .../net/ethernet/freescale/fs_enet/mac-fcc.c  |  2 +-
 2 files changed, 19 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index 78e008b81374..4276becd07cf 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -94,14 +94,22 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 	int curidx;
 	int dirtyidx, do_wake, do_restart;
 	int tx_left = TX_RING_SIZE;
+	u32 int_events;
 
 	spin_lock(&fep->tx_lock);
 	bdp = fep->dirty_tx;
+	do_wake = do_restart = 0;
+
+	int_events = (*fep->ops->get_int_events)(dev);
+
+	if (int_events & fep->ev_err) {
+		(*fep->ops->ev_error)(dev, int_events);
+		do_restart = 1;
+	}
 
 	/* clear status bits for napi*/
 	(*fep->ops->napi_clear_event)(dev);
 
-	do_wake = do_restart = 0;
 	while (((sc = CBDR_SC(bdp)) & BD_ENET_TX_READY) == 0 && tx_left) {
 		dirtyidx = bdp - fep->tx_bd_base;
 
@@ -318,43 +326,24 @@ fs_enet_interrupt(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
 	struct fs_enet_private *fep;
-	const struct fs_platform_info *fpi;
 	u32 int_events;
-	u32 int_clr_events;
-	int nr, napi_ok;
-	int handled;
 
 	fep = netdev_priv(dev);
-	fpi = fep->fpi;
 
-	nr = 0;
-	while ((int_events = (*fep->ops->get_int_events)(dev)) != 0) {
-		nr++;
+	int_events = (*fep->ops->get_int_events)(dev);
+	if (!int_events)
+		return IRQ_NONE;
 
-		int_clr_events = int_events;
-		int_clr_events &= ~fep->ev_napi;
+	int_events &= ~fep->ev_napi;
 
-		(*fep->ops->clear_int_events)(dev, int_clr_events);
-
-		if (int_events & fep->ev_err)
-			(*fep->ops->ev_error)(dev, int_events);
-
-		if (int_events & fep->ev) {
-			napi_ok = napi_schedule_prep(&fep->napi);
-
-			(*fep->ops->napi_disable)(dev);
-			(*fep->ops->clear_int_events)(dev, fep->ev_napi);
-
-			/* NOTE: it is possible for FCCs in NAPI mode    */
-			/* to submit a spurious interrupt while in poll  */
-			if (napi_ok)
-				__napi_schedule(&fep->napi);
-		}
+	(*fep->ops->clear_int_events)(dev, int_events);
 
+	if (napi_schedule_prep(&fep->napi)) {
+		(*fep->ops->napi_disable)(dev);
+		__napi_schedule(&fep->napi);
 	}
 
-	handled = nr > 0;
-	return IRQ_RETVAL(handled);
+	return IRQ_HANDLED;
 }
 
 void fs_init_bds(struct net_device *dev)
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
index b47490be872c..66c8f82a8333 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
@@ -124,7 +124,7 @@ static int do_pd_setup(struct fs_enet_private *fep)
 	return ret;
 }
 
-#define FCC_NAPI_EVENT_MSK	(FCC_ENET_RXF | FCC_ENET_RXB | FCC_ENET_TXB)
+#define FCC_NAPI_EVENT_MSK	(FCC_ENET_RXF | FCC_ENET_RXB | FCC_ENET_TXB | FCC_ENET_TXE)
 #define FCC_EVENT		(FCC_ENET_RXF | FCC_ENET_TXB)
 #define FCC_ERR_EVENT_MSK	(FCC_ENET_TXE)
 
-- 
2.35.1

Re: [RFC][PATCH] net: fs_enet: fix tx error handling

[Christophe Leroy]

Le 17/03/2022 à 16:38, Mans Rullgard a écrit :
> In some cases, the TXE flag is apparently set without any error
> indication in the buffer descriptor status. When this happens, tx
> stalls until the tx_restart() function is called via the device
> watchdog which can take a long time.

Is there an errata from NXP about this ?

Did you report the issue to them ? What feedback did you get ?

> 
> To fix this, check for TXE in the napi poll function and trigger a
> tx_restart() call as for errors reported in the buffer descriptor.

I'm not sure to understand. You change seems to do a lot more than that. 
Especially it changes to location of the handling of errors. Previously 
errors where handled in interrupt routine. Now it's handled in the napi 
poll routine. You have to explain all that.

> 
> This change makes the FCC based Ethernet controller on MPC82xx devices
> usable. It probably breaks the other modes (FEC, SCC) which I have no
> way of testing.

You should at least change mac-scc.h and mac-fec.h to match the changes 
you did in mac-fcc.h

This would allow me to at least test the FEC one.

> 
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>   .../ethernet/freescale/fs_enet/fs_enet-main.c | 47 +++++++------------
>   .../net/ethernet/freescale/fs_enet/mac-fcc.c  |  2 +-
>   2 files changed, 19 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> index 78e008b81374..4276becd07cf 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> @@ -94,14 +94,22 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   	int curidx;
>   	int dirtyidx, do_wake, do_restart;
>   	int tx_left = TX_RING_SIZE;
> +	u32 int_events;
>   
>   	spin_lock(&fep->tx_lock);
>   	bdp = fep->dirty_tx;
> +	do_wake = do_restart = 0;
> +
> +	int_events = (*fep->ops->get_int_events)(dev);
> +
> +	if (int_events & fep->ev_err) {
> +		(*fep->ops->ev_error)(dev, int_events);
> +		do_restart = 1;
> +	}
>   
>   	/* clear status bits for napi*/
>   	(*fep->ops->napi_clear_event)(dev);
>   
> -	do_wake = do_restart = 0;
>   	while (((sc = CBDR_SC(bdp)) & BD_ENET_TX_READY) == 0 && tx_left) {
>   		dirtyidx = bdp - fep->tx_bd_base;
>   
> @@ -318,43 +326,24 @@ fs_enet_interrupt(int irq, void *dev_id)
>   {
>   	struct net_device *dev = dev_id;
>   	struct fs_enet_private *fep;
> -	const struct fs_platform_info *fpi;
>   	u32 int_events;
> -	u32 int_clr_events;
> -	int nr, napi_ok;
> -	int handled;
>   
>   	fep = netdev_priv(dev);
> -	fpi = fep->fpi;
>   
> -	nr = 0;
> -	while ((int_events = (*fep->ops->get_int_events)(dev)) != 0) {
> -		nr++;
> +	int_events = (*fep->ops->get_int_events)(dev);
> +	if (!int_events)
> +		return IRQ_NONE;
>   
> -		int_clr_events = int_events;
> -		int_clr_events &= ~fep->ev_napi;
> +	int_events &= ~fep->ev_napi;
>   
> -		(*fep->ops->clear_int_events)(dev, int_clr_events);
> -
> -		if (int_events & fep->ev_err)
> -			(*fep->ops->ev_error)(dev, int_events);
> -
> -		if (int_events & fep->ev) {
> -			napi_ok = napi_schedule_prep(&fep->napi);
> -
> -			(*fep->ops->napi_disable)(dev);
> -			(*fep->ops->clear_int_events)(dev, fep->ev_napi);
> -
> -			/* NOTE: it is possible for FCCs in NAPI mode    */
> -			/* to submit a spurious interrupt while in poll  */
> -			if (napi_ok)
> -				__napi_schedule(&fep->napi);
> -		}
> +	(*fep->ops->clear_int_events)(dev, int_events);
>   
> +	if (napi_schedule_prep(&fep->napi)) {
> +		(*fep->ops->napi_disable)(dev);
> +		__napi_schedule(&fep->napi);
>   	}
>   
> -	handled = nr > 0;
> -	return IRQ_RETVAL(handled);
> +	return IRQ_HANDLED;
>   }
>   
>   void fs_init_bds(struct net_device *dev)
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> index b47490be872c..66c8f82a8333 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> @@ -124,7 +124,7 @@ static int do_pd_setup(struct fs_enet_private *fep)
>   	return ret;
>   }
>   
> -#define FCC_NAPI_EVENT_MSK	(FCC_ENET_RXF | FCC_ENET_RXB | FCC_ENET_TXB)
> +#define FCC_NAPI_EVENT_MSK	(FCC_ENET_RXF | FCC_ENET_RXB | FCC_ENET_TXB | FCC_ENET_TXE)
>   #define FCC_EVENT		(FCC_ENET_RXF | FCC_ENET_TXB)
>   #define FCC_ERR_EVENT_MSK	(FCC_ENET_TXE)
>