Received-SPF: pass (google.com: domain of linux-kernel+bounces-44052-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 30 Jan 2024 08:36:50 +0100
From: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Linux PM <linux-pm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>
Subject: Re: [PATCH v2 07/10] PM: sleep: stats: Log errors right after
 running suspend callbacks
Message-ID: <ZbinEuC6IZqjDIpf@linux.intel.com>
References: <5770175.DvuYhMxLoT@kreacher>
 <2200952.Mh6RI2rZIc@kreacher>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2200952.Mh6RI2rZIc@kreacher>

On Mon, Jan 29, 2024 at 05:25:48PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The error logging and failure statistics updates are carried out in two
> places in each system-wide device suspend phase, which is unnecessary
> code duplication, so do that in one place in each phase, right after
> invoking device suspend callbacks.
> 
> While at it, add "noirq" or "late" to the "async" string printed when
> the failing device callback in the "noirq" or "late" suspend phase,
> respectively, was run asynchronously.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/base/power/main.c |   49 ++++++++++++----------------------------------
>  1 file changed, 13 insertions(+), 36 deletions(-)
> 
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -1244,6 +1244,8 @@ Run:
>  	error = dpm_run_callback(callback, dev, state, info);
>  	if (error) {
>  		async_error = error;
> +		dpm_save_failed_dev(dev_name(dev));
> +		pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
>  		goto Complete;
>  	}
>  
> @@ -1273,14 +1275,8 @@ Complete:
>  static void async_suspend_noirq(void *data, async_cookie_t cookie)
>  {
>  	struct device *dev = data;
> -	int error;
> -
> -	error = __device_suspend_noirq(dev, pm_transition, true);
> -	if (error) {
> -		dpm_save_failed_dev(dev_name(dev));
> -		pm_dev_err(dev, pm_transition, " async", error);
> -	}
>  
> +	__device_suspend_noirq(dev, pm_transition, true);
>  	put_device(dev);
>  }
>  
> @@ -1312,12 +1308,8 @@ static int dpm_noirq_suspend_devices(pm_
>  
>  		mutex_lock(&dpm_list_mtx);
>  
> -		if (error) {
> -			pm_dev_err(dev, state, " noirq", error);
> -			dpm_save_failed_dev(dev_name(dev));
> -		} else if (!list_empty(&dev->power.entry)) {
> +		if (!error && !list_empty(&dev->power.entry))
>  			list_move(&dev->power.entry, &dpm_noirq_list);
> -		}
>  
>  		mutex_unlock(&dpm_list_mtx);
>  
> @@ -1437,6 +1429,8 @@ Run:
>  	error = dpm_run_callback(callback, dev, state, info);
>  	if (error) {
>  		async_error = error;
> +		dpm_save_failed_dev(dev_name(dev));
> +		pm_dev_err(dev, state, async ? " async late" : " late", error);
>  		goto Complete;
>  	}
>  	dpm_propagate_wakeup_to_parent(dev);
> @@ -1453,13 +1447,8 @@ Complete:
>  static void async_suspend_late(void *data, async_cookie_t cookie)
>  {
>  	struct device *dev = data;
> -	int error;
>  
> -	error = __device_suspend_late(dev, pm_transition, true);
> -	if (error) {
> -		dpm_save_failed_dev(dev_name(dev));
> -		pm_dev_err(dev, pm_transition, " async", error);
> -	}
> +	__device_suspend_late(dev, pm_transition, true);
>  	put_device(dev);
>  }
>  
> @@ -1500,11 +1489,6 @@ int dpm_suspend_late(pm_message_t state)
>  		if (!list_empty(&dev->power.entry))
>  			list_move(&dev->power.entry, &dpm_late_early_list);
>  
> -		if (error) {
> -			pm_dev_err(dev, state, " late", error);
> -			dpm_save_failed_dev(dev_name(dev));
> -		}
> -
>  		mutex_unlock(&dpm_list_mtx);
>  
>  		put_device(dev);
> @@ -1719,8 +1703,11 @@ static int __device_suspend(struct devic
>  	dpm_watchdog_clear(&wd);
>  
>   Complete:
> -	if (error)
> +	if (error) {
>  		async_error = error;
> +		dpm_save_failed_dev(dev_name(dev));
> +		pm_dev_err(dev, state, async ? " async" : "", error);
> +	}
>  
>  	complete_all(&dev->power.completion);
>  	TRACE_SUSPEND(error);
> @@ -1730,14 +1717,8 @@ static int __device_suspend(struct devic
>  static void async_suspend(void *data, async_cookie_t cookie)
>  {
>  	struct device *dev = data;
> -	int error;
> -
> -	error = __device_suspend(dev, pm_transition, true);
> -	if (error) {
> -		dpm_save_failed_dev(dev_name(dev));
> -		pm_dev_err(dev, pm_transition, " async", error);
> -	}
>  
> +	__device_suspend(dev, pm_transition, true);
>  	put_device(dev);
>  }
>  
> @@ -1778,12 +1759,8 @@ int dpm_suspend(pm_message_t state)
>  
>  		mutex_lock(&dpm_list_mtx);
>  
> -		if (error) {
> -			pm_dev_err(dev, state, "", error);
> -			dpm_save_failed_dev(dev_name(dev));
> -		} else if (!list_empty(&dev->power.entry)) {
> +		if (!error && !list_empty(&dev->power.entry))
>  			list_move(&dev->power.entry, &dpm_suspended_list);
> -		}
>  
>  		mutex_unlock(&dpm_list_mtx);
>  
> 
> 
>