Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Alan Stern wrote:
> On Wed, 7 May 2008, Rafael J. Wysocki wrote:
> 
> > On Wednesday, 7 of May 2008, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > The new suspend and hibernation callbacks introduced with
> > > > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > > > pm_message_t argument, so the drivers using them will not be able
> > > > to use pci_choose_state() in its present form.  For this reason,
> > > > introduce the new function pci_preferred_state() playing the role
> > > > of pci_choose_state(), but taking only a pointer to the device
> > > > object.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > ---
> > > >  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
> > > >  include/linux/pci.h |    1 +
> > > >  include/linux/pm.h  |   10 ++++++++++
> > > >  3 files changed, 43 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/drivers/pci/pci.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/pci/pci.c
> > > > +++ linux-2.6/drivers/pci/pci.c
> > > > @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
> > > >  }
> > > >  
> > > >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > > > - 
> > > > +
> > > > +/**
> > > > + * pci_preferred_state - Choose the preferred power state of a PCI device
> > > > + * @dev: PCI device to be put into the low power state
> > > > + * @sp: Information aboutabout what the driver would prefer to do with
> > > > + *	the device if there were no platform-implemeted policy.
> > > > + *
> > > > + * Returns PCI power state suitable for given device and given suspend policy.
> > > > + * The policy, however, is only used if platform_pci_choose_state() fails or is
> > > > + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> > > > + * implements the right policy.
> > > > + */
> > > > +
> > > > +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> > > > +{
> > > > +	pci_power_t ret;
> > > > +
> > > > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > > +		return PCI_D0;
> > > > +
> > > > +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> > > > +	if (platform_pci_choose_state) {
> > > > +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> > > > +
> > > > +		if (platform_ret != PCI_POWER_ERROR)
> > > > +			ret = platform_ret;
> > > > +	}
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +EXPORT_SYMBOL(pci_preferred_state);
> > > 
> > > I don't get it. How is driver supposed to use this? How does the
> > > driver decide between SP_TURN_OFF and SP_TURN_ON?
> > > 
> > > ...and it seems to be clearer to just inline this in the driver... or
> > > pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
> > > define...
> > 
> > I thought about that too.  I'd like to know what the other people think,
> > though.
> 
> The point of this isn't at all clear.
> 
> Is this routine meant to be called during a hibernation 
> transition?

Yes, it is.

> Or is it just for suspend? 
> 
> And why would the return value ever be anything other than D3_hot?  (Or 
> why would the driver ever want to put a device in a different state?)

In principle, the driver may want to put the device into a state having shorter
wake up latency than D3_hot.

> AFAICS, the only reason would be because platform_pci_choose_state() 
> suggested something else.  In which case there's no need for the 
> "policy" argument.

There is a need in two cases:
- if platform_pci_choose_state() is not defined (it only is defined for ACPI
  systems at the moment),
- if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it cannot
  handle the device.

I agree with Pavel that the driver could pass a "fallback state" as a second
argument to be used in case the platform cannot provide it with one.

Thanks,
Rafael

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Rafael J. Wysocki wrote:
> On Wednesday, 7 of May 2008, Alan Stern wrote:
> > On Wed, 7 May 2008, Rafael J. Wysocki wrote:
> > 
> > > On Wednesday, 7 of May 2008, Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > 
> > > > > The new suspend and hibernation callbacks introduced with
> > > > > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > > > > pm_message_t argument, so the drivers using them will not be able
> > > > > to use pci_choose_state() in its present form.  For this reason,
> > > > > introduce the new function pci_preferred_state() playing the role
> > > > > of pci_choose_state(), but taking only a pointer to the device
> > > > > object.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > ---
> > > > >  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
> > > > >  include/linux/pci.h |    1 +
> > > > >  include/linux/pm.h  |   10 ++++++++++
> > > > >  3 files changed, 43 insertions(+), 1 deletion(-)
> > > > > 
> > > > > Index: linux-2.6/drivers/pci/pci.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/pci/pci.c
> > > > > +++ linux-2.6/drivers/pci/pci.c
> > > > > @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
> > > > >  }
> > > > >  
> > > > >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > > > > - 
> > > > > +
> > > > > +/**
> > > > > + * pci_preferred_state - Choose the preferred power state of a PCI device
> > > > > + * @dev: PCI device to be put into the low power state
> > > > > + * @sp: Information aboutabout what the driver would prefer to do with
> > > > > + *	the device if there were no platform-implemeted policy.
> > > > > + *
> > > > > + * Returns PCI power state suitable for given device and given suspend policy.
> > > > > + * The policy, however, is only used if platform_pci_choose_state() fails or is
> > > > > + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> > > > > + * implements the right policy.
> > > > > + */
> > > > > +
> > > > > +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> > > > > +{
> > > > > +	pci_power_t ret;
> > > > > +
> > > > > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > > > +		return PCI_D0;
> > > > > +
> > > > > +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> > > > > +	if (platform_pci_choose_state) {
> > > > > +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> > > > > +
> > > > > +		if (platform_ret != PCI_POWER_ERROR)
> > > > > +			ret = platform_ret;
> > > > > +	}
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +EXPORT_SYMBOL(pci_preferred_state);
> > > > 
> > > > I don't get it. How is driver supposed to use this? How does the
> > > > driver decide between SP_TURN_OFF and SP_TURN_ON?
> > > > 
> > > > ...and it seems to be clearer to just inline this in the driver... or
> > > > pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
> > > > define...
> > > 
> > > I thought about that too.  I'd like to know what the other people think,
> > > though.
> > 
> > The point of this isn't at all clear.
> > 
> > Is this routine meant to be called during a hibernation 
> > transition?
> 
> Yes, it is.
> 
> > Or is it just for suspend? 
> > 
> > And why would the return value ever be anything other than D3_hot?  (Or 
> > why would the driver ever want to put a device in a different state?)
> 
> In principle, the driver may want to put the device into a state having shorter
> wake up latency than D3_hot.
> 
> > AFAICS, the only reason would be because platform_pci_choose_state() 
> > suggested something else.  In which case there's no need for the 
> > "policy" argument.
> 
> There is a need in two cases:
> - if platform_pci_choose_state() is not defined (it only is defined for ACPI
>   systems at the moment),
> - if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it cannot
>   handle the device.
> 
> I agree with Pavel that the driver could pass a "fallback state" as a second
> argument to be used in case the platform cannot provide it with one.

Modified patch follows.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

The new suspend and hibernation callbacks introduced with
'struct pm_ops' and 'struct pm_ext_ops' do not take a
pm_message_t argument, so the drivers using them will not be able
to use pci_choose_state() in its present form.  For this reason,
introduce the new function pci_preferred_state() playing the role
of pci_choose_state(), but taking only the pointer to the device
object.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c   |   28 +++++++++++++++++++++++++++-
 include/linux/pci.h |    1 +
 2 files changed, 28 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -509,7 +509,33 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
- 
+
+/**
+ * pci_preferred_state - Choose the preferred power state of a PCI device
+ * @dev: PCI device to be put into the low power state
+ * @state: State to put the device into if the platform cannot handle it
+ *
+ * Returns PCI power state suitable for given device according to the platform.
+ * However, if platform_pci_choose_state() is not defined or returns
+ * PCI_POWER_ERROR, @state is returned.
+ */
+
+pci_power_t pci_preferred_state(struct pci_dev *dev, pci_power_t state)
+{
+	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+		return state;
+
+	if (platform_pci_choose_state) {
+		pci_power_t ret = platform_pci_choose_state(dev);
+
+		if (ret != PCI_POWER_ERROR)
+			state = ret;
+	}
+	return state;
+}
+
+EXPORT_SYMBOL(pci_preferred_state);
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -615,6 +615,7 @@ size_t pci_get_rom_size(void __iomem *ro
 int pci_save_state(struct pci_dev *dev);
 int pci_restore_state(struct pci_dev *dev);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+pci_power_t pci_preferred_state(struct pci_dev *dev, pci_power_t state);
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable);
 

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Jesse Barnes]

> > > > I thought about that too.  I'd like to know what the other people
> > > > think, though.
> > >
> > > The point of this isn't at all clear.
> > >
> > > Is this routine meant to be called during a hibernation
> > > transition?
> >
> > Yes, it is.
> >
> > > Or is it just for suspend?
> > >
> > > And why would the return value ever be anything other than D3_hot?  (Or
> > > why would the driver ever want to put a device in a different state?)
> >
> > In principle, the driver may want to put the device into a state having
> > shorter wake up latency than D3_hot.
> >
> > > AFAICS, the only reason would be because platform_pci_choose_state()
> > > suggested something else.  In which case there's no need for the
> > > "policy" argument.
> >
> > There is a need in two cases:
> > - if platform_pci_choose_state() is not defined (it only is defined for
> > ACPI systems at the moment),
> > - if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it
> > cannot handle the device.
> >
> > I agree with Pavel that the driver could pass a "fallback state" as a
> > second argument to be used in case the platform cannot provide it with
> > one.
>
> Modified patch follows.

So why not make platform_pci_choose_state do:
+ pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t 
state)
+ {
+       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+               return state;
+ }

instead?  Then in the PCI core we would assign either 
platform_pci_choose_state to acpi_pci_choose_state or noacpi_pci_choose_state 
(though that's a bad name).

But really, since drivers should probably know what power state to put their 
devices in for suspend & hibernate, maybe on non-ACPI systems the function 
should just return an error and the driver can choose...

Jesse

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Friday, 9 of May 2008, Jesse Barnes wrote:
> > > > > I thought about that too.  I'd like to know what the other people
> > > > > think, though.
> > > >
> > > > The point of this isn't at all clear.
> > > >
> > > > Is this routine meant to be called during a hibernation
> > > > transition?
> > >
> > > Yes, it is.
> > >
> > > > Or is it just for suspend?
> > > >
> > > > And why would the return value ever be anything other than D3_hot?  (Or
> > > > why would the driver ever want to put a device in a different state?)
> > >
> > > In principle, the driver may want to put the device into a state having
> > > shorter wake up latency than D3_hot.
> > >
> > > > AFAICS, the only reason would be because platform_pci_choose_state()
> > > > suggested something else.  In which case there's no need for the
> > > > "policy" argument.
> > >
> > > There is a need in two cases:
> > > - if platform_pci_choose_state() is not defined (it only is defined for
> > > ACPI systems at the moment),
> > > - if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it
> > > cannot handle the device.
> > >
> > > I agree with Pavel that the driver could pass a "fallback state" as a
> > > second argument to be used in case the platform cannot provide it with
> > > one.
> >
> > Modified patch follows.
> 
> So why not make platform_pci_choose_state do:
> + pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t 
> state)
> + {
> +       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> +               return state;
> + }
> 
> instead?  Then in the PCI core we would assign either 
> platform_pci_choose_state to acpi_pci_choose_state or noacpi_pci_choose_state 

Good idea.

> (though that's a bad name).

Does generic_pci_choose_state() sound better?

> But really, since drivers should probably know what power state to put their 
> devices in for suspend & hibernate, maybe on non-ACPI systems the function 
> should just return an error and the driver can choose...

That's one possibility too, but in that case many drivers will do

state = pci_preferred_state(dev);
if (state == PCI_POWER_ERROR)
	state = something;

It's just shorter to write

state = pci_preferred_state(dev, something);

Thanks,
Rafael

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Jesse Barnes]

On Friday, May 09, 2008 10:13 am Rafael J. Wysocki wrote:
> > So why not make platform_pci_choose_state do:
> > + pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t
> > state)
> > + {
> > +       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > +               return state;
> > + }
> >
> > instead?  Then in the PCI core we would assign either
> > platform_pci_choose_state to acpi_pci_choose_state or
> > noacpi_pci_choose_state
>
> Good idea.
>
> > (though that's a bad name).
>
> Does generic_pci_choose_state() sound better?

Yeah, that's better.

> > But really, since drivers should probably know what power state to put
> > their devices in for suspend & hibernate, maybe on non-ACPI systems the
> > function should just return an error and the driver can choose...
>
> That's one possibility too, but in that case many drivers will do
>
> state = pci_preferred_state(dev);
> if (state == PCI_POWER_ERROR)
> 	state = something;
>
> It's just shorter to write
>
> state = pci_preferred_state(dev, something);

But really that's the idea, since if the core doesn't know what state your 
device should be in (and in many non-ACPI cases I'd argue that to be true) 
your driver should be picking something sensible.  After all, states other 
than D0 and D3 are really device dependent, right?

One way to avoid some ugliness like you show above would be:

device_suspend(...)
{
  ...
  state = PCI_D3hot;
  pci_choose_state(dev, pm_state, &state);
  pci_set_power_state(dev, state);
  ...
}

So in this case pci_choose_state would either change state or leave it 
untouched if it didn't have a better idea about things.  But now that I look 
at it I'm not sure it's an improvement. :)

Jesse

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Friday, 9 of May 2008, Jesse Barnes wrote:
> On Friday, May 09, 2008 10:13 am Rafael J. Wysocki wrote:
> > > So why not make platform_pci_choose_state do:
> > > + pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t
> > > state)
> > > + {
> > > +       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > +               return state;
> > > + }
> > >
> > > instead?  Then in the PCI core we would assign either
> > > platform_pci_choose_state to acpi_pci_choose_state or
> > > noacpi_pci_choose_state
> >
> > Good idea.
> >
> > > (though that's a bad name).
> >
> > Does generic_pci_choose_state() sound better?
> 
> Yeah, that's better.
> 
> > > But really, since drivers should probably know what power state to put
> > > their devices in for suspend & hibernate, maybe on non-ACPI systems the
> > > function should just return an error and the driver can choose...
> >
> > That's one possibility too, but in that case many drivers will do
> >
> > state = pci_preferred_state(dev);
> > if (state == PCI_POWER_ERROR)
> > 	state = something;
> >
> > It's just shorter to write
> >
> > state = pci_preferred_state(dev, something);
> 
> But really that's the idea, since if the core doesn't know what state your 
> device should be in (and in many non-ACPI cases I'd argue that to be true) 
> your driver should be picking something sensible.  After all, states other 
> than D0 and D3 are really device dependent, right?
> 
> One way to avoid some ugliness like you show above would be:
> 
> device_suspend(...)
> {
>   ...
>   state = PCI_D3hot;
>   pci_choose_state(dev, pm_state, &state);
>   pci_set_power_state(dev, state);
>   ...
> }
> 
> So in this case pci_choose_state would either change state or leave it 
> untouched if it didn't have a better idea about things.  But now that I look 
> at it I'm not sure it's an improvement. :)

Well, in principle we could go farther and introduce a wrapper around
pci_set_power_state() that will call platform_pci_choose_state() to obtain the
new state or use the driver-provided one if that fails.

What do you think?

Rafael

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Jesse Barnes]

On Friday, May 09, 2008 10:34 am Rafael J. Wysocki wrote:
> > So in this case pci_choose_state would either change state or leave it
> > untouched if it didn't have a better idea about things.  But now that I
> > look at it I'm not sure it's an improvement. :)
>
> Well, in principle we could go farther and introduce a wrapper around
> pci_set_power_state() that will call platform_pci_choose_state() to obtain
> the new state or use the driver-provided one if that fails.

Hm, yeah that sounds pretty reasonable actually.  Especially given that so 
many drivers just do:
  pci_set_power_state(pdev, pci_choose_state(pdev, state));
anyway...

Jesse

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Friday, 9 of May 2008, Jesse Barnes wrote:
> On Friday, May 09, 2008 10:34 am Rafael J. Wysocki wrote:
> > > So in this case pci_choose_state would either change state or leave it
> > > untouched if it didn't have a better idea about things.  But now that I
> > > look at it I'm not sure it's an improvement. :)
> >
> > Well, in principle we could go farther and introduce a wrapper around
> > pci_set_power_state() that will call platform_pci_choose_state() to obtain
> > the new state or use the driver-provided one if that fails.
> 
> Hm, yeah that sounds pretty reasonable actually.  Especially given that so 
> many drivers just do:
>   pci_set_power_state(pdev, pci_choose_state(pdev, state));
> anyway...

Okay, what about this:

---
From: Rafael J. Wysocki <rjw@sisk.pl>

The new suspend and hibernation callbacks introduced with
'struct pm_ops' and 'struct pm_ext_ops' do not take a
pm_message_t argument, so the drivers using them will not be able
to use pci_choose_state() in its present form.  For this reason,
introduce a new function, pci_choose_and_set_state(), playing the
role of pci_choose_state() combined with pci_set_power_state() and
allowing the driver to put the device into a power state chosen by
the platform.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c   |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h |    1 +
 2 files changed, 46 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -509,7 +509,51 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
- 
+
+/**
+ * pci_choose_and_set_state - Choose the power state of a PCI device and put
+ *                            the device into that state.
+ * @dev: PCI device to be put into a low power state
+ * @state: State to put the device into by default
+ *
+ * Use the platform driver to choose the preferred PCI power state of given
+ * device and put the device into that state.  If the target power state of
+ * the device cannot be chosen using the platform driver, the driver-provided
+ * @state is used.
+ *
+ * RETURN VALUE:
+ * -EINVAL if trying to enter a lower state than we're already in.
+ * 0 if we're already in the requested state.
+ * -EIO if device does not support PCI PM.
+ * 0 if we can successfully change the power state.
+ */
+
+int pci_choose_and_set_state(struct pci_dev *dev, pci_power_t state)
+{
+	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+		return -EIO;
+
+	if (platform_pci_choose_state) {
+		pci_power_t ret = platform_pci_choose_state(dev);
+
+		switch (ret) {
+		case PCI_POWER_ERROR:
+		case PCI_UNKNOWN:
+			break;
+		case PCI_D1:
+		case PCI_D2:
+			if (pci_no_d1d2(dev))
+				break;
+		default:
+			state = ret;
+		}
+	}
+
+	return pci_set_power_state(dev, state);
+}
+
+EXPORT_SYMBOL(pci_choose_and_set_state);
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -615,6 +615,7 @@ size_t pci_get_rom_size(void __iomem *ro
 int pci_save_state(struct pci_dev *dev);
 int pci_restore_state(struct pci_dev *dev);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+int pci_choose_and_set_state(struct pci_dev *dev, pci_power_t state);
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable);
 

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Jesse Barnes]

On Friday, May 09, 2008 2:44 pm Rafael J. Wysocki wrote:
> Okay, what about this:
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> The new suspend and hibernation callbacks introduced with
> 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> pm_message_t argument, so the drivers using them will not be able
> to use pci_choose_state() in its present form.  For this reason,
> introduce a new function, pci_choose_and_set_state(), playing the
> role of pci_choose_state() combined with pci_set_power_state() and
> allowing the driver to put the device into a power state chosen by
> the platform.

Yeah, that looks pretty good.  The name is long but I can't think of a better 
one offhand.  Can you also update Documentation/power/pci.txt with the latest 
best practices?  I wonder if we should do a pass through the drivers 
converting them to this interface as well...

Thanks,
Jesse

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Saturday, 10 of May 2008, Jesse Barnes wrote:
> On Friday, May 09, 2008 2:44 pm Rafael J. Wysocki wrote:
> > Okay, what about this:
> >
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > The new suspend and hibernation callbacks introduced with
> > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > pm_message_t argument, so the drivers using them will not be able
> > to use pci_choose_state() in its present form.  For this reason,
> > introduce a new function, pci_choose_and_set_state(), playing the
> > role of pci_choose_state() combined with pci_set_power_state() and
> > allowing the driver to put the device into a power state chosen by
> > the platform.
> 
> Yeah, that looks pretty good.  The name is long but I can't think of a better 
> one offhand.  Can you also update Documentation/power/pci.txt with the latest 
> best practices?

I'm going to do that, eventually, but rather in a separate patch, when
everything is ready for the new framework, while at the moment we still have
some design work to do.

For example, some drivers may want to call pci_enable_wake() for the
target state and that must be done before pci_set_power_state() in case the
target state is D3cold.  To allow them to do that, we'll need a variant of
pci_choose_and_set_state() with a 'wake_enabled' argument.

> I wonder if we should do a pass through the drivers converting them to this
> interface as well... 

Well, that would be lots of work and since we'd like the drivers to switch to
the new framework entirely, we'll need to pass through them anyway for this
purpose.  I'd prefer that to be one pass. ;-)

Thanks,
Rafael

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Saturday, 10 of May 2008, Rafael J. Wysocki wrote:
> On Saturday, 10 of May 2008, Jesse Barnes wrote:
> > On Friday, May 09, 2008 2:44 pm Rafael J. Wysocki wrote:
> > > Okay, what about this:
> > >
> > > ---
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > >
> > > The new suspend and hibernation callbacks introduced with
> > > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > > pm_message_t argument, so the drivers using them will not be able
> > > to use pci_choose_state() in its present form.  For this reason,
> > > introduce a new function, pci_choose_and_set_state(), playing the
> > > role of pci_choose_state() combined with pci_set_power_state() and
> > > allowing the driver to put the device into a power state chosen by
> > > the platform.
> > 
> > Yeah, that looks pretty good.  The name is long but I can't think of a better 
> > one offhand.  Can you also update Documentation/power/pci.txt with the latest 
> > best practices?
> 
> I'm going to do that, eventually, but rather in a separate patch, when
> everything is ready for the new framework, while at the moment we still have
> some design work to do.
> 
> For example, some drivers may want to call pci_enable_wake() for the
> target state and that must be done before pci_set_power_state() in case the
> target state is D3cold.  To allow them to do that, we'll need a variant of
> pci_choose_and_set_state() with a 'wake_enabled' argument.

Below is how I think that may look like.  Please tell me what you think.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

The new suspend and hibernation callbacks introduced with
'struct pm_ops' and 'struct pm_ext_ops' do not take a
pm_message_t argument, so the drivers using them will not be able
to use pci_choose_state() in its present form.  For this reason,
introduce a new function, raw_pci_change_state(), playing the
role of pci_choose_state() combined with pci_enable_wake() and
pci_set_power_state() and allowing the driver to put the device
into a power state chosen by the platform, optionally configuring
the device as a source of wakeup events.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c   |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h |   11 +++++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -509,7 +509,67 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
- 
+
+/**
+ * raw_pci_change_state - Choose the power state of a PCI device and put the
+ *                        device into that state.
+ * @dev: PCI device to be put into a low power state
+ * @state: State to put the device into by default
+ * @enable_wake: If set, attempt to enable device to generate wakeup events
+ * @fail_no_wake: If set, return error code if the attempt to enable device
+ *                to generate wakeup events fails
+ *
+ * Use the platform driver to choose the preferred PCI power state of given
+ * device and put the device into that state.  If the target power state of
+ * the device cannot be chosen using the platform driver, the driver-provided
+ * @state is used.  If @enable_wake is set, try to enable the device to
+ * generate wakeup events.
+ *
+ * RETURN VALUE:
+ * -EINVAL if trying to enter a lower state than we're already in.
+ * 0 if we're already in the requested state.
+ * -EIO if device does not support PCI PM.
+ * 0 if we can successfully change the power state.
+ *
+ * If both @enable_wake and @fail_no_wake are set, additionally:
+ * -EIO if the device can't ever be a wakeup event source.
+ * -EINVAL if the device can't generate wakeup events from given state.
+ */
+
+int raw_pci_change_state(struct pci_dev *dev, pci_power_t state,
+                         bool enable_wake, bool fail_no_wake)
+{
+	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+		return -EIO;
+
+	if (platform_pci_choose_state) {
+		pci_power_t ret = platform_pci_choose_state(dev);
+
+		switch (ret) {
+		case PCI_POWER_ERROR:
+		case PCI_UNKNOWN:
+			break;
+		case PCI_D1:
+		case PCI_D2:
+			if (pci_no_d1d2(dev))
+				break;
+		default:
+			state = ret;
+		}
+	}
+
+	if (enable_wake) {
+		int error = pci_enable_wake(dev, state, true);
+
+		if (error && fail_no_wake)
+			return error;
+	}
+
+	return pci_set_power_state(dev, state);
+}
+
+EXPORT_SYMBOL(raw_pci_change_state);
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -615,6 +615,17 @@ size_t pci_get_rom_size(void __iomem *ro
 int pci_save_state(struct pci_dev *dev);
 int pci_restore_state(struct pci_dev *dev);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+int raw_pci_change_state(struct pci_dev *dev, pci_power_t state,
+                         bool enable_wake, bool fail_no_wake);
+static inline int pci_change_state(struct pci_dev *dev, pci_power_t state)
+{
+	return raw_pci_change_state(dev, state, false, false);
+}
+static inline int pci_change_state_wake(struct pci_dev *dev,
+                                          pci_power_t state)
+{
+	return raw_pci_change_state(dev, state, true, false);
+}
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable);
 

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The new suspend and hibernation callbacks introduced with
> 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> pm_message_t argument, so the drivers using them will not be able
> to use pci_choose_state() in its present form.  For this reason,
> introduce a new function, pci_choose_and_set_state(), playing the
> role of pci_choose_state() combined with pci_set_power_state() and
> allowing the driver to put the device into a power state chosen by
> the platform.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/pci/pci.c   |   46 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pci.h |    1 +
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -509,7 +509,51 @@ pci_set_power_state(struct pci_dev *dev,
>  }
>  
>  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> - 
> +
> +/**
> + * pci_choose_and_set_state - Choose the power state of a PCI device and put
> + *                            the device into that state.
> + * @dev: PCI device to be put into a low power state
> + * @state: State to put the device into by default
> + *
> + * Use the platform driver to choose the preferred PCI power state of given
> + * device and put the device into that state.  If the target power state of
> + * the device cannot be chosen using the platform driver, the driver-provided
> + * @state is used.
> + *
> + * RETURN VALUE:
> + * -EINVAL if trying to enter a lower state than we're already in.
> + * 0 if we're already in the requested state.
> + * -EIO if device does not support PCI PM.
> + * 0 if we can successfully change the power state.
> + */
> +
> +int pci_choose_and_set_state(struct pci_dev *dev, pci_power_t state)
> +{

> +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> +		return -EIO;

Hmm, not sure if this is good idea. First, simple drivers will just
ignore the return value, and second, it returns -EIO even if I'm
asking it to enter state it already has -- like PCI_D0 -- no?


> +	if (platform_pci_choose_state) {
> +		pci_power_t ret = platform_pci_choose_state(dev);
> +
> +		switch (ret) {
> +		case PCI_POWER_ERROR:
> +		case PCI_UNKNOWN:
> +			break;
> +		case PCI_D1:
> +		case PCI_D2:
> +			if (pci_no_d1d2(dev))
> +				break;
> +		default:
> +			state = ret;
> +		}
> +	}
> +
> +	return pci_set_power_state(dev, state);
> +}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Monday, 12 of May 2008, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The new suspend and hibernation callbacks introduced with
> > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > pm_message_t argument, so the drivers using them will not be able
> > to use pci_choose_state() in its present form.  For this reason,
> > introduce a new function, pci_choose_and_set_state(), playing the
> > role of pci_choose_state() combined with pci_set_power_state() and
> > allowing the driver to put the device into a power state chosen by
> > the platform.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/pci/pci.c   |   46 +++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/pci.h |    1 +
> >  2 files changed, 46 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/drivers/pci/pci.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/pci.c
> > +++ linux-2.6/drivers/pci/pci.c
> > @@ -509,7 +509,51 @@ pci_set_power_state(struct pci_dev *dev,
> >  }
> >  
> >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > - 
> > +
> > +/**
> > + * pci_choose_and_set_state - Choose the power state of a PCI device and put
> > + *                            the device into that state.
> > + * @dev: PCI device to be put into a low power state
> > + * @state: State to put the device into by default
> > + *
> > + * Use the platform driver to choose the preferred PCI power state of given
> > + * device and put the device into that state.  If the target power state of
> > + * the device cannot be chosen using the platform driver, the driver-provided
> > + * @state is used.
> > + *
> > + * RETURN VALUE:
> > + * -EINVAL if trying to enter a lower state than we're already in.
> > + * 0 if we're already in the requested state.
> > + * -EIO if device does not support PCI PM.
> > + * 0 if we can successfully change the power state.
> > + */
> > +
> > +int pci_choose_and_set_state(struct pci_dev *dev, pci_power_t state)
> > +{
> 
> > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > +		return -EIO;
> 
> Hmm, not sure if this is good idea. First, simple drivers will just
> ignore the return value,

That doesn't matter to us.

> and second, it returns -EIO even if I'm 
> asking it to enter state it already has -- like PCI_D0 -- no?

The -EIO means the operation could not be carried out, whatever the driver
author meant.  In particular, we wouldn't check what the current state of the
device was, so why would we return anything other than error code?

Thanks,
Rafael

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Alan Stern]

On Wed, 7 May 2008, Rafael J. Wysocki wrote:

> On Wednesday, 7 of May 2008, Pavel Machek wrote:
> > Hi!
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > The new suspend and hibernation callbacks introduced with
> > > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > > pm_message_t argument, so the drivers using them will not be able
> > > to use pci_choose_state() in its present form.  For this reason,
> > > introduce the new function pci_preferred_state() playing the role
> > > of pci_choose_state(), but taking only a pointer to the device
> > > object.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
> > >  include/linux/pci.h |    1 +
> > >  include/linux/pm.h  |   10 ++++++++++
> > >  3 files changed, 43 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6/drivers/pci/pci.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/pci/pci.c
> > > +++ linux-2.6/drivers/pci/pci.c
> > > @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
> > >  }
> > >  
> > >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > > - 
> > > +
> > > +/**
> > > + * pci_preferred_state - Choose the preferred power state of a PCI device
> > > + * @dev: PCI device to be put into the low power state
> > > + * @sp: Information aboutabout what the driver would prefer to do with
> > > + *	the device if there were no platform-implemeted policy.
> > > + *
> > > + * Returns PCI power state suitable for given device and given suspend policy.
> > > + * The policy, however, is only used if platform_pci_choose_state() fails or is
> > > + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> > > + * implements the right policy.
> > > + */
> > > +
> > > +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> > > +{
> > > +	pci_power_t ret;
> > > +
> > > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > +		return PCI_D0;
> > > +
> > > +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> > > +	if (platform_pci_choose_state) {
> > > +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> > > +
> > > +		if (platform_ret != PCI_POWER_ERROR)
> > > +			ret = platform_ret;
> > > +	}
> > > +	return ret;
> > > +}
> > > +
> > > +EXPORT_SYMBOL(pci_preferred_state);
> > 
> > I don't get it. How is driver supposed to use this? How does the
> > driver decide between SP_TURN_OFF and SP_TURN_ON?
> > 
> > ...and it seems to be clearer to just inline this in the driver... or
> > pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
> > define...
> 
> I thought about that too.  I'd like to know what the other people think,
> though.

The point of this isn't at all clear.

Is this routine meant to be called during a hibernation 
transition?  Or is it just for suspend?

And why would the return value ever be anything other than D3_hot?  (Or 
why would the driver ever want to put a device in a different state?)
AFAICS, the only reason would be because platform_pci_choose_state() 
suggested something else.  In which case there's no need for the 
"policy" argument.

Alan Stern

[PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

[Well, this essentially is a PCI patch, but it modifies include/linux/pm.h too.]
---
From: Rafael J. Wysocki <rjw@sisk.pl>

The new suspend and hibernation callbacks introduced with
'struct pm_ops' and 'struct pm_ext_ops' do not take a
pm_message_t argument, so the drivers using them will not be able
to use pci_choose_state() in its present form.  For this reason,
introduce the new function pci_preferred_state() playing the role
of pci_choose_state(), but taking only a pointer to the device
object.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
 include/linux/pci.h |    1 +
 include/linux/pm.h  |   10 ++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
- 
+
+/**
+ * pci_preferred_state - Choose the preferred power state of a PCI device
+ * @dev: PCI device to be put into the low power state
+ * @sp: Information aboutabout what the driver would prefer to do with
+ *	the device if there were no platform-implemeted policy.
+ *
+ * Returns PCI power state suitable for given device and given suspend policy.
+ * The policy, however, is only used if platform_pci_choose_state() fails or is
+ * not present.  Otherwise, it is assumed that platform_pci_choose_state()
+ * implements the right policy.
+ */
+
+pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
+{
+	pci_power_t ret;
+
+	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+		return PCI_D0;
+
+	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
+	if (platform_pci_choose_state) {
+		pci_power_t platform_ret = platform_pci_choose_state(dev);
+
+		if (platform_ret != PCI_POWER_ERROR)
+			ret = platform_ret;
+	}
+	return ret;
+}
+
+EXPORT_SYMBOL(pci_preferred_state);
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -615,6 +615,7 @@ size_t pci_get_rom_size(void __iomem *ro
 int pci_save_state(struct pci_dev *dev);
 int pci_restore_state(struct pci_dev *dev);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp);
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable);
 
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -391,6 +391,16 @@ struct dev_pm_info {
 };
 
 /*
+ * Used to pass the information about what the driver would prefer to do with
+ * its device before the system in put into a sleep state, if there were no
+ * platform-implemeted policy.
+ */
+enum suspend_policy {
+	SP_TURN_ON,
+	SP_TURN_OFF,
+};
+
+/*
  * The PM_EVENT_ messages are also used by drivers implementing the legacy
  * suspend framework, based on the ->suspend() and ->resume() callbacks common
  * for suspend and hibernation transitions, according to the rules below.

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The new suspend and hibernation callbacks introduced with
> 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> pm_message_t argument, so the drivers using them will not be able
> to use pci_choose_state() in its present form.  For this reason,
> introduce the new function pci_preferred_state() playing the role
> of pci_choose_state(), but taking only a pointer to the device
> object.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
>  include/linux/pci.h |    1 +
>  include/linux/pm.h  |   10 ++++++++++
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
>  }
>  
>  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> - 
> +
> +/**
> + * pci_preferred_state - Choose the preferred power state of a PCI device
> + * @dev: PCI device to be put into the low power state
> + * @sp: Information aboutabout what the driver would prefer to do with
> + *	the device if there were no platform-implemeted policy.
> + *
> + * Returns PCI power state suitable for given device and given suspend policy.
> + * The policy, however, is only used if platform_pci_choose_state() fails or is
> + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> + * implements the right policy.
> + */
> +
> +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> +{
> +	pci_power_t ret;
> +
> +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> +		return PCI_D0;
> +
> +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> +	if (platform_pci_choose_state) {
> +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> +
> +		if (platform_ret != PCI_POWER_ERROR)
> +			ret = platform_ret;
> +	}
> +	return ret;
> +}
> +
> +EXPORT_SYMBOL(pci_preferred_state);

I don't get it. How is driver supposed to use this? How does the
driver decide between SP_TURN_OFF and SP_TURN_ON?

...and it seems to be clearer to just inline this in the driver... or
pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
define...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The new suspend and hibernation callbacks introduced with
> > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > pm_message_t argument, so the drivers using them will not be able
> > to use pci_choose_state() in its present form.  For this reason,
> > introduce the new function pci_preferred_state() playing the role
> > of pci_choose_state(), but taking only a pointer to the device
> > object.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
> >  include/linux/pci.h |    1 +
> >  include/linux/pm.h  |   10 ++++++++++
> >  3 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/drivers/pci/pci.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/pci.c
> > +++ linux-2.6/drivers/pci/pci.c
> > @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
> >  }
> >  
> >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > - 
> > +
> > +/**
> > + * pci_preferred_state - Choose the preferred power state of a PCI device
> > + * @dev: PCI device to be put into the low power state
> > + * @sp: Information aboutabout what the driver would prefer to do with
> > + *	the device if there were no platform-implemeted policy.
> > + *
> > + * Returns PCI power state suitable for given device and given suspend policy.
> > + * The policy, however, is only used if platform_pci_choose_state() fails or is
> > + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> > + * implements the right policy.
> > + */
> > +
> > +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> > +{
> > +	pci_power_t ret;
> > +
> > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > +		return PCI_D0;
> > +
> > +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> > +	if (platform_pci_choose_state) {
> > +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> > +
> > +		if (platform_ret != PCI_POWER_ERROR)
> > +			ret = platform_ret;
> > +	}
> > +	return ret;
> > +}
> > +
> > +EXPORT_SYMBOL(pci_preferred_state);
> 
> I don't get it. How is driver supposed to use this? How does the
> driver decide between SP_TURN_OFF and SP_TURN_ON?
> 
> ...and it seems to be clearer to just inline this in the driver... or
> pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
> define...

I thought about that too.  I'd like to know what the other people think,
though.

Thanks,
Rafael