Received-SPF: pass (google.com: domain of linux-kernel+bounces-149733-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <14e1e567-3429-4714-b0e2-14ad3eb66241@redhat.com>
Date: Thu, 18 Apr 2024 10:40:12 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] platform/x86: asus-laptop: Use sysfs_emit() to replace
 sprintf()
To: yunshui <jiangyunshui@kylinos.cn>, linux-kernel@vger.kernel.org,
 platform-driver-x86@vger.kernel.org
Cc: corentin.chary@gmail.com, luke@ljones.dev, ilpo.jarvinen@linux.intel.com,
 Ai Chao <aichao@kylinos.cn>
References: <20240418083136.668869-1-jiangyunshui@kylinos.cn>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20240418083136.668869-1-jiangyunshui@kylinos.cn>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi yunshui,

Thank you for your patch.

On 4/18/24 10:31 AM, yunshui wrote:
> 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.
> 
> Signed-off-by: yunshui <jiangyunshui@kylinos.cn>
> Signed-off-by: Ai Chao <aichao@kylinos.cn>
> ---
>  drivers/platform/x86/asus-laptop.c | 44 +++++++++++++++---------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> index 78c42767295a..2857aefc3d60 100644
> --- a/drivers/platform/x86/asus-laptop.c
> +++ b/drivers/platform/x86/asus-laptop.c
> @@ -852,8 +852,8 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
>  	 * so we don't set eof to 1
>  	 */
>  
> -	len += sprintf(page, ASUS_LAPTOP_NAME " " ASUS_LAPTOP_VERSION "\n");
> -	len += sprintf(page + len, "Model reference    : %s\n", asus->name);
> +	len += sysfs_emit(page, ASUS_LAPTOP_NAME " " ASUS_LAPTOP_VERSION "\n");
> +	len += sysfs_emit(page + len, "Model reference    : %s\n", asus->name);

One of the purposes of sysfs_emit is to check that the output actually fits in
a page size and to truncate the output otherwise. The original code did not
check this, but if we are going to change this then we do want to check this,
so you should use sysfs_emit_at, iow the 2 new lines here should be:

	len += sysfs_emit_at(page, len, ASUS_LAPTOP_NAME " " ASUS_LAPTOP_VERSION "\n");
	len += sysfs_emit_at(page, len, "Model reference    : %s\n", asus->name);

And all further lines also should use sysfs_emit_at(page, len, "...", ...);

Regards,

Hans


>  	/*
>  	 * The SFUN method probably allows the original driver to get the list
>  	 * of features supported by a given model. For now, 0x0100 or 0x0800
> @@ -862,7 +862,7 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
>  	 */
>  	rv = acpi_evaluate_integer(asus->handle, "SFUN", NULL, &temp);
>  	if (ACPI_SUCCESS(rv))
> -		len += sprintf(page + len, "SFUN value         : %#x\n",
> +		len += sysfs_emit(page + len, "SFUN value         : %#x\n",
>  			       (uint) temp);
>  	/*
>  	 * The HWRS method return informations about the hardware.
> @@ -874,7 +874,7 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
>  	 */
>  	rv = acpi_evaluate_integer(asus->handle, "HWRS", NULL, &temp);
>  	if (ACPI_SUCCESS(rv))
> -		len += sprintf(page + len, "HWRS value         : %#x\n",
> +		len += sysfs_emit(page + len, "HWRS value         : %#x\n",
>  			       (uint) temp);
>  	/*
>  	 * Another value for userspace: the ASYM method returns 0x02 for
> @@ -885,25 +885,25 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
>  	 */
>  	rv = acpi_evaluate_integer(asus->handle, "ASYM", NULL, &temp);
>  	if (ACPI_SUCCESS(rv))
> -		len += sprintf(page + len, "ASYM value         : %#x\n",
> +		len += sysfs_emit(page + len, "ASYM value         : %#x\n",
>  			       (uint) temp);
>  	if (asus->dsdt_info) {
>  		snprintf(buf, 16, "%d", asus->dsdt_info->length);
> -		len += sprintf(page + len, "DSDT length        : %s\n", buf);
> +		len += sysfs_emit(page + len, "DSDT length        : %s\n", buf);
>  		snprintf(buf, 16, "%d", asus->dsdt_info->checksum);
> -		len += sprintf(page + len, "DSDT checksum      : %s\n", buf);
> +		len += sysfs_emit(page + len, "DSDT checksum      : %s\n", buf);
>  		snprintf(buf, 16, "%d", asus->dsdt_info->revision);
> -		len += sprintf(page + len, "DSDT revision      : %s\n", buf);
> +		len += sysfs_emit(page + len, "DSDT revision      : %s\n", buf);
>  		snprintf(buf, 7, "%s", asus->dsdt_info->oem_id);
> -		len += sprintf(page + len, "OEM id             : %s\n", buf);
> +		len += sysfs_emit(page + len, "OEM id             : %s\n", buf);
>  		snprintf(buf, 9, "%s", asus->dsdt_info->oem_table_id);
> -		len += sprintf(page + len, "OEM table id       : %s\n", buf);
> +		len += sysfs_emit(page + len, "OEM table id       : %s\n", buf);
>  		snprintf(buf, 16, "%x", asus->dsdt_info->oem_revision);
> -		len += sprintf(page + len, "OEM revision       : 0x%s\n", buf);
> +		len += sysfs_emit(page + len, "OEM revision       : 0x%s\n", buf);
>  		snprintf(buf, 5, "%s", asus->dsdt_info->asl_compiler_id);
> -		len += sprintf(page + len, "ASL comp vendor id : %s\n", buf);
> +		len += sysfs_emit(page + len, "ASL comp vendor id : %s\n", buf);
>  		snprintf(buf, 16, "%x", asus->dsdt_info->asl_compiler_revision);
> -		len += sprintf(page + len, "ASL comp revision  : 0x%s\n", buf);
> +		len += sysfs_emit(page + len, "ASL comp revision  : 0x%s\n", buf);
>  	}
>  
>  	return len;
> @@ -933,7 +933,7 @@ static ssize_t ledd_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  
> -	return sprintf(buf, "0x%08x\n", asus->ledd_status);
> +	return sysfs_emit(buf, "0x%08x\n", asus->ledd_status);
>  }
>  
>  static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> @@ -993,7 +993,7 @@ static ssize_t wlan_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  
> -	return sprintf(buf, "%d\n", asus_wireless_status(asus, WL_RSTS));
> +	return sysfs_emit(buf, "%d\n", asus_wireless_status(asus, WL_RSTS));
>  }
>  
>  static ssize_t wlan_store(struct device *dev, struct device_attribute *attr,
> @@ -1022,7 +1022,7 @@ static ssize_t bluetooth_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  
> -	return sprintf(buf, "%d\n", asus_wireless_status(asus, BT_RSTS));
> +	return sysfs_emit(buf, "%d\n", asus_wireless_status(asus, BT_RSTS));
>  }
>  
>  static ssize_t bluetooth_store(struct device *dev,
> @@ -1052,7 +1052,7 @@ static ssize_t wimax_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  
> -	return sprintf(buf, "%d\n", asus_wireless_status(asus, WM_RSTS));
> +	return sysfs_emit(buf, "%d\n", asus_wireless_status(asus, WM_RSTS));
>  }
>  
>  static ssize_t wimax_store(struct device *dev, struct device_attribute *attr,
> @@ -1081,7 +1081,7 @@ static ssize_t wwan_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  
> -	return sprintf(buf, "%d\n", asus_wireless_status(asus, WW_RSTS));
> +	return sysfs_emit(buf, "%d\n", asus_wireless_status(asus, WW_RSTS));
>  }
>  
>  static ssize_t wwan_store(struct device *dev, struct device_attribute *attr,
> @@ -1151,7 +1151,7 @@ static ssize_t ls_switch_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  
> -	return sprintf(buf, "%d\n", asus->light_switch);
> +	return sysfs_emit(buf, "%d\n", asus->light_switch);
>  }
>  
>  static ssize_t ls_switch_store(struct device *dev,
> @@ -1182,7 +1182,7 @@ static ssize_t ls_level_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  
> -	return sprintf(buf, "%d\n", asus->light_level);
> +	return sysfs_emit(buf, "%d\n", asus->light_level);
>  }
>  
>  static ssize_t ls_level_store(struct device *dev, struct device_attribute *attr,
> @@ -1228,7 +1228,7 @@ static ssize_t ls_value_show(struct device *dev, struct device_attribute *attr,
>  	if (!err)
>  		err = pega_int_read(asus, PEGA_READ_ALS_L, &lo);
>  	if (!err)
> -		return sprintf(buf, "%d\n", 10 * hi + lo);
> +		return sysfs_emit(buf, "%d\n", 10 * hi + lo);
>  	return err;
>  }
>  static DEVICE_ATTR_RO(ls_value);
> @@ -1264,7 +1264,7 @@ static ssize_t gps_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct asus_laptop *asus = dev_get_drvdata(dev);
>  
> -	return sprintf(buf, "%d\n", asus_gps_status(asus));
> +	return sysfs_emit(buf, "%d\n", asus_gps_status(asus));
>  }
>  
>  static ssize_t gps_store(struct device *dev, struct device_attribute *attr,