Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

David Brownell
2008-03-20T23:03:05Z

> > + switch (mesg.event) {
> > case PM_EVENT_SUSPEND:
> > + /* 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;
> > + }
> > + /* FALLTHROUGH ... D3hot works, but may be suboptimal */
> > case PM_EVENT_HIBERNATE:
> > - return PCI_D3hot;
> > + ret = PCI_D3hot;
>
> This is clearly wrong. It should do the same as for suspend here (_S4D may be
> defined and we should take it into account if it is).

So you're saying the original code was wrong, and this patch should
change that behavior too?

> > + 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);
>
> I really don't think pci_choose_state() should take the state argument at all.

There is no "state" argument. It's a pm_message_t, which does
not package the target state.

A patch to change the calling syntax for this would necessarily
be a different patch... and would need to change the callers too.

- Dave