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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <346b9fec-f0a4-70d0-529c-32659490df03@csgroup.eu>
Date:   Tue, 12 Apr 2022 12:44:04 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Mans Rullgard <mans@...sr.com>,
        Pantelis Antoniou <pantelis.antoniou@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Vitaly Bordug <vbordug@...mvista.com>,
        Dan Malek <dan@...eddededge.com>,
        Joakim Tjernlund <joakim.tjernlund@...entis.se>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH] net: fs_enet: fix tx error handling



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@...sr.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)
>