question about tulip/winbond-840.c

question about tulip/winbond-840.c

[Julia Lawall]

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.  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.  Perhaps the call
has a small functionality, 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.

julia

Re: question about tulip/winbond-840.c

[Grant Grundler]

On Thu, Jan 2, 2014 at 6:24 AM, Julia Lawall <julia.lawall@lip6.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

Re: question about tulip/winbond-840.c

[Julia Lawall]

On Thu, 2 Jan 2014, Grant Grundler wrote:

> On Thu, Jan 2, 2014 at 6:24 AM, Julia Lawall <julia.lawall@lip6.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

Re: question about tulip/winbond-840.c

[Grant Grundler]

On Thu, Jan 2, 2014 at 8:16 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
...
> 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

Sure...but this driver _should_ disable on suspend. But who knows...my
guess is that if anyone has tried suspending this device, it works
anyway on resume.

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

I think both cases are wrong. I'm pretty sure enable/disable should be
symmetric for both "init_one/remove_one"  and suspend/resume.  I'm not
aware of any reason why they would not be.

cheers,
grant