netdev - Re: question about tulip/winbond-840.c

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:	Thu, 2 Jan 2014 07:37:54 -0800
From:	Grant Grundler <grantgrundler@...il.com>
To:	Julia Lawall <julia.lawall@...6.fr>
Cc:	Grant Grundler <grundler@...isc-linux.org>,
	"open list:TULIP NETWORK DRI..." <netdev@...r.kernel.org>
Subject: Re: question about tulip/winbond-840.c

On Thu, Jan 2, 2014 at 6:24 AM, Julia Lawall <julia.lawall@...6.fr> wrote:
> I don't know if you want to bother about this, because it is very old code
> and is apparently not hurting anyone, but I don't see the point of the
> call to pci_enable_device in the function w840_resume.

I believe the call to pci_enable_device is required:
    http://lxr.free-electrons.com/source/Documentation/PCI/pci.txt

254 3.1 Enable the PCI device
255 ~~~~~~~~~~~~~~~~~~~~~~~~~
256 Before touching any device registers, the driver needs to enable
257 the PCI device by calling pci_enable_device(). This will:
258         o wake up the device if it was in suspended state,
259         o allocate I/O and memory regions of the device (if BIOS did not),
260         o allocate an IRQ (if BIOS did not).

> The driver had a
> call to pci_enable_device in its probe function, but it contains no call
> to pci_disable_device, unlike some other tulip drivers.

I think the bug is the pci_disable_device is not called. The driver
should call pci_disable_device in suspend function and on failure path
in the resume function. I've never looked at this code before and I
believe has existed before PCI API was "mature". That's probably why
it's not using the API correctly.

>  Perhaps the call has a small functionality,

Yes - assuming pci.txt isn't stale. :)

> but it seems like it can at least never return
> a value other than 0, because the result of
> atomic_inc_return(&dev->enable_cnt) in pci_enable_device_flags should
> be greater than 1.

Off hand I don't recall..pci_enable_device used to setup alot more
stuff including power state and many of those could fail.

>
> julia
--
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