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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1e7e5737-8dd8-172f-f5f7-9cc7967e129e@huawei.com>
Date: Tue, 26 Nov 2024 11:15:56 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: Guenter Roeck <linux@...ck-us.net>, <linux-hwmon@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
CC: <jdelvare@...e.com>, <liuyonglong@...wei.com>, <zhanjie9@...ilicon.com>,
	<zhenglifeng1@...wei.com>
Subject: Re: [PATCH v1 4/4] hwmon: (acpi_power_meter) Add the print of no
 notification that hardware limit is enforced


在 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@...wei.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
>
> .