Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

["Rafael J. Wysocki" <rjw@sisk.pl>]

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);