Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel,vm-map" not defined

[Guenter Roeck]

On 8/30/22 12:21, Eliav Farber wrote:
> Bug fix - in case "intel,vm-map" is missing in device-tree ,'num' is set
> to 0, and no voltage channel infos are allocated.
>
> Signed-off-by: Eliav Farber <farbere@xxxxxxxxxx>
> ---
>   drivers/hwmon/mr75203.c | 28 ++++++++++++----------------
>   1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index 046523d47c29..0e29877a1a9c 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -580,8 +580,6 @@ static int mr75203_probe(struct platform_device *pdev)
>   	}
>   
>   	if (vm_num) {
> -		u32 num = vm_num;
> -
>   		ret = pvt_get_regmap(pdev, "vm", pvt);
>   		if (ret)
>   			return ret;
> @@ -594,30 +592,28 @@ static int mr75203_probe(struct platform_device *pdev)
>   		ret = device_property_read_u8_array(dev, "intel,vm-map",
>   						    pvt->vm_idx, vm_num);
>   		if (ret) {
> -			num = 0;
> +			/*
> +			 * Incase intel,vm-map property is not defined, we
> +			 * assume incremental channel numbers.
> +			 */
> +			for (i = 0; i < vm_num; i++)
> +				pvt->vm_idx[i] = i;
>   		} else {
>   			for (i = 0; i < vm_num; i++)
>   				if (pvt->vm_idx[i] >= vm_num ||
> -				    pvt->vm_idx[i] == 0xff) {
> -					num = i;
> +				    pvt->vm_idx[i] == 0xff)
>   					break;
> -				}
> -		}
>   
> -		/*
> -		 * Incase intel,vm-map property is not defined, we assume
> -		 * incremental channel numbers.
> -		 */
> -		for (i = num; i < vm_num; i++)
> -			pvt->vm_idx[i] = i;
> +			vm_num = i;
> +		}
>   

So this is actually a functional change: In the old code, unspecified channel
numbers (num ... vm_num) were filled with incremental channel numbers.
This is no longer the case.

> -		in_config = devm_kcalloc(dev, num + 1,
> +		in_config = devm_kcalloc(dev, vm_num + 1,
>   					 sizeof(*in_config), GFP_KERNEL);

The relevant difference (and maybe bug in the existing code ?) seems to be
this change. Have you considered leaving everything else in place and only
changing this code as well as the num -> vm_num changes below ?

Thanks,
Guenter

>   		if (!in_config)
>   			return -ENOMEM;
>   
> -		memset32(in_config, HWMON_I_INPUT, num);
> -		in_config[num] = 0;
> +		memset32(in_config, HWMON_I_INPUT, vm_num);
> +		in_config[vm_num] = 0;
>   		pvt_in.config = in_config;
>   
>   		pvt_info[index++] = &pvt_in;