On Saturday, 22 of March 2008, David Brownell wrote:
> 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?
Something else. As I said before somewhere, I'd introduce a new function
imilar to pci_choose_state(), but with the semantics you want it to have.
Then, I'd make drivers switch to it and remove the original pci_choose_state()
when it's no longer used. Clean and safe.
> > > > 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...
I'm not against fixing bugs, but let's do that without giving users and testers
a hard time if possible.
> > 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.
In fact if a bugfix turns up any later bugs, then we should fix all of the
later bugs along with that bugfix, preferably in one series of patches.
> > > + 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.
I obviously disagree with that. What exactly is incomprehensible in it?
"Return D3hot if an error has been returned or a state we can't use has been
returned". Quite simple, no?
> > + ret = PCI_D3hot;
> > + } else {
> > + /* D3hot works, but may be suboptimal */
> > + ret = PCI_D3hot;
> > + }
>
> Extra/pointless brackets after "else" ...
Please have a look at Chapter 3 in Documentation/CodingStyle (just before 3.1).
> 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;
And so this reads "If an error is has been returned, return D3hot. And by
the way, if D1 or D2 has been returned and we can't use any of them,
return D3hot". I really don't see why it's better than just one condition, but
whatever.
> + }
> + 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)
> --
Thanks,
Rafael
--
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
