Re: [PATCH v4] PCI / PM: tune down RPM suspend error message with EBUSY and EAGAIN retval

[Bjorn Helgaas <helgaas@kernel.org>]

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