linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices
@ 2012-11-20  8:08 Huang Ying
  2012-11-20 16:04 ` Alan Stern
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Huang Ying @ 2012-11-20  8:08 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Huang Ying,
	Rafael J. Wysocki, Alan Stern

For unbound PCI devices, what we need is:

 - Always in D0 state, because some devices does not work again after
   being put into D3 by the PCI bus.

 - In SUSPENDED state if allowed, so that the parent devices can still
   be put into low power state.

To satisfy these requirement, the runtime PM for the unbound PCI
devices are disabled and set to SUSPENDED state.  One issue of this
solution is that the PCI devices will be put into SUSPENDED state even
if the SUSPENDED state is forbidden via the sysfs interface
(.../power/control) of the device.  This is not an issue for most
devices, because most PCI devices are not used at all if unbounded.
But there are exceptions.  For example, unbound VGA card can be used
for display, but suspend its parents make it stop working.

To fix the issue, we keep the runtime PM enabled when the PCI devices
are unbound.  But the runtime PM callbacks will do nothing if the PCI
devices are unbound.  This way, we can put the PCI devices into
SUSPENDED state without put the PCI devices into D3 state.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
CC: stable@vger.kernel.org          # v3.6+
---
 drivers/pci/pci-driver.c |   69 +++++++++++++++++++++++++++--------------------
 drivers/pci/pci.c        |    2 +
 2 files changed, 43 insertions(+), 28 deletions(-)

--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1900,6 +1900,8 @@ void pci_pm_init(struct pci_dev *dev)
 	u16 pmc;
 
 	pm_runtime_forbid(&dev->dev);
+	pm_runtime_set_active(&dev->dev);
+	pm_runtime_enable(&dev->dev);
 	device_enable_async_suspend(&dev->dev);
 	dev->wakeup_prepared = false;
 
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -256,31 +256,26 @@ struct drv_dev_and_id {
 static long local_pci_probe(void *_ddi)
 {
 	struct drv_dev_and_id *ddi = _ddi;
-	struct device *dev = &ddi->dev->dev;
-	struct device *parent = dev->parent;
+	struct pci_dev *pci_dev = ddi->dev;
+	struct pci_driver *pci_drv = ddi->drv;
+	struct device *dev = &pci_dev->dev;
 	int rc;
 
-	/* The parent bridge must be in active state when probing */
-	if (parent)
-		pm_runtime_get_sync(parent);
-	/* Unbound PCI devices are always set to disabled and suspended.
-	 * During probe, the device is set to enabled and active and the
-	 * usage count is incremented.  If the driver supports runtime PM,
-	 * it should call pm_runtime_put_noidle() in its probe routine and
-	 * pm_runtime_get_noresume() in its remove routine.
-	 */
-	pm_runtime_get_noresume(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
-	rc = ddi->drv->probe(ddi->dev, ddi->id);
+	/*
+	 * Unbound PCI devices are always put in D0, regardless of
+	 * runtime PM status.  During probe, the device is set to
+	 * active and the usage count is incremented.  If the driver
+	 * supports runtime PM, it should call pm_runtime_put_noidle()
+	 * in its probe routine and pm_runtime_get_noresume() in its
+	 * remove routine.
+	 */
+	pm_runtime_get_sync(dev);
+	pci_dev->driver = pci_drv;
+	rc = pci_drv->probe(pci_dev, ddi->id);
 	if (rc) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_suspended(dev);
-		pm_runtime_put_noidle(dev);
+		pci_dev->driver = NULL;
+		pm_runtime_put_sync(dev);
 	}
-	if (parent)
-		pm_runtime_put(parent);
 	return rc;
 }
 
@@ -330,10 +325,8 @@ __pci_device_probe(struct pci_driver *dr
 		id = pci_match_device(drv, pci_dev);
 		if (id)
 			error = pci_call_probe(drv, pci_dev, id);
-		if (error >= 0) {
-			pci_dev->driver = drv;
+		if (error >= 0)
 			error = 0;
-		}
 	}
 	return error;
 }
@@ -369,9 +362,7 @@ static int pci_device_remove(struct devi
 	}
 
 	/* Undo the runtime PM settings in local_pci_probe() */
-	pm_runtime_disable(dev);
-	pm_runtime_set_suspended(dev);
-	pm_runtime_put_noidle(dev);
+	pm_runtime_put_sync(dev);
 
 	/*
 	 * If the device is still on, set the power state as "unknown",
@@ -994,6 +985,13 @@ static int pci_pm_runtime_suspend(struct
 	pci_power_t prev = pci_dev->current_state;
 	int error;
 
+	/*
+	 * If pci_dev->driver is not set (unbound), the device should
+	 * always remain in D0 regardless of the runtime PM status
+	 */
+	if (!pci_dev->driver)
+		return 0;
+
 	if (!pm || !pm->runtime_suspend)
 		return -ENOSYS;
 
@@ -1029,6 +1027,13 @@ static int pci_pm_runtime_resume(struct
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
+	/*
+	 * If pci_dev->driver is not set (unbound), the device should
+	 * always remain in D0 regardless of the runtime PM status
+	 */
+	if (!pci_dev->driver)
+		return 0;
+
 	if (!pm || !pm->runtime_resume)
 		return -ENOSYS;
 
@@ -1046,8 +1051,16 @@ static int pci_pm_runtime_resume(struct
 
 static int pci_pm_runtime_idle(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
+	/*
+	 * If pci_dev->driver is not set (unbound), the device should
+	 * always remain in D0 regardless of the runtime PM status
+	 */
+	if (!pci_dev->driver)
+		goto out;
+
 	if (!pm)
 		return -ENOSYS;
 
@@ -1057,8 +1070,8 @@ static int pci_pm_runtime_idle(struct de
 			return ret;
 	}
 
+out:
 	pm_runtime_suspend(dev);
-
 	return 0;
 }
 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices
  2012-11-20  8:08 [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices Huang Ying
@ 2012-11-20 16:04 ` Alan Stern
  2012-11-20 21:38 ` Rafael J. Wysocki
  2012-11-28 22:25 ` Bjorn Helgaas
  2 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2012-11-20 16:04 UTC (permalink / raw)
  To: Huang Ying
  Cc: Bjorn Helgaas, linux-kernel, linux-pci, linux-pm,
	Rafael J. Wysocki, Rafael J. Wysocki

On Tue, 20 Nov 2012, Huang Ying wrote:

> For unbound PCI devices, what we need is:
> 
>  - Always in D0 state, because some devices does not work again after
>    being put into D3 by the PCI bus.
> 
>  - In SUSPENDED state if allowed, so that the parent devices can still
>    be put into low power state.
> 
> To satisfy these requirement, the runtime PM for the unbound PCI
> devices are disabled and set to SUSPENDED state.  One issue of this
> solution is that the PCI devices will be put into SUSPENDED state even
> if the SUSPENDED state is forbidden via the sysfs interface
> (.../power/control) of the device.  This is not an issue for most
> devices, because most PCI devices are not used at all if unbounded.
> But there are exceptions.  For example, unbound VGA card can be used
> for display, but suspend its parents make it stop working.
> 
> To fix the issue, we keep the runtime PM enabled when the PCI devices
> are unbound.  But the runtime PM callbacks will do nothing if the PCI
> devices are unbound.  This way, we can put the PCI devices into
> SUSPENDED state without put the PCI devices into D3 state.

Acked-by: Alan Stern <stern@rowland.harvard.edu>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices
  2012-11-20  8:08 [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices Huang Ying
  2012-11-20 16:04 ` Alan Stern
@ 2012-11-20 21:38 ` Rafael J. Wysocki
  2012-11-28 22:25 ` Bjorn Helgaas
  2 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2012-11-20 21:38 UTC (permalink / raw)
  To: Huang Ying
  Cc: Bjorn Helgaas, linux-kernel, linux-pci, linux-pm,
	Rafael J. Wysocki, Alan Stern

On Tuesday, November 20, 2012 04:08:22 PM Huang Ying wrote:
> For unbound PCI devices, what we need is:
> 
>  - Always in D0 state, because some devices does not work again after
>    being put into D3 by the PCI bus.
> 
>  - In SUSPENDED state if allowed, so that the parent devices can still
>    be put into low power state.
> 
> To satisfy these requirement, the runtime PM for the unbound PCI
> devices are disabled and set to SUSPENDED state.  One issue of this
> solution is that the PCI devices will be put into SUSPENDED state even
> if the SUSPENDED state is forbidden via the sysfs interface
> (.../power/control) of the device.  This is not an issue for most
> devices, because most PCI devices are not used at all if unbounded.
> But there are exceptions.  For example, unbound VGA card can be used
> for display, but suspend its parents make it stop working.
> 
> To fix the issue, we keep the runtime PM enabled when the PCI devices
> are unbound.  But the runtime PM callbacks will do nothing if the PCI
> devices are unbound.  This way, we can put the PCI devices into
> SUSPENDED state without put the PCI devices into D3 state.
> 
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> CC: stable@vger.kernel.org          # v3.6+

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci-driver.c |   69 +++++++++++++++++++++++++++--------------------
>  drivers/pci/pci.c        |    2 +
>  2 files changed, 43 insertions(+), 28 deletions(-)
> 
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1900,6 +1900,8 @@ void pci_pm_init(struct pci_dev *dev)
>  	u16 pmc;
>  
>  	pm_runtime_forbid(&dev->dev);
> +	pm_runtime_set_active(&dev->dev);
> +	pm_runtime_enable(&dev->dev);
>  	device_enable_async_suspend(&dev->dev);
>  	dev->wakeup_prepared = false;
>  
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -256,31 +256,26 @@ struct drv_dev_and_id {
>  static long local_pci_probe(void *_ddi)
>  {
>  	struct drv_dev_and_id *ddi = _ddi;
> -	struct device *dev = &ddi->dev->dev;
> -	struct device *parent = dev->parent;
> +	struct pci_dev *pci_dev = ddi->dev;
> +	struct pci_driver *pci_drv = ddi->drv;
> +	struct device *dev = &pci_dev->dev;
>  	int rc;
>  
> -	/* The parent bridge must be in active state when probing */
> -	if (parent)
> -		pm_runtime_get_sync(parent);
> -	/* Unbound PCI devices are always set to disabled and suspended.
> -	 * During probe, the device is set to enabled and active and the
> -	 * usage count is incremented.  If the driver supports runtime PM,
> -	 * it should call pm_runtime_put_noidle() in its probe routine and
> -	 * pm_runtime_get_noresume() in its remove routine.
> -	 */
> -	pm_runtime_get_noresume(dev);
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> -
> -	rc = ddi->drv->probe(ddi->dev, ddi->id);
> +	/*
> +	 * Unbound PCI devices are always put in D0, regardless of
> +	 * runtime PM status.  During probe, the device is set to
> +	 * active and the usage count is incremented.  If the driver
> +	 * supports runtime PM, it should call pm_runtime_put_noidle()
> +	 * in its probe routine and pm_runtime_get_noresume() in its
> +	 * remove routine.
> +	 */
> +	pm_runtime_get_sync(dev);
> +	pci_dev->driver = pci_drv;
> +	rc = pci_drv->probe(pci_dev, ddi->id);
>  	if (rc) {
> -		pm_runtime_disable(dev);
> -		pm_runtime_set_suspended(dev);
> -		pm_runtime_put_noidle(dev);
> +		pci_dev->driver = NULL;
> +		pm_runtime_put_sync(dev);
>  	}
> -	if (parent)
> -		pm_runtime_put(parent);
>  	return rc;
>  }
>  
> @@ -330,10 +325,8 @@ __pci_device_probe(struct pci_driver *dr
>  		id = pci_match_device(drv, pci_dev);
>  		if (id)
>  			error = pci_call_probe(drv, pci_dev, id);
> -		if (error >= 0) {
> -			pci_dev->driver = drv;
> +		if (error >= 0)
>  			error = 0;
> -		}
>  	}
>  	return error;
>  }
> @@ -369,9 +362,7 @@ static int pci_device_remove(struct devi
>  	}
>  
>  	/* Undo the runtime PM settings in local_pci_probe() */
> -	pm_runtime_disable(dev);
> -	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
> +	pm_runtime_put_sync(dev);
>  
>  	/*
>  	 * If the device is still on, set the power state as "unknown",
> @@ -994,6 +985,13 @@ static int pci_pm_runtime_suspend(struct
>  	pci_power_t prev = pci_dev->current_state;
>  	int error;
>  
> +	/*
> +	 * If pci_dev->driver is not set (unbound), the device should
> +	 * always remain in D0 regardless of the runtime PM status
> +	 */
> +	if (!pci_dev->driver)
> +		return 0;
> +
>  	if (!pm || !pm->runtime_suspend)
>  		return -ENOSYS;
>  
> @@ -1029,6 +1027,13 @@ static int pci_pm_runtime_resume(struct
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> +	/*
> +	 * If pci_dev->driver is not set (unbound), the device should
> +	 * always remain in D0 regardless of the runtime PM status
> +	 */
> +	if (!pci_dev->driver)
> +		return 0;
> +
>  	if (!pm || !pm->runtime_resume)
>  		return -ENOSYS;
>  
> @@ -1046,8 +1051,16 @@ static int pci_pm_runtime_resume(struct
>  
>  static int pci_pm_runtime_idle(struct device *dev)
>  {
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> +	/*
> +	 * If pci_dev->driver is not set (unbound), the device should
> +	 * always remain in D0 regardless of the runtime PM status
> +	 */
> +	if (!pci_dev->driver)
> +		goto out;
> +
>  	if (!pm)
>  		return -ENOSYS;
>  
> @@ -1057,8 +1070,8 @@ static int pci_pm_runtime_idle(struct de
>  			return ret;
>  	}
>  
> +out:
>  	pm_runtime_suspend(dev);
> -
>  	return 0;
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices
  2012-11-20  8:08 [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices Huang Ying
  2012-11-20 16:04 ` Alan Stern
  2012-11-20 21:38 ` Rafael J. Wysocki
@ 2012-11-28 22:25 ` Bjorn Helgaas
  2012-11-28 22:43   ` Rafael J. Wysocki
  2 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2012-11-28 22:25 UTC (permalink / raw)
  To: Huang Ying
  Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki,
	Rafael J. Wysocki, Alan Stern

On Tue, Nov 20, 2012 at 1:08 AM, Huang Ying <ying.huang@intel.com> wrote:
> For unbound PCI devices, what we need is:
>
>  - Always in D0 state, because some devices does not work again after
>    being put into D3 by the PCI bus.
>
>  - In SUSPENDED state if allowed, so that the parent devices can still
>    be put into low power state.
>
> To satisfy these requirement, the runtime PM for the unbound PCI
> devices are disabled and set to SUSPENDED state.  One issue of this
> solution is that the PCI devices will be put into SUSPENDED state even
> if the SUSPENDED state is forbidden via the sysfs interface
> (.../power/control) of the device.  This is not an issue for most
> devices, because most PCI devices are not used at all if unbounded.
> But there are exceptions.  For example, unbound VGA card can be used
> for display, but suspend its parents make it stop working.
>
> To fix the issue, we keep the runtime PM enabled when the PCI devices
> are unbound.  But the runtime PM callbacks will do nothing if the PCI
> devices are unbound.  This way, we can put the PCI devices into
> SUSPENDED state without put the PCI devices into D3 state.

Does this fix a reported problem?  Is there a bug report URL?  What
problem would a user notice?

This doesn't look critical enough to try to put in v3.7 (correct me if
I'm wrong).  It's getting pretty late for the v3.8 merge window, but
it looks like it qualifies as a bug fix that we would merge even after
the merge window, so I'll put it in my -next branch headed for v3.8.
Then it can be backported to v3.6 and v3.7.

> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> CC: stable@vger.kernel.org          # v3.6+
> ---
>  drivers/pci/pci-driver.c |   69 +++++++++++++++++++++++++++--------------------
>  drivers/pci/pci.c        |    2 +
>  2 files changed, 43 insertions(+), 28 deletions(-)
>
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1900,6 +1900,8 @@ void pci_pm_init(struct pci_dev *dev)
>         u16 pmc;
>
>         pm_runtime_forbid(&dev->dev);
> +       pm_runtime_set_active(&dev->dev);
> +       pm_runtime_enable(&dev->dev);
>         device_enable_async_suspend(&dev->dev);
>         dev->wakeup_prepared = false;
>
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -256,31 +256,26 @@ struct drv_dev_and_id {
>  static long local_pci_probe(void *_ddi)
>  {
>         struct drv_dev_and_id *ddi = _ddi;
> -       struct device *dev = &ddi->dev->dev;
> -       struct device *parent = dev->parent;
> +       struct pci_dev *pci_dev = ddi->dev;
> +       struct pci_driver *pci_drv = ddi->drv;
> +       struct device *dev = &pci_dev->dev;
>         int rc;
>
> -       /* The parent bridge must be in active state when probing */
> -       if (parent)
> -               pm_runtime_get_sync(parent);
> -       /* Unbound PCI devices are always set to disabled and suspended.
> -        * During probe, the device is set to enabled and active and the
> -        * usage count is incremented.  If the driver supports runtime PM,
> -        * it should call pm_runtime_put_noidle() in its probe routine and
> -        * pm_runtime_get_noresume() in its remove routine.
> -        */
> -       pm_runtime_get_noresume(dev);
> -       pm_runtime_set_active(dev);
> -       pm_runtime_enable(dev);
> -
> -       rc = ddi->drv->probe(ddi->dev, ddi->id);
> +       /*
> +        * Unbound PCI devices are always put in D0, regardless of
> +        * runtime PM status.  During probe, the device is set to
> +        * active and the usage count is incremented.  If the driver
> +        * supports runtime PM, it should call pm_runtime_put_noidle()
> +        * in its probe routine and pm_runtime_get_noresume() in its
> +        * remove routine.
> +        */
> +       pm_runtime_get_sync(dev);
> +       pci_dev->driver = pci_drv;
> +       rc = pci_drv->probe(pci_dev, ddi->id);
>         if (rc) {
> -               pm_runtime_disable(dev);
> -               pm_runtime_set_suspended(dev);
> -               pm_runtime_put_noidle(dev);
> +               pci_dev->driver = NULL;
> +               pm_runtime_put_sync(dev);
>         }
> -       if (parent)
> -               pm_runtime_put(parent);
>         return rc;
>  }
>
> @@ -330,10 +325,8 @@ __pci_device_probe(struct pci_driver *dr
>                 id = pci_match_device(drv, pci_dev);
>                 if (id)
>                         error = pci_call_probe(drv, pci_dev, id);
> -               if (error >= 0) {
> -                       pci_dev->driver = drv;
> +               if (error >= 0)
>                         error = 0;
> -               }
>         }
>         return error;
>  }
> @@ -369,9 +362,7 @@ static int pci_device_remove(struct devi
>         }
>
>         /* Undo the runtime PM settings in local_pci_probe() */
> -       pm_runtime_disable(dev);
> -       pm_runtime_set_suspended(dev);
> -       pm_runtime_put_noidle(dev);
> +       pm_runtime_put_sync(dev);
>
>         /*
>          * If the device is still on, set the power state as "unknown",
> @@ -994,6 +985,13 @@ static int pci_pm_runtime_suspend(struct
>         pci_power_t prev = pci_dev->current_state;
>         int error;
>
> +       /*
> +        * If pci_dev->driver is not set (unbound), the device should
> +        * always remain in D0 regardless of the runtime PM status
> +        */
> +       if (!pci_dev->driver)
> +               return 0;
> +
>         if (!pm || !pm->runtime_suspend)
>                 return -ENOSYS;
>
> @@ -1029,6 +1027,13 @@ static int pci_pm_runtime_resume(struct
>         struct pci_dev *pci_dev = to_pci_dev(dev);
>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>
> +       /*
> +        * If pci_dev->driver is not set (unbound), the device should
> +        * always remain in D0 regardless of the runtime PM status
> +        */
> +       if (!pci_dev->driver)
> +               return 0;
> +
>         if (!pm || !pm->runtime_resume)
>                 return -ENOSYS;
>
> @@ -1046,8 +1051,16 @@ static int pci_pm_runtime_resume(struct
>
>  static int pci_pm_runtime_idle(struct device *dev)
>  {
> +       struct pci_dev *pci_dev = to_pci_dev(dev);
>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>
> +       /*
> +        * If pci_dev->driver is not set (unbound), the device should
> +        * always remain in D0 regardless of the runtime PM status
> +        */
> +       if (!pci_dev->driver)
> +               goto out;
> +
>         if (!pm)
>                 return -ENOSYS;
>
> @@ -1057,8 +1070,8 @@ static int pci_pm_runtime_idle(struct de
>                         return ret;
>         }
>
> +out:
>         pm_runtime_suspend(dev);
> -
>         return 0;
>  }
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices
  2012-11-28 22:25 ` Bjorn Helgaas
@ 2012-11-28 22:43   ` Rafael J. Wysocki
  2012-12-05  0:06     ` Bjorn Helgaas
  0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2012-11-28 22:43 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Huang Ying, linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki,
	Alan Stern

On Wednesday, November 28, 2012 03:25:59 PM Bjorn Helgaas wrote:
> On Tue, Nov 20, 2012 at 1:08 AM, Huang Ying <ying.huang@intel.com> wrote:
> > For unbound PCI devices, what we need is:
> >
> >  - Always in D0 state, because some devices does not work again after
> >    being put into D3 by the PCI bus.
> >
> >  - In SUSPENDED state if allowed, so that the parent devices can still
> >    be put into low power state.
> >
> > To satisfy these requirement, the runtime PM for the unbound PCI
> > devices are disabled and set to SUSPENDED state.  One issue of this
> > solution is that the PCI devices will be put into SUSPENDED state even
> > if the SUSPENDED state is forbidden via the sysfs interface
> > (.../power/control) of the device.  This is not an issue for most
> > devices, because most PCI devices are not used at all if unbounded.
> > But there are exceptions.  For example, unbound VGA card can be used
> > for display, but suspend its parents make it stop working.
> >
> > To fix the issue, we keep the runtime PM enabled when the PCI devices
> > are unbound.  But the runtime PM callbacks will do nothing if the PCI
> > devices are unbound.  This way, we can put the PCI devices into
> > SUSPENDED state without put the PCI devices into D3 state.
> 
> Does this fix a reported problem?  Is there a bug report URL?  What
> problem would a user notice?

There is a BZ: https://bugzilla.kernel.org/show_bug.cgi?id=48201

Unfortunately, the reporter hasn't confirmed that the bug is fixed,
although we're quite confident that it will be.

> This doesn't look critical enough to try to put in v3.7 (correct me if
> I'm wrong).  It's getting pretty late for the v3.8 merge window, but
> it looks like it qualifies as a bug fix that we would merge even after
> the merge window, so I'll put it in my -next branch headed for v3.8.
> Then it can be backported to v3.6 and v3.7.

I think that's OK.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices
  2012-11-28 22:43   ` Rafael J. Wysocki
@ 2012-12-05  0:06     ` Bjorn Helgaas
  0 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2012-12-05  0:06 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Huang Ying, linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki,
	Alan Stern

On Wed, Nov 28, 2012 at 3:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, November 28, 2012 03:25:59 PM Bjorn Helgaas wrote:
>> On Tue, Nov 20, 2012 at 1:08 AM, Huang Ying <ying.huang@intel.com> wrote:
>> > For unbound PCI devices, what we need is:
>> >
>> >  - Always in D0 state, because some devices does not work again after
>> >    being put into D3 by the PCI bus.
>> >
>> >  - In SUSPENDED state if allowed, so that the parent devices can still
>> >    be put into low power state.
>> >
>> > To satisfy these requirement, the runtime PM for the unbound PCI
>> > devices are disabled and set to SUSPENDED state.  One issue of this
>> > solution is that the PCI devices will be put into SUSPENDED state even
>> > if the SUSPENDED state is forbidden via the sysfs interface
>> > (.../power/control) of the device.  This is not an issue for most
>> > devices, because most PCI devices are not used at all if unbounded.
>> > But there are exceptions.  For example, unbound VGA card can be used
>> > for display, but suspend its parents make it stop working.
>> >
>> > To fix the issue, we keep the runtime PM enabled when the PCI devices
>> > are unbound.  But the runtime PM callbacks will do nothing if the PCI
>> > devices are unbound.  This way, we can put the PCI devices into
>> > SUSPENDED state without put the PCI devices into D3 state.
>>
>> Does this fix a reported problem?  Is there a bug report URL?  What
>> problem would a user notice?
>
> There is a BZ: https://bugzilla.kernel.org/show_bug.cgi?id=48201

I added the BZ link and put this in my -next branch.  Thanks!

Bjorn

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-12-05  0:06 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-20  8:08 [PATCH] PCI/PM: Keep runtime PM enabled for unbound PCI devices Huang Ying
2012-11-20 16:04 ` Alan Stern
2012-11-20 21:38 ` Rafael J. Wysocki
2012-11-28 22:25 ` Bjorn Helgaas
2012-11-28 22:43   ` Rafael J. Wysocki
2012-12-05  0:06     ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).