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 17:16:00 +0100 (CET)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Grant Grundler <grantgrundler@...il.com>
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, 2 Jan 2014, Grant Grundler wrote:

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

It does set up more things.  But it doesn't do much if the device has not
been disabled.
http://lxr.free-electrons.com/source/drivers/pci/pci.c#L1190

Looking across the entire Linux kernel, I see cases where there are
enables but no disables, and cases where there are disables and no
enables.  So I wouldn't really know whether a change is needed.

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