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] [day] [month] [year] [list]
Message-ID: <e89642a8-15d5-a000-c6e4-09953b07aed4@huawei.com>
Date: Thu, 9 Jan 2025 09:34:08 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: Guenter Roeck <linux@...ck-us.net>, <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


在 2025/1/8 22:57, Guenter Roeck 写道:
> 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.
That's what it sounds like.
Ok, let's drop this patch.
>
> 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);
>
> .