[PATCH v1 0/4] hwmon: (acpi_power_meter) Some trival optimizations

[PATCH v1 0/4] hwmon: (acpi_power_meter) Some trival optimizations

[Huisong Li]

I get an issue that the 'power1_alarm' attribute shows wrong if
we doesn't query the 'power1_average' and 'power1_cap' first.
So fix it and do some trival optimizations for acpi_power_meter
by the way. 

Huisong Li (4):
  hwmon: (acpi_power_meter) Fix using uninitialized variables
  hwmon: (acpi_power_meter) Fix update the power trip points on failure
  hwmon: (acpi_power_meter) Remove redundant 'sensors_valid' variable
  hwmon: (acpi_power_meter) Add the print of no notification that
    hardware limit is enforced

 drivers/hwmon/acpi_power_meter.c | 35 ++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 9 deletions(-)

-- 
2.22.0

[PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[Huisong Li]

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@huawei.com>
---
 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
-- 
2.22.0

[PATCH v1 2/4] hwmon: (acpi_power_meter) Fix update the power trip points on failure

[Huisong Li]

The power trip points maintained in local should not be updated when '_PTP'
method fails to evaluate.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/hwmon/acpi_power_meter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
index 4c3314e35d30..95da73858a0b 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -292,8 +292,8 @@ static ssize_t set_trip(struct device *dev, struct device_attribute *devattr,
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
+	unsigned long temp, trip_bk;
 	int res;
-	unsigned long temp;
 
 	res = kstrtoul(buf, 10, &temp);
 	if (res)
@@ -302,8 +302,11 @@ static ssize_t set_trip(struct device *dev, struct device_attribute *devattr,
 	temp = DIV_ROUND_CLOSEST(temp, 1000);
 
 	mutex_lock(&resource->lock);
+	trip_bk = resource->trip[attr->index - 7];
 	resource->trip[attr->index - 7] = temp;
 	res = set_acpi_trip(resource);
+	if (!res)
+		resource->trip[attr->index - 7] = trip_bk;
 	mutex_unlock(&resource->lock);
 
 	if (res)
-- 
2.22.0

[PATCH v1 3/4] hwmon: (acpi_power_meter) Remove redundant 'sensors_valid' variable

[Huisong Li]

The 'sensors_valid' in acpi_power_meter_resource structure is always one
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.

Signed-off-by: Huisong Li <lihuisong@huawei.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 95da73858a0b..3500859ff0bf 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -84,7 +84,6 @@ struct acpi_power_meter_resource {
 	u64		power;
 	u64		cap;
 	u64		avg_interval;
-	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;
 		ret = update_cap(resource);
@@ -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;
@@ -890,7 +891,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);
-- 
2.22.0

[PATCH v1 4/4] hwmon: (acpi_power_meter) Add the print of no notification that hardware limit is enforced

[Huisong Li]

As ACPI spec said, the bit3 of the supported capabilities in _PMC indicates
that the power meter supports notifications when the hardware limit is
enforced. If one platform doesn't report this bit, but support hardware
forced limit through some out-of-band mechanism. Driver wouldn't receive
the related notifications to notify the OSPM to re-read the hardware limit.
So add the print of no notifcation that hardware limit is enforced.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/hwmon/acpi_power_meter.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
index 3500859ff0bf..d3f144986fae 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -712,6 +712,10 @@ static int setup_attrs(struct acpi_power_meter_resource *resource)
 			goto skip_unsafe_cap;
 		}
 
+		if (resource->caps.flags & POWER_METER_CAN_NOTIFY == 0)
+			dev_info(&resource->acpi_dev->dev,
+				 "no notifcation when the hardware limit is enforced.\n");
+
 		if (resource->caps.configurable_cap)
 			res = register_attrs(resource, rw_cap_attrs);
 		else
-- 
2.22.0

Re: [PATCH v1 2/4] hwmon: (acpi_power_meter) Fix update the power trip points on failure

[Guenter Roeck]

On 11/25/24 01:34, Huisong Li wrote:
> The power trip points maintained in local should not be updated when '_PTP'
> method fails to evaluate.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>   drivers/hwmon/acpi_power_meter.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 4c3314e35d30..95da73858a0b 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -292,8 +292,8 @@ static ssize_t set_trip(struct device *dev, struct device_attribute *devattr,
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct acpi_device *acpi_dev = to_acpi_device(dev);
>   	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
> +	unsigned long temp, trip_bk;
>   	int res;
> -	unsigned long temp;
>   
>   	res = kstrtoul(buf, 10, &temp);
>   	if (res)
> @@ -302,8 +302,11 @@ static ssize_t set_trip(struct device *dev, struct device_attribute *devattr,
>   	temp = DIV_ROUND_CLOSEST(temp, 1000);
>   
>   	mutex_lock(&resource->lock);
> +	trip_bk = resource->trip[attr->index - 7];
>   	resource->trip[attr->index - 7] = temp;
>   	res = set_acpi_trip(resource);
> +	if (!res)
> +		resource->trip[attr->index - 7] = trip_bk;

Unless I am missing something, this restores the old value if setting
the new value succeeded. Please explain.

Thanks,
Guenter

>   	mutex_unlock(&resource->lock);
>   
>   	if (res)

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

[Guenter Roeck]

On 11/25/24 01:34, Huisong Li wrote:
> The 'sensors_valid' in acpi_power_meter_resource structure is always one
> 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.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.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 95da73858a0b..3500859ff0bf 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -84,7 +84,6 @@ struct acpi_power_meter_resource {
>   	u64		power;
>   	u64		cap;
>   	u64		avg_interval;
> -	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;
>   		ret = update_cap(resource);
> @@ -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;
> +

This forces an update of the meter attribute even if no one reads it
subsequently for a long period of time. Avoiding that is the whole point
of the flag. Otherwise every driver using the flag could just read its
entire set of attributes to avoid it. I don't see the value of this patch,
sorry. You'd have to explain why it is better to do the unnecessary read
to avoid the flag.

Guenter

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[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@huawei.com>
> ---
>   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.

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).

Guenter

Re: [PATCH v1 4/4] hwmon: (acpi_power_meter) Add the print of no notification that hardware limit is enforced

[Guenter Roeck]

On 11/25/24 01:34, Huisong Li wrote:
> As ACPI spec said, the bit3 of the supported capabilities in _PMC indicates
> that the power meter supports notifications when the hardware limit is
> enforced. If one platform doesn't report this bit, but support hardware
> forced limit through some out-of-band mechanism. Driver wouldn't receive
> the related notifications to notify the OSPM to re-read the hardware limit.
> So add the print of no notifcation that hardware limit is enforced.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>   drivers/hwmon/acpi_power_meter.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 3500859ff0bf..d3f144986fae 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -712,6 +712,10 @@ static int setup_attrs(struct acpi_power_meter_resource *resource)
>   			goto skip_unsafe_cap;
>   		}
>   
> +		if (resource->caps.flags & POWER_METER_CAN_NOTIFY == 0)

== has higher precedence than &, so this expression will never be true.

And, indeed:

drivers/hwmon/acpi_power_meter.c: In function ‘setup_attrs’:
drivers/hwmon/acpi_power_meter.c:701:42: error: suggest parentheses around comparison in operand of ‘&’

> +			dev_info(&resource->acpi_dev->dev,
> +				 "no notifcation when the hardware limit is enforced.\n");
> +
>   		if (resource->caps.configurable_cap)
>   			res = register_attrs(resource, rw_cap_attrs);
>   		else

On top of that, I don't see the value in this patch.

Overall, really, this driver could benefit from a complete overhaul.
Its use of the deprecated hwmon_device_register() should tell it all.
There is lots of questionable code, such as the unprotected calls to
remove_attrs() followed by setup_attrs() in the notification handler.
Any updates should be limited to bug fixes and not try to make minor
improvements for little if any gain.

Guenter

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[lihuisong (C)]

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@huawei.com>
>> ---
>>   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.

Re: [PATCH v1 2/4] hwmon: (acpi_power_meter) Fix update the power trip points on failure

[lihuisong (C)]

在 2024/11/25 23:22, Guenter Roeck 写道:
> On 11/25/24 01:34, Huisong Li wrote:
>> The power trip points maintained in local should not be updated when 
>> '_PTP'
>> method fails to evaluate.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/hwmon/acpi_power_meter.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/acpi_power_meter.c 
>> b/drivers/hwmon/acpi_power_meter.c
>> index 4c3314e35d30..95da73858a0b 100644
>> --- a/drivers/hwmon/acpi_power_meter.c
>> +++ b/drivers/hwmon/acpi_power_meter.c
>> @@ -292,8 +292,8 @@ static ssize_t set_trip(struct device *dev, 
>> struct device_attribute *devattr,
>>       struct sensor_device_attribute *attr = 
>> to_sensor_dev_attr(devattr);
>>       struct acpi_device *acpi_dev = to_acpi_device(dev);
>>       struct acpi_power_meter_resource *resource = 
>> acpi_dev->driver_data;
>> +    unsigned long temp, trip_bk;
>>       int res;
>> -    unsigned long temp;
>>         res = kstrtoul(buf, 10, &temp);
>>       if (res)
>> @@ -302,8 +302,11 @@ static ssize_t set_trip(struct device *dev, 
>> struct device_attribute *devattr,
>>       temp = DIV_ROUND_CLOSEST(temp, 1000);
>>         mutex_lock(&resource->lock);
>> +    trip_bk = resource->trip[attr->index - 7];
>>       resource->trip[attr->index - 7] = temp;
>>       res = set_acpi_trip(resource);
>> +    if (!res)
>> +        resource->trip[attr->index - 7] = trip_bk;
>
> Unless I am missing something, this restores the old value if setting
> the new value succeeded. Please explain.
>
Yes, you are right.
Restore the old value on faiure. will fix it.
>
>> mutex_unlock(&resource->lock);
>>         if (res)
>
> .

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

[lihuisong (C)]

Hi Guenter,

在 2024/11/25 23:38, Guenter Roeck 写道:
> On 11/25/24 01:34, Huisong Li wrote:
>> The 'sensors_valid' in acpi_power_meter_resource structure is always one
>> 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.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.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 95da73858a0b..3500859ff0bf 100644
>> --- a/drivers/hwmon/acpi_power_meter.c
>> +++ b/drivers/hwmon/acpi_power_meter.c
>> @@ -84,7 +84,6 @@ struct acpi_power_meter_resource {
>>       u64        power;
>>       u64        cap;
>>       u64        avg_interval;
>> -    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;
>>           ret = update_cap(resource);
>> @@ -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;
>> +
>
> This forces an update of the meter attribute even if no one reads it
> subsequently for a long period of time. Avoiding that is the whole point
> of the flag. Otherwise every driver using the flag could just read its
> entire set of attributes to avoid it. I don't see the value of this 
> patch,
> sorry. You'd have to explain why it is better to do the unnecessary read
> to avoid the flag.
It's just to make sure that the first query after driver is initialized 
can return a valid value when user echo the sysfs
within the sampling time(namely, jiffies < sensors_last_updated + 
caps.sampling_time).
The default value of the 'sensors_valid' in resource is zero, which just 
ensure user can query power successfully without any time requirement at 
first query.
This read without check during probing phase is just for not to change 
the original behavior of the first query.
Maybe my commit log didn't make it clear. Please take a look at the 
commit log of this patch again. Thanks.
>
>
>
> .

Re: [PATCH v1 4/4] hwmon: (acpi_power_meter) Add the print of no notification that hardware limit is enforced

[lihuisong (C)]

在 2024/11/26 0:13, Guenter Roeck 写道:
> On 11/25/24 01:34, Huisong Li wrote:
>> As ACPI spec said, the bit3 of the supported capabilities in _PMC 
>> indicates
>> that the power meter supports notifications when the hardware limit is
>> enforced. If one platform doesn't report this bit, but support hardware
>> forced limit through some out-of-band mechanism. Driver wouldn't receive
>> the related notifications to notify the OSPM to re-read the hardware 
>> limit.
>> So add the print of no notifcation that hardware limit is enforced.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/hwmon/acpi_power_meter.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/hwmon/acpi_power_meter.c 
>> b/drivers/hwmon/acpi_power_meter.c
>> index 3500859ff0bf..d3f144986fae 100644
>> --- a/drivers/hwmon/acpi_power_meter.c
>> +++ b/drivers/hwmon/acpi_power_meter.c
>> @@ -712,6 +712,10 @@ static int setup_attrs(struct 
>> acpi_power_meter_resource *resource)
>>               goto skip_unsafe_cap;
>>           }
>>   +        if (resource->caps.flags & POWER_METER_CAN_NOTIFY == 0)
>
> == has higher precedence than &, so this expression will never be true.
Indeed.
>
> And, indeed:
>
> drivers/hwmon/acpi_power_meter.c: In function ‘setup_attrs’:
> drivers/hwmon/acpi_power_meter.c:701:42: error: suggest parentheses 
> around comparison in operand of ‘&’
What compilation parameters did you use to intercept this?😁
>
>> + dev_info(&resource->acpi_dev->dev,
>> +                 "no notifcation when the hardware limit is 
>> enforced.\n");
>> +
>>           if (resource->caps.configurable_cap)
>>               res = register_attrs(resource, rw_cap_attrs);
>>           else
>
> On top of that, I don't see the value in this patch.
 From the current implement, the value of this patch is little. It's 
just telling the user that he won't be notified. Notifications are not 
available.

Actually, I'd like to add some necessary updates in the notification 
handler when OSPM receive some notifications, like 0x82, 0x83 event.
These updates are necessary for this driver, which more follow ACPI spec.
But I don't know how do handle the notify 0x81 to fix the trip points, 
so I don't modify it yet.
>
> Overall, really, this driver could benefit from a complete overhaul.
> Its use of the deprecated hwmon_device_register() should tell it all.
Yes, I also found it.
But I don't know how to handle struct hwmon_chip_info and if it is 
appropriate to this driver yet.
It will be a big modification if it is ok.
> There is lots of questionable code, such as the unprotected calls to
> remove_attrs() followed by setup_attrs() in the notification handler.
Agreed.
In addition, using struct sensor_template  to create sysfs interface is 
hard to maintain and not good to me.
The show_val and show_str are to display based on the index in struct 
sensor_template.
> Any updates should be limited to bug fixes and not try to make minor
> improvements for little if any gain.
>
Yes
>
> .

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[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@huawei.com>
>>> ---
>>>   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.

Guenter

Re: [PATCH v1 4/4] hwmon: (acpi_power_meter) Add the print of no notification that hardware limit is enforced

[Guenter Roeck]

On 11/25/24 19:15, lihuisong (C) wrote:
> 
> 在 2024/11/26 0:13, Guenter Roeck 写道:
>> On 11/25/24 01:34, Huisong Li wrote:
>>> As ACPI spec said, the bit3 of the supported capabilities in _PMC indicates
>>> that the power meter supports notifications when the hardware limit is
>>> enforced. If one platform doesn't report this bit, but support hardware
>>> forced limit through some out-of-band mechanism. Driver wouldn't receive
>>> the related notifications to notify the OSPM to re-read the hardware limit.
>>> So add the print of no notifcation that hardware limit is enforced.
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> ---
>>>   drivers/hwmon/acpi_power_meter.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
>>> index 3500859ff0bf..d3f144986fae 100644
>>> --- a/drivers/hwmon/acpi_power_meter.c
>>> +++ b/drivers/hwmon/acpi_power_meter.c
>>> @@ -712,6 +712,10 @@ static int setup_attrs(struct acpi_power_meter_resource *resource)
>>>               goto skip_unsafe_cap;
>>>           }
>>>   +        if (resource->caps.flags & POWER_METER_CAN_NOTIFY == 0)
>>
>> == has higher precedence than &, so this expression will never be true.
> Indeed.
>>
>> And, indeed:
>>
>> drivers/hwmon/acpi_power_meter.c: In function ‘setup_attrs’:
>> drivers/hwmon/acpi_power_meter.c:701:42: error: suggest parentheses around comparison in operand of ‘&’
> What compilation parameters did you use to intercept this?😁

Nothing special.

make allmodconfig; make drivers/hwmon/acpi_power_meter.o

Guenter

>>
>>> + dev_info(&resource->acpi_dev->dev,
>>> +                 "no notifcation when the hardware limit is enforced.\n");
>>> +
>>>           if (resource->caps.configurable_cap)
>>>               res = register_attrs(resource, rw_cap_attrs);
>>>           else
>>
>> On top of that, I don't see the value in this patch.
>  From the current implement, the value of this patch is little. It's just telling the user that he won't be notified. Notifications are not available.
> 
> Actually, I'd like to add some necessary updates in the notification handler when OSPM receive some notifications, like 0x82, 0x83 event.
> These updates are necessary for this driver, which more follow ACPI spec.
> But I don't know how do handle the notify 0x81 to fix the trip points, so I don't modify it yet.
>>
>> Overall, really, this driver could benefit from a complete overhaul.
>> Its use of the deprecated hwmon_device_register() should tell it all.
> Yes, I also found it.
> But I don't know how to handle struct hwmon_chip_info and if it is appropriate to this driver yet.
> It will be a big modification if it is ok.
>> There is lots of questionable code, such as the unprotected calls to
>> remove_attrs() followed by setup_attrs() in the notification handler.
> Agreed.
> In addition, using struct sensor_template  to create sysfs interface is hard to maintain and not good to me.
> The show_val and show_str are to display based on the index in struct sensor_template.
>> Any updates should be limited to bug fixes and not try to make minor
>> improvements for little if any gain.
>>
> Yes
>>
>> .
> 

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[lihuisong (C)]

在 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@huawei.com>
>>>> ---
>>>>   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.
>
>
> .

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[Guenter Roeck]

On 11/25/24 23:03, lihuisong (C) wrote:
> 
> 在 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@huawei.com>
>>>>> ---
>>>>>   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.

This isn't about enforcing anything, it is about reporting an alarm
if the power consumed exceeds the maximum configured.

Guenter

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[lihuisong (C)]

在 2024/11/27 0:19, Guenter Roeck 写道:
> On 11/25/24 23:03, lihuisong (C) wrote:
>>
>> 在 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@huawei.com>
>>>>>> ---
>>>>>>   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.
>
> This isn't about enforcing anything, it is about reporting an alarm
> if the power consumed exceeds the maximum configured.
>
Sorry, I don't quite understand what you mean.
What your mean is to delete the current 'power1_alarm' sysfs and just 
use the related notify event to user?
How should we fix this issue?

/Huisong
>
> .

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[lihuisong (C)]

Hi Guenter，

How about the modification as below? But driver doesn't know what the 
time is to set resource->power_alarm to false.

在 2024/11/27 0:19, Guenter Roeck 写道:
> On 11/25/24 23:03, lihuisong (C) wrote:
>>
>> 在 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@huawei.com>
>>>>>> ---
>>>>>>   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.
>
> This isn't about enforcing anything, it is about reporting an alarm
> if the power consumed exceeds the maximum configured.
>
-->

index 2f1c9d97ad21..b436ebd863e6
--- 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,21 @@ static ssize_t show_val(struct device *dev,
                         val = 0;
                 break;
         case 6:
-               if (resource->power > resource->cap)
-                       val = 1;
-               else
-                       val = 0;
+               /* report alarm status based on the notification if 
support. */
+               if (resource->caps.flags & POWER_METER_CAN_NOTIFY) {
+                       val = resource->power_alarm;
+               } else {
+                       ret = update_meter(resource);
+                       if (ret)
+                               return ret;
+                       ret = update_cap(resource);
+                       if (ret)
+                               return ret;
+                       if (resource->power > resource->cap)
+                               val = 1;
+                       else
+                               val = 0;
+               }
                 break;
         case 7:
         case 8:
@@ -853,6 +868,7 @@ static void acpi_power_meter_notify(struct 
acpi_device *device, u32 event)
                 sysfs_notify(&device->dev.kobj, NULL, 
POWER_AVG_INTERVAL_NAME);
                 break;
         case METER_NOTIFY_CAPPING:
+               resource->power_alarm = true;
                 sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
                 dev_info(&device->dev, "Capping in progress.\n");
                 break;

> .

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[lihuisong (C)]

Hi Guenter,

Can you take a look at my following reply? Looking forward to your reply.

/Huisong


在 2024/11/27 11:43, lihuisong (C) 写道:
> Hi Guenter，
>
> How about the modification as below? But driver doesn't know what the 
> time is to set resource->power_alarm to false.
>
> 在 2024/11/27 0:19, Guenter Roeck 写道:
>> On 11/25/24 23:03, lihuisong (C) wrote:
>>>
>>> 在 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@huawei.com>
>>>>>>> ---
>>>>>>>   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.
>>
>> This isn't about enforcing anything, it is about reporting an alarm
>> if the power consumed exceeds the maximum configured.
>>
> -->
>
> index 2f1c9d97ad21..b436ebd863e6
> --- 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,21 @@ static ssize_t show_val(struct device *dev,
>                         val = 0;
>                 break;
>         case 6:
> -               if (resource->power > resource->cap)
> -                       val = 1;
> -               else
> -                       val = 0;
> +               /* report alarm status based on the notification if 
> support. */
> +               if (resource->caps.flags & POWER_METER_CAN_NOTIFY) {
> +                       val = resource->power_alarm;
> +               } else {
> +                       ret = update_meter(resource);
> +                       if (ret)
> +                               return ret;
> +                       ret = update_cap(resource);
> +                       if (ret)
> +                               return ret;
> +                       if (resource->power > resource->cap)
> +                               val = 1;
> +                       else
> +                               val = 0;
> +               }
>                 break;
>         case 7:
>         case 8:
> @@ -853,6 +868,7 @@ static void acpi_power_meter_notify(struct 
> acpi_device *device, u32 event)
>                 sysfs_notify(&device->dev.kobj, NULL, 
> POWER_AVG_INTERVAL_NAME);
>                 break;
>         case METER_NOTIFY_CAPPING:
> +               resource->power_alarm = true;
>                 sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
>                 dev_info(&device->dev, "Capping in progress.\n");
>                 break;
>
>> .
>
> .

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[Guenter Roeck]

On 11/26/24 19:43, lihuisong (C) wrote:
> Hi Guenter，
> 
> How about the modification as below? But driver doesn't know what the time is to set resource->power_alarm to false.
> 
It's a start, but incomplete because power_alarm must be reset.

See below.

> 在 2024/11/27 0:19, Guenter Roeck 写道:
>> On 11/25/24 23:03, lihuisong (C) wrote:
>>>
>>> 在 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@huawei.com>
>>>>>>> ---
>>>>>>>   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.
>>
>> This isn't about enforcing anything, it is about reporting an alarm
>> if the power consumed exceeds the maximum configured.
>>
> -->
> 
> index 2f1c9d97ad21..b436ebd863e6
> --- 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,21 @@ static ssize_t show_val(struct device *dev,
>                          val = 0;
>                  break;
>          case 6:
> -               if (resource->power > resource->cap)
> -                       val = 1;
> -               else
> -                       val = 0;
> +               /* report alarm status based on the notification if support. */
> +               if (resource->caps.flags & POWER_METER_CAN_NOTIFY) {
> +                       val = resource->power_alarm;
> +               } else {
> +                       ret = update_meter(resource);
> +                       if (ret)
> +                               return ret;
> +                       ret = update_cap(resource);
> +                       if (ret)
> +                               return ret;
> +                       if (resource->power > resource->cap)
> +                               val = 1;
> +                       else
> +                               val = 0;
> +               }

It would have to be something like

		ret = update_meter(resource);
		if (ret)
			return ret;

		val = resource->power_alarm || resource->power > resource->cap;
		/* clear alarm if no longer active */
		resource->power_alarm &= resource->power > resource->cap;

This ensures that alarms are cached if supported, and that cached values are
reported at once. It is far from perfect but the best I can think of since
there is no notification that the alarm is cleared.

Guenter
		
>                  break;
>          case 7:
>          case 8:
> @@ -853,6 +868,7 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>                  sysfs_notify(&device->dev.kobj, NULL, POWER_AVG_INTERVAL_NAME);
>                  break;
>          case METER_NOTIFY_CAPPING:
> +               resource->power_alarm = true;
>                  sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
>                  dev_info(&device->dev, "Capping in progress.\n");
>                  break;
> 
>> .
> 

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[lihuisong (C)]

在 2024/12/12 9:51, Guenter Roeck 写道:
> On 11/26/24 19:43, lihuisong (C) wrote:
>> Hi Guenter，
>>
>> How about the modification as below? But driver doesn't know what the 
>> time is to set resource->power_alarm to false.
>>
> It's a start, but incomplete because power_alarm must be reset.
>
> See below.
>
>> 在 2024/11/27 0:19, Guenter Roeck 写道:
>>> On 11/25/24 23:03, lihuisong (C) wrote:
>>>>
>>>> 在 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@huawei.com>
>>>>>>>> ---
>>>>>>>>   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.
>>>
>>> This isn't about enforcing anything, it is about reporting an alarm
>>> if the power consumed exceeds the maximum configured.
>>>
>> -->
>>
>> index 2f1c9d97ad21..b436ebd863e6
>> --- 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,21 @@ static ssize_t show_val(struct device *dev,
>>                          val = 0;
>>                  break;
>>          case 6:
>> -               if (resource->power > resource->cap)
>> -                       val = 1;
>> -               else
>> -                       val = 0;
>> +               /* report alarm status based on the notification if 
>> support. */
>> +               if (resource->caps.flags & POWER_METER_CAN_NOTIFY) {
>> +                       val = resource->power_alarm;
>> +               } else {
>> +                       ret = update_meter(resource);
>> +                       if (ret)
>> +                               return ret;
>> +                       ret = update_cap(resource);
>> +                       if (ret)
>> +                               return ret;
>> +                       if (resource->power > resource->cap)
>> +                               val = 1;
>> +                       else
>> +                               val = 0;
>> +               }
>
> It would have to be something like
>
>         ret = update_meter(resource);
>         if (ret)
>             return ret;
>
>         val = resource->power_alarm || resource->power > resource->cap;
>         /* clear alarm if no longer active */
>         resource->power_alarm &= resource->power > resource->cap;
>
> This ensures that alarms are cached if supported, and that cached 
> values are
> reported at once. It is far from perfect but the best I can think of 
> since
> there is no notification that the alarm is cleared.
>
Indeed, since there is no notification that the alarm is cleared, driver 
have to compare 'power' and 'cap' to clear it anyway.
If platform support notify to OSPM, driver also need to update 'power' 
to show this alarm status.
In this case, no need to update 'cap' which can be updated by nofity 
0x82 event, right? But this also depands on the initialization of the 
"resource->cap" the probe phase needs to add.
For the platform doesn't support notify, driver have to update 'cap' and 
'power' to show this status, right?

But considering above two cases, directly to update 'power' and 'cap' is 
simple to handle this without more switch case.
what do you think, Guenter?
>
>>                  break;
>>          case 7:
>>          case 8:
>> @@ -853,6 +868,7 @@ static void acpi_power_meter_notify(struct 
>> acpi_device *device, u32 event)
>>                  sysfs_notify(&device->dev.kobj, NULL, 
>> POWER_AVG_INTERVAL_NAME);
>>                  break;
>>          case METER_NOTIFY_CAPPING:
>> +               resource->power_alarm = true;
>>                  sysfs_notify(&device->dev.kobj, NULL, 
>> POWER_ALARM_NAME);
>>                  dev_info(&device->dev, "Capping in progress.\n");
>>                  break;
>>
>>> .
>>
>
>
> .

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[lihuisong (C)]

在 2024/12/12 11:00, lihuisong (C) 写道:
>
> 在 2024/12/12 9:51, Guenter Roeck 写道:
>> On 11/26/24 19:43, lihuisong (C) wrote:
>>> Hi Guenter，
>>>
>>> How about the modification as below? But driver doesn't know what 
>>> the time is to set resource->power_alarm to false.
>>>
>> It's a start, but incomplete because power_alarm must be reset.
>>
>> See below.
>>
>>> 在 2024/11/27 0:19, Guenter Roeck 写道:
>>>> On 11/25/24 23:03, lihuisong (C) wrote:
>>>>>
>>>>> 在 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@huawei.com>
>>>>>>>>> ---
>>>>>>>>>   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.
>>>>
>>>> This isn't about enforcing anything, it is about reporting an alarm
>>>> if the power consumed exceeds the maximum configured.
>>>>
>>> -->
>>>
>>> index 2f1c9d97ad21..b436ebd863e6
>>> --- 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,21 @@ static ssize_t show_val(struct device *dev,
>>>                          val = 0;
>>>                  break;
>>>          case 6:
>>> -               if (resource->power > resource->cap)
>>> -                       val = 1;
>>> -               else
>>> -                       val = 0;
>>> +               /* report alarm status based on the notification if 
>>> support. */
>>> +               if (resource->caps.flags & POWER_METER_CAN_NOTIFY) {
>>> +                       val = resource->power_alarm;
>>> +               } else {
>>> +                       ret = update_meter(resource);
>>> +                       if (ret)
>>> +                               return ret;
>>> +                       ret = update_cap(resource);
>>> +                       if (ret)
>>> +                               return ret;
>>> +                       if (resource->power > resource->cap)
>>> +                               val = 1;
>>> +                       else
>>> +                               val = 0;
>>> +               }
>>
>> It would have to be something like
>>
>>         ret = update_meter(resource);
>>         if (ret)
>>             return ret;
>>
>>         val = resource->power_alarm || resource->power > resource->cap;
>>         /* clear alarm if no longer active */
>>         resource->power_alarm &= resource->power > resource->cap;
>>
>> This ensures that alarms are cached if supported, and that cached 
>> values are
>> reported at once. It is far from perfect but the best I can think of 
>> since
>> there is no notification that the alarm is cleared.
>>
> Indeed, since there is no notification that the alarm is cleared, 
> driver have to compare 'power' and 'cap' to clear it anyway.
> If platform support notify to OSPM, driver also need to update 'power' 
> to show this alarm status.
> In this case, no need to update 'cap' which can be updated by nofity 
> 0x82 event, right? But this also depands on the initialization of the 
> "resource->cap" the probe phase needs to add.
> For the platform doesn't support notify, driver have to update 'cap' 
> and 'power' to show this status, right?
>
> But considering above two cases, directly to update 'power' and 'cap' 
> is simple to handle this without more switch case.
> what do you think, Guenter?

Hi Guenter,

What do you think? Looking forward to your reply.😁

/Huisong Li

>>
>>>                  break;
>>>          case 7:
>>>          case 8:
>>> @@ -853,6 +868,7 @@ static void acpi_power_meter_notify(struct 
>>> acpi_device *device, u32 event)
>>>                  sysfs_notify(&device->dev.kobj, NULL, 
>>> POWER_AVG_INTERVAL_NAME);
>>>                  break;
>>>          case METER_NOTIFY_CAPPING:
>>> +               resource->power_alarm = true;
>>>                  sysfs_notify(&device->dev.kobj, NULL, 
>>> POWER_ALARM_NAME);
>>>                  dev_info(&device->dev, "Capping in progress.\n");
>>>                  break;
>>>
>>>> .
>>>
>>
>>
>> .
>
> .

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[Guenter Roeck]

On 12/18/24 19:45, lihuisong (C) wrote:
> 
> 在 2024/12/12 11:00, lihuisong (C) 写道:
>>
>> 在 2024/12/12 9:51, Guenter Roeck 写道:
>>> On 11/26/24 19:43, lihuisong (C) wrote:
>>>> Hi Guenter，
>>>>
>>>> How about the modification as below? But driver doesn't know what the time is to set resource->power_alarm to false.
>>>>
>>> It's a start, but incomplete because power_alarm must be reset.
>>>
>>> See below.
>>>
>>>> 在 2024/11/27 0:19, Guenter Roeck 写道:
>>>>> On 11/25/24 23:03, lihuisong (C) wrote:
>>>>>>
>>>>>> 在 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@huawei.com>
>>>>>>>>>> ---
>>>>>>>>>>   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.
>>>>>
>>>>> This isn't about enforcing anything, it is about reporting an alarm
>>>>> if the power consumed exceeds the maximum configured.
>>>>>
>>>> -->
>>>>
>>>> index 2f1c9d97ad21..b436ebd863e6
>>>> --- 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,21 @@ static ssize_t show_val(struct device *dev,
>>>>                          val = 0;
>>>>                  break;
>>>>          case 6:
>>>> -               if (resource->power > resource->cap)
>>>> -                       val = 1;
>>>> -               else
>>>> -                       val = 0;
>>>> +               /* report alarm status based on the notification if support. */
>>>> +               if (resource->caps.flags & POWER_METER_CAN_NOTIFY) {
>>>> +                       val = resource->power_alarm;
>>>> +               } else {
>>>> +                       ret = update_meter(resource);
>>>> +                       if (ret)
>>>> +                               return ret;
>>>> +                       ret = update_cap(resource);
>>>> +                       if (ret)
>>>> +                               return ret;
>>>> +                       if (resource->power > resource->cap)
>>>> +                               val = 1;
>>>> +                       else
>>>> +                               val = 0;
>>>> +               }
>>>
>>> It would have to be something like
>>>
>>>         ret = update_meter(resource);
>>>         if (ret)
>>>             return ret;
>>>
>>>         val = resource->power_alarm || resource->power > resource->cap;
>>>         /* clear alarm if no longer active */
>>>         resource->power_alarm &= resource->power > resource->cap;
>>>
>>> This ensures that alarms are cached if supported, and that cached values are
>>> reported at once. It is far from perfect but the best I can think of since
>>> there is no notification that the alarm is cleared.
>>>
>> Indeed, since there is no notification that the alarm is cleared, driver have to compare 'power' and 'cap' to clear it anyway.
>> If platform support notify to OSPM, driver also need to update 'power' to show this alarm status.
>> In this case, no need to update 'cap' which can be updated by nofity 0x82 event, right? But this also depands on the initialization of the "resource->cap" the probe phase needs to add.
>> For the platform doesn't support notify, driver have to update 'cap' and 'power' to show this status, right?
>>
>> But considering above two cases, directly to update 'power' and 'cap' is simple to handle this without more switch case.
>> what do you think, Guenter?
> 
> Hi Guenter,
> 
> What do you think? Looking forward to your reply.😁
> 

This is getting too complicated for a reply after a casual glance at the driver,
and I don't currently have time for a deeper look into the driver, sorry.

Guenter

> /Huisong Li
> 
>>>
>>>>                  break;
>>>>          case 7:
>>>>          case 8:
>>>> @@ -853,6 +868,7 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>                  sysfs_notify(&device->dev.kobj, NULL, POWER_AVG_INTERVAL_NAME);
>>>>                  break;
>>>>          case METER_NOTIFY_CAPPING:
>>>> +               resource->power_alarm = true;
>>>>                  sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
>>>>                  dev_info(&device->dev, "Capping in progress.\n");
>>>>                  break;
>>>>
>>>>> .
>>>>
>>>
>>>
>>> .
>>
>> .

Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized variables

[lihuisong (C)]

在 2024/12/19 11:50, Guenter Roeck 写道:
> On 12/18/24 19:45, lihuisong (C) wrote:
>>
>> 在 2024/12/12 11:00, lihuisong (C) 写道:
>>>
>>> 在 2024/12/12 9:51, Guenter Roeck 写道:
>>>> On 11/26/24 19:43, lihuisong (C) wrote:
>>>>> Hi Guenter，
>>>>>
>>>>> How about the modification as below? But driver doesn't know what 
>>>>> the time is to set resource->power_alarm to false.
>>>>>
>>>> It's a start, but incomplete because power_alarm must be reset.
>>>>
>>>> See below.
>>>>
>>>>> 在 2024/11/27 0:19, Guenter Roeck 写道:
>>>>>> On 11/25/24 23:03, lihuisong (C) wrote:
>>>>>>>
>>>>>>> 在 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@huawei.com>
>>>>>>>>>>> ---
>>>>>>>>>>>   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.
>>>>>>
>>>>>> This isn't about enforcing anything, it is about reporting an alarm
>>>>>> if the power consumed exceeds the maximum configured.
>>>>>>
>>>>> -->
>>>>>
>>>>> index 2f1c9d97ad21..b436ebd863e6
>>>>> --- 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,21 @@ static ssize_t show_val(struct device *dev,
>>>>>                          val = 0;
>>>>>                  break;
>>>>>          case 6:
>>>>> -               if (resource->power > resource->cap)
>>>>> -                       val = 1;
>>>>> -               else
>>>>> -                       val = 0;
>>>>> +               /* report alarm status based on the notification 
>>>>> if support. */
>>>>> +               if (resource->caps.flags & POWER_METER_CAN_NOTIFY) {
>>>>> +                       val = resource->power_alarm;
>>>>> +               } else {
>>>>> +                       ret = update_meter(resource);
>>>>> +                       if (ret)
>>>>> +                               return ret;
>>>>> +                       ret = update_cap(resource);
>>>>> +                       if (ret)
>>>>> +                               return ret;
>>>>> +                       if (resource->power > resource->cap)
>>>>> +                               val = 1;
>>>>> +                       else
>>>>> +                               val = 0;
>>>>> +               }
>>>>
>>>> It would have to be something like
>>>>
>>>>         ret = update_meter(resource);
>>>>         if (ret)
>>>>             return ret;
>>>>
>>>>         val = resource->power_alarm || resource->power > 
>>>> resource->cap;
>>>>         /* clear alarm if no longer active */
>>>>         resource->power_alarm &= resource->power > resource->cap;
>>>>
>>>> This ensures that alarms are cached if supported, and that cached 
>>>> values are
>>>> reported at once. It is far from perfect but the best I can think 
>>>> of since
>>>> there is no notification that the alarm is cleared.
>>>>
>>> Indeed, since there is no notification that the alarm is cleared, 
>>> driver have to compare 'power' and 'cap' to clear it anyway.
>>> If platform support notify to OSPM, driver also need to update 
>>> 'power' to show this alarm status.
>>> In this case, no need to update 'cap' which can be updated by nofity 
>>> 0x82 event, right? But this also depands on the initialization of 
>>> the "resource->cap" the probe phase needs to add.
>>> For the platform doesn't support notify, driver have to update 'cap' 
>>> and 'power' to show this status, right?
>>>
>>> But considering above two cases, directly to update 'power' and 
>>> 'cap' is simple to handle this without more switch case.
>>> what do you think, Guenter?
>>
>> Hi Guenter,
>>
>> What do you think? Looking forward to your reply.😁
>>
>
> This is getting too complicated for a reply after a casual glance at 
> the driver,
> and I don't currently have time for a deeper look into the driver, sorry.
All right. Thanks for your review and advice. We want to make it more 
useful.
I will send out v2 first based on our discussion and my understanding.
>
> Guenter
>
>> /Huisong Li
>>
>>>>
>>>>>                  break;
>>>>>          case 7:
>>>>>          case 8:
>>>>> @@ -853,6 +868,7 @@ static void acpi_power_meter_notify(struct 
>>>>> acpi_device *device, u32 event)
>>>>>                  sysfs_notify(&device->dev.kobj, NULL, 
>>>>> POWER_AVG_INTERVAL_NAME);
>>>>>                  break;
>>>>>          case METER_NOTIFY_CAPPING:
>>>>> +               resource->power_alarm = true;
>>>>>                  sysfs_notify(&device->dev.kobj, NULL, 
>>>>> POWER_ALARM_NAME);
>>>>>                  dev_info(&device->dev, "Capping in progress.\n");
>>>>>                  break;
>>>>>
>>>>>> .
>>>>>
>>>>
>>>>
>>>> .
>>>
>>> .
>
> .