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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABPSWR5cG=xTA72BHayYQTb=24VS3N+=dbsiMcU+gyqTKvNXAQ@mail.gmail.com>
Date: Fri, 5 Sep 2025 22:57:31 +0530
From: vivek yadav <vivekyadav1207731111@...il.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: daniel.lezcano@...aro.org, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, joe@...ches.com
Subject: Re: [PATCH] cpuidle: sysfs: Use sysfs_emit/sysfs_emit_at instead of sprintf/scnprintf

On Mon, Aug 25, 2025 at 8:58 PM <vivekyadav1207731111@...il.com> wrote:
>>
>> From: Vivek Yadav <vivekyadav1207731111@...il.com>
>>
>> The ->show() callbacks in sysfs should use sysfs_emit() or
>> sysfs_emit_at() when formatting values for user space. These helpers
>> are the recommended way to ensure correct buffer handling and
>> consistency across the kernel.
>>
>> See Documentation/filesystems/sysfs.rst for details.
>>
>> No functional change intended.
>>
>> Suggested-by: Joe Perches <joe@...ches.com>
>> Signed-off-by: Vivek Yadav <vivekyadav1207731111@...il.com>
>> ---
>>  drivers/cpuidle/sysfs.c | 38 +++++++++++++++++++-------------------
>>  1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
>> index d6f5da61cb7d..c7af09460b74 100644
>> --- a/drivers/cpuidle/sysfs.c
>> +++ b/drivers/cpuidle/sysfs.c
>> @@ -22,21 +22,21 @@ static ssize_t show_available_governors(struct device *dev,
>>                                         struct device_attribute *attr,
>>                                         char *buf)
>>  {
>> -       ssize_t i = 0;
>> +       ssize_t len = 0;
>
>The variable rename is not necessary or even useful AFAICS ->

There is no harm if we leave the variable name as 'i' but it would be better
if we give it a suitable name like 'offset'. It will definitely improve
readability.

>
>>         struct cpuidle_governor *tmp;
>>
>>         mutex_lock(&cpuidle_lock);
>>         list_for_each_entry(tmp, &cpuidle_governors, governor_list) {
>> -               if (i >= (ssize_t) (PAGE_SIZE - (CPUIDLE_NAME_LEN + 2)))
>> +               if (len >= (ssize_t)(PAGE_SIZE - (CPUIDLE_NAME_LEN + 2)))
>>                         goto out;
>>
>> -               i += scnprintf(&buf[i], CPUIDLE_NAME_LEN + 1, "%s ", tmp->name);
>> +               len += sysfs_emit_at(buf, len, "%.*s ", CPUIDLE_NAME_LEN, tmp->name);
>
>-> because the second argument here is still an offset relative to
>buf, isn't it?

I think 'len' is also not a good name. Now I have two options.
Either I can remove this part from the patch or I can make new patch
where I can change the variable name 'i' into a more meaningful name
like 'offset'.

Let me know in which direction I should move.

~Vivek


On Fri, Sep 5, 2025 at 1:05 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Mon, Aug 25, 2025 at 8:58 PM <vivekyadav1207731111@...il.com> wrote:
> >
> > From: Vivek Yadav <vivekyadav1207731111@...il.com>
> >
> > The ->show() callbacks in sysfs should use sysfs_emit() or
> > sysfs_emit_at() when formatting values for user space. These helpers
> > are the recommended way to ensure correct buffer handling and
> > consistency across the kernel.
> >
> > See Documentation/filesystems/sysfs.rst for details.
> >
> > No functional change intended.
> >
> > Suggested-by: Joe Perches <joe@...ches.com>
> > Signed-off-by: Vivek Yadav <vivekyadav1207731111@...il.com>
> > ---
> >  drivers/cpuidle/sysfs.c | 38 +++++++++++++++++++-------------------
> >  1 file changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> > index d6f5da61cb7d..c7af09460b74 100644
> > --- a/drivers/cpuidle/sysfs.c
> > +++ b/drivers/cpuidle/sysfs.c
> > @@ -22,21 +22,21 @@ static ssize_t show_available_governors(struct device *dev,
> >                                         struct device_attribute *attr,
> >                                         char *buf)
> >  {
> > -       ssize_t i = 0;
> > +       ssize_t len = 0;
>
> The variable rename is not necessary or even useful AFAICS ->
>
> >         struct cpuidle_governor *tmp;
> >
> >         mutex_lock(&cpuidle_lock);
> >         list_for_each_entry(tmp, &cpuidle_governors, governor_list) {
> > -               if (i >= (ssize_t) (PAGE_SIZE - (CPUIDLE_NAME_LEN + 2)))
> > +               if (len >= (ssize_t)(PAGE_SIZE - (CPUIDLE_NAME_LEN + 2)))
> >                         goto out;
> >
> > -               i += scnprintf(&buf[i], CPUIDLE_NAME_LEN + 1, "%s ", tmp->name);
> > +               len += sysfs_emit_at(buf, len, "%.*s ", CPUIDLE_NAME_LEN, tmp->name);
>
> -> because the second argument here is still an offset relative to
> buf, isn't it?
>
> >         }
> >
> >  out:
> > -       i+= sprintf(&buf[i], "\n");
> > +       len += sysfs_emit_at(buf, len, "\n");
> >         mutex_unlock(&cpuidle_lock);
> > -       return i;
> > +       return len;
> >  }
> >
> >  static ssize_t show_current_driver(struct device *dev,
> > @@ -49,9 +49,9 @@ static ssize_t show_current_driver(struct device *dev,
> >         spin_lock(&cpuidle_driver_lock);
> >         drv = cpuidle_get_driver();
> >         if (drv)
> > -               ret = sprintf(buf, "%s\n", drv->name);
> > +               ret = sysfs_emit(buf, "%s\n", drv->name);
> >         else
> > -               ret = sprintf(buf, "none\n");
> > +               ret = sysfs_emit(buf, "none\n");
> >         spin_unlock(&cpuidle_driver_lock);
> >
> >         return ret;
> > @@ -65,9 +65,9 @@ static ssize_t show_current_governor(struct device *dev,
> >
> >         mutex_lock(&cpuidle_lock);
> >         if (cpuidle_curr_governor)
> > -               ret = sprintf(buf, "%s\n", cpuidle_curr_governor->name);
> > +               ret = sysfs_emit(buf, "%s\n", cpuidle_curr_governor->name);
> >         else
> > -               ret = sprintf(buf, "none\n");
> > +               ret = sysfs_emit(buf, "none\n");
> >         mutex_unlock(&cpuidle_lock);
> >
> >         return ret;
> > @@ -230,7 +230,7 @@ static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644, show, store)
> >  static ssize_t show_state_##_name(struct cpuidle_state *state, \
> >                          struct cpuidle_state_usage *state_usage, char *buf) \
> >  { \
> > -       return sprintf(buf, "%u\n", state->_name);\
> > +       return sysfs_emit(buf, "%u\n", state->_name);\
> >  }
> >
> >  #define define_show_state_ull_function(_name) \
> > @@ -238,7 +238,7 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \
> >                                   struct cpuidle_state_usage *state_usage, \
> >                                   char *buf)                            \
> >  { \
> > -       return sprintf(buf, "%llu\n", state_usage->_name);\
> > +       return sysfs_emit(buf, "%llu\n", state_usage->_name);\
> >  }
> >
> >  #define define_show_state_str_function(_name) \
> > @@ -247,8 +247,8 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \
> >                                   char *buf)                            \
> >  { \
> >         if (state->_name[0] == '\0')\
> > -               return sprintf(buf, "<null>\n");\
> > -       return sprintf(buf, "%s\n", state->_name);\
> > +               return sysfs_emit(buf, "<null>\n");\
> > +       return sysfs_emit(buf, "%s\n", state->_name);\
> >  }
> >
> >  #define define_show_state_time_function(_name) \
> > @@ -256,7 +256,7 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \
> >                                   struct cpuidle_state_usage *state_usage, \
> >                                   char *buf) \
> >  { \
> > -       return sprintf(buf, "%llu\n", ktime_to_us(state->_name##_ns)); \
> > +       return sysfs_emit(buf, "%llu\n", ktime_to_us(state->_name##_ns)); \
> >  }
> >
> >  define_show_state_time_function(exit_latency)
> > @@ -273,14 +273,14 @@ static ssize_t show_state_time(struct cpuidle_state *state,
> >                                struct cpuidle_state_usage *state_usage,
> >                                char *buf)
> >  {
> > -       return sprintf(buf, "%llu\n", ktime_to_us(state_usage->time_ns));
> > +       return sysfs_emit(buf, "%llu\n", ktime_to_us(state_usage->time_ns));
> >  }
> >
> >  static ssize_t show_state_disable(struct cpuidle_state *state,
> >                                   struct cpuidle_state_usage *state_usage,
> >                                   char *buf)
> >  {
> > -       return sprintf(buf, "%llu\n",
> > +       return sysfs_emit(buf, "%llu\n",
> >                        state_usage->disable & CPUIDLE_STATE_DISABLED_BY_USER);
> >  }
> >
> > @@ -310,7 +310,7 @@ static ssize_t show_state_default_status(struct cpuidle_state *state,
> >                                           struct cpuidle_state_usage *state_usage,
> >                                           char *buf)
> >  {
> > -       return sprintf(buf, "%s\n",
> > +       return sysfs_emit(buf, "%s\n",
> >                        state->flags & CPUIDLE_FLAG_OFF ? "disabled" : "enabled");
> >  }
> >
> > @@ -358,7 +358,7 @@ static ssize_t show_state_s2idle_##_name(struct cpuidle_state *state, \
> >                                          struct cpuidle_state_usage *state_usage, \
> >                                          char *buf)                             \
> >  { \
> > -       return sprintf(buf, "%llu\n", state_usage->s2idle_##_name);\
> > +       return sysfs_emit(buf, "%llu\n", state_usage->s2idle_##_name);\
> >  }
> >
> >  define_show_state_s2idle_ull_function(usage);
> > @@ -550,7 +550,7 @@ static ssize_t show_driver_name(struct cpuidle_driver *drv, char *buf)
> >         ssize_t ret;
> >
> >         spin_lock(&cpuidle_driver_lock);
> > -       ret = sprintf(buf, "%s\n", drv ? drv->name : "none");
> > +       ret = sysfs_emit(buf, "%s\n", drv ? drv->name : "none");
> >         spin_unlock(&cpuidle_driver_lock);
> >
> >         return ret;
> > --

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ