| Message ID | 3290637.44csPzL39Z@kreacher (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PM: sleep: Fix up suspend stats handling and clean up core code | expand |
On Mon, Jan 22, 2024 at 12:29:11PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The first (index 0) cell of the step_failures[] array in struct > suspend_stats introduced previously can be used as a counter of > successful suspend-resume cycles instead of the separate "success" > field in it, so do that. > > While at it, change the type of the "fail" field in struct > suspend_stats to unsigned int, because it cannot be negative. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > include/linux/suspend.h | 3 +-- > kernel/power/main.c | 9 +++++---- > kernel/power/suspend.c | 2 +- > 3 files changed, 7 insertions(+), 7 deletions(-) > > Index: linux-pm/include/linux/suspend.h > =================================================================== > --- linux-pm.orig/include/linux/suspend.h > +++ linux-pm/include/linux/suspend.h > @@ -55,8 +55,7 @@ enum suspend_stat_step { > > struct suspend_stats { > unsigned int step_failures[SUSPEND_NR_STEPS]; > - int success; <snip> > - suspend_stats.success, suspend_stats.fail); > + seq_printf(s, "success: %u\nfail: %u\n", > + suspend_stats.step_failures[SUSPEND_NONE], > + suspend_stats.fail); > > for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++) > seq_printf(s, "failed_%s: %u\n", suspend_step_names[step], > Index: linux-pm/kernel/power/suspend.c > =================================================================== > --- linux-pm.orig/kernel/power/suspend.c > +++ linux-pm/kernel/power/suspend.c > @@ -620,7 +620,7 @@ int pm_suspend(suspend_state_t state) > suspend_stats.fail++; > dpm_save_failed_errno(error); > } else { > - suspend_stats.success++; > + suspend_stats.step_failures[SUSPEND_NONE]++; This looks confusing for me. I think would be better keep success field and just remove SUSPEND_NONE from the suspend_stat_step and suspend_stat_names. Actually do not introduce it, SUSPEND_NONE does not seems to be necessary (SUSPEND_FREEZE can be 0). Regards Stanislaw
On Thu, Jan 25, 2024 at 8:52 AM Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> wrote: > > On Mon, Jan 22, 2024 at 12:29:11PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The first (index 0) cell of the step_failures[] array in struct > > suspend_stats introduced previously can be used as a counter of > > successful suspend-resume cycles instead of the separate "success" > > field in it, so do that. > > > > While at it, change the type of the "fail" field in struct > > suspend_stats to unsigned int, because it cannot be negative. > > > > No intentional functional impact. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > include/linux/suspend.h | 3 +-- > > kernel/power/main.c | 9 +++++---- > > kernel/power/suspend.c | 2 +- > > 3 files changed, 7 insertions(+), 7 deletions(-) > > > > Index: linux-pm/include/linux/suspend.h > > =================================================================== > > --- linux-pm.orig/include/linux/suspend.h > > +++ linux-pm/include/linux/suspend.h > > @@ -55,8 +55,7 @@ enum suspend_stat_step { > > > > struct suspend_stats { > > unsigned int step_failures[SUSPEND_NR_STEPS]; > > - int success; > <snip> > > - suspend_stats.success, suspend_stats.fail); > > + seq_printf(s, "success: %u\nfail: %u\n", > > + suspend_stats.step_failures[SUSPEND_NONE], > > + suspend_stats.fail); > > > > for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++) > > seq_printf(s, "failed_%s: %u\n", suspend_step_names[step], > > Index: linux-pm/kernel/power/suspend.c > > =================================================================== > > --- linux-pm.orig/kernel/power/suspend.c > > +++ linux-pm/kernel/power/suspend.c > > @@ -620,7 +620,7 @@ int pm_suspend(suspend_state_t state) > > suspend_stats.fail++; > > dpm_save_failed_errno(error); > > } else { > > - suspend_stats.success++; > > + suspend_stats.step_failures[SUSPEND_NONE]++; > > This looks confusing for me. I think would be better keep > success field and just remove SUSPEND_NONE from the > suspend_stat_step and suspend_stat_names. Actually do > not introduce it, SUSPEND_NONE does not seems to be necessary > (SUSPEND_FREEZE can be 0). OK I'll need to rearrange the series for that somewhat except for the first two patches. I guess it's OK to retain the R-by tags? Thanks for all of the reviews!
On Thu, Jan 25, 2024 at 04:11:08PM +0100, Rafael J. Wysocki wrote: > On Thu, Jan 25, 2024 at 8:52 AM Stanislaw Gruszka > <stanislaw.gruszka@linux.intel.com> wrote: > > > > On Mon, Jan 22, 2024 at 12:29:11PM +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The first (index 0) cell of the step_failures[] array in struct > > > suspend_stats introduced previously can be used as a counter of > > > successful suspend-resume cycles instead of the separate "success" > > > field in it, so do that. > > > > > > While at it, change the type of the "fail" field in struct > > > suspend_stats to unsigned int, because it cannot be negative. > > > > > > No intentional functional impact. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > include/linux/suspend.h | 3 +-- > > > kernel/power/main.c | 9 +++++---- > > > kernel/power/suspend.c | 2 +- > > > 3 files changed, 7 insertions(+), 7 deletions(-) > > > > > > Index: linux-pm/include/linux/suspend.h > > > =================================================================== > > > --- linux-pm.orig/include/linux/suspend.h > > > +++ linux-pm/include/linux/suspend.h > > > @@ -55,8 +55,7 @@ enum suspend_stat_step { > > > > > > struct suspend_stats { > > > unsigned int step_failures[SUSPEND_NR_STEPS]; > > > - int success; > > <snip> > > > - suspend_stats.success, suspend_stats.fail); > > > + seq_printf(s, "success: %u\nfail: %u\n", > > > + suspend_stats.step_failures[SUSPEND_NONE], > > > + suspend_stats.fail); > > > > > > for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++) > > > seq_printf(s, "failed_%s: %u\n", suspend_step_names[step], > > > Index: linux-pm/kernel/power/suspend.c > > > =================================================================== > > > --- linux-pm.orig/kernel/power/suspend.c > > > +++ linux-pm/kernel/power/suspend.c > > > @@ -620,7 +620,7 @@ int pm_suspend(suspend_state_t state) > > > suspend_stats.fail++; > > > dpm_save_failed_errno(error); > > > } else { > > > - suspend_stats.success++; > > > + suspend_stats.step_failures[SUSPEND_NONE]++; > > > > This looks confusing for me. I think would be better keep > > success field and just remove SUSPEND_NONE from the > > suspend_stat_step and suspend_stat_names. Actually do > > not introduce it, SUSPEND_NONE does not seems to be necessary > > (SUSPEND_FREEZE can be 0). > > OK > > I'll need to rearrange the series for that somewhat except for the > first two patches. I wouldn't mind to skip this change and just remove SUSPEND_NONE in separate patch. > I guess it's OK to retain the R-by tags? Yes, is OK. Regards Stanislaw
Index: linux-pm/include/linux/suspend.h =================================================================== --- linux-pm.orig/include/linux/suspend.h +++ linux-pm/include/linux/suspend.h @@ -55,8 +55,7 @@ enum suspend_stat_step { struct suspend_stats { unsigned int step_failures[SUSPEND_NR_STEPS]; - int success; - int fail; + unsigned int fail; #define REC_FAILED_NUM 2 int last_failed_dev; char failed_devs[REC_FAILED_NUM][40]; Index: linux-pm/kernel/power/main.c =================================================================== --- linux-pm.orig/kernel/power/main.c +++ linux-pm/kernel/power/main.c @@ -339,8 +339,7 @@ static ssize_t _name##_show(struct kobje } \ static struct kobj_attribute _name = __ATTR_RO(_name) -suspend_attr(success, "%d\n"); -suspend_attr(fail, "%d\n"); +suspend_attr(fail, "%u\n"); suspend_attr(last_hw_sleep, "%llu\n"); suspend_attr(total_hw_sleep, "%llu\n"); suspend_attr(max_hw_sleep, "%llu\n"); @@ -354,6 +353,7 @@ static ssize_t _name##_show(struct kobje } \ static struct kobj_attribute _name = __ATTR_RO(_name) +suspend_step_attr(success, SUSPEND_NONE); suspend_step_attr(failed_freeze, SUSPEND_FREEZE); suspend_step_attr(failed_prepare, SUSPEND_PREPARE); suspend_step_attr(failed_suspend, SUSPEND_SUSPEND); @@ -458,8 +458,9 @@ static int suspend_stats_show(struct seq last_step = suspend_stats.last_failed_step + REC_FAILED_NUM - 1; last_step %= REC_FAILED_NUM; - seq_printf(s, "success: %d\nfail: %d\n", - suspend_stats.success, suspend_stats.fail); + seq_printf(s, "success: %u\nfail: %u\n", + suspend_stats.step_failures[SUSPEND_NONE], + suspend_stats.fail); for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++) seq_printf(s, "failed_%s: %u\n", suspend_step_names[step], Index: linux-pm/kernel/power/suspend.c =================================================================== --- linux-pm.orig/kernel/power/suspend.c +++ linux-pm/kernel/power/suspend.c @@ -620,7 +620,7 @@ int pm_suspend(suspend_state_t state) suspend_stats.fail++; dpm_save_failed_errno(error); } else { - suspend_stats.success++; + suspend_stats.step_failures[SUSPEND_NONE]++; } pr_info("suspend exit\n"); return error;