Re: [patch 2.6.21-git] pci_choose_state() works, does ACPI magic

[David Brownell <david-b@pacbell.net>]

On Monday 21 May 2007, Bjorn Helgaas wrote:
> On Wednesday 09 May 2007 01:22:29 pm David Brownell wrote:
> >  static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> >  {
> > ...
> > +	switch (state) {
> > +	case PCI_D0:
> > +	case PCI_D1:
> > +	case PCI_D2:
> > +	case PCI_D3hot:
> > +	case PCI_D3cold:
> > +		return acpi_bus_set_power(handle, state_conv[state]);
> > +	}
> > +
> > +	return -EINVAL;

... i.e. a cleaned-up version of the existing code ...

> >  }
> 
> Did you account for the fact that PCI works on a per-function basis,
> but ACPI power management works on a per-slot basis?

This is a per-function method though.  The ACPI code should
be able to keep that straight ... simple refcounting on the
power resources would suffice, assuming it goes the route of
managing power resources rather than just invoking the methods
associated with each device.


> We had a bug[1] a while back where e1000 would suspend a device and
> call pci_power_state(), which used ACPI to turn off the slot.  The
> only problem was that this was a dual-port card and the other port
> was still active when the lights went out.

Sounds like an ACPI bug to me:  not properly tracking the users
of a given PCI power resource.  I can't see that bug without
getting a Novell login -- you might want post the report rather
than its URL.

 
> Maybe you deal with this already; it just wasn't obvious to me from
> a quick glance through your patch.

Didn't touch it ... pre-existing bugs shouldn't have been affected.

The only behavioral change in that particular method should have been
safer handling of an out-of-range parameter.

- Dave
 

> Bjorn
> 
> 
> [1] https://bugzilla.novell.com/show_bug.cgi?id=162320
>