All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
@ 2015-11-18  9:16 Imre Deak
  2015-11-18 10:16 ` kbuild test robot
  2015-11-18 10:56 ` [PATCH v2] " Imre Deak
  0 siblings, 2 replies; 18+ messages in thread
From: Imre Deak @ 2015-11-18  9:16 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Daniel Vetter, intel-gfx

The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
suspend hooks as errors, but they still show up as errors in dmesg. Tune
them down.

One problem caused by this was noticed by Daniel: the i915 driver
returns EAGAIN to signal a temporary failure to suspend and as a request
towards the RPM core for scheduling a suspend again. This is a normal
event, but the resulting error message flags a breakage during the
driver's automated testing which parses dmesg and picks up the error.

Reported-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/base/power/main.c |  7 +++++--
 drivers/pci/pci-driver.c  |  2 +-
 include/linux/pm.h        | 10 ++++++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 1710c26..39d2090 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1679,9 +1679,12 @@ int dpm_suspend_start(pm_message_t state)
 }
 EXPORT_SYMBOL_GPL(dpm_suspend_start);
 
-void __suspend_report_result(const char *function, void *fn, int ret)
+void __suspend_report_result(const char *function, void *fn, int ret,
+			     bool runtime_pm)
 {
-	if (ret)
+	if (runtime_pm && (ret == -EBUSY || ret == -EAGAIN))
+		printk(KERN_DEBUG "%s(): %pF returns %d\n", function, fn, ret);
+	else if (ret)
 		printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
 }
 EXPORT_SYMBOL_GPL(__suspend_report_result);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 108a311..9569572 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1142,7 +1142,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
 	pci_dev->state_saved = false;
 	pci_dev->no_d3cold = false;
 	error = pm->runtime_suspend(dev);
-	suspend_report_result(pm->runtime_suspend, error);
+	rpm_suspend_report_result(pm->runtime_suspend, error);
 	if (error)
 		return error;
 	if (!pci_dev->d3cold_allowed)
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 35d599e..f3dca18 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -702,11 +702,17 @@ extern int dpm_suspend_late(pm_message_t state);
 extern int dpm_suspend(pm_message_t state);
 extern int dpm_prepare(pm_message_t state);
 
-extern void __suspend_report_result(const char *function, void *fn, int ret);
+extern void __suspend_report_result(const char *function, void *fn, int ret,
+				    bool runtime_pm);
 
 #define suspend_report_result(fn, ret)					\
 	do {								\
-		__suspend_report_result(__func__, fn, ret);		\
+		__suspend_report_result(__func__, fn, ret, false);	\
+	} while (0)
+
+#define rpm_suspend_report_result(fn, ret)				\
+	do {								\
+		__suspend_report_result(__func__, fn, ret, true);	\
 	} while (0)
 
 extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-18  9:16 [PATCH] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval Imre Deak
@ 2015-11-18 10:16 ` kbuild test robot
  2015-11-18 10:56 ` [PATCH v2] " Imre Deak
  1 sibling, 0 replies; 18+ messages in thread
From: kbuild test robot @ 2015-11-18 10:16 UTC (permalink / raw)
  To: Imre Deak; +Cc: Daniel Vetter, intel-gfx, Rafael J. Wysocki, kbuild-all

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

Hi Imre,

[auto build test ERROR on pci/next]
[also build test ERROR on v4.4-rc1 next-20151118]

url:    https://github.com/0day-ci/linux/commits/Imre-Deak/PCI-PM-tune-down-RPM-suspend-error-message-with-EBUSY-and-EAGAIN-retval/20151118-171831
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-a0-201546 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/pci/pci-driver.c: In function 'pci_pm_runtime_suspend':
>> drivers/pci/pci-driver.c:1149:2: error: implicit declaration of function 'rpm_suspend_report_result' [-Werror=implicit-function-declaration]
     rpm_suspend_report_result(pm->runtime_suspend, error);
     ^
   cc1: some warnings being treated as errors

vim +/rpm_suspend_report_result +1149 drivers/pci/pci-driver.c

  1143		if (!pm || !pm->runtime_suspend)
  1144			return -ENOSYS;
  1145	
  1146		pci_dev->state_saved = false;
  1147		pci_dev->no_d3cold = false;
  1148		error = pm->runtime_suspend(dev);
> 1149		rpm_suspend_report_result(pm->runtime_suspend, error);
  1150		if (error)
  1151			return error;
  1152		if (!pci_dev->d3cold_allowed)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25191 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [PATCH v2] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-18  9:16 [PATCH] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval Imre Deak
  2015-11-18 10:16 ` kbuild test robot
@ 2015-11-18 10:56 ` Imre Deak
  2015-11-18 13:28   ` Imre Deak
  2015-11-27 18:17   ` [PATCH v3] " Imre Deak
  1 sibling, 2 replies; 18+ messages in thread
From: Imre Deak @ 2015-11-18 10:56 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Daniel Vetter, intel-gfx

The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
suspend hooks as errors, but they still show up as errors in dmesg. Tune
them down.

One problem caused by this was noticed by Daniel: the i915 driver
returns EAGAIN to signal a temporary failure to suspend and as a request
towards the RPM core for scheduling a suspend again. This is a normal
event, but the resulting error message flags a breakage during the
driver's automated testing which parses dmesg and picks up the error.

v2:
- fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)

Reported-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/base/power/main.c |  7 +++++--
 drivers/pci/pci-driver.c  |  2 +-
 include/linux/pm.h        | 11 +++++++++--
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 1710c26..39d2090 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1679,9 +1679,12 @@ int dpm_suspend_start(pm_message_t state)
 }
 EXPORT_SYMBOL_GPL(dpm_suspend_start);
 
-void __suspend_report_result(const char *function, void *fn, int ret)
+void __suspend_report_result(const char *function, void *fn, int ret,
+			     bool runtime_pm)
 {
-	if (ret)
+	if (runtime_pm && (ret == -EBUSY || ret == -EAGAIN))
+		printk(KERN_DEBUG "%s(): %pF returns %d\n", function, fn, ret);
+	else if (ret)
 		printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
 }
 EXPORT_SYMBOL_GPL(__suspend_report_result);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 108a311..9569572 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1142,7 +1142,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
 	pci_dev->state_saved = false;
 	pci_dev->no_d3cold = false;
 	error = pm->runtime_suspend(dev);
-	suspend_report_result(pm->runtime_suspend, error);
+	rpm_suspend_report_result(pm->runtime_suspend, error);
 	if (error)
 		return error;
 	if (!pci_dev->d3cold_allowed)
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 35d599e..54f37e3 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -702,11 +702,17 @@ extern int dpm_suspend_late(pm_message_t state);
 extern int dpm_suspend(pm_message_t state);
 extern int dpm_prepare(pm_message_t state);
 
-extern void __suspend_report_result(const char *function, void *fn, int ret);
+extern void __suspend_report_result(const char *function, void *fn, int ret,
+				    bool runtime_pm);
 
 #define suspend_report_result(fn, ret)					\
 	do {								\
-		__suspend_report_result(__func__, fn, ret);		\
+		__suspend_report_result(__func__, fn, ret, false);	\
+	} while (0)
+
+#define rpm_suspend_report_result(fn, ret)				\
+	do {								\
+		__suspend_report_result(__func__, fn, ret, true);	\
 	} while (0)
 
 extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);
@@ -744,6 +750,7 @@ static inline int dpm_suspend_start(pm_message_t state)
 }
 
 #define suspend_report_result(fn, ret)		do {} while (0)
+#define rpm_suspend_report_result(fn, ret)	do {} while (0)
 
 static inline int device_pm_wait_for_dev(struct device *a, struct device *b)
 {
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH v2] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-18 10:56 ` [PATCH v2] " Imre Deak
@ 2015-11-18 13:28   ` Imre Deak
  2015-11-18 14:19     ` Daniel Vetter
  2015-11-27 18:17   ` [PATCH v3] " Imre Deak
  1 sibling, 1 reply; 18+ messages in thread
From: Imre Deak @ 2015-11-18 13:28 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Daniel Vetter, intel-gfx

On ke, 2015-11-18 at 12:56 +0200, Imre Deak wrote:
> The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
> suspend hooks as errors, but they still show up as errors in dmesg. Tune
> them down.
> 
> One problem caused by this was noticed by Daniel: the i915 driver
> returns EAGAIN to signal a temporary failure to suspend and as a request
> towards the RPM core for scheduling a suspend again. This is a normal
> event, but the resulting error message flags a breakage during the
> driver's automated testing which parses dmesg and picks up the error.
> 
> v2:
> - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> 
> Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992

> ---
>  drivers/base/power/main.c |  7 +++++--
>  drivers/pci/pci-driver.c  |  2 +-
>  include/linux/pm.h        | 11 +++++++++--
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 1710c26..39d2090 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1679,9 +1679,12 @@ int dpm_suspend_start(pm_message_t state)
>  }
>  EXPORT_SYMBOL_GPL(dpm_suspend_start);
>  
> -void __suspend_report_result(const char *function, void *fn, int ret)
> +void __suspend_report_result(const char *function, void *fn, int ret,
> +			     bool runtime_pm)
>  {
> -	if (ret)
> +	if (runtime_pm && (ret == -EBUSY || ret == -EAGAIN))
> +		printk(KERN_DEBUG "%s(): %pF returns %d\n", function, fn, ret);
> +	else if (ret)
>  		printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
>  }
>  EXPORT_SYMBOL_GPL(__suspend_report_result);
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 108a311..9569572 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1142,7 +1142,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
>  	pci_dev->state_saved = false;
>  	pci_dev->no_d3cold = false;
>  	error = pm->runtime_suspend(dev);
> -	suspend_report_result(pm->runtime_suspend, error);
> +	rpm_suspend_report_result(pm->runtime_suspend, error);
>  	if (error)
>  		return error;
>  	if (!pci_dev->d3cold_allowed)
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 35d599e..54f37e3 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -702,11 +702,17 @@ extern int dpm_suspend_late(pm_message_t state);
>  extern int dpm_suspend(pm_message_t state);
>  extern int dpm_prepare(pm_message_t state);
>  
> -extern void __suspend_report_result(const char *function, void *fn, int ret);
> +extern void __suspend_report_result(const char *function, void *fn, int ret,
> +				    bool runtime_pm);
>  
>  #define suspend_report_result(fn, ret)					\
>  	do {								\
> -		__suspend_report_result(__func__, fn, ret);		\
> +		__suspend_report_result(__func__, fn, ret, false);	\
> +	} while (0)
> +
> +#define rpm_suspend_report_result(fn, ret)				\
> +	do {								\
> +		__suspend_report_result(__func__, fn, ret, true);	\
>  	} while (0)
>  
>  extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);
> @@ -744,6 +750,7 @@ static inline int dpm_suspend_start(pm_message_t state)
>  }
>  
>  #define suspend_report_result(fn, ret)		do {} while (0)
> +#define rpm_suspend_report_result(fn, ret)	do {} while (0)
>  
>  static inline int device_pm_wait_for_dev(struct device *a, struct device *b)
>  {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH v2] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-18 13:28   ` Imre Deak
@ 2015-11-18 14:19     ` Daniel Vetter
  2015-11-27 11:39       ` Jani Nikula
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2015-11-18 14:19 UTC (permalink / raw)
  To: Imre Deak; +Cc: Daniel Vetter, intel-gfx, Rafael J. Wysocki

On Wed, Nov 18, 2015 at 03:28:38PM +0200, Imre Deak wrote:
> On ke, 2015-11-18 at 12:56 +0200, Imre Deak wrote:
> > The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
> > suspend hooks as errors, but they still show up as errors in dmesg. Tune
> > them down.
> > 
> > One problem caused by this was noticed by Daniel: the i915 driver
> > returns EAGAIN to signal a temporary failure to suspend and as a request
> > towards the RPM core for scheduling a suspend again. This is a normal
> > event, but the resulting error message flags a breakage during the
> > driver's automated testing which parses dmesg and picks up the error.
> > 
> > v2:
> > - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> > 
> > Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Rafael, can you please pick this up for 4.4? The spurious KERN_ERR noise
in dmesg is causing a lot fo spurious fail in our (very recently put into
place) i915 CI system.

Thanks, Daniel

> 
> > ---
> >  drivers/base/power/main.c |  7 +++++--
> >  drivers/pci/pci-driver.c  |  2 +-
> >  include/linux/pm.h        | 11 +++++++++--
> >  3 files changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 1710c26..39d2090 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1679,9 +1679,12 @@ int dpm_suspend_start(pm_message_t state)
> >  }
> >  EXPORT_SYMBOL_GPL(dpm_suspend_start);
> >  
> > -void __suspend_report_result(const char *function, void *fn, int ret)
> > +void __suspend_report_result(const char *function, void *fn, int ret,
> > +			     bool runtime_pm)
> >  {
> > -	if (ret)
> > +	if (runtime_pm && (ret == -EBUSY || ret == -EAGAIN))
> > +		printk(KERN_DEBUG "%s(): %pF returns %d\n", function, fn, ret);
> > +	else if (ret)
> >  		printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
> >  }
> >  EXPORT_SYMBOL_GPL(__suspend_report_result);
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 108a311..9569572 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1142,7 +1142,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
> >  	pci_dev->state_saved = false;
> >  	pci_dev->no_d3cold = false;
> >  	error = pm->runtime_suspend(dev);
> > -	suspend_report_result(pm->runtime_suspend, error);
> > +	rpm_suspend_report_result(pm->runtime_suspend, error);
> >  	if (error)
> >  		return error;
> >  	if (!pci_dev->d3cold_allowed)
> > diff --git a/include/linux/pm.h b/include/linux/pm.h
> > index 35d599e..54f37e3 100644
> > --- a/include/linux/pm.h
> > +++ b/include/linux/pm.h
> > @@ -702,11 +702,17 @@ extern int dpm_suspend_late(pm_message_t state);
> >  extern int dpm_suspend(pm_message_t state);
> >  extern int dpm_prepare(pm_message_t state);
> >  
> > -extern void __suspend_report_result(const char *function, void *fn, int ret);
> > +extern void __suspend_report_result(const char *function, void *fn, int ret,
> > +				    bool runtime_pm);
> >  
> >  #define suspend_report_result(fn, ret)					\
> >  	do {								\
> > -		__suspend_report_result(__func__, fn, ret);		\
> > +		__suspend_report_result(__func__, fn, ret, false);	\
> > +	} while (0)
> > +
> > +#define rpm_suspend_report_result(fn, ret)				\
> > +	do {								\
> > +		__suspend_report_result(__func__, fn, ret, true);	\
> >  	} while (0)
> >  
> >  extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);
> > @@ -744,6 +750,7 @@ static inline int dpm_suspend_start(pm_message_t state)
> >  }
> >  
> >  #define suspend_report_result(fn, ret)		do {} while (0)
> > +#define rpm_suspend_report_result(fn, ret)	do {} while (0)
> >  
> >  static inline int device_pm_wait_for_dev(struct device *a, struct device *b)
> >  {

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH v2] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-18 14:19     ` Daniel Vetter
@ 2015-11-27 11:39       ` Jani Nikula
  2015-11-27 14:44           ` Rafael J. Wysocki
  0 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2015-11-27 11:39 UTC (permalink / raw)
  To: Daniel Vetter, Imre Deak
  Cc: Daniel Vetter, intel-gfx, Rafael J. Wysocki, Rafael J. Wysocki

On Wed, 18 Nov 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Nov 18, 2015 at 03:28:38PM +0200, Imre Deak wrote:
>> On ke, 2015-11-18 at 12:56 +0200, Imre Deak wrote:
>> > The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
>> > suspend hooks as errors, but they still show up as errors in dmesg. Tune
>> > them down.
>> > 
>> > One problem caused by this was noticed by Daniel: the i915 driver
>> > returns EAGAIN to signal a temporary failure to suspend and as a request
>> > towards the RPM core for scheduling a suspend again. This is a normal
>> > event, but the resulting error message flags a breakage during the
>> > driver's automated testing which parses dmesg and picks up the error.
>> > 
>> > v2:
>> > - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
>> > 
>> > Reported-by: Daniel Vetter <daniel.vetter@intel.com>
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Rafael, can you please pick this up for 4.4? The spurious KERN_ERR noise
> in dmesg is causing a lot fo spurious fail in our (very recently put into
> place) i915 CI system.

Rafael, ping.

BR,
Jani.


>
> Thanks, Daniel
>
>> 
>> > ---
>> >  drivers/base/power/main.c |  7 +++++--
>> >  drivers/pci/pci-driver.c  |  2 +-
>> >  include/linux/pm.h        | 11 +++++++++--
>> >  3 files changed, 15 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> > index 1710c26..39d2090 100644
>> > --- a/drivers/base/power/main.c
>> > +++ b/drivers/base/power/main.c
>> > @@ -1679,9 +1679,12 @@ int dpm_suspend_start(pm_message_t state)
>> >  }
>> >  EXPORT_SYMBOL_GPL(dpm_suspend_start);
>> >  
>> > -void __suspend_report_result(const char *function, void *fn, int ret)
>> > +void __suspend_report_result(const char *function, void *fn, int ret,
>> > +			     bool runtime_pm)
>> >  {
>> > -	if (ret)
>> > +	if (runtime_pm && (ret == -EBUSY || ret == -EAGAIN))
>> > +		printk(KERN_DEBUG "%s(): %pF returns %d\n", function, fn, ret);
>> > +	else if (ret)
>> >  		printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
>> >  }
>> >  EXPORT_SYMBOL_GPL(__suspend_report_result);
>> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> > index 108a311..9569572 100644
>> > --- a/drivers/pci/pci-driver.c
>> > +++ b/drivers/pci/pci-driver.c
>> > @@ -1142,7 +1142,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
>> >  	pci_dev->state_saved = false;
>> >  	pci_dev->no_d3cold = false;
>> >  	error = pm->runtime_suspend(dev);
>> > -	suspend_report_result(pm->runtime_suspend, error);
>> > +	rpm_suspend_report_result(pm->runtime_suspend, error);
>> >  	if (error)
>> >  		return error;
>> >  	if (!pci_dev->d3cold_allowed)
>> > diff --git a/include/linux/pm.h b/include/linux/pm.h
>> > index 35d599e..54f37e3 100644
>> > --- a/include/linux/pm.h
>> > +++ b/include/linux/pm.h
>> > @@ -702,11 +702,17 @@ extern int dpm_suspend_late(pm_message_t state);
>> >  extern int dpm_suspend(pm_message_t state);
>> >  extern int dpm_prepare(pm_message_t state);
>> >  
>> > -extern void __suspend_report_result(const char *function, void *fn, int ret);
>> > +extern void __suspend_report_result(const char *function, void *fn, int ret,
>> > +				    bool runtime_pm);
>> >  
>> >  #define suspend_report_result(fn, ret)					\
>> >  	do {								\
>> > -		__suspend_report_result(__func__, fn, ret);		\
>> > +		__suspend_report_result(__func__, fn, ret, false);	\
>> > +	} while (0)
>> > +
>> > +#define rpm_suspend_report_result(fn, ret)				\
>> > +	do {								\
>> > +		__suspend_report_result(__func__, fn, ret, true);	\
>> >  	} while (0)
>> >  
>> >  extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);
>> > @@ -744,6 +750,7 @@ static inline int dpm_suspend_start(pm_message_t state)
>> >  }
>> >  
>> >  #define suspend_report_result(fn, ret)		do {} while (0)
>> > +#define rpm_suspend_report_result(fn, ret)	do {} while (0)
>> >  
>> >  static inline int device_pm_wait_for_dev(struct device *a, struct device *b)
>> >  {

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-27 11:39       ` Jani Nikula
@ 2015-11-27 14:44           ` Rafael J. Wysocki
  0 siblings, 0 replies; 18+ messages in thread
From: Rafael J. Wysocki @ 2015-11-27 14:44 UTC (permalink / raw)
  To: Jani Nikula, Daniel Vetter, Imre Deak
  Cc: Daniel Vetter, intel-gfx, Rafael J. Wysocki, Linux PM, Linux PCI,
	Bjorn Helgaas

On 11/27/2015 12:39 PM, Jani Nikula wrote:
> On Wed, 18 Nov 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Nov 18, 2015 at 03:28:38PM +0200, Imre Deak wrote:
>>> On ke, 2015-11-18 at 12:56 +0200, Imre Deak wrote:
>>>> The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
>>>> suspend hooks as errors, but they still show up as errors in dmesg. Tune
>>>> them down.
>>>>
>>>> One problem caused by this was noticed by Daniel: the i915 driver
>>>> returns EAGAIN to signal a temporary failure to suspend and as a request
>>>> towards the RPM core for scheduling a suspend again. This is a normal
>>>> event, but the resulting error message flags a breakage during the
>>>> driver's automated testing which parses dmesg and picks up the error.
>>>>
>>>> v2:
>>>> - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
>>>>
>>>> Reported-by: Daniel Vetter <daniel.vetter@intel.com>
>>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Rafael, can you please pick this up for 4.4? The spurious KERN_ERR noise
>> in dmesg is causing a lot fo spurious fail in our (very recently put into
>> place) i915 CI system.
> Rafael, ping.

Well, so I'm not sure about this one.

And the question is ->

>>>> ---
>>>>   drivers/base/power/main.c |  7 +++++--
>>>>   drivers/pci/pci-driver.c  |  2 +-
>>>>   include/linux/pm.h        | 11 +++++++++--
>>>>   3 files changed, 15 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>>> index 1710c26..39d2090 100644
>>>> --- a/drivers/base/power/main.c
>>>> +++ b/drivers/base/power/main.c
>>>> @@ -1679,9 +1679,12 @@ int dpm_suspend_start(pm_message_t state)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(dpm_suspend_start);
>>>>   
>>>> -void __suspend_report_result(const char *function, void *fn, int ret)
>>>> +void __suspend_report_result(const char *function, void *fn, int ret,
>>>> +			     bool runtime_pm)
>>>>   {
>>>> -	if (ret)
>>>> +	if (runtime_pm && (ret == -EBUSY || ret == -EAGAIN))
>>>> +		printk(KERN_DEBUG "%s(): %pF returns %d\n", function, fn, ret);
>>>> +	else if (ret)
>>>>   		printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
>>>>   }

-> why you are adding overhead to this function, instead of -->

>>>>   EXPORT_SYMBOL_GPL(__suspend_report_result);
>>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>>>> index 108a311..9569572 100644
>>>> --- a/drivers/pci/pci-driver.c
>>>> +++ b/drivers/pci/pci-driver.c
>>>> @@ -1142,7 +1142,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
>>>>   	pci_dev->state_saved = false;
>>>>   	pci_dev->no_d3cold = false;
>>>>   	error = pm->runtime_suspend(dev);
>>>> -	suspend_report_result(pm->runtime_suspend, error);
>>>> +	rpm_suspend_report_result(pm->runtime_suspend, error);

--> replacing the suspend_report_result() above with a direct printk() 
in the if (error) block below.

Surely, suspend_report_result() was not designed with runtime PM in mind 
and it was a mistake to use it here.  It just seemed to do the right 
thing, but it clearly doesn't.

>>>>   	if (error)
>>>>   		return error;
>>>>   	if (!pci_dev->d3cold_allowed)
>>>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>>>> index 35d599e..54f37e3 100644
>>>> --- a/include/linux/pm.h
>>>> +++ b/include/linux/pm.h
>>>> @@ -702,11 +702,17 @@ extern int dpm_suspend_late(pm_message_t state);
>>>>   extern int dpm_suspend(pm_message_t state);
>>>>   extern int dpm_prepare(pm_message_t state);
>>>>   
>>>> -extern void __suspend_report_result(const char *function, void *fn, int ret);
>>>> +extern void __suspend_report_result(const char *function, void *fn, int ret,
>>>> +				    bool runtime_pm);
>>>>   
>>>>   #define suspend_report_result(fn, ret)					\
>>>>   	do {								\
>>>> -		__suspend_report_result(__func__, fn, ret);		\
>>>> +		__suspend_report_result(__func__, fn, ret, false);	\
>>>> +	} while (0)
>>>> +
>>>> +#define rpm_suspend_report_result(fn, ret)				\
>>>> +	do {								\
>>>> +		__suspend_report_result(__func__, fn, ret, true);	\
>>>>   	} while (0)
>>>>   
>>>>   extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);
>>>> @@ -744,6 +750,7 @@ static inline int dpm_suspend_start(pm_message_t state)
>>>>   }
>>>>   
>>>>   #define suspend_report_result(fn, ret)		do {} while (0)
>>>> +#define rpm_suspend_report_result(fn, ret)	do {} while (0)
>>>>   
>>>>   static inline int device_pm_wait_for_dev(struct device *a, struct device *b)
>>>>   {

BTW, if you're changing PM code, it is good to CC linux-pm too (now 
done) and if you're changing PCI code, it is mandatory to CC linux-pci 
and the PCI maintainer (now done too).

Thanks,
Rafael


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

* Re: [PATCH v2] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
@ 2015-11-27 14:44           ` Rafael J. Wysocki
  0 siblings, 0 replies; 18+ messages in thread
From: Rafael J. Wysocki @ 2015-11-27 14:44 UTC (permalink / raw)
  To: Jani Nikula, Daniel Vetter, Imre Deak
  Cc: Linux PM, Linux PCI, intel-gfx, Rafael J. Wysocki, Daniel Vetter,
	Bjorn Helgaas

On 11/27/2015 12:39 PM, Jani Nikula wrote:
> On Wed, 18 Nov 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Nov 18, 2015 at 03:28:38PM +0200, Imre Deak wrote:
>>> On ke, 2015-11-18 at 12:56 +0200, Imre Deak wrote:
>>>> The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
>>>> suspend hooks as errors, but they still show up as errors in dmesg. Tune
>>>> them down.
>>>>
>>>> One problem caused by this was noticed by Daniel: the i915 driver
>>>> returns EAGAIN to signal a temporary failure to suspend and as a request
>>>> towards the RPM core for scheduling a suspend again. This is a normal
>>>> event, but the resulting error message flags a breakage during the
>>>> driver's automated testing which parses dmesg and picks up the error.
>>>>
>>>> v2:
>>>> - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
>>>>
>>>> Reported-by: Daniel Vetter <daniel.vetter@intel.com>
>>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Rafael, can you please pick this up for 4.4? The spurious KERN_ERR noise
>> in dmesg is causing a lot fo spurious fail in our (very recently put into
>> place) i915 CI system.
> Rafael, ping.

Well, so I'm not sure about this one.

And the question is ->

>>>> ---
>>>>   drivers/base/power/main.c |  7 +++++--
>>>>   drivers/pci/pci-driver.c  |  2 +-
>>>>   include/linux/pm.h        | 11 +++++++++--
>>>>   3 files changed, 15 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>>> index 1710c26..39d2090 100644
>>>> --- a/drivers/base/power/main.c
>>>> +++ b/drivers/base/power/main.c
>>>> @@ -1679,9 +1679,12 @@ int dpm_suspend_start(pm_message_t state)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(dpm_suspend_start);
>>>>   
>>>> -void __suspend_report_result(const char *function, void *fn, int ret)
>>>> +void __suspend_report_result(const char *function, void *fn, int ret,
>>>> +			     bool runtime_pm)
>>>>   {
>>>> -	if (ret)
>>>> +	if (runtime_pm && (ret == -EBUSY || ret == -EAGAIN))
>>>> +		printk(KERN_DEBUG "%s(): %pF returns %d\n", function, fn, ret);
>>>> +	else if (ret)
>>>>   		printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
>>>>   }

-> why you are adding overhead to this function, instead of -->

>>>>   EXPORT_SYMBOL_GPL(__suspend_report_result);
>>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>>>> index 108a311..9569572 100644
>>>> --- a/drivers/pci/pci-driver.c
>>>> +++ b/drivers/pci/pci-driver.c
>>>> @@ -1142,7 +1142,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
>>>>   	pci_dev->state_saved = false;
>>>>   	pci_dev->no_d3cold = false;
>>>>   	error = pm->runtime_suspend(dev);
>>>> -	suspend_report_result(pm->runtime_suspend, error);
>>>> +	rpm_suspend_report_result(pm->runtime_suspend, error);

--> replacing the suspend_report_result() above with a direct printk() 
in the if (error) block below.

Surely, suspend_report_result() was not designed with runtime PM in mind 
and it was a mistake to use it here.  It just seemed to do the right 
thing, but it clearly doesn't.

>>>>   	if (error)
>>>>   		return error;
>>>>   	if (!pci_dev->d3cold_allowed)
>>>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>>>> index 35d599e..54f37e3 100644
>>>> --- a/include/linux/pm.h
>>>> +++ b/include/linux/pm.h
>>>> @@ -702,11 +702,17 @@ extern int dpm_suspend_late(pm_message_t state);
>>>>   extern int dpm_suspend(pm_message_t state);
>>>>   extern int dpm_prepare(pm_message_t state);
>>>>   
>>>> -extern void __suspend_report_result(const char *function, void *fn, int ret);
>>>> +extern void __suspend_report_result(const char *function, void *fn, int ret,
>>>> +				    bool runtime_pm);
>>>>   
>>>>   #define suspend_report_result(fn, ret)					\
>>>>   	do {								\
>>>> -		__suspend_report_result(__func__, fn, ret);		\
>>>> +		__suspend_report_result(__func__, fn, ret, false);	\
>>>> +	} while (0)
>>>> +
>>>> +#define rpm_suspend_report_result(fn, ret)				\
>>>> +	do {								\
>>>> +		__suspend_report_result(__func__, fn, ret, true);	\
>>>>   	} while (0)
>>>>   
>>>>   extern int device_pm_wait_for_dev(struct device *sub, struct device *dev);
>>>> @@ -744,6 +750,7 @@ static inline int dpm_suspend_start(pm_message_t state)
>>>>   }
>>>>   
>>>>   #define suspend_report_result(fn, ret)		do {} while (0)
>>>> +#define rpm_suspend_report_result(fn, ret)	do {} while (0)
>>>>   
>>>>   static inline int device_pm_wait_for_dev(struct device *a, struct device *b)
>>>>   {

BTW, if you're changing PM code, it is good to CC linux-pm too (now 
done) and if you're changing PCI code, it is mandatory to CC linux-pci 
and the PCI maintainer (now done too).

Thanks,
Rafael

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-27 14:44           ` Rafael J. Wysocki
  (?)
@ 2015-11-27 14:56           ` Imre Deak
  -1 siblings, 0 replies; 18+ messages in thread
From: Imre Deak @ 2015-11-27 14:56 UTC (permalink / raw)
  To: Rafael J. Wysocki, Jani Nikula, Daniel Vetter
  Cc: Daniel Vetter, intel-gfx, Rafael J. Wysocki, Linux PM, Linux PCI,
	Bjorn Helgaas

On pe, 2015-11-27 at 15:44 +0100, Rafael J. Wysocki wrote:
> On 11/27/2015 12:39 PM, Jani Nikula wrote:
> > On Wed, 18 Nov 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Wed, Nov 18, 2015 at 03:28:38PM +0200, Imre Deak wrote:
> > > > On ke, 2015-11-18 at 12:56 +0200, Imre Deak wrote:
> > > > > The runtime PM core doesn't treat EBUSY and EAGAIN retvals
> > > > > from the driver
> > > > > suspend hooks as errors, but they still show up as errors in
> > > > > dmesg. Tune
> > > > > them down.
> > > > > 
> > > > > One problem caused by this was noticed by Daniel: the i915
> > > > > driver
> > > > > returns EAGAIN to signal a temporary failure to suspend and
> > > > > as a request
> > > > > towards the RPM core for scheduling a suspend again. This is
> > > > > a normal
> > > > > event, but the resulting error message flags a breakage
> > > > > during the
> > > > > driver's automated testing which parses dmesg and picks up
> > > > > the error.
> > > > > 
> > > > > v2:
> > > > > - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> > > > > 
> > > > > Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > Rafael, can you please pick this up for 4.4? The spurious
> > > KERN_ERR noise
> > > in dmesg is causing a lot fo spurious fail in our (very recently
> > > put into
> > > place) i915 CI system.
> > Rafael, ping.
> 
> Well, so I'm not sure about this one.
> 
> And the question is ->
> 
> > > > > ---
> > > > >   drivers/base/power/main.c |  7 +++++--
> > > > >   drivers/pci/pci-driver.c  |  2 +-
> > > > >   include/linux/pm.h        | 11 +++++++++--
> > > > >   3 files changed, 15 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/base/power/main.c
> > > > > b/drivers/base/power/main.c
> > > > > index 1710c26..39d2090 100644
> > > > > --- a/drivers/base/power/main.c
> > > > > +++ b/drivers/base/power/main.c
> > > > > @@ -1679,9 +1679,12 @@ int dpm_suspend_start(pm_message_t
> > > > > state)
> > > > >   }
> > > > >   EXPORT_SYMBOL_GPL(dpm_suspend_start);
> > > > >   
> > > > > -void __suspend_report_result(const char *function, void *fn,
> > > > > int ret)
> > > > > +void __suspend_report_result(const char *function, void *fn,
> > > > > int ret,
> > > > > +			     bool runtime_pm)
> > > > >   {
> > > > > -	if (ret)
> > > > > +	if (runtime_pm && (ret == -EBUSY || ret == -EAGAIN))
> > > > > +		printk(KERN_DEBUG "%s(): %pF returns %d\n",
> > > > > function, fn, ret);
> > > > > +	else if (ret)
> > > > >   		printk(KERN_ERR "%s(): %pF returns %d\n",
> > > > > function, fn, ret);
> > > > >   }
> 
> -> why you are adding overhead to this function, instead of -->
> 
> > > > >   EXPORT_SYMBOL_GPL(__suspend_report_result);
> > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-
> > > > > driver.c
> > > > > index 108a311..9569572 100644
> > > > > --- a/drivers/pci/pci-driver.c
> > > > > +++ b/drivers/pci/pci-driver.c
> > > > > @@ -1142,7 +1142,7 @@ static int
> > > > > pci_pm_runtime_suspend(struct device *dev)
> > > > >   	pci_dev->state_saved = false;
> > > > >   	pci_dev->no_d3cold = false;
> > > > >   	error = pm->runtime_suspend(dev);
> > > > > -	suspend_report_result(pm->runtime_suspend, error);
> > > > > +	rpm_suspend_report_result(pm->runtime_suspend,
> > > > > error);
> 
> --> replacing the suspend_report_result() above with a direct
> printk() 
> in the if (error) block below.
> 
> Surely, suspend_report_result() was not designed with runtime PM in
> mind 
> and it was a mistake to use it here.  It just seemed to do the right 
> thing, but it clearly doesn't.

Ok, a helper like rpm_suspend_report_result() seemed like a good idea,
since handling -EBUSY and -EAGAIN error reporting will be the same for
callers of the pm->runtime_suspend hooks not just the PCI drivers. But
since the only user of this is the PCI core atm we can just add a
printk locally as you suggested. I'll follow up with v2.

> > > > >   	if (error)
> > > > >   		return error;
> > > > >   	if (!pci_dev->d3cold_allowed)
> > > > > diff --git a/include/linux/pm.h b/include/linux/pm.h
> > > > > index 35d599e..54f37e3 100644
> > > > > --- a/include/linux/pm.h
> > > > > +++ b/include/linux/pm.h
> > > > > @@ -702,11 +702,17 @@ extern int
> > > > > dpm_suspend_late(pm_message_t state);
> > > > >   extern int dpm_suspend(pm_message_t state);
> > > > >   extern int dpm_prepare(pm_message_t state);
> > > > >   
> > > > > -extern void __suspend_report_result(const char *function,
> > > > > void *fn, int ret);
> > > > > +extern void __suspend_report_result(const char *function,
> > > > > void *fn, int ret,
> > > > > +				    bool runtime_pm);
> > > > >   
> > > > >   #define suspend_report_result(fn, ret)			
> > > > > 		\
> > > > >   	do {						
> > > > > 		\
> > > > > -		__suspend_report_result(__func__, fn, ret);	
> > > > > 	\
> > > > > +		__suspend_report_result(__func__, fn, ret,
> > > > > false);	\
> > > > > +	} while (0)
> > > > > +
> > > > > +#define rpm_suspend_report_result(fn, ret)			
> > > > > 	\
> > > > > +	do {							
> > > > > 	\
> > > > > +		__suspend_report_result(__func__, fn, ret,
> > > > > true);	\
> > > > >   	} while (0)
> > > > >   
> > > > >   extern int device_pm_wait_for_dev(struct device *sub,
> > > > > struct device *dev);
> > > > > @@ -744,6 +750,7 @@ static inline int
> > > > > dpm_suspend_start(pm_message_t state)
> > > > >   }
> > > > >   
> > > > >   #define suspend_report_result(fn, ret)		do {}
> > > > > while (0)
> > > > > +#define rpm_suspend_report_result(fn, ret)	do {}
> > > > > while (0)
> > > > >   
> > > > >   static inline int device_pm_wait_for_dev(struct device *a,
> > > > > struct device *b)
> > > > >   {
> 
> BTW, if you're changing PM code, it is good to CC linux-pm too (now 
> done) and if you're changing PCI code, it is mandatory to CC linux-
> pci 
> and the PCI maintainer (now done too).

Sorry, I thought about it too after sending it. Will do so in the
future.

--Imre

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

* [PATCH v3] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-18 10:56 ` [PATCH v2] " Imre Deak
  2015-11-18 13:28   ` Imre Deak
@ 2015-11-27 18:17   ` Imre Deak
  2015-11-27 22:23     ` Rafael J. Wysocki
  2015-11-28  8:34     ` [PATCH v4] " Imre Deak
  1 sibling, 2 replies; 18+ messages in thread
From: Imre Deak @ 2015-11-27 18:17 UTC (permalink / raw)
  To: Bjorn Helgaas, Rafael J. Wysocki
  Cc: Jani Nikula, Daniel Vetter, Linux PM, intel-gfx, Linux PCI

The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
suspend hooks as errors, but they still show up as errors in dmesg. Tune
them down.

One problem caused by this was noticed by Daniel: the i915 driver
returns EAGAIN to signal a temporary failure to suspend and as a request
towards the RPM core for scheduling a suspend again. This is a normal
event, but the resulting error message flags a breakage during the
driver's automated testing which parses dmesg and picks up the error.

v2:
- fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
v3:
- instead of modifying the suspend_report_result() helper to disinguish
  between the runtime and system suspend case, inline the error
  printing, it's not used anywhere else (Rafael)

Reported-by: Daniel Vetter <daniel.vetter@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/pci/pci-driver.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 4446fcb..67eb4ac 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1146,9 +1146,20 @@ static int pci_pm_runtime_suspend(struct device *dev)
 	pci_dev->state_saved = false;
 	pci_dev->no_d3cold = false;
 	error = pm->runtime_suspend(dev);
-	suspend_report_result(pm->runtime_suspend, error);
-	if (error)
+	if (error) {
+		/*
+		 * -EBUSY and -EAGAIN is used to request the runtime PM core
+		 * to schedule a new suspend, so don't flag it as an error.
+		 */
+		if (error == -EBUSY || error == -EAGAIN)
+			printk(KERN_DEBUG "%s(): %pF returns %d\n", __func__,
+			       pm->runtime_suspend, error);
+		else
+			printk(KERN_ERR "%s(): %pF returns %d\n", __func__,
+			       pm->runtime_suspend, error);
+
 		return error;
+	}
 	if (!pci_dev->d3cold_allowed)
 		pci_dev->no_d3cold = true;
 
-- 
2.5.0


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

* Re: [PATCH v3] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-27 18:17   ` [PATCH v3] " Imre Deak
@ 2015-11-27 22:23     ` Rafael J. Wysocki
  2015-11-28  8:34     ` [PATCH v4] " Imre Deak
  1 sibling, 0 replies; 18+ messages in thread
From: Rafael J. Wysocki @ 2015-11-27 22:23 UTC (permalink / raw)
  To: Imre Deak
  Cc: Bjorn Helgaas, Rafael J. Wysocki, Jani Nikula, Daniel Vetter,
	Linux PM, intel-gfx, Linux PCI

On Fri, Nov 27, 2015 at 7:17 PM, Imre Deak <imre.deak@intel.com> wrote:
> The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
> suspend hooks as errors, but they still show up as errors in dmesg. Tune
> them down.
>
> One problem caused by this was noticed by Daniel: the i915 driver
> returns EAGAIN to signal a temporary failure to suspend and as a request
> towards the RPM core for scheduling a suspend again. This is a normal
> event, but the resulting error message flags a breakage during the
> driver's automated testing which parses dmesg and picks up the error.
>
> v2:
> - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> v3:
> - instead of modifying the suspend_report_result() helper to disinguish
>   between the runtime and system suspend case, inline the error
>   printing, it's not used anywhere else (Rafael)
>
> Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/pci/pci-driver.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 4446fcb..67eb4ac 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1146,9 +1146,20 @@ static int pci_pm_runtime_suspend(struct device *dev)
>         pci_dev->state_saved = false;
>         pci_dev->no_d3cold = false;
>         error = pm->runtime_suspend(dev);
> -       suspend_report_result(pm->runtime_suspend, error);
> -       if (error)
> +       if (error) {
> +               /*
> +                * -EBUSY and -EAGAIN is used to request the runtime PM core
> +                * to schedule a new suspend, so don't flag it as an error.
> +                */

KERN_DEBUG and KERN_ERR are log levels, not flags.

Besides, maybe we can use pr_err() and pr_debug() here?

> +               if (error == -EBUSY || error == -EAGAIN)
> +                       printk(KERN_DEBUG "%s(): %pF returns %d\n", __func__,
> +                              pm->runtime_suspend, error);
> +               else
> +                       printk(KERN_ERR "%s(): %pF returns %d\n", __func__,
> +                              pm->runtime_suspend, error);
> +
>                 return error;
> +       }
>         if (!pci_dev->d3cold_allowed)
>                 pci_dev->no_d3cold = true;
>
> --

Thanks,
Rafael

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

* [PATCH v4] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-27 18:17   ` [PATCH v3] " Imre Deak
  2015-11-27 22:23     ` Rafael J. Wysocki
@ 2015-11-28  8:34     ` Imre Deak
  2015-11-30  2:07       ` Rafael J. Wysocki
                         ` (2 more replies)
  1 sibling, 3 replies; 18+ messages in thread
From: Imre Deak @ 2015-11-28  8:34 UTC (permalink / raw)
  To: Bjorn Helgaas, Rafael J. Wysocki
  Cc: Jani Nikula, Daniel Vetter, intel-gfx, Linux PCI, Linux PM

The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
suspend hooks as errors, but they still show up as errors in dmesg. Tune
them down.

One problem caused by this was noticed by Daniel: the i915 driver
returns EAGAIN to signal a temporary failure to suspend and as a request
towards the RPM core for scheduling a suspend again. This is a normal
event, but the resulting error message flags a breakage during the
driver's automated testing which parses dmesg and picks up the error.

v2:
- fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
v3:
- instead of modifying the suspend_report_result() helper to disinguish
  between the runtime and system suspend case, inline the error
  printing, it's not used anywhere else (Rafael)
v4:
- don't refer to log levels as flags in code comment (Rafael)
- use pr_debug(), pr_err() instead of the corresponding printk() (Rafael)

Reported-by: Daniel Vetter <daniel.vetter@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/pci/pci-driver.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 4446fcb..32a9947 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1146,9 +1146,21 @@ static int pci_pm_runtime_suspend(struct device *dev)
 	pci_dev->state_saved = false;
 	pci_dev->no_d3cold = false;
 	error = pm->runtime_suspend(dev);
-	suspend_report_result(pm->runtime_suspend, error);
-	if (error)
+	if (error) {
+		/*
+		 * -EBUSY and -EAGAIN is used to request the runtime PM core
+		 * to schedule a new suspend, so log the event only with debug
+		 * log level.
+		 */
+		if (error == -EBUSY || error == -EAGAIN)
+			pr_debug("%s(): %pF returns %d\n", __func__,
+				 pm->runtime_suspend, error);
+		else
+			pr_err("%s(): %pF returns %d\n", __func__,
+			       pm->runtime_suspend, error);
+
 		return error;
+	}
 	if (!pci_dev->d3cold_allowed)
 		pci_dev->no_d3cold = true;
 
-- 
2.5.0


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

* Re: [PATCH v4] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-28  8:34     ` [PATCH v4] " Imre Deak
@ 2015-11-30  2:07       ` Rafael J. Wysocki
  2015-11-30 18:07       ` Bjorn Helgaas
  2015-11-30 19:02       ` [PATCH v5] PCI / PM: Tune down retryable runtime suspend error messages Imre Deak
  2 siblings, 0 replies; 18+ messages in thread
From: Rafael J. Wysocki @ 2015-11-30  2:07 UTC (permalink / raw)
  To: Imre Deak, Bjorn Helgaas
  Cc: Jani Nikula, Daniel Vetter, intel-gfx, Linux PCI, Linux PM

On Saturday, November 28, 2015 10:34:24 AM Imre Deak wrote:
> The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
> suspend hooks as errors, but they still show up as errors in dmesg. Tune
> them down.
> 
> One problem caused by this was noticed by Daniel: the i915 driver
> returns EAGAIN to signal a temporary failure to suspend and as a request
> towards the RPM core for scheduling a suspend again. This is a normal
> event, but the resulting error message flags a breakage during the
> driver's automated testing which parses dmesg and picks up the error.
> 
> v2:
> - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> v3:
> - instead of modifying the suspend_report_result() helper to disinguish
>   between the runtime and system suspend case, inline the error
>   printing, it's not used anywhere else (Rafael)
> v4:
> - don't refer to log levels as flags in code comment (Rafael)
> - use pr_debug(), pr_err() instead of the corresponding printk() (Rafael)
> 
> Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

This is fine by me.

Bjorn, any objections against applying it?  It reduces log noise quite
a bit for i915.

> ---
>  drivers/pci/pci-driver.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 4446fcb..32a9947 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1146,9 +1146,21 @@ static int pci_pm_runtime_suspend(struct device *dev)
>  	pci_dev->state_saved = false;
>  	pci_dev->no_d3cold = false;
>  	error = pm->runtime_suspend(dev);
> -	suspend_report_result(pm->runtime_suspend, error);
> -	if (error)
> +	if (error) {
> +		/*
> +		 * -EBUSY and -EAGAIN is used to request the runtime PM core
> +		 * to schedule a new suspend, so log the event only with debug
> +		 * log level.
> +		 */
> +		if (error == -EBUSY || error == -EAGAIN)
> +			pr_debug("%s(): %pF returns %d\n", __func__,
> +				 pm->runtime_suspend, error);
> +		else
> +			pr_err("%s(): %pF returns %d\n", __func__,
> +			       pm->runtime_suspend, error);
> +
>  		return error;
> +	}
>  	if (!pci_dev->d3cold_allowed)
>  		pci_dev->no_d3cold = true;
>  
> 

Thanks,
Rafael


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

* Re: [PATCH v4] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval
  2015-11-28  8:34     ` [PATCH v4] " Imre Deak
  2015-11-30  2:07       ` Rafael J. Wysocki
@ 2015-11-30 18:07       ` Bjorn Helgaas
  2015-11-30 19:02       ` [PATCH v5] PCI / PM: Tune down retryable runtime suspend error messages Imre Deak
  2 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2015-11-30 18:07 UTC (permalink / raw)
  To: Imre Deak
  Cc: Bjorn Helgaas, Rafael J. Wysocki, Jani Nikula, Daniel Vetter,
	intel-gfx, Linux PCI, Linux PM

Hi Imre,

> PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval

Please capitalize the first word, e.g., "Tune ..."

Maybe "Tune down non-fatal runtime suspend error messages" would be
more to the point.  Or maybe "retryable," since it sounds like that's
what's supposed to happen.


On Sat, Nov 28, 2015 at 10:34:24AM +0200, Imre Deak wrote:
> The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
> suspend hooks as errors, but they still show up as errors in dmesg. Tune
> them down.

I looked briefly for the place in the runtime PM core where we handle
EBUSY and EAGAIN specially, but I didn't find it.  A pointer would be
helpful.

> One problem caused by this was noticed by Daniel: the i915 driver
> returns EAGAIN to signal a temporary failure to suspend and as a request
> towards the RPM core for scheduling a suspend again. This is a normal
> event, but the resulting error message flags a breakage during the
> driver's automated testing which parses dmesg and picks up the error.
> 
> v2:
> - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> v3:
> - instead of modifying the suspend_report_result() helper to disinguish
>   between the runtime and system suspend case, inline the error
>   printing, it's not used anywhere else (Rafael)
> v4:
> - don't refer to log levels as flags in code comment (Rafael)
> - use pr_debug(), pr_err() instead of the corresponding printk() (Rafael)

The version history is very useful during development but not after
merging, so I prefer it to go after the "---" marker so it doesn't get
included in the permanent changelog.

> Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/pci/pci-driver.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 4446fcb..32a9947 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1146,9 +1146,21 @@ static int pci_pm_runtime_suspend(struct device *dev)
>  	pci_dev->state_saved = false;
>  	pci_dev->no_d3cold = false;
>  	error = pm->runtime_suspend(dev);
> -	suspend_report_result(pm->runtime_suspend, error);
> -	if (error)
> +	if (error) {
> +		/*
> +		 * -EBUSY and -EAGAIN is used to request the runtime PM core
> +		 * to schedule a new suspend, so log the event only with debug
> +		 * log level.
> +		 */
> +		if (error == -EBUSY || error == -EAGAIN)
> +			pr_debug("%s(): %pF returns %d\n", __func__,
> +				 pm->runtime_suspend, error);
> +		else
> +			pr_err("%s(): %pF returns %d\n", __func__,
> +			       pm->runtime_suspend, error);

This looks fine to me in principle.

We have the device pointer, so please use dev_printk(KERN_DEBUG) and
dev_err().  If you're OK with changing the semantics so the debug
message is only emitted when dynamically enabled, you could use
dev_dbg().

I don't know what a user is supposed to do with a message like:

  pci_pm_runtime_suspend(): ata_port_runtime_suspend+0x0/0x200 returns -12

It feels a little bit ... internal.  Using dev_err() will help anchor
it to a specific device.  It's not related to a specific PC in the
function, so maybe use %pf instead of %pF to get rid of the function
offset and size.  And the message doesn't say anything about what the
return code *means*; maybe something like:

  ata_piix 0000:00:14.0: can't suspend now (ata_port_runtime_suspend returned -11)
  ata_piix 0000:00:14.0: can't suspend (ata_port_runtime_suspend returned -12)

> +
>  		return error;
> +	}
>  	if (!pci_dev->d3cold_allowed)
>  		pci_dev->no_d3cold = true;
>  
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* [PATCH v5] PCI / PM: Tune down retryable runtime suspend error messages
  2015-11-28  8:34     ` [PATCH v4] " Imre Deak
  2015-11-30  2:07       ` Rafael J. Wysocki
  2015-11-30 18:07       ` Bjorn Helgaas
@ 2015-11-30 19:02       ` Imre Deak
  2015-12-02  1:54         ` Rafael J. Wysocki
  2 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2015-11-30 19:02 UTC (permalink / raw)
  To: Bjorn Helgaas, Rafael J. Wysocki
  Cc: Jani Nikula, Daniel Vetter, Linux PM, intel-gfx, Linux PCI

The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
suspend hooks as errors, but they still show up as errors in dmesg. Tune
them down. See rpm_suspend() for details of handling these return values.

Note that we use dev_dbg() for the retryable retvals, so after this
change you'll need either CONFIG_DYNAMIC_DEBUG or CONFIG_PCI_DEBUG
for them to show up in the log.

One problem caused by this was noticed by Daniel: the i915 driver
returns EAGAIN to signal a temporary failure to suspend and as a request
towards the RPM core for scheduling a suspend again. This is a normal
event, but the resulting error message flags a breakage during the
driver's automated testing which parses dmesg and picks up the error.

Reported-by: Daniel Vetter <daniel.vetter@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>

---

v2:
- fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
v3:
- instead of modifying the suspend_report_result() helper to disinguish
  between the runtime and system suspend case, inline the error
  printing, it's not used anywhere else (Rafael)
v4:
- don't refer to log levels as flags in code comment (Rafael)
- use pr_debug(), pr_err() instead of the corresponding printk() (Rafael)
v5:
- clarify commit message (Bjorn)
- use dev_dbg, dev_err instead of pr_debug, pr_err (Bjorn)
- use %pf in printk format instead of %pF (Bjorn)
- make the debug/error messages more meaningful (Bjorn)
---
 drivers/pci/pci-driver.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 4446fcb..d7ffd66 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1146,9 +1146,21 @@ static int pci_pm_runtime_suspend(struct device *dev)
 	pci_dev->state_saved = false;
 	pci_dev->no_d3cold = false;
 	error = pm->runtime_suspend(dev);
-	suspend_report_result(pm->runtime_suspend, error);
-	if (error)
+	if (error) {
+		/*
+		 * -EBUSY and -EAGAIN is used to request the runtime PM core
+		 * to schedule a new suspend, so log the event only with debug
+		 * log level.
+		 */
+		if (error == -EBUSY || error == -EAGAIN)
+			dev_dbg(dev, "can't suspend now (%pf returned %d)\n",
+				pm->runtime_suspend, error);
+		else
+			dev_err(dev, "can't suspend (%pf returned %d)\n",
+				pm->runtime_suspend, error);
+
 		return error;
+	}
 	if (!pci_dev->d3cold_allowed)
 		pci_dev->no_d3cold = true;
 
-- 
2.5.0


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

* Re: [PATCH v5] PCI / PM: Tune down retryable runtime suspend error messages
  2015-11-30 19:02       ` [PATCH v5] PCI / PM: Tune down retryable runtime suspend error messages Imre Deak
@ 2015-12-02  1:54         ` Rafael J. Wysocki
  2015-12-02  4:43             ` Bjorn Helgaas
  0 siblings, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2015-12-02  1:54 UTC (permalink / raw)
  To: Imre Deak, Bjorn Helgaas
  Cc: Jani Nikula, Daniel Vetter, Linux PM, intel-gfx, Linux PCI

On Monday, November 30, 2015 09:02:55 PM Imre Deak wrote:
> The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
> suspend hooks as errors, but they still show up as errors in dmesg. Tune
> them down. See rpm_suspend() for details of handling these return values.
> 
> Note that we use dev_dbg() for the retryable retvals, so after this
> change you'll need either CONFIG_DYNAMIC_DEBUG or CONFIG_PCI_DEBUG
> for them to show up in the log.
> 
> One problem caused by this was noticed by Daniel: the i915 driver
> returns EAGAIN to signal a temporary failure to suspend and as a request
> towards the RPM core for scheduling a suspend again. This is a normal
> event, but the resulting error message flags a breakage during the
> driver's automated testing which parses dmesg and picks up the error.
> 
> Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Hi Bjorn,

Are you going to handle this one or should I take care of it?

Rafael


> ---
> 
> v2:
> - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> v3:
> - instead of modifying the suspend_report_result() helper to disinguish
>   between the runtime and system suspend case, inline the error
>   printing, it's not used anywhere else (Rafael)
> v4:
> - don't refer to log levels as flags in code comment (Rafael)
> - use pr_debug(), pr_err() instead of the corresponding printk() (Rafael)
> v5:
> - clarify commit message (Bjorn)
> - use dev_dbg, dev_err instead of pr_debug, pr_err (Bjorn)
> - use %pf in printk format instead of %pF (Bjorn)
> - make the debug/error messages more meaningful (Bjorn)
> ---
>  drivers/pci/pci-driver.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 4446fcb..d7ffd66 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1146,9 +1146,21 @@ static int pci_pm_runtime_suspend(struct device *dev)
>  	pci_dev->state_saved = false;
>  	pci_dev->no_d3cold = false;
>  	error = pm->runtime_suspend(dev);
> -	suspend_report_result(pm->runtime_suspend, error);
> -	if (error)
> +	if (error) {
> +		/*
> +		 * -EBUSY and -EAGAIN is used to request the runtime PM core
> +		 * to schedule a new suspend, so log the event only with debug
> +		 * log level.
> +		 */
> +		if (error == -EBUSY || error == -EAGAIN)
> +			dev_dbg(dev, "can't suspend now (%pf returned %d)\n",
> +				pm->runtime_suspend, error);
> +		else
> +			dev_err(dev, "can't suspend (%pf returned %d)\n",
> +				pm->runtime_suspend, error);
> +
>  		return error;
> +	}
>  	if (!pci_dev->d3cold_allowed)
>  		pci_dev->no_d3cold = true;
>  
> 

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

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

* Re: [PATCH v5] PCI / PM: Tune down retryable runtime suspend error messages
  2015-12-02  1:54         ` Rafael J. Wysocki
@ 2015-12-02  4:43             ` Bjorn Helgaas
  0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2015-12-02  4:43 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Imre Deak, Bjorn Helgaas, Jani Nikula, Daniel Vetter, Linux PM,
	intel-gfx, Linux PCI

On Wed, Dec 02, 2015 at 02:54:45AM +0100, Rafael J. Wysocki wrote:
> On Monday, November 30, 2015 09:02:55 PM Imre Deak wrote:
> > The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
> > suspend hooks as errors, but they still show up as errors in dmesg. Tune
> > them down. See rpm_suspend() for details of handling these return values.
> > 
> > Note that we use dev_dbg() for the retryable retvals, so after this
> > change you'll need either CONFIG_DYNAMIC_DEBUG or CONFIG_PCI_DEBUG
> > for them to show up in the log.
> > 
> > One problem caused by this was noticed by Daniel: the i915 driver
> > returns EAGAIN to signal a temporary failure to suspend and as a request
> > towards the RPM core for scheduling a suspend again. This is a normal
> > event, but the resulting error message flags a breakage during the
> > driver's automated testing which parses dmesg and picks up the error.
> > 
> > Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
> > CC: Bjorn Helgaas <bhelgaas@google.com>
> > CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Hi Bjorn,
> 
> Are you going to handle this one or should I take care of it?

Why don't you take it, since you're the PM guru :)

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks for all your work, Imre!

> > ---
> > 
> > v2:
> > - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> > v3:
> > - instead of modifying the suspend_report_result() helper to disinguish
> >   between the runtime and system suspend case, inline the error
> >   printing, it's not used anywhere else (Rafael)
> > v4:
> > - don't refer to log levels as flags in code comment (Rafael)
> > - use pr_debug(), pr_err() instead of the corresponding printk() (Rafael)
> > v5:
> > - clarify commit message (Bjorn)
> > - use dev_dbg, dev_err instead of pr_debug, pr_err (Bjorn)
> > - use %pf in printk format instead of %pF (Bjorn)
> > - make the debug/error messages more meaningful (Bjorn)
> > ---
> >  drivers/pci/pci-driver.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 4446fcb..d7ffd66 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1146,9 +1146,21 @@ static int pci_pm_runtime_suspend(struct device *dev)
> >  	pci_dev->state_saved = false;
> >  	pci_dev->no_d3cold = false;
> >  	error = pm->runtime_suspend(dev);
> > -	suspend_report_result(pm->runtime_suspend, error);
> > -	if (error)
> > +	if (error) {
> > +		/*
> > +		 * -EBUSY and -EAGAIN is used to request the runtime PM core
> > +		 * to schedule a new suspend, so log the event only with debug
> > +		 * log level.
> > +		 */
> > +		if (error == -EBUSY || error == -EAGAIN)
> > +			dev_dbg(dev, "can't suspend now (%pf returned %d)\n",
> > +				pm->runtime_suspend, error);
> > +		else
> > +			dev_err(dev, "can't suspend (%pf returned %d)\n",
> > +				pm->runtime_suspend, error);
> > +
> >  		return error;
> > +	}
> >  	if (!pci_dev->d3cold_allowed)
> >  		pci_dev->no_d3cold = true;
> >  
> > 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [PATCH v5] PCI / PM: Tune down retryable runtime suspend error messages
@ 2015-12-02  4:43             ` Bjorn Helgaas
  0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2015-12-02  4:43 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux PM, Jani Nikula, Daniel Vetter, Linux PCI, Bjorn Helgaas,
	intel-gfx

On Wed, Dec 02, 2015 at 02:54:45AM +0100, Rafael J. Wysocki wrote:
> On Monday, November 30, 2015 09:02:55 PM Imre Deak wrote:
> > The runtime PM core doesn't treat EBUSY and EAGAIN retvals from the driver
> > suspend hooks as errors, but they still show up as errors in dmesg. Tune
> > them down. See rpm_suspend() for details of handling these return values.
> > 
> > Note that we use dev_dbg() for the retryable retvals, so after this
> > change you'll need either CONFIG_DYNAMIC_DEBUG or CONFIG_PCI_DEBUG
> > for them to show up in the log.
> > 
> > One problem caused by this was noticed by Daniel: the i915 driver
> > returns EAGAIN to signal a temporary failure to suspend and as a request
> > towards the RPM core for scheduling a suspend again. This is a normal
> > event, but the resulting error message flags a breakage during the
> > driver's automated testing which parses dmesg and picks up the error.
> > 
> > Reported-by: Daniel Vetter <daniel.vetter@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92992
> > CC: Bjorn Helgaas <bhelgaas@google.com>
> > CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Hi Bjorn,
> 
> Are you going to handle this one or should I take care of it?

Why don't you take it, since you're the PM guru :)

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks for all your work, Imre!

> > ---
> > 
> > v2:
> > - fix compile breake when CONFIG_PM_SLEEP=n (0-day builder)
> > v3:
> > - instead of modifying the suspend_report_result() helper to disinguish
> >   between the runtime and system suspend case, inline the error
> >   printing, it's not used anywhere else (Rafael)
> > v4:
> > - don't refer to log levels as flags in code comment (Rafael)
> > - use pr_debug(), pr_err() instead of the corresponding printk() (Rafael)
> > v5:
> > - clarify commit message (Bjorn)
> > - use dev_dbg, dev_err instead of pr_debug, pr_err (Bjorn)
> > - use %pf in printk format instead of %pF (Bjorn)
> > - make the debug/error messages more meaningful (Bjorn)
> > ---
> >  drivers/pci/pci-driver.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 4446fcb..d7ffd66 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1146,9 +1146,21 @@ static int pci_pm_runtime_suspend(struct device *dev)
> >  	pci_dev->state_saved = false;
> >  	pci_dev->no_d3cold = false;
> >  	error = pm->runtime_suspend(dev);
> > -	suspend_report_result(pm->runtime_suspend, error);
> > -	if (error)
> > +	if (error) {
> > +		/*
> > +		 * -EBUSY and -EAGAIN is used to request the runtime PM core
> > +		 * to schedule a new suspend, so log the event only with debug
> > +		 * log level.
> > +		 */
> > +		if (error == -EBUSY || error == -EAGAIN)
> > +			dev_dbg(dev, "can't suspend now (%pf returned %d)\n",
> > +				pm->runtime_suspend, error);
> > +		else
> > +			dev_err(dev, "can't suspend (%pf returned %d)\n",
> > +				pm->runtime_suspend, error);
> > +
> >  		return error;
> > +	}
> >  	if (!pci_dev->d3cold_allowed)
> >  		pci_dev->no_d3cold = true;
> >  
> > 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

end of thread, other threads:[~2015-12-02  4:43 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-18  9:16 [PATCH] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval Imre Deak
2015-11-18 10:16 ` kbuild test robot
2015-11-18 10:56 ` [PATCH v2] " Imre Deak
2015-11-18 13:28   ` Imre Deak
2015-11-18 14:19     ` Daniel Vetter
2015-11-27 11:39       ` Jani Nikula
2015-11-27 14:44         ` [Intel-gfx] " Rafael J. Wysocki
2015-11-27 14:44           ` Rafael J. Wysocki
2015-11-27 14:56           ` [Intel-gfx] " Imre Deak
2015-11-27 18:17   ` [PATCH v3] " Imre Deak
2015-11-27 22:23     ` Rafael J. Wysocki
2015-11-28  8:34     ` [PATCH v4] " Imre Deak
2015-11-30  2:07       ` Rafael J. Wysocki
2015-11-30 18:07       ` Bjorn Helgaas
2015-11-30 19:02       ` [PATCH v5] PCI / PM: Tune down retryable runtime suspend error messages Imre Deak
2015-12-02  1:54         ` Rafael J. Wysocki
2015-12-02  4:43           ` Bjorn Helgaas
2015-12-02  4:43             ` Bjorn Helgaas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.