Received-SPF: pass (google.com: domain of linux-kernel+bounces-44018-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 30 Jan 2024 08:02:17 +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 02/10] PM: sleep: stats: Use an array of step failure
 counters
Message-ID: <Zbie+bepNv1xob3J@linux.intel.com>
References: <5770175.DvuYhMxLoT@kreacher>
 <2192653.irdbgypaU6@kreacher>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2192653.irdbgypaU6@kreacher>

On Mon, Jan 29, 2024 at 05:11:57PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of using a set of individual struct suspend_stats fields
> representing suspend step failure counters, use an array of counters
> indexed by enum suspend_stat_step for this purpose, which allows
> dpm_save_failed_step() to increment the appropriate counter
> automatically, so that its callers don't need to do that directly.
> 
> It also allows suspend_stats_show() to carry out a loop over the
> counters array to print their values.
> 
> Because the counters cannot become negative, use unsigned int for
> representing them.
> 
> The only user-observable impact of this change is a different
> ordering of entries in the suspend_stats debugfs file which is not
> expected to matter.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2:
>    * Use one cell less in suspend_stats.step_failures[] to avoid
>      introducing an unused array cell (Stanislaw).
> 
> @Stanislaw: This is different from setting SUSPEND_FREEZE to 0, because
> that would complicate printing in the sysfs attributes and the debugfs
> code, so I've not added the R-by.

LGTM.

Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> +	for (step = SUSPEND_FREEZE; step <= SUSPEND_NR_STEPS; step++)
> +		seq_printf(s, "failed_%s: %u\n", suspend_step_names[step],
> +			   suspend_stats.step_failures[step-1]);

Consider (in separate patch) removing SUSPEND_NONE from suspend_step_names[]
and use step-1 for it as well.

Regards
Stanislaw