netdev - Re: [E1000-devel] [PATCH net-2.6.24] e100: fix driver init lockup on e100

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]

Date:	Tue, 28 Aug 2007 09:22:36 +0100
From:	James Chapman <jchapman@...alix.com>
To:	David Miller <davem@...emloft.net>
CC:	auke-jan.h.kok@...el.com, netdev@...r.kernel.org,
	e1000-devel@...ts.sourceforge.net
Subject: Re: [E1000-devel] [PATCH net-2.6.24] e100: fix driver init lockup
 on e100_up()

David Miller wrote:
> From: James Chapman <jchapman@...alix.com>
> Date: Mon, 27 Aug 2007 22:03:15 +0100
> 
>> Kok, Auke wrote:
>>> James Chapman wrote:
>>>>      nic = netdev_priv(netdev);
>>>> -    netif_napi_add(netdev, &nic->napi, e100_poll, E100_NAPI_WEIGHT);
>>>>      nic->netdev = netdev;
>>>>      nic->pdev = pdev;
>>>>      nic->msg_enable = (1 << debug) - 1;
>>>>      pci_set_drvdata(pdev, netdev);
>>>> +    netif_napi_add(netdev, &nic->napi, e100_poll, E100_NAPI_WEIGHT);
>>>> +    napi_disable(&nic->napi);
>>> Just wondering, could we even reverse this order? IOW disable NAPI 
>>> first, then add it ?
>> I think the order shouldn't matter. DaveM?
> 
> It doesn't matter.
> 
> I'm beginning to think maybe we should do an implicit napi_disable()
> in netif_napi_add(), then it's easier for drivers to play nice.

I like this idea. A quick survey of other NAPI drivers doesn't show any 
that do an explicit napi_disable() at init, though they might do so from 
internal functions. Each driver would need to be examined to check that 
it won't hang the same way as e100, so adding the call to 
netif_napi_add() would just be easier.

> On open you do napi_enable(), in close you do napi_disable().
> That's it.
> 
> And anywhere else in your driver that you have to napi_disable()
> (suspend, recovering from hardware errors, etc.) you must be sure to
> do the associated napi_enable() later on in order to keep things
> balanced.

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

-
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