Re: [PATCH v2] hwmon: (lm90) Switch channel parsing to fwnode APIs

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Incompatible pointer type causing build failure

Pre-existing issues:
- [Critical] Sysfs read triggers NULL pointer dereference via uninitialized hwmon_dev
--

--- Patch [1]: [PATCH v2] hwmon: (lm90) Switch channel parsing to fwnode APIs ---
commit 3d5d792e5529b3545ccefc21f7dfd9c2acecc999
Author: Flaviu Nistor <flaviu.nistor@gmail.com>

hwmon: (lm90) Switch channel parsing to fwnode APIs
    
This commit replaces OF property handling with fwnode APIs in the probe
function to read channel properties. This improves compatibility by no longer
limiting it to Device Tree.

Link: https://lore.kernel.org/all/20260713190659.4511-1-flaviu.nistor@gmail.com/

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 4b9c0ccdf2609..7d4b8e61bec51 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
[ ... ]
> @@ -2701,27 +2701,27 @@ static int lm90_probe_channel_from_dt(struct i2c_client *client,
[ ... ]
> -	err = of_property_read_s32(child, "temperature-offset-millicelsius", &val);
> +	err = fwnode_property_read_u32(child, "temperature-offset-millicelsius", &val);

[Severity: Low]
Will this cause a build failure or compiler warning?

In lm90_probe_channel(), val is declared as an s32, but it is being passed
to fwnode_property_read_u32() which expects a u32 pointer. This pointer type
mismatch can trigger a -Wincompatible-pointer-types error in modern builds.

[ ... ]
> @@ -2887,12 +2886,10 @@ static int lm90_probe(struct i2c_client *client)
>  	/* Set maximum conversion rate */
>  	data->max_convrate = lm90_params[data->kind].max_convrate;
>  
> -	/* Parse device-tree channel information */
> -	if (client->dev.of_node) {
> -		err = lm90_parse_dt_channel_info(client, data);
> -		if (err)
> -			return err;
> -	}
> +	/* Parse channel information */
> +	err = lm90_parse_channel_info(client, data);
> +	if (err)
> +		return err;
>  
>  	/* Initialize the LM90 chip */
>  	err = lm90_init_client(client, data);

[Severity: Critical]
This is a pre-existing issue, but does this create a race condition that
results in a NULL pointer dereference?

In lm90_probe(), the sysfs files are exposed when calling
devm_hwmon_device_register_with_info() later in the function:

	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
							 data, &data->chip,
							 NULL);
	if (IS_ERR(hwmon_dev))
		return PTR_ERR(hwmon_dev);

	data->hwmon_dev = hwmon_dev;

If an unprivileged userspace process reads a sysfs attribute before
data->hwmon_dev is initialized, it triggers lm90_temp_read(), which schedules
data->report_work.

The asynchronous work runs lm90_report_alarms():

	struct device *hwmon_dev = data->hwmon_dev;
	...
	if (st & LM90_STATUS_LLOW)
		hwmon_notify_event(hwmon_dev, hwmon_temp,
				   hwmon_temp_min_alarm, 0);

This would pass the still-NULL hwmon_dev to hwmon_notify_event(), causing a
kernel panic. Should data->hwmon_dev be initialized before the device is
registered?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717063730.5617-1-flaviu.nistor@gmail.com?part=1