linux-kernel - Re: [PATCH v2 3/3] hwmon: (acpi_power_meter) Remove redundant 'sensors

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: <5854a361-f88e-49e6-8807-f86c42aabd1a@roeck-us.net>
Date: Wed, 8 Jan 2025 06:57:42 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Huisong Li <lihuisong@...wei.com>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, jdelvare@...e.com, liuyonglong@...wei.com,
 zhanjie9@...ilicon.com, zhenglifeng1@...wei.com
Subject: Re: [PATCH v2 3/3] hwmon: (acpi_power_meter) Remove redundant
 'sensors_valid' variable

On 1/7/25 03:33, Huisong Li wrote:
> The 'sensors_valid' in acpi_power_meter_resource structure is always '1'
> after querying power once. The default value of this variable is zero which
> just ensure user can query power successfully without any time requirement
> at first time. We can get power and fill the 'sensors_last_updated' field
> at probing phase to make sure that a valid value is returned to user at
> first query within the sampling interval. Then this redundant variable can
> be safely removed.
> 

The "benefit" of this change is the saved variable. The cost associated with it
is that update_meter() is now _always_ called from setup_attrs(), during probe
and when handling configuration change notifications. It seems to me that this
is much more costly than keeping the variable since it is unlikely that the show
functions are actually called within the sampling time.

I fail to see why removing a variable would be more beneficial than the overhead
of unnecessarily calling update_meter() during probe (which also increases probe
time) and while handling configuration change notifications.

This would need a much better rationale to be acceptable.

Guenter

> Signed-off-by: Huisong Li <lihuisong@...wei.com>
> ---
>   drivers/hwmon/acpi_power_meter.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 594f7681d523..49bef3350439 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -85,7 +85,6 @@ struct acpi_power_meter_resource {
>   	u64		cap;
>   	u64		avg_interval;
>   	bool		power_alarm;
> -	int			sensors_valid;
>   	unsigned long		sensors_last_updated;
>   	struct sensor_device_attribute	sensors[NUM_SENSORS];
>   	int			num_sensors;
> @@ -316,15 +315,14 @@ static ssize_t set_trip(struct device *dev, struct device_attribute *devattr,
>   }
>   
>   /* Power meter */
> -static int update_meter(struct acpi_power_meter_resource *resource)
> +static int update_meter(struct acpi_power_meter_resource *resource, bool check)
>   {
>   	unsigned long long data;
>   	acpi_status status;
>   	unsigned long local_jiffies = jiffies;
>   
> -	if (time_before(local_jiffies, resource->sensors_last_updated +
> -			msecs_to_jiffies(resource->caps.sampling_time)) &&
> -			resource->sensors_valid)
> +	if (check && time_before(local_jiffies, resource->sensors_last_updated +
> +			msecs_to_jiffies(resource->caps.sampling_time)))
>   		return 0;
>   
>   	status = acpi_evaluate_integer(resource->acpi_dev->handle, "_PMM",
> @@ -336,7 +334,6 @@ static int update_meter(struct acpi_power_meter_resource *resource)
>   	}
>   
>   	resource->power = data;
> -	resource->sensors_valid = 1;
>   	resource->sensors_last_updated = jiffies;
>   	return 0;
>   }
> @@ -349,7 +346,7 @@ static ssize_t show_power(struct device *dev,
>   	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
>   
>   	mutex_lock(&resource->lock);
> -	update_meter(resource);
> +	update_meter(resource, true);
>   	mutex_unlock(&resource->lock);
>   
>   	if (resource->power == UNKNOWN_POWER)
> @@ -429,7 +426,7 @@ static ssize_t show_val(struct device *dev,
>   			val = 0;
>   		break;
>   	case 6:
> -		ret = update_meter(resource);
> +		ret = update_meter(resource, true);
>   		if (ret)
>   			return ret;
>   		/* need to update cap if not to support the notification. */
> @@ -699,6 +696,10 @@ static int setup_attrs(struct acpi_power_meter_resource *resource)
>   		return res;
>   
>   	if (resource->caps.flags & POWER_METER_CAN_MEASURE) {
> +		res = update_meter(resource, false);
> +		if (res)
> +			goto error;
> +
>   		res = register_attrs(resource, meter_attrs);
>   		if (res)
>   			goto error;
> @@ -898,7 +899,6 @@ static int acpi_power_meter_add(struct acpi_device *device)
>   	if (!resource)
>   		return -ENOMEM;
>   
> -	resource->sensors_valid = 0;
>   	resource->acpi_dev = device;
>   	mutex_init(&resource->lock);
>   	strcpy(acpi_device_name(device), ACPI_POWER_METER_DEVICE_NAME);