On Thursday, 21 of February 2008, Linus Torvalds wrote:
quoted text
> 
> On Thu, 21 Feb 2008, Rafael J. Wysocki wrote:
> > 
> > > Secondly, the one that people should use ("pci_choose_state()") doesn't 
> > > actually do what you claim it does. It does all kinds of wrong things, and 
> > > doesn't even take the target state into account at all. So look again.
> > 
> > Well, if platform_pci_choose_state() is defined, pci_choose_state() returns
> > its result and on ACPI systems that points to acpi_pci_choose_state(), so in
> > fact it does what I said (apart from the error path). 
> 
> Did you check closer?

Yes, I did.

quoted text
> I repeat: acpi_pci_choose_state() (when called from pci_choose_state()) 
> doesn't even look at the target 'state'. It just blindly assumes that you 
> want the deepest sleep-state you can have.

acpi_pm_device_sleep_state() (that is called by acpi_pci_choose_state())
takes the target state directly from the ACPI layer.

We just want to get rid of the argument passed to ->suspend() eventually, but
there may be many _suspend_ states available (eg. "mem" and "standby") and
for each of them there may be different constraints on the device's state.  We
have to tell the driver which device states are possible in the target system
sleep state.  Right now we arbitrarily choose the one with the lowest power
usage - for given target system sleep state.

quoted text
> Which happens to be correct for normal suspend, but means that if you want 
> to test other states (through '/sys/devices/.../power'), that sounds 
> broken.

This interface is not available any more (ie. there's only "wakeup" in
/sys/devices/.../power).

quoted text
> I didn't check any closer, but go check it yourself. The short and sweet: 
> acpi_pci_choose_state() totally ignores its 'state' argument. Do you 
> really think that's correct?

Yes, I do.

quoted text
> But yes, "pci_choose_state()' effectively does that too, apart from
> PM_EVENT_ON, which is never used. 
> 
> (But the whole and only point of pci_choose_state() was to do the 
> PM_EVENT_FREEZE thing differently, which it doesn't do, so I think the 
> real issue here is that the interface is really rather mis-designed)

You're wrong, sorry.  With PM_EVENT_FREEZE it wouldn't even be necessary.
It's there, because potentially there are many possibilities with
PM_EVENT_SUSPEND and in fact it shouldn't even be used with
PM_EVENT_FREEZE.

All of this is more or less orthogonal to the issue at hand, which boils down
to the fact that we use the _suspend_ callbacks for hibernation and we
shouldn't be doing that.

Thanks,
Rafael
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/