lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJZ5v0iYRXQTTnHic-x7ExOL3OW=RD4CgvgBGWgLUpqVXfsvtA@mail.gmail.com>
Date: Wed, 31 Jul 2024 12:55:21 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: xueqin Luo <luoxueqin@...inos.cn>
Cc: rafael@...nel.org, pavel@....cz, len.brown@...el.com, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, xiongxin@...inos.cn
Subject: Re: [PATCH v2 2/2] PM: Use sysfs_emit() and sysfs_emit_at() in "show" functions

On Wed, Jul 31, 2024 at 4:45 AM xueqin Luo <luoxueqin@...inos.cn> wrote:
>
> From: Xueqin Luo <luoxueqin@...inos.cn>
>
> As Documentation/filesystems/sysfs.rst suggested,
> show() should only use sysfs_emit() or sysfs_emit_at() when formatting
> the value to be returned to user space.
>
> No functional change intended.
>
> Signed-off-by: Xueqin Luo <luoxueqin@...inos.cn>
> ---
>  kernel/power/main.c | 82 +++++++++++++++++++++++++--------------------
>  1 file changed, 45 insertions(+), 37 deletions(-)
>
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index a9e0693aaf69..18948b694205 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -115,7 +115,7 @@ int pm_async_enabled = 1;
>  static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
>                              char *buf)
>  {
> -       return sprintf(buf, "%d\n", pm_async_enabled);
> +       return sysfs_emit(buf, "%d\n", pm_async_enabled);
>  }
>
>  static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -139,8 +139,8 @@ power_attr(pm_async);
>  static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
>                               char *buf)
>  {
> -       char *s = buf;
>         suspend_state_t i;
> +       ssize_t sz = 0;

Please call the new variable "count", and analogously below and in the
first patch.

>
>         for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
>                 if (i >= PM_SUSPEND_MEM && cxl_mem_active())
> @@ -149,17 +149,19 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
>                         const char *label = mem_sleep_states[i];
>
>                         if (mem_sleep_current == i)
> -                               s += sprintf(s, "[%s] ", label);
> +                               sz += sysfs_emit_at(buf, sz, "[%s] ", label);
>                         else
> -                               s += sprintf(s, "%s ", label);
> +                               sz += sysfs_emit_at(buf, sz, "%s ", label);
>                 }
>         }
>
>         /* Convert the last space to a newline if needed. */
> -       if (s != buf)
> -               *(s-1) = '\n';
> +       if (sz) {
> +               sz--;
> +               sz += sysfs_emit_at(buf, sz, "\n");
> +       }

Why do you need to update the count var above?  Something like this
should work too AFAICS:

if (count > 0)
        sysfs_emit_at(buf, count - 1, "\n");

Analogously below.

>
> -       return (s - buf);
> +       return sz;
>  }
>
>  static suspend_state_t decode_suspend_state(const char *buf, size_t n)
> @@ -220,7 +222,7 @@ bool sync_on_suspend_enabled = !IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC);
>  static ssize_t sync_on_suspend_show(struct kobject *kobj,
>                                    struct kobj_attribute *attr, char *buf)
>  {
> -       return sprintf(buf, "%d\n", sync_on_suspend_enabled);
> +       return sysfs_emit(buf, "%d\n", sync_on_suspend_enabled);
>  }
>
>  static ssize_t sync_on_suspend_store(struct kobject *kobj,
> @@ -257,22 +259,24 @@ static const char * const pm_tests[__TEST_AFTER_LAST] = {
>  static ssize_t pm_test_show(struct kobject *kobj, struct kobj_attribute *attr,
>                                 char *buf)
>  {
> -       char *s = buf;
>         int level;
> +       size_t sz = 0;

Reverse xmas tree order, please.

>
>         for (level = TEST_FIRST; level <= TEST_MAX; level++)
>                 if (pm_tests[level]) {
>                         if (level == pm_test_level)
> -                               s += sprintf(s, "[%s] ", pm_tests[level]);
> +                               sz += sysfs_emit_at(buf, sz, "[%s] ", pm_tests[level]);
>                         else
> -                               s += sprintf(s, "%s ", pm_tests[level]);
> +                               sz += sysfs_emit_at(buf, sz, "%s ", pm_tests[level]);
>                 }
>
> -       if (s != buf)
> -               /* convert the last space to a newline */
> -               *(s-1) = '\n';
> +       /* Convert the last space to a newline if needed. */
> +       if (sz) {
> +               sz--;
> +               sz += sysfs_emit_at(buf, sz, "\n");
> +       }
>
> -       return (s - buf);
> +       return sz;
>  }
>
>  static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -390,7 +394,7 @@ static const char * const suspend_step_names[] = {
>  static ssize_t _name##_show(struct kobject *kobj,              \
>                 struct kobj_attribute *attr, char *buf)         \
>  {                                                              \
> -       return sprintf(buf, format_str, suspend_stats._name);   \
> +       return sysfs_emit(buf, format_str, suspend_stats._name);\
>  }                                                              \
>  static struct kobj_attribute _name = __ATTR_RO(_name)
>
> @@ -404,7 +408,7 @@ suspend_attr(max_hw_sleep, "%llu\n");
>  static ssize_t _name##_show(struct kobject *kobj,              \
>                 struct kobj_attribute *attr, char *buf)         \
>  {                                                              \
> -       return sprintf(buf, "%u\n",                             \
> +       return sysfs_emit(buf, "%u\n",                          \
>                        suspend_stats.step_failures[step-1]);    \
>  }                                                              \
>  static struct kobj_attribute _name = __ATTR_RO(_name)
> @@ -428,7 +432,7 @@ static ssize_t last_failed_dev_show(struct kobject *kobj,
>         index %= REC_FAILED_NUM;
>         last_failed_dev = suspend_stats.failed_devs[index];
>
> -       return sprintf(buf, "%s\n", last_failed_dev);
> +       return sysfs_emit(buf, "%s\n", last_failed_dev);
>  }
>  static struct kobj_attribute last_failed_dev = __ATTR_RO(last_failed_dev);
>
> @@ -442,7 +446,7 @@ static ssize_t last_failed_errno_show(struct kobject *kobj,
>         index %= REC_FAILED_NUM;
>         last_failed_errno = suspend_stats.errno[index];
>
> -       return sprintf(buf, "%d\n", last_failed_errno);
> +       return sysfs_emit(buf, "%d\n", last_failed_errno);
>  }
>  static struct kobj_attribute last_failed_errno = __ATTR_RO(last_failed_errno);
>
> @@ -456,7 +460,7 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
>         index %= REC_FAILED_NUM;
>         step = suspend_stats.failed_steps[index];
>
> -       return sprintf(buf, "%s\n", suspend_step_names[step]);
> +       return sysfs_emit(buf, "%s\n", suspend_step_names[step]);
>  }
>  static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
>
> @@ -571,7 +575,7 @@ bool pm_print_times_enabled;
>  static ssize_t pm_print_times_show(struct kobject *kobj,
>                                    struct kobj_attribute *attr, char *buf)
>  {
> -       return sprintf(buf, "%d\n", pm_print_times_enabled);
> +       return sysfs_emit(buf, "%d\n", pm_print_times_enabled);
>  }
>
>  static ssize_t pm_print_times_store(struct kobject *kobj,
> @@ -604,7 +608,7 @@ static ssize_t pm_wakeup_irq_show(struct kobject *kobj,
>         if (!pm_wakeup_irq())
>                 return -ENODATA;
>
> -       return sprintf(buf, "%u\n", pm_wakeup_irq());
> +       return sysfs_emit(buf, "%u\n", pm_wakeup_irq());
>  }
>
>  power_attr_ro(pm_wakeup_irq);
> @@ -620,7 +624,7 @@ EXPORT_SYMBOL_GPL(pm_debug_messages_should_print);
>  static ssize_t pm_debug_messages_show(struct kobject *kobj,
>                                       struct kobj_attribute *attr, char *buf)
>  {
> -       return sprintf(buf, "%d\n", pm_debug_messages_on);
> +       return sysfs_emit(buf, "%d\n", pm_debug_messages_on);
>  }
>
>  static ssize_t pm_debug_messages_store(struct kobject *kobj,
> @@ -668,21 +672,25 @@ struct kobject *power_kobj;
>  static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
>                           char *buf)
>  {
> -       char *s = buf;
> +       ssize_t sz = 0;
>  #ifdef CONFIG_SUSPEND
>         suspend_state_t i;
>
>         for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
>                 if (pm_states[i])
> -                       s += sprintf(s,"%s ", pm_states[i]);
> +                       sz += sysfs_emit_at(buf, sz, "%s ", pm_states[i]);
>
>  #endif
>         if (hibernation_available())
> -               s += sprintf(s, "disk ");
> -       if (s != buf)
> -               /* convert the last space to a newline */
> -               *(s-1) = '\n';
> -       return (s - buf);
> +               sz += sysfs_emit_at(buf, sz, "disk ");
> +
> +       /* Convert the last space to a newline if needed. */
> +       if (sz) {
> +               sz--;
> +               sz += sysfs_emit_at(buf, sz, "\n");
> +       }
> +
> +       return sz;
>  }
>
>  static suspend_state_t decode_state(const char *buf, size_t n)
> @@ -782,7 +790,7 @@ static ssize_t wakeup_count_show(struct kobject *kobj,
>         unsigned int val;
>
>         return pm_get_wakeup_count(&val, true) ?
> -               sprintf(buf, "%u\n", val) : -EINTR;
> +               sysfs_emit(buf, "%u\n", val) : -EINTR;
>  }
>
>  static ssize_t wakeup_count_store(struct kobject *kobj,
> @@ -824,17 +832,17 @@ static ssize_t autosleep_show(struct kobject *kobj,
>         suspend_state_t state = pm_autosleep_state();
>
>         if (state == PM_SUSPEND_ON)
> -               return sprintf(buf, "off\n");
> +               return sysfs_emit(buf, "off\n");
>
>  #ifdef CONFIG_SUSPEND
>         if (state < PM_SUSPEND_MAX)
> -               return sprintf(buf, "%s\n", pm_states[state] ?
> +               return sysfs_emit(buf, "%s\n", pm_states[state] ?
>                                         pm_states[state] : "error");
>  #endif
>  #ifdef CONFIG_HIBERNATION
> -       return sprintf(buf, "disk\n");
> +       return sysfs_emit(buf, "disk\n");
>  #else
> -       return sprintf(buf, "error");
> +       return sysfs_emit(buf, "error\n");
>  #endif
>  }
>
> @@ -903,7 +911,7 @@ int pm_trace_enabled;
>  static ssize_t pm_trace_show(struct kobject *kobj, struct kobj_attribute *attr,
>                              char *buf)
>  {
> -       return sprintf(buf, "%d\n", pm_trace_enabled);
> +       return sysfs_emit(buf, "%d\n", pm_trace_enabled);
>  }
>
>  static ssize_t
> @@ -940,7 +948,7 @@ power_attr_ro(pm_trace_dev_match);
>  static ssize_t pm_freeze_timeout_show(struct kobject *kobj,
>                                       struct kobj_attribute *attr, char *buf)
>  {
> -       return sprintf(buf, "%u\n", freeze_timeout_msecs);
> +       return sysfs_emit(buf, "%u\n", freeze_timeout_msecs);
>  }
>
>  static ssize_t pm_freeze_timeout_store(struct kobject *kobj,
> --

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ