Just to wrap this up ... if this #3 isn't going to merge,
then #4 can't merge either (teaching USB to use that call
to talk to ACPI etc).
See below.
However, several of the other patches in that series should
still IMO go upstream... I think I saw only one of them get
into the ACPI tree.
On Saturday 22 March 2008, Rafael J. Wysocki wrote:
> 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.
The implementation of pci_choose_state() which first merged was
admittedly very broken. That bothered me a lot ... since it
didn't adhere to my original proposal for such a function.
So while there's a part of me that would rather see my original
proposal working, rather than this broken version which reused
that name ... I can certainly live with some *other* function
doing the right thing, instead.
> > > > > 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.
I'm opposed to making design bugs in the first place. I was
quite surprised to see so many functions got changed to use
the broken pci_choose_state() thing!
> > > 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.
Fine.
> > > > + 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?
I've learned over time that complex conditions are *VERY* easily
misunderstood. And that's pretty complex ... fortunately it has
no negation, but the previous version was simpler. It clearly
split out the error condition from the "no D1/D2 support", and
that latter condition was easier to see (since it wasn't tangled
up with any unrelated logic).
> > > + 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.
As I said: One big complex condition is harder to understand.
Particularly compared to two simple standalone conditions.
> > + }
> > + 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
