| Message ID | 20241125093415.21719-5-lihuisong@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (acpi_power_meter) Some trival optimizations | expand |
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
在 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?
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?
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
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(+)