2006-02-18 02:04:05

by Patrick Mochel

Subject: [PATCH 5/5] [pci pm] Make pci_choose_state() use the real device state requested

- Look at pm_message_t::state (along with the message event type) to
determine the PCI power state to return. This allows all of the PCI
power states to be used with this helper (and with the sysfs runtime
PM interface).

[ Previously, the returned value was limited to PCI_D0 or PCI_D3hot
depending on whether PM_EVENT_ON or PM_EVENT_{FREEZE,SUSPEND} were
specified. ]

- Print an error w/ device info when we get an invalid state; use
WARN_ON() instead of BUG(), so we don't needlessly crash a machine.

- Print state translation when debug is enabled.

Signed-off-by: Patrick Mochel <[email protected]>

---

drivers/pci/pci.c | 29 +++++++++++++++++++----------
1 files changed, 19 insertions(+), 10 deletions(-)

applies-to: 7b4d08b2816a4aefc2b9891091e6a790526ea15e
8a658ba724d1f1e71fc951745e091aa1afa4ff6f
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d2d1879..2f160aa 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -404,30 +404,39 @@ int (*platform_pci_choose_state)(struct
* message.
*/

-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 msg)
{
+ pci_power_t state = PCI_D3hot;
int ret;

if (!pci_find_capability(dev, PCI_CAP_ID_PM))
return PCI_D0;

if (platform_pci_choose_state) {
- ret = platform_pci_choose_state(dev, state);
+ ret = platform_pci_choose_state(dev, msg);
if (ret >= 0)
- state.event = ret;
+ msg.state = ret;
}

- switch (state.event) {
+ switch (msg.event) {
case PM_EVENT_ON:
- return PCI_D0;
+ state = PCI_D0;
+ break;
case PM_EVENT_FREEZE:
case PM_EVENT_SUSPEND:
- return PCI_D3hot;
+ if (msg.state && msg.state <= PCI_D3hot)
+ state = msg.state;
+ break;
default:
- printk("They asked me for state %d\n", state.event);
- BUG();
- }
- return PCI_D0;
+ dev_err(&dev->dev, "PCI: Invalid PM Event [Event %d] [State %u]\n",
+ msg.event, msg.state);
+ WARN_ON(1);
+ state = PCI_D0;
+ break;
+ }
+ dev_dbg(&dev->dev, "PCI: Translated Power Message %d/%u -> %u\n",
+ msg.event, msg.state, state);
+ return state;
}

EXPORT_SYMBOL(pci_choose_state);
---
0.99.9.GIT

2006-02-18 16:01:01

by Pavel Machek

Subject: Re: [linux-pm] [PATCH 5/5] [pci pm] Make pci_choose_state() use the real device state requested

Hi!

> case PM_EVENT_FREEZE:
> case PM_EVENT_SUSPEND:
> - return PCI_D3hot;
> + if (msg.state && msg.state <= PCI_D3hot)
> + state = msg.state;
> + break;
> default:

Silently ignores wrong value passed-in by user. Not nice...
Pavel

--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms

2006-02-20 00:10:00

by Patrick Mochel

Subject: Re: [linux-pm] [PATCH 5/5] [pci pm] Make pci_choose_state() use the real device state requested

On Sat, 18 Feb 2006, Pavel Machek wrote:

> Hi!
>
> > case PM_EVENT_FREEZE:
> > case PM_EVENT_SUSPEND:
> > - return PCI_D3hot;
> > + if (msg.state && msg.state <= PCI_D3hot)
> > + state = msg.state;
> > + break;
> > default:
>
> Silently ignores wrong value passed-in by user. Not nice...

The value is bounded, so anything passed above PCI_D3hot is translated to
PCI_D3hot.

[ That's the best we can do since we can't report an error nicely because
the pci_choose_state() interface was poorly designed. The function must be
called by the drivers, and returns a pci_power_t. It is not expected to
fail, and the argument is usually passed unchecked to
pci_set_power_state().

It would have been much safer to have pci_device_suspend() in
drivers/pci/pci-driver.c call pci_choose_state() to translate the value.
That could easily verify the user input, and it could just pass the
translated state to the drivers, which would have been compatible with the
previous API and not required a change to every single PCI driver.. ]

Thanks,


Pat