Re: [RESEND PATCH 2/2] PM: Use sysfs_emit() and sysfs_emit_at() in "show" functions

[Christophe JAILLET]

Le 30/07/2024 à 08:54, Xueqin Luo a écrit :
> 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@xxxxxxxxxx>
> ---

Hi,

> @@ -149,17 +149,17 @@ 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';

The goal here is to remove the trailing space. So you slightly change 
the behavior.
Not sure if it matters or not (this was not done in disk_show() in patch 
1/2).

Maybe an (ugly)
	if (sz)
		sz += sysfs_emit_at(buf, sz - 1, "\n");

or something like:
	if (sz) {
		sz--;
		sz += sysfs_emit_at(buf, sz, "\n");
	}

> +	if (sz)
> +		sz += sysfs_emit_at(buf, sz, "\n");
>   
> -	return (s - buf);
> +	return sz;
>   }
>   
>   static suspend_state_t decode_suspend_state(const char *buf, size_t n)

...

> @@ -257,22 +257,22 @@ 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;
>   
>   	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)
> +	if (sz)
>   		/* convert the last space to a newline */
> -		*(s-1) = '\n';
> +		sz += sysfs_emit_at(buf, sz, "\n");

Same here.

>   
> -	return (s - buf);
> +	return sz;
>   }
>   
>   static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -390,7 +390,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);	\

Maybe 1 tab should be removed?

>   }								\
>   static struct kobj_attribute _name = __ATTR_RO(_name)
>   

...

> @@ -668,21 +668,21 @@ 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)
> +		sz += sysfs_emit_at(buf, sz, "disk ");
> +	if (sz)
>   		/* convert the last space to a newline */
> -		*(s-1) = '\n';
> -	return (s - buf);
> +		sz += sysfs_emit_at(buf, sz, "\n");

Same here

> +	return sz;
>   }
>   
>   static suspend_state_t decode_state(const char *buf, size_t n)

...

CJ