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: <9d835642-0059-44aa-93ed-8b8f2a636b9b@roeck-us.net>
Date: Mon, 25 Nov 2024 20:06:12 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: "lihuisong (C)" <lihuisong@...wei.com>, 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

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@...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?😁

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