Re: [PATCH v5 1/3] hwmon: pmbus: event notification with alarms

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Using hwmon_notify_event() for PMBus temperature alarms silently fails to notify the thermal subsystem due to an architectural mismatch.

Pre-existing issues:
- [High] pmbus_notify() calls sysfs_notify() on the parent I2C device's kobject instead of the hwmon device, breaking userspace poll() on alarm attributes.
--

--- Patch [1]: [PATCH v5 1/3] hwmon: pmbus: event notification with alarms ---
commit 3c1ad67c1176e741304b7aff4d177996b81ebe1a
Author: Vincent Jardin <[email protected]>

hwmon: pmbus: event notification with alarms

This patch adds an accessor for the hwmon class device registered by the
pmbus core. It is exported so chip drivers can use it to call
hwmon_notify_event() for alarm attributes in polling fallback routines.

> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 3143b9e0316c4..6ba00ad732973 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -695,6 +695,14 @@ const struct pmbus_driver_info *pmbus_get_driver_info(struct i2c_client *client)
>  }
>  EXPORT_SYMBOL_NS_GPL(pmbus_get_driver_info, "PMBUS");
>  
> +struct device *pmbus_get_hwmon_device(struct i2c_client *client)
> +{
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +
> +	return data->hwmon_dev;
> +}
> +EXPORT_SYMBOL_NS_GPL(pmbus_get_hwmon_device, "PMBUS");

[Severity: High]
Does using hwmon_notify_event() with this device successfully notify the
thermal subsystem?

Looking at how hwmon_notify_event() works, it relies on iterating over the
hwdev->tzdata list:

drivers/hwmon/hwmon.c:hwmon_thermal_notify() {
    ...
    list_for_each_entry(tzdata, &hwdev->tzdata, node) {
        if (tzdata->index == index) {
            thermal_zone_device_update(tzdata->tzd, THERMAL_EVENT_UNSPECIFIED);
        }
    }
    ...
}

Because pmbus_core.c registers the hwmon device using
devm_hwmon_device_register_with_groups() rather than
devm_hwmon_device_register_with_info(), and manually registers thermal zones
via devm_thermal_of_zone_register(), will hwdev->tzdata be empty?

If hwdev->tzdata is unpopulated, will thermal alarms signaled via this new
polling fallback silently fail to trigger the thermal subsystem?


[Severity: High]
This is a pre-existing issue, but does pmbus_notify() currently notify the wrong
kobject when SMBALERT# interrupts do fire?

Looking at pmbus_notify() in drivers/hwmon/pmbus/pmbus_core.c:

drivers/hwmon/pmbus/pmbus_core.c:pmbus_notify() {
    ...
    sysfs_notify(&data->dev->kobj, NULL, da->attr.name);
    kobject_uevent(&data->dev->kobj, KOBJ_CHANGE);
    ...
}

Since devm_hwmon_device_register_with_groups() registers the attribute
groups on the newly created hwmon child device (data->hwmon_dev) and not the
parent I2C device (data->dev), and sysfs_notify() does not recurse into
child directories, will userspace applications relying on poll(POLLPRI) for
these alarm attributes fail to wake up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1