On Friday 21 March 2008, Rafael J. Wysocki wrote:
> On Friday, 21 of March 2008, David Brownell wrote:
> > All it does is implement the rules that have been defined for
> > ages now: most of the pm_message_t transitions should not
> > change device power states.
>
> But they do at the moment, so you're going to change how the code
> currently works on a quite large scale.
Are you arguing that this bug "should not be fixed"?
Or are you arguing that there's something wrong with
the rules, so that they "should not be followed"?
Or something else?
> > > I'd make it return PCI_D3hot if platform_pci_choose_state()
> > > is undefined or fails
> >
> > See above: this implements the current rules, which say
> > that in most cases devices shoudn't change powerstates.
>
> Okay, say you're changing pci_choose_state() to follow the
> documentation.
Not just documentation -- the current architecture of the
whole suspend process. Surely it's essential not to have
bugs like those at the core of that process?
If that process is broken, but this PCI bug hides it, we'll
be having a boatload of unexpected trouble in a while...
> Are you sure, however, that it won't cause any regressions to appear?
No more than any other bugfix turning up other latent bugs.
That's why we have a process which lets fixes cook for a while
before they merge.
> > + switch (mesg.event) {
> > case PM_EVENT_SUSPEND:
> > case PM_EVENT_HIBERNATE:
> > - return PCI_D3hot;
> > + /* NOTE: platform_pci_choose_state() should only return
> > + * states where wakeup won't work if
> > + * - !device_may_wakeup(&dev->dev), or
> > + * - dev can't wake from the target system state
> > + */
> > + if (platform_pci_choose_state) {
> > + ret = platform_pci_choose_state(dev, mesg);
> > + if (ret == PCI_POWER_ERROR)
> > + ret = PCI_D3hot;
> > + else if ((ret == PCI_D1 || ret == PCI_D2)
> > + && pci_no_d1d2(dev))
> > + ret = PCI_D3hot;
> > + break;
> > + }
> > + /* D3hot works, but may be suboptimal */
> > + ret = PCI_D3hot;
> > + break;
>
> I would do:
>
> + if (platform_pci_choose_state) {
> + ret = platform_pci_choose_state(dev, mesg);
> + if (ret == PCI_POWER_ERROR || (pci_no_d1d2(dev)
> + && (ret == PCI_D1 || ret == PCI_D2)))
That's ... phenomenally ugly and incomprehensible! It hides
almost the entire structure of the conditionals.
> + ret = PCI_D3hot;
> + } else {
> + /* D3hot works, but may be suboptimal */
> + ret = PCI_D3hot;
> + }
Extra/pointless brackets after "else" ... so you think the quickie
fix should have removed that first "break"? Simpler to just have
said so.
> + break;
======== CUT HERE
Clean up pci_choose_state():
- pci_choose_state() should only return PCI_D0, unless the system is
entering a suspend (or hibernate) system state.
- Only use platform_pci_choose_state() when entering a suspend
state ... and avoid PCI_D1 and PCI_D2 when appropriate.
- Corrrect kerneldoc.
Note that for now only ACPI provides platform_pci_choose_state(), so
this could be a minor change in behavior on some non-PC systems: it
avoids D3 except in the final stage of hibernation.
Signed-off-by: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/pci/pci.c | 53 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 30 insertions(+), 23 deletions(-)
--- g26.orig/drivers/pci/pci.c 2008-03-22 10:25:48.000000000 -0700
+++ g26/drivers/pci/pci.c 2008-03-22 10:44:28.000000000 -0700
@@ -523,46 +523,53 @@ pci_set_power_state(struct pci_dev *dev,
}
pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
-
+
/**
* pci_choose_state - Choose the power state of a PCI device
* @dev: PCI device to be suspended
- * @state: target sleep state for the whole system. This is the value
- * that is passed to suspend() function.
+ * @mesg: value passed to suspend() function.
*
* Returns PCI power state suitable for given device and given system
- * message.
+ * power state transition.
*/
-
-pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
+pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
{
pci_power_t ret;
+ /* PCI legacy PM? */
if (!pci_find_capability(dev, PCI_CAP_ID_PM))
return PCI_D0;
- if (platform_pci_choose_state) {
- ret = platform_pci_choose_state(dev, state);
- if (ret != PCI_POWER_ERROR)
- return ret;
- }
-
- switch (state.event) {
- case PM_EVENT_ON:
- return PCI_D0;
- case PM_EVENT_FREEZE:
- case PM_EVENT_PRETHAW:
- /* REVISIT both freeze and pre-thaw "should" use D0 */
+ switch (mesg.event) {
case PM_EVENT_SUSPEND:
case PM_EVENT_HIBERNATE:
- return PCI_D3hot;
+ /*
+ * D3hot works on all devices, but may be suboptimal on
+ * a given system. Factors include wakeups, power use,
+ * the optional D3hot->D0 reset, latency, and more.
+ *
+ * If there's a platform_pci_choose_state(), it could
+ * provide better answers for this system. It should
+ * only return states where wakeup won't work if:
+ * - !device_may_wakeup(&dev->dev), or
+ * - dev can't wake from the target system state
+ */
+ ret = PCI_D3hot;
+ if (platform_pci_choose_state) {
+ ret = platform_pci_choose_state(dev, mesg);
+ if (ret == PCI_POWER_ERROR)
+ ret = PCI_D3hot;
+ else if ((ret == PCI_D1 || ret == PCI_D2)
+ && pci_no_d1d2(dev))
+ ret = PCI_D3hot;
+ }
+ break;
default:
- printk("Unrecognized suspend event %d\n", state.event);
- BUG();
+ ret = PCI_D0;
+ break;
}
- return PCI_D0;
+ return ret;
}
-
EXPORT_SYMBOL(pci_choose_state);
static int pci_save_pcie_state(struct pci_dev *dev)
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
