在 2024/11/26 12:04, Guenter Roeck 写道:
On 11/25/24 17:56, lihuisong (C) wrote:
Hi Guente,
Thanks for your timely review.
在 2024/11/26 0:03, Guenter Roeck 写道:
On 11/25/24 01:34, Huisong Li wrote:
The 'power1_alarm' attribute uses the 'power' and 'cap' in the
acpi_power_meter_resource structure. However, these two fields are
just
updated when user query 'power' and 'cap' attribute, or hardware
enforced
limit. If user directly query the 'power1_alarm' attribute without
queryng
above two attributes, driver will use the uninitialized variables
to judge.
In addition, the 'power1_alarm' attribute needs to update power and
cap to
show the real state.
Signed-off-by: Huisong Li <lihuisong@xxxxxxxxxx>
---
drivers/hwmon/acpi_power_meter.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/hwmon/acpi_power_meter.c
b/drivers/hwmon/acpi_power_meter.c
index 2f1c9d97ad21..4c3314e35d30 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -396,6 +396,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,6 +426,13 @@ static ssize_t show_val(struct device *dev,
val = 0;
break;
case 6:
+ ret = update_meter(resource);
+ if (ret)
+ return ret;
+ ret = update_cap(resource);
+ if (ret)
+ return ret;
+
if (resource->power > resource->cap)
val = 1;
else
While technically correct, the implementation of this attribute
defeats its
purpose. It is supposed to reflect the current status as reported by
the
hardware. A real fix would be to use the associated notification to
set or
reset a status flag, and to report the current value of that flag as
reported
by the hardware.
I know what you mean.
The Notify(power_meter, 0x83) is supposed to meet your proposal IIUC.
It's good, but it depands on hardware support notification.
If there is no notification support, the attribute should not even
exist,
unless there is a means to retrieve its value from ACPI (the status
itself,
not by comparing temperature values).
Currently, the 'power1_alarm' attribute is created just when platform
support the power meter meassurement(bit0 of the supported
capabilities in _PMC).
And it doesn't see if the platform support notifications.
From the current implementation of this driver, this sysfs can also
reflect the status by comparing power and cap,
which is good to the platform that support hardware limit from some
out-of-band mechanism but doesn't support any notification.
The point is that this can also be done from userspace. Hardware
monitoring drivers
are supposed to provide hardware attributes, not software attributes
derived from it.
So this 'power1_alarm' attribute can be exposed when platform supports
hardware enforced limit and notifcations when the hardware limit is
enforced, right?
If so, we have to change the condition that driver creates this sysfs
interface.
.
