Re: [PATCH v1 3/4] hwmon: (acpi_power_meter) Remove redundant 'sensors_valid' variable

[Guenter Roeck]

On 11/25/24 01:34, Huisong Li wrote:
> The 'sensors_valid' in acpi_power_meter_resource structure is always one
> 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.
>
> Signed-off-by: Huisong Li <lihuisong@xxxxxxxxxx>
> ---
>   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 95da73858a0b..3500859ff0bf 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -84,7 +84,6 @@ struct acpi_power_meter_resource {
>   	u64		power;
>   	u64		cap;
>   	u64		avg_interval;
> -	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;
>   		ret = update_cap(resource);
> @@ -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;
> +

This forces an update of the meter attribute even if no one reads it
subsequently for a long period of time. Avoiding that is the whole point
of the flag. Otherwise every driver using the flag could just read its
entire set of attributes to avoid it. I don't see the value of this patch,
sorry. You'd have to explain why it is better to do the unnecessary read
to avoid the flag.

Guenter