netdev - Re: [PATCH] net/fec: infinite spin on sirq-net-tx on real-time

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1328522191.2220.9.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
Date:	Mon, 06 Feb 2012 10:56:31 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Hector Palacios <hector.palacios@...i.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, shawn.guo@...aro.org,
	jgq516@...il.com, rostedt@...dmis.org, tim.sander@....com,
	u.kleine-koenig@...gutronix.de, tglx@...utronix.de,
	Zeng Zhaoming <b32542@...escale.com>,
	Frank Li <Frank.Li@...escale.com>
Subject: Re: [PATCH] net/fec: infinite spin on sirq-net-tx on real-time

Le lundi 06 février 2012 à 10:33 +0100, Hector Palacios a écrit :
> If softirqd is a real time task, an inifinite spin is hit
> if the FEC driver tries to send a packet before the autonegotiation
> with the PHY has completed.
> 
> This was seen when booting the platform with DHCP on. The driver
> sends the DHCP request before the PHY has completed autonegotiation.
> As a consequence, the driver's dev_hard_start_xmit returns NETDEV_TX_BUSY.
> NETDEV_TX_BUSY is part of NET_TX_MASK thus the packet is requeued (the
> skb->next = nskb) in dev_hard_start_xmit(). And the NETDEV_TX_BUSY is
> passed back to sch_derect_xmit() which calls dev_requeue_skb() which
> then calls __netif_schedule(q) which will call __netif_reschedule(q)
> which will then do raise_softirq_irqoff(NET_TX_SOFTIRQ).
> 
> Thus, as soon as ksoftirq exits this routine, it will restart the
> process over again. As the fec driver never finished with its
> negotiations, the process starts over again and we never move forward.
> 
> Newsgroup reference: linux-rt-users http://www.spinics.net/lists/linux-rt-users/msg07551.html
> 
> Signed-off-by: Zeng Zhaoming <b32542@...escale.com>
> Signed-off-by: Frank Li <Frank.Li@...escale.com>
> Signed-off-by: Hector Palacios <hector.palacios@...i.com>
> ---
>  drivers/net/ethernet/freescale/fec.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index c136230..3fa7d3b 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -284,6 +284,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  	if (!fep->link) {
>  		/* Link is down or autonegotiation is in progress. */
> +		netif_stop_queue(dev);

This seems odd.

IMHO, NETDEV_TX_BUSY should be avoided as much as possible.

Its part of the old driver API.

If queue was stopped, you would not have to test fep->link at all in
fast path, and Qdisc would not have to requeue a packet eventually.

>  		return NETDEV_TX_BUSY;
>  	}
>  
> @@ -530,6 +531,7 @@ fec_stop(struct net_device *ndev)
>  	udelay(10);
>  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>  	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
> +	fep->link = 0;


Why not call netif_stop_queue(dev) here ?

>  
>  	/* We have to keep ENET enabled to have MII interrupt stay working */
>  	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
> @@ -866,6 +868,7 @@ static void fec_enet_adjust_link(struct net_device *ndev)
>  	if (phy_dev->link) {
>  		if (fep->full_duplex != phy_dev->duplex) {
>  			fec_restart(ndev, phy_dev->duplex);
> +			netif_wake_queue(dev);
>  			status_change = 1;
>  		}
>  	}


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html