Re: [PATCH] hwmon: emc1812: reject channel index 0 in device tree config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Rejecting channel index 0 (`reg = 0`) breaks Devicetree ABI backward compatibility and prevents probing of valid, existing device trees.
- [Low] When reading the `reg` property fails, the error message logs an unread, stale `reg_nr` value, misdirecting debugging efforts.
--

--- Patch [1]: [PATCH] hwmon: emc1812: reject channel index 0 in device tree config ---
commit f33e5fdce14729c5f30dbfaad69487c1d7f783de
Author: Ivy Lopez <skunkolee@gmail.com>

hwmon: emc1812: reject channel index 0 in device tree config

This commit explicitly rejects channel index 0 in child nodes to prevent
the internal diode label from being overwritten. It also updates the error
message to include the invalid index.

> diff --git a/drivers/hwmon/emc1812.c b/drivers/hwmon/emc1812.c
> --- a/drivers/hwmon/emc1812.c
> +++ b/drivers/hwmon/emc1812.c
> @@ -809,9 +809,10 @@ static int emc1812_parse_fw_config(struct emc1812_data *data, struct device *dev
>  
>  	device_for_each_child_node_scoped(dev, child) {
>  		ret = fwnode_property_read_u32(child, "reg", &reg_nr);
> -		if (ret || reg_nr >= data->chip->phys_channels)
> +		if (ret || reg_nr == 0 || reg_nr >= data->chip->phys_channels)

[Severity: Critical]
Does adding reg_nr == 0 break Devicetree ABI backward compatibility?

If there are existing device trees that specify a child node for the internal
sensor (channel@0), emc1812_parse_fw_config() will now return -EINVAL during
probe and fail to load the driver.

>  			return dev_err_probe(dev, -EINVAL,
> -					     "The index is higher then the chip supports\n");
> +					"Invalid channel index %u\n",
> +					reg_nr);

[Severity: Low]
If fwnode_property_read_u32() fails and returns a non-zero error code 
(ret != 0), is it safe to print reg_nr?

If the child node is missing the reg property, fwnode_property_read_u32() 
leaves reg_nr unmodified. The error message would then log a stale reg_nr 
value from the previous iteration of the loop, which might misdirect 
debugging efforts.

>  		/* Mark channel as active */
>  		set_bit(reg_nr, &data->active_ch_mask);
>

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