| Message ID | 20250107113330.7970-2-lihuisong@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | some bugfix for acpi power meter | expand |
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@huawei.com> > --- > 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. > 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. > + 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;
在 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@huawei.com> >> --- >> 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; > > .
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; 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"); + 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;
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@huawei.com> --- drivers/hwmon/acpi_power_meter.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)