Re: [PATCH v2 1/3] hwmon: (acpi_power_meter) Fix uninitialized variables

[lihuisong (C)]

在 2025/1/8 22:45, Guenter Roeck 写道:
> On 1/7/25 03:33, Huisong Li wrote:
> > The 'power1_alarm' attribute uses the 'power' and 'cap' in the
> > acpi_power_meter_resource structure. Currently, these two fields are 
> > just
> > updated when user query 'power' and 'cap' attribute. If user directly 
> > query
> > the 'power1_alarm' attribute without queryng above two attributes, 
> > driver
> > will use uninitialized variables to judge.
> >
> > So this patch adds the setting of alarm state and update 'cap' in the
> > notification callback and update 'power' and 'cap' if needed to show the
> > real alarm state.
> >
> > Signed-off-by: Huisong Li <lihuisong@xxxxxxxxxx>
> > ---
> >   drivers/hwmon/acpi_power_meter.c | 27 +++++++++++++++++++++++----
> >   1 file changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwmon/acpi_power_meter.c 
> > b/drivers/hwmon/acpi_power_meter.c
> > index 2f1c9d97ad21..0b337d4fb212 100644
> > --- a/drivers/hwmon/acpi_power_meter.c
> > +++ b/drivers/hwmon/acpi_power_meter.c
> > @@ -84,6 +84,7 @@ struct acpi_power_meter_resource {
> >       u64        power;
> >       u64        cap;
> >       u64        avg_interval;
> > +    bool        power_alarm;
> >       int            sensors_valid;
> >       unsigned long        sensors_last_updated;
> >       struct sensor_device_attribute    sensors[NUM_SENSORS];
> > @@ -396,6 +397,9 @@ static ssize_t show_val(struct device *dev,
> >       struct acpi_device *acpi_dev = to_acpi_device(dev);
> >       struct acpi_power_meter_resource *resource = 
> > acpi_dev->driver_data;
> >       u64 val = 0;
> > +    int ret;
> > +
> > +    guard(mutex)(&resource->lock);
> >         switch (attr->index) {
> >       case 0:
> > @@ -423,10 +427,17 @@ static ssize_t show_val(struct device *dev,
> >               val = 0;
> >           break;
> >       case 6:
> > -        if (resource->power > resource->cap)
> > -            val = 1;
> > -        else
> > -            val = 0;
> > +        ret = update_meter(resource);
> > +        if (ret)
> > +            return ret;
> > +        /* need to update cap if not to support the notification. */
> > +        if (!(resource->caps.flags & POWER_METER_CAN_NOTIFY)) {
> > +            ret = update_cap(resource);
> > +            if (ret)
> > +                return ret;
> > +        }
> > +        val = resource->power_alarm || resource->power > resource->cap;
> > +        resource->power_alarm = resource->power > resource->cap ? 
> > true : false;
>
> The result of the comparison is already a boolean. ? : is unnecessary.
Right, will fix in v3.
> >           break;
> >       case 7:
> >       case 8:
> > @@ -847,12 +858,20 @@ static void acpi_power_meter_notify(struct 
> > acpi_device *device, u32 event)
> >           sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
> >           break;
> >       case METER_NOTIFY_CAP:
> > +        mutex_lock(&resource->lock);
> > +        res = update_cap(resource);
> > +        if (res)
> > +            dev_err(&device->dev, "update cap failed when capping 
> > value is changed.\n");
>
> Please make this dev_err_once() to avoid logging noise if the problem
> is persistent.

will fix in v3.

Thanks.

> > + mutex_unlock(&resource->lock);
> >           sysfs_notify(&device->dev.kobj, NULL, POWER_CAP_NAME);
> >           break;
> >       case METER_NOTIFY_INTERVAL:
> >           sysfs_notify(&device->dev.kobj, NULL, 
> > POWER_AVG_INTERVAL_NAME);
> >           break;
> >       case METER_NOTIFY_CAPPING:
> > +        mutex_lock(&resource->lock);
> > +        resource->power_alarm = true;
> > +        mutex_unlock(&resource->lock);
> >           sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
> >           dev_info(&device->dev, "Capping in progress.\n");
> >           break;
>
> .