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

["lihuisong (C)" <lihuisong@xxxxxxxxxx>]

在 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@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 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);
>
> .