Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday, 9 of May 2008, Jesse Barnes wrote:
> > > > > I thought about that too.  I'd like to know what the other people
> > > > > think, though.
> > > >
> > > > The point of this isn't at all clear.
> > > >
> > > > Is this routine meant to be called during a hibernation
> > > > transition?
> > >
> > > Yes, it is.
> > >
> > > > Or is it just for suspend?
> > > >
> > > > And why would the return value ever be anything other than D3_hot?  (Or
> > > > why would the driver ever want to put a device in a different state?)
> > >
> > > In principle, the driver may want to put the device into a state having
> > > shorter wake up latency than D3_hot.
> > >
> > > > AFAICS, the only reason would be because platform_pci_choose_state()
> > > > suggested something else.  In which case there's no need for the
> > > > "policy" argument.
> > >
> > > There is a need in two cases:
> > > - if platform_pci_choose_state() is not defined (it only is defined for
> > > ACPI systems at the moment),
> > > - if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it
> > > cannot handle the device.
> > >
> > > I agree with Pavel that the driver could pass a "fallback state" as a
> > > second argument to be used in case the platform cannot provide it with
> > > one.
> >
> > Modified patch follows.
> 
> So why not make platform_pci_choose_state do:
> + pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t 
> state)
> + {
> +       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> +               return state;
> + }
> 
> instead?  Then in the PCI core we would assign either 
> platform_pci_choose_state to acpi_pci_choose_state or noacpi_pci_choose_state 

Good idea.

> (though that's a bad name).

Does generic_pci_choose_state() sound better?

> But really, since drivers should probably know what power state to put their 
> devices in for suspend & hibernate, maybe on non-ACPI systems the function 
> should just return an error and the driver can choose...

That's one possibility too, but in that case many drivers will do

state = pci_preferred_state(dev);
if (state == PCI_POWER_ERROR)
	state = something;

It's just shorter to write

state = pci_preferred_state(dev, something);

Thanks,
Rafael