Re: [PATCH v1 06/12] PM: sleep: stats: Define suspend_stats next to the code using it

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

On Mon, Jan 22, 2024 at 12:31:20PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is not necessary to define struct suspend_stats in a header file and the
> suspend_stats variable in the core device system-wide PM code.  They both
> can be defined in kernel/power/main.c, next to the sysfs and debugfs code
> accessing suspend_stats, which can be static.
> 
> Modify the code in question in accordance with the above observation and
> replace the static inline functions manipulating suspend_stats with
> regular ones defined in kernel/power/main.c.
> 
> While at it, move the enum suspend_stat_step to the end of suspend.h which
> is a more suitable place for it.
> 
> No intentional functional impact.
> 
> 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 |    1 
>  include/linux/suspend.h   |   70 +++++++++----------------------------------
>  kernel/power/main.c       |   74 +++++++++++++++++++++++++++++++++++++---------
>  kernel/power/power.h      |    2 +
>  kernel/power/suspend.c    |    7 ----
>  5 files changed, 80 insertions(+), 74 deletions(-)
> 
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -40,60 +40,6 @@ typedef int __bitwise suspend_state_t;
>  #define PM_SUSPEND_MIN		PM_SUSPEND_TO_IDLE
>  #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
>  
> -enum suspend_stat_step {
> -	SUSPEND_NONE = 0,
> -	SUSPEND_FREEZE,
> -	SUSPEND_PREPARE,
> -	SUSPEND_SUSPEND,
> -	SUSPEND_SUSPEND_LATE,
> -	SUSPEND_SUSPEND_NOIRQ,
> -	SUSPEND_RESUME_NOIRQ,
> -	SUSPEND_RESUME_EARLY,
> -	SUSPEND_RESUME,
> -	SUSPEND_NR_STEPS
> -};
> -
> -struct suspend_stats {
> -	unsigned int step_failures[SUSPEND_NR_STEPS];
> -	unsigned int fail;
> -#define	REC_FAILED_NUM	2
> -	int	last_failed_dev;
> -	char	failed_devs[REC_FAILED_NUM][40];
> -	int	last_failed_errno;
> -	int	errno[REC_FAILED_NUM];
> -	int	last_failed_step;
> -	u64	last_hw_sleep;
> -	u64	total_hw_sleep;
> -	u64	max_hw_sleep;
> -	enum suspend_stat_step	failed_steps[REC_FAILED_NUM];
> -};
> -
> -extern struct suspend_stats suspend_stats;
> -
> -static inline void dpm_save_failed_dev(const char *name)
> -{
> -	strscpy(suspend_stats.failed_devs[suspend_stats.last_failed_dev],
> -		name,
> -		sizeof(suspend_stats.failed_devs[0]));
> -	suspend_stats.last_failed_dev++;
> -	suspend_stats.last_failed_dev %= REC_FAILED_NUM;
> -}
> -
> -static inline void dpm_save_failed_errno(int err)
> -{
> -	suspend_stats.errno[suspend_stats.last_failed_errno] = err;
> -	suspend_stats.last_failed_errno++;
> -	suspend_stats.last_failed_errno %= REC_FAILED_NUM;
> -}
> -
> -static inline void dpm_save_failed_step(enum suspend_stat_step step)
> -{
> -	suspend_stats.step_failures[step]++;
> -	suspend_stats.failed_steps[suspend_stats.last_failed_step] = step;
> -	suspend_stats.last_failed_step++;
> -	suspend_stats.last_failed_step %= REC_FAILED_NUM;
> -}
> -
>  /**
>   * struct platform_suspend_ops - Callbacks for managing platform dependent
>   *	system sleep states.
> @@ -621,4 +567,20 @@ static inline void queue_up_suspend_work
>  
>  #endif /* !CONFIG_PM_AUTOSLEEP */
>  
> +enum suspend_stat_step {
> +	SUSPEND_NONE = 0,
> +	SUSPEND_FREEZE,
> +	SUSPEND_PREPARE,
> +	SUSPEND_SUSPEND,
> +	SUSPEND_SUSPEND_LATE,
> +	SUSPEND_SUSPEND_NOIRQ,
> +	SUSPEND_RESUME_NOIRQ,
> +	SUSPEND_RESUME_EARLY,
> +	SUSPEND_RESUME,
> +	SUSPEND_NR_STEPS
> +};
> +
> +void dpm_save_failed_dev(const char *name);
> +void dpm_save_failed_step(enum suspend_stat_step step);
> +
>  #endif /* _LINUX_SUSPEND_H */
> Index: linux-pm/kernel/power/main.c
> ===================================================================
> --- linux-pm.orig/kernel/power/main.c
> +++ linux-pm/kernel/power/main.c
> @@ -95,19 +95,6 @@ int unregister_pm_notifier(struct notifi
>  }
>  EXPORT_SYMBOL_GPL(unregister_pm_notifier);
>  
> -void pm_report_hw_sleep_time(u64 t)
> -{
> -	suspend_stats.last_hw_sleep = t;
> -	suspend_stats.total_hw_sleep += t;
> -}
> -EXPORT_SYMBOL_GPL(pm_report_hw_sleep_time);
> -
> -void pm_report_max_hw_sleep(u64 t)
> -{
> -	suspend_stats.max_hw_sleep = t;
> -}
> -EXPORT_SYMBOL_GPL(pm_report_max_hw_sleep);
> -
>  int pm_notifier_call_chain_robust(unsigned long val_up, unsigned long val_down)
>  {
>  	int ret;
> @@ -319,6 +306,67 @@ static ssize_t pm_test_store(struct kobj
>  power_attr(pm_test);
>  #endif /* CONFIG_PM_SLEEP_DEBUG */
>  
> +#define REC_FAILED_NUM	2
> +
> +struct suspend_stats {
> +	unsigned int step_failures[SUSPEND_NR_STEPS];
> +	unsigned int fail;
> +	int last_failed_dev;
> +	char failed_devs[REC_FAILED_NUM][40];
> +	int last_failed_errno;
> +	int errno[REC_FAILED_NUM];
> +	int last_failed_step;
> +	u64 last_hw_sleep;
> +	u64 total_hw_sleep;
> +	u64 max_hw_sleep;
> +	enum suspend_stat_step failed_steps[REC_FAILED_NUM];
> +};
> +
> +static struct suspend_stats suspend_stats;
> +
> +void dpm_save_failed_dev(const char *name)
> +{
> +	strscpy(suspend_stats.failed_devs[suspend_stats.last_failed_dev],
> +		name, sizeof(suspend_stats.failed_devs[0]));
> +	suspend_stats.last_failed_dev++;
> +	suspend_stats.last_failed_dev %= REC_FAILED_NUM;
> +}
> +
> +void dpm_save_failed_step(enum suspend_stat_step step)
> +{
> +	suspend_stats.step_failures[step]++;
> +	suspend_stats.failed_steps[suspend_stats.last_failed_step] = step;
> +	suspend_stats.last_failed_step++;
> +	suspend_stats.last_failed_step %= REC_FAILED_NUM;
> +}
> +
> +void dpm_save_errno(int err)
> +{
> +	if (!err) {
> +		suspend_stats.step_failures[SUSPEND_NONE]++;
> +		return;
> +	}
> +
> +	suspend_stats.fail++;
> +
> +	suspend_stats.errno[suspend_stats.last_failed_errno] = err;
> +	suspend_stats.last_failed_errno++;
> +	suspend_stats.last_failed_errno %= REC_FAILED_NUM;
> +}
> +
> +void pm_report_hw_sleep_time(u64 t)
> +{
> +	suspend_stats.last_hw_sleep = t;
> +	suspend_stats.total_hw_sleep += t;
> +}
> +EXPORT_SYMBOL_GPL(pm_report_hw_sleep_time);
> +
> +void pm_report_max_hw_sleep(u64 t)
> +{
> +	suspend_stats.max_hw_sleep = t;
> +}
> +EXPORT_SYMBOL_GPL(pm_report_max_hw_sleep);
> +
>  static const char * const suspend_step_names[] = {
>  	[SUSPEND_NONE] = "",
>  	[SUSPEND_FREEZE] = "freeze",
> Index: linux-pm/kernel/power/power.h
> ===================================================================
> --- linux-pm.orig/kernel/power/power.h
> +++ linux-pm/kernel/power/power.h
> @@ -327,3 +327,5 @@ static inline void pm_sleep_enable_secon
>  	suspend_enable_secondary_cpus();
>  	cpuidle_resume();
>  }
> +
> +void dpm_save_errno(int err);
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -616,12 +616,7 @@ int pm_suspend(suspend_state_t state)
>  
>  	pr_info("suspend entry (%s)\n", mem_sleep_labels[state]);
>  	error = enter_state(state);
> -	if (error) {
> -		suspend_stats.fail++;
> -		dpm_save_failed_errno(error);
> -	} else {
> -		suspend_stats.step_failures[SUSPEND_NONE]++;
> -	}
> +	dpm_save_errno(error);
>  	pr_info("suspend exit\n");
>  	return error;
>  }
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -60,7 +60,6 @@ static LIST_HEAD(dpm_suspended_list);
>  static LIST_HEAD(dpm_late_early_list);
>  static LIST_HEAD(dpm_noirq_list);
>  
> -struct suspend_stats suspend_stats;
>  static DEFINE_MUTEX(dpm_list_mtx);
>  static pm_message_t pm_transition;
>  
> 
> 
> 
>