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: <4F2FB39B.9010205@digi.com>
Date:	Mon, 6 Feb 2012 12:03:55 +0100
From:	Hector Palacios <hector.palacios@...i.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"shawn.guo@...aro.org" <shawn.guo@...aro.org>,
	"jgq516@...il.com" <jgq516@...il.com>,
	"rostedt@...dmis.org" <rostedt@...dmis.org>,
	"tim.sander@....com" <tim.sander@....com>,
	"u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
	"tglx@...utronix.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

On 02/06/2012 10:56 AM, Eric Dumazet wrote:
> 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;
>>   		}
>>   	}
>
>

I'm no network driver expert so I'll leave it up to others to comment. I just forward 
ported a patch I came across in Freescale's BSP which solves the problem in mainline 
and in RT.
-- 
Héctor Palacios
--
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